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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
