Skip to content

vmware: Add force parameter to iso attach/detach operations #4907

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
yadvr merged 5 commits into from
Apr 28, 2021
Merged

vmware: Add force parameter to iso attach/detach operations #4907

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c88c3d6
vmware: Add force parameter to iso attach/detach operations
Apr 12, 2021
5180d19
Merge branch '4.15' of https://github.com/apache/cloudstack into add-…
Apr 19, 2021
8ce9d62
Formatted code
Apr 19, 2021
4c227a3
Fix configdrive issue with deploy-as-is templates
Apr 22, 2021
b5a2482
Address comments
Apr 22, 2021
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
4 changes: 2 additions & 2 deletions api/src/main/java/com/cloud/template/TemplateApiService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,9 @@ public interface TemplateApiService {

VirtualMachineTemplate prepareTemplate(long templateId, long zoneId, Long storageId);

boolean detachIso(long vmId);
boolean detachIso(long vmId, boolean forced);
Comment thread
Pearl1594 marked this conversation as resolved.

boolean attachIso(long isoId, long vmId);
boolean attachIso(long isoId, long vmId, boolean forced);

/**
* Deletes a template
Expand Down
10 changes: 9 additions & 1 deletion api/src/main/java/org/apache/cloudstack/api/command/user/iso/AttachIsoCmd.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,10 @@ public class AttachIsoCmd extends BaseAsyncCmd implements UserCmd {
required = true, description = "the ID of the virtual machine")
protected Long virtualMachineId;

@Parameter(name = ApiConstants.FORCED, type = CommandType.BOOLEAN,
description = "If true, ejects existing ISO before attaching on VMware. Default: false", since = "4.15.1")
protected Boolean forced;
Copy link
Copy Markdown
Contributor

@nvazquez nvazquez Apr 15, 2021

Choose a reason for hiding this comment

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

IMO this is not needed. In case there an existing ISO it can be ejected by detachIso setting forced = true

Copy link
Copy Markdown
Contributor Author

@Pearl1594 Pearl1594 Apr 15, 2021

Choose a reason for hiding this comment

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

That's true @nvazquez. However, this has been done in cases where we have a configdrive iso- under such cases, we can't do a detach operation.

Copy link
Copy Markdown
Contributor

@nvazquez nvazquez Apr 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for clarifying @Pearl1594


/////////////////////////////////////////////////////
/////////////////// Accessors ///////////////////////
/////////////////////////////////////////////////////
Expand All @@ -66,6 +70,10 @@ public Long getVirtualMachineId() {
return virtualMachineId;
}

public Boolean isForced() {
return forced != null;
}

/////////////////////////////////////////////////////
/////////////// API Implementation///////////////////
/////////////////////////////////////////////////////
Expand Down Expand Up @@ -98,7 +106,7 @@ public String getEventDescription() {
@Override
public void execute() {
CallContext.current().setEventDetails("Vm Id: " + getVirtualMachineId() + " ISO ID: " + getId());
boolean result = _templateService.attachIso(id, virtualMachineId);
boolean result = _templateService.attachIso(id, virtualMachineId, isForced());
if (result) {
UserVm userVm = _responseGenerator.findUserVmById(virtualMachineId);
if (userVm != null) {
Expand Down
14 changes: 11 additions & 3 deletions api/src/main/java/org/apache/cloudstack/api/command/user/iso/DetachIsoCmd.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,14 @@ public class DetachIsoCmd extends BaseAsyncCmd implements UserCmd {
//////////////// API parameters /////////////////////
/////////////////////////////////////////////////////

@Parameter(name=ApiConstants.VIRTUAL_MACHINE_ID, type=CommandType.UUID, entityType = UserVmResponse.class,
required=true, description="The ID of the virtual machine")
@Parameter(name = ApiConstants.VIRTUAL_MACHINE_ID, type = CommandType.UUID, entityType = UserVmResponse.class,
required = true, description = "The ID of the virtual machine")
protected Long virtualMachineId;

@Parameter(name = ApiConstants.FORCED, type = CommandType.BOOLEAN,
description = "If true, ejects the ISO before detaching on VMware. Default: false", since = "4.15.1")
protected Boolean forced;

/////////////////////////////////////////////////////
/////////////////// Accessors ///////////////////////
/////////////////////////////////////////////////////
Expand All @@ -56,6 +60,10 @@ public Long getVirtualMachineId() {
return virtualMachineId;
}

public Boolean isForced() {
return forced != null;
}

/////////////////////////////////////////////////////
/////////////// API Implementation///////////////////
/////////////////////////////////////////////////////
Expand Down Expand Up @@ -87,7 +95,7 @@ public String getEventDescription() {

@Override
public void execute() {
boolean result = _templateService.detachIso(virtualMachineId);
boolean result = _templateService.detachIso(virtualMachineId, isForced());
if (result) {
UserVm userVm = _entityMgr.findById(UserVm.class, virtualMachineId);
UserVmResponse response = _responseGenerator.createUserVmResponse(getResponseView(), "virtualmachine", userVm).get(0);
Expand Down
9 changes: 9 additions & 0 deletions core/src/main/java/org/apache/cloudstack/storage/command/AttachCommand.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ public final class AttachCommand extends StorageSubSystemCommand {
private DiskTO disk;
private String vmName;
private boolean inSeq = false;
private boolean forced = false;
private Map<String, String> controllerInfo;

public AttachCommand(final DiskTO disk, final String vmName) {
Expand Down Expand Up @@ -69,6 +70,14 @@ public void setVmName(final String vmName) {
this.vmName = vmName;
}

public boolean isForced() {
return forced;
}

public void setForced(boolean forced) {
this.forced = forced;
}

@Override
public void setExecuteInSequence(final boolean inSeq) {
this.inSeq = inSeq;
Expand Down
9 changes: 9 additions & 0 deletions core/src/main/java/org/apache/cloudstack/storage/command/DettachCommand.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ public class DettachCommand extends StorageSubSystemCommand {
private String _storageHost;
private int _storagePort;
private Map<String, String> params;
private boolean forced;

public DettachCommand(final DiskTO disk, final String vmName) {
super();
Expand Down Expand Up @@ -106,6 +107,14 @@ public void setParams(Map<String, String> params) {
this.params = params;
}

public boolean isForced() {
return forced;
}

public void setForced(boolean forced) {
this.forced = forced;
}

@Override
public void setExecuteInSequence(final boolean inSeq) {

Expand Down
82 changes: 51 additions & 31 deletions ...hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2019,35 +2019,7 @@ protected StartAnswer execute(StartCommand cmd) {
if (volIso != null) {
for (DiskTO vol : disks) {
if (vol.getType() == Volume.Type.ISO) {

TemplateObjectTO iso = (TemplateObjectTO) vol.getData();

if (iso.getPath() != null && !iso.getPath().isEmpty()) {
DataStoreTO imageStore = iso.getDataStore();
if (!(imageStore instanceof NfsTO)) {
s_logger.debug("unsupported protocol");
throw new Exception("unsupported protocol");
}
NfsTO nfsImageStore = (NfsTO) imageStore;
String isoPath = nfsImageStore.getUrl() + File.separator + iso.getPath();
Pair<String, ManagedObjectReference> isoDatastoreInfo = getIsoDatastoreInfo(hyperHost, isoPath);
assert (isoDatastoreInfo != null);
assert (isoDatastoreInfo.second() != null);

deviceConfigSpecArray[i] = new VirtualDeviceConfigSpec();
Pair<VirtualDevice, Boolean> isoInfo =
VmwareHelper.prepareIsoDevice(vmMo, isoDatastoreInfo.first(), isoDatastoreInfo.second(), true, true, ideUnitNumber++, i + 1);
deviceConfigSpecArray[i].setDevice(isoInfo.first());
if (isoInfo.second()) {
if (s_logger.isDebugEnabled())
s_logger.debug("Prepare ISO volume at new device " + _gson.toJson(isoInfo.first()));
deviceConfigSpecArray[i].setOperation(VirtualDeviceConfigSpecOperation.ADD);
} else {
if (s_logger.isDebugEnabled())
s_logger.debug("Prepare ISO volume at existing device " + _gson.toJson(isoInfo.first()));
deviceConfigSpecArray[i].setOperation(VirtualDeviceConfigSpecOperation.EDIT);
}
}
configureIso(hyperHost, vmMo, vol, deviceConfigSpecArray, ideUnitNumber++, i);
i++;
}
}
Expand Down Expand Up @@ -2075,8 +2047,16 @@ protected StartAnswer execute(StartCommand cmd) {
//
// Setup ROOT/DATA disk devices
//
if (multipleIsosAtached(sortedDisks) && deployAsIs) {
sortedDisks = getDisks(sortedDisks);
}

for (DiskTO vol : sortedDisks) {
if (vol.getType() == Volume.Type.ISO) {
if (deployAsIs) {
configureIso(hyperHost, vmMo, vol, deviceConfigSpecArray, ideUnitNumber++, i);
i++;
}
continue;
}

Expand Down Expand Up @@ -2457,6 +2437,46 @@ protected StartAnswer execute(StartCommand cmd) {
}
}

private boolean multipleIsosAtached(DiskTO[] sortedDisks) {
return Arrays.stream(sortedDisks).filter(disk -> disk.getType() == Volume.Type.ISO).count() > 1;
}

private DiskTO[] getDisks(DiskTO[] sortedDisks) {
return Arrays.stream(sortedDisks).filter(vol -> ((vol.getPath() != null &&
vol.getPath().contains("configdrive"))) || (vol.getType() != Volume.Type.ISO)).toArray(DiskTO[]::new);
}
private void configureIso(VmwareHypervisorHost hyperHost, VirtualMachineMO vmMo, DiskTO vol,
VirtualDeviceConfigSpec[] deviceConfigSpecArray, int ideUnitNumber, int i) throws Exception {
TemplateObjectTO iso = (TemplateObjectTO) vol.getData();

if (iso.getPath() != null && !iso.getPath().isEmpty()) {
DataStoreTO imageStore = iso.getDataStore();
if (!(imageStore instanceof NfsTO)) {
s_logger.debug("unsupported protocol");
throw new Exception("unsupported protocol");
}
NfsTO nfsImageStore = (NfsTO) imageStore;
String isoPath = nfsImageStore.getUrl() + File.separator + iso.getPath();
Pair<String, ManagedObjectReference> isoDatastoreInfo = getIsoDatastoreInfo(hyperHost, isoPath);
assert (isoDatastoreInfo != null);
assert (isoDatastoreInfo.second() != null);

deviceConfigSpecArray[i] = new VirtualDeviceConfigSpec();
Pair<VirtualDevice, Boolean> isoInfo =
VmwareHelper.prepareIsoDevice(vmMo, isoDatastoreInfo.first(), isoDatastoreInfo.second(), true, true, ideUnitNumber, i + 1);
deviceConfigSpecArray[i].setDevice(isoInfo.first());
if (isoInfo.second()) {
if (s_logger.isDebugEnabled())
s_logger.debug("Prepare ISO volume at new device " + _gson.toJson(isoInfo.first()));
deviceConfigSpecArray[i].setOperation(VirtualDeviceConfigSpecOperation.ADD);
} else {
if (s_logger.isDebugEnabled())
s_logger.debug("Prepare ISO volume at existing device " + _gson.toJson(isoInfo.first()));
deviceConfigSpecArray[i].setOperation(VirtualDeviceConfigSpecOperation.EDIT);
}
}
}

private String mapAdapterType(String adapterStringFromOVF) {
if (StringUtils.isBlank(adapterStringFromOVF) || adapterStringFromOVF.equalsIgnoreCase(VirtualEthernetCardType.E1000.toString())) {
return VirtualEthernetCardType.E1000.toString();
Expand Down Expand Up @@ -5382,7 +5402,7 @@ protected AttachIsoAnswer execute(AttachIsoCommand cmd) {
"Failed to unmount vmware-tools installer ISO as the corresponding CDROM device is locked by VM. Please unmount the CDROM device inside the VM and ret-try.");
}
} catch (Throwable e) {
vmMo.detachIso(null);
vmMo.detachIso(null, cmd.isForce());
Copy link
Copy Markdown
Contributor

@sureshanaparti sureshanaparti Apr 16, 2021

Choose a reason for hiding this comment

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

cmd.isForced() here

Copy link
Copy Markdown
Contributor Author

@Pearl1594 Pearl1594 Apr 19, 2021

Choose a reason for hiding this comment

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

@sureshanaparti isForce() is a function of AttachIsoCommand(not AttachCommand), that already existed, and I'm just calling that.

}
}

Expand Down Expand Up @@ -5411,7 +5431,7 @@ protected AttachIsoAnswer execute(AttachIsoCommand cmd) {
String isoDatastorePath = String.format("[%s] %s%s", storeName, isoStorePathFromRoot, isoFileName);

if (cmd.isAttach()) {
vmMo.attachIso(isoDatastorePath, morSecondaryDs, true, false, cmd.getDeviceKey());
vmMo.attachIso(isoDatastorePath, morSecondaryDs, true, false, cmd.getDeviceKey(), cmd.isForce());
Copy link
Copy Markdown
Contributor

@sureshanaparti sureshanaparti Apr 16, 2021

Choose a reason for hiding this comment

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

cmd.isForced() here

Copy link
Copy Markdown
Contributor Author

@Pearl1594 Pearl1594 Apr 19, 2021

Choose a reason for hiding this comment

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

@sureshanaparti isForce() is a function of AttachIsoCommand(not AttachCommand), that already existed, and I'm just calling that.

Copy link
Copy Markdown
Contributor

@sureshanaparti sureshanaparti Apr 21, 2021

Choose a reason for hiding this comment

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

OK @Pearl1594

return new AttachIsoAnswer(cmd);
} else {
int key = vmMo.detachIso(isoDatastorePath, cmd.isForce());
Expand Down
12 changes: 6 additions & 6 deletions ...s/hypervisors/vmware/src/main/java/com/cloud/storage/resource/VmwareStorageProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2045,7 +2045,7 @@ public Answer backupSnapshot(CopyCommand cmd) {

@Override
public Answer attachIso(AttachCommand cmd) {
return this.attachIso(cmd.getDisk(), true, cmd.getVmName());
return this.attachIso(cmd.getDisk(), true, cmd.getVmName(), cmd.isForced());
}

@Override
Expand Down Expand Up @@ -2411,7 +2411,7 @@ private synchronized ManagedObjectReference prepareSecondaryDatastoreOnHost(Stri
return morDatastore;
}

private Answer attachIso(DiskTO disk, boolean isAttach, String vmName) {
private Answer attachIso(DiskTO disk, boolean isAttach, String vmName, boolean force) {
try {
VmwareContext context = hostService.getServiceContext(null);
VmwareHypervisorHost hyperHost = hostService.getHyperHost(context, null);
Expand Down Expand Up @@ -2441,7 +2441,7 @@ private Answer attachIso(DiskTO disk, boolean isAttach, String vmName) {
return new AttachAnswer("Failed to unmount vmware-tools installer ISO as the corresponding CDROM device is locked by VM. Please unmount the CDROM device inside the VM and ret-try.");
}
} catch(Throwable e){
vmMo.detachIso(null);
vmMo.detachIso(null, force);
}
}

Expand Down Expand Up @@ -2469,9 +2469,9 @@ private Answer attachIso(DiskTO disk, boolean isAttach, String vmName) {
String isoDatastorePath = String.format("[%s] %s/%s", storeName, isoStorePathFromRoot, isoFileName);

if (isAttach) {
vmMo.attachIso(isoDatastorePath, morSecondaryDs, true, false);
vmMo.attachIso(isoDatastorePath, morSecondaryDs, true, false, force);
} else {
vmMo.detachIso(isoDatastorePath);
vmMo.detachIso(isoDatastorePath, force);
}

return new AttachAnswer(disk);
Expand All @@ -2497,7 +2497,7 @@ private Answer attachIso(DiskTO disk, boolean isAttach, String vmName) {

@Override
public Answer dettachIso(DettachCommand cmd) {
return this.attachIso(cmd.getDisk(), false, cmd.getVmName());
return this.attachIso(cmd.getDisk(), false, cmd.getVmName(), cmd.isForced());
}

@Override
Expand Down
4 changes: 2 additions & 2 deletions ...c/main/java/com/cloud/kubernetes/cluster/actionworkers/KubernetesClusterActionWorker.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,7 +319,7 @@ protected void attachIsoKubernetesVMs(List<UserVm> clusterVMs, final KubernetesS
}
for (UserVm vm : clusterVMs) {
try {
templateService.attachIso(iso.getId(), vm.getId());
templateService.attachIso(iso.getId(), vm.getId(), true);
if (LOGGER.isInfoEnabled()) {
LOGGER.info(String.format("Attached binaries ISO for VM : %s in cluster: %s", vm.getDisplayName(), kubernetesCluster.getName()));
}
Expand All @@ -337,7 +337,7 @@ protected void detachIsoKubernetesVMs(List<UserVm> clusterVMs) {
for (UserVm vm : clusterVMs) {
boolean result = false;
try {
result = templateService.detachIso(vm.getId());
result = templateService.detachIso(vm.getId(), true);
} catch (CloudRuntimeException ex) {
LOGGER.warn(String.format("Failed to detach binaries ISO from VM : %s in the Kubernetes cluster : %s ", vm.getDisplayName(), kubernetesCluster.getName()), ex);
}
Expand Down
20 changes: 11 additions & 9 deletions server/src/main/java/com/cloud/template/TemplateManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1150,7 +1150,7 @@ public boolean templateIsDeleteable(long templateId) {

@Override
@ActionEvent(eventType = EventTypes.EVENT_ISO_DETACH, eventDescription = "detaching ISO", async = true)
public boolean detachIso(long vmId) {
public boolean detachIso(long vmId, boolean forced) {
Account caller = CallContext.current().getCallingAccount();
Long userId = CallContext.current().getCallingUserId();

Expand Down Expand Up @@ -1178,7 +1178,7 @@ public boolean detachIso(long vmId) {
throw new InvalidParameterValueException("Please specify a VM that is either Stopped or Running.");
}

boolean result = attachISOToVM(vmId, userId, isoId, false); // attach=false
boolean result = attachISOToVM(vmId, userId, isoId, false, forced); // attach=false
// => detach
if (result) {
return result;
Expand All @@ -1189,7 +1189,7 @@ public boolean detachIso(long vmId) {

@Override
@ActionEvent(eventType = EventTypes.EVENT_ISO_ATTACH, eventDescription = "attaching ISO", async = true)
public boolean attachIso(long isoId, long vmId) {
public boolean attachIso(long isoId, long vmId, boolean forced) {
Account caller = CallContext.current().getCallingAccount();
Long userId = CallContext.current().getCallingUserId();

Expand Down Expand Up @@ -1231,7 +1231,7 @@ public boolean attachIso(long isoId, long vmId) {
if ("vmware-tools.iso".equals(iso.getName()) && vm.getHypervisorType() != Hypervisor.HypervisorType.VMware) {
throw new InvalidParameterValueException("Cannot attach VMware tools drivers to incompatible hypervisor " + vm.getHypervisorType());
}
boolean result = attachISOToVM(vmId, userId, isoId, true);
boolean result = attachISOToVM(vmId, userId, isoId, true, forced);
if (result) {
return result;
} else {
Expand Down Expand Up @@ -1270,7 +1270,7 @@ public TemplateInfo prepareIso(long isoId, long dcId, Long hostId, Long poolId)
}
}

private boolean attachISOToVM(long vmId, long isoId, boolean attach) {
private boolean attachISOToVM(long vmId, long isoId, boolean attach, boolean forced) {
UserVmVO vm = _userVmDao.findById(vmId);

if (vm == null) {
Expand Down Expand Up @@ -1302,19 +1302,21 @@ private boolean attachISOToVM(long vmId, long isoId, boolean attach) {

Command cmd = null;
if (attach) {
cmd = new AttachCommand(disk, vmName, vmTO.getDetails());
Copy link
Copy Markdown
Contributor

@slavkap slavkap Jun 2, 2021

Choose a reason for hiding this comment

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

Hi all,
I'm sorry to ask on already merged PR but is the getDetails() breaks the force functionality on VMware? It's used on KVM when the VM boot type is UEFI, and now it causes problems when detaching/attaching the ISO.

Copy link
Copy Markdown
Contributor Author

@Pearl1594 Pearl1594 Jun 2, 2021

Choose a reason for hiding this comment

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

@slavkap I think this got reverted while rebasing the code. Thanks for pointing it out. This would need to be fixed.

Copy link
Copy Markdown
Contributor

@slavkap slavkap Jun 2, 2021

Choose a reason for hiding this comment

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

Thanks, @Pearl1594, for the answer! I could push a fix for this, but it has to be on 4.15.2 (when it's open) or on 4.16?

cmd = new AttachCommand(disk, vmName);
((AttachCommand)cmd).setForced(forced);
} else {
cmd = new DettachCommand(disk, vmName, vmTO.getDetails());
cmd = new DettachCommand(disk, vmName);
((DettachCommand)cmd).setForced(forced);
}
Answer a = _agentMgr.easySend(vm.getHostId(), cmd);
return (a != null && a.getResult());
}

private boolean attachISOToVM(long vmId, long userId, long isoId, boolean attach) {
private boolean attachISOToVM(long vmId, long userId, long isoId, boolean attach, boolean forced) {
UserVmVO vm = _userVmDao.findById(vmId);
VMTemplateVO iso = _tmpltDao.findById(isoId);

boolean success = attachISOToVM(vmId, isoId, attach);
boolean success = attachISOToVM(vmId, isoId, attach, forced);
if (success && attach) {
vm.setIsoId(iso.getId());
_userVmDao.update(vmId, vm);
Expand Down
2 changes: 1 addition & 1 deletion 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 @@ -5350,7 +5350,7 @@ public UserVm createVirtualMachine(DeployVMCmd cmd) throws InsufficientCapacityE
for (VMTemplateVO tmpl: child_templates){
if (tmpl.getFormat() == Storage.ImageFormat.ISO){
s_logger.info("MDOV trying to attach disk to the VM " + tmpl.getId() + " vmid=" + vm.getId());
_tmplService.attachIso(tmpl.getId(), vm.getId());
_tmplService.attachIso(tmpl.getId(), vm.getId(), true);
}
}

Expand Down
Loading