Skip to content
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

Refactor configuration SystemVMDefaultHypervisor #8934

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

GaOrtiga
Copy link
Contributor

Description

The configuration SystemVMDefaultHypervisor is not dynamic, it is necessary to restart the management server after each alteration to its value. Besides, its input is a String, which forces the user to type in the name of the chosen hypervisor, being subject to typos.

This configuration was refactored, allowing its value to be altered in runtime; besides, its input was converted to a select, in order to facilitate the process of choosing the new value.

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

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

How Has This Been Tested?

I changed the values of the configurations and verified that the default hypervisors for new system vms changed accordingly

@GaOrtiga GaOrtiga changed the title refactor configuration SystemVMDefaultHypervisor Refactor configuration SystemVMDefaultHypervisor Apr 17, 2024
ConfigKey<String> SystemVMDefaultHypervisor = new ConfigKey<String>(String.class,
"system.vm.default.hypervisor", "Advanced", "Any", "Hypervisor type used to create System VMs. Valid values are: XenServer, KVM, VMware, Hyperv, VirtualBox, " +
"Parralels, BareMetal, Ovm, LXC, Any", true, ConfigKey.Scope.Global, null, null, null, null, null, ConfigKey.Kind.Select, "XenServer, KVM, VMware, Hyperv, " +
"VirtualBox, Parralels, BareMetal, Ovm, LXC, Any");
Copy link
Contributor

@sureshanaparti sureshanaparti Apr 18, 2024

Choose a reason for hiding this comment

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

as this is moved to configkey, can change the scope to zone, and handle zone detail "defaultSystemVMHypervisorType" in upgrade?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sureshanaparti I agree with changing the scope to zone, however, can you further explain what you mean by handling "defaultSystemVMHypervisorType" in upgrade?

Wouldn't changing the scope to zone and fetching the value of the config for the specific zone be sufficient to guarantee the expected behaviour?

Copy link
Member

Choose a reason for hiding this comment

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

@GaOrtiga
should the default value be "None" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@weizhouapache
Yes, the default must be "None" to keep the current behaviour. However, since this configuration is only setting a default, "None" and "Any" have the same behaviour.

Copy link
Member

Choose a reason for hiding this comment

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

@GaOrtiga
I quickly checked the code. it looks like the default value is NULL which is same as "None"
I suggest we keep it empty or None.

@sureshanaparti
Copy link
Contributor

@blueorangutan package

@blueorangutan
Copy link

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

@blueorangutan
Copy link

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

@DaanHoogland
Copy link
Contributor

@blueorangutan test

@blueorangutan
Copy link

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

@blueorangutan
Copy link

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

Test Result Time (s) Test File
test_01_events_resource Error 300.66 test_events_resource.py
test_01_events_resource Error 300.67 test_events_resource.py
test_04_deploy_vm_for_other_user_and_test_vm_operations Failure 92.05 test_network_permissions.py
ContextSuite context=TestNetworkPermissions>:teardown Error 1.45 test_network_permissions.py

@BryanMLima BryanMLima closed this Jun 12, 2024
@BryanMLima BryanMLima reopened this Jun 12, 2024
@BryanMLima
Copy link
Contributor

@blueorangutan package

@blueorangutan
Copy link

@BryanMLima 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 Jun 12, 2024

Codecov Report

Attention: Patch coverage is 33.33333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 15.81%. Comparing base (b19c069) to head (13becce).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...n/java/com/cloud/resource/ResourceManagerImpl.java 0.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##               main    #8934     +/-   ##
===========================================
  Coverage     15.81%   15.81%             
  Complexity    12554    12554             
===========================================
  Files          5629     5629             
  Lines        492028   492026      -2     
  Branches      62750    61393   -1357     
===========================================
  Hits          77813    77813             
+ Misses       405892   405890      -2     
  Partials       8323     8323             
Flag Coverage Δ
uitests 4.48% <ø> (ø)
unittests 16.60% <33.33%> (+<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]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 9908

@JoaoJandre
Copy link
Contributor

@blueorangutan package

@blueorangutan
Copy link

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

@blueorangutan
Copy link

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

@GaOrtiga GaOrtiga force-pushed the Refactor_config_SystemVMDefaultHypervisor branch from 9d226fe to 13becce Compare September 12, 2024 17:01
@GaOrtiga
Copy link
Contributor Author

@blueorangutan package

@blueorangutan
Copy link

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

@blueorangutan
Copy link

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

@DaanHoogland
Copy link
Contributor

@GaOrtiga @BryanMLima what is the status of this PR?
@sureshanaparti @weizhouapache , any concerns to be dealt with?

@GaOrtiga
Copy link
Contributor Author

@GaOrtiga @BryanMLima what is the status of this PR? @sureshanaparti @weizhouapache , any concerns to be dealt with?

It is ready for review.

@DaanHoogland
Copy link
Contributor

@blueorangutan package

@blueorangutan
Copy link

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

@blueorangutan
Copy link

Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 11154

@blueorangutan
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants