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.

Sign up for GitHub

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

Jump to bottom

Refactor configuration SystemVMDefaultHypervisor #8934

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

Refactor configuration SystemVMDefaultHypervisor #8934

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions engine/components-api/src/main/java/com/cloud/resource/ResourceManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,11 @@ public interface ResourceManager extends ResourceService, Configurable {
+ "To force-stop VMs, choose 'ForceStop' strategy",
true, ConfigKey.Scope.Global, null, null, null, null, null, ConfigKey.Kind.Select, "Error,Migration,ForceStop");

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 •
edited
Loading

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.


/**
* Register a listener for different types of resource life cycle events.
* There can only be one type of listener per type of host.
Expand Down
7 changes: 0 additions & 7 deletions server/src/main/java/com/cloud/configuration/Config.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -644,13 +644,6 @@ public enum Config {
"true",
"Indicates whether or not to automatically reserver system VM standby capacity.",
null),
SystemVMDefaultHypervisor("Advanced",
ManagementServer.class,
String.class,
"system.vm.default.hypervisor",
null,
"Hypervisor type used to create system vm, valid values are: XenServer, KVM, VMware, Hyperv, VirtualBox, Parralels, BareMetal, Ovm, LXC, Any",
null),
SystemVMRandomPassword(
"Advanced",
ManagementServer.class,
Expand Down
9 changes: 2 additions & 7 deletions server/src/main/java/com/cloud/resource/ResourceManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,6 @@
private final HashMap<String, ResourceStateAdapter> _resourceStateAdapters = new HashMap<String, ResourceStateAdapter>();

private final HashMap<Integer, List<ResourceListener>> _lifeCycleListeners = new HashMap<Integer, List<ResourceListener>>();
private HypervisorType _defaultSystemVMHypervisor;

private static final int ACQUIRE_GLOBAL_LOCK_TIMEOUT_FOR_COOPERATION = 30; // seconds

Expand Down Expand Up @@ -2031,7 +2030,6 @@

@Override
public boolean configure(final String name, final Map<String, Object> params) throws ConfigurationException {
_defaultSystemVMHypervisor = HypervisorType.getType(_configDao.getValue(Config.SystemVMDefaultHypervisor.toString()));
_gson = GsonHelper.getGson();

_hypervisorsInDC = _hostDao.createSearchBuilder(String.class);
Expand Down Expand Up @@ -2077,10 +2075,7 @@

@Override
public HypervisorType getDefaultHypervisor(final long zoneId) {
HypervisorType defaultHyper = HypervisorType.None;
if (_defaultSystemVMHypervisor != HypervisorType.None) {
defaultHyper = _defaultSystemVMHypervisor;
}
HypervisorType defaultHyper = HypervisorType.getType(ResourceManager.SystemVMDefaultHypervisor.value());

Check warning on line 2078 in server/src/main/java/com/cloud/resource/ResourceManagerImpl.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/com/cloud/resource/ResourceManagerImpl.java#L2078 

Added line #L2078 was not covered by tests

final DataCenterVO dc = _dcDao.findById(zoneId);
if (dc == null) {
Expand Down Expand Up @@ -3567,6 +3562,6 @@

@Override
public ConfigKey<?>[] getConfigKeys() {
return new ConfigKey<?>[] {KvmSshToAgentEnabled, HOST_MAINTENANCE_LOCAL_STRATEGY};
return new ConfigKey<?>[] {KvmSshToAgentEnabled, HOST_MAINTENANCE_LOCAL_STRATEGY, SystemVMDefaultHypervisor};

Check warning on line 3565 in server/src/main/java/com/cloud/resource/ResourceManagerImpl.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/com/cloud/resource/ResourceManagerImpl.java#L3565 

Added line #L3565 was not covered by tests
}
}
Loading