-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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.
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
Backroll Plugin #8251
base: main
Are you sure you want to change the base?
Backroll Plugin #8251
Conversation
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)
|
@blueorangutan package |
@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 ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8251 +/- ##
============================================
+ Coverage 16.15% 16.35% +0.19%
- Complexity 13265 13466 +201
============================================
Files 5666 5710 +44
Lines 497960 499744 +1784
Branches 60241 60401 +160
============================================
+ Hits 80469 81734 +1265
- Misses 408482 408919 +437
- Partials 9009 9091 +82
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 7806 |
@PeterBackroll , nice to see this coming in. Will you add a documentation PR as well? I think you'll have a lot of reviews ;) |
@blueorangutan test |
@DaanHoogland a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
[SF] Trillian test result (tid-8383)
|
Hi @DaanHoogland Yes I will add a documentation PR as soon as possible :) |
There was a problem hiding this 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 ;)
|
||
public class BackrollClientTest { | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | |
} | |
assertEquals("Backroll Backup Plugin", "Backroll Backup Plugin"); | ||
|
||
}*/ | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | |
} | |
...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
@blueorangutan package |
@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. |
Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 7834 |
@PeterBackroll can you please have a look at the GH actions failures |
There was a problem hiding this 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
There was a problem hiding this 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
|
||
public ConfigKey<String> BackrollPasswordConfigKey = new ConfigKey<>("Advanced", String.class, | ||
"backup.plugin.backroll.config.password", | ||
"VviX8dALauSyYJMqVYJqf3UyZOpO3joS", |
There was a problem hiding this comment.
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
"VviX8dALauSyYJMqVYJqf3UyZOpO3joS", | |
"password", |
or
"VviX8dALauSyYJMqVYJqf3UyZOpO3joS", | |
"C'est vraiment secret", |
plugins/backup/backroll/src/main/java/org/apache/cloudstack/backup/BackrollBackupProvider.java
Outdated
Show resolved
Hide resolved
} | ||
|
||
@Override | ||
public void syncBackups(VirtualMachine vm, Backup.Metric metric) { |
There was a problem hiding this comment.
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
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"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@PeterBackroll do we have any docs on how to configure the plugin, what kind of storage is supported, any limitations, etc? |
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 12045 |
root@169.254.109.99
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should not be here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed this change
@blueorangutan package |
@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. |
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 12052 |
@blueorangutan package |
@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. |
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 12068 |
@blueorangutan package |
@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. |
Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 12441 |
@blueorangutan package |
@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. |
Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 12555 |
Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 12557 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you please do these suggested changes here if it can fix the build failures. Also please verify if those changes will not break your changes too.
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
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
Outdated
Show resolved
Hide resolved
...s/backup/backroll/src/test/java/org/apache/cloudstack/backup/BackrollBackupProviderTest.java
Outdated
Show resolved
Hide resolved
// under the License. | ||
package org.apache.cloudstack.backup.backroll.model; | ||
|
||
public class BackrollBackupMetrics { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we use Backup.Metric like all other backup providers?
In Backup.Metric we have two metrics: backupSize (backup size after storage efficiency) and dataSize (total size/ virtual size)
Adding new Metric class with different nomenclature might get confusing while reading code.
@blueorangutan package |
@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. |
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 12764 |
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
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity