Destroyvm also removes volumes (#2793)

* Allow user to detach and delete volumes when destroyinh VMs

* Minor code refactoring

api/src/main/java/com/cloud/storage/VolumeApiService.java

@@ -79,6 +79,8 @@ public interface VolumeApiService {
 
     Volume attachVolumeToVM(AttachVolumeCmd command);
 
+    Volume detachVolumeViaDestroyVM(long vmId, long volumeId);
+
     Volume detachVolumeFromVM(DetachVolumeCmd cmd);
 
     Snapshot takeSnapshot(Long volumeId, Long policyId, Long snapshotId, Account account, boolean quiescevm, Snapshot.LocationType locationType, boolean asyncBackup)

api/src/main/java/org/apache/cloudstack/api/ApiConstants.java

@@ -714,6 +714,7 @@ public class ApiConstants {
     public static final String STDERR = "stderr";
     public static final String EXITCODE = "exitcode";
     public static final String TARGET_ID = "targetid";
+    public static final String VOLUME_IDS = "volumeids";
 
     public enum HostDetails {
         all, capacity, events, stats, min;

api/src/main/java/org/apache/cloudstack/api/command/user/vm/DestroyVMCmd.java

@@ -31,6 +31,7 @@ import org.apache.cloudstack.api.Parameter;
 import org.apache.cloudstack.api.ResponseObject.ResponseView;
 import org.apache.cloudstack.api.ServerApiException;
 import org.apache.cloudstack.api.response.UserVmResponse;
+import org.apache.cloudstack.api.response.VolumeResponse;
 import org.apache.cloudstack.context.CallContext;
 
 import com.cloud.event.EventTypes;
@@ -63,6 +64,14 @@ public class DestroyVMCmd extends BaseAsyncCmd {
                since = "4.2.1")
     private Boolean expunge;
 
+    @Parameter( name = ApiConstants.VOLUME_IDS,
+                type = CommandType.LIST,
+                collectionType = CommandType.UUID,
+                entityType = VolumeResponse.class,
+                description = "Comma separated list of UUIDs for volumes that will be deleted",
+                since = "4.12.0")
+    private List<Long> volumeIds;
+
     /////////////////////////////////////////////////////
     /////////////////// Accessors ///////////////////////
     /////////////////////////////////////////////////////
@@ -78,6 +87,10 @@ public class DestroyVMCmd extends BaseAsyncCmd {
         return expunge;
     }
 
+    public List<Long> getVolumeIds() {
+        return volumeIds;
+    }
+
     /////////////////////////////////////////////////////
     /////////////// API Implementation///////////////////
     /////////////////////////////////////////////////////

server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java

@@ -1857,8 +1857,12 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic
         }
     }
 
-    private Volume orchestrateDetachVolumeFromVM(long vmId, long volumeId) {
+    @ActionEvent(eventType = EventTypes.EVENT_VOLUME_DETACH, eventDescription = "detaching volume")
+    public Volume detachVolumeViaDestroyVM(long vmId, long volumeId) {
+        return orchestrateDetachVolumeFromVM(vmId, volumeId);
+    }
 
+    private Volume orchestrateDetachVolumeFromVM(long vmId, long volumeId) {
         Volume volume = _volsDao.findById(volumeId);
         VMInstanceVO vm = _vmInstanceDao.findById(vmId);
 
@@ -1869,7 +1873,6 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic
 
         if (hostId == null) {
             hostId = vm.getLastHostId();
-
             HostVO host = _hostDao.findById(hostId);
 
             if (host != null && host.getHypervisorType() == HypervisorType.VMware) {

server/src/main/java/com/cloud/vm/UserVmManagerImpl.java

@@ -39,12 +39,6 @@ import java.util.stream.Collectors;
 import javax.inject.Inject;
 import javax.naming.ConfigurationException;
 
-import org.apache.commons.codec.binary.Base64;
-import org.apache.commons.collections.MapUtils;
-import org.apache.commons.lang3.StringUtils;
-import org.apache.log4j.Logger;
-
-import com.cloud.user.AccountVO;
 import org.apache.cloudstack.acl.ControlledEntity.ACLType;
 import org.apache.cloudstack.acl.SecurityChecker.AccessType;
 import org.apache.cloudstack.affinity.AffinityGroupService;
@@ -97,6 +91,10 @@ import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao;
 import org.apache.cloudstack.storage.datastore.db.StoragePoolVO;
 import org.apache.cloudstack.storage.datastore.db.TemplateDataStoreDao;
 import org.apache.cloudstack.storage.datastore.db.TemplateDataStoreVO;
+import org.apache.commons.codec.binary.Base64;
+import org.apache.commons.collections.MapUtils;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.log4j.Logger;
 
 import com.cloud.agent.AgentManager;
 import com.cloud.agent.api.Answer;
@@ -256,6 +254,7 @@ import com.cloud.template.VirtualMachineTemplate;
 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;
@@ -517,6 +516,9 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir
 
     private static final ConfigKey<Boolean> EnableAdditionalVmConfig = new ConfigKey<>("Advanced", Boolean.class, "enable.additional.vm.configuration",
             "false", "allow additional arbitrary configuration to vm", true, ConfigKey.Scope.Account);
+    private static final ConfigKey<Boolean> VmDestroyForcestop = new ConfigKey<Boolean>("Advanced", Boolean.class, "vm.destroy.forcestop", "false",
+            "On destroy, force-stop takes this value ", true);
+
 
     @Override
     public UserVmVO getVirtualMachine(long vmId) {
@@ -2757,10 +2759,15 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir
         // check if VM exists
         UserVmVO vm = _vmDao.findById(vmId);
 
-        if (vm == null) {
+        if (vm == null || vm.getRemoved() != null) {
             throw new InvalidParameterValueException("unable to find a virtual machine with id " + vmId);
         }
 
+        if (vm.getState() == State.Destroyed || vm.getState() == State.Expunging) {
+            s_logger.debug("Vm id=" + vmId + " is already destroyed");
+            return vm;
+        }
+
         // check if there are active volume snapshots tasks
         s_logger.debug("Checking if there are any ongoing snapshots on the ROOT volumes associated with VM with ID " + vmId);
         if (checkStatusOfVolumeSnapshots(vmId, Volume.Type.ROOT)) {
@@ -2768,6 +2775,15 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir
         }
         s_logger.debug("Found no ongoing snapshots on volume of type ROOT, for the vm with id " + vmId);
 
+        List<VolumeVO> volumes = getVolumesFromIds(cmd);
+
+        checkForUnattachedVolumes(vmId, volumes);
+        validateVolumes(volumes);
+
+        stopVirtualMachine(vmId, VmDestroyForcestop.value());
+
+        detachVolumesFromVm(volumes);
+
         UserVm destroyedVm = destroyVm(vmId, expunge);
         if (expunge) {
             if (!expunge(vm, ctx.getCallingUserId(), ctx.getCallingAccount())) {
@@ -2775,9 +2791,26 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir
             }
         }
 
+        deleteVolumesFromVm(volumes);
+
         return destroyedVm;
     }
 
+    private List<VolumeVO> getVolumesFromIds(DestroyVMCmd cmd) {
+        List<VolumeVO> volumes = new ArrayList<>();
+        if (cmd.getVolumeIds() != null) {
+            for (Long volId : cmd.getVolumeIds()) {
+                VolumeVO vol = _volsDao.findById(volId);
+
+                if (vol == null) {
+                    throw new InvalidParameterValueException("Unable to find volume with ID: " + volId);
+                }
+                volumes.add(vol);
+            }
+        }
+        return volumes;
+    }
+
     @Override
     @DB
     public InstanceGroupVO createVmGroup(CreateVMGroupCmd cmd) {
@@ -6518,4 +6551,52 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir
         }
         return false;
     }
+
+    private void checkForUnattachedVolumes(long vmId, List<VolumeVO> volumes) {
+
+        StringBuilder sb = new StringBuilder();
+
+        for (VolumeVO volume : volumes) {
+            if (volume.getInstanceId() == null || vmId != volume.getInstanceId()) {
+                sb.append(volume.toString() + "; ");
+            }
+        }
+
+        if (!StringUtils.isEmpty(sb.toString())) {
+            throw new InvalidParameterValueException("The following supplied volumes are not attached to the VM: " + sb.toString());
+        }
+    }
+
+    private void validateVolumes(List<VolumeVO> volumes) {
+
+        for (VolumeVO volume : volumes) {
+            if (!(volume.getVolumeType() == Volume.Type.ROOT || volume.getVolumeType() == Volume.Type.DATADISK)) {
+                throw new InvalidParameterValueException("Please specify volume of type " + Volume.Type.DATADISK.toString() + " or " + Volume.Type.ROOT.toString());
+            }
+        }
+    }
+
+    private void detachVolumesFromVm(List<VolumeVO> volumes) {
+
+        for (VolumeVO volume : volumes) {
+
+            Volume detachResult = _volumeService.detachVolumeViaDestroyVM(volume.getInstanceId(), volume.getId());
+
+            if (detachResult == null) {
+                s_logger.error("DestroyVM remove volume - failed to detach and delete volume " + volume.getInstanceId() + " from instance " + volume.getId());
+            }
+        }
+    }
+
+    private void deleteVolumesFromVm(List<VolumeVO> volumes) {
+
+        for (VolumeVO volume : volumes) {
+
+            boolean deleteResult = _volumeService.deleteVolume(volume.getId(), CallContext.current().getCallingAccount());
+
+            if (!deleteResult) {
+                s_logger.error("DestroyVM remove volume - failed to delete volume " + volume.getInstanceId() + " from instance " + volume.getId());
+            }
+        }
+    }
 }

test/integration/smoke/test_vm_life_cycle.py

@@ -32,7 +32,9 @@ from marvin.lib.base import (Account,
                              Host,
                              Iso,
                              Router,
-                             Configurations)
+                             Configurations,
+                             Volume,
+                             DiskOffering)
 from marvin.lib.common import (get_domain,
                                 get_zone,
                                 get_template,
@@ -786,6 +788,43 @@ class TestVMLifeCycle(cloudstackTestCase):
                          )
         return
 
+    @attr(tags = ["devcloud", "advanced", "advancedns", "smoke", "basic", "sg"], required_hardware="false")
+    def test_11_destroy_vm_and_volumes(self):
+        """Test destroy Virtual Machine and it's volumes
+        """
+
+        # Validate the following
+        # 1. Deploys a VM and attaches disks to it
+        # 2. Destroys the VM with DataDisks option
+
+        small_disk_offering = DiskOffering.list(self.apiclient, name='Small')[0]
+
+        small_virtual_machine = VirtualMachine.create(
+            self.apiclient,
+            self.services["small"],
+            accountid=self.account.name,
+            domainid=self.account.domainid,
+            serviceofferingid=self.small_offering.id,
+            mode=self.services["mode"]
+        )
+        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("Destroy VM - ID: %s" % small_virtual_machine.id)
+        small_virtual_machine.delete(self.apiclient, volumeIds=vol1.id)
+
+        self.assertEqual(VirtualMachine.list(self.apiclient, id=small_virtual_machine.id), None, "List response contains records when it should not")
+
+        self.assertEqual(Volume.list(self.apiclient, id=vol1.id), None, "List response contains records when it should not")
+
 class TestSecuredVmMigration(cloudstackTestCase):
 
     @classmethod

ui/css/cloudstack3.css

@@ -4179,6 +4179,15 @@ textarea {
   float: left;
 }
 
+.ui-dialog div.form-container div.value label {
+  display: block;
+  width: 119px;
+  text-align: left;
+  font-size: 13px;
+  margin-top: 2px;
+  margin-left: -10px;
+}
+
 .ui-dialog div.form-container div.value input.hasDatepicker {
   color: #2F5D86;
   cursor: pointer;

ui/l10n/en.js

@@ -640,6 +640,7 @@ var dictionary = {
 "label.delete.secondary.staging.store":"Delete Secondary Staging Store",
 "label.delete.sslcertificate":"Delete SSL Certificate",
 "label.delete.ucs.manager":"Delete UCS Manager",
+"label.delete.volumes":"Volumes to be deleted",
 "label.delete.vpn.user":"Delete VPN user",
 "label.deleting.failed":"Deleting Failed",
 "label.deleting.processing":"Deleting....",
@@ -1813,6 +1814,8 @@ var dictionary = {
 "label.volgroup":"Volume Group",
 "label.volume":"Volume",
 "label.volume.details":"Volume details",
+"label.volume.empty":"No volumes attached to this VM",
+"label.volume.ids":"Volume ID's",
 "label.volume.limits":"Volume Limits",
 "label.volume.migrated":"Volume migrated",
 "label.volume.name":"Volume Name",

ui/scripts/instances.js

@@ -112,6 +112,34 @@
                         label: 'label.expunge',
                         isBoolean: true,
                         isChecked: false
+                    },
+                    volumes: {
+                        label: 'label.delete.volumes',
+                        isBoolean: true,
+                        isChecked: false
+                    },
+                    volumeids: {
+                        label: 'label.volume.ids',
+                        dependsOn: 'volumes',
+                        isBoolean: true,
+                        isHidden: true,
+                        emptyMessage: 'label.volume.empty',
+                        multiDataArray: true,
+                        multiData: function(args) {
+                            $.ajax({
+                                url: createURL("listVolumes&virtualMachineId=" + args.context.instances[0].id) + "&type=DATADISK",
+                                  dataType: "json",
+                                  async: true,
+                                  success: function(json) {
+                                    var volumes = json.listvolumesresponse.volume;
+                                    args.response.success({
+                                        descriptionField: 'name',
+                                        valueField: 'id',
+                                        data: volumes
+                                    });
+                                  }
+                            });
+                        }
                     }
                 }
             },
@@ -126,6 +154,26 @@
                             expunge: true
                         });
                     }
+                    if (args.data.volumes == 'on') {
+
+                        var regex = RegExp('[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}');
+
+                        var selectedVolumes = [];
+
+                        for (var key in args.data) {
+                            var matches = key.match(regex);
+
+                            if (matches != null) {
+                                selectedVolumes.push(key);
+                            }
+                        }
+
+                        $.extend(data, {
+                            volumeids: $(selectedVolumes).map(function(index, volume) {
+                                return volume;
+                            }).toArray().join(',')
+                        });
+                    }
                     $.ajax({
                         url: createURL('destroyVirtualMachine'),
                         data: data,

ui/scripts/ui/dialog.js

@@ -433,6 +433,68 @@
                             );
                         });
 
+                    } else if (field.multiDataArray) {
+
+                        $input = $('<div>');
+
+                        multiArgs = {
+                            context: args.context,
+                            response: {
+                                success: function(args) {
+                                    if (args.data == undefined || args.data.length == 0) {
+
+                                        var label = field.emptyMessage != null ? field.emptyMessage : 'No data available';
+
+                                        $input
+                                            .addClass('value')
+                                            .appendTo($value)
+                                            .append(
+                                                $('<label>').html(_l(label))
+                                            );
+
+                                    } else {
+
+                                        $input.addClass('multi-array').addClass(key).appendTo($value);
+
+                                        $(args.data).each(function() {
+
+                                            var id;
+                                            if (field.valueField)
+                                                id = this[field.valueField];
+                                             else
+                                                id = this.id !== undefined ? this.id : this.name;
+
+                                            var desc;
+                                            if (args.descriptionField)
+                                                desc = this[args.descriptionField];
+                                            else
+                                                desc = _l(this.description);
+
+                                            $input.append(
+                                                $('<div>')
+                                                    .addClass('item')
+                                                    .append(
+                                                        $.merge(
+                                                            $('<div>').addClass('name').html(_l(desc)),
+                                                            $('<div>').addClass('value').append(
+                                                                $('<input>').attr({
+                                                                    name: id,
+                                                                    type: 'checkbox'
+                                                                })
+                                                                .data('json-obj', this)
+                                                                .appendTo($value)
+                                                            )
+                                                        )
+                                                    )
+                                            );
+                                        });
+                                    }
+                                }
+                            }
+                        }
+
+                        multiFn = field.multiData;
+                        multiFn(multiArgs);
                     } else {
                         $input = $('<input>').attr({
                             name: key,