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/16 17:31:55 UTC

[GitHub] [cloudstack] atrocitytheme opened a new pull request #5216: Support for new Feature: Clone a Virtual Machine

atrocitytheme opened a new pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216


   ### Description
   
   This PR is a GSoC project #4818 
   
   This PR adds a full clone feature to the API level, where a fully cloned virtual machine will be created with root disk, data disk, and system configuration the same as the original VM.
   
   Feature included: 
   1. the creation of temporary snapshots during the clone for both root and data disks
   2. creation of template from the root disk
   3. newly copied data disk attachments to the new VM
   4. cleanup of temporary resources and error handling of the full clone process
   5. A new CloneVmCmd API interface to use this feature
   6. Automatic assignment of new network resources for the new Cloned VM
   ### Types of changes
   
   - [ ] Breaking change (fix or feature that would cause existing functionality to change)
   - [x] New feature (non-breaking change which adds functionality)
   - [ ] Bug fix (non-breaking change which fixes an issue)
   - [ ] Enhancement (improves an existing feature and functionality)
   - [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
   
   ### Feature/Enhancement Scale or Bug Severity
   
   #### Feature/Enhancement Scale
   
   - [x] Major
   - [ ] Minor
   
   #### Bug Severity
   
   - [ ] BLOCKER
   - [ ] Critical
   - [ ] Major
   - [ ] Minor
   - [ ] Trivial
   
   
   ### Screenshots (if appropriate):
   
   
   ### How Has This Been Tested?
   <!-- Please describe in detail how you tested your changes. -->
   <!-- Include details of your testing environment, and the tests you ran to -->
   <!-- see how your change affects other areas of the code, etc. -->
   This has been manually tested with a mbx KVM setup on a local machine and mbx kvm setup on GCP
   
   - This feature has been tested on local Linux system with kvm support (5.3.0-64-generic Ubuntu) and Google Cloud instance (4.9.0-15-amd64 Debian)
   - Try with cloudmonkey if the management server are running with default configurations.
   ```
   cloneVirtualMachine virtualmachineid=<target_vm_id>
   ```
   will start the clone process, it'll create a new cloned VM and start it (with all copied data available), network Ip will be assigned to DB instantly and the actual VM will get this IP after a while
   - When secondary system VM agents are not available, the cloning process will fail and clean the previously cloned resources
   - It'll copy all the data disk content from the target VM no matter whether the data disks are mounted in the VM system or the VM is running
   - Temporary resources created (snapshots) during the process will not remain whether the clone succeeds or not
   <!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md) document -->
   


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



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

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#discussion_r692791800



##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -4509,6 +4518,245 @@ protected String validateUserData(String userData, HTTPMethod httpmethod) {
         return null;
     }
 
+    @Override
+    public void validateCloneCondition(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!");
+//        }

Review comment:
       please remove

##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -4509,6 +4518,245 @@ protected String validateUserData(String userData, HTTPMethod httpmethod) {
         return null;
     }
 
+    @Override
+    public void validateCloneCondition(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());

Review comment:
       please remove




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



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

Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#discussion_r687949990



##########
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 {

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

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



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

Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#discussion_r687954670



##########
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);

Review comment:
       Better move the code at line# 105 -115 to User VM service.




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



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

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#issuecomment-963130621


   Packaging result: :heavy_multiplication_x: el7 :heavy_multiplication_x: el8 :heavy_multiplication_x: debian :heavy_multiplication_x: suse15. SL-JID 1685


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



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

Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#issuecomment-963170879


   @blueorangutan package


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



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

Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#discussion_r687946686



##########
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")

Review comment:
       ```suggestion
               required = true, description = "The ID of the virtual machine to clone")
   ```




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



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

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#discussion_r692764573



##########
File path: api/src/main/java/com/cloud/storage/Snapshot.java
##########
@@ -26,7 +26,7 @@
 
 public interface Snapshot extends ControlledEntity, Identity, InternalIdentity, StateObject<Snapshot.State> {
     public enum Type {
-        MANUAL, RECURRING, TEMPLATE, HOURLY, DAILY, WEEKLY, MONTHLY;
+        MANUAL, RECURRING, TEMPLATE, HOURLY, DAILY, WEEKLY, MONTHLY, INTERNAL;

Review comment:
       maybe this will work but as we have now two fixed values, it is maybe time to introduce a construcor with values to use?




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



[GitHub] [cloudstack] sureshanaparti removed a comment on pull request #5216: Support for new Feature: Clone a Virtual Machine (#4818)

Posted by GitBox <gi...@apache.org>.
sureshanaparti removed a comment on pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#issuecomment-963125040


   @blueorangutan package


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



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

Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#issuecomment-903671697


   @blueorangutan package


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



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

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#issuecomment-901287838


   @sureshanaparti a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.


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



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

Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#discussion_r687977992



##########
File path: server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
##########
@@ -908,6 +909,12 @@ public VolumeVO createVolume(CreateVolumeCmd cmd) {
         }
     }
 
+    @Override
+    public Volume cloneDataVolume(CloneVMCmd cmd, long snapshotId, Volume volume) throws StorageUnavailableException {

Review comment:
       update relevant method name, not exactly cloning data volume.




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



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

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#issuecomment-901589148


   <b>Trillian test result (tid-1703)</b>
   Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 34430 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5216-t1703-kvm-centos7.zip
   Smoke tests completed. 89 look OK, 0 have error(s)
   Only failed tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   


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



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

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#discussion_r692791458



##########
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);

Review comment:
       I see you did apply this in `UserVmManagerImpl` but didn't remove the parameters.




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



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

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#discussion_r692794263



##########
File path: server/src/main/java/com/cloud/template/TemplateManagerImpl.java
##########
@@ -1742,6 +1744,272 @@ 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, long snapshotId, long templateId) throws CloudRuntimeException {
+        UserVm curVm = cmd.getTargetVM();
+        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);
+                try {
+                    srcTmpltStore.getSize();
+                } catch (NullPointerException e) {
+                    srcTmpltStore.setSize(0L);
+                    _tmplStoreDao.update(srcTmpltStore.getId(), srcTmpltStore);
+                }
+                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
+    public Snapshot createSnapshotFromTemplateOwner(long vmId, UserVm curVm, Account templateOwner, VolumeApiService volumeService) throws ResourceAllocationException {
+        Account caller = CallContext.current().getCallingAccount();
+        _accountMgr.checkAccess(caller, null, true, templateOwner);
+//        UserVm curVm = cmd.getTargetVM();
+        Long nextSnapId = _tmpltDao.getNextInSequence(Long.class, "id");
+        Long volumeId = _volumeDao.findByInstanceAndType(vmId, Volume.Type.ROOT).get(0).getId();
+        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);
+        s_logger.info("Creating snapshot for the tempalte creation");
+        SnapshotVO snapshot = (SnapshotVO) volumeService.allocSnapshot(volumeId, Snapshot.INTERNAL_POLICY_ID, curVm.getDisplayName() + "-Clone-" + nextSnapId, 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());
+        }
+        return snapshot;
+    }
+    @Override
+    @ActionEvent(eventType = EventTypes.EVENT_TEMPLATE_CREATE, eventDescription = "creating template from clone", create = true)
+    public VMTemplateVO createPrivateTemplateRecord(CloneVMCmd cmd, Account templateOwner, VolumeApiService volumeService, Snapshot snapshot) 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);
+//        }
+

Review comment:
       ```suggestion
   ```




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



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

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#discussion_r692792394



##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -5494,6 +5742,82 @@ public UserVm createVirtualMachine(DeployVMCmd cmd) throws InsufficientCapacityE
         return vm;
     }
 
+    @Override
+    public UserVm recordVirtualMachineToDB(CloneVMCmd cmd, long templateId) 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);

Review comment:
       please remove

##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -5494,6 +5742,82 @@ public UserVm createVirtualMachine(DeployVMCmd cmd) throws InsufficientCapacityE
         return vm;
     }
 
+    @Override
+    public UserVm recordVirtualMachineToDB(CloneVMCmd cmd, long templateId) 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);

Review comment:
       please remove




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



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

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#issuecomment-902555234


   @atrocitytheme I like the premise and most of your implementation. Two worries:
   1. your methods in `SnapshotManagerImpl` and in `UserVmManagerImpl` are rather long, Can you please extract sensible parts from them (and maybe at times re-use). A good indicator would be if there is a comment, it can probably be used as a camelCase method name and the code below it moved in the new method.
   2. will the usage server pick up cloned machines for billing? (I don't think it will but I might be missing something)
   
   thanks for this PR, good addition.


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



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

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#issuecomment-903672403


   @sureshanaparti a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.


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



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

Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#issuecomment-901310916


   @blueorangutan test


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



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

Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#issuecomment-903982585


   @blueorangutan test


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



[GitHub] [cloudstack] nvazquez edited a comment on pull request #5216: Support for new Feature: Clone a Virtual Machine (#4818)

Posted by GitBox <gi...@apache.org>.
nvazquez edited a comment on pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#issuecomment-1059774173


   Thanks for fixing them @atrocitytheme - yes this PR has been open for some time and there have been changes in between, also merging main branch on it helps getting the latest changes. I will review the PR, thanks
   
   @blueorangutan package


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



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

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#issuecomment-901311168


   @sureshanaparti a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests


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



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

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#discussion_r692794577



##########
File path: server/src/main/java/com/cloud/template/TemplateManagerImpl.java
##########
@@ -1742,6 +1744,272 @@ 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, long snapshotId, long templateId) throws CloudRuntimeException {
+        UserVm curVm = cmd.getTargetVM();
+        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);
+                try {
+                    srcTmpltStore.getSize();
+                } catch (NullPointerException e) {
+                    srcTmpltStore.setSize(0L);
+                    _tmplStoreDao.update(srcTmpltStore.getId(), srcTmpltStore);
+                }
+                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
+    public Snapshot createSnapshotFromTemplateOwner(long vmId, UserVm curVm, Account templateOwner, VolumeApiService volumeService) throws ResourceAllocationException {
+        Account caller = CallContext.current().getCallingAccount();
+        _accountMgr.checkAccess(caller, null, true, templateOwner);
+//        UserVm curVm = cmd.getTargetVM();
+        Long nextSnapId = _tmpltDao.getNextInSequence(Long.class, "id");
+        Long volumeId = _volumeDao.findByInstanceAndType(vmId, Volume.Type.ROOT).get(0).getId();
+        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);
+        s_logger.info("Creating snapshot for the tempalte creation");
+        SnapshotVO snapshot = (SnapshotVO) volumeService.allocSnapshot(volumeId, Snapshot.INTERNAL_POLICY_ID, curVm.getDisplayName() + "-Clone-" + nextSnapId, 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());
+        }
+        return snapshot;
+    }
+    @Override
+    @ActionEvent(eventType = EventTypes.EVENT_TEMPLATE_CREATE, eventDescription = "creating template from clone", create = true)
+    public VMTemplateVO createPrivateTemplateRecord(CloneVMCmd cmd, Account templateOwner, VolumeApiService volumeService, Snapshot snapshot) 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");
+        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);

Review comment:
       ```suggestion
   ```




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



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

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#discussion_r693532725



##########
File path: test/integration/smoke/test_vm_life_cycle.py
##########
@@ -1991,3 +1990,135 @@ def test_01_vapps_vm_cycle(self):
             cmd = destroyVirtualMachine.destroyVirtualMachineCmd()
             cmd.id = vm.id
             self.apiclient.destroyVirtualMachine(cmd)
+
+class TestCloneVM(cloudstackTestCase):
+    
+    @classmethod
+    def setUpClass(cls):
+        testClient = super(TestCloneVM, cls).getClsTestClient()
+        cls.apiclient = testClient.getApiClient()
+        cls.services = testClient.getParsedTestDataConfig()
+        cls.hypervisor = testClient.getHypervisorInfo()
+
+        # Get Zone, Domain and templates
+        domain = get_domain(cls.apiclient)
+        cls.zone = get_zone(cls.apiclient, cls.testClient.getZoneForTests())
+        cls.services['mode'] = cls.zone.networktype
+
+        # if local storage is enabled, alter the offerings to use localstorage
+        # this step is needed for devcloud
+        if cls.zone.localstorageenabled == True:
+            cls.services["service_offerings"]["tiny"]["storagetype"] = 'local'
+            cls.services["service_offerings"]["small"]["storagetype"] = 'local'
+            cls.services["service_offerings"]["medium"]["storagetype"] = 'local'
+
+        template = get_suitable_test_template(
+            cls.apiclient,
+            cls.zone.id,
+            cls.services["ostype"],
+            cls.hypervisor
+        )
+        if template == FAILED:
+            assert False, "get_suitable_test_template() failed to return template with description %s" % cls.services["ostype"]
+
+        # Set Zones and disk offerings
+        cls.services["small"]["zoneid"] = cls.zone.id
+        cls.services["small"]["template"] = template.id
+
+        cls.services["iso1"]["zoneid"] = cls.zone.id
+
+        # Create VMs, NAT Rules etc
+        cls.account = Account.create(
+            cls.apiclient,
+            cls.services["account"],
+            domainid=domain.id
+        )
+
+        cls.small_offering = ServiceOffering.create(
+            cls.apiclient,
+            cls.services["service_offerings"]["small"]
+        )
+
+        cls.medium_offering = ServiceOffering.create(
+            cls.apiclient,
+            cls.services["service_offerings"]["medium"]
+        )
+        # create small and large virtual machines
+        cls.small_virtual_machine = VirtualMachine.create(
+            cls.apiclient,
+            cls.services["small"],
+            accountid=cls.account.name,
+            domainid=cls.account.domainid,
+            serviceofferingid=cls.small_offering.id,
+            mode=cls.services["mode"]
+        )
+        cls.medium_virtual_machine = VirtualMachine.create(
+            cls.apiclient,
+            cls.services["small"],
+            accountid=cls.account.name,
+            domainid=cls.account.domainid,
+            serviceofferingid=cls.medium_offering.id,
+            mode=cls.services["mode"]
+        )
+        cls.virtual_machine = VirtualMachine.create(
+            cls.apiclient,
+            cls.services["small"],
+            accountid=cls.account.name,
+            domainid=cls.account.domainid,
+            serviceofferingid=cls.small_offering.id,
+            mode=cls.services["mode"]
+        )
+        cls._cleanup = [
+            cls.small_offering,
+            cls.medium_offering,
+            cls.account
+        ]
+
+    @classmethod
+    def tearDownClass(cls):
+        super(TestCloneVM, cls).tearDownClass()
+
+    def setUp(self):
+        self.apiclient = self.testClient.getApiClient()
+        self.dbclient = self.testClient.getDbConnection()
+        self.cleanup = []
+
+    def tearDown(self):
+        try:
+            # Clean up, terminate the created ISOs
+            cleanup_resources(self.apiclient, self.cleanup)
+        except Exception as e:
+            raise Exception("Warning: Exception during cleanup : %s" % e)
+        return
+
+    @attr(tags = ["clone","devcloud", "advanced", "smoke", "basic", "sg"], required_hardware="false")
+    def test_clone_vm_and_volumes(self):
+        small_disk_offering = DiskOffering.list(self.apiclient, name='Small')[0];
+        config = Configurations.list(self.apiclient,
+                              name="kvm.snapshot.enabled"
+                              )
+        if len(config) == 0 or config[0].value != "true":
+            self.skipTest("Please enable kvm.snapshot.enable global config")
+        if self.hypervisor.lower() in ["kvm", "simulator"]:
+            small_virtual_machine = VirtualMachine.create(
+                self.apiclient,
+                self.services["small"],
+                accountid=self.account.name,
+                domainid=self.account.domainid,
+                serviceofferingid=self.small_offering.id,)
+            vol1 = Volume.create(
+                self.apiclient,
+                self.services,
+                account=self.account.name,
+                diskofferingid=small_disk_offering.id,
+                domainid=self.account.domainid,
+                zoneid=self.zone.id
+            )
+            small_virtual_machine.attach_volume(self.apiclient, vol1)
+            self.debug("Clone VM - ID: %s" % small_virtual_machine.id)
+            try:
+                clone_response = small_virtual_machine.clone(self.apiclient, small_virtual_machine)

Review comment:
       this is not what I mean, unless the clone_response *is* the resulting VM, I didn't check.




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



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

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#discussion_r708331939



##########
File path: test/integration/smoke/test_vm_life_cycle.py
##########
@@ -2085,14 +2085,19 @@ def setUp(self):
         self.cleanup = []
 
     def tearDown(self):
-        super(TestCloneVM, self).tearDown()
+        try:
+            cleanup_resources(self.apiclient, self.cleanup)
+        except Exception as e:
+            raise Exception("Warning: Exception during cleanup : %s" % e)

Review comment:
       please revert this change before merging




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



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

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#issuecomment-919209165


   @atrocitytheme you have conflicts. Can you have a look?


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



[GitHub] [cloudstack] blueorangutan removed a comment on pull request #5216: Support for new Feature: Clone a Virtual Machine (#4818)

Posted by GitBox <gi...@apache.org>.
blueorangutan removed a comment on pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#issuecomment-963125220






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



[GitHub] [cloudstack] atrocitytheme edited a comment on pull request #5216: Support for new Feature: Clone a Virtual Machine (#4818)

Posted by GitBox <gi...@apache.org>.
atrocitytheme edited a comment on pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#issuecomment-1059689613


   > Hi @atrocitytheme can you please fix the conflicts?
   
   Just pushed a commit that can resolve the conflict. Seems some of the methods are either deleted or changed, causing compatibility issues


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



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

Posted by GitBox <gi...@apache.org>.
nvazquez commented on pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#issuecomment-1059660316


   Hi @atrocitytheme can you please fix the conflicts?


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



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

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#discussion_r692766471



##########
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")

Review comment:
       @atrocitytheme can you add a reason why you didn't appluy this sugestion?




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



[GitHub] [cloudstack] sureshanaparti closed pull request #5216: Support for new Feature: Clone a Virtual Machine (#4818)

Posted by GitBox <gi...@apache.org>.
sureshanaparti closed pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216


   


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



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

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#discussion_r708331939



##########
File path: test/integration/smoke/test_vm_life_cycle.py
##########
@@ -2085,14 +2085,19 @@ def setUp(self):
         self.cleanup = []
 
     def tearDown(self):
-        super(TestCloneVM, self).tearDown()
+        try:
+            cleanup_resources(self.apiclient, self.cleanup)
+        except Exception as e:
+            raise Exception("Warning: Exception during cleanup : %s" % e)

Review comment:
       please use the parent method , change before merging
   ```suggestion
           super(TestCloneVM,self).tearDown()
   ```




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



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

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#issuecomment-964636760


   <b>Trillian test result (tid-2549)</b>
   Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 33154 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5216-t2549-kvm-centos7.zip
   Smoke tests completed. 90 look OK, 1 have errors
   Only failed tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   test_clone_vm_and_volumes | `Error` | 89.55 | test_vm_life_cycle.py
   


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



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

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#issuecomment-963182442


   Packaging result: :heavy_multiplication_x: el7 :heavy_multiplication_x: el8 :heavy_multiplication_x: debian :heavy_multiplication_x: suse15. SL-JID 1686


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



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

Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#discussion_r687960367



##########
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);

Review comment:
       check if volume and snapshot service objects are required to be passed here, or these can be accessed from the service layer?




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



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

Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#issuecomment-963360231


   @blueorangutan package


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



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

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#discussion_r692765756



##########
File path: api/src/main/java/org/apache/cloudstack/api/command/user/vm/CloneVMCmd.java
##########
@@ -0,0 +1,169 @@
+package org.apache.cloudstack.api.command.user.vm;

Review comment:
       we need a license header above.




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



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

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#discussion_r693533406



##########
File path: test/integration/smoke/test_vm_life_cycle.py
##########
@@ -1991,3 +1990,135 @@ def test_01_vapps_vm_cycle(self):
             cmd = destroyVirtualMachine.destroyVirtualMachineCmd()
             cmd.id = vm.id
             self.apiclient.destroyVirtualMachine(cmd)
+
+class TestCloneVM(cloudstackTestCase):
+    
+    @classmethod
+    def setUpClass(cls):
+        testClient = super(TestCloneVM, cls).getClsTestClient()
+        cls.apiclient = testClient.getApiClient()
+        cls.services = testClient.getParsedTestDataConfig()
+        cls.hypervisor = testClient.getHypervisorInfo()
+
+        # Get Zone, Domain and templates
+        domain = get_domain(cls.apiclient)
+        cls.zone = get_zone(cls.apiclient, cls.testClient.getZoneForTests())
+        cls.services['mode'] = cls.zone.networktype
+
+        # if local storage is enabled, alter the offerings to use localstorage
+        # this step is needed for devcloud
+        if cls.zone.localstorageenabled == True:
+            cls.services["service_offerings"]["tiny"]["storagetype"] = 'local'
+            cls.services["service_offerings"]["small"]["storagetype"] = 'local'
+            cls.services["service_offerings"]["medium"]["storagetype"] = 'local'
+
+        template = get_suitable_test_template(
+            cls.apiclient,
+            cls.zone.id,
+            cls.services["ostype"],
+            cls.hypervisor
+        )
+        if template == FAILED:
+            assert False, "get_suitable_test_template() failed to return template with description %s" % cls.services["ostype"]
+
+        # Set Zones and disk offerings
+        cls.services["small"]["zoneid"] = cls.zone.id
+        cls.services["small"]["template"] = template.id
+
+        cls.services["iso1"]["zoneid"] = cls.zone.id
+
+        # Create VMs, NAT Rules etc
+        cls.account = Account.create(
+            cls.apiclient,
+            cls.services["account"],
+            domainid=domain.id
+        )
+
+        cls.small_offering = ServiceOffering.create(
+            cls.apiclient,
+            cls.services["service_offerings"]["small"]
+        )
+
+        cls.medium_offering = ServiceOffering.create(
+            cls.apiclient,
+            cls.services["service_offerings"]["medium"]
+        )
+        # create small and large virtual machines
+        cls.small_virtual_machine = VirtualMachine.create(
+            cls.apiclient,
+            cls.services["small"],
+            accountid=cls.account.name,
+            domainid=cls.account.domainid,
+            serviceofferingid=cls.small_offering.id,
+            mode=cls.services["mode"]
+        )
+        cls.medium_virtual_machine = VirtualMachine.create(
+            cls.apiclient,
+            cls.services["small"],
+            accountid=cls.account.name,
+            domainid=cls.account.domainid,
+            serviceofferingid=cls.medium_offering.id,
+            mode=cls.services["mode"]
+        )
+        cls.virtual_machine = VirtualMachine.create(
+            cls.apiclient,
+            cls.services["small"],
+            accountid=cls.account.name,
+            domainid=cls.account.domainid,
+            serviceofferingid=cls.small_offering.id,
+            mode=cls.services["mode"]
+        )
+        cls._cleanup = [
+            cls.small_offering,
+            cls.medium_offering,
+            cls.account
+        ]
+
+    @classmethod
+    def tearDownClass(cls):
+        super(TestCloneVM, cls).tearDownClass()
+
+    def setUp(self):
+        self.apiclient = self.testClient.getApiClient()
+        self.dbclient = self.testClient.getDbConnection()
+        self.cleanup = []
+
+    def tearDown(self):
+        try:
+            # Clean up, terminate the created ISOs
+            cleanup_resources(self.apiclient, self.cleanup)
+        except Exception as e:
+            raise Exception("Warning: Exception during cleanup : %s" % e)
+        return
+
+    @attr(tags = ["clone","devcloud", "advanced", "smoke", "basic", "sg"], required_hardware="false")
+    def test_clone_vm_and_volumes(self):
+        small_disk_offering = DiskOffering.list(self.apiclient, name='Small')[0];
+        config = Configurations.list(self.apiclient,
+                              name="kvm.snapshot.enabled"
+                              )
+        if len(config) == 0 or config[0].value != "true":
+            self.skipTest("Please enable kvm.snapshot.enable global config")
+        if self.hypervisor.lower() in ["kvm", "simulator"]:
+            small_virtual_machine = VirtualMachine.create(
+                self.apiclient,
+                self.services["small"],
+                accountid=self.account.name,
+                domainid=self.account.domainid,
+                serviceofferingid=self.small_offering.id,)
+            vol1 = Volume.create(
+                self.apiclient,
+                self.services,
+                account=self.account.name,
+                diskofferingid=small_disk_offering.id,
+                domainid=self.account.domainid,
+                zoneid=self.zone.id
+            )
+            small_virtual_machine.attach_volume(self.apiclient, vol1)
+            self.debug("Clone VM - ID: %s" % small_virtual_machine.id)
+            try:
+                clone_response = small_virtual_machine.clone(self.apiclient, small_virtual_machine)

Review comment:
       checked, it is, should work as meant, thnx




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



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

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#issuecomment-903782490


   Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 977


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



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

Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#discussion_r687965480



##########
File path: server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
##########
@@ -1698,6 +1705,11 @@ private Volume orchestrateAttachVolumeToVM(Long vmId, Long volumeId, Long device
         return newVol;
     }
 
+    @Override
+    public Volume attachVolumeToVm(CloneVMCmd cmd, Long volumeId, Long deviceId) {
+        return attachVolumeToVM(cmd.getEntityId(), volumeId, deviceId);
+    }

Review comment:
       can you reuse the existing method below.




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



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

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#issuecomment-903759915


   @sureshanaparti a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.


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



[GitHub] [cloudstack] blueorangutan removed a comment on pull request #5216: Support for new Feature: Clone a Virtual Machine (#4818)

Posted by GitBox <gi...@apache.org>.
blueorangutan removed a comment on pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#issuecomment-963172630






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



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

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#issuecomment-1059780954


   @nvazquez a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests


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



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

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#issuecomment-1059863379


   <b>Trillian test result (tid-3505)</b>
   Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 33457 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5216-t3505-kvm-centos7.zip
   Smoke tests completed. 91 look OK, 1 have errors
   Only failed tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   test_clone_vm_and_volumes | `Error` | 89.47 | test_vm_life_cycle.py
   


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



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

Posted by GitBox <gi...@apache.org>.
nvazquez commented on pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#issuecomment-1059950414


   Thanks @atrocitytheme - the introduced marvin test is now failing, can you please check/fix? Please advise if you need help


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



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

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#issuecomment-963423058


   @sureshanaparti a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress.


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



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

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#issuecomment-903699828


   Packaging result: :heavy_check_mark: el7 :heavy_multiplication_x: el8 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 975


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



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

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#issuecomment-904307530


   <b>Trillian test result (tid-1753)</b>
   Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 35551 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5216-t1753-kvm-centos7.zip
   Intermittent failure detected: /marvin/tests/smoke/test_vm_life_cycle.py
   Smoke tests completed. 88 look OK, 1 have error(s)
   Only failed tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   test_clone_vm_and_volumes | `Error` | 100.64 | test_vm_life_cycle.py
   


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



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

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#discussion_r692788328



##########
File path: test/integration/smoke/test_vm_life_cycle.py
##########
@@ -631,7 +631,7 @@ def test_08_migrate_vm(self):
         if self.hypervisor.lower() in ["lxc"]:
             self.skipTest("Migration is not supported on LXC")
 
-        # For KVM, two hosts used for migration should  be present in same cluster
+        # For KVM, two hosts used fo r migration should  be present in same cluster

Review comment:
       accidental space




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



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

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#discussion_r692770925



##########
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);

Review comment:
       @atrocitytheme , I unresolved this comment as it is still valid. `VolumeService` is accessible and `SnapshotService` can easily be added to the `UserVmServiceImpl`. no reason to pass them.




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



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

Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#discussion_r687954670



##########
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);

Review comment:
       Better move the code at line# 105 -112 to User VM service.




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



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

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#issuecomment-1059774292


   @nvazquez a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.


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



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

Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#issuecomment-963125040


   @blueorangutan package


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



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

Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#issuecomment-963218952


   @blueorangutan package


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



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

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#issuecomment-963224938


   Packaging result: :heavy_multiplication_x: el7 :heavy_multiplication_x: el8 :heavy_multiplication_x: debian :heavy_multiplication_x: suse15. SL-JID 1687


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



[GitHub] [cloudstack] atrocitytheme edited a comment on pull request #5216: Support for new Feature: Clone a Virtual Machine (#4818)

Posted by GitBox <gi...@apache.org>.
atrocitytheme edited a comment on pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#issuecomment-1059689613


   > Hi @atrocitytheme can you please fix the conflicts?
   
   Just pushed a commit that can resolve the conflict. Seems some of the project methods this PR relies on are either deleted or changed, causing compatibility issues


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



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

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#discussion_r692766471



##########
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")

Review comment:
       @atrocitytheme can you add a reason why you didn't apply this sugestion?

##########
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")

Review comment:
       @atrocitytheme can you add a reason why you didn't apply this suggestion?




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



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

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#issuecomment-963125220


   @sureshanaparti a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.


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



[GitHub] [cloudstack] sureshanaparti removed a comment on pull request #5216: Support for new Feature: Clone a Virtual Machine (#4818)

Posted by GitBox <gi...@apache.org>.
sureshanaparti removed a comment on pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#issuecomment-963170879


   @blueorangutan package


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



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

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#issuecomment-963172630


   @sureshanaparti a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.


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



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

Posted by GitBox <gi...@apache.org>.
nvazquez commented on pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#issuecomment-1070340145


   No problem @atrocitytheme - I will move this PR for the next milestone then


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



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

Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#discussion_r687952414



##########
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;

Review comment:
       typo _temporaryTemplateId_




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



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

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#discussion_r692775212



##########
File path: test/integration/smoke/test_vm_life_cycle.py
##########
@@ -1991,3 +1990,135 @@ def test_01_vapps_vm_cycle(self):
             cmd = destroyVirtualMachine.destroyVirtualMachineCmd()
             cmd.id = vm.id
             self.apiclient.destroyVirtualMachine(cmd)
+
+class TestCloneVM(cloudstackTestCase):
+    
+    @classmethod
+    def setUpClass(cls):
+        testClient = super(TestCloneVM, cls).getClsTestClient()
+        cls.apiclient = testClient.getApiClient()
+        cls.services = testClient.getParsedTestDataConfig()
+        cls.hypervisor = testClient.getHypervisorInfo()
+
+        # Get Zone, Domain and templates
+        domain = get_domain(cls.apiclient)
+        cls.zone = get_zone(cls.apiclient, cls.testClient.getZoneForTests())
+        cls.services['mode'] = cls.zone.networktype
+
+        # if local storage is enabled, alter the offerings to use localstorage
+        # this step is needed for devcloud
+        if cls.zone.localstorageenabled == True:
+            cls.services["service_offerings"]["tiny"]["storagetype"] = 'local'
+            cls.services["service_offerings"]["small"]["storagetype"] = 'local'
+            cls.services["service_offerings"]["medium"]["storagetype"] = 'local'
+
+        template = get_suitable_test_template(
+            cls.apiclient,
+            cls.zone.id,
+            cls.services["ostype"],
+            cls.hypervisor
+        )
+        if template == FAILED:
+            assert False, "get_suitable_test_template() failed to return template with description %s" % cls.services["ostype"]
+
+        # Set Zones and disk offerings
+        cls.services["small"]["zoneid"] = cls.zone.id
+        cls.services["small"]["template"] = template.id
+
+        cls.services["iso1"]["zoneid"] = cls.zone.id
+
+        # Create VMs, NAT Rules etc
+        cls.account = Account.create(
+            cls.apiclient,
+            cls.services["account"],
+            domainid=domain.id
+        )
+
+        cls.small_offering = ServiceOffering.create(
+            cls.apiclient,
+            cls.services["service_offerings"]["small"]
+        )
+
+        cls.medium_offering = ServiceOffering.create(
+            cls.apiclient,
+            cls.services["service_offerings"]["medium"]
+        )
+        # create small and large virtual machines
+        cls.small_virtual_machine = VirtualMachine.create(
+            cls.apiclient,
+            cls.services["small"],
+            accountid=cls.account.name,
+            domainid=cls.account.domainid,
+            serviceofferingid=cls.small_offering.id,
+            mode=cls.services["mode"]
+        )

Review comment:
       ```suggestion
           cls.small_virtual_machine = VirtualMachine.create(
               cls.apiclient,
               cls.services["small"],
               accountid=cls.account.name,
               domainid=cls.account.domainid,
               serviceofferingid=cls.small_offering.id,
               mode=cls.services["mode"]
           )
           cls._cleanup.append(cls.small_virtual_machine)
   ```

##########
File path: test/integration/smoke/test_vm_life_cycle.py
##########
@@ -1991,3 +1990,135 @@ def test_01_vapps_vm_cycle(self):
             cmd = destroyVirtualMachine.destroyVirtualMachineCmd()
             cmd.id = vm.id
             self.apiclient.destroyVirtualMachine(cmd)
+
+class TestCloneVM(cloudstackTestCase):
+    
+    @classmethod
+    def setUpClass(cls):
+        testClient = super(TestCloneVM, cls).getClsTestClient()
+        cls.apiclient = testClient.getApiClient()
+        cls.services = testClient.getParsedTestDataConfig()
+        cls.hypervisor = testClient.getHypervisorInfo()
+
+        # Get Zone, Domain and templates
+        domain = get_domain(cls.apiclient)
+        cls.zone = get_zone(cls.apiclient, cls.testClient.getZoneForTests())
+        cls.services['mode'] = cls.zone.networktype
+
+        # if local storage is enabled, alter the offerings to use localstorage
+        # this step is needed for devcloud
+        if cls.zone.localstorageenabled == True:
+            cls.services["service_offerings"]["tiny"]["storagetype"] = 'local'
+            cls.services["service_offerings"]["small"]["storagetype"] = 'local'
+            cls.services["service_offerings"]["medium"]["storagetype"] = 'local'
+
+        template = get_suitable_test_template(
+            cls.apiclient,
+            cls.zone.id,
+            cls.services["ostype"],
+            cls.hypervisor
+        )
+        if template == FAILED:
+            assert False, "get_suitable_test_template() failed to return template with description %s" % cls.services["ostype"]
+
+        # Set Zones and disk offerings
+        cls.services["small"]["zoneid"] = cls.zone.id
+        cls.services["small"]["template"] = template.id
+
+        cls.services["iso1"]["zoneid"] = cls.zone.id
+
+        # Create VMs, NAT Rules etc
+        cls.account = Account.create(
+            cls.apiclient,
+            cls.services["account"],
+            domainid=domain.id
+        )
+
+        cls.small_offering = ServiceOffering.create(
+            cls.apiclient,
+            cls.services["service_offerings"]["small"]
+        )
+
+        cls.medium_offering = ServiceOffering.create(
+            cls.apiclient,
+            cls.services["service_offerings"]["medium"]
+        )

Review comment:
       ```suggestion
           cls.medium_offering = ServiceOffering.create(
               cls.apiclient,
               cls.services["service_offerings"]["medium"]
           )
           cls._cleanup.append(cls.medium_offering)
   ```

##########
File path: test/integration/smoke/test_vm_life_cycle.py
##########
@@ -1991,3 +1990,135 @@ def test_01_vapps_vm_cycle(self):
             cmd = destroyVirtualMachine.destroyVirtualMachineCmd()
             cmd.id = vm.id
             self.apiclient.destroyVirtualMachine(cmd)
+
+class TestCloneVM(cloudstackTestCase):
+    
+    @classmethod
+    def setUpClass(cls):
+        testClient = super(TestCloneVM, cls).getClsTestClient()
+        cls.apiclient = testClient.getApiClient()
+        cls.services = testClient.getParsedTestDataConfig()
+        cls.hypervisor = testClient.getHypervisorInfo()
+
+        # Get Zone, Domain and templates
+        domain = get_domain(cls.apiclient)
+        cls.zone = get_zone(cls.apiclient, cls.testClient.getZoneForTests())
+        cls.services['mode'] = cls.zone.networktype
+
+        # if local storage is enabled, alter the offerings to use localstorage
+        # this step is needed for devcloud
+        if cls.zone.localstorageenabled == True:
+            cls.services["service_offerings"]["tiny"]["storagetype"] = 'local'
+            cls.services["service_offerings"]["small"]["storagetype"] = 'local'
+            cls.services["service_offerings"]["medium"]["storagetype"] = 'local'
+
+        template = get_suitable_test_template(
+            cls.apiclient,
+            cls.zone.id,
+            cls.services["ostype"],
+            cls.hypervisor
+        )
+        if template == FAILED:
+            assert False, "get_suitable_test_template() failed to return template with description %s" % cls.services["ostype"]
+
+        # Set Zones and disk offerings
+        cls.services["small"]["zoneid"] = cls.zone.id
+        cls.services["small"]["template"] = template.id
+
+        cls.services["iso1"]["zoneid"] = cls.zone.id
+
+        # Create VMs, NAT Rules etc
+        cls.account = Account.create(
+            cls.apiclient,
+            cls.services["account"],
+            domainid=domain.id
+        )
+
+        cls.small_offering = ServiceOffering.create(
+            cls.apiclient,
+            cls.services["service_offerings"]["small"]
+        )
+
+        cls.medium_offering = ServiceOffering.create(
+            cls.apiclient,
+            cls.services["service_offerings"]["medium"]
+        )
+        # create small and large virtual machines
+        cls.small_virtual_machine = VirtualMachine.create(
+            cls.apiclient,
+            cls.services["small"],
+            accountid=cls.account.name,
+            domainid=cls.account.domainid,
+            serviceofferingid=cls.small_offering.id,
+            mode=cls.services["mode"]
+        )
+        cls.medium_virtual_machine = VirtualMachine.create(
+            cls.apiclient,
+            cls.services["small"],
+            accountid=cls.account.name,
+            domainid=cls.account.domainid,
+            serviceofferingid=cls.medium_offering.id,
+            mode=cls.services["mode"]
+        )
+        cls.virtual_machine = VirtualMachine.create(
+            cls.apiclient,
+            cls.services["small"],
+            accountid=cls.account.name,
+            domainid=cls.account.domainid,
+            serviceofferingid=cls.small_offering.id,
+            mode=cls.services["mode"]
+        )
+        cls._cleanup = [
+            cls.small_offering,
+            cls.medium_offering,
+            cls.account
+        ]

Review comment:
       ```suggestion
   ```

##########
File path: test/integration/smoke/test_vm_life_cycle.py
##########
@@ -1991,3 +1990,135 @@ def test_01_vapps_vm_cycle(self):
             cmd = destroyVirtualMachine.destroyVirtualMachineCmd()
             cmd.id = vm.id
             self.apiclient.destroyVirtualMachine(cmd)
+
+class TestCloneVM(cloudstackTestCase):
+    
+    @classmethod
+    def setUpClass(cls):
+        testClient = super(TestCloneVM, cls).getClsTestClient()
+        cls.apiclient = testClient.getApiClient()
+        cls.services = testClient.getParsedTestDataConfig()
+        cls.hypervisor = testClient.getHypervisorInfo()
+
+        # Get Zone, Domain and templates
+        domain = get_domain(cls.apiclient)
+        cls.zone = get_zone(cls.apiclient, cls.testClient.getZoneForTests())
+        cls.services['mode'] = cls.zone.networktype
+
+        # if local storage is enabled, alter the offerings to use localstorage
+        # this step is needed for devcloud
+        if cls.zone.localstorageenabled == True:
+            cls.services["service_offerings"]["tiny"]["storagetype"] = 'local'
+            cls.services["service_offerings"]["small"]["storagetype"] = 'local'
+            cls.services["service_offerings"]["medium"]["storagetype"] = 'local'
+
+        template = get_suitable_test_template(
+            cls.apiclient,
+            cls.zone.id,
+            cls.services["ostype"],
+            cls.hypervisor
+        )
+        if template == FAILED:
+            assert False, "get_suitable_test_template() failed to return template with description %s" % cls.services["ostype"]
+
+        # Set Zones and disk offerings
+        cls.services["small"]["zoneid"] = cls.zone.id
+        cls.services["small"]["template"] = template.id
+
+        cls.services["iso1"]["zoneid"] = cls.zone.id
+
+        # Create VMs, NAT Rules etc
+        cls.account = Account.create(
+            cls.apiclient,
+            cls.services["account"],
+            domainid=domain.id
+        )
+
+        cls.small_offering = ServiceOffering.create(
+            cls.apiclient,
+            cls.services["service_offerings"]["small"]
+        )
+
+        cls.medium_offering = ServiceOffering.create(
+            cls.apiclient,
+            cls.services["service_offerings"]["medium"]
+        )
+        # create small and large virtual machines
+        cls.small_virtual_machine = VirtualMachine.create(
+            cls.apiclient,
+            cls.services["small"],
+            accountid=cls.account.name,
+            domainid=cls.account.domainid,
+            serviceofferingid=cls.small_offering.id,
+            mode=cls.services["mode"]
+        )
+        cls.medium_virtual_machine = VirtualMachine.create(
+            cls.apiclient,
+            cls.services["small"],
+            accountid=cls.account.name,
+            domainid=cls.account.domainid,
+            serviceofferingid=cls.medium_offering.id,
+            mode=cls.services["mode"]
+        )
+        cls.virtual_machine = VirtualMachine.create(
+            cls.apiclient,
+            cls.services["small"],
+            accountid=cls.account.name,
+            domainid=cls.account.domainid,
+            serviceofferingid=cls.small_offering.id,
+            mode=cls.services["mode"]
+        )
+        cls._cleanup = [
+            cls.small_offering,
+            cls.medium_offering,
+            cls.account
+        ]
+
+    @classmethod
+    def tearDownClass(cls):
+        super(TestCloneVM, cls).tearDownClass()
+
+    def setUp(self):
+        self.apiclient = self.testClient.getApiClient()
+        self.dbclient = self.testClient.getDbConnection()
+        self.cleanup = []
+
+    def tearDown(self):
+        try:
+            # Clean up, terminate the created ISOs
+            cleanup_resources(self.apiclient, self.cleanup)
+        except Exception as e:
+            raise Exception("Warning: Exception during cleanup : %s" % e)
+        return
+
+    @attr(tags = ["clone","devcloud", "advanced", "smoke", "basic", "sg"], required_hardware="false")
+    def test_clone_vm_and_volumes(self):
+        small_disk_offering = DiskOffering.list(self.apiclient, name='Small')[0];
+        config = Configurations.list(self.apiclient,
+                              name="kvm.snapshot.enabled"
+                              )
+        if len(config) == 0 or config[0].value != "true":
+            self.skipTest("Please enable kvm.snapshot.enable global config")
+        if self.hypervisor.lower() in ["kvm", "simulator"]:
+            small_virtual_machine = VirtualMachine.create(
+                self.apiclient,
+                self.services["small"],
+                accountid=self.account.name,
+                domainid=self.account.domainid,
+                serviceofferingid=self.small_offering.id,)
+            vol1 = Volume.create(
+                self.apiclient,
+                self.services,
+                account=self.account.name,
+                diskofferingid=small_disk_offering.id,
+                domainid=self.account.domainid,
+                zoneid=self.zone.id
+            )
+            small_virtual_machine.attach_volume(self.apiclient, vol1)
+            self.debug("Clone VM - ID: %s" % small_virtual_machine.id)
+            try:
+                clone_response = small_virtual_machine.clone(self.apiclient, small_virtual_machine)

Review comment:
       probably some cleanup item needs to extracted from this response as well.

##########
File path: test/integration/smoke/test_vm_life_cycle.py
##########
@@ -1991,3 +1990,135 @@ def test_01_vapps_vm_cycle(self):
             cmd = destroyVirtualMachine.destroyVirtualMachineCmd()
             cmd.id = vm.id
             self.apiclient.destroyVirtualMachine(cmd)
+
+class TestCloneVM(cloudstackTestCase):
+    
+    @classmethod
+    def setUpClass(cls):
+        testClient = super(TestCloneVM, cls).getClsTestClient()
+        cls.apiclient = testClient.getApiClient()
+        cls.services = testClient.getParsedTestDataConfig()
+        cls.hypervisor = testClient.getHypervisorInfo()
+
+        # Get Zone, Domain and templates
+        domain = get_domain(cls.apiclient)
+        cls.zone = get_zone(cls.apiclient, cls.testClient.getZoneForTests())
+        cls.services['mode'] = cls.zone.networktype
+
+        # if local storage is enabled, alter the offerings to use localstorage
+        # this step is needed for devcloud
+        if cls.zone.localstorageenabled == True:
+            cls.services["service_offerings"]["tiny"]["storagetype"] = 'local'
+            cls.services["service_offerings"]["small"]["storagetype"] = 'local'
+            cls.services["service_offerings"]["medium"]["storagetype"] = 'local'
+
+        template = get_suitable_test_template(
+            cls.apiclient,
+            cls.zone.id,
+            cls.services["ostype"],
+            cls.hypervisor
+        )
+        if template == FAILED:
+            assert False, "get_suitable_test_template() failed to return template with description %s" % cls.services["ostype"]
+
+        # Set Zones and disk offerings
+        cls.services["small"]["zoneid"] = cls.zone.id
+        cls.services["small"]["template"] = template.id
+
+        cls.services["iso1"]["zoneid"] = cls.zone.id
+
+        # Create VMs, NAT Rules etc
+        cls.account = Account.create(
+            cls.apiclient,
+            cls.services["account"],
+            domainid=domain.id
+        )
+
+        cls.small_offering = ServiceOffering.create(
+            cls.apiclient,
+            cls.services["service_offerings"]["small"]
+        )
+
+        cls.medium_offering = ServiceOffering.create(
+            cls.apiclient,
+            cls.services["service_offerings"]["medium"]
+        )
+        # create small and large virtual machines
+        cls.small_virtual_machine = VirtualMachine.create(
+            cls.apiclient,
+            cls.services["small"],
+            accountid=cls.account.name,
+            domainid=cls.account.domainid,
+            serviceofferingid=cls.small_offering.id,
+            mode=cls.services["mode"]
+        )
+        cls.medium_virtual_machine = VirtualMachine.create(
+            cls.apiclient,
+            cls.services["small"],
+            accountid=cls.account.name,
+            domainid=cls.account.domainid,
+            serviceofferingid=cls.medium_offering.id,
+            mode=cls.services["mode"]
+        )

Review comment:
       ```suggestion
           cls.medium_virtual_machine = VirtualMachine.create(
               cls.apiclient,
               cls.services["small"],
               accountid=cls.account.name,
               domainid=cls.account.domainid,
               serviceofferingid=cls.medium_offering.id,
               mode=cls.services["mode"]
           )
           cls._cleanup.append(cls.medium_virtual_machine)
   ```

##########
File path: test/integration/smoke/test_vm_life_cycle.py
##########
@@ -1991,3 +1990,135 @@ def test_01_vapps_vm_cycle(self):
             cmd = destroyVirtualMachine.destroyVirtualMachineCmd()
             cmd.id = vm.id
             self.apiclient.destroyVirtualMachine(cmd)
+
+class TestCloneVM(cloudstackTestCase):
+    
+    @classmethod
+    def setUpClass(cls):
+        testClient = super(TestCloneVM, cls).getClsTestClient()
+        cls.apiclient = testClient.getApiClient()
+        cls.services = testClient.getParsedTestDataConfig()
+        cls.hypervisor = testClient.getHypervisorInfo()
+
+        # Get Zone, Domain and templates
+        domain = get_domain(cls.apiclient)
+        cls.zone = get_zone(cls.apiclient, cls.testClient.getZoneForTests())
+        cls.services['mode'] = cls.zone.networktype
+
+        # if local storage is enabled, alter the offerings to use localstorage
+        # this step is needed for devcloud
+        if cls.zone.localstorageenabled == True:
+            cls.services["service_offerings"]["tiny"]["storagetype"] = 'local'
+            cls.services["service_offerings"]["small"]["storagetype"] = 'local'
+            cls.services["service_offerings"]["medium"]["storagetype"] = 'local'
+
+        template = get_suitable_test_template(
+            cls.apiclient,
+            cls.zone.id,
+            cls.services["ostype"],
+            cls.hypervisor
+        )
+        if template == FAILED:
+            assert False, "get_suitable_test_template() failed to return template with description %s" % cls.services["ostype"]
+
+        # Set Zones and disk offerings
+        cls.services["small"]["zoneid"] = cls.zone.id
+        cls.services["small"]["template"] = template.id
+
+        cls.services["iso1"]["zoneid"] = cls.zone.id
+
+        # Create VMs, NAT Rules etc
+        cls.account = Account.create(
+            cls.apiclient,
+            cls.services["account"],
+            domainid=domain.id
+        )
+
+        cls.small_offering = ServiceOffering.create(
+            cls.apiclient,
+            cls.services["service_offerings"]["small"]
+        )

Review comment:
       
   ```suggestion
           cls.small_offering = ServiceOffering.create(
               cls.apiclient,
               cls.services["service_offerings"]["small"]
           )
           cls._cleanup.append(cls.small_offering)
   ```

##########
File path: test/integration/smoke/test_vm_life_cycle.py
##########
@@ -1991,3 +1990,135 @@ def test_01_vapps_vm_cycle(self):
             cmd = destroyVirtualMachine.destroyVirtualMachineCmd()
             cmd.id = vm.id
             self.apiclient.destroyVirtualMachine(cmd)
+
+class TestCloneVM(cloudstackTestCase):
+    
+    @classmethod
+    def setUpClass(cls):
+        testClient = super(TestCloneVM, cls).getClsTestClient()
+        cls.apiclient = testClient.getApiClient()
+        cls.services = testClient.getParsedTestDataConfig()
+        cls.hypervisor = testClient.getHypervisorInfo()
+
+        # Get Zone, Domain and templates
+        domain = get_domain(cls.apiclient)
+        cls.zone = get_zone(cls.apiclient, cls.testClient.getZoneForTests())
+        cls.services['mode'] = cls.zone.networktype
+
+        # if local storage is enabled, alter the offerings to use localstorage
+        # this step is needed for devcloud
+        if cls.zone.localstorageenabled == True:
+            cls.services["service_offerings"]["tiny"]["storagetype"] = 'local'
+            cls.services["service_offerings"]["small"]["storagetype"] = 'local'
+            cls.services["service_offerings"]["medium"]["storagetype"] = 'local'
+
+        template = get_suitable_test_template(
+            cls.apiclient,
+            cls.zone.id,
+            cls.services["ostype"],
+            cls.hypervisor
+        )
+        if template == FAILED:
+            assert False, "get_suitable_test_template() failed to return template with description %s" % cls.services["ostype"]
+
+        # Set Zones and disk offerings
+        cls.services["small"]["zoneid"] = cls.zone.id
+        cls.services["small"]["template"] = template.id
+
+        cls.services["iso1"]["zoneid"] = cls.zone.id
+
+        # Create VMs, NAT Rules etc
+        cls.account = Account.create(
+            cls.apiclient,
+            cls.services["account"],
+            domainid=domain.id
+        )

Review comment:
       cleanup mechs should be slighhtly different:
   ```suggestion
           cls._cleanup = []
   
               cls.account = Account.create(
               cls.apiclient,
               cls.services["account"],
               domainid=domain.id
           )
           cls._cleanup.append(cls.account)
   ```

##########
File path: test/integration/smoke/test_vm_life_cycle.py
##########
@@ -1991,3 +1990,135 @@ def test_01_vapps_vm_cycle(self):
             cmd = destroyVirtualMachine.destroyVirtualMachineCmd()
             cmd.id = vm.id
             self.apiclient.destroyVirtualMachine(cmd)
+
+class TestCloneVM(cloudstackTestCase):
+    
+    @classmethod
+    def setUpClass(cls):
+        testClient = super(TestCloneVM, cls).getClsTestClient()
+        cls.apiclient = testClient.getApiClient()
+        cls.services = testClient.getParsedTestDataConfig()
+        cls.hypervisor = testClient.getHypervisorInfo()
+
+        # Get Zone, Domain and templates
+        domain = get_domain(cls.apiclient)
+        cls.zone = get_zone(cls.apiclient, cls.testClient.getZoneForTests())
+        cls.services['mode'] = cls.zone.networktype
+
+        # if local storage is enabled, alter the offerings to use localstorage
+        # this step is needed for devcloud
+        if cls.zone.localstorageenabled == True:
+            cls.services["service_offerings"]["tiny"]["storagetype"] = 'local'
+            cls.services["service_offerings"]["small"]["storagetype"] = 'local'
+            cls.services["service_offerings"]["medium"]["storagetype"] = 'local'
+
+        template = get_suitable_test_template(
+            cls.apiclient,
+            cls.zone.id,
+            cls.services["ostype"],
+            cls.hypervisor
+        )
+        if template == FAILED:
+            assert False, "get_suitable_test_template() failed to return template with description %s" % cls.services["ostype"]
+
+        # Set Zones and disk offerings
+        cls.services["small"]["zoneid"] = cls.zone.id
+        cls.services["small"]["template"] = template.id
+
+        cls.services["iso1"]["zoneid"] = cls.zone.id
+
+        # Create VMs, NAT Rules etc
+        cls.account = Account.create(
+            cls.apiclient,
+            cls.services["account"],
+            domainid=domain.id
+        )
+
+        cls.small_offering = ServiceOffering.create(
+            cls.apiclient,
+            cls.services["service_offerings"]["small"]
+        )
+
+        cls.medium_offering = ServiceOffering.create(
+            cls.apiclient,
+            cls.services["service_offerings"]["medium"]
+        )
+        # create small and large virtual machines
+        cls.small_virtual_machine = VirtualMachine.create(
+            cls.apiclient,
+            cls.services["small"],
+            accountid=cls.account.name,
+            domainid=cls.account.domainid,
+            serviceofferingid=cls.small_offering.id,
+            mode=cls.services["mode"]
+        )
+        cls.medium_virtual_machine = VirtualMachine.create(
+            cls.apiclient,
+            cls.services["small"],
+            accountid=cls.account.name,
+            domainid=cls.account.domainid,
+            serviceofferingid=cls.medium_offering.id,
+            mode=cls.services["mode"]
+        )
+        cls.virtual_machine = VirtualMachine.create(
+            cls.apiclient,
+            cls.services["small"],
+            accountid=cls.account.name,
+            domainid=cls.account.domainid,
+            serviceofferingid=cls.small_offering.id,
+            mode=cls.services["mode"]
+        )
+        cls._cleanup = [
+            cls.small_offering,
+            cls.medium_offering,
+            cls.account
+        ]
+
+    @classmethod
+    def tearDownClass(cls):
+        super(TestCloneVM, cls).tearDownClass()
+
+    def setUp(self):
+        self.apiclient = self.testClient.getApiClient()
+        self.dbclient = self.testClient.getDbConnection()
+        self.cleanup = []
+
+    def tearDown(self):
+        try:
+            # Clean up, terminate the created ISOs
+            cleanup_resources(self.apiclient, self.cleanup)
+        except Exception as e:
+            raise Exception("Warning: Exception during cleanup : %s" % e)
+        return

Review comment:
       ```suggestion
           super(TestCloneVM, self).tearDown()
   ```

##########
File path: test/integration/smoke/test_vm_life_cycle.py
##########
@@ -1991,3 +1990,135 @@ def test_01_vapps_vm_cycle(self):
             cmd = destroyVirtualMachine.destroyVirtualMachineCmd()
             cmd.id = vm.id
             self.apiclient.destroyVirtualMachine(cmd)
+
+class TestCloneVM(cloudstackTestCase):
+    
+    @classmethod
+    def setUpClass(cls):
+        testClient = super(TestCloneVM, cls).getClsTestClient()
+        cls.apiclient = testClient.getApiClient()
+        cls.services = testClient.getParsedTestDataConfig()
+        cls.hypervisor = testClient.getHypervisorInfo()
+
+        # Get Zone, Domain and templates
+        domain = get_domain(cls.apiclient)
+        cls.zone = get_zone(cls.apiclient, cls.testClient.getZoneForTests())
+        cls.services['mode'] = cls.zone.networktype
+
+        # if local storage is enabled, alter the offerings to use localstorage
+        # this step is needed for devcloud
+        if cls.zone.localstorageenabled == True:
+            cls.services["service_offerings"]["tiny"]["storagetype"] = 'local'
+            cls.services["service_offerings"]["small"]["storagetype"] = 'local'
+            cls.services["service_offerings"]["medium"]["storagetype"] = 'local'
+
+        template = get_suitable_test_template(
+            cls.apiclient,
+            cls.zone.id,
+            cls.services["ostype"],
+            cls.hypervisor
+        )
+        if template == FAILED:
+            assert False, "get_suitable_test_template() failed to return template with description %s" % cls.services["ostype"]
+
+        # Set Zones and disk offerings
+        cls.services["small"]["zoneid"] = cls.zone.id
+        cls.services["small"]["template"] = template.id
+
+        cls.services["iso1"]["zoneid"] = cls.zone.id
+
+        # Create VMs, NAT Rules etc
+        cls.account = Account.create(
+            cls.apiclient,
+            cls.services["account"],
+            domainid=domain.id
+        )
+
+        cls.small_offering = ServiceOffering.create(
+            cls.apiclient,
+            cls.services["service_offerings"]["small"]
+        )
+
+        cls.medium_offering = ServiceOffering.create(
+            cls.apiclient,
+            cls.services["service_offerings"]["medium"]
+        )
+        # create small and large virtual machines
+        cls.small_virtual_machine = VirtualMachine.create(
+            cls.apiclient,
+            cls.services["small"],
+            accountid=cls.account.name,
+            domainid=cls.account.domainid,
+            serviceofferingid=cls.small_offering.id,
+            mode=cls.services["mode"]
+        )
+        cls.medium_virtual_machine = VirtualMachine.create(
+            cls.apiclient,
+            cls.services["small"],
+            accountid=cls.account.name,
+            domainid=cls.account.domainid,
+            serviceofferingid=cls.medium_offering.id,
+            mode=cls.services["mode"]
+        )
+        cls.virtual_machine = VirtualMachine.create(
+            cls.apiclient,
+            cls.services["small"],
+            accountid=cls.account.name,
+            domainid=cls.account.domainid,
+            serviceofferingid=cls.small_offering.id,
+            mode=cls.services["mode"]
+        )
+        cls._cleanup = [
+            cls.small_offering,
+            cls.medium_offering,
+            cls.account
+        ]
+
+    @classmethod
+    def tearDownClass(cls):
+        super(TestCloneVM, cls).tearDownClass()
+
+    def setUp(self):
+        self.apiclient = self.testClient.getApiClient()
+        self.dbclient = self.testClient.getDbConnection()
+        self.cleanup = []
+
+    def tearDown(self):
+        try:
+            # Clean up, terminate the created ISOs
+            cleanup_resources(self.apiclient, self.cleanup)
+        except Exception as e:
+            raise Exception("Warning: Exception during cleanup : %s" % e)
+        return
+
+    @attr(tags = ["clone","devcloud", "advanced", "smoke", "basic", "sg"], required_hardware="false")
+    def test_clone_vm_and_volumes(self):
+        small_disk_offering = DiskOffering.list(self.apiclient, name='Small')[0];
+        config = Configurations.list(self.apiclient,
+                              name="kvm.snapshot.enabled"
+                              )
+        if len(config) == 0 or config[0].value != "true":
+            self.skipTest("Please enable kvm.snapshot.enable global config")
+        if self.hypervisor.lower() in ["kvm", "simulator"]:
+            small_virtual_machine = VirtualMachine.create(
+                self.apiclient,
+                self.services["small"],
+                accountid=self.account.name,
+                domainid=self.account.domainid,
+                serviceofferingid=self.small_offering.id,)

Review comment:
       ```suggestion
               small_virtual_machine = VirtualMachine.create(
                   self.apiclient,
                   self.services["small"],
                   accountid=self.account.name,
                   domainid=self.account.domainid,
                   serviceofferingid=self.small_offering.id,)
                   self.cleanup.append(small_virtual_machine)
   ```

##########
File path: test/integration/smoke/test_vm_life_cycle.py
##########
@@ -1991,3 +1990,135 @@ def test_01_vapps_vm_cycle(self):
             cmd = destroyVirtualMachine.destroyVirtualMachineCmd()
             cmd.id = vm.id
             self.apiclient.destroyVirtualMachine(cmd)
+
+class TestCloneVM(cloudstackTestCase):
+    
+    @classmethod
+    def setUpClass(cls):
+        testClient = super(TestCloneVM, cls).getClsTestClient()
+        cls.apiclient = testClient.getApiClient()
+        cls.services = testClient.getParsedTestDataConfig()
+        cls.hypervisor = testClient.getHypervisorInfo()
+
+        # Get Zone, Domain and templates
+        domain = get_domain(cls.apiclient)
+        cls.zone = get_zone(cls.apiclient, cls.testClient.getZoneForTests())
+        cls.services['mode'] = cls.zone.networktype
+
+        # if local storage is enabled, alter the offerings to use localstorage
+        # this step is needed for devcloud
+        if cls.zone.localstorageenabled == True:
+            cls.services["service_offerings"]["tiny"]["storagetype"] = 'local'
+            cls.services["service_offerings"]["small"]["storagetype"] = 'local'
+            cls.services["service_offerings"]["medium"]["storagetype"] = 'local'
+
+        template = get_suitable_test_template(
+            cls.apiclient,
+            cls.zone.id,
+            cls.services["ostype"],
+            cls.hypervisor
+        )
+        if template == FAILED:
+            assert False, "get_suitable_test_template() failed to return template with description %s" % cls.services["ostype"]
+
+        # Set Zones and disk offerings
+        cls.services["small"]["zoneid"] = cls.zone.id
+        cls.services["small"]["template"] = template.id
+
+        cls.services["iso1"]["zoneid"] = cls.zone.id
+
+        # Create VMs, NAT Rules etc
+        cls.account = Account.create(
+            cls.apiclient,
+            cls.services["account"],
+            domainid=domain.id
+        )
+
+        cls.small_offering = ServiceOffering.create(
+            cls.apiclient,
+            cls.services["service_offerings"]["small"]
+        )
+
+        cls.medium_offering = ServiceOffering.create(
+            cls.apiclient,
+            cls.services["service_offerings"]["medium"]
+        )
+        # create small and large virtual machines
+        cls.small_virtual_machine = VirtualMachine.create(
+            cls.apiclient,
+            cls.services["small"],
+            accountid=cls.account.name,
+            domainid=cls.account.domainid,
+            serviceofferingid=cls.small_offering.id,
+            mode=cls.services["mode"]
+        )
+        cls.medium_virtual_machine = VirtualMachine.create(
+            cls.apiclient,
+            cls.services["small"],
+            accountid=cls.account.name,
+            domainid=cls.account.domainid,
+            serviceofferingid=cls.medium_offering.id,
+            mode=cls.services["mode"]
+        )
+        cls.virtual_machine = VirtualMachine.create(
+            cls.apiclient,
+            cls.services["small"],
+            accountid=cls.account.name,
+            domainid=cls.account.domainid,
+            serviceofferingid=cls.small_offering.id,
+            mode=cls.services["mode"]
+        )

Review comment:
       ```suggestion
           cls.virtual_machine = VirtualMachine.create(
               cls.apiclient,
               cls.services["small"],
               accountid=cls.account.name,
               domainid=cls.account.domainid,
               serviceofferingid=cls.small_offering.id,
               mode=cls.services["mode"]
           )
           cls._cleanup.append(cls.virtual_machine)
   ```




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



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

Posted by GitBox <gi...@apache.org>.
atrocitytheme commented on pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#issuecomment-903168557


   
   @DaanHoogland Hi, thanks for the feedback. 1. Sure, I can extract and refactor these implementations. 2. It doesn't pick up for billing. May add this in the future commits


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



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

Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#issuecomment-901287073


   @blueorangutan package


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



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

Posted by GitBox <gi...@apache.org>.
atrocitytheme commented on pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#issuecomment-1059689613


   > Hi @atrocitytheme can you please fix the conflicts?
   
   Just pushed a commit that can resolve the conflict


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



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

Posted by GitBox <gi...@apache.org>.
nvazquez commented on pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#issuecomment-1059774173


   Thanks for fixing them @atrocitytheme - yes is this PR has been open for some time there have been changes in between, also merging main branch on it helps getting the latest changes. I will review the PR, thanks
   
   @blueorangutan package


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



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

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#issuecomment-903983170


   @sureshanaparti a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests


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



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

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#discussion_r692788940



##########
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:
       no ;)




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



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

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#discussion_r692793605



##########
File path: server/src/main/java/com/cloud/template/TemplateManagerImpl.java
##########
@@ -1742,6 +1744,272 @@ 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, long snapshotId, long templateId) throws CloudRuntimeException {
+        UserVm curVm = cmd.getTargetVM();
+        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);

Review comment:
       ```suggestion
   ```




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



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

Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#discussion_r687957581



##########
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());

Review comment:
       Move this to service layer




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



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

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#issuecomment-963362398


   @sureshanaparti a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.


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



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

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#issuecomment-963426783


   UI build: :heavy_check_mark:
   Live QA URL: http://qa.cloudstack.cloud:8080/client/pr/5216 (SL-JID-813)


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



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

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#issuecomment-963402919


   Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 1688


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



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

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#issuecomment-903302180


   > @DaanHoogland Hi, thanks for the feedback. 1. Sure, I can extract and refactor these implementations. 2. It doesn't pick up for billing. May add this in the future commits
   ad 1. thanks
   ad 2. ok than we need to hide it behind a global setting that will enable the operator to disable the feature.


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



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

Posted by GitBox <gi...@apache.org>.
nvazquez commented on pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#issuecomment-1059780785


   @blueorangutan test


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



[GitHub] [cloudstack] atrocitytheme edited a comment on pull request #5216: Support for new Feature: Clone a Virtual Machine (#4818)

Posted by GitBox <gi...@apache.org>.
atrocitytheme edited a comment on pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#issuecomment-1059689613


   > Hi @atrocitytheme can you please fix the conflicts?
   
   Just pushed a commit that can resolve the conflict. Seems some of the class methods, on which this PR relies are either deleted or changed, causing compatibility issues


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



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

Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#discussion_r687946429



##########
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;

Review comment:
       ```suggestion
       private Long virtualMachineId;
   ```




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



[GitHub] [cloudstack] DaanHoogland edited a comment on pull request #5216: Support for new Feature: Clone a Virtual Machine (#4818)

Posted by GitBox <gi...@apache.org>.
DaanHoogland edited a comment on pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#issuecomment-903302180


   > @DaanHoogland Hi, thanks for the feedback. 1. Sure, I can extract and refactor these implementations. 2. It doesn't pick up for billing. May add this in the future commits
   
   ad 1. thanks
   ad 2. ok than we need to hide it behind a global setting that will enable the operator to disable the feature.


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



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

Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#discussion_r687938988



##########
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})

Review comment:
       Add 'since' parameter to the API cmd




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



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

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#issuecomment-901303358


   Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian. SL-JID 913


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



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

Posted by GitBox <gi...@apache.org>.
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



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

Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#issuecomment-903759073


   @blueorangutan package


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



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

Posted by GitBox <gi...@apache.org>.
atrocitytheme commented on pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#issuecomment-919210762


   Sure
   On Tue, Sep 14, 2021 at 10:31 AM dahn ***@***.***> wrote:
   
   > @atrocitytheme <https://github.com/atrocitytheme> you have conflicts. Can
   > you have a look?
   >
   > —
   > You are receiving this because you were mentioned.
   > Reply to this email directly, view it on GitHub
   > <https://github.com/apache/cloudstack/pull/5216#issuecomment-919209165>,
   > or unsubscribe
   > <https://github.com/notifications/unsubscribe-auth/AFSZIU3HNDEA6BBHEVWOURTUB5MF7ANCNFSM5AP7CLEA>
   > .
   > Triage notifications on the go with GitHub Mobile for iOS
   > <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
   > or Android
   > <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
   >
   >
   


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



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

Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#discussion_r687933746



##########
File path: api/src/main/java/com/cloud/storage/Snapshot.java
##########
@@ -26,7 +26,7 @@
 
 public interface Snapshot extends ControlledEntity, Identity, InternalIdentity, StateObject<Snapshot.State> {
     public enum Type {
-        MANUAL, RECURRING, TEMPLATE, HOURLY, DAILY, WEEKLY, MONTHLY;
+        MANUAL, RECURRING, TEMPLATE, HOURLY, DAILY, WEEKLY, MONTHLY, INTERNAL;

Review comment:
       Better to add a comment to indicate any new type can be added after INTERNAL (so that the internal enum value [7] doesn't change)




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



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

Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#issuecomment-963422013


   @blueorangutan ui


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



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

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#issuecomment-963219155


   @sureshanaparti a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.


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



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

Posted by GitBox <gi...@apache.org>.
atrocitytheme commented on pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#issuecomment-1060010078


   > Thanks @atrocitytheme - the introduced marvin test is now failing, can you please check/fix? Please advise if you need help
   
   might take some time, will find a time to fix it


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



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

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5216:
URL: https://github.com/apache/cloudstack/pull/5216#issuecomment-1059779088


   Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 2771


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