Skip to content

Commit 2ed7868

Browse files
RodrigoDLopezLopezstephankruggg
authored
Inserts timer in check detach volume (#6508)
Co-authored-by: Lopez <rodrigo@scclouds.com.br> Co-authored-by: Stephan Krug <stekrug@icloud.com>
1 parent 162af93 commit 2ed7868

File tree

4 files changed

+275
-42
lines changed

4 files changed

+275
-42
lines changed

‎core/src/main/java/org/apache/cloudstack/storage/command/DettachCommand.java‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ public class DettachCommand extends StorageSubSystemCommand {
3232
private int _storagePort;
3333
private Map<String, String> params;
3434
private boolean forced;
35+
private long waitDetachDevice;
3536

3637
public DettachCommand(final DiskTO disk, final String vmName) {
3738
super();
@@ -115,6 +116,14 @@ public void setForced(boolean forced) {
115116
this.forced = forced;
116117
}
117118

119+
public void setWaitDetachDevice(long wait) {
120+
this.waitDetachDevice = wait;
121+
}
122+
123+
public long getWaitDetachDevice(){
124+
return waitDetachDevice;
125+
}
126+
118127
@Override
119128
public void setExecuteInSequence(final boolean inSeq) {
120129

‎plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java‎

Lines changed: 156 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,6 @@
2525
import java.io.FileNotFoundException;
2626
import java.io.FileOutputStream;
2727
import java.io.IOException;
28-
import java.net.URISyntaxException;
2928
import java.text.DateFormat;
3029
import java.text.SimpleDateFormat;
3130
import java.util.Arrays;
@@ -155,6 +154,10 @@ public class KVMStorageProcessor implements StorageProcessor {
155154
private static final String CEPH_AUTH_KEY = "key";
156155
private static final String CEPH_CLIENT_MOUNT_TIMEOUT = "client_mount_timeout";
157156
private static final String CEPH_DEFAULT_MOUNT_TIMEOUT = "30";
157+
/**
158+
* Time interval before rechecking virsh commands
159+
*/
160+
private long waitDelayForVirshCommands = 1000l;
158161

159162
public KVMStorageProcessor(final KVMStoragePoolManager storagePoolMgr, final LibvirtComputingResource resource) {
160163
this.storagePoolMgr = storagePoolMgr;
@@ -1068,10 +1071,9 @@ public Answer backupSnapshot(final CopyCommand cmd) {
10681071
}
10691072
}
10701073
}
1071-
1072-
protected synchronized String attachOrDetachISO(final Connect conn, final String vmName, String isoPath, final boolean isAttach, Map<String, String> params) throws LibvirtException, URISyntaxException,
1073-
InternalErrorException {
1074-
String isoXml = null;
1074+
protected synchronized void attachOrDetachISO(final Connect conn, final String vmName, String isoPath, final boolean isAttach, Map<String, String> params) throws
1075+
LibvirtException, InternalErrorException {
1076+
DiskDef iso = new DiskDef();
10751077
boolean isUefiEnabled = MapUtils.isNotEmpty(params) && params.containsKey("UEFI");
10761078
if (isoPath != null && isAttach) {
10771079
final int index = isoPath.lastIndexOf("/");
@@ -1081,26 +1083,21 @@ protected synchronized String attachOrDetachISO(final Connect conn, final String
10811083
final KVMPhysicalDisk isoVol = secondaryPool.getPhysicalDisk(name);
10821084
isoPath = isoVol.getPath();
10831085

1084-
final DiskDef iso = new DiskDef();
10851086
iso.defISODisk(isoPath, isUefiEnabled);
1086-
isoXml = iso.toString();
10871087
} else {
1088-
final DiskDef iso = new DiskDef();
10891088
iso.defISODisk(null, isUefiEnabled);
1090-
isoXml = iso.toString();
10911089
}
10921090

10931091
final List<DiskDef> disks = resource.getDisks(conn, vmName);
1094-
final String result = attachOrDetachDevice(conn, true, vmName, isoXml);
1095-
if (result == null && !isAttach) {
1092+
attachOrDetachDevice(conn, true, vmName, iso);
1093+
if (!isAttach) {
10961094
for (final DiskDef disk : disks) {
10971095
if (disk.getDeviceType() == DiskDef.DeviceType.CDROM) {
10981096
resource.cleanupDisk(disk);
10991097
}
11001098
}
11011099

11021100
}
1103-
return result;
11041101
}
11051102

11061103
@Override
@@ -1115,8 +1112,6 @@ public Answer attachIso(final AttachCommand cmd) {
11151112
attachOrDetachISO(conn, cmd.getVmName(), dataStoreUrl + File.separator + isoTO.getPath(), true, cmd.getControllerInfo());
11161113
} catch (final LibvirtException e) {
11171114
return new Answer(cmd, false, e.toString());
1118-
} catch (final URISyntaxException e) {
1119-
return new Answer(cmd, false, e.toString());
11201115
} catch (final InternalErrorException e) {
11211116
return new Answer(cmd, false, e.toString());
11221117
} catch (final InvalidParameterValueException e) {
@@ -1138,8 +1133,6 @@ public Answer dettachIso(final DettachCommand cmd) {
11381133
attachOrDetachISO(conn, cmd.getVmName(), dataStoreUrl + File.separator + isoTO.getPath(), false, cmd.getParams());
11391134
} catch (final LibvirtException e) {
11401135
return new Answer(cmd, false, e.toString());
1141-
} catch (final URISyntaxException e) {
1142-
return new Answer(cmd, false, e.toString());
11431136
} catch (final InternalErrorException e) {
11441137
return new Answer(cmd, false, e.toString());
11451138
} catch (final InvalidParameterValueException e) {
@@ -1169,27 +1162,47 @@ private String getDataStoreUrlFromStore(DataStoreTO store) {
11691162
}
11701163
return store.getUrl();
11711164
}
1165+
protected synchronized void attachOrDetachDevice(final Connect conn, final boolean attach, final String vmName, final DiskDef xml)
1166+
throws LibvirtException, InternalErrorException {
1167+
attachOrDetachDevice(conn, attach, vmName, xml, 0l);
1168+
}
11721169

1173-
protected synchronized String attachOrDetachDevice(final Connect conn, final boolean attach, final String vmName, final String xml) throws LibvirtException, InternalErrorException {
1170+
/**
1171+
* Attaches or detaches a device (ISO or disk) to an instance.
1172+
* @param conn libvirt connection
1173+
* @param attach boolean that determines whether the device will be attached or detached
1174+
* @param vmName instance name
1175+
* @param diskDef disk definition or iso to be attached or detached
1176+
* @param waitDetachDevice value set in milliseconds to wait before assuming device removal failed
1177+
* @throws LibvirtException
1178+
* @throws InternalErrorException
1179+
*/
1180+
protected synchronized void attachOrDetachDevice(final Connect conn, final boolean attach, final String vmName, final DiskDef diskDef, long waitDetachDevice)
1181+
throws LibvirtException, InternalErrorException {
11741182
Domain dm = null;
1183+
String diskXml = diskDef.toString();
1184+
String diskPath = diskDef.getDiskPath();
11751185
try {
11761186
dm = conn.domainLookupByName(vmName);
11771187

11781188
if (attach) {
1179-
s_logger.debug("Attaching device: " + xml);
1180-
dm.attachDevice(xml);
1181-
} else {
1182-
s_logger.debug("Detaching device: " + xml);
1183-
dm.detachDevice(xml);
1184-
LibvirtDomainXMLParser parser = new LibvirtDomainXMLParser();
1185-
parser.parseDomainXML(dm.getXMLDesc(0));
1186-
List<DiskDef> disks = parser.getDisks();
1187-
for (DiskDef diskDef : disks) {
1188-
if (StringUtils.contains(xml, diskDef.getDiskPath())) {
1189-
throw new InternalErrorException("Could not detach volume. Probably the VM is in boot state at the moment");
1190-
}
1191-
}
1192-
}
1189+
s_logger.debug("Attaching device: " + diskXml);
1190+
dm.attachDevice(diskXml);
1191+
return;
1192+
}
1193+
s_logger.debug(String.format("Detaching device: [%s].", diskXml));
1194+
dm.detachDevice(diskXml);
1195+
long wait = waitDetachDevice;
1196+
while (!checkDetachSuccess(diskPath, dm) && wait > 0) {
1197+
wait = getWaitAfterSleep(dm, diskPath, wait);
1198+
}
1199+
if (wait <= 0) {
1200+
throw new InternalErrorException(String.format("Could not detach volume after sending the command and waiting for [%s] milliseconds. Probably the VM does " +
1201+
"not support the sent detach command or the device is busy at the moment. Try again in a couple of minutes.",
1202+
waitDetachDevice));
1203+
}
1204+
s_logger.debug(String.format("The detach command was executed successfully. The device [%s] was removed from the VM instance with UUID [%s].",
1205+
diskPath, dm.getUUIDString()));
11931206
} catch (final LibvirtException e) {
11941207
if (attach) {
11951208
s_logger.warn("Failed to attach device to " + vmName + ": " + e.getMessage());
@@ -1206,15 +1219,115 @@ protected synchronized String attachOrDetachDevice(final Connect conn, final boo
12061219
}
12071220
}
12081221
}
1222+
}
12091223

1210-
return null;
1224+
/**
1225+
* Waits {@link #waitDelayForVirshCommands} milliseconds before checking again if the device has been removed.
1226+
* @return The configured value in wait.detach.device reduced by {@link #waitDelayForVirshCommands}
1227+
* @throws LibvirtException
1228+
*/
1229+
private long getWaitAfterSleep(Domain dm, String diskPath, long wait) throws LibvirtException {
1230+
try {
1231+
wait -= waitDelayForVirshCommands;
1232+
Thread.sleep(waitDelayForVirshCommands);
1233+
s_logger.trace(String.format("Trying to detach device [%s] from VM instance with UUID [%s]. " +
1234+
"Waiting [%s] milliseconds before assuming the VM was unable to detach the volume.", diskPath, dm.getUUIDString(), wait));
1235+
} catch (InterruptedException e) {
1236+
throw new CloudRuntimeException(e);
1237+
}
1238+
return wait;
1239+
}
1240+
1241+
/**
1242+
* Checks if the device has been removed from the instance
1243+
* @param diskPath Path to the device that was removed
1244+
* @param dm instance to be checked if the device was properly removed
1245+
* @throws LibvirtException
1246+
*/
1247+
protected boolean checkDetachSuccess(String diskPath, Domain dm) throws LibvirtException {
1248+
LibvirtDomainXMLParser parser = new LibvirtDomainXMLParser();
1249+
parser.parseDomainXML(dm.getXMLDesc(0));
1250+
List<DiskDef> disks = parser.getDisks();
1251+
for (DiskDef diskDef : disks) {
1252+
if (StringUtils.equals(diskPath, diskDef.getDiskPath())) {
1253+
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].",
1254+
diskPath, dm.getUUIDString()));
1255+
return false;
1256+
}
1257+
}
1258+
return true;
12111259
}
12121260

1213-
protected synchronized String attachOrDetachDisk(final Connect conn, final boolean attach, final String vmName, final KVMPhysicalDisk attachingDisk, final int devId, final String serial,
1214-
final Long bytesReadRate, final Long bytesReadRateMax, final Long bytesReadRateMaxLength,
1215-
final Long bytesWriteRate, final Long bytesWriteRateMax, final Long bytesWriteRateMaxLength,
1216-
final Long iopsReadRate, final Long iopsReadRateMax, final Long iopsReadRateMaxLength,
1217-
final Long iopsWriteRate, final Long iopsWriteRateMax, final Long iopsWriteRateMaxLength, final String cacheMode, final DiskDef.LibvirtDiskEncryptDetails encryptDetails) throws LibvirtException, InternalErrorException {
1261+
/**
1262+
* Attaches or detaches a disk to an instance.
1263+
* @param conn libvirt connection
1264+
* @param attach boolean that determines whether the device will be attached or detached
1265+
* @param vmName instance name
1266+
* @param attachingDisk kvm physical disk
1267+
* @param devId device id in instance
1268+
* @param serial
1269+
* @param bytesReadRate bytes read rate
1270+
* @param bytesReadRateMax bytes read rate max
1271+
* @param bytesReadRateMaxLength bytes read rate max length
1272+
* @param bytesWriteRate bytes write rate
1273+
* @param bytesWriteRateMax bytes write rate amx
1274+
* @param bytesWriteRateMaxLength bytes write rate max length
1275+
* @param iopsReadRate iops read rate
1276+
* @param iopsReadRateMax iops read rate max
1277+
* @param iopsReadRateMaxLength iops read rate max length
1278+
* @param iopsWriteRate iops write rate
1279+
* @param iopsWriteRateMax iops write rate max
1280+
* @param iopsWriteRateMaxLength iops write rate max length
1281+
* @param cacheMode cache mode
1282+
* @param encryptDetails encrypt details
1283+
* @throws LibvirtException
1284+
* @throws InternalErrorException
1285+
*/
1286+
protected synchronized void attachOrDetachDisk(final Connect conn, final boolean attach, final String vmName, final KVMPhysicalDisk attachingDisk, final int devId,
1287+
final String serial, final Long bytesReadRate, final Long bytesReadRateMax, final Long bytesReadRateMaxLength,
1288+
final Long bytesWriteRate, final Long bytesWriteRateMax, final Long bytesWriteRateMaxLength, final Long iopsReadRate,
1289+
final Long iopsReadRateMax, final Long iopsReadRateMaxLength, final Long iopsWriteRate, final Long iopsWriteRateMax,
1290+
final Long iopsWriteRateMaxLength, final String cacheMode, final DiskDef.LibvirtDiskEncryptDetails encryptDetails)
1291+
throws LibvirtException, InternalErrorException {
1292+
attachOrDetachDisk(conn, attach, vmName, attachingDisk, devId, serial, bytesReadRate, bytesReadRateMax, bytesReadRateMaxLength,
1293+
bytesWriteRate, bytesWriteRateMax, bytesWriteRateMaxLength, iopsReadRate, iopsReadRateMax, iopsReadRateMaxLength, iopsWriteRate,
1294+
iopsWriteRateMax, iopsWriteRateMaxLength, cacheMode, encryptDetails, 0l);
1295+
}
1296+
1297+
/**
1298+
*
1299+
* Attaches or detaches a disk to an instance.
1300+
* @param conn libvirt connection
1301+
* @param attach boolean that determines whether the device will be attached or detached
1302+
* @param vmName instance name
1303+
* @param attachingDisk kvm physical disk
1304+
* @param devId device id in instance
1305+
* @param serial
1306+
* @param bytesReadRate bytes read rate
1307+
* @param bytesReadRateMax bytes read rate max
1308+
* @param bytesReadRateMaxLength bytes read rate max length
1309+
* @param bytesWriteRate bytes write rate
1310+
* @param bytesWriteRateMax bytes write rate amx
1311+
* @param bytesWriteRateMaxLength bytes write rate max length
1312+
* @param iopsReadRate iops read rate
1313+
* @param iopsReadRateMax iops read rate max
1314+
* @param iopsReadRateMaxLength iops read rate max length
1315+
* @param iopsWriteRate iops write rate
1316+
* @param iopsWriteRateMax iops write rate max
1317+
* @param iopsWriteRateMaxLength iops write rate max length
1318+
* @param cacheMode cache mode
1319+
* @param encryptDetails encrypt details
1320+
* @param waitDetachDevice value set in milliseconds to wait before assuming device removal failed
1321+
* @throws LibvirtException
1322+
* @throws InternalErrorException
1323+
*/
1324+
protected synchronized void attachOrDetachDisk(final Connect conn, final boolean attach, final String vmName, final KVMPhysicalDisk attachingDisk, final int devId,
1325+
final String serial, final Long bytesReadRate, final Long bytesReadRateMax, final Long bytesReadRateMaxLength,
1326+
final Long bytesWriteRate, final Long bytesWriteRateMax, final Long bytesWriteRateMaxLength, final Long iopsReadRate,
1327+
final Long iopsReadRateMax, final Long iopsReadRateMaxLength, final Long iopsWriteRate, final Long iopsWriteRateMax,
1328+
final Long iopsWriteRateMaxLength, final String cacheMode, final DiskDef.LibvirtDiskEncryptDetails encryptDetails,
1329+
long waitDetachDevice)
1330+
throws LibvirtException, InternalErrorException {
12181331
List<DiskDef> disks = null;
12191332
Domain dm = null;
12201333
DiskDef diskdef = null;
@@ -1244,7 +1357,9 @@ protected synchronized String attachOrDetachDisk(final Connect conn, final boole
12441357
}
12451358
}
12461359
if (diskdef == null) {
1247-
throw new InternalErrorException("disk: " + attachingDisk.getPath() + " is not attached before");
1360+
s_logger.warn(String.format("Could not find disk [%s] attached to VM instance with UUID [%s]. We will set it as detached in the database to ensure consistency.",
1361+
attachingDisk.getPath(), dm.getUUIDString()));
1362+
return;
12481363
}
12491364
} else {
12501365
DiskDef.DiskBus busT = DiskDef.DiskBus.VIRTIO;
@@ -1338,8 +1453,7 @@ protected synchronized String attachOrDetachDisk(final Connect conn, final boole
13381453
}
13391454
}
13401455

1341-
final String xml = diskdef.toString();
1342-
return attachOrDetachDevice(conn, attach, vmName, xml);
1456+
attachOrDetachDevice(conn, attach, vmName, diskdef, waitDetachDevice);
13431457
} finally {
13441458
if (dm != null) {
13451459
dm.free();
@@ -1399,6 +1513,7 @@ public Answer dettachVolume(final DettachCommand cmd) {
13991513
final PrimaryDataStoreTO primaryStore = (PrimaryDataStoreTO)vol.getDataStore();
14001514
final String vmName = cmd.getVmName();
14011515
final String serial = resource.diskUuidToSerial(vol.getUuid());
1516+
long waitDetachDevice = cmd.getWaitDetachDevice();
14021517
try {
14031518
final Connect conn = LibvirtConnection.getConnectionByVmName(vmName);
14041519

@@ -1409,7 +1524,7 @@ public Answer dettachVolume(final DettachCommand cmd) {
14091524
vol.getBytesReadRate(), vol.getBytesReadRateMax(), vol.getBytesReadRateMaxLength(),
14101525
vol.getBytesWriteRate(), vol.getBytesWriteRateMax(), vol.getBytesWriteRateMaxLength(),
14111526
vol.getIopsReadRate(), vol.getIopsReadRateMax(), vol.getIopsReadRateMaxLength(),
1412-
vol.getIopsWriteRate(), vol.getIopsWriteRateMax(), vol.getIopsWriteRateMaxLength(), volCacheMode, null);
1527+
vol.getIopsWriteRate(), vol.getIopsWriteRateMax(), vol.getIopsWriteRateMaxLength(), volCacheMode, null, waitDetachDevice);
14131528

14141529
storagePoolMgr.disconnectPhysicalDisk(primaryStore.getPoolType(), primaryStore.getUuid(), vol.getPath());
14151530

0 commit comments

Comments
 (0)