[GitHub] [cloudstack] GutoVeronezi commented on a change in pull request #5149: Refactor and improvements for method com.cloud.hypervisor.kvm.resource.LibvirtComputingResource.createVMFromSpec()

[GitBox <gi...@apache.org>]

GutoVeronezi commented on a change in pull request #5149:
URL: https://github.com/apache/cloudstack/pull/5149#discussion_r658003465



##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java
##########
@@ -2239,211 +2305,303 @@ 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));
-                }
-            }
+     // Add extra configuration to User VM Domain XML before starting
+        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);
+        }

Review comment:
       We can extract this code to a method and turn the comment into a javadoc.




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org