You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by GitBox <gi...@apache.org> on 2021/07/27 17:48:03 UTC

[GitHub] [cloudstack] GutoVeronezi commented on a change in pull request #5216: Support for new Feature: Clone a Virtual Machine (#4818)

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



##########
File path: api/src/main/java/org/apache/cloudstack/api/command/user/vm/CloneVMCmd.java
##########
@@ -0,0 +1,193 @@
+package org.apache.cloudstack.api.command.user.vm;
+
+import com.cloud.event.EventTypes;
+import com.cloud.exception.ConcurrentOperationException;
+import com.cloud.exception.InsufficientCapacityException;
+import com.cloud.exception.InvalidParameterValueException;
+import com.cloud.exception.ResourceAllocationException;
+import com.cloud.exception.ResourceUnavailableException;
+import com.cloud.template.VirtualMachineTemplate;
+import com.cloud.user.Account;
+import com.cloud.uservm.UserVm;
+import com.cloud.utils.exception.CloudRuntimeException;
+import com.cloud.vm.VirtualMachine;
+import org.apache.cloudstack.acl.SecurityChecker.AccessType;
+import org.apache.cloudstack.api.ACL;
+import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiCommandJobType;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.ApiErrorCode;
+import org.apache.cloudstack.api.BaseAsyncCreateCustomIdCmd;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.ResponseObject;
+import org.apache.cloudstack.api.ServerApiException;
+import org.apache.cloudstack.api.command.user.UserCmd;
+import org.apache.cloudstack.api.response.DomainResponse;
+import org.apache.cloudstack.api.response.UserVmResponse;
+//import org.apache.cloudstack.context.CallContext;
+import org.apache.cloudstack.context.CallContext;
+import org.apache.log4j.Logger;
+
+import java.util.Optional;
+
+@APICommand(name = "cloneVirtualMachine", responseObject = UserVmResponse.class, description = "clone a virtual VM in full clone mode",
+        responseView = ResponseObject.ResponseView.Restricted, requestHasSensitiveInfo = false, responseHasSensitiveInfo = true, entityType = {VirtualMachine.class})
+public class CloneVMCmd extends BaseAsyncCreateCustomIdCmd implements UserCmd {
+    public static final Logger s_logger = Logger.getLogger(CloneVMCmd.class.getName());
+    private static final String s_name = "clonevirtualmachineresponse";
+
+    /////////////////////////////////////////////////////
+    //////////////// API parameters /////////////////////
+    /////////////////////////////////////////////////////
+    @ACL(accessType = AccessType.OperateEntry)
+    @Parameter(name = ApiConstants.VIRTUAL_MACHINE_ID, type = CommandType.UUID, entityType=UserVmResponse.class,
+            required = true, description = "The ID of the virtual machine")
+    private Long id;
+
+    //Owner information
+    @Parameter(name = ApiConstants.ACCOUNT, type = CommandType.STRING, description = "an optional account for the virtual machine. Must be used with domainId.")
+    private String accountName;
+
+    @Parameter(name = ApiConstants.DOMAIN_ID, type = CommandType.UUID, entityType = DomainResponse.class, description = "an optional domainId for the virtual machine. If the account parameter is used, domainId must also be used.")
+    private Long domainId;
+
+    private Long temporaryTemlateId;
+
+    private Long temporarySnapShotId;
+
+    public String getAccountName() {
+        return accountName;
+    }
+
+    public Long getDomainId() {
+        return domainId;
+    }
+
+    public Long getId() {
+        return this.id;
+    }
+    @Override
+    public String getEventType() {
+        return EventTypes.EVENT_VM_CLONE;
+    }
+
+    @Override
+    public ApiCommandJobType getInstanceType() {
+        return ApiCommandJobType.VirtualMachine;
+    }
+
+    @Override
+    public String getEventDescription() {
+        return "Cloning user VM: " + this._uuidMgr.getUuid(VirtualMachine.class, getId());
+    }
+
+    public Long getTemporaryTemlateId() {
+        return this.temporaryTemlateId;
+    }
+
+    public void setTemporarySnapShotId(Long snapshotId) {
+        this.temporarySnapShotId = snapshotId;
+    }
+
+    public Long getTemporarySnapShotId() {
+        return temporarySnapShotId;
+    }
+
+
+    public void setTemporaryTemlateId(long tempId) {
+        this.temporaryTemlateId = tempId;
+    }
+
+    @Override
+    public void create() throws ResourceAllocationException {
+        try {
+            _userVmService.checkCloneCondition(this);
+            VirtualMachineTemplate template = _templateService.createPrivateTemplateRecord(this, _accountService.getAccount(getEntityOwnerId()), _volumeService);
+            if (template == null) {
+                throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, "failed to create a template to db");
+            }
+            s_logger.info("The template id recorded is: " + template.getId());
+            setTemporaryTemlateId(template.getId());
+            _templateService.createPrivateTemplate(this);
+            UserVm vmRecord = _userVmService.recordVirtualMachineToDB(this);
+            if (vmRecord == null) {
+                throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, "unable to record a new VM to db!");
+            }
+            setEntityId(vmRecord.getId());
+            setEntityUuid(vmRecord.getUuid());
+        }
+        catch (ResourceUnavailableException | InsufficientCapacityException e) {
+            s_logger.warn("Exception: ", e);
+            throw new ServerApiException(ApiErrorCode.RESOURCE_UNAVAILABLE_ERROR, e.getMessage());
+        } catch (InvalidParameterValueException e) {
+            s_logger.warn("Exception: ", e);
+            throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, e.getMessage());
+        } catch (ServerApiException e) {
+            throw new ServerApiException(e.getErrorCode(), e.getDescription());
+        } catch (CloudRuntimeException e) {
+            throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, e.getMessage());

Review comment:
       We could improve these logs with some context data and unify the catches.

##########
File path: api/src/main/java/org/apache/cloudstack/api/command/user/vm/CloneVMCmd.java
##########
@@ -0,0 +1,193 @@
+package org.apache.cloudstack.api.command.user.vm;
+
+import com.cloud.event.EventTypes;
+import com.cloud.exception.ConcurrentOperationException;
+import com.cloud.exception.InsufficientCapacityException;
+import com.cloud.exception.InvalidParameterValueException;
+import com.cloud.exception.ResourceAllocationException;
+import com.cloud.exception.ResourceUnavailableException;
+import com.cloud.template.VirtualMachineTemplate;
+import com.cloud.user.Account;
+import com.cloud.uservm.UserVm;
+import com.cloud.utils.exception.CloudRuntimeException;
+import com.cloud.vm.VirtualMachine;
+import org.apache.cloudstack.acl.SecurityChecker.AccessType;
+import org.apache.cloudstack.api.ACL;
+import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiCommandJobType;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.ApiErrorCode;
+import org.apache.cloudstack.api.BaseAsyncCreateCustomIdCmd;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.ResponseObject;
+import org.apache.cloudstack.api.ServerApiException;
+import org.apache.cloudstack.api.command.user.UserCmd;
+import org.apache.cloudstack.api.response.DomainResponse;
+import org.apache.cloudstack.api.response.UserVmResponse;
+//import org.apache.cloudstack.context.CallContext;
+import org.apache.cloudstack.context.CallContext;
+import org.apache.log4j.Logger;
+
+import java.util.Optional;
+
+@APICommand(name = "cloneVirtualMachine", responseObject = UserVmResponse.class, description = "clone a virtual VM in full clone mode",
+        responseView = ResponseObject.ResponseView.Restricted, requestHasSensitiveInfo = false, responseHasSensitiveInfo = true, entityType = {VirtualMachine.class})
+public class CloneVMCmd extends BaseAsyncCreateCustomIdCmd implements UserCmd {
+    public static final Logger s_logger = Logger.getLogger(CloneVMCmd.class.getName());
+    private static final String s_name = "clonevirtualmachineresponse";
+
+    /////////////////////////////////////////////////////
+    //////////////// API parameters /////////////////////
+    /////////////////////////////////////////////////////
+    @ACL(accessType = AccessType.OperateEntry)
+    @Parameter(name = ApiConstants.VIRTUAL_MACHINE_ID, type = CommandType.UUID, entityType=UserVmResponse.class,
+            required = true, description = "The ID of the virtual machine")
+    private Long id;
+
+    //Owner information
+    @Parameter(name = ApiConstants.ACCOUNT, type = CommandType.STRING, description = "an optional account for the virtual machine. Must be used with domainId.")
+    private String accountName;
+
+    @Parameter(name = ApiConstants.DOMAIN_ID, type = CommandType.UUID, entityType = DomainResponse.class, description = "an optional domainId for the virtual machine. If the account parameter is used, domainId must also be used.")
+    private Long domainId;
+
+    private Long temporaryTemlateId;
+
+    private Long temporarySnapShotId;
+
+    public String getAccountName() {
+        return accountName;
+    }
+
+    public Long getDomainId() {
+        return domainId;
+    }
+
+    public Long getId() {
+        return this.id;
+    }
+    @Override
+    public String getEventType() {
+        return EventTypes.EVENT_VM_CLONE;
+    }
+
+    @Override
+    public ApiCommandJobType getInstanceType() {
+        return ApiCommandJobType.VirtualMachine;
+    }
+
+    @Override
+    public String getEventDescription() {
+        return "Cloning user VM: " + this._uuidMgr.getUuid(VirtualMachine.class, getId());
+    }
+
+    public Long getTemporaryTemlateId() {
+        return this.temporaryTemlateId;
+    }
+
+    public void setTemporarySnapShotId(Long snapshotId) {
+        this.temporarySnapShotId = snapshotId;
+    }
+
+    public Long getTemporarySnapShotId() {
+        return temporarySnapShotId;
+    }
+
+
+    public void setTemporaryTemlateId(long tempId) {
+        this.temporaryTemlateId = tempId;
+    }
+
+    @Override
+    public void create() throws ResourceAllocationException {
+        try {
+            _userVmService.checkCloneCondition(this);
+            VirtualMachineTemplate template = _templateService.createPrivateTemplateRecord(this, _accountService.getAccount(getEntityOwnerId()), _volumeService);
+            if (template == null) {
+                throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, "failed to create a template to db");
+            }
+            s_logger.info("The template id recorded is: " + template.getId());
+            setTemporaryTemlateId(template.getId());
+            _templateService.createPrivateTemplate(this);
+            UserVm vmRecord = _userVmService.recordVirtualMachineToDB(this);
+            if (vmRecord == null) {
+                throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, "unable to record a new VM to db!");
+            }
+            setEntityId(vmRecord.getId());
+            setEntityUuid(vmRecord.getUuid());
+        }
+        catch (ResourceUnavailableException | InsufficientCapacityException e) {
+            s_logger.warn("Exception: ", e);
+            throw new ServerApiException(ApiErrorCode.RESOURCE_UNAVAILABLE_ERROR, e.getMessage());
+        } catch (InvalidParameterValueException e) {
+            s_logger.warn("Exception: ", e);
+            throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, e.getMessage());
+        } catch (ServerApiException e) {
+            throw new ServerApiException(e.getErrorCode(), e.getDescription());
+        } catch (CloudRuntimeException e) {
+            throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, e.getMessage());
+        } finally {
+            if (getTemporarySnapShotId() != null) {
+                _snapshotService.deleteSnapshot(getTemporarySnapShotId());
+                s_logger.warn("clearing the temporary snapshot: " + getTemporarySnapShotId());
+            }
+        }
+    }
+
+    public boolean isPublic() {
+        return false;
+    }
+
+    public String getVMName() {
+        return getTargetVM().getInstanceName();
+    }
+
+    public String getTemplateName() {
+        return (getVMName() + "-Clone-" + _uuidMgr.generateUuid(VirtualMachineTemplate.class, null)).substring(0, 32);
+    }
+
+    @Override
+    public void execute() {
+        Optional<UserVm> result;
+        try {
+            CallContext.current().setEventDetails("Vm Id for full clone: " + getEntityId());
+            s_logger.info("starting actual VM id: " + getEntityId());
+            result = _userVmService.cloneVirtualMachine(this, _volumeService, _snapshotService);
+        } catch (ResourceUnavailableException ex) {
+            s_logger.warn("Exception: ", ex);
+            throw new ServerApiException(ApiErrorCode.RESOURCE_UNAVAILABLE_ERROR, ex.getMessage());
+        } catch (ConcurrentOperationException ex) {
+            s_logger.warn("Exception: ", ex);
+            throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, ex.getMessage());
+        }
+        catch (ResourceAllocationException | InsufficientCapacityException ex) {
+            s_logger.warn("Exception: ", ex);
+            throw new ServerApiException(ApiErrorCode.RESOURCE_ALLOCATION_ERROR, ex.getMessage());
+        }

Review comment:
       We could improve these logs with some context data and unify the catches.

##########
File path: server/src/main/java/com/cloud/template/TemplateManagerImpl.java
##########
@@ -1742,6 +1744,253 @@ public void doInTransactionWithoutResult(TransactionStatus status) {
         }
     }
 
+    @Override
+    @DB
+    @ActionEvent(eventType = EventTypes.EVENT_TEMPLATE_CREATE, eventDescription = "creating actual private template", create = true)
+    public VirtualMachineTemplate createPrivateTemplate(CloneVMCmd cmd) throws CloudRuntimeException {
+        UserVm curVm = cmd.getTargetVM();
+        long templateId = cmd.getTemporaryTemlateId();
+        long snapshotId = cmd.getTemporarySnapShotId();
+        final Long accountId = curVm.getAccountId();
+        Account caller = CallContext.current().getCallingAccount();
+        List<VolumeVO> volumes = _volumeDao.findByInstanceAndType(cmd.getId(), Volume.Type.ROOT);
+        VolumeVO targetVolume = volumes.get(0);
+        long volumeId = targetVolume.getId();
+        VMTemplateVO finalTmpProduct = null;
+        SnapshotVO snapshot = null;
+        try {
+            TemplateInfo cloneTempalateInfp = _tmplFactory.getTemplate(templateId, DataStoreRole.Image);
+            long zoneId = curVm.getDataCenterId();
+            AsyncCallFuture<TemplateApiResult> future = null;
+            VolumeInfo vInfo = _volFactory.getVolume(volumeId);
+            DataStore store = _dataStoreMgr.getImageStoreWithFreeCapacity(zoneId);
+            snapshot = _snapshotDao.findById(snapshotId);
+//            future = _tmpltSvr.createTemplateFromVolumeAsync(vInfo, cloneTempalateInfp, store);
+            // create template from snapshot
+            DataStoreRole dataStoreRole = ApiResponseHelper.getDataStoreRole(snapshot, _snapshotStoreDao, _dataStoreMgr);
+            SnapshotInfo snapInfo = _snapshotFactory.getSnapshot(snapshotId, dataStoreRole);
+            if (dataStoreRole == DataStoreRole.Image) {
+                if (snapInfo == null) {
+                    snapInfo = _snapshotFactory.getSnapshot(snapshotId, DataStoreRole.Primary);
+                    if(snapInfo == null) {
+                        throw new CloudRuntimeException("Cannot find snapshot "+snapshotId);
+                    }
+                    // We need to copy the snapshot onto secondary.
+                    SnapshotStrategy snapshotStrategy = _storageStrategyFactory.getSnapshotStrategy(snapshot, SnapshotOperation.BACKUP);
+                    snapshotStrategy.backupSnapshot(snapInfo);
+
+                    // Attempt to grab it again.
+                    snapInfo = _snapshotFactory.getSnapshot(snapshotId, dataStoreRole);
+                    if(snapInfo == null) {
+                        throw new CloudRuntimeException("Cannot find snapshot " + snapshotId + " on secondary and could not create backup");
+                    }
+                }
+                _accountMgr.checkAccess(caller, null, true, snapInfo);
+                DataStore snapStore = snapInfo.getDataStore();
+
+                if (snapStore != null) {
+                    store = snapStore; // pick snapshot image store to create template
+                }
+            }
+            future = _tmpltSvr.createTemplateFromSnapshotAsync(snapInfo, cloneTempalateInfp, store);
+            // wait for the result to converge
+            CommandResult result = null;
+            try {
+                result = future.get();
+
+                if (result.isFailed()) {
+                    finalTmpProduct = null;
+                    s_logger.warn("Failed to create template: " + result.getResult());
+                    throw new CloudRuntimeException("Failed to create template: " + result.getResult());
+                }
+                if (_dataStoreMgr.isRegionStore(store)) {
+                    _tmpltSvr.associateTemplateToZone(templateId, null);
+                } else {
+                    // Already done in the record to db step
+                    VMTemplateZoneVO templateZone = new VMTemplateZoneVO(zoneId, templateId, new Date());
+                    _tmpltZoneDao.persist(templateZone);
+                }
+                s_logger.info("successfully created the template with Id: " + templateId);
+                finalTmpProduct = _tmpltDao.findById(templateId);
+                TemplateDataStoreVO srcTmpltStore = _tmplStoreDao.findByStoreTemplate(store.getId(), templateId);
+                UsageEventVO usageEvent =
+                        new UsageEventVO(EventTypes.EVENT_TEMPLATE_CREATE, finalTmpProduct.getAccountId(), zoneId, finalTmpProduct.getId(), finalTmpProduct.getName(), null,
+                                finalTmpProduct.getSourceTemplateId(), srcTmpltStore.getPhysicalSize(), finalTmpProduct.getSize());
+                _usageEventDao.persist(usageEvent);
+            } catch (InterruptedException e) {
+                s_logger.debug("Failed to create template for id: " + templateId, e);
+                throw new CloudRuntimeException("Failed to create template" , e);
+            } catch (ExecutionException e) {
+                s_logger.debug("Failed to create template for id: " + templateId, e);
+                throw new CloudRuntimeException("Failed to create template ", e);
+            }
+
+        } finally {
+            finalTmpProduct = _tmpltDao.findById(templateId);
+            if (finalTmpProduct == null) {
+                final VolumeVO volumeFinal = targetVolume;
+                final SnapshotVO snapshotFinal = snapshot;
+                Transaction.execute(new TransactionCallbackNoReturn() {
+                    @Override
+                    public void doInTransactionWithoutResult(TransactionStatus status) {
+                        // template_store_ref entries should have been removed using our
+                        // DataObject.processEvent command in case of failure, but clean
+                        // it up here to avoid
+                        // some leftovers which will cause removing template from
+                        // vm_template table fail.
+                        _tmplStoreDao.deletePrimaryRecordsForTemplate(templateId);
+                        // Remove the template_zone_ref record
+                        _tmpltZoneDao.deletePrimaryRecordsForTemplate(templateId);
+                        // Remove the template record
+                        _tmpltDao.expunge(templateId);
+
+                        // decrement resource count
+                        if (accountId != null) {
+                            _resourceLimitMgr.decrementResourceCount(accountId, ResourceType.template);
+                            _resourceLimitMgr.decrementResourceCount(accountId, ResourceType.secondary_storage, new Long(volumeFinal != null ? volumeFinal.getSize()
+                                    : snapshotFinal.getSize()));
+                        }
+                    }
+                });
+
+            }
+        }
+        return null;
+    }
+
+    @Override
+    @ActionEvent(eventType = EventTypes.EVENT_TEMPLATE_CREATE, eventDescription = "creating template from clone", create = true)
+    public VMTemplateVO createPrivateTemplateRecord(CloneVMCmd cmd, Account templateOwner, VolumeApiService volumeService) throws ResourceAllocationException {
+        Account caller = CallContext.current().getCallingAccount();
+        _accountMgr.checkAccess(caller, null, true, templateOwner);
+        String name = cmd.getTemplateName();
+        if (name.length() > 32) {
+            name = name.substring(5) + "-QA-Clone";
+        }
+
+        boolean featured = false;
+        boolean isPublic = cmd.isPublic();
+        UserVm curVm = cmd.getTargetVM();
+        long zoneId = curVm.getDataCenterId();
+        Long volumeId = _volumeDao.findByInstanceAndType(cmd.getId(), Volume.Type.ROOT).get(0).getId();
+        HypervisorType hyperType = null;
+        VolumeVO volume = _volumeDao.findById(volumeId);
+        if (volume == null) {
+            throw new InvalidParameterValueException("Failed to create private template record, unable to find root volume " + volumeId);
+        }
+
+        // check permissions
+        _accountMgr.checkAccess(caller, null, true, volume);
+
+        // If private template is created from Volume, check that the volume
+        // will not be active when the private template is
+        // created
+//        if (!_volumeMgr.volumeInactive(volume)) {
+//            String msg = "Unable to create private template for volume: " + volume.getName() + "; volume is attached to a non-stopped VM, please stop the VM first";
+//            if (s_logger.isInfoEnabled()) {
+//                s_logger.info(msg);
+//            }
+//            throw new CloudRuntimeException(msg);
+//        }
+
+        hyperType = _volumeDao.getHypervisorType(volumeId);
+        if (HypervisorType.LXC.equals(hyperType)) {
+            throw new InvalidParameterValueException("Template creation is not supported for LXC volume: " + volumeId);
+        }
+
+        _resourceLimitMgr.checkResourceLimit(templateOwner, ResourceType.template);
+        _resourceLimitMgr.checkResourceLimit(templateOwner, ResourceType.secondary_storage, volume.getSize());
+
+        Long guestOSId = cmd.getTargetVM().getGuestOSId();
+        GuestOSVO guestOS = _guestOSDao.findById(guestOSId);
+        if (guestOS == null) {
+            throw new InvalidParameterValueException("GuestOS with ID: " + guestOSId + " does not exist.");
+        }
+        // get snapshot from this step
+        Long nextTemplateId = _tmpltDao.getNextInSequence(Long.class, "id");
+        s_logger.info("Creating snapshot for the tempalte creation");
+        SnapshotVO snapshot = (SnapshotVO) volumeService.allocSnapshot(volumeId, Snapshot.INTERNAL_POLICY_ID, curVm.getDisplayName() + "-Clone-" + nextTemplateId, null);
+        if (snapshot == null) {
+            throw new CloudRuntimeException("Unable to create a snapshot during the template creation recording");
+        }
+        Snapshot snapshotEntity = volumeService.takeSnapshot(volumeId, Snapshot.INTERNAL_POLICY_ID, snapshot.getId(), caller, false, null, false, new HashMap<>());
+        if (snapshotEntity == null) {
+            throw new CloudRuntimeException("Error when creating the snapshot entity");
+        }
+        if (snapshotEntity.getState() != Snapshot.State.BackedUp) {
+            throw new CloudRuntimeException("Async backup of snapshot happens during the clone for snapshot id: " + snapshot.getId());
+        }
+        cmd.setTemporarySnapShotId(snapshot.getId());
+        String description = ""; // TODO: add this to clone parameter in the future
+        boolean isExtractable = false;
+        Long sourceTemplateId = null;
+        if (volume != null) {
+            VMTemplateVO template = ApiDBUtils.findTemplateById(volume.getTemplateId());
+            isExtractable = template != null && template.isExtractable() && template.getTemplateType() != Storage.TemplateType.SYSTEM;
+            if (volume.getIsoId() != null && volume.getIsoId() != 0) {
+                sourceTemplateId = volume.getIsoId();
+            } else if (volume.getTemplateId() != null) {
+                sourceTemplateId = volume.getTemplateId();
+            }
+        }
+
+        VMTemplateVO privateTemplate = null;
+        privateTemplate = new VMTemplateVO(nextTemplateId, name, ImageFormat.RAW, isPublic, featured, isExtractable,
+                TemplateType.USER, null, true, 64, templateOwner.getId(), null, description,
+                false, guestOS.getId(), true, hyperType, null, new HashMap<>(){{put("template to be cleared", "yes");}}, false, false, false, false);
+        List<ImageStoreVO> stores = _imgStoreDao.findRegionImageStores();
+        if (!CollectionUtils.isEmpty(stores)) {
+            privateTemplate.setCrossZones(true);
+        }
+
+        privateTemplate.setSourceTemplateId(sourceTemplateId);
+        VMTemplateVO template = _tmpltDao.persist(privateTemplate);
+        // persist this to the template zone area and remember to remove the resource count in the execute phase once in failure or clean up phase
+//        VMTemplateZoneVO templateZone = new VMTemplateZoneVO(zoneId, template.getId(), new Date());
+//        _tmpltZoneDao.persist(templateZone);
+//        TemplateDataStoreVO voRecord = _tmplStoreDao.createTemplateDirectDownloadEntry(template.getId(), template.getSize());
+//        voRecord.setDataStoreId(2);
+//        _tmplStoreDao.persist(voRecord);
+        // Increment the number of templates
+        if (template != null) {
+            Map<String, String> details = new HashMap<String, String>();
+
+            if (sourceTemplateId != null) {
+                VMTemplateVO sourceTemplate = _tmpltDao.findById(sourceTemplateId);
+                if (sourceTemplate != null && sourceTemplate.getDetails() != null) {
+                    details.putAll(sourceTemplate.getDetails());
+                }
+            }
+
+            if (volume != null) {
+                Long vmId = volume.getInstanceId();
+                if (vmId != null) {
+                    UserVmVO userVm = _userVmDao.findById(vmId);
+                    if (userVm != null) {
+                        _userVmDao.loadDetails(userVm);
+                        Map<String, String> vmDetails = userVm.getDetails();
+                        vmDetails = vmDetails.entrySet()
+                                .stream()
+                                .filter(map -> map.getValue() != null)
+                                .collect(Collectors.toMap(map -> map.getKey(), map -> map.getValue()));
+                        details.putAll(vmDetails);
+                    }
+                }
+            }
+
+            if (!details.isEmpty()) {
+                privateTemplate.setDetails(details);
+                _tmpltDao.saveDetails(privateTemplate);
+            }
+
+            _resourceLimitMgr.incrementResourceCount(templateOwner.getId(), ResourceType.template);
+            _resourceLimitMgr.incrementResourceCount(templateOwner.getId(), ResourceType.secondary_storage,
+                    snapshot.getSize());
+        }
+
+        return template;
+    }

Review comment:
       This method is extremely long, we could extract some code and add javadocs and unit tests. Also, I believe we do not need commented code.

##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -4499,6 +4506,216 @@ protected String validateUserData(String userData, HTTPMethod httpmethod) {
         return null;
     }
 
+    @Override
+    public void checkCloneCondition(CloneVMCmd cmd) throws InvalidParameterValueException, ResourceUnavailableException, CloudRuntimeException, ResourceAllocationException {
+
+        if (cmd.getAccountName() != null && cmd.getDomainId() == null) {
+            throw new InvalidParameterValueException("You must input the domainId together with the account name");
+        }
+
+        final DomainVO domain = cmd.getDomainId() == null ? null : _domainDao.findById(cmd.getDomainId());
+        final Account account = cmd.getAccountName() == null ? null : _accountService.getActiveAccountByName(cmd.getAccountName(), cmd.getDomainId());
+        if (domain  != null && account != null) {
+            if (account.getType() == Account.ACCOUNT_TYPE_PROJECT) {
+                throw new InvalidParameterValueException("Invalid user type: project to clone the VM");
+            }
+            if (account.getState() != Account.State.enabled) {
+                throw new InvalidParameterValueException("User is not enabled to clone this VM");
+            }
+        }
+        UserVm curVm = cmd.getTargetVM();
+        if (curVm == null) {
+            throw new CloudRuntimeException("the VM doesn't exist or not registered in management server!");
+        }
+        UserVmVO vmStatus = _vmDao.findById(cmd.getId());
+//        if (vmStatus.state != State.Shutdown && vmStatus.state != State.Stopped) {
+//            throw new CloudRuntimeException("You should clone an instance that's shutdown!");
+//        }
+        if (vmStatus.getHypervisorType() != HypervisorType.KVM && vmStatus.getHypervisorType() != HypervisorType.Simulator) {
+            throw new CloudRuntimeException("The clone operation is only supported on KVM and Simulator!");
+        }
+        String kvmEnabled = _configDao.getValue("kvm.snapshot.enabled");
+        if (kvmEnabled == null || !kvmEnabled.equalsIgnoreCase("true")) {
+            throw new CloudRuntimeException("Clone VM is not supported, as snapshots are disabled");
+        }
+        Long accountId = curVm.getAccountId();
+        Account vmOwner = _accountDao.findById(accountId);
+        if (vmOwner == null) {
+            throw new CloudRuntimeException("This VM doesn't have an owner account, please assign one to it");
+        }
+        List<VolumeVO> volumes = _volsDao.findByInstanceAndType(cmd.getId(), Volume.Type.ROOT);
+        if (CollectionUtils.isEmpty(volumes)) {
+            throw new CloudRuntimeException("The VM to copy does not have a Volume attached!");
+        }
+        // verify that the VM doesn't expire
+        Map<String, String> details = curVm.getDetails();
+        verifyDetails(details);
+//        Account activeOwner = _accountDao.findById(cmd.getEntityOwnerId());
+        long zoneId = curVm.getDataCenterId();
+        DataCenter zone = _entityMgr.findById(DataCenter.class, zoneId);
+        if (zone == null) {
+            throw new InvalidParameterValueException("Unable to find a zone in the current VM by zone id=" + zoneId);
+        }
+        // service offering check
+        long serviceOfferingId = curVm.getServiceOfferingId();
+        ServiceOffering serviceOffering = _entityMgr.findById(ServiceOffering.class, serviceOfferingId);
+        if (serviceOffering == null) {
+            throw new InvalidParameterValueException("Service offering Id for this VM: " + serviceOfferingId + " doesn't exist now");
+        }
+        if (!serviceOffering.isDynamic() && details != null) {
+            for(String detail: details.keySet()) {
+                if(detail.equalsIgnoreCase(VmDetailConstants.CPU_NUMBER) || detail.equalsIgnoreCase(VmDetailConstants.CPU_SPEED) || detail.equalsIgnoreCase(VmDetailConstants.MEMORY)) {
+                    throw new InvalidParameterValueException("cpuNumber or cpuSpeed or memory should not be specified for static service offering");
+                }
+            }
+        }
+        // disk offering check
+        VolumeVO rootDisk = volumes.get(0);
+        Long diskOfferingID = rootDisk.getDiskOfferingId();
+        DiskOfferingVO diskOffering =null;
+        if (diskOfferingID != null) {
+            diskOffering = _diskOfferingDao.findById(diskOfferingID);
+            if (diskOffering == null) {
+                throw new CloudRuntimeException("Unable to find disk offering " + diskOfferingID);
+            }
+        }
+        if (!zone.isLocalStorageEnabled()) {
+            if (serviceOffering.isUseLocalStorage()) {
+                throw new CloudRuntimeException("Zone is not configured to use local storage now but this service offering " + serviceOffering.getName() + " uses it");
+            }
+            if (diskOffering != null && diskOffering.isUseLocalStorage()) {
+                throw new CloudRuntimeException("Zone is not configured to use local storage but disk offering " + diskOffering.getName() + " uses it");
+            }
+        }
+        // resource limit checks & account check
+        AccountVO activeOwner = _accountDao.findById(cmd.getEntityOwnerId());
+        List<VolumeVO> totalVolumes = _volsDao.findByInstance(cmd.getId());
+        _resourceLimitMgr.checkResourceLimit(activeOwner, ResourceType.volume, totalVolumes.size());
+        Long totalSize = 0L;
+        for (VolumeVO volumeToCheck : totalVolumes) {
+            totalSize += volumeToCheck.getSize();
+        }
+        _resourceLimitMgr.checkResourceLimit(activeOwner, ResourceType.primary_storage, totalSize);
+    }

Review comment:
       This method is extremely long, we could extract some code and add javadocs and unit tests. Also, I believe we do not need commented code.

##########
File path: server/src/main/java/com/cloud/template/TemplateManagerImpl.java
##########
@@ -1742,6 +1744,253 @@ public void doInTransactionWithoutResult(TransactionStatus status) {
         }
     }
 
+    @Override
+    @DB
+    @ActionEvent(eventType = EventTypes.EVENT_TEMPLATE_CREATE, eventDescription = "creating actual private template", create = true)
+    public VirtualMachineTemplate createPrivateTemplate(CloneVMCmd cmd) throws CloudRuntimeException {
+        UserVm curVm = cmd.getTargetVM();
+        long templateId = cmd.getTemporaryTemlateId();
+        long snapshotId = cmd.getTemporarySnapShotId();
+        final Long accountId = curVm.getAccountId();
+        Account caller = CallContext.current().getCallingAccount();
+        List<VolumeVO> volumes = _volumeDao.findByInstanceAndType(cmd.getId(), Volume.Type.ROOT);
+        VolumeVO targetVolume = volumes.get(0);
+        long volumeId = targetVolume.getId();
+        VMTemplateVO finalTmpProduct = null;
+        SnapshotVO snapshot = null;
+        try {
+            TemplateInfo cloneTempalateInfp = _tmplFactory.getTemplate(templateId, DataStoreRole.Image);
+            long zoneId = curVm.getDataCenterId();
+            AsyncCallFuture<TemplateApiResult> future = null;
+            VolumeInfo vInfo = _volFactory.getVolume(volumeId);
+            DataStore store = _dataStoreMgr.getImageStoreWithFreeCapacity(zoneId);
+            snapshot = _snapshotDao.findById(snapshotId);
+//            future = _tmpltSvr.createTemplateFromVolumeAsync(vInfo, cloneTempalateInfp, store);
+            // create template from snapshot
+            DataStoreRole dataStoreRole = ApiResponseHelper.getDataStoreRole(snapshot, _snapshotStoreDao, _dataStoreMgr);
+            SnapshotInfo snapInfo = _snapshotFactory.getSnapshot(snapshotId, dataStoreRole);
+            if (dataStoreRole == DataStoreRole.Image) {
+                if (snapInfo == null) {
+                    snapInfo = _snapshotFactory.getSnapshot(snapshotId, DataStoreRole.Primary);
+                    if(snapInfo == null) {
+                        throw new CloudRuntimeException("Cannot find snapshot "+snapshotId);
+                    }
+                    // We need to copy the snapshot onto secondary.
+                    SnapshotStrategy snapshotStrategy = _storageStrategyFactory.getSnapshotStrategy(snapshot, SnapshotOperation.BACKUP);
+                    snapshotStrategy.backupSnapshot(snapInfo);
+
+                    // Attempt to grab it again.
+                    snapInfo = _snapshotFactory.getSnapshot(snapshotId, dataStoreRole);
+                    if(snapInfo == null) {
+                        throw new CloudRuntimeException("Cannot find snapshot " + snapshotId + " on secondary and could not create backup");
+                    }
+                }
+                _accountMgr.checkAccess(caller, null, true, snapInfo);
+                DataStore snapStore = snapInfo.getDataStore();
+
+                if (snapStore != null) {
+                    store = snapStore; // pick snapshot image store to create template
+                }
+            }
+            future = _tmpltSvr.createTemplateFromSnapshotAsync(snapInfo, cloneTempalateInfp, store);
+            // wait for the result to converge
+            CommandResult result = null;
+            try {
+                result = future.get();
+
+                if (result.isFailed()) {
+                    finalTmpProduct = null;
+                    s_logger.warn("Failed to create template: " + result.getResult());
+                    throw new CloudRuntimeException("Failed to create template: " + result.getResult());
+                }
+                if (_dataStoreMgr.isRegionStore(store)) {
+                    _tmpltSvr.associateTemplateToZone(templateId, null);
+                } else {
+                    // Already done in the record to db step
+                    VMTemplateZoneVO templateZone = new VMTemplateZoneVO(zoneId, templateId, new Date());
+                    _tmpltZoneDao.persist(templateZone);
+                }
+                s_logger.info("successfully created the template with Id: " + templateId);
+                finalTmpProduct = _tmpltDao.findById(templateId);
+                TemplateDataStoreVO srcTmpltStore = _tmplStoreDao.findByStoreTemplate(store.getId(), templateId);
+                UsageEventVO usageEvent =
+                        new UsageEventVO(EventTypes.EVENT_TEMPLATE_CREATE, finalTmpProduct.getAccountId(), zoneId, finalTmpProduct.getId(), finalTmpProduct.getName(), null,
+                                finalTmpProduct.getSourceTemplateId(), srcTmpltStore.getPhysicalSize(), finalTmpProduct.getSize());
+                _usageEventDao.persist(usageEvent);
+            } catch (InterruptedException e) {
+                s_logger.debug("Failed to create template for id: " + templateId, e);
+                throw new CloudRuntimeException("Failed to create template" , e);
+            } catch (ExecutionException e) {
+                s_logger.debug("Failed to create template for id: " + templateId, e);
+                throw new CloudRuntimeException("Failed to create template ", e);
+            }
+
+        } finally {
+            finalTmpProduct = _tmpltDao.findById(templateId);
+            if (finalTmpProduct == null) {
+                final VolumeVO volumeFinal = targetVolume;
+                final SnapshotVO snapshotFinal = snapshot;
+                Transaction.execute(new TransactionCallbackNoReturn() {
+                    @Override
+                    public void doInTransactionWithoutResult(TransactionStatus status) {
+                        // template_store_ref entries should have been removed using our
+                        // DataObject.processEvent command in case of failure, but clean
+                        // it up here to avoid
+                        // some leftovers which will cause removing template from
+                        // vm_template table fail.
+                        _tmplStoreDao.deletePrimaryRecordsForTemplate(templateId);
+                        // Remove the template_zone_ref record
+                        _tmpltZoneDao.deletePrimaryRecordsForTemplate(templateId);
+                        // Remove the template record
+                        _tmpltDao.expunge(templateId);
+
+                        // decrement resource count
+                        if (accountId != null) {
+                            _resourceLimitMgr.decrementResourceCount(accountId, ResourceType.template);
+                            _resourceLimitMgr.decrementResourceCount(accountId, ResourceType.secondary_storage, new Long(volumeFinal != null ? volumeFinal.getSize()
+                                    : snapshotFinal.getSize()));
+                        }
+                    }
+                });
+
+            }
+        }
+        return null;
+    }

Review comment:
       This method is extremely long, we could extract some code and add javadocs and unit tests.

##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -49,7 +50,53 @@
 import javax.xml.parsers.DocumentBuilder;
 import javax.xml.parsers.DocumentBuilderFactory;
 import javax.xml.parsers.ParserConfigurationException;
-
+//import com.cloud.network.IpAddress;
+//import com.cloud.network.IpAddress;

Review comment:
       Do we need these comments?

##########
File path: server/src/main/java/com/cloud/template/TemplateManagerImpl.java
##########
@@ -33,6 +33,26 @@
 import javax.inject.Inject;
 import javax.naming.ConfigurationException;
 
+import com.cloud.storage.DataStoreRole;
+import com.cloud.storage.GuestOSVO;
+import com.cloud.storage.ImageStoreUploadMonitorImpl;
+import com.cloud.storage.LaunchPermissionVO;
+import com.cloud.storage.Snapshot;
+import com.cloud.storage.SnapshotVO;
+import com.cloud.storage.Storage;
+import com.cloud.storage.StorageManager;
+import com.cloud.storage.StoragePool;
+import com.cloud.storage.StoragePoolHostVO;
+import com.cloud.storage.StoragePoolStatus;
+import com.cloud.storage.TemplateProfile;
+import com.cloud.storage.Upload;
+import com.cloud.storage.VMTemplateStoragePoolVO;
+import com.cloud.storage.VMTemplateStorageResourceAssoc;
+import com.cloud.storage.VMTemplateVO;
+import com.cloud.storage.VMTemplateZoneVO;
+import com.cloud.storage.Volume;
+import com.cloud.storage.VolumeApiService;
+import com.cloud.storage.VolumeVO;

Review comment:
       Do we need this import reorganization?

##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -49,7 +50,53 @@
 import javax.xml.parsers.DocumentBuilder;
 import javax.xml.parsers.DocumentBuilderFactory;
 import javax.xml.parsers.ParserConfigurationException;
-
+//import com.cloud.network.IpAddress;
+//import com.cloud.network.IpAddress;
+import com.cloud.network.IpAddressManager;
+import com.cloud.network.Network;
+import com.cloud.network.NetworkModel;
+import com.cloud.network.PhysicalNetwork;
+import com.cloud.network.security.SecurityGroupVO;
+import com.cloud.storage.DataStoreRole;
+import com.cloud.storage.DiskOfferingVO;
+import com.cloud.storage.GuestOSCategoryVO;
+import com.cloud.storage.GuestOSVO;
+import com.cloud.storage.ScopeType;
+import com.cloud.storage.Snapshot;
+import com.cloud.storage.SnapshotVO;
+import com.cloud.storage.Storage;
+import com.cloud.storage.StorageManager;
+import com.cloud.storage.StoragePool;
+import com.cloud.storage.StoragePoolStatus;
+import com.cloud.storage.VMTemplateStorageResourceAssoc;
+import com.cloud.storage.VMTemplateVO;
+import com.cloud.storage.VMTemplateZoneVO;
+import com.cloud.storage.Volume;
+import com.cloud.storage.VolumeApiService;
+import com.cloud.storage.VolumeVO;
+import com.cloud.storage.snapshot.SnapshotApiService;
+import com.cloud.user.Account;
+import com.cloud.user.AccountManager;
+import com.cloud.user.AccountService;
+import com.cloud.user.AccountVO;
+import com.cloud.user.ResourceLimitService;
+import com.cloud.user.SSHKeyPair;
+import com.cloud.user.SSHKeyPairVO;
+import com.cloud.user.User;
+import com.cloud.user.UserStatisticsVO;
+import com.cloud.user.UserVO;
+import com.cloud.user.VmDiskStatisticsVO;
+import com.cloud.utils.db.DB;
+import com.cloud.utils.db.EntityManager;
+import com.cloud.utils.db.GlobalLock;
+import com.cloud.utils.db.SearchCriteria;
+import com.cloud.utils.db.Transaction;
+import com.cloud.utils.db.TransactionCallback;
+import com.cloud.utils.db.TransactionCallbackNoReturn;
+import com.cloud.utils.db.TransactionCallbackWithException;
+import com.cloud.utils.db.TransactionCallbackWithExceptionNoReturn;
+import com.cloud.utils.db.TransactionStatus;
+import com.cloud.utils.db.UUIDManager;

Review comment:
       Do we need this import reorganization?

##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -4499,6 +4506,216 @@ protected String validateUserData(String userData, HTTPMethod httpmethod) {
         return null;
     }
 
+    @Override
+    public void checkCloneCondition(CloneVMCmd cmd) throws InvalidParameterValueException, ResourceUnavailableException, CloudRuntimeException, ResourceAllocationException {
+
+        if (cmd.getAccountName() != null && cmd.getDomainId() == null) {
+            throw new InvalidParameterValueException("You must input the domainId together with the account name");
+        }
+
+        final DomainVO domain = cmd.getDomainId() == null ? null : _domainDao.findById(cmd.getDomainId());
+        final Account account = cmd.getAccountName() == null ? null : _accountService.getActiveAccountByName(cmd.getAccountName(), cmd.getDomainId());
+        if (domain  != null && account != null) {
+            if (account.getType() == Account.ACCOUNT_TYPE_PROJECT) {
+                throw new InvalidParameterValueException("Invalid user type: project to clone the VM");
+            }
+            if (account.getState() != Account.State.enabled) {
+                throw new InvalidParameterValueException("User is not enabled to clone this VM");
+            }
+        }
+        UserVm curVm = cmd.getTargetVM();
+        if (curVm == null) {
+            throw new CloudRuntimeException("the VM doesn't exist or not registered in management server!");
+        }
+        UserVmVO vmStatus = _vmDao.findById(cmd.getId());
+//        if (vmStatus.state != State.Shutdown && vmStatus.state != State.Stopped) {
+//            throw new CloudRuntimeException("You should clone an instance that's shutdown!");
+//        }
+        if (vmStatus.getHypervisorType() != HypervisorType.KVM && vmStatus.getHypervisorType() != HypervisorType.Simulator) {
+            throw new CloudRuntimeException("The clone operation is only supported on KVM and Simulator!");
+        }
+        String kvmEnabled = _configDao.getValue("kvm.snapshot.enabled");
+        if (kvmEnabled == null || !kvmEnabled.equalsIgnoreCase("true")) {
+            throw new CloudRuntimeException("Clone VM is not supported, as snapshots are disabled");
+        }
+        Long accountId = curVm.getAccountId();
+        Account vmOwner = _accountDao.findById(accountId);
+        if (vmOwner == null) {
+            throw new CloudRuntimeException("This VM doesn't have an owner account, please assign one to it");
+        }
+        List<VolumeVO> volumes = _volsDao.findByInstanceAndType(cmd.getId(), Volume.Type.ROOT);
+        if (CollectionUtils.isEmpty(volumes)) {
+            throw new CloudRuntimeException("The VM to copy does not have a Volume attached!");
+        }
+        // verify that the VM doesn't expire
+        Map<String, String> details = curVm.getDetails();
+        verifyDetails(details);
+//        Account activeOwner = _accountDao.findById(cmd.getEntityOwnerId());
+        long zoneId = curVm.getDataCenterId();
+        DataCenter zone = _entityMgr.findById(DataCenter.class, zoneId);
+        if (zone == null) {
+            throw new InvalidParameterValueException("Unable to find a zone in the current VM by zone id=" + zoneId);
+        }
+        // service offering check
+        long serviceOfferingId = curVm.getServiceOfferingId();
+        ServiceOffering serviceOffering = _entityMgr.findById(ServiceOffering.class, serviceOfferingId);
+        if (serviceOffering == null) {
+            throw new InvalidParameterValueException("Service offering Id for this VM: " + serviceOfferingId + " doesn't exist now");
+        }
+        if (!serviceOffering.isDynamic() && details != null) {
+            for(String detail: details.keySet()) {
+                if(detail.equalsIgnoreCase(VmDetailConstants.CPU_NUMBER) || detail.equalsIgnoreCase(VmDetailConstants.CPU_SPEED) || detail.equalsIgnoreCase(VmDetailConstants.MEMORY)) {
+                    throw new InvalidParameterValueException("cpuNumber or cpuSpeed or memory should not be specified for static service offering");
+                }
+            }
+        }
+        // disk offering check
+        VolumeVO rootDisk = volumes.get(0);
+        Long diskOfferingID = rootDisk.getDiskOfferingId();
+        DiskOfferingVO diskOffering =null;
+        if (diskOfferingID != null) {
+            diskOffering = _diskOfferingDao.findById(diskOfferingID);
+            if (diskOffering == null) {
+                throw new CloudRuntimeException("Unable to find disk offering " + diskOfferingID);
+            }
+        }
+        if (!zone.isLocalStorageEnabled()) {
+            if (serviceOffering.isUseLocalStorage()) {
+                throw new CloudRuntimeException("Zone is not configured to use local storage now but this service offering " + serviceOffering.getName() + " uses it");
+            }
+            if (diskOffering != null && diskOffering.isUseLocalStorage()) {
+                throw new CloudRuntimeException("Zone is not configured to use local storage but disk offering " + diskOffering.getName() + " uses it");
+            }
+        }
+        // resource limit checks & account check
+        AccountVO activeOwner = _accountDao.findById(cmd.getEntityOwnerId());
+        List<VolumeVO> totalVolumes = _volsDao.findByInstance(cmd.getId());
+        _resourceLimitMgr.checkResourceLimit(activeOwner, ResourceType.volume, totalVolumes.size());
+        Long totalSize = 0L;
+        for (VolumeVO volumeToCheck : totalVolumes) {
+            totalSize += volumeToCheck.getSize();
+        }
+        _resourceLimitMgr.checkResourceLimit(activeOwner, ResourceType.primary_storage, totalSize);
+    }
+
+    private VolumeVO saveDataDiskVolumeFromSnapShot(final Account owner, final Boolean displayVolume, final Long zoneId, final Long diskOfferingId,
+                                                    final Storage.ProvisioningType provisioningType, final Long size, final Long minIops, final Long maxIops, final VolumeVO parentVolume, final String volumeName, final String uuid, final Map<String, String> details) {
+        return Transaction.execute((TransactionCallback<VolumeVO>) status -> {
+            VolumeVO volume = new VolumeVO(volumeName, -1, -1, -1, -1, new Long(-1), null, null, provisioningType, 0, Volume.Type.DATADISK);
+            volume.setPoolId(null);
+            volume.setUuid(uuid);
+            volume.setDataCenterId(zoneId);
+            volume.setPodId(null);
+            volume.setAccountId(owner.getId());
+            volume.setDomainId(owner.getDomainId());
+            volume.setDiskOfferingId(diskOfferingId);
+            volume.setSize(size);
+            volume.setMinIops(minIops);
+            volume.setMaxIops(maxIops);
+            volume.setInstanceId(null);
+            volume.setUpdated(new Date());
+            volume.setDisplayVolume(displayVolume);
+            if (parentVolume != null) {
+                volume.setTemplateId(parentVolume.getTemplateId());
+                volume.setFormat(parentVolume.getFormat());
+            } else {
+                volume.setTemplateId(null);
+            }
+
+            volume = _volsDao.persist(volume);
+            CallContext.current().setEventDetails("Volume Id: " + volume.getUuid());
+
+            // Increment resource count during allocation; if actual creation fails,
+            // decrement it
+            _resourceLimitMgr.incrementResourceCount(volume.getAccountId(), ResourceType.volume, displayVolume);
+            _resourceLimitMgr.incrementResourceCount(volume.getAccountId(), ResourceType.primary_storage, displayVolume, new Long(volume.getSize()));
+            return volume;
+        });
+    }
+
+    @Override
+    @ActionEvent(eventType = EventTypes.EVENT_VM_CLONE, eventDescription = "clone vm", async = true)
+    public Optional<UserVm> cloneVirtualMachine(CloneVMCmd cmd, VolumeApiService volumeService, SnapshotApiService snapshotService) throws ResourceUnavailableException, ConcurrentOperationException, CloudRuntimeException, InsufficientCapacityException, ResourceAllocationException {
+        long vmId = cmd.getEntityId();
+        UserVmVO curVm = _vmDao.findById(vmId);
+        // create and attach data disk
+        long targetClonedVmId = cmd.getId();
+        Account caller = CallContext.current().getCallingAccount();
+        List<VolumeVO> dataDisks = _volsDao.findByInstanceAndType(targetClonedVmId, Volume.Type.DATADISK);
+        List<Snapshot> createdSnapshots = new ArrayList<>();
+        List<VolumeVO> createdVolumes = new ArrayList<>();
+        long zoneId = cmd.getTargetVM().getDataCenterId();
+        s_logger.info("Trying to attach data disk before starting the VM...");
+        if (dataDisks.size() > 0) {
+            VolumeVO newDatadisk = null;
+            try {
+                for (VolumeVO dataDisk : dataDisks) {
+                    long diskId = dataDisk.getId();
+                    SnapshotVO dataSnapShot = (SnapshotVO) volumeService.allocSnapshot(diskId, Snapshot.INTERNAL_POLICY_ID, "DataDisk-Clone" + dataDisk.getName(), null);
+                    if (dataSnapShot == null) {
+                        throw new CloudRuntimeException("Unable to allocate snapshot of data disk: " + dataDisk.getId() + " name: " + dataDisk.getName());
+                    }
+                    createdSnapshots.add(dataSnapShot);
+                    SnapshotVO snapshotEntity = (SnapshotVO) volumeService.takeSnapshot(diskId, Snapshot.INTERNAL_POLICY_ID, dataSnapShot.getId(), caller, false, null, false, new HashMap<>());
+                    if (snapshotEntity == null) {
+                        throw new CloudRuntimeException("Error when creating the snapshot entity");
+                    }
+                    if (snapshotEntity.getState() != Snapshot.State.BackedUp) {
+                        throw new CloudRuntimeException("Async backup of snapshot happens during the clone for snapshot id: " + dataSnapShot.getId());
+                    }
+                    long diskOfferingId = snapshotEntity.getDiskOfferingId();
+                    DiskOfferingVO diskOffering = _diskOfferingDao.findById(diskOfferingId);
+                    Long minIops = snapshotEntity.getMinIops();
+                    Long maxIops = snapshotEntity.getMaxIops();
+                    Long size = snapshotEntity.getSize();
+                    Storage.ProvisioningType provisioningType = diskOffering.getProvisioningType();
+                    DataCenterVO dataCenter = _dcDao.findById(zoneId);
+                    String volumeName = snapshotEntity.getName() + "-DataDisk-Volume";
+                    VolumeVO parentVolume = _volsDao.findByIdIncludingRemoved(snapshotEntity.getVolumeId());
+                    newDatadisk = saveDataDiskVolumeFromSnapShot(caller, true, zoneId,
+                            diskOfferingId, provisioningType, size, minIops, maxIops, parentVolume, volumeName, _uuidMgr.generateUuid(Volume.class, null), new HashMap<>());
+                    VolumeVO volumeEntity = (VolumeVO) volumeService.cloneDataVolume(cmd, snapshotEntity.getId(), newDatadisk);
+                    createdVolumes.add(volumeEntity);
+                }
+
+                for (VolumeVO createdVol : createdVolumes) {
+                    volumeService.attachVolumeToVm(cmd, createdVol.getId(), createdVol.getDeviceId());
+                }
+            } catch (CloudRuntimeException e){
+                s_logger.warn("data disk process failed during clone, clearing the temporary resources...");
+                for (VolumeVO dataDiskToClear : createdVolumes) {
+                    volumeService.destroyVolume(dataDiskToClear.getId(), caller, true, false);
+                }
+                // clear the created disks
+                if (newDatadisk != null) {
+                    volumeService.destroyVolume(newDatadisk.getId(), caller, true, false);
+                }
+                destroyVm(vmId, true);
+                throw new CloudRuntimeException(e.getMessage());
+            } finally {
+                // clear the temporary data snapshots
+                for (Snapshot snapshotLeftOver : createdSnapshots) {
+                    snapshotService.deleteSnapshot(snapshotLeftOver.getId());
+                }
+            }
+        }
+
+        // start the VM if successfull
+        Long podId = curVm.getPodIdToDeployIn();
+        Long clusterId = null;
+        Long hostId = curVm.getHostId();
+        Map<VirtualMachineProfile.Param, Object> additonalParams =  new HashMap<>();
+        Map<Long, DiskOffering> diskOfferingMap = new HashMap<>();
+        if (MapUtils.isNotEmpty(curVm.getDetails()) && curVm.getDetails().containsKey(ApiConstants.BootType.UEFI.toString())) {
+            Map<String, String> map = curVm.getDetails();
+            additonalParams.put(VirtualMachineProfile.Param.UefiFlag, "Yes");
+            additonalParams.put(VirtualMachineProfile.Param.BootType, ApiConstants.BootType.UEFI.toString());
+            additonalParams.put(VirtualMachineProfile.Param.BootMode, map.get(ApiConstants.BootType.UEFI.toString()));
+        }
+
+        return Optional.of(startVirtualMachine(vmId, podId, clusterId, hostId, diskOfferingMap, additonalParams, null));
+    }
+

Review comment:
       This method is long, we could extract some code and add javadocs and unit tests. Also, I believe we do not need these comments.

##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -5484,6 +5701,84 @@ public UserVm createVirtualMachine(DeployVMCmd cmd) throws InsufficientCapacityE
         return vm;
     }
 
+    @Override
+    public UserVm recordVirtualMachineToDB(CloneVMCmd cmd) throws ConcurrentOperationException, ResourceAllocationException, InsufficientCapacityException, ResourceUnavailableException {
+        //network configurations and check, then create the template
+        UserVm curVm = cmd.getTargetVM();
+        // check if host is available
+        Long hostId = curVm.getHostId();
+        getDestinationHost(hostId, true);
+        Long zoneId = curVm.getDataCenterId();
+        DataCenter dataCenter = _entityMgr.findById(DataCenter.class, zoneId);
+        Map<String, String> vmProperties = curVm.getDetails() != null ? curVm.getDetails() : new HashMap<>();
+        String keyboard = vmProperties.get(VmDetailConstants.KEYBOARD);
+        HypervisorType hypervisorType = curVm.getHypervisorType();
+        Account curAccount = _accountDao.findById(curVm.getAccountId());
+        long callingUserId = CallContext.current().getCallingUserId();
+        Account callerAccount = CallContext.current().getCallingAccount();
+//        IpAddress ipAddress = _ipAddrMgr.assignPublicIpAddress(zoneId, curVm.getPodIdToDeployIn(), callerAccount, VlanType.DirectAttached, )
+//        IpAddress ipAddress = _ipAddrMgr.allocateIp(curAccount, false, callerAccount, callingUserId, dataCenter, true, null);
+        String ipv6Address = null;
+        String macAddress = null;
+        IpAddresses addr = new IpAddresses(null, ipv6Address, macAddress);
+//        IpAddresses addr = new IpAddresses("172.20.0.98", ipv6Address, macAddress);
+        long serviceOfferingId = curVm.getServiceOfferingId();
+        ServiceOffering serviceOffering = _serviceOfferingDao.findById(curVm.getId(), serviceOfferingId);
+        List<SecurityGroupVO> securityGroupList = _securityGroupMgr.getSecurityGroupsForVm(curVm.getId());
+        List<Long> securityGroupIdList = securityGroupList.stream().map(SecurityGroupVO::getId).collect(Collectors.toList());
+        String uuidName = _uuidMgr.generateUuid(UserVm.class, null);
+        String hostName = generateHostName(uuidName);
+        String displayName = hostName + "-Clone";
+        Long diskOfferingId = curVm.getDiskOfferingId();
+        Long size = null; // mutual exclusive with disk offering id
+        HTTPMethod httpMethod = cmd.getHttpMethod();
+        String userData = curVm.getUserData();
+        String sshKeyPair = null;
+        Map<Long, IpAddresses> ipToNetoworkMap = null; // Since we've specified Ip
+        boolean isDisplayVM = curVm.isDisplayVm();
+        boolean dynamicScalingEnabled = curVm.isDynamicallyScalable();
+        Long templateId = cmd.getTemporaryTemlateId();
+        VirtualMachineTemplate template = _entityMgr.findById(VirtualMachineTemplate.class, templateId);
+        if (template == null) {
+            throw new CloudRuntimeException("the temporary template is not created, server error, contact your sys admin");
+        }
+        List<Long> networkIds = _networkModel.listNetworksUsedByVm(curVm.getId());
+        String group = null;
+        InstanceGroupVO groupVo = getGroupForVm(cmd.getId());
+        if (groupVo != null) {
+            group = groupVo.getName();
+        }
+        UserVm vmResult = null;
+        List<Long> affinityGroupIdList = _affinityGroupDao.findByAccountAndNames(curAccount.getId(), curAccount.getAccountName())
+                                        .stream().
+                                        mapToLong(AffinityGroupVO::getId).
+                                        boxed().
+                                        collect(Collectors.toList());
+        try {
+            if (dataCenter.getNetworkType() == NetworkType.Basic) {
+                vmResult = createBasicSecurityGroupVirtualMachine(dataCenter, serviceOffering, template, securityGroupIdList, curAccount, hostName, displayName, diskOfferingId,
+                        size, group, hypervisorType, cmd.getHttpMethod(), userData, sshKeyPair, ipToNetoworkMap, addr, isDisplayVM, keyboard, affinityGroupIdList,
+                        curVm.getDetails() == null ? new HashMap<>() : curVm.getDetails(), cmd.getCustomId(), new HashMap<>(),
+                        null, new HashMap<>(), dynamicScalingEnabled);
+            } else {
+                if (dataCenter.isSecurityGroupEnabled()) {
+                    vmResult = createAdvancedSecurityGroupVirtualMachine(dataCenter, serviceOffering, template, networkIds, securityGroupIdList, curAccount, hostName,
+                            displayName, diskOfferingId, size, group, hypervisorType, cmd.getHttpMethod(), userData, sshKeyPair, ipToNetoworkMap, addr, isDisplayVM, keyboard,
+                            affinityGroupIdList, curVm.getDetails() == null ? new HashMap<>() : curVm.getDetails(), cmd.getCustomId(), new HashMap<>(),
+                            null, new HashMap<>(), dynamicScalingEnabled);
+                } else {
+                    vmResult = createAdvancedVirtualMachine(dataCenter, serviceOffering, template, networkIds, curAccount, hostName, displayName, diskOfferingId, size, group,
+                            hypervisorType, cmd.getHttpMethod(), userData, sshKeyPair, ipToNetoworkMap, addr, isDisplayVM, keyboard, affinityGroupIdList, curVm.getDetails() == null ? new HashMap<>() : curVm.getDetails(),
+                            cmd.getCustomId(), new HashMap<>(), null, new HashMap<>(), dynamicScalingEnabled);
+                }
+            }
+        } catch (CloudRuntimeException e) {
+            _templateMgr.delete(curAccount.getId(), template.getId(), zoneId);
+            throw new CloudRuntimeException("Unable to create the VM record");
+        }
+        return vmResult;
+    }

Review comment:
       This method is long, we could extract some code and add javadocs and unit tests. Also, I believe we do not need commented code.




-- 
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: commits-unsubscribe@cloudstack.apache.org

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