Fixes issue with loading Capacity dashboard when mulitple backup providers configured#12550
Fixes issue with loading Capacity dashboard when mulitple backup providers configured#12550
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 4.20 #12550 +/- ##
============================================
+ Coverage 16.26% 16.37% +0.10%
- Complexity 13428 13467 +39
============================================
Files 5660 5661 +1
Lines 499907 502387 +2480
Branches 60696 61802 +1106
============================================
+ Hits 81316 82264 +948
- Misses 409521 410996 +1475
- Partials 9070 9127 +57
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:
|
|
@blueorangutan package |
|
@Pearl1594 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. |
|
@abh1sar @DaanHoogland I've added this bit : 94ac3ea to add a validator that can be used for any configuration. Do you think this is useful? |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 16636 |
|
@blueorangutan package |
|
@Pearl1594 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 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 16639 |
api/src/main/java/org/apache/cloudstack/backup/BackupManager.java
Outdated
Show resolved
Hide resolved
api/src/main/java/org/apache/cloudstack/backup/BackupManager.java
Outdated
Show resolved
Hide resolved
api/src/main/java/org/apache/cloudstack/backup/BackupManager.java
Outdated
Show resolved
Hide resolved
api/src/main/java/org/apache/cloudstack/backup/BackupManager.java
Outdated
Show resolved
Hide resolved
framework/config/src/main/java/org/apache/cloudstack/framework/config/ValidatedConfigKey.java
Outdated
Show resolved
Hide resolved
abh1sar
left a comment
There was a problem hiding this comment.
Code LGTM.
just a few nitpicks.
| "dummy", | ||
| "The backup and recovery provider plugin.", true, ConfigKey.Scope.Zone, BackupFrameworkEnabled.key()); | ||
| "The backup and recovery provider plugin. Valid plugin values: dummy, veeam, networker and nas", | ||
| true, ConfigKey.Scope.Zone, BackupFrameworkEnabled.key(), value -> validateBackupProviderConfig((String)value)); |
There was a problem hiding this comment.
| true, ConfigKey.Scope.Zone, BackupFrameworkEnabled.key(), value -> validateBackupProviderConfig((String)value)); | |
| true, ConfigKey.Scope.Zone, BackupFrameworkEnabled.key(), value -> validateBackupProviderConfig((String) value)); |
| throw new CloudRuntimeException("Invalid backup provider name provided"); | ||
| } | ||
| if (!backupProvidersMap.containsKey(name)) { | ||
| String[] backupProviderNames = name.split(","); |
There was a problem hiding this comment.
This is not possible now, right?
There was a problem hiding this comment.
I don't think so, currently, ACS allows taking any string as input, so if we provide a comma separated string, it would compare that with providers supported in ACS, and would fail and report Cannot find backup provider by name: dummy,nas (if both were set)
There was a problem hiding this comment.
I meant that validator will not allow , in the name, so no need for splitting the name by comma. We can revert to old code.
There was a problem hiding this comment.
--@abh1sar , what do you mean by “the validator”? are you talking about the ui/service/manager…—
never mind, got it.
There was a problem hiding this comment.
You're right, with the validator - this isn't required.
|
@Pearl1594 , can you address @abh1sar ’s comments? |
|
@blueorangutan package |
|
@DaanHoogland 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 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 16698 |
| if (value != null && (value.contains(",") || value.trim().contains(" "))) { | ||
| throw new IllegalArgumentException("Multiple backup provider plugins are not supported. Please provide a single plugin value."); | ||
| } | ||
| List<String> validPlugins = List.of("dummy", "veeam", "networker", "nas"); |
There was a problem hiding this comment.
can check these values with the enum?
There was a problem hiding this comment.
we don't have enums - do you want me to create one?
abh1sar
left a comment
There was a problem hiding this comment.
Code LGTM. Thanks @Pearl1594 .
|
@Pearl1594 , I never answerred this query; I think it is great, I wonder though if it shouldn’t just be part of |




Description
This PR fixed: #12449
The capacity dashboard issue is seen on 4.22 - but in general the getBackupProvider() is fixed to return the first provider set in the global settings when a comma separated list of providers is set.
94ac3ea - adds a validator, that prevents adding a comma separated list for backup provider setting. It also checks if the entered value is a valid one.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?