DaanHoogland commented on a change in pull request #5539:
URL: https://github.com/apache/cloudstack/pull/5539#discussion_r721101443
##########
File path:
plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java
##########
@@ -1473,7 +1498,7 @@ private void plugPublicNic(VirtualMachineMO vmMo, final
String vlanId, final IpA
// restore allocation mask in case of exceptions
String nicMasksStr =
vmMo.getCustomFieldValue(CustomFieldConstants.CLOUD_NIC_MASK);
int nicMasks = Integer.parseInt(nicMasksStr);
- nicMasks &= ~(1 << nicIndex);
+ nicMasks &= ~(1 << nicIndex);
Review comment:
```suggestion
nicMasks &= ~(1 << nicIndex);
```
##########
File path:
plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java
##########
@@ -3254,70 +3279,72 @@ private static void
postNvpConfigBeforeStart(VirtualMachineMO vmMo, VirtualMachi
nicIndex++;
}
}
+ private VirtualMachineDiskInfo
getMatchingExistingDiskWithVolumeDetails(VirtualMachineDiskInfoBuilder
diskInfoBuilder, String volumePath,
+
String chainInfo, boolean isManaged, String iScsiName, String datastoreUUID,
+
VmwareHypervisorHost hyperHost, VmwareContext context) throws Exception {
+ String dsName = null;
+ String diskBackingFileBaseName = null;
- private VirtualMachineDiskInfo
getMatchingExistingDisk(VirtualMachineDiskInfoBuilder diskInfoBuilder, DiskTO
vol, VmwareHypervisorHost hyperHost, VmwareContext context)
- throws Exception {
- if (diskInfoBuilder != null) {
- VolumeObjectTO volume = (VolumeObjectTO) vol.getData();
-
- String dsName = null;
- String diskBackingFileBaseName = null;
-
- Map<String, String> details = vol.getDetails();
- boolean isManaged = details != null &&
Boolean.parseBoolean(details.get(DiskTO.MANAGED));
-
- if (isManaged) {
- String iScsiName = details.get(DiskTO.IQN);
-
- // if the storage is managed, iScsiName should not be null
- dsName = VmwareResource.getDatastoreName(iScsiName);
-
- diskBackingFileBaseName = new
DatastoreFile(volume.getPath()).getFileBaseName();
- } else {
- ManagedObjectReference morDs =
HypervisorHostHelper.findDatastoreWithBackwardsCompatibility(hyperHost,
volume.getDataStore().getUuid());
- DatastoreMO dsMo = new DatastoreMO(context, morDs);
-
- dsName = dsMo.getName();
-
- diskBackingFileBaseName = volume.getPath();
- }
-
- VirtualMachineDiskInfo diskInfo =
diskInfoBuilder.getDiskInfoByBackingFileBaseName(diskBackingFileBaseName,
dsName);
- if (diskInfo != null) {
- s_logger.info("Found existing disk info from volume path: " +
volume.getPath());
- return diskInfo;
- } else {
- String chainInfo = volume.getChainInfo();
- if (chainInfo != null) {
- VirtualMachineDiskInfo infoInChain =
_gson.fromJson(chainInfo, VirtualMachineDiskInfo.class);
- if (infoInChain != null) {
- String[] disks = infoInChain.getDiskChain();
- if (disks.length > 0) {
- for (String diskPath : disks) {
- DatastoreFile file = new
DatastoreFile(diskPath);
- diskInfo =
diskInfoBuilder.getDiskInfoByBackingFileBaseName(file.getFileBaseName(),
dsName);
- if (diskInfo != null) {
- s_logger.info("Found existing disk from
chain info: " + diskPath);
- return diskInfo;
- }
- }
- }
+ if (isManaged) {
+ // if the storage is managed, iScsiName should not be null
+ dsName = VmwareResource.getDatastoreName(iScsiName);
+ diskBackingFileBaseName = new
DatastoreFile(volumePath).getFileBaseName();
+ } else {
+ ManagedObjectReference morDs =
HypervisorHostHelper.findDatastoreWithBackwardsCompatibility(hyperHost,
datastoreUUID);
+ DatastoreMO dsMo = new DatastoreMO(context, morDs);
+ dsName = dsMo.getName();
+ diskBackingFileBaseName = volumePath;
+ }
Review comment:
can this be moved to a separate method, please?
##########
File path:
plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java
##########
@@ -892,6 +860,53 @@ private Answer execute(ResizeVolumeCommand cmd) {
_storageProcessor.expandDatastore(hostDatastoreSystem, dsMo);
}
+ boolean datastoreChangeObserved = false;
+ boolean volumePathChangeObserved = false;
+
+ if (cmd.getContextParam(DiskTO.PROTOCOL_TYPE) != null &&
cmd.getContextParam(DiskTO.PROTOCOL_TYPE).equalsIgnoreCase(Storage.StoragePoolType.DatastoreCluster.toString()))
{
+ VirtualMachineDiskInfoBuilder diskInfoBuilder =
vmMo.getDiskInfoBuilder();
+ VirtualMachineDiskInfo matchingExistingDisk =
getMatchingExistingDiskWithVolumeDetails(diskInfoBuilder, path, chainInfo,
managed, cmd.get_iScsiName(), poolUUID, hyperHost, context);
+ if (diskInfoBuilder != null && matchingExistingDisk != null) {
+ String[] diskChain = matchingExistingDisk.getDiskChain();
+ DatastoreFile file = new DatastoreFile(diskChain[0]);
+ if (!file.getFileBaseName().equalsIgnoreCase(path)) {
+ if (s_logger.isInfoEnabled())
+ s_logger.info("Detected disk-chain top file change
on volume: " + path + " -> " + file.getFileBaseName());
+ path = file.getFileBaseName();
+ volumePathChangeObserved = true;
+ chainInfo = _gson.toJson(matchingExistingDisk);
+ }
+ DatacenterMO dcMo = new
DatacenterMO(hyperHost.getContext(), hyperHost.getHyperHostDatacenter());
+ DatastoreMO diskDatastoreMofromVM = new
DatastoreMO(context, dcMo.findDatastore(file.getDatastoreName()));
+ if (diskDatastoreMofromVM != null) {
+ String actualPoolUuid =
diskDatastoreMofromVM.getCustomFieldValue(CustomFieldConstants.CLOUD_UUID);
+ if (!actualPoolUuid.equalsIgnoreCase(poolUUID)) {
+ s_logger.warn(String.format("Volume %s found to be
in a different storage pool %s", path, actualPoolUuid));
+ datastoreChangeObserved = true;
+ poolUUID = actualPoolUuid;
+ chainInfo = _gson.toJson(matchingExistingDisk);
+ }
+ }
+ }
+ }
+
+ // OfflineVmwareMigration: 5. ignore/replace the rest of the
try-block; It is the functional bit
+ Pair<VirtualDisk, String> vdisk = vmMo.getDiskDevice(path);
+
+ if (vdisk == null) {
+ if (s_logger.isTraceEnabled()) {
+ s_logger.trace("resize volume done (failed)");
+ }
+
+ throw new Exception("No such disk device: " + path);
+ }
+
+ // IDE virtual disk cannot be re-sized if VM is running
+ if (vdisk.second() != null && vdisk.second().contains("ide")) {
+ throw new Exception("Re-sizing a virtual disk over an IDE
controller is not supported in the VMware hypervisor. " +
+ "Please re-try when virtual disk is attached to a VM
using a SCSI controller.");
+ }
Review comment:
can this be moved to a separate method, please?
##########
File path:
plugins/storage/volume/default/src/main/java/org/apache/cloudstack/storage/datastore/driver/CloudStackPrimaryDataStoreDriverImpl.java
##########
@@ -418,6 +423,30 @@ public void resize(DataObject data,
AsyncCompletionCallback<CreateCmdResult> cal
vol.setSize(finalSize);
vol.update();
+
+ VolumeVO volumeVO = volumeDao.findById(vol.getId());
+ String datastoreUUID = answer.getContextParam("datastoreUUID");
+ if (datastoreUUID != null) {
+ StoragePoolVO storagePoolVO =
primaryStoreDao.findByUuid(datastoreUUID);
+ if (storagePoolVO != null) {
+ volumeVO.setPoolId(storagePoolVO.getId());
+ } else {
+ s_logger.warn(String.format("Unable to find datastore
%s while updating the new datastore of the volume %d", datastoreUUID,
vol.getId()));
+ }
+ }
+
+ String volumePath = answer.getContextParam("volumePath");
+ if (volumePath != null) {
+ volumeVO.setPath(volumePath);
+ }
+
+ String chainInfo = answer.getContextParam("chainInfo");
+ if (chainInfo != null) {
+ volumeVO.setChainInfo(chainInfo);
+ }
+
+ volumeDao.update(volumeVO.getId(), volumeVO);
Review comment:
can this be moved to a separate methods, please?
##########
File path:
plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java
##########
@@ -892,6 +860,53 @@ private Answer execute(ResizeVolumeCommand cmd) {
_storageProcessor.expandDatastore(hostDatastoreSystem, dsMo);
}
+ boolean datastoreChangeObserved = false;
+ boolean volumePathChangeObserved = false;
+
+ if (cmd.getContextParam(DiskTO.PROTOCOL_TYPE) != null &&
cmd.getContextParam(DiskTO.PROTOCOL_TYPE).equalsIgnoreCase(Storage.StoragePoolType.DatastoreCluster.toString()))
{
+ VirtualMachineDiskInfoBuilder diskInfoBuilder =
vmMo.getDiskInfoBuilder();
+ VirtualMachineDiskInfo matchingExistingDisk =
getMatchingExistingDiskWithVolumeDetails(diskInfoBuilder, path, chainInfo,
managed, cmd.get_iScsiName(), poolUUID, hyperHost, context);
+ if (diskInfoBuilder != null && matchingExistingDisk != null) {
+ String[] diskChain = matchingExistingDisk.getDiskChain();
+ DatastoreFile file = new DatastoreFile(diskChain[0]);
+ if (!file.getFileBaseName().equalsIgnoreCase(path)) {
+ if (s_logger.isInfoEnabled())
+ s_logger.info("Detected disk-chain top file change
on volume: " + path + " -> " + file.getFileBaseName());
+ path = file.getFileBaseName();
+ volumePathChangeObserved = true;
+ chainInfo = _gson.toJson(matchingExistingDisk);
+ }
+ DatacenterMO dcMo = new
DatacenterMO(hyperHost.getContext(), hyperHost.getHyperHostDatacenter());
+ DatastoreMO diskDatastoreMofromVM = new
DatastoreMO(context, dcMo.findDatastore(file.getDatastoreName()));
+ if (diskDatastoreMofromVM != null) {
+ String actualPoolUuid =
diskDatastoreMofromVM.getCustomFieldValue(CustomFieldConstants.CLOUD_UUID);
+ if (!actualPoolUuid.equalsIgnoreCase(poolUUID)) {
+ s_logger.warn(String.format("Volume %s found to be
in a different storage pool %s", path, actualPoolUuid));
+ datastoreChangeObserved = true;
+ poolUUID = actualPoolUuid;
+ chainInfo = _gson.toJson(matchingExistingDisk);
+ }
+ }
+ }
+ }
Review comment:
can this be moved to a separate method, please?
##########
File path:
plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java
##########
@@ -892,6 +860,53 @@ private Answer execute(ResizeVolumeCommand cmd) {
_storageProcessor.expandDatastore(hostDatastoreSystem, dsMo);
}
+ boolean datastoreChangeObserved = false;
+ boolean volumePathChangeObserved = false;
+
+ if (cmd.getContextParam(DiskTO.PROTOCOL_TYPE) != null &&
cmd.getContextParam(DiskTO.PROTOCOL_TYPE).equalsIgnoreCase(Storage.StoragePoolType.DatastoreCluster.toString()))
{
+ VirtualMachineDiskInfoBuilder diskInfoBuilder =
vmMo.getDiskInfoBuilder();
+ VirtualMachineDiskInfo matchingExistingDisk =
getMatchingExistingDiskWithVolumeDetails(diskInfoBuilder, path, chainInfo,
managed, cmd.get_iScsiName(), poolUUID, hyperHost, context);
+ if (diskInfoBuilder != null && matchingExistingDisk != null) {
+ String[] diskChain = matchingExistingDisk.getDiskChain();
+ DatastoreFile file = new DatastoreFile(diskChain[0]);
+ if (!file.getFileBaseName().equalsIgnoreCase(path)) {
+ if (s_logger.isInfoEnabled())
+ s_logger.info("Detected disk-chain top file change
on volume: " + path + " -> " + file.getFileBaseName());
+ path = file.getFileBaseName();
+ volumePathChangeObserved = true;
+ chainInfo = _gson.toJson(matchingExistingDisk);
+ }
+ DatacenterMO dcMo = new
DatacenterMO(hyperHost.getContext(), hyperHost.getHyperHostDatacenter());
+ DatastoreMO diskDatastoreMofromVM = new
DatastoreMO(context, dcMo.findDatastore(file.getDatastoreName()));
+ if (diskDatastoreMofromVM != null) {
+ String actualPoolUuid =
diskDatastoreMofromVM.getCustomFieldValue(CustomFieldConstants.CLOUD_UUID);
+ if (!actualPoolUuid.equalsIgnoreCase(poolUUID)) {
+ s_logger.warn(String.format("Volume %s found to be
in a different storage pool %s", path, actualPoolUuid));
+ datastoreChangeObserved = true;
+ poolUUID = actualPoolUuid;
+ chainInfo = _gson.toJson(matchingExistingDisk);
+ }
+ }
+ }
+ }
+
+ // OfflineVmwareMigration: 5. ignore/replace the rest of the
try-block; It is the functional bit
+ Pair<VirtualDisk, String> vdisk = vmMo.getDiskDevice(path);
+
+ if (vdisk == null) {
+ if (s_logger.isTraceEnabled()) {
+ s_logger.trace("resize volume done (failed)");
+ }
+
+ throw new Exception("No such disk device: " + path);
+ }
+
Review comment:
can this be moved to a separate method, please?
##########
File path:
plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java
##########
@@ -922,7 +937,17 @@ private Answer execute(ResizeVolumeCommand cmd) {
throw new Exception("Failed to configure VM to resize disk.
vmName: " + vmName);
}
- return new ResizeVolumeAnswer(cmd, true, "success", newSize *
1024);
+ ResizeVolumeAnswer answer = new ResizeVolumeAnswer(cmd, true,
"success", newSize * 1024);
+ if (datastoreChangeObserved) {
+ answer.setContextParam("datastoreUUID", poolUUID);
+ answer.setContextParam("chainInfo", chainInfo);
+ }
+
+ if (volumePathChangeObserved) {
+ answer.setContextParam("volumePath", path);
+ answer.setContextParam("chainInfo", chainInfo);
+ }
+ return answer;
Review comment:
can this be moved to a separate method, please?
##########
File path:
plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java
##########
@@ -3254,70 +3279,72 @@ private static void
postNvpConfigBeforeStart(VirtualMachineMO vmMo, VirtualMachi
nicIndex++;
}
}
+ private VirtualMachineDiskInfo
getMatchingExistingDiskWithVolumeDetails(VirtualMachineDiskInfoBuilder
diskInfoBuilder, String volumePath,
+
String chainInfo, boolean isManaged, String iScsiName, String datastoreUUID,
+
VmwareHypervisorHost hyperHost, VmwareContext context) throws Exception {
+ String dsName = null;
+ String diskBackingFileBaseName = null;
- private VirtualMachineDiskInfo
getMatchingExistingDisk(VirtualMachineDiskInfoBuilder diskInfoBuilder, DiskTO
vol, VmwareHypervisorHost hyperHost, VmwareContext context)
- throws Exception {
- if (diskInfoBuilder != null) {
- VolumeObjectTO volume = (VolumeObjectTO) vol.getData();
-
- String dsName = null;
- String diskBackingFileBaseName = null;
-
- Map<String, String> details = vol.getDetails();
- boolean isManaged = details != null &&
Boolean.parseBoolean(details.get(DiskTO.MANAGED));
-
- if (isManaged) {
- String iScsiName = details.get(DiskTO.IQN);
-
- // if the storage is managed, iScsiName should not be null
- dsName = VmwareResource.getDatastoreName(iScsiName);
-
- diskBackingFileBaseName = new
DatastoreFile(volume.getPath()).getFileBaseName();
- } else {
- ManagedObjectReference morDs =
HypervisorHostHelper.findDatastoreWithBackwardsCompatibility(hyperHost,
volume.getDataStore().getUuid());
- DatastoreMO dsMo = new DatastoreMO(context, morDs);
-
- dsName = dsMo.getName();
-
- diskBackingFileBaseName = volume.getPath();
- }
-
- VirtualMachineDiskInfo diskInfo =
diskInfoBuilder.getDiskInfoByBackingFileBaseName(diskBackingFileBaseName,
dsName);
- if (diskInfo != null) {
- s_logger.info("Found existing disk info from volume path: " +
volume.getPath());
- return diskInfo;
- } else {
- String chainInfo = volume.getChainInfo();
- if (chainInfo != null) {
- VirtualMachineDiskInfo infoInChain =
_gson.fromJson(chainInfo, VirtualMachineDiskInfo.class);
- if (infoInChain != null) {
- String[] disks = infoInChain.getDiskChain();
- if (disks.length > 0) {
- for (String diskPath : disks) {
- DatastoreFile file = new
DatastoreFile(diskPath);
- diskInfo =
diskInfoBuilder.getDiskInfoByBackingFileBaseName(file.getFileBaseName(),
dsName);
- if (diskInfo != null) {
- s_logger.info("Found existing disk from
chain info: " + diskPath);
- return diskInfo;
- }
- }
- }
+ if (isManaged) {
+ // if the storage is managed, iScsiName should not be null
+ dsName = VmwareResource.getDatastoreName(iScsiName);
+ diskBackingFileBaseName = new
DatastoreFile(volumePath).getFileBaseName();
+ } else {
+ ManagedObjectReference morDs =
HypervisorHostHelper.findDatastoreWithBackwardsCompatibility(hyperHost,
datastoreUUID);
+ DatastoreMO dsMo = new DatastoreMO(context, morDs);
+ dsName = dsMo.getName();
+ diskBackingFileBaseName = volumePath;
+ }
- if (diskInfo == null) {
- diskInfo =
diskInfoBuilder.getDiskInfoByDeviceBusName(infoInChain.getDiskDeviceBusName());
+ VirtualMachineDiskInfo diskInfo =
diskInfoBuilder.getDiskInfoByBackingFileBaseName(diskBackingFileBaseName,
dsName);
+ if (diskInfo != null) {
+ s_logger.info("Found existing disk info from volume path: " +
volumePath);
+ return diskInfo;
+ } else {
+ if (chainInfo != null) {
+ VirtualMachineDiskInfo infoInChain = _gson.fromJson(chainInfo,
VirtualMachineDiskInfo.class);
+ if (infoInChain != null) {
+ String[] disks = infoInChain.getDiskChain();
+ if (disks.length > 0) {
+ for (String diskPath : disks) {
+ DatastoreFile file = new DatastoreFile(diskPath);
+ diskInfo =
diskInfoBuilder.getDiskInfoByBackingFileBaseName(file.getFileBaseName(),
dsName);
if (diskInfo != null) {
- s_logger.info("Found existing disk from from
chain device bus information: " + infoInChain.getDiskDeviceBusName());
+ s_logger.info("Found existing disk from chain
info: " + diskPath);
Review comment:
we are five new levels deep here, I know the old code wasn't much
better, but while touching it, can this be moved to a separate methods, please?
##########
File path:
plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java
##########
@@ -892,6 +860,53 @@ private Answer execute(ResizeVolumeCommand cmd) {
_storageProcessor.expandDatastore(hostDatastoreSystem, dsMo);
}
+ boolean datastoreChangeObserved = false;
+ boolean volumePathChangeObserved = false;
+
+ if (cmd.getContextParam(DiskTO.PROTOCOL_TYPE) != null &&
cmd.getContextParam(DiskTO.PROTOCOL_TYPE).equalsIgnoreCase(Storage.StoragePoolType.DatastoreCluster.toString()))
{
+ VirtualMachineDiskInfoBuilder diskInfoBuilder =
vmMo.getDiskInfoBuilder();
+ VirtualMachineDiskInfo matchingExistingDisk =
getMatchingExistingDiskWithVolumeDetails(diskInfoBuilder, path, chainInfo,
managed, cmd.get_iScsiName(), poolUUID, hyperHost, context);
+ if (diskInfoBuilder != null && matchingExistingDisk != null) {
+ String[] diskChain = matchingExistingDisk.getDiskChain();
+ DatastoreFile file = new DatastoreFile(diskChain[0]);
+ if (!file.getFileBaseName().equalsIgnoreCase(path)) {
+ if (s_logger.isInfoEnabled())
+ s_logger.info("Detected disk-chain top file change
on volume: " + path + " -> " + file.getFileBaseName());
+ path = file.getFileBaseName();
+ volumePathChangeObserved = true;
+ chainInfo = _gson.toJson(matchingExistingDisk);
+ }
+ DatacenterMO dcMo = new
DatacenterMO(hyperHost.getContext(), hyperHost.getHyperHostDatacenter());
+ DatastoreMO diskDatastoreMofromVM = new
DatastoreMO(context, dcMo.findDatastore(file.getDatastoreName()));
+ if (diskDatastoreMofromVM != null) {
+ String actualPoolUuid =
diskDatastoreMofromVM.getCustomFieldValue(CustomFieldConstants.CLOUD_UUID);
+ if (!actualPoolUuid.equalsIgnoreCase(poolUUID)) {
+ s_logger.warn(String.format("Volume %s found to be
in a different storage pool %s", path, actualPoolUuid));
+ datastoreChangeObserved = true;
+ poolUUID = actualPoolUuid;
+ chainInfo = _gson.toJson(matchingExistingDisk);
+ }
+ }
+ }
+ }
Review comment:
sorry methods (plural)
--
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]
