Skip to content

[WIP] Preferred storage pool setting as a cluster setting #9564

New issue

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

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

Already on GitHub? Sign in to your account

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

nvazquez
Copy link
Contributor

Description

This PR changes the (preferred.storage.pool setting scope from Account to Cluster
Fixes: #8959

Types of changes

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

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

@nvazquez
Copy link
Contributor Author

@blueorangutan package

@blueorangutan
Copy link

@nvazquez 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.

Copy link

codecov bot commented Aug 21, 2024

Codecov Report

Attention: Patch coverage is 6.25000% with 15 lines in your changes missing coverage. Please review.

Project coverage is 16.18%. Comparing base (81e052c) to head (bb4fc89).

Files with missing lines Patch % Lines
...stack/engine/orchestration/VolumeOrchestrator.java 0.00% 8 Missing ⚠️
...om/cloud/deploy/DeploymentPlanningManagerImpl.java 0.00% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##               main    #9564   +/-   ##
=========================================
  Coverage     16.18%   16.18%           
  Complexity    13046    13046           
=========================================
  Files          5645     5645           
  Lines        494795   494803    +8     
  Branches      59955    59958    +3     
=========================================
+ Hits          80065    80076   +11     
+ Misses       405897   405893    -4     
- Partials       8833     8834    +1     
Flag Coverage Δ
uitests 4.01% <ø> (ø)
unittests 17.03% <6.25%> (+<0.01%) ⬆️

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

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

@blueorangutan
Copy link

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

@@ -174,8 +174,8 @@ public interface StorageManager extends StorageService {
"If set to true, the disk is created only when there is a suitable storage pool that supports the disk provisioning type specified by the service/disk offering. " +
"If set to false, the disk is created with a disk provisioning type supported by the pool. Default value is false, and this is currently supported for VMware only.",
true, ConfigKey.Scope.Zone);
ConfigKey<String> PreferredStoragePool = new ConfigKey<String>(String.class, "preferred.storage.pool", "Advanced", "",
"The UUID of preferred storage pool for allocation.", true, ConfigKey.Scope.Account, null);
ConfigKey<String> PreferredStoragePool = new ConfigKey<>(String.class, "preferred.storage.pool", "Advanced", "",
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we can have two configurations with same key

  • 1 for global/zone/cluster
  • 2 for global/domain/account

changing from Account to Cluster may be disliked by some users (maybe)

@nvazquez
Copy link
Contributor Author

@blueorangutan test

@blueorangutan
Copy link

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

@blueorangutan
Copy link

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

Test Result Time (s) Test File

Copy link

github-actions bot commented Jan 8, 2025

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

@DaanHoogland
Copy link
Contributor

@nvazquez should this target 4.19?

@nvazquez
Copy link
Contributor Author

@DaanHoogland the issue this PR fixes was on the 4.19.2 milestone but I think it shouldn't be there as 4.20.0 is already out and changing the scope of the setting would be conflicting, perhaps it should be targeted to 4.20 instead of main?

@DaanHoogland
Copy link
Contributor

@nvazquez we can change to v21 as well as it is an enhancement...?

@nvazquez nvazquez added this to the 4.21.0 milestone Jan 23, 2025
@nvazquez
Copy link
Contributor Author

Sure @DaanHoogland fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

VM allocation on Primary Storage
4 participants