Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Backroll Plugin #8251

Open
wants to merge 69 commits into
base: main
Choose a base branch
from
Open

Backroll Plugin #8251

wants to merge 69 commits into from

Conversation

PeterBackroll
Copy link

@PeterBackroll PeterBackroll commented Nov 20, 2023 •
edited by soreana
Loading

Description

This is the pull request for Backroll Plugin (a backup & restore plugin).
This plugin allows users to use Backroll as backup provider inside Cloudstack.

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)
  • build/CI

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

@boring-cyborg boring-cyborg
Copy link

boring-cyborg bot commented Nov 20, 2023

Congratulations on your first Pull Request and welcome to the Apache CloudStack community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md)
Here are some useful points:

@weizhouapache
Copy link
Member

@blueorangutan package

@blueorangutan
Copy link

@weizhouapache a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@codecov Codecov
Copy link

codecov bot commented Nov 20, 2023 •
edited
Loading

Codecov Report

Attention: Patch coverage is 67.73333% with 242 lines in your changes missing coverage. Please review.

Project coverage is 17.02%. Comparing base (fadb39e) to head (010728e).

Files with missing lines Patch % Lines
...ache/cloudstack/backup/BackrollBackupProvider.java 62.96% 80 Missing and 20 partials ⚠️
...kup/backroll/utils/BackrollHttpClientProvider.java 66.80% 69 Missing and 12 partials ⚠️
...dstack/backup/backroll/model/BackrollOffering.java 31.25% 21 Missing and 1 partial ⚠️
...che/cloudstack/backup/backroll/BackrollClient.java 86.29% 6 Missing and 11 partials ⚠️
...oudstack/backup/backroll/model/BackrollBackup.java 0.00% 7 Missing ⚠️
...tack/backup/backroll/model/BackrollTaskStatus.java 68.42% 6 Missing ⚠️
...k/backup/backroll/model/BackrollBackupMetrics.java 90.90% 0 Missing and 1 partial ⚠️
...dstack/backup/backroll/model/BackrollVmBackup.java 93.33% 0 Missing and 1 partial ⚠️
...tack/backup/backroll/model/response/TaskState.java 0.00% 1 Missing ⚠️
...kup/backroll/model/response/TaskStateResponse.java 0.00% 1 Missing ⚠️
... and 5 more
Additional details and impacted files 
@@             Coverage Diff              @@
##               main    #8251      +/-   ##
============================================
+ Coverage     16.09%   17.02%   +0.93%     
- Complexity    12934    13003      +69     
============================================
  Files          5644     5286     -358     
  Lines        494582   462926   -31656     
  Branches      59963    54295    -5668     
============================================
- Hits          79622    78835     -787     
+ Misses       406124   375355   -30769     
+ Partials       8836     8736     -100
Flag Coverage Δ
uitests ?
unittests 17.02% <67.73%> (+0.08%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@blueorangutan
Copy link

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 7806

@DaanHoogland
Copy link
Contributor

@PeterBackroll , nice to see this coming in. Will you add a documentation PR as well? I think you'll have a lot of reviews ;)

@DaanHoogland
Copy link
Contributor

@blueorangutan test

@blueorangutan
Copy link

@DaanHoogland a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

@blueorangutan
Copy link

[SF] Trillian test result (tid-8383)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 45945 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8251-t8383-kvm-centos7.zip
Smoke tests completed. 117 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test Result Time (s) Test File
test_08_upgrade_kubernetes_ha_cluster Failure 672.30 test_kubernetes_clusters.py

@PeterBackroll
Copy link
Author

Hi @DaanHoogland Yes I will add a documentation PR as soon as possible :)

@DaanHoogland DaanHoogland added this to the 4.20.0.0 milestone Nov 23, 2023
DaanHoogland
DaanHoogland reviewed Nov 23, 2023
View reviewed changes
Copy link
Contributor

@DaanHoogland DaanHoogland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some comments @PeterBackroll .
I haven't gotten to review the Provider and Client yet, the core of the plugin.
I'll do that next week ;)

.../backup/backroll/src/test/java/org/apache/cloudstack/backup/backroll/BackrollClientTest.java Outdated

public class BackrollClientTest {

}
Copy link
Contributor

@DaanHoogland DaanHoogland Nov 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
}
}

...s/backup/backroll/src/test/java/org/apache/cloudstack/backup/BackrollBackupProviderTest.java Outdated
assertEquals("Backroll Backup Plugin", "Backroll Backup Plugin");

}*/
}
Copy link
Contributor

@DaanHoogland DaanHoogland Nov 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
}
}

...java/org/apache/cloudstack/backup/backroll/model/response/policy/BackupPoliciesResponse.java Show resolved Hide resolved
...rg/apache/cloudstack/backup/backroll/model/response/policy/BackrollBackupPolicyResponse.java Show resolved Hide resolved
...kup/backroll/model/response/metrics/virtualMachineBackups/VirtualMachineBackupsResponse.java Show resolved Hide resolved
...p/backroll/src/main/java/org/apache/cloudstack/backup/backroll/model/BackrollTaskStatus.java Show resolved Hide resolved
...kup/backroll/src/main/java/org/apache/cloudstack/backup/backroll/model/BackrollOffering.java Show resolved Hide resolved
...ackroll/src/main/java/org/apache/cloudstack/backup/backroll/model/BackrollBackupMetrics.java Show resolved Hide resolved
...ackup/backroll/src/main/java/org/apache/cloudstack/backup/backroll/model/BackrollBackup.java Show resolved Hide resolved
plugins/backup/backroll/src/main/java/org/apache/cloudstack/backup/backroll/BackrollClient.java Show resolved Hide resolved
@rohityadavcloud
Copy link
Member

@blueorangutan package

@blueorangutan
Copy link

@rohityadavcloud a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@blueorangutan
Copy link

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 7834

@shwstppr
Copy link
Contributor

@PeterBackroll can you please have a look at the GH actions failures

DaanHoogland
DaanHoogland reviewed Nov 28, 2023
View reviewed changes
Copy link
Contributor

@DaanHoogland DaanHoogland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some comments @PeterBackroll .
I haven't gotten to review the Provider and Client yet, the core of the plugin.
Thatś up next ;)

...ckroll/src/main/java/org/apache/cloudstack/backup/backroll/api/command/GetBackrollHello.java Outdated Show resolved Hide resolved
...ckroll/src/main/java/org/apache/cloudstack/backup/backroll/api/command/GetBackrollHello.java Outdated Show resolved Hide resolved
...kroll/src/main/java/org/apache/cloudstack/backup/backroll/api/response/ApiHelloResponse.java Outdated Show resolved Hide resolved
DaanHoogland
DaanHoogland reviewed Dec 1, 2023
View reviewed changes
Copy link
Contributor

@DaanHoogland DaanHoogland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks generally good, @PeterBackroll .
some changes will have to be done though (before merging)

plugins/backup/backroll/src/main/java/org/apache/cloudstack/backup/BackrollBackupProvider.java Outdated Show resolved Hide resolved
plugins/backup/backroll/src/main/java/org/apache/cloudstack/backup/BackrollBackupProvider.java Show resolved Hide resolved
plugins/backup/backroll/src/main/java/org/apache/cloudstack/backup/BackrollBackupProvider.java

public ConfigKey<String> BackrollPasswordConfigKey = new ConfigKey<>("Advanced", String.class,
"backup.plugin.backroll.config.password",
"VviX8dALauSyYJMqVYJqf3UyZOpO3joS",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we make this more clear, this string is just as insecure but less inviting to change

Suggested change
"VviX8dALauSyYJMqVYJqf3UyZOpO3joS",
"password",

or

Suggested change
"VviX8dALauSyYJMqVYJqf3UyZOpO3joS",
"C'est vraiment secret",

plugins/backup/backroll/src/main/java/org/apache/cloudstack/backup/BackrollBackupProvider.java Outdated Show resolved Hide resolved
plugins/backup/backroll/src/main/java/org/apache/cloudstack/backup/BackrollBackupProvider.java
}

@Override
public void syncBackups(VirtualMachine vm, Backup.Metric metric) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe disect this method. it is rather big.

plugins/backup/backroll/src/main/java/org/apache/cloudstack/backup/BackrollBackupProvider.java Outdated Show resolved Hide resolved
plugins/backup/backroll/src/main/java/org/apache/cloudstack/backup/BackrollBackupProvider.java Outdated Show resolved Hide resolved
plugins/backup/backroll/src/main/java/org/apache/cloudstack/backup/BackrollBackupProvider.java Outdated Show resolved Hide resolved
plugins/backup/backroll/src/main/java/org/apache/cloudstack/backup/BackrollBackupProvider.java
public boolean deleteBackup(Backup backup, boolean forced) {
s_logger.info("backroll delete backup id: " + backup.getExternalId());
if(backup.getStatus().equals(Backup.Status.BackingUp)) {
throw new CloudRuntimeException("You can't delete a backup while it still BackingUp");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
throw new CloudRuntimeException("You can't delete a backup while it still BackingUp");
throw new CloudRuntimeException("You can't delete a backup while it's still BackingUp");

plugins/backup/backroll/src/main/java/org/apache/cloudstack/backup/backroll/BackrollClient.java Outdated Show resolved Hide resolved
@andrijapanicsb
Copy link
Contributor

@PeterBackroll do we have any docs on how to configure the plugin, what kind of storage is supported, any limitations, etc?
Thx!

@rohityadavcloud rohityadavcloud changed the base branch from main to 4.19 February 5, 2024 09:04
DaanHoogland
DaanHoogland reviewed Dec 23, 2024
View reviewed changes
[email protected] Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this file be here?

JoaoJandre
JoaoJandre reviewed Dec 23, 2024
View reviewed changes
ui/package-lock.json Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this file be commited? cc @DaanHoogland

ui/package.json Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change necessary? cc @DaanHoogland

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea. @PeterBackroll or colleagues, can you answer that?

btw, if this change is needed, the ui/package-lock.json is probably needed as well (is my best guess), otherwise for sure no.

@rajujith
Copy link
Collaborator

@blueorangutan package

@blueorangutan
Copy link

@rajujith a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@blueorangutan
Copy link

Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 11943

@DaanHoogland
Copy link
Contributor

@PeterBackroll , can you look at unused imports? the build fails on those.

@ous16
Copy link

ous16 commented Jan 10, 2025

@blueorangutan package

@blueorangutan
Copy link

@ous16 a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@blueorangutan
Copy link

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 12045

DaanHoogland
DaanHoogland reviewed Jan 10, 2025
View reviewed changes
[email protected] Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should not be here

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed this change

@ous16
Copy link

ous16 commented Jan 10, 2025

@blueorangutan package

@blueorangutan
Copy link

@ous16 a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@blueorangutan
Copy link

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 12052

@rajujith
Copy link
Collaborator

@blueorangutan package

@blueorangutan
Copy link

@rajujith a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@blueorangutan
Copy link

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 12068

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ous16 ous16 ous16 left review comments

@m-dhellin m-dhellin m-dhellin left review comments

@JoaoJandre JoaoJandre JoaoJandre requested changes

@rajujith rajujith rajujith left review comments

@DaanHoogland DaanHoogland DaanHoogland left review comments

@rohityadavcloud rohityadavcloud rohityadavcloud approved these changes

@shwstppr shwstppr Awaiting requested review from shwstppr

@soreana soreana Awaiting requested review from soreana

@GutoVeronezi GutoVeronezi Awaiting requested review from GutoVeronezi

@BryanMLima BryanMLima Awaiting requested review from BryanMLima

@harikrishna-patnala harikrishna-patnala Awaiting requested review from harikrishna-patnala

@sureshanaparti sureshanaparti Awaiting requested review from sureshanaparti

Assignees
No one assigned
Labels
component:backup component:build
Projects
None yet
Milestone
4.21.0
Development

Successfully merging this pull request may close these issues.