Skip to content

Destroyvm also removes volumes #2793

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
nvazquez merged 2 commits into from
Nov 30, 2018
Merged

Destroyvm also removes volumes #2793

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0962e74
Allow user to detach and delete volumes when destroyinh VMs
Jul 9, 2018
e159220
Minor code refactoring
Nov 27, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions api/src/main/java/com/cloud/storage/VolumeApiService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
1 change: 1 addition & 0 deletions api/src/main/java/org/apache/cloudstack/api/ApiConstants.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Copy link
Member

@yadvr yadvr Aug 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove newline above, at line 728


public enum HostDetails {
all, capacity, events, stats, min;
Expand Down
13 changes: 13 additions & 0 deletions api/src/main/java/org/apache/cloudstack/api/command/user/vm/DestroyVMCmd.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 ///////////////////////
/////////////////////////////////////////////////////
Expand All @@ -78,6 +87,10 @@ public boolean getExpunge() {
return expunge;
}

public List<Long> getVolumeIds() {
return volumeIds;
}

/////////////////////////////////////////////////////
/////////////// API Implementation///////////////////
/////////////////////////////////////////////////////
Expand Down
7 changes: 5 additions & 2 deletions server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1857,8 +1857,12 @@ 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) {
return orchestrateDetachVolumeFromVM(vmId, volumeId);
}

private Volume orchestrateDetachVolumeFromVM(long vmId, long volumeId) {
Volume volume = _volsDao.findById(volumeId);
VMInstanceVO vm = _vmInstanceDao.findById(vmId);

Expand All @@ -1869,7 +1873,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) {
Expand Down
95 changes: 88 additions & 7 deletions server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,6 @@
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;
Expand Down Expand Up @@ -97,6 +91,10 @@
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;
Expand Down Expand Up @@ -256,6 +254,7 @@
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;
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -2757,27 +2759,58 @@ 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)) {
throw new CloudRuntimeException("There is/are unbacked up snapshot(s) on ROOT volume, vm destroy is not permitted, please try again later.");
}
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);
Copy link
Contributor

@wido wido Aug 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we detach after the VM has been stopped? As the detach might not work, let's say libvirt (KVM) hangs or something else.

If the VM is stopped the detach is a lot easier and you can remove them then.

Copy link
Contributor Author

@houthuis houthuis Aug 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't stop the VM for a regular detach cmd? This has been tested on KVM (with a small and large amount of volumes) and it worked fine. I'm happy to make the change if required though

Copy link
Contributor

@wido wido Aug 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@houthuis Yes, I know. I was just thinking of it, what implications it might have. The stop of the VM just make sures the detach works even better, it saves additional commands to the Agent to detach the data volumes. This is because the KVM Agent needs to talk to libvirt to detach the disks.

Copy link
Contributor

@DaanHoogland DaanHoogland Aug 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wido Can you explain the risk here? It seems no different than a regular detach. Do these fail a lot?

Copy link
Contributor

@wido wido Aug 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DaanHoogland My point is that it's extra calls towards the KVM Agent, it needs to talk to libvirt, etc, etc. Since we are stopping the VM a few seconds later it would be best to do the stop first.

This would make it less calls and thus less error prone.

Copy link
Contributor Author

@houthuis houthuis Sep 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wido this change has been made. please give the PR another review


UserVm destroyedVm = destroyVm(vmId, expunge);
if (expunge) {
if (!expunge(vm, ctx.getCallingUserId(), ctx.getCallingAccount())) {
throw new CloudRuntimeException("Failed to expunge vm " + destroyedVm);
}
}

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) {
Expand Down Expand Up @@ -6518,4 +6551,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());
}
}
}
}
41 changes: 40 additions & 1 deletion test/integration/smoke/test_vm_life_cycle.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,9 @@
Host,
Iso,
Router,
Configurations)
Configurations,
Volume,
DiskOffering)
from marvin.lib.common import (get_domain,
get_zone,
get_template,
Expand Down Expand Up @@ -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
Expand Down
9 changes: 9 additions & 0 deletions ui/css/cloudstack3.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
3 changes: 3 additions & 0 deletions ui/l10n/en.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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....",
Expand Down Expand Up @@ -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",
Expand Down
48 changes: 48 additions & 0 deletions ui/scripts/instances.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
});
}
});
}
}
}
},
Expand All @@ -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,
Expand Down
Loading