You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by "sureshanaparti (via GitHub)" <gi...@apache.org> on 2025/09/03 12:53:28 UTC
[PR] Check backup framework enabled config in zone scope first (before checking in global scope) [cloudstack]
sureshanaparti opened a new pull request, #11567:
URL: https://github.com/apache/cloudstack/pull/11567
### Description
This PR checks backup framework enabled config in zone scope first, before checking in global scope. If the config is set in the zone, it should be honored first.
<!--- 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)
- [ ] Bug fix (non-breaking change which fixes an issue)
- [x] 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
- [x] Minor
#### Bug Severity
- [ ] BLOCKER
- [ ] Critical
- [ ] Major
- [ ] 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] Check backup framework enabled config in zone scope first (before checking in global scope) [cloudstack]
Posted by "sureshanaparti (via GitHub)" <gi...@apache.org>.
sureshanaparti commented on PR #11567:
URL: https://github.com/apache/cloudstack/pull/11567#issuecomment-3249133543
@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] Update backup framework enabled config to global scope [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #11567:
URL: https://github.com/apache/cloudstack/pull/11567#issuecomment-3816018652
@sureshanaparti 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] Update backup framework enabled config to global scope [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #11567:
URL: https://github.com/apache/cloudstack/pull/11567#issuecomment-3816345938
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 16610
--
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] [WIP] Update backup framework enabled config to global scope [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #11567:
URL: https://github.com/apache/cloudstack/pull/11567#issuecomment-3327174563
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 15153
--
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] Update backup framework enabled config to global scope [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #11567:
URL: https://github.com/apache/cloudstack/pull/11567#issuecomment-3270076237
@sureshanaparti 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] Check backup framework enabled config in zone scope first (before checking in global scope) [cloudstack]
Posted by "sureshanaparti (via GitHub)" <gi...@apache.org>.
sureshanaparti commented on PR #11567:
URL: https://github.com/apache/cloudstack/pull/11567#issuecomment-3390041549
> > @sureshanaparti I think it is not a bug. I suggest to move to 4.22.0
> > please check other zone-level settings too a doc PR may be also needed to describe the behaviour
>
> @weizhouapache There is no change in config scope, but seems minor doc changes required (I'll raise the doc PR). It fixes the issue when 'backup.framework.enabled' is set to true at zone level and set to false in global settings.
Raised doc PR - https://github.com/apache/cloudstack-documentation/pull/579
--
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] Check backup framework enabled config in zone scope first (before checking in global scope) [cloudstack]
Posted by "abh1sar (via GitHub)" <gi...@apache.org>.
abh1sar commented on code in PR #11567:
URL: https://github.com/apache/cloudstack/pull/11567#discussion_r2427038725
##########
framework/config/src/main/java/org/apache/cloudstack/framework/config/impl/ConfigDepotImpl.java:
##########
@@ -296,6 +296,27 @@ protected String getConfigStringValueInternal(Ternary<String, ConfigKey.Scope, L
return null;
}
+ public boolean doesConfigKeyAndValueExistInScope(String key, String value, ConfigKey.Scope scope) {
+ if (!ConfigKey.Scope.Global.equals(scope)) {
+ ScopedConfigStorage scopedConfigStorage = null;
+ for (ScopedConfigStorage storage : _scopedStorages) {
+ if (storage.getScope() == scope) {
+ scopedConfigStorage = storage;
+ }
+ }
+ if (scopedConfigStorage == null) {
+ logger.warn("Unable to check existence of config key {} and value {} in scope: {}, couldn't find config storage for this scope", key, value, scope);
+ return false;
+ }
+ return scopedConfigStorage.doesConfigKeyAndValueExist(key, value);
+ }
+ ConfigurationVO configurationVO = _configDao.findByName(key);
+ if (configurationVO != null) {
Review Comment:
For Global scope value() is good enough, right?
why do we need this 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] Enable backup framework when the config 'backup.framework.enabled' is set to true in Zone scope and not in Global setting [cloudstack]
Posted by "abh1sar (via GitHub)" <gi...@apache.org>.
abh1sar commented on PR #11567:
URL: https://github.com/apache/cloudstack/pull/11567#issuecomment-3668889864
Sorry, I don't remember if it was discussed before. But what about making this config global scope only?
--
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] [WIP] Update backup framework enabled config to global scope [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #11567:
URL: https://github.com/apache/cloudstack/pull/11567#issuecomment-3332650091
@sureshanaparti 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] Enable backup framework when the config 'backup.framework.enabled' is set to true in Zone scope and not in Global setting [cloudstack]
Posted by "shwstppr (via GitHub)" <gi...@apache.org>.
shwstppr commented on PR #11567:
URL: https://github.com/apache/cloudstack/pull/11567#issuecomment-3803069556
@nvazquez I don't have a strong opinion either way and others can chime in
--
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] Update backup framework enabled config to global scope [cloudstack]
Posted by "sureshanaparti (via GitHub)" <gi...@apache.org>.
sureshanaparti commented on PR #11567:
URL: https://github.com/apache/cloudstack/pull/11567#issuecomment-3815980399
> I think it would be better to make this setting global. It makes sense then that all the zones see backup related APIs in the UI. Backup offerings are created per Zone by the Admin. So admin can just created backup offerings for the Zone where they want to. Other zones will see the API `Assign Backup Offering` but there wouldn't be any offering to assign to. Admins can still configure different providers for each zone.
>
> With the solution of having per Zone setting and still showing backup APIs to other zones, there will be confusion and IMO it doesn't solve anything.
@abh1sar as discussed, backup config is updated to global setting again (reverted current changes). please check.
--
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] [WIP] Update backup framework enabled config to global scope [cloudstack]
Posted by "sureshanaparti (via GitHub)" <gi...@apache.org>.
sureshanaparti commented on PR #11567:
URL: https://github.com/apache/cloudstack/pull/11567#issuecomment-3326906476
@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] [WIP] Update backup framework enabled config to global scope [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #11567:
URL: https://github.com/apache/cloudstack/pull/11567#issuecomment-3326912938
@sureshanaparti 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] Check backup framework enabled config in zone scope first (before checking in global scope) [cloudstack]
Posted by "sureshanaparti (via GitHub)" <gi...@apache.org>.
sureshanaparti commented on PR #11567:
URL: https://github.com/apache/cloudstack/pull/11567#issuecomment-3389965587
> @sureshanaparti I think it is not a bug. I suggest to move to 4.22.0
>
> please check other zone-level settings too a doc PR may be also needed to describe the behaviour
@weizhouapache no doc changes required as scope is unchanged. it fixes the issue when 'backup.framework.enabled' is set to true at zone level and set to false in global settings.
--
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] Check backup framework enabled config in zone scope first (before checking in global scope) [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #11567:
URL: https://github.com/apache/cloudstack/pull/11567#issuecomment-3370111502
@sureshanaparti
I think it is not a bug. I suggest to move to 4.22.0
please check other zone-level settings too
a doc PR may be also needed to describe the behaviour
--
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] Check backup framework enabled config in zone scope first (before checking in global scope) [cloudstack]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #11567:
URL: https://github.com/apache/cloudstack/pull/11567#issuecomment-3249158917
## [Codecov](https://app.codecov.io/gh/apache/cloudstack/pull/11567?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.00%. Comparing base ([`f24b175`](https://app.codecov.io/gh/apache/cloudstack/commit/f24b1757a0b920d88470033bdf4bd658717a8d20?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`2f0ee6d`](https://app.codecov.io/gh/apache/cloudstack/commit/2f0ee6d59e6d2632d6714db39ee3d03ec8dcdbb1?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 (f24b175) and HEAD (2f0ee6d). Click for more details.
>
> <details><summary>HEAD has 1 upload less than BASE</summary>
>
>| Flag | BASE (f24b175) | HEAD (2f0ee6d) |
>|------|------|------|
>|unittests|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## 4.20 #11567 +/- ##
=============================================
- Coverage 16.17% 4.00% -12.17%
=============================================
Files 5656 402 -5254
Lines 498015 32635 -465380
Branches 60406 5798 -54608
=============================================
- Hits 80538 1308 -79230
+ Misses 408514 31177 -377337
+ Partials 8963 150 -8813
```
| [Flag](https://app.codecov.io/gh/apache/cloudstack/pull/11567/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/11567/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `4.00% <ø> (ø)` | |
| [unittests](https://app.codecov.io/gh/apache/cloudstack/pull/11567/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/11567?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] Enable backup framework when the config 'backup.framework.enabled' is set to true in Zone scope and not in Global setting [cloudstack]
Posted by "sureshanaparti (via GitHub)" <gi...@apache.org>.
sureshanaparti commented on code in PR #11567:
URL: https://github.com/apache/cloudstack/pull/11567#discussion_r2429740973
##########
server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java:
##########
@@ -980,7 +980,7 @@ public BackupProvider getBackupProvider(final String name) {
@Override
public List<Class<?>> getCommands() {
final List<Class<?>> cmdList = new ArrayList<Class<?>>();
- if (!BackupFrameworkEnabled.value()) {
+ if (!BackupFrameworkEnabled.value() && !BackupFrameworkEnabled.hasValueInScope(Boolean.TRUE.toString())) {
Review Comment:
@shwstppr it doesn't restrict API access. it allows API access if it is enabled in global or in any zone (not mandatory to enable in global scope). previously, enabling at global level is mandatory for API access.
--
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] Update backup framework enabled config to global scope [cloudstack]
Posted by "sureshanaparti (via GitHub)" <gi...@apache.org>.
sureshanaparti commented on PR #11567:
URL: https://github.com/apache/cloudstack/pull/11567#issuecomment-3816012899
@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] Check backup framework enabled config in zone scope first (before checking in global scope) [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #11567:
URL: https://github.com/apache/cloudstack/pull/11567#issuecomment-3390498884
@rohityadavcloud
the last change on the global setting was introduced in the #4202 , what do you think about this PR ?
cc @sureshanaparti
--
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] Check backup framework enabled config in zone scope first (before checking in global scope) [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #11567:
URL: https://github.com/apache/cloudstack/pull/11567#issuecomment-3337399160
@sureshanaparti a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests
--
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] Update backup framework enabled config to global scope [cloudstack]
Posted by "abh1sar (via GitHub)" <gi...@apache.org>.
abh1sar commented on PR #11567:
URL: https://github.com/apache/cloudstack/pull/11567#issuecomment-3830975782
> @sureshanaparti @abh1sar please check if we need to handle upgrade scenario with existing values at zone
Thanks for pointing @harikrishna-patnala. I had to think about it, but I don't think we need to handle db upgrade.
Someone might have one of the following scenarios in their current env:
1. only Global scope config is set -> then no change in behaviour.
2. both Zone scope and Global scope config is set -> the Global scope config will continue working. The config detail stored in the Zone details will just get ignored.
3. Zone scope config is set but Global scope config is not -> this is not usable as it doesn't expose any backup APIs. So no one would be using 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] Update backup framework enabled config to global scope [cloudstack]
Posted by "sureshanaparti (via GitHub)" <gi...@apache.org>.
sureshanaparti commented on PR #11567:
URL: https://github.com/apache/cloudstack/pull/11567#issuecomment-3833425551
> > > should it be "Disable" instead of "Enable" ?
> >
> >
> > Default is disabled, so "Enable" here is right.
>
> it looks different as what is described <img alt="image" width="1631" height="357" src="https://private-user-images.githubusercontent.com/57355700/543626697-e76386ae-87a6-4853-a97b-f55051ec1b45.png?jwt=eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3NzAwMTcxMjcsIm5iZiI6MTc3MDAxNjgyNywicGF0aCI6Ii81NzM1NTcwMC81NDM2MjY2OTctZTc2Mzg2YWUtODdhNi00ODUzLWE5N2ItZjU1MDUxZWMxYjQ1LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNjAyMDIlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjYwMjAyVDA3MjAyN1omWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTkwZDQ0YmVmNTc4ZWU3ZDljYzVlYzI5MDNlMGJkZjBmN2E4MmE5ZDJjZTdjMjQ2N2FkZjQyMGFiZDc3YjlmMmEmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0In0.KN411vCI78JYCzSl_eUrQTPUOVGqhngcD7oFpLiuSsU">
@weizhouapache this is earlier description, maybe I didn't confirm my last update, so it's not saved. check now. thanks.
--
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] Update backup framework enabled config to global scope [cloudstack]
Posted by "sureshanaparti (via GitHub)" <gi...@apache.org>.
sureshanaparti commented on PR #11567:
URL: https://github.com/apache/cloudstack/pull/11567#issuecomment-3833269830
> > @sureshanaparti @abh1sar please check if we need to handle upgrade scenario with existing values at zone
>
> Thanks for pointing @harikrishna-patnala. I had to think about it, but I don't think we need to handle db upgrade.
>
> Someone might have one of the following scenarios in their current env:
>
> 1. only Global scope config is set -> then no change in behaviour.
> 2. both Zone scope and Global scope config is set -> the Global scope config will continue working. The config detail stored in the Zone details will just get ignored.
> 3. Zone scope config is set but Global scope config is not -> this is not usable as it doesn't expose any backup APIs. So no one would be using it.
+1, upgrade is required only when the zone-level config is enabled (in the cases 2 and 3), and for that cases also, even though zone-level config is enabled, the global-level backup config should be enabled for backup framework/APIs to work.
--
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] Enable backup framework when the config 'backup.framework.enabled' is set to true in Zone scope and not in Global setting [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #11567:
URL: https://github.com/apache/cloudstack/pull/11567#issuecomment-3804289009
if the new behavior applies on all the zone/global settings, and documented , I am ok with 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] Check backup framework enabled config in zone scope first (before checking in global scope) [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #11567:
URL: https://github.com/apache/cloudstack/pull/11567#issuecomment-3400448275
> @DaanHoogland No, ConfigKey.valueInScope(Scope, Long) fetches the value in that scope with id specified, if it doesn't exist, picks the global value.
reading the title of this PR that seems to be the intention?!?
--
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] Check backup framework enabled config in zone scope first (before checking in global scope) [cloudstack]
Posted by "sureshanaparti (via GitHub)" <gi...@apache.org>.
sureshanaparti commented on code in PR #11567:
URL: https://github.com/apache/cloudstack/pull/11567#discussion_r2428148738
##########
framework/config/src/main/java/org/apache/cloudstack/framework/config/impl/ConfigDepotImpl.java:
##########
@@ -296,6 +296,27 @@ protected String getConfigStringValueInternal(Ternary<String, ConfigKey.Scope, L
return null;
}
+ public boolean doesConfigKeyAndValueExistInScope(String key, String value, ConfigKey.Scope scope) {
+ if (!ConfigKey.Scope.Global.equals(scope)) {
+ ScopedConfigStorage scopedConfigStorage = null;
+ for (ScopedConfigStorage storage : _scopedStorages) {
+ if (storage.getScope() == scope) {
+ scopedConfigStorage = storage;
+ }
+ }
+ if (scopedConfigStorage == null) {
+ logger.warn("Unable to check existence of config key {} and value {} in scope: {}, couldn't find config storage for this scope", key, value, scope);
+ return false;
+ }
+ return scopedConfigStorage.doesConfigKeyAndValueExist(key, value);
+ }
+ ConfigurationVO configurationVO = _configDao.findByName(key);
+ if (configurationVO != null) {
Review Comment:
@abh1sar some earlier config keys are not part of config framework, so these are fetched directly from the db. it's the same with the current method _getConfigStringValueInternal()_ as well.
--
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] Update backup framework enabled config to global scope [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #11567:
URL: https://github.com/apache/cloudstack/pull/11567#issuecomment-5476678949
ping @sureshanaparti
--
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] Check backup framework enabled config in zone scope first (before checking in global scope) [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #11567:
URL: https://github.com/apache/cloudstack/pull/11567#issuecomment-3390390657
> > > @sureshanaparti I think it is not a bug. I suggest to move to 4.22.0
> > > please check other zone-level settings too a doc PR may be also needed to describe the behaviour
> >
> >
> > @weizhouapache There is no change in config scope, but seems minor doc changes required (I'll raise the doc PR). It fixes the issue when 'backup.framework.enabled' is set to true at zone level and set to false in global settings.
>
> Raised doc PR - [apache/cloudstack-documentation#579](https://github.com/apache/cloudstack-documentation/pull/579)
thanks @sureshanaparti
moving back to 4.20.2 milestone
can you check other settings ? for example `RoutedNetworkVpcEnabled` (I can only find this)
--
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] [WIP] Update backup framework enabled config to global scope [cloudstack]
Posted by "sureshanaparti (via GitHub)" <gi...@apache.org>.
sureshanaparti commented on PR #11567:
URL: https://github.com/apache/cloudstack/pull/11567#issuecomment-3332646604
@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] Check backup framework enabled config in zone scope first (before checking in global scope) [cloudstack]
Posted by "sureshanaparti (via GitHub)" <gi...@apache.org>.
sureshanaparti commented on PR #11567:
URL: https://github.com/apache/cloudstack/pull/11567#issuecomment-3397304699
> > Hi @weizhouapache Yes, 'routed.network.vpc.enabled' is also Zone scoped, also has to be updated (at the below files/code) to work when 'routed.network.vpc.enabled' is set to true in Zone, and false in Global Settings. If it is set to true in Global Settings, all zones in the deployment are enabled with it by default when the respective Zone setting is not updated to false. Also, any relevant doc changes to be updated if required.
>
> thanks @sureshanaparti for the research
>
> can you check the PR #4202 ?
Checked it @weizhouapache , B&R APIs are not exported when the setting is not enabled globally there because the config framework couldn't check whether any scoped level has the setting enabled or not. This PR introduces config framework to check any scoped level setting is set with any particular value set or not, which provides flexibility to check the value is set or not at the scope level.
--
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] Check backup framework enabled config in zone scope first (before checking in global scope) [cloudstack]
Posted by "sureshanaparti (via GitHub)" <gi...@apache.org>.
sureshanaparti commented on PR #11567:
URL: https://github.com/apache/cloudstack/pull/11567#issuecomment-3396972190
>
> thanks @sureshanaparti moving back to 4.20.2 milestone
>
> can you check other settings ? for example `RoutedNetworkVpcEnabled` (I can only find this)
Hi @weizhouapache Yes, 'routed.network.vpc.enabled' is also Zone scoped, also has to be updated (at the below files/code) to work when 'routed.network.vpc.enabled' is set to true in Zone, and false in Global Settings. Also, any doc changes if required.
https://github.com/apache/cloudstack/blob/86cad79c156298e66dcabf668a05dc79fd46a129/server/src/main/java/org/apache/cloudstack/network/RoutedIpv4ManagerImpl.java#L171-L174
https://github.com/apache/cloudstack/blob/86cad79c156298e66dcabf668a05dc79fd46a129/server/src/main/java/com/cloud/network/vpc/VpcManagerImpl.java#L502-L513
https://github.com/apache/cloudstack/blob/86cad79c156298e66dcabf668a05dc79fd46a129/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java#L6842-L6851
--
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] Enable backup framework when the config 'backup.framework.enabled' is set to true in Zone scope and not in Global setting [cloudstack]
Posted by "abh1sar (via GitHub)" <gi...@apache.org>.
abh1sar commented on PR #11567:
URL: https://github.com/apache/cloudstack/pull/11567#issuecomment-3809889810
I think it would be better to make this setting global.
It makes sense then that all the zones see backup related APIs in the UI.
Backup offerings are created per Zone by the Admin. So admin can just created backup offerings for the Zone where they want to.
Other zones will see the API `Assign Backup Offering` but there wouldn't be any offering to assign to.
Admins can still configure different providers for each zone.
With the solution of having per Zone setting and still showing backup APIs to other zones, there will be confusion and IMO it doesn't solve anything.
--
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] Update backup framework enabled config to global scope [cloudstack]
Posted by "abh1sar (via GitHub)" <gi...@apache.org>.
abh1sar commented on PR #11567:
URL: https://github.com/apache/cloudstack/pull/11567#issuecomment-3831021495
> @sureshanaparti in the description
>
> ```
> Enable 'backup.framework.enabled' config in global settings.
> ```
>
> should it be "Disable" instead of "Enable" ?
Default is disabled, so "Enable" here is 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] Update backup framework enabled config to global scope [cloudstack]
Posted by "harikrishna-patnala (via GitHub)" <gi...@apache.org>.
harikrishna-patnala commented on PR #11567:
URL: https://github.com/apache/cloudstack/pull/11567#issuecomment-3822292906
@sureshanaparti @abh1sar please check if we need to handle upgrade scenario with existing values at zone
--
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] Update backup framework enabled config to global scope [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #11567:
URL: https://github.com/apache/cloudstack/pull/11567#issuecomment-3833471355
> > > > should it be "Disable" instead of "Enable" ?
> > >
> > >
> > > Default is disabled, so "Enable" here is right.
> >
> >
> > it looks different as what is described <img alt="image" width="1631" height="357" src="https://private-user-images.githubusercontent.com/57355700/543626697-e76386ae-87a6-4853-a97b-f55051ec1b45.png?jwt=eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3NzAwMTcxMjcsIm5iZiI6MTc3MDAxNjgyNywicGF0aCI6Ii81NzM1NTcwMC81NDM2MjY2OTctZTc2Mzg2YWUtODdhNi00ODUzLWE5N2ItZjU1MDUxZWMxYjQ1LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNjAyMDIlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjYwMjAyVDA3MjAyN1omWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTkwZDQ0YmVmNTc4ZWU3ZDljYzVlYzI5MDNlMGJkZjBmN2E4MmE5ZDJjZTdjMjQ2N2FkZjQyMGFiZDc3YjlmMmEmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0In0.KN411vCI78JYCzSl_eUrQTPUOVGqhngcD7oFpLiuSsU">
>
> @weizhouapache this is earlier description, maybe I didn't confirm my last update, so it's not saved. check now. thanks.
ok, thanks @sureshanaparti
@abh1sar 's testing results look ok
some questions
1. since the setting is now gloabl, is there a way to determine if the backup is enabled for the zone, for example, by backup provider ?
2. if not needed, can you remove some validation steps in the soure code ?
3. on the UI, is there a way to disable or hide the icons for backup for vms in the zone without backup support ? sounds like a UI improvement
--
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] Update backup framework enabled config to global scope [cloudstack]
Posted by "winterhazel (via GitHub)" <gi...@apache.org>.
winterhazel commented on PR #11567:
URL: https://github.com/apache/cloudstack/pull/11567#issuecomment-4509190439
@sureshanaparti could you have a look at @weizhouapache's comment at https://github.com/apache/cloudstack/pull/11567#issuecomment-3833471355?
--
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] Update backup framework enabled config to global scope [cloudstack]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #11567:
URL: https://github.com/apache/cloudstack/pull/11567#discussion_r2333008501
##########
server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java:
##########
@@ -946,13 +946,13 @@ public boolean configure(String name, Map<String, Object> params) throws Configu
return true;
}
Review Comment:
The method signature change from `isDisabled(final Long zoneId)` to `isDisabled()` is a breaking change that removes the zone parameter. This could break existing callers that pass a zone ID. Consider deprecating the old method first or ensuring all callers are updated in this PR.
```suggestion
/**
* @deprecated Use {@link #isDisabled()} instead. This method will be removed in a future release.
*/
@Deprecated
public boolean isDisabled(final Long zoneId) {
return isDisabled();
}
```
##########
server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java:
##########
@@ -1236,12 +1236,11 @@ protected void runInContext() {
if (logger.isTraceEnabled()) {
logger.trace("Backup sync background task is running...");
}
+ if (isDisabled()) {
+ logger.debug("Backup Sync Task is not enabled. Skipping!");
+ return;
+ }
for (final DataCenter dataCenter : dataCenterDao.listAllZones()) {
Review Comment:
Moving the `isDisabled()` check outside the zone loop improves performance by avoiding redundant global configuration checks for each zone. However, this changes the behavior from per-zone checking to a single global check that exits early.
--
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] Check backup framework enabled config in zone scope first (before checking in global scope) [cloudstack]
Posted by "sureshanaparti (via GitHub)" <gi...@apache.org>.
sureshanaparti commented on PR #11567:
URL: https://github.com/apache/cloudstack/pull/11567#issuecomment-3337391598
@blueorangutan test
--
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] Enable backup framework when the config 'backup.framework.enabled' is set to true in Zone scope and not in Global setting [cloudstack]
Posted by "shwstppr (via GitHub)" <gi...@apache.org>.
shwstppr commented on code in PR #11567:
URL: https://github.com/apache/cloudstack/pull/11567#discussion_r2625935073
##########
server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java:
##########
@@ -980,7 +980,7 @@ public BackupProvider getBackupProvider(final String name) {
@Override
public List<Class<?>> getCommands() {
final List<Class<?>> cmdList = new ArrayList<Class<?>>();
- if (!BackupFrameworkEnabled.value()) {
+ if (!BackupFrameworkEnabled.value() && !BackupFrameworkEnabled.hasValueInScope(Boolean.TRUE.toString())) {
Review Comment:
@sureshanaparti I understand that, but won't it create an issue in UI showing backup-related options for zones and for zone resources where backup feature is not enabled?
eg: assignVirtualMachineToBackupOffering action will start showing for all VMs, deploy VM form will start showing backup options even for zone where option isn't available.
Though, this could be currently true for the other way around as well
--
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] Update backup framework enabled config to global scope [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #11567:
URL: https://github.com/apache/cloudstack/pull/11567#issuecomment-3822251019
@sureshanaparti
in the description
```
Enable 'backup.framework.enabled' config in global settings.
```
should it be "Disable" instead of "Enable" ?
--
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] Enable backup framework when the config 'backup.framework.enabled' is set to true in Zone scope and not in Global setting [cloudstack]
Posted by "nvazquez (via GitHub)" <gi...@apache.org>.
nvazquez commented on PR #11567:
URL: https://github.com/apache/cloudstack/pull/11567#issuecomment-3800131631
Hi @sureshanaparti @shwstppr are all the review comments addressed on this PR? If that is the case, is it ready for testing?
--
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] Update backup framework enabled config to global scope [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #11567:
URL: https://github.com/apache/cloudstack/pull/11567#issuecomment-3833399834
>
> >
> > should it be "Disable" instead of "Enable" ?
>
> Default is disabled, so "Enable" here is right.
it looks different as what is described
<img width="1631" height="357" alt="image" src="https://github.com/user-attachments/assets/e76386ae-87a6-4853-a97b-f55051ec1b45" />
--
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] Check backup framework enabled config in zone scope first (before checking in global scope) [cloudstack]
Posted by "sureshanaparti (via GitHub)" <gi...@apache.org>.
sureshanaparti commented on PR #11567:
URL: https://github.com/apache/cloudstack/pull/11567#issuecomment-3400426114
> @sureshanaparti , doesn’t `ConfigKey.velueInScope(Scope, Long)` already address this problem?
@DaanHoogland No, ConfigKey.valueInScope(Scope, Long) fetches the value in that scope with id specified, if it doesn't exist, picks the global value.
--
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] Enable backup framework when the config 'backup.framework.enabled' is set to true in Zone scope and not in Global setting [cloudstack]
Posted by "sureshanaparti (via GitHub)" <gi...@apache.org>.
sureshanaparti commented on PR #11567:
URL: https://github.com/apache/cloudstack/pull/11567#issuecomment-3401399411
> > @DaanHoogland No, ConfigKey.valueInScope(Scope, Long) fetches the value in that scope with id specified, if it doesn't exist, picks the global value.
>
> reading the title of this PR that seems to be the intention?!?
updated title & description.
--
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] Enable backup framework when the config 'backup.framework.enabled' is set to true in Zone scope and not in Global setting [cloudstack]
Posted by "shwstppr (via GitHub)" <gi...@apache.org>.
shwstppr commented on code in PR #11567:
URL: https://github.com/apache/cloudstack/pull/11567#discussion_r2429722685
##########
server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java:
##########
@@ -980,7 +980,7 @@ public BackupProvider getBackupProvider(final String name) {
@Override
public List<Class<?>> getCommands() {
final List<Class<?>> cmdList = new ArrayList<Class<?>>();
- if (!BackupFrameworkEnabled.value()) {
+ if (!BackupFrameworkEnabled.value() && !BackupFrameworkEnabled.hasValueInScope(Boolean.TRUE.toString())) {
Review Comment:
We should not restrict/allow API access if based on the zone config value. It might break UI/clients.
##########
server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java:
##########
@@ -980,7 +980,7 @@ public BackupProvider getBackupProvider(final String name) {
@Override
public List<Class<?>> getCommands() {
final List<Class<?>> cmdList = new ArrayList<Class<?>>();
- if (!BackupFrameworkEnabled.value()) {
+ if (!BackupFrameworkEnabled.value() && !BackupFrameworkEnabled.hasValueInScope(Boolean.TRUE.toString())) {
Review Comment:
We should not restrict/allow API access based on the zone config value. It might break UI/clients.
--
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] Check backup framework enabled config in zone scope first (before checking in global scope) [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #11567:
URL: https://github.com/apache/cloudstack/pull/11567#issuecomment-3340965507
<b>[SF] Trillian test result (tid-14446)</b>
Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8
Total time taken: 57034 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr11567-t14446-kvm-ol8.zip
Smoke tests completed. 141 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
--
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] [WIP] Update backup framework enabled config to global scope [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #11567:
URL: https://github.com/apache/cloudstack/pull/11567#issuecomment-3333037198
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 15163
--
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] Enable backup framework when the config 'backup.framework.enabled' is set to true in Zone scope and not in Global setting [cloudstack]
Posted by "shwstppr (via GitHub)" <gi...@apache.org>.
shwstppr commented on code in PR #11567:
URL: https://github.com/apache/cloudstack/pull/11567#discussion_r2429722685
##########
server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java:
##########
@@ -980,7 +980,7 @@ public BackupProvider getBackupProvider(final String name) {
@Override
public List<Class<?>> getCommands() {
final List<Class<?>> cmdList = new ArrayList<Class<?>>();
- if (!BackupFrameworkEnabled.value()) {
+ if (!BackupFrameworkEnabled.value() && !BackupFrameworkEnabled.hasValueInScope(Boolean.TRUE.toString())) {
Review Comment:
We should not restrict API access if a zone has a backup framework disabled, but it is enabled globally. It might break UI/clients.
--
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] Enable backup framework when the config 'backup.framework.enabled' is set to true in Zone scope and not in Global setting [cloudstack]
Posted by "shwstppr (via GitHub)" <gi...@apache.org>.
shwstppr commented on code in PR #11567:
URL: https://github.com/apache/cloudstack/pull/11567#discussion_r2429722685
##########
server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java:
##########
@@ -980,7 +980,7 @@ public BackupProvider getBackupProvider(final String name) {
@Override
public List<Class<?>> getCommands() {
final List<Class<?>> cmdList = new ArrayList<Class<?>>();
- if (!BackupFrameworkEnabled.value()) {
+ if (!BackupFrameworkEnabled.value() && !BackupFrameworkEnabled.hasValueInScope(Boolean.TRUE.toString())) {
Review Comment:
We should not restrict API access if based on the zone config value. It might break UI/clients.
--
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] Check backup framework enabled config in zone scope first (before checking in global scope) [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #11567:
URL: https://github.com/apache/cloudstack/pull/11567#issuecomment-3397723774
@sureshanaparti , doesn’t `ConfigKey.velueInScope(Scope, Long)` already address this problem?
--
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] Check backup framework enabled config in zone scope first (before checking in global scope) [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #11567:
URL: https://github.com/apache/cloudstack/pull/11567#issuecomment-3397268922
> Hi @weizhouapache Yes, 'routed.network.vpc.enabled' is also Zone scoped, also has to be updated (at the below files/code) to work when 'routed.network.vpc.enabled' is set to true in Zone, and false in Global Settings. If it is set to true in Global Settings, all zones in the deployment are enabled with it by default when the respective Zone setting is not updated to false. Also, any relevant doc changes to be updated if required.
>
thanks @sureshanaparti for the research
can you check the PR #4202 ?
--
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] Update backup framework enabled config to global scope [cloudstack]
Posted by "abh1sar (via GitHub)" <gi...@apache.org>.
abh1sar commented on PR #11567:
URL: https://github.com/apache/cloudstack/pull/11567#issuecomment-3821870091
@shwstppr @weizhouapache are you ok with keeping the setting global?
--
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] Update backup framework enabled config to global scope [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #11567:
URL: https://github.com/apache/cloudstack/pull/11567#issuecomment-3270754373
Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 14918
--
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] Update backup framework enabled config to global scope [cloudstack]
Posted by "sureshanaparti (via GitHub)" <gi...@apache.org>.
sureshanaparti commented on PR #11567:
URL: https://github.com/apache/cloudstack/pull/11567#issuecomment-3270071955
@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] Enable backup framework when the config 'backup.framework.enabled' is set to true in Zone scope and not in Global setting [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #11567:
URL: https://github.com/apache/cloudstack/pull/11567#issuecomment-3661052334
@shwstppr @weizhouapache @abh1sar can you have another look, please?
--
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