nvazquez commented on code in PR #7712: URL: https://github.com/apache/cloudstack/pull/7712#discussion_r1306489645
########## plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtGetUnmanagedInstancesCommandWrapper.java: ########## @@ -0,0 +1,220 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package com.cloud.hypervisor.kvm.resource.wrapper; + +import com.cloud.agent.api.GetUnmanagedInstancesAnswer; +import com.cloud.agent.api.GetUnmanagedInstancesCommand; +import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource; +import com.cloud.hypervisor.kvm.resource.LibvirtDomainXMLParser; +import com.cloud.hypervisor.kvm.resource.LibvirtVMDef; +import com.cloud.resource.CommandWrapper; +import com.cloud.resource.ResourceWrapper; +import com.cloud.utils.Pair; +import com.cloud.utils.exception.CloudRuntimeException; +import com.cloud.vm.VirtualMachine; +import org.apache.cloudstack.utils.qemu.QemuImg; +import org.apache.cloudstack.utils.qemu.QemuImgException; +import org.apache.cloudstack.utils.qemu.QemuImgFile; +import org.apache.cloudstack.vm.UnmanagedInstanceTO; +import org.apache.commons.lang3.StringUtils; +import org.apache.log4j.Logger; +import org.libvirt.Connect; +import org.libvirt.Domain; +import org.libvirt.LibvirtException; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +@ResourceWrapper(handles= GetUnmanagedInstancesCommand.class) +public final class LibvirtGetUnmanagedInstancesCommandWrapper extends CommandWrapper<GetUnmanagedInstancesCommand, GetUnmanagedInstancesAnswer, LibvirtComputingResource> { + private static final Logger s_logger = Logger.getLogger(LibvirtPrepareUnmanageVMInstanceCommandWrapper.class); + + @Override + public GetUnmanagedInstancesAnswer execute(GetUnmanagedInstancesCommand command, LibvirtComputingResource libvirtComputingResource) { + s_logger.info("Need to implement business logic"); Review Comment: Outdated comment :) ########## api/src/main/java/com/cloud/vm/UserVmService.java: ########## @@ -422,6 +429,9 @@ UserVm createAdvancedVirtualMachine(DataCenter zone, ServiceOffering serviceOffe UserVm createVirtualMachine(DeployVMCmd cmd) throws InsufficientCapacityException, ResourceUnavailableException, ConcurrentOperationException, StorageUnavailableException, ResourceAllocationException; +// +// UserVm createVirtualMachineToImport(ImportUnmanagedInstanceCmd cmd, UnmanagedInstanceTO unmanagedInstance, Host host) throws InsufficientCapacityException, ResourceUnavailableException, ConcurrentOperationException, Review Comment: Same here ########## server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java: ########## @@ -302,6 +306,7 @@ private List<String> getAdditionalNameFilters(Cluster cluster) { if (cluster == null) { return additionalNameFilter; } + // TODO: Ayush - invesgigate KVM specific changes Review Comment: Same here :) ########## server/src/main/java/com/cloud/vm/UserVmManagerImpl.java: ########## @@ -4760,15 +4762,14 @@ protected String validateUserData(String userData, HTTPMethod httpmethod) { } @Override - @ActionEvent(eventType = EventTypes.EVENT_VM_CREATE, eventDescription = "starting Vm", async = true) + @ActionEvent(eventType = EventTypes.EVENT_VM_START, eventDescription = "starting Vm", async = true) Review Comment: This seems correct however it may impact on usage, opinions? @DaanHoogland @rohityadavcloud @harikrishna-patnala @weizhouapache ########## plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java: ########## @@ -1185,7 +1196,6 @@ enum HostNicType { private String _sourceName; private String _networkName; private String _macAddr; - private String _ipAddr; Review Comment: Why is this being removed? ########## api/src/main/java/com/cloud/vm/UserVmService.java: ########## @@ -21,6 +21,7 @@ import org.apache.cloudstack.api.BaseCmd.HTTPMethod; import org.apache.cloudstack.api.command.admin.vm.AssignVMCmd; +//import org.apache.cloudstack.api.command.admin.vm.ImportUnmanagedInstanceCmd; Review Comment: Lets remove commented code ########## server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java: ########## @@ -1211,30 +1257,13 @@ public UserVmResponse importUnmanagedInstance(ImportUnmanagedInstanceCmd cmd) { throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, String.format("Unable to retrieve details for unmanaged VM: %s", name)); } if (template.getName().equals(VM_IMPORT_DEFAULT_TEMPLATE_NAME)) { - String osName = unmanagedInstance.getOperatingSystem(); - GuestOS guestOS = null; - if (StringUtils.isNotEmpty(osName)) { - guestOS = guestOSDao.listByDisplayName(osName); - } - GuestOSHypervisor guestOSHypervisor = null; - if (guestOS != null) { - guestOSHypervisor = guestOSHypervisorDao.findByOsIdAndHypervisor(guestOS.getId(), host.getHypervisorType().toString(), host.getHypervisorVersion()); - } - if (guestOSHypervisor == null && StringUtils.isNotEmpty(unmanagedInstance.getOperatingSystemId())) { - guestOSHypervisor = guestOSHypervisorDao.findByOsNameAndHypervisor(unmanagedInstance.getOperatingSystemId(), host.getHypervisorType().toString(), host.getHypervisorVersion()); - } - if (guestOSHypervisor == null) { - if (guestOS != null) { - throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, String.format("Unable to find hypervisor guest OS ID: %s details for unmanaged VM: %s for hypervisor: %s version: %s. templateid parameter can be used to assign template for VM", guestOS.getUuid(), name, host.getHypervisorType().toString(), host.getHypervisorVersion())); - } - throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, String.format("Unable to retrieve guest OS details for unmanaged VM: %s with OS name: %s, OS ID: %s for hypervisor: %s version: %s. templateid parameter can be used to assign template for VM", name, osName, unmanagedInstance.getOperatingSystemId(), host.getHypervisorType().toString(), host.getHypervisorVersion())); - } - template.setGuestOSId(guestOSHypervisor.getGuestOsId()); + throw new InvalidParameterValueException("Template is needed and unable to use default template for hypervisor " + host.getHypervisorType().toString()); Review Comment: This would create regressions on Vmware imports as they support the default template, can you add a check for the hypervisor type on this logic? ########## server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java: ########## @@ -907,10 +925,33 @@ private void publishVMUsageUpdateResourceCount(final UserVm userVm, ServiceOffer } private UserVm importVirtualMachineInternal(final UnmanagedInstanceTO unmanagedInstance, final String instanceName, final DataCenter zone, final Cluster cluster, final HostVO host, - final VirtualMachineTemplate template, final String displayName, final String hostName, final Account caller, final Account owner, final Long userId, + final VMTemplateVO template, final String displayName, final String hostName, final Account caller, final Account owner, final Long userId, Review Comment: Can we keep `VirtualMachineTemplate` here? `VMTemplateVO` is an implementation of that interface, if the explicit cast is needed it can be done previously checking with a check such as: `if (template instanceof VMTemplateVO) ....` ########## server/src/main/java/com/cloud/vm/UserVmManagerImpl.java: ########## @@ -5813,6 +5814,66 @@ public UserVm createVirtualMachine(DeployVMCmd cmd) throws InsufficientCapacityE return vm; } +// @Override +// public UserVm createVirtualMachineToImport( Review Comment: Commented code ########## server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java: ########## @@ -927,6 +968,7 @@ private UserVm importVirtualMachineInternal(final UnmanagedInstanceTO unmanagedI } Map<String, String> allDetails = new HashMap<>(details); if (validatedServiceOffering.isDynamic()) { + allDetails.put(VmDetailConstants.KVM_IMPORT_DYNAMIC_SCALING, String.valueOf(true)); Review Comment: Perhaps would be safer to assume it as false here -- 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]
