Skip to content

Destroyvm also removes volumes #18

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

Closed
houthuis wants to merge 15 commits into from
Closed

Destroyvm also removes volumes #18

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
4a154b6
WIP volume validation, volume detach
Jul 9, 2018
880b38e
WIP delete volume code added. functionally complete, cleanup and refa…
Jul 9, 2018
eb929a0
WIP ui code
Jul 24, 2018
12e627b
Adding a test for DestroyVM with volumes
borisstoyanov Jul 25, 2018
e953db9
WIP front end complete
Jul 31, 2018
fbc3fdf
Merge branch 'destroyvm-also-removes-volumes' of https://github.com/s…
Jul 31, 2018
d2f3b35
code cleanup
Jul 31, 2018
bff19b3
Merge branch 'master' of https://github.com/shapeblue/cloudstack into…
Jul 31, 2018
3c7e0cc
improved wording
Jul 31, 2018
83ae447
changes following code review
Jul 31, 2018
6129eb6
changes following code review
Aug 1, 2018
ff5004e
Adding a test for DestroyVM with volumes
borisstoyanov Aug 2, 2018
eb02664
ui enhancement when no volumes are attached to a vm
Aug 3, 2018
9590da1
Merge branch 'destroyvm-also-removes-volumes' of https://github.com/s…
Aug 3, 2018
d996e77
added code to allow users to detach and delete volumes when destroyin…
Jul 9, 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
2 changes: 2 additions & 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 @@ -726,6 +726,8 @@ public class ApiConstants {
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;
}
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
8 changes: 8 additions & 0 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 @@ -1849,6 +1849,14 @@ private void validateRootVolumeDetachAttach(VolumeVO volume, UserVmVO vm) {
}
}

@ActionEvent(eventType = EventTypes.EVENT_VOLUME_DETACH, eventDescription = "detaching volume")
public Volume detachVolumeViaDestroyVM(long vmId, long volumeId) {
Copy link
Copy Markdown
Member

@DaanHoogland DaanHoogland Jul 31, 2018

Choose a reason for hiding this comment

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

Why would this be specific to destroy vm ? isn't it just a detach method and didn't it already exist?

Copy link
Copy Markdown
Member

@DaanHoogland DaanHoogland Jul 31, 2018

Choose a reason for hiding this comment

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

couple of questions here:
Is this really public? it may a only be called as part of destroy volume!
How different is it really (even being consequential) from regular detachVolumeFromVM? it should at least also fire a detach event to keep usage data in check.


Volume result = orchestrateDetachVolumeFromVM(vmId, volumeId);

return result;
}

private Volume orchestrateDetachVolumeFromVM(long vmId, long volumeId) {

Volume volume = _volsDao.findById(volumeId);
Expand Down
236 changes: 155 additions & 81 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 @@ -16,86 +16,6 @@
// under the License.
package com.cloud.vm;

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;

import javax.inject.Inject;
import javax.naming.ConfigurationException;

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 com.cloud.agent.AgentManager;
import com.cloud.agent.api.Answer;
import com.cloud.agent.api.Command;
Expand Down Expand Up @@ -301,6 +221,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 {
Expand Down Expand Up @@ -2751,27 +2749,55 @@ 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.trace("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);
detachVolumesFromVm(volumes);

Copy link
Copy Markdown
Member

@DaanHoogland DaanHoogland Jul 31, 2018

Choose a reason for hiding this comment

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

detach here

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 @@ -6446,4 +6472,52 @@ private boolean checkStatusOfVolumeSnapshots(long vmId, Volume.Type type) {
}
return false;
}

Copy link
Copy Markdown
Member

@DaanHoogland DaanHoogland Jul 31, 2018

Choose a reason for hiding this comment

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

the below methods can do with unit tests

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());
}
}
}
}
Loading