GabrielBrascher commented on a change in pull request #5149:
URL: https://github.com/apache/cloudstack/pull/5149#discussion_r662457986
##########
File path:
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java
##########
@@ -2239,211 +2305,309 @@ public LibvirtVMDef createVMFromSpec(final
VirtualMachineTO vmTO) {
Map<String, String> customParams = vmTO.getDetails();
boolean isUefiEnabled = false;
boolean isSecureBoot = false;
- String bootMode =null;
+ String bootMode = null;
+
if (MapUtils.isNotEmpty(customParams) &&
customParams.containsKey(GuestDef.BootType.UEFI.toString())) {
isUefiEnabled = true;
- bootMode = customParams.get(GuestDef.BootType.UEFI.toString());
- if (StringUtils.isNotBlank(bootMode) &&
"secure".equalsIgnoreCase(bootMode)) {
+ s_logger.debug(String.format("Enabled UEFI for VM UUID [%s].",
uuid));
+
+ if
(isSecureMode(customParams.get(GuestDef.BootType.UEFI.toString()))) {
+ s_logger.debug(String.format("Enabled Secure Boot for VM UUID
[%s].", uuid));
isSecureBoot = true;
}
}
Map<String, String> extraConfig = vmTO.getExtraConfig();
if (dpdkSupport && (!extraConfig.containsKey(DpdkHelper.DPDK_NUMA) ||
!extraConfig.containsKey(DpdkHelper.DPDK_HUGE_PAGES))) {
- s_logger.info("DPDK is enabled but it needs extra configurations
for CPU NUMA and Huge Pages for VM deployment");
+ s_logger.info(String.format("DPDK is enabled for VM [%s], but it
needs extra configurations for CPU NUMA and Huge Pages for VM deployment.",
vmTO.toString()));
}
+ configureVM(vmTO, vm, customParams, isUefiEnabled, isSecureBoot,
bootMode, extraConfig, uuid);
+ return vm;
+ }
- final GuestDef guest = new GuestDef();
+ /**
+ * Configure created VM from specification, adding the necessary
components to VM.
+ */
+ private void configureVM(VirtualMachineTO vmTO, LibvirtVMDef vm,
Map<String, String> customParams, boolean isUefiEnabled, boolean isSecureBoot,
String bootMode,
+ Map<String, String> extraConfig, String uuid) {
+ s_logger.debug(String.format("Configuring VM with UUID [%s].", uuid));
- if (HypervisorType.LXC == _hypervisorType && VirtualMachine.Type.User
== vmTO.getType()) {
- // LXC domain is only valid for user VMs. Use KVM for system VMs.
- guest.setGuestType(GuestDef.GuestType.LXC);
- vm.setHvsType(HypervisorType.LXC.toString().toLowerCase());
- } else {
- guest.setGuestType(GuestDef.GuestType.KVM);
- vm.setHvsType(HypervisorType.KVM.toString().toLowerCase());
- vm.setLibvirtVersion(_hypervisorLibvirtVersion);
- vm.setQemuVersion(_hypervisorQemuVersion);
+ GuestDef guest = createGuestFromSpec(vmTO, vm, uuid, customParams);
+ if (isUefiEnabled) {
+ configureGuestIfUefiEnabled(isSecureBoot, bootMode, guest);
}
- guest.setGuestArch(_guestCpuArch != null ? _guestCpuArch :
vmTO.getArch());
- guest.setMachineType(_guestCpuArch != null &&
_guestCpuArch.equals("aarch64") ? "virt" : "pc");
- guest.setBootType(GuestDef.BootType.BIOS);
- if (MapUtils.isNotEmpty(customParams) &&
customParams.containsKey(GuestDef.BootType.UEFI.toString())) {
- guest.setBootType(GuestDef.BootType.UEFI);
- guest.setBootMode(GuestDef.BootMode.LEGACY);
- guest.setMachineType("q35");
- if
(StringUtils.isNotBlank(customParams.get(GuestDef.BootType.UEFI.toString())) &&
"secure".equalsIgnoreCase(customParams.get(GuestDef.BootType.UEFI.toString())))
{
- guest.setBootMode(GuestDef.BootMode.SECURE); // setting to
secure mode
- }
+
+ vm.addComp(guest);
+ vm.addComp(createGuestResourceDef(vmTO));
+
+ int vcpus = vmTO.getCpus();
+ if (!extraConfig.containsKey(DpdkHelper.DPDK_NUMA)) {
+ vm.addComp(createCpuModeDef(vmTO, vcpus));
}
- guest.setUuid(uuid);
- guest.setBootOrder(GuestDef.BootOrder.CDROM);
- guest.setBootOrder(GuestDef.BootOrder.HARDISK);
- if (isUefiEnabled) {
- if (_uefiProperties.getProperty(GuestDef.GUEST_LOADER_SECURE) !=
null && "secure".equalsIgnoreCase(bootMode)) {
-
guest.setLoader(_uefiProperties.getProperty(GuestDef.GUEST_LOADER_SECURE));
- }
+ if (_hypervisorLibvirtVersion >=
MIN_LIBVIRT_VERSION_FOR_GUEST_CPU_TUNE) {
+ vm.addComp(createCpuTuneDef(vmTO));
+ }
- if (_uefiProperties.getProperty(GuestDef.GUEST_LOADER_LEGACY) !=
null && "legacy".equalsIgnoreCase(bootMode)) {
-
guest.setLoader(_uefiProperties.getProperty(GuestDef.GUEST_LOADER_LEGACY));
- }
+ FeaturesDef features = createFeaturesDef(customParams, isUefiEnabled,
isSecureBoot);
+ enlightenWindowsVm(vmTO, features);
+ vm.addComp(features);
- if (_uefiProperties.getProperty(GuestDef.GUEST_NVRAM_PATH) !=
null) {
-
guest.setNvram(_uefiProperties.getProperty(GuestDef.GUEST_NVRAM_PATH));
- }
+ vm.addComp(createTermPolicy());
+ vm.addComp(createClockDef(vmTO));
+ vm.addComp(createDevicesDef(vmTO, guest, vcpus, isUefiEnabled));
- if (isSecureBoot) {
- if
(_uefiProperties.getProperty(GuestDef.GUEST_NVRAM_TEMPLATE_SECURE) != null &&
"secure".equalsIgnoreCase(bootMode)) {
-
guest.setNvramTemplate(_uefiProperties.getProperty(GuestDef.GUEST_NVRAM_TEMPLATE_SECURE));
- }
- } else {
- if
(_uefiProperties.getProperty(GuestDef.GUEST_NVRAM_TEMPLATE_LEGACY) != null) {
-
guest.setNvramTemplate(_uefiProperties.getProperty(GuestDef.GUEST_NVRAM_TEMPLATE_LEGACY));
- }
- }
+ addExtraConfigsToVM(vmTO, vm, extraConfig);
+ }
+
+ /**
+ * Add extra configuration to User VM Domain XML before starting.
+ */
+ private void addExtraConfigsToVM(VirtualMachineTO vmTO, LibvirtVMDef vm,
Map<String, String> extraConfig) {
+ if (MapUtils.isNotEmpty(extraConfig) &&
VirtualMachine.Type.User.equals(vmTO.getType())) {
+ s_logger.debug(String.format("Appending extra configuration data
[%s] to guest VM [%s] domain XML.", extraConfig, vmTO.toString()));
+ addExtraConfigComponent(extraConfig, vm);
}
+ }
- vm.addComp(guest);
+ /**
+ * Add devices components to VM.
+ */
+ protected DevicesDef createDevicesDef(VirtualMachineTO vmTO, GuestDef
guest, int vcpus, boolean isUefiEnabled) {
+ DevicesDef devices = new DevicesDef();
+ devices.setEmulatorPath(_hypervisorPath);
+ devices.setGuestType(guest.getGuestType());
+ devices.addDevice(createSerialDef());
- final GuestResourceDef grd = new GuestResourceDef();
+ if (_rngEnable) {
+ devices.addDevice(createRngDef());
+ }
- if (vmTO.getMinRam() != vmTO.getMaxRam() && !_noMemBalloon) {
- grd.setMemBalloning(true);
- grd.setCurrentMem(vmTO.getMinRam() / 1024);
- grd.setMemorySize(vmTO.getMaxRam() / 1024);
- } else {
- grd.setMemorySize(vmTO.getMaxRam() / 1024);
+ devices.addDevice(createChannelDef(vmTO));
+ devices.addDevice(createWatchDogDef());
+ devices.addDevice(createVideoDef());
+ devices.addDevice(createConsoleDef());
+ devices.addDevice(createGraphicDef(vmTO));
+ devices.addDevice(createTabletInputDef());
+
+ if (isGuestAarch64()) {
+ createArm64UsbDef(devices);
}
- final int vcpus = vmTO.getCpus();
- grd.setVcpuNum(vcpus);
- vm.addComp(grd);
- if (!extraConfig.containsKey(DpdkHelper.DPDK_NUMA)) {
- final CpuModeDef cmd = new CpuModeDef();
- cmd.setMode(_guestCpuMode);
- cmd.setModel(_guestCpuModel);
- if (vmTO.getType() == VirtualMachine.Type.User) {
- cmd.setFeatures(_cpuFeatures);
- }
- setCpuTopology(cmd, vcpus, vmTO.getDetails());
- vm.addComp(cmd);
+ DiskDef.DiskBus busT = getDiskModelFromVMDetail(vmTO);
+ if (busT == null) {
+ busT = getGuestDiskModel(vmTO.getPlatformEmulator(),
isUefiEnabled);
+ }
+
+ if (busT == DiskDef.DiskBus.SCSI) {
+ devices.addDevice(createSCSIDef(vcpus));
}
+ return devices;
+ }
- if (_hypervisorLibvirtVersion >= 9000) {
- final CpuTuneDef ctd = new CpuTuneDef();
- /**
- A 4.0.X/4.1.X management server doesn't send the correct JSON
- command for getMinSpeed, it only sends a 'speed' field.
+ protected WatchDogDef createWatchDogDef() {
+ return new WatchDogDef(_watchDogAction, _watchDogModel);
+ }
- So if getMinSpeed() returns null we fall back to getSpeed().
+ /*
+ * Add an explicit USB devices for ARM64
+ */
+ protected void createArm64UsbDef(DevicesDef devices) {
+ devices.addDevice(new InputDef(KEYBOARD, USB));
+ devices.addDevice(new InputDef(MOUSE, USB));
+ devices.addDevice(new LibvirtVMDef.USBDef((short)0, 0, 5, 0, 0));
+ }
- This way a >4.1 agent can work communicate a <=4.1 management
server
+ protected InputDef createTabletInputDef() {
+ return new InputDef(TABLET, USB);
+ }
- This change is due to the overcommit feature in 4.2
- */
- if (vmTO.getMinSpeed() != null) {
- ctd.setShares(vmTO.getCpus() * vmTO.getMinSpeed());
- } else {
- ctd.setShares(vmTO.getCpus() * vmTO.getSpeed());
- }
+ /**
+ * Add the VNC port passwd here, get the passwd from the vmInstance.
+ */
+ protected GraphicDef createGraphicDef(VirtualMachineTO vmTO) {
+ return new GraphicDef(VNC, (short)0, true, vmTO.getVncAddr(),
vmTO.getVncPassword(), null);
+ }
- setQuotaAndPeriod(vmTO, ctd);
+ /**
+ * Add a VirtIO channel for the Qemu Guest Agent tools.
+ */
+ protected ChannelDef createChannelDef(VirtualMachineTO vmTO) {
+ File virtIoChannel = Paths.get(_qemuSocketsPath.getPath(),
vmTO.getName() + "." + _qemuGuestAgentSocketName).toFile();
+ return new ChannelDef(_qemuGuestAgentSocketName,
ChannelDef.ChannelType.UNIX, virtIoChannel);
+ }
- vm.addComp(ctd);
- }
+ /**
+ * If we're using virtio scsi, then we need to add a virtual scsi
controller.
+ */
+ protected SCSIDef createSCSIDef(int vcpus) {
+ return new SCSIDef((short)0, 0, 0, 9, 0, vcpus);
+ }
- final FeaturesDef features = new FeaturesDef();
- features.addFeatures("pae");
- features.addFeatures("apic");
- features.addFeatures("acpi");
- if (isUefiEnabled &&
isSecureMode(customParams.get(GuestDef.BootType.UEFI.toString()))) {
- features.addFeatures("smm");
- }
+ protected ConsoleDef createConsoleDef() {
+ return new ConsoleDef(PTY, null, null, (short)0);
+ }
- //KVM hyperv enlightenment features based on OS Type
- enlightenWindowsVm(vmTO, features);
+ protected VideoDef createVideoDef() {
+ return new VideoDef(_videoHw, _videoRam);
+ }
- vm.addComp(features);
+ protected RngDef createRngDef() {
+ return new RngDef(_rngPath, _rngBackendModel, _rngRateBytes,
_rngRatePeriod);
+ }
- final TermPolicy term = new TermPolicy();
- term.setCrashPolicy("destroy");
- term.setPowerOffPolicy("destroy");
- term.setRebootPolicy("restart");
- vm.addComp(term);
+ protected SerialDef createSerialDef() {
+ return new SerialDef(PTY, null, (short)0);
+ }
- final ClockDef clock = new ClockDef();
- if (vmTO.getOs().startsWith("Windows")) {
+ protected ClockDef createClockDef(final VirtualMachineTO vmTO) {
+ ClockDef clock = new ClockDef();
+ if (org.apache.commons.lang.StringUtils.startsWith(vmTO.getOs(),
WINDOWS)) {
clock.setClockOffset(ClockDef.ClockOffset.LOCALTIME);
- clock.setTimer("hypervclock", null, null);
- } else if (vmTO.getType() != VirtualMachine.Type.User ||
isGuestPVEnabled(vmTO.getOs())) {
- if (_hypervisorLibvirtVersion >= 9 * 1000 + 10) {
- clock.setTimer("kvmclock", null, null, _noKvmClock);
- }
+ clock.setTimer(HYPERVCLOCK, null, null);
+ } else if ((vmTO.getType() != VirtualMachine.Type.User ||
isGuestPVEnabled(vmTO.getOs())) && _hypervisorLibvirtVersion >=
MIN_LIBVIRT_VERSION_FOR_GUEST_CPU_MODE) {
Review comment:
That's a good question @SadiJr. In fact, some code extractions have
little to do with duplicated code but with making cleaner code and also helping
with unit test coverage.
For instance, there are cases where conditionals can be replaced by
polymorphism. In some cases, it is also possible to replace a conditional with
map/set. As an example:
```
if (var == value1 || var == value2 || ... var == valueN) {
<body>
}
```
:arrow_down:
```
/**
* This JavaDoc might help. But if the variable name is self-explanatory
this Javadoc might be unnecessary.
*/
final static Set<Type> GOOD_VAR_NAME = new HashSet<>(Arrays.asList(value1,
value2, ..., valueN));
....
protected void method() {
....
if (GOOD_VAR_NAME.contains(var)) {
<body>
}
}
```
Therefore, the goal of such extraction would be to:
1. keep code clean
2. add documentation (or not)
3. allow easy JUnit test that covers such if-conditional
You can proceed with the code as it is or look for some alternatives that
might improve this IF. I would not block or judge if you keep it as it is. What
matters the most is to always try to keep such care when developing/reviewing.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
