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 2022/10/11 08:31:47 UTC
[GitHub] [cloudstack] DaanHoogland commented on a diff in pull request #6508: Inserts timer in check detach volume
DaanHoogland commented on code in PR #6508:
URL: https://github.com/apache/cloudstack/pull/6508#discussion_r992000677
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java:
##########
@@ -1207,14 +1222,38 @@ protected synchronized String attachOrDetachDevice(final Connect conn, final boo
}
}
- return null;
+ return;
Review Comment:
```suggestion
```
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java:
##########
@@ -1068,10 +1072,9 @@ public Answer backupSnapshot(final CopyCommand cmd) {
}
}
}
-
- protected synchronized String attachOrDetachISO(final Connect conn, final String vmName, String isoPath, final boolean isAttach, Map<String, String> params) throws LibvirtException, URISyntaxException,
+ protected synchronized void attachOrDetachISO(final Connect conn, final String vmName, String isoPath, final boolean isAttach, Map<String, String> params) throws LibvirtException, URISyntaxException,
Review Comment:
sonarcode sugesstion(s):
```suggestion
protected synchronized void attachOrDetachISO(final Connect conn, final String vmName, String isoPath, final boolean isAttach, Map<String, String> params) throws LibvirtException,
```
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java:
##########
@@ -1207,14 +1222,38 @@ protected synchronized String attachOrDetachDevice(final Connect conn, final boo
}
}
- return null;
+ return;
}
-
- protected synchronized String attachOrDetachDisk(final Connect conn, final boolean attach, final String vmName, final KVMPhysicalDisk attachingDisk, final int devId, final String serial,
- final Long bytesReadRate, final Long bytesReadRateMax, final Long bytesReadRateMaxLength,
- final Long bytesWriteRate, final Long bytesWriteRateMax, final Long bytesWriteRateMaxLength,
- final Long iopsReadRate, final Long iopsReadRateMax, final Long iopsReadRateMaxLength,
- final Long iopsWriteRate, final Long iopsWriteRateMax, final Long iopsWriteRateMaxLength, final String cacheMode, final DiskDef.LibvirtDiskEncryptDetails encryptDetails) throws LibvirtException, InternalErrorException {
+ protected boolean checkDetachSucess(String diskPath, Domain dm) throws LibvirtException {
+ LibvirtDomainXMLParser parser = new LibvirtDomainXMLParser();
+ parser.parseDomainXML(dm.getXMLDesc(0));
+ List<DiskDef> disks = parser.getDisks();
+ for (DiskDef diskDef : disks) {
+ if (StringUtils.equals(diskPath, diskDef.getDiskPath())) {
+ s_logger.debug(String.format("The hypervisor sent the detach command, but it is still possible to identify the device [%s] in the instance with UUID [%s].",
+ diskPath, dm.getUUIDString()));
+ return false;
+ }
+ }
+ return true;
+ }
+ protected synchronized void attachOrDetachDisk(final Connect conn, final boolean attach, final String vmName, final KVMPhysicalDisk attachingDisk, final int devId,
Review Comment:
```suggestion
/**
*
*/
protected synchronized void attachOrDetachDisk(final Connect conn, final boolean attach, final String vmName, final KVMPhysicalDisk attachingDisk, final int devId,
```
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java:
##########
@@ -1169,27 +1168,43 @@ private String getDataStoreUrlFromStore(DataStoreTO store) {
}
return store.getUrl();
}
-
- protected synchronized String attachOrDetachDevice(final Connect conn, final boolean attach, final String vmName, final String xml) throws LibvirtException, InternalErrorException {
+ protected synchronized void attachOrDetachDevice(final Connect conn, final boolean attach, final String vmName, final DiskDef xml)
+ throws LibvirtException, InternalErrorException {
+ attachOrDetachDevice(conn, attach, vmName, xml, 0l);
+ }
+ protected synchronized void attachOrDetachDevice(final Connect conn, final boolean attach, final String vmName, final DiskDef diskDef, long waitDetachDevice)
Review Comment:
```suggestion
/**
*
*/
protected synchronized void attachOrDetachDevice(final Connect conn, final boolean attach, final String vmName, final DiskDef diskDef, long waitDetachDevice)
```
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java:
##########
@@ -1081,26 +1084,22 @@ protected synchronized String attachOrDetachISO(final Connect conn, final String
final KVMPhysicalDisk isoVol = secondaryPool.getPhysicalDisk(name);
isoPath = isoVol.getPath();
- final DiskDef iso = new DiskDef();
iso.defISODisk(isoPath, isUefiEnabled);
- isoXml = iso.toString();
} else {
- final DiskDef iso = new DiskDef();
iso.defISODisk(null, isUefiEnabled);
- isoXml = iso.toString();
}
final List<DiskDef> disks = resource.getDisks(conn, vmName);
- final String result = attachOrDetachDevice(conn, true, vmName, isoXml);
- if (result == null && !isAttach) {
+ attachOrDetachDevice(conn, true, vmName, iso);
+ if (!isAttach) {
for (final DiskDef disk : disks) {
if (disk.getDeviceType() == DiskDef.DeviceType.CDROM) {
resource.cleanupDisk(disk);
}
}
}
- return result;
+ return;
Review Comment:
```suggestion
```
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java:
##########
@@ -1207,14 +1222,38 @@ protected synchronized String attachOrDetachDevice(final Connect conn, final boo
}
}
- return null;
+ return;
}
-
- protected synchronized String attachOrDetachDisk(final Connect conn, final boolean attach, final String vmName, final KVMPhysicalDisk attachingDisk, final int devId, final String serial,
- final Long bytesReadRate, final Long bytesReadRateMax, final Long bytesReadRateMaxLength,
- final Long bytesWriteRate, final Long bytesWriteRateMax, final Long bytesWriteRateMaxLength,
- final Long iopsReadRate, final Long iopsReadRateMax, final Long iopsReadRateMaxLength,
- final Long iopsWriteRate, final Long iopsWriteRateMax, final Long iopsWriteRateMaxLength, final String cacheMode, final DiskDef.LibvirtDiskEncryptDetails encryptDetails) throws LibvirtException, InternalErrorException {
+ protected boolean checkDetachSucess(String diskPath, Domain dm) throws LibvirtException {
+ LibvirtDomainXMLParser parser = new LibvirtDomainXMLParser();
+ parser.parseDomainXML(dm.getXMLDesc(0));
+ List<DiskDef> disks = parser.getDisks();
+ for (DiskDef diskDef : disks) {
+ if (StringUtils.equals(diskPath, diskDef.getDiskPath())) {
+ s_logger.debug(String.format("The hypervisor sent the detach command, but it is still possible to identify the device [%s] in the instance with UUID [%s].",
+ diskPath, dm.getUUIDString()));
+ return false;
+ }
+ }
+ return true;
+ }
+ protected synchronized void attachOrDetachDisk(final Connect conn, final boolean attach, final String vmName, final KVMPhysicalDisk attachingDisk, final int devId,
+ final String serial, final Long bytesReadRate, final Long bytesReadRateMax, final Long bytesReadRateMaxLength,
+ final Long bytesWriteRate, final Long bytesWriteRateMax, final Long bytesWriteRateMaxLength, final Long iopsReadRate,
+ final Long iopsReadRateMax, final Long iopsReadRateMaxLength, final Long iopsWriteRate, final Long iopsWriteRateMax,
+ final Long iopsWriteRateMaxLength, final String cacheMode, final DiskDef.LibvirtDiskEncryptDetails encryptDetails)
+ throws LibvirtException, InternalErrorException {
+ attachOrDetachDisk(conn, attach, vmName, attachingDisk, devId, serial, bytesReadRate, bytesReadRateMax, bytesReadRateMaxLength,
+ bytesWriteRate, bytesWriteRateMax, bytesWriteRateMaxLength, iopsReadRate, iopsReadRateMax, iopsReadRateMaxLength, iopsWriteRate,
+ iopsWriteRateMax, iopsWriteRateMaxLength, cacheMode, encryptDetails, 0l);
+ }
+ protected synchronized void attachOrDetachDisk(final Connect conn, final boolean attach, final String vmName, final KVMPhysicalDisk attachingDisk, final int devId,
+ final String serial, final Long bytesReadRate, final Long bytesReadRateMax, final Long bytesReadRateMaxLength,
+ final Long bytesWriteRate, final Long bytesWriteRateMax, final Long bytesWriteRateMaxLength, final Long iopsReadRate,
+ final Long iopsReadRateMax, final Long iopsReadRateMaxLength, final Long iopsWriteRate, final Long iopsWriteRateMax,
+ final Long iopsWriteRateMaxLength, final String cacheMode, final DiskDef.LibvirtDiskEncryptDetails encryptDetails,
+ long waitDetachDevice)
Review Comment:
```suggestion
/**
*
*/
protected synchronized void attachOrDetachDisk(final Connect conn, final boolean attach, final String vmName, final KVMPhysicalDisk attachingDisk, final int devId,
final String serial, final Long bytesReadRate, final Long bytesReadRateMax, final Long bytesReadRateMaxLength,
final Long bytesWriteRate, final Long bytesWriteRateMax, final Long bytesWriteRateMaxLength, final Long iopsReadRate,
final Long iopsReadRateMax, final Long iopsReadRateMaxLength, final Long iopsWriteRate, final Long iopsWriteRateMax,
final Long iopsWriteRateMaxLength, final String cacheMode, final DiskDef.LibvirtDiskEncryptDetails encryptDetails)
```
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java:
##########
@@ -1169,27 +1168,43 @@ private String getDataStoreUrlFromStore(DataStoreTO store) {
}
return store.getUrl();
}
-
- protected synchronized String attachOrDetachDevice(final Connect conn, final boolean attach, final String vmName, final String xml) throws LibvirtException, InternalErrorException {
+ protected synchronized void attachOrDetachDevice(final Connect conn, final boolean attach, final String vmName, final DiskDef xml)
+ throws LibvirtException, InternalErrorException {
+ attachOrDetachDevice(conn, attach, vmName, xml, 0l);
+ }
+ protected synchronized void attachOrDetachDevice(final Connect conn, final boolean attach, final String vmName, final DiskDef diskDef, long waitDetachDevice)
+ throws LibvirtException, InternalErrorException {
Domain dm = null;
+ String diskXml = diskDef.toString();
+ String diskPath = diskDef.getDiskPath();
try {
dm = conn.domainLookupByName(vmName);
if (attach) {
- s_logger.debug("Attaching device: " + xml);
- dm.attachDevice(xml);
- } else {
- s_logger.debug("Detaching device: " + xml);
- dm.detachDevice(xml);
- LibvirtDomainXMLParser parser = new LibvirtDomainXMLParser();
- parser.parseDomainXML(dm.getXMLDesc(0));
- List<DiskDef> disks = parser.getDisks();
- for (DiskDef diskDef : disks) {
- if (StringUtils.contains(xml, diskDef.getDiskPath())) {
- throw new InternalErrorException("Could not detach volume. Probably the VM is in boot state at the moment");
- }
+ s_logger.debug("Attaching device: " + diskXml);
+ dm.attachDevice(diskXml);
+ return;
+ }
+ s_logger.debug(String.format("Detaching device: [%s].", diskXml));
+ dm.detachDevice(diskXml);
+ long wait = waitDetachDevice;
+ while (!checkDetachSucess(diskPath, dm) && wait > 0) {
+ try {
+ wait -= waitDelayForVirshCommands;
+ Thread.sleep(waitDelayForVirshCommands);
+ s_logger.trace(String.format("Trying to detach device [%s] from VM instance with UUID [%s]. " +
+ "Waiting [%s] milliseconds before assuming the VM was unable to detach the volume.", diskPath, dm.getUUIDString(), wait));
+ } catch (InterruptedException e) {
+ throw new RuntimeException(e);
}
Review Comment:
extract this block into a method. Also a sonar suggestion but this one I very much agree on. It makes code both more readable and more debug-able. The exception can be cloudstack specific.
```suggestion
try {
wait -= waitDelayForVirshCommands;
Thread.sleep(waitDelayForVirshCommands);
s_logger.trace(String.format("Trying to detach device [%s] from VM instance with UUID [%s]. " +
"Waiting [%s] milliseconds before assuming the VM was unable to detach the volume.", diskPath, dm.getUUIDString(), wait));
} catch (InterruptedException e) {
throw new CloudRuntimeException(e);
}
```
##########
server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java:
##########
@@ -336,6 +336,14 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic
public static final ConfigKey<Boolean> MatchStoragePoolTagsWithDiskOffering = new ConfigKey<Boolean>("Advanced", Boolean.class, "match.storage.pool.tags.with.disk.offering", "true",
"If true, volume's disk offering can be changed only with the matched storage tags", true, ConfigKey.Scope.Zone);
+ public static ConfigKey<Long> WaitDetachDevice = new ConfigKey<>(
Review Comment:
```suggestion
public static final ConfigKey<Long> WaitDetachDevice = new ConfigKey<>(
```
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java:
##########
@@ -1207,14 +1222,38 @@ protected synchronized String attachOrDetachDevice(final Connect conn, final boo
}
}
- return null;
+ return;
}
-
- protected synchronized String attachOrDetachDisk(final Connect conn, final boolean attach, final String vmName, final KVMPhysicalDisk attachingDisk, final int devId, final String serial,
- final Long bytesReadRate, final Long bytesReadRateMax, final Long bytesReadRateMaxLength,
- final Long bytesWriteRate, final Long bytesWriteRateMax, final Long bytesWriteRateMaxLength,
- final Long iopsReadRate, final Long iopsReadRateMax, final Long iopsReadRateMaxLength,
- final Long iopsWriteRate, final Long iopsWriteRateMax, final Long iopsWriteRateMaxLength, final String cacheMode, final DiskDef.LibvirtDiskEncryptDetails encryptDetails) throws LibvirtException, InternalErrorException {
+ protected boolean checkDetachSucess(String diskPath, Domain dm) throws LibvirtException {
Review Comment:
```suggestion
/**
*
*/
protected boolean checkDetachSucess(String diskPath, Domain dm) throws LibvirtException {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org