You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by "Pearl1594 (via GitHub)" <gi...@apache.org> on 2026/01/29 18:21:29 UTC
[PR] Fixes issue with loading Capacity dashboard when mulitple backup providers configured [cloudstack]
Pearl1594 opened a new pull request, #12550:
URL: https://github.com/apache/cloudstack/pull/12550
### 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.
<!--- Describe your changes in DETAIL - And how has behaviour functionally changed. -->
<!-- For new features, provide link to FS, dev ML discussion etc. -->
<!-- In case of bug fix, the expected and actual behaviours, steps to reproduce. -->
<!-- When "Fixes: #<id>" is specified, the issue/PR will automatically be closed when this PR gets merged -->
<!-- For addressing multiple issues/PRs, use multiple "Fixes: #<id>" -->
<!-- Fixes: # -->
<!--- ******************************************************************************* -->
<!--- NOTE: AUTOMATION USES THE DESCRIPTIONS TO SET LABELS AND PRODUCE DOCUMENTATION. -->
<!--- PLEASE PUT AN 'X' in only **ONE** box -->
<!--- ******************************************************************************* -->
### Types of changes
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] New feature (non-breaking change which adds functionality)
- [X] 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
- [ ] Test (unit or integration test code)
### Feature/Enhancement Scale or Bug Severity
#### Feature/Enhancement Scale
- [ ] Major
- [ ] Minor
#### Bug Severity
- [ ] BLOCKER
- [ ] Critical
- [ ] Major
- [X] Minor
- [ ] Trivial
### Screenshots (if appropriate):
### How Has This Been Tested?
<!-- Please describe in detail how you tested your changes. -->
<!-- Include details of your testing environment, and the tests you ran to -->
#### How did you try to break this feature and the system with this change?
<!-- see how your change affects other areas of the code, etc. -->
<!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md) document -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes issue with loading Capacity dashboard when mulitple backup providers configured [cloudstack]
Posted by "Pearl1594 (via GitHub)" <gi...@apache.org>.
Pearl1594 commented on PR #12550:
URL: https://github.com/apache/cloudstack/pull/12550#issuecomment-3820164417
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes issue with loading Capacity dashboard when mulitple backup providers configured [cloudstack]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #12550:
URL: https://github.com/apache/cloudstack/pull/12550#issuecomment-3847687935
## [](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=12550) **Quality Gate failed**
Failed conditions
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=12550&metric=new_coverage&view=list) (required ≥ 40%)
 [C Maintainability Rating on New Code](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=12550) (required ≥ B)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=12550)
##
 Catch issues before they fail your Quality Gate with our IDE extension  [SonarQube for IDE](https://www.sonarsource.com/products/sonarlint/features/connected-mode/?referrer=pull-request)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes issue with loading Capacity dashboard when mulitple backup providers configured [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #12550:
URL: https://github.com/apache/cloudstack/pull/12550#issuecomment-3847739499
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 16698
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes issue with loading Capacity dashboard when mulitple backup providers configured [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #12550:
URL: https://github.com/apache/cloudstack/pull/12550#issuecomment-3821204580
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 16639
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes issue with loading Capacity dashboard when mulitple backup providers configured [cloudstack]
Posted by "shwstppr (via GitHub)" <gi...@apache.org>.
shwstppr commented on code in PR #12550:
URL: https://github.com/apache/cloudstack/pull/12550#discussion_r2745302877
##########
framework/config/src/main/java/org/apache/cloudstack/framework/config/ValidatedConfigKey.java:
##########
@@ -0,0 +1,38 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package org.apache.cloudstack.framework.config;
+
+import java.util.function.Consumer;
+
+public class ValidatedConfigKey<T> extends ConfigKey<String> {
Review Comment:
+1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes issue with loading Capacity dashboard when mulitple backup providers configured [cloudstack]
Posted by "abh1sar (via GitHub)" <gi...@apache.org>.
abh1sar commented on code in PR #12550:
URL: https://github.com/apache/cloudstack/pull/12550#discussion_r2744931629
##########
framework/config/src/main/java/org/apache/cloudstack/framework/config/ValidatedConfigKey.java:
##########
@@ -0,0 +1,38 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package org.apache.cloudstack.framework.config;
+
+import java.util.function.Consumer;
+
+public class ValidatedConfigKey<T> extends ConfigKey<String> {
Review Comment:
I am not sure, but is there a way we can make the type Generic so that the validator can work for all types of ConfigKeys not just String?
##########
api/src/main/java/org/apache/cloudstack/backup/BackupManager.java:
##########
@@ -148,4 +150,14 @@ public interface BackupManager extends BackupService, Configurable, PluggableSer
boolean deleteBackup(final Long backupId, final Boolean forced);
BackupOffering updateBackupOffering(UpdateBackupOfferingCmd updateBackupOfferingCmd);
+
+ static void validateBackupProviderConfig(String value) {
+ if (value != null && (value.trim().contains(",") || value.contains(" "))) {
Review Comment:
```suggestion
if (value != null && (value.contains(",") || value.trim().contains(" "))) {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes issue with loading Capacity dashboard when mulitple backup providers configured [cloudstack]
Posted by "Pearl1594 (via GitHub)" <gi...@apache.org>.
Pearl1594 commented on code in PR #12550:
URL: https://github.com/apache/cloudstack/pull/12550#discussion_r2747226256
##########
server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java:
##########
@@ -972,10 +972,11 @@ public BackupProvider getBackupProvider(final String name) {
if (StringUtils.isEmpty(name)) {
throw new CloudRuntimeException("Invalid backup provider name provided");
}
- if (!backupProvidersMap.containsKey(name)) {
+ String[] backupProviderNames = name.split(",");
Review 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)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes issue with loading Capacity dashboard when mulitple backup providers configured [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #12550:
URL: https://github.com/apache/cloudstack/pull/12550#issuecomment-3869969933
@RosiKyu 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes issue with loading Capacity dashboard when mulitple backup providers configured [cloudstack]
Posted by "Pearl1594 (via GitHub)" <gi...@apache.org>.
Pearl1594 commented on PR #12550:
URL: https://github.com/apache/cloudstack/pull/12550#issuecomment-3862329541
Thanks for pointing out a possible issue in the logic @abh1sar - since we changed the volumePaths to point to the backup volume's path, during restore, it source was pointing correctly to the backed up volume's path but the destination was also pointing to the backup volume path rather than the current volume's path hence while restore volume succeeds, when the vm is booted it boots with the old volume.
I have addressed the issue - @RosiKyu this may require to be tested again, apologies.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes issue with loading Capacity dashboard when mulitple backup providers configured [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #12550:
URL: https://github.com/apache/cloudstack/pull/12550#issuecomment-3820166808
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes issue with loading Capacity dashboard when mulitple backup providers configured [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #12550:
URL: https://github.com/apache/cloudstack/pull/12550#issuecomment-3821042727
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes issue with loading Capacity dashboard when mulitple backup providers configured [cloudstack]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #12550:
URL: https://github.com/apache/cloudstack/pull/12550#issuecomment-3824390278
## [](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=12550) **Quality Gate failed**
Failed conditions
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=12550&metric=new_coverage&view=list) (required ≥ 40%)
 [C Maintainability Rating on New Code](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=12550) (required ≥ B)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=12550)
##
 Catch issues before they fail your Quality Gate with our IDE extension  [SonarQube for IDE](https://www.sonarsource.com/products/sonarlint/features/connected-mode/?referrer=pull-request)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes issue with loading Capacity dashboard when mulitple backup providers configured [cloudstack]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #12550:
URL: https://github.com/apache/cloudstack/pull/12550#issuecomment-3819776916
## [](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=12550) **Quality Gate failed**
Failed conditions
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=12550&metric=new_coverage&view=list) (required ≥ 40%)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=12550)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes issue with loading Capacity dashboard when mulitple backup providers configured [cloudstack]
Posted by "Pearl1594 (via GitHub)" <gi...@apache.org>.
Pearl1594 commented on PR #12550:
URL: https://github.com/apache/cloudstack/pull/12550#issuecomment-3820459268
@abh1sar @DaanHoogland I've added this bit : 94ac3eae32825b4f72736dc04debaec13c1e75a4 to add a validator that can be used for any configuration. Do you think this is useful?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes issue with loading Capacity dashboard when mulitple backup providers configured [cloudstack]
Posted by "Pearl1594 (via GitHub)" <gi...@apache.org>.
Pearl1594 commented on code in PR #12550:
URL: https://github.com/apache/cloudstack/pull/12550#discussion_r2763951777
##########
server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java:
##########
@@ -972,10 +972,11 @@ public BackupProvider getBackupProvider(final String name) {
if (StringUtils.isEmpty(name)) {
throw new CloudRuntimeException("Invalid backup provider name provided");
}
- if (!backupProvidersMap.containsKey(name)) {
+ String[] backupProviderNames = name.split(",");
Review Comment:
You're right, with the validator - this isn't required.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes issue with loading Capacity dashboard when mulitple backup providers configured [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #12550:
URL: https://github.com/apache/cloudstack/pull/12550#issuecomment-3847312621
@Pearl1594 , can you address @abh1sar ’s comments?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes issue with loading Capacity dashboard when mulitple backup providers configured [cloudstack]
Posted by "Pearl1594 (via GitHub)" <gi...@apache.org>.
Pearl1594 commented on code in PR #12550:
URL: https://github.com/apache/cloudstack/pull/12550#discussion_r2764706333
##########
api/src/main/java/org/apache/cloudstack/backup/BackupManager.java:
##########
@@ -148,4 +150,14 @@ public interface BackupManager extends BackupService, Configurable, PluggableSer
boolean deleteBackup(final Long backupId, final Boolean forced);
BackupOffering updateBackupOffering(UpdateBackupOfferingCmd updateBackupOfferingCmd);
+
+ static void validateBackupProviderConfig(String value) {
+ 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");
Review Comment:
we don't have enums - do you want me to create one?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes issue with loading Capacity dashboard when mulitple backup providers configured [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #12550:
URL: https://github.com/apache/cloudstack/pull/12550#issuecomment-3852214286
> [94ac3ea](https://github.com/apache/cloudstack/commit/94ac3eae32825b4f72736dc04debaec13c1e75a4)
@Pearl1594 , I never answerred this query; I think it is great, I wonder though if it shouldn’t just be part of `ConfigKey` instead of in a derived class??
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes issue with loading Capacity dashboard when mulitple backup providers configured [cloudstack]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #12550:
URL: https://github.com/apache/cloudstack/pull/12550#issuecomment-3820738217
## [](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=12550) **Quality Gate failed**
Failed conditions
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=12550&metric=new_coverage&view=list) (required ≥ 40%)
 [C Maintainability Rating on New Code](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=12550) (required ≥ B)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=12550)
##
 Catch issues before they fail your Quality Gate with our IDE extension  [SonarQube for IDE](https://www.sonarsource.com/products/sonarlint/features/connected-mode/?referrer=pull-request)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes issue with loading Capacity dashboard when mulitple backup providers configured [cloudstack]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #12550:
URL: https://github.com/apache/cloudstack/pull/12550#issuecomment-3820924071
## [](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=12550) **Quality Gate failed**
Failed conditions
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=12550&metric=new_coverage&view=list) (required ≥ 40%)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=12550)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes issue with loading Capacity dashboard when mulitple backup providers configured [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #12550:
URL: https://github.com/apache/cloudstack/pull/12550#issuecomment-3847428450
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes issue with loading Capacity dashboard when mulitple backup providers configured [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #12550:
URL: https://github.com/apache/cloudstack/pull/12550#discussion_r2763798083
##########
server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java:
##########
@@ -972,10 +972,11 @@ public BackupProvider getBackupProvider(final String name) {
if (StringUtils.isEmpty(name)) {
throw new CloudRuntimeException("Invalid backup provider name provided");
}
- if (!backupProvidersMap.containsKey(name)) {
+ String[] backupProviderNames = name.split(",");
Review Comment:
--@abh1sar , what do you mean by “the validator”? are you talking about the ui/service/manager…—
never mind, got it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes issue with loading Capacity dashboard when mulitple backup providers configured [cloudstack]
Posted by "Pearl1594 (via GitHub)" <gi...@apache.org>.
Pearl1594 commented on code in PR #12550:
URL: https://github.com/apache/cloudstack/pull/12550#discussion_r2744571345
##########
api/src/main/java/org/apache/cloudstack/backup/BackupManager.java:
##########
@@ -42,10 +43,20 @@ public interface BackupManager extends BackupService, Configurable, PluggableSer
"false",
"Is backup and recovery framework enabled.", false, ConfigKey.Scope.Zone);
- ConfigKey<String> BackupProviderPlugin = new ConfigKey<>("Advanced", String.class,
+ ConfigKey<String> BackupProviderPlugin = new ValidatedConfigKey<>("Advanced", String.class,
"backup.framework.provider.plugin",
"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 -> {
+ if (value != null && ((String)value).contains(",") || ((String)value).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");
+ if (!validPlugins.contains(((String) value).toLowerCase())) {
Review Comment:
If empty or null, it defaults to "dummy". Reset behaves normally.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes issue with loading Capacity dashboard when mulitple backup providers configured [cloudstack]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #12550:
URL: https://github.com/apache/cloudstack/pull/12550#issuecomment-3821939238
## [](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=12550) **Quality Gate failed**
Failed conditions
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=12550&metric=new_coverage&view=list) (required ≥ 40%)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=12550)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes issue with loading Capacity dashboard when mulitple backup providers configured [cloudstack]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #12550:
URL: https://github.com/apache/cloudstack/pull/12550#issuecomment-3819474163
## [Codecov](https://app.codecov.io/gh/apache/cloudstack/pull/12550?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 4.15%. Comparing base ([`a6ccde4`](https://app.codecov.io/gh/apache/cloudstack/commit/a6ccde44c4bc3c2b4623aca6b55500a722144763?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`0bb1616`](https://app.codecov.io/gh/apache/cloudstack/commit/0bb161612273b892b720d94a3e73ebdbcda054bb?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
> :exclamation: There is a different number of reports uploaded between BASE (a6ccde4) and HEAD (0bb1616). Click for more details.
>
> <details><summary>HEAD has 1 upload less than BASE</summary>
>
>| Flag | BASE (a6ccde4) | HEAD (0bb1616) |
>|------|------|------|
>|unittests|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## 4.20 #12550 +/- ##
=============================================
- Coverage 16.26% 4.15% -12.12%
=============================================
Files 5660 403 -5257
Lines 499907 32902 -467005
Branches 60696 5881 -54815
=============================================
- Hits 81316 1366 -79950
+ Misses 409521 31361 -378160
+ Partials 9070 175 -8895
```
| [Flag](https://app.codecov.io/gh/apache/cloudstack/pull/12550/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [uitests](https://app.codecov.io/gh/apache/cloudstack/pull/12550/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `4.15% <ø> (ø)` | |
| [unittests](https://app.codecov.io/gh/apache/cloudstack/pull/12550/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/cloudstack/pull/12550?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes issue with loading Capacity dashboard when mulitple backup providers configured [cloudstack]
Posted by "abh1sar (via GitHub)" <gi...@apache.org>.
abh1sar commented on code in PR #12550:
URL: https://github.com/apache/cloudstack/pull/12550#discussion_r2746919700
##########
api/src/main/java/org/apache/cloudstack/backup/BackupManager.java:
##########
@@ -42,10 +43,11 @@ public interface BackupManager extends BackupService, Configurable, PluggableSer
"false",
"Is backup and recovery framework enabled.", false, ConfigKey.Scope.Zone);
- ConfigKey<String> BackupProviderPlugin = new ConfigKey<>("Advanced", String.class,
+ ConfigKey<String> BackupProviderPlugin = new ValidatedConfigKey<>("Advanced", String.class,
"backup.framework.provider.plugin",
"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));
Review Comment:
```suggestion
true, ConfigKey.Scope.Zone, BackupFrameworkEnabled.key(), value -> validateBackupProviderConfig((String) value));
```
##########
server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java:
##########
@@ -972,10 +972,11 @@ public BackupProvider getBackupProvider(final String name) {
if (StringUtils.isEmpty(name)) {
throw new CloudRuntimeException("Invalid backup provider name provided");
}
- if (!backupProvidersMap.containsKey(name)) {
+ String[] backupProviderNames = name.split(",");
Review Comment:
This is not possible now, right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes issue with loading Capacity dashboard when mulitple backup providers configured [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland merged PR #12550:
URL: https://github.com/apache/cloudstack/pull/12550
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes issue with loading Capacity dashboard when mulitple backup providers configured [cloudstack]
Posted by "abh1sar (via GitHub)" <gi...@apache.org>.
abh1sar commented on code in PR #12550:
URL: https://github.com/apache/cloudstack/pull/12550#discussion_r2744465747
##########
api/src/main/java/org/apache/cloudstack/backup/BackupManager.java:
##########
@@ -42,10 +43,20 @@ public interface BackupManager extends BackupService, Configurable, PluggableSer
"false",
"Is backup and recovery framework enabled.", false, ConfigKey.Scope.Zone);
- ConfigKey<String> BackupProviderPlugin = new ConfigKey<>("Advanced", String.class,
+ ConfigKey<String> BackupProviderPlugin = new ValidatedConfigKey<>("Advanced", String.class,
"backup.framework.provider.plugin",
"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 -> {
+ if (value != null && ((String)value).contains(",") || ((String)value).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");
+ if (!validPlugins.contains(((String) value).toLowerCase())) {
+ throw new IllegalArgumentException("Invalid backup provider plugin: " + value + ". Valid plugin values are: " + String.join(", ", validPlugins));
+ }
+ });
Review Comment:
This is a great idea. But can we put the validation logic in a separate method?
ConfigKey declaration looks a bit verbose
##########
api/src/main/java/org/apache/cloudstack/backup/BackupManager.java:
##########
@@ -42,10 +43,20 @@ public interface BackupManager extends BackupService, Configurable, PluggableSer
"false",
"Is backup and recovery framework enabled.", false, ConfigKey.Scope.Zone);
- ConfigKey<String> BackupProviderPlugin = new ConfigKey<>("Advanced", String.class,
+ ConfigKey<String> BackupProviderPlugin = new ValidatedConfigKey<>("Advanced", String.class,
"backup.framework.provider.plugin",
"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 -> {
+ if (value != null && ((String)value).contains(",") || ((String)value).contains(" ")) {
Review Comment:
Maybe we should strip the value before checking if it contains spaces?
##########
api/src/main/java/org/apache/cloudstack/backup/BackupManager.java:
##########
@@ -42,10 +43,20 @@ public interface BackupManager extends BackupService, Configurable, PluggableSer
"false",
"Is backup and recovery framework enabled.", false, ConfigKey.Scope.Zone);
- ConfigKey<String> BackupProviderPlugin = new ConfigKey<>("Advanced", String.class,
+ ConfigKey<String> BackupProviderPlugin = new ValidatedConfigKey<>("Advanced", String.class,
"backup.framework.provider.plugin",
"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 -> {
+ if (value != null && ((String)value).contains(",") || ((String)value).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");
+ if (!validPlugins.contains(((String) value).toLowerCase())) {
Review Comment:
What happens when the setting is reset? when the value contains null or empty string?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes issue with loading Capacity dashboard when mulitple backup providers configured [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #12550:
URL: https://github.com/apache/cloudstack/pull/12550#issuecomment-3820516754
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 16636
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes issue with loading Capacity dashboard when mulitple backup providers configured [cloudstack]
Posted by "Pearl1594 (via GitHub)" <gi...@apache.org>.
Pearl1594 commented on PR #12550:
URL: https://github.com/apache/cloudstack/pull/12550#issuecomment-3821037468
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes issue with loading Capacity dashboard when mulitple backup providers configured [cloudstack]
Posted by "abh1sar (via GitHub)" <gi...@apache.org>.
abh1sar commented on code in PR #12550:
URL: https://github.com/apache/cloudstack/pull/12550#discussion_r2751140732
##########
server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java:
##########
@@ -972,10 +972,11 @@ public BackupProvider getBackupProvider(final String name) {
if (StringUtils.isEmpty(name)) {
throw new CloudRuntimeException("Invalid backup provider name provided");
}
- if (!backupProvidersMap.containsKey(name)) {
+ String[] backupProviderNames = name.split(",");
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes issue with loading Capacity dashboard when mulitple backup providers configured [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #12550:
URL: https://github.com/apache/cloudstack/pull/12550#discussion_r2763798083
##########
server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java:
##########
@@ -972,10 +972,11 @@ public BackupProvider getBackupProvider(final String name) {
if (StringUtils.isEmpty(name)) {
throw new CloudRuntimeException("Invalid backup provider name provided");
}
- if (!backupProvidersMap.containsKey(name)) {
+ String[] backupProviderNames = name.split(",");
Review Comment:
@abh1sar , what do you mean by “the validator”? are you talking about the ui/service/manager...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes issue with loading Capacity dashboard when mulitple backup providers configured [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #12550:
URL: https://github.com/apache/cloudstack/pull/12550#issuecomment-3847438791
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes issue with loading Capacity dashboard when mulitple backup providers configured [cloudstack]
Posted by "sureshanaparti (via GitHub)" <gi...@apache.org>.
sureshanaparti commented on code in PR #12550:
URL: https://github.com/apache/cloudstack/pull/12550#discussion_r2764474466
##########
api/src/main/java/org/apache/cloudstack/backup/BackupManager.java:
##########
@@ -148,4 +150,14 @@ public interface BackupManager extends BackupService, Configurable, PluggableSer
boolean deleteBackup(final Long backupId, final Boolean forced);
BackupOffering updateBackupOffering(UpdateBackupOfferingCmd updateBackupOfferingCmd);
+
+ static void validateBackupProviderConfig(String value) {
+ 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");
Review Comment:
can check these values with the enum?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes issue with loading Capacity dashboard when mulitple backup providers configured [cloudstack]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #12550:
URL: https://github.com/apache/cloudstack/pull/12550#issuecomment-3862725604
## [](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=12550) **Quality Gate failed**
Failed conditions
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=12550&metric=new_coverage&view=list) (required ≥ 40%)
 [C Maintainability Rating on New Code](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=12550) (required ≥ B)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=12550)
##
 Catch issues before they fail your Quality Gate with our IDE extension  [SonarQube for IDE](https://www.sonarsource.com/products/sonarlint/features/connected-mode/?referrer=pull-request)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes issue with loading Capacity dashboard when mulitple backup providers configured [cloudstack]
Posted by "RosiKyu (via GitHub)" <gi...@apache.org>.
RosiKyu commented on PR #12550:
URL: https://github.com/apache/cloudstack/pull/12550#issuecomment-3869962974
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixes issue with loading Capacity dashboard when mulitple backup providers configured [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #12550:
URL: https://github.com/apache/cloudstack/pull/12550#issuecomment-3870287318
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 16745
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org