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 2018/10/16 12:21:00 UTC
[GitHub] houthuis closed pull request #2793: Destroyvm also removes volumes
houthuis closed pull request #2793: Destroyvm also removes volumes
URL: https://github.com/apache/cloudstack/pull/2793
This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:
As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):
diff --git a/api/src/main/java/com/cloud/storage/VolumeApiService.java b/api/src/main/java/com/cloud/storage/VolumeApiService.java
index ec20c33e17e..91b0bc0712f 100644
--- a/api/src/main/java/com/cloud/storage/VolumeApiService.java
+++ b/api/src/main/java/com/cloud/storage/VolumeApiService.java
@@ -79,6 +79,8 @@
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)
diff --git a/api/src/main/java/org/apache/cloudstack/api/ApiConstants.java b/api/src/main/java/org/apache/cloudstack/api/ApiConstants.java
index b7779cb2a8d..0ea57e0929e 100644
--- a/api/src/main/java/org/apache/cloudstack/api/ApiConstants.java
+++ b/api/src/main/java/org/apache/cloudstack/api/ApiConstants.java
@@ -713,6 +713,7 @@
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;
diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/vm/DestroyVMCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/vm/DestroyVMCmd.java
index 730c6776772..7b359b79047 100644
--- a/api/src/main/java/org/apache/cloudstack/api/command/user/vm/DestroyVMCmd.java
+++ b/api/src/main/java/org/apache/cloudstack/api/command/user/vm/DestroyVMCmd.java
@@ -31,6 +31,7 @@
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 @@
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 boolean getExpunge() {
return expunge;
}
+ public List<Long> getVolumeIds() {
+ return volumeIds;
+ }
+
/////////////////////////////////////////////////////
/////////////// API Implementation///////////////////
/////////////////////////////////////////////////////
diff --git a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
index 0dfb990c8bd..91896ddbab3 100644
--- a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
+++ b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
@@ -1857,8 +1857,13 @@ private void validateRootVolumeDetachAttach(VolumeVO volume, UserVmVO vm) {
}
}
- private Volume orchestrateDetachVolumeFromVM(long vmId, long volumeId) {
+ @ActionEvent(eventType = EventTypes.EVENT_VOLUME_DETACH, eventDescription = "detaching volume")
+ public Volume detachVolumeViaDestroyVM(long vmId, long volumeId) {
+ Volume result = orchestrateDetachVolumeFromVM(vmId, volumeId);
+ return result;
+ }
+ private Volume orchestrateDetachVolumeFromVM(long vmId, long volumeId) {
Volume volume = _volsDao.findById(volumeId);
VMInstanceVO vm = _vmInstanceDao.findById(vmId);
@@ -1869,7 +1874,6 @@ private Volume orchestrateDetachVolumeFromVM(long vmId, long volumeId) {
if (hostId == null) {
hostId = vm.getLastHostId();
-
HostVO host = _hostDao.findById(hostId);
if (host != null && host.getHypervisorType() == HypervisorType.VMware) {
diff --git a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
index a06b595fa89..13f7e79ae38 100644
--- a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
+++ b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
@@ -302,6 +302,84 @@
import com.cloud.vm.snapshot.VMSnapshotManager;
import com.cloud.vm.snapshot.VMSnapshotVO;
import com.cloud.vm.snapshot.dao.VMSnapshotDao;
+import org.apache.cloudstack.acl.ControlledEntity.ACLType;
+import org.apache.cloudstack.acl.SecurityChecker.AccessType;
+import org.apache.cloudstack.affinity.AffinityGroupService;
+import org.apache.cloudstack.affinity.AffinityGroupVO;
+import org.apache.cloudstack.affinity.dao.AffinityGroupDao;
+import org.apache.cloudstack.affinity.dao.AffinityGroupVMMapDao;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.BaseCmd.HTTPMethod;
+import org.apache.cloudstack.api.command.admin.vm.AssignVMCmd;
+import org.apache.cloudstack.api.command.admin.vm.RecoverVMCmd;
+import org.apache.cloudstack.api.command.user.vm.AddNicToVMCmd;
+import org.apache.cloudstack.api.command.user.vm.DeployVMCmd;
+import org.apache.cloudstack.api.command.user.vm.DestroyVMCmd;
+import org.apache.cloudstack.api.command.user.vm.RebootVMCmd;
+import org.apache.cloudstack.api.command.user.vm.RemoveNicFromVMCmd;
+import org.apache.cloudstack.api.command.user.vm.ResetVMPasswordCmd;
+import org.apache.cloudstack.api.command.user.vm.ResetVMSSHKeyCmd;
+import org.apache.cloudstack.api.command.user.vm.RestoreVMCmd;
+import org.apache.cloudstack.api.command.user.vm.ScaleVMCmd;
+import org.apache.cloudstack.api.command.user.vm.SecurityGroupAction;
+import org.apache.cloudstack.api.command.user.vm.StartVMCmd;
+import org.apache.cloudstack.api.command.user.vm.UpdateDefaultNicForVMCmd;
+import org.apache.cloudstack.api.command.user.vm.UpdateVMCmd;
+import org.apache.cloudstack.api.command.user.vm.UpdateVmNicIpCmd;
+import org.apache.cloudstack.api.command.user.vm.UpgradeVMCmd;
+import org.apache.cloudstack.api.command.user.vmgroup.CreateVMGroupCmd;
+import org.apache.cloudstack.api.command.user.vmgroup.DeleteVMGroupCmd;
+import org.apache.cloudstack.api.command.user.volume.ResizeVolumeCmd;
+import org.apache.cloudstack.context.CallContext;
+import org.apache.cloudstack.engine.cloud.entity.api.VirtualMachineEntity;
+import org.apache.cloudstack.engine.cloud.entity.api.db.dao.VMNetworkMapDao;
+import org.apache.cloudstack.engine.orchestration.service.NetworkOrchestrationService;
+import org.apache.cloudstack.engine.orchestration.service.VolumeOrchestrationService;
+import org.apache.cloudstack.engine.service.api.OrchestrationService;
+import org.apache.cloudstack.engine.subsystem.api.storage.DataStore;
+import org.apache.cloudstack.engine.subsystem.api.storage.DataStoreManager;
+import org.apache.cloudstack.engine.subsystem.api.storage.PrimaryDataStore;
+import org.apache.cloudstack.engine.subsystem.api.storage.VolumeDataFactory;
+import org.apache.cloudstack.engine.subsystem.api.storage.VolumeInfo;
+import org.apache.cloudstack.engine.subsystem.api.storage.VolumeService;
+import org.apache.cloudstack.engine.subsystem.api.storage.VolumeService.VolumeApiResult;
+import org.apache.cloudstack.framework.async.AsyncCallFuture;
+import org.apache.cloudstack.framework.config.ConfigKey;
+import org.apache.cloudstack.framework.config.Configurable;
+import org.apache.cloudstack.framework.config.dao.ConfigurationDao;
+import org.apache.cloudstack.managed.context.ManagedContextRunnable;
+import org.apache.cloudstack.storage.command.DeleteCommand;
+import org.apache.cloudstack.storage.command.DettachCommand;
+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 javax.inject.Inject;
+import javax.naming.ConfigurationException;
+import java.io.UnsupportedEncodingException;
+import java.net.URLDecoder;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Date;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Map.Entry;
+import java.util.Set;
+import java.util.UUID;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
+import java.util.stream.Collectors;
public class UserVmManagerImpl extends ManagerBase implements UserVmManager, VirtualMachineGuru, UserVmService, Configurable {
@@ -514,6 +592,9 @@
private static final ConfigKey<Boolean> AllowDeployVmIfGivenHostFails = new ConfigKey<Boolean>("Advanced", Boolean.class, "allow.deploy.vm.if.deploy.on.given.host.fails", "false",
"allow vm to deploy on different host if vm fails to deploy on the given host ", true);
+ 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) {
@@ -2746,10 +2827,15 @@ public UserVm destroyVm(DestroyVMCmd cmd) throws ResourceUnavailableException, C
// 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)) {
@@ -2757,6 +2843,15 @@ public UserVm destroyVm(DestroyVMCmd cmd) throws ResourceUnavailableException, C
}
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())) {
@@ -2764,9 +2859,26 @@ public UserVm destroyVm(DestroyVMCmd cmd) throws ResourceUnavailableException, C
}
}
+ 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) {
@@ -6441,4 +6553,52 @@ private boolean checkStatusOfVolumeSnapshots(long vmId, Volume.Type type) {
}
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());
+ }
+ }
+ }
}
\ No newline at end of file
diff --git a/test/integration/smoke/test_vm_life_cycle.py b/test/integration/smoke/test_vm_life_cycle.py
index 5906a94869c..3ba43ac6245 100644
--- a/test/integration/smoke/test_vm_life_cycle.py
+++ b/test/integration/smoke/test_vm_life_cycle.py
@@ -32,7 +32,9 @@
Host,
Iso,
Router,
- Configurations)
+ Configurations,
+ Volume,
+ DiskOffering)
from marvin.lib.common import (get_domain,
get_zone,
get_template,
@@ -786,6 +788,43 @@ def test_10_attachAndDetach_iso(self):
)
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
diff --git a/ui/css/cloudstack3.css b/ui/css/cloudstack3.css
index 9eea2e590d4..3fb90a0b329 100644
--- a/ui/css/cloudstack3.css
+++ b/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;
diff --git a/ui/l10n/en.js b/ui/l10n/en.js
index 59fa35779ec..208749a4a91 100644
--- a/ui/l10n/en.js
+++ b/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....",
@@ -1811,6 +1812,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",
diff --git a/ui/scripts/instances.js b/ui/scripts/instances.js
index 6dc45c067cc..2e64fd96b54 100644
--- a/ui/scripts/instances.js
+++ b/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,
diff --git a/ui/scripts/ui/dialog.js b/ui/scripts/ui/dialog.js
index 1f954d907d0..346e95ed42d 100644
--- a/ui/scripts/ui/dialog.js
+++ b/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,
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services