Copilot commented on code in PR #13578:
URL: https://github.com/apache/cloudstack/pull/13578#discussion_r3553132024
##########
server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java:
##########
@@ -5092,6 +5092,11 @@ private VolumeVO sendAttachVolumeCommand(UserVmVO vm,
VolumeVO volumeToAttach, L
throw new CloudRuntimeException(e.getMessage());
}
+
+ // Reload volume from DB after grantAccess — managed storage
drivers (e.g. ONTAP)
+ // may update the volume's path and iScsiName during
grantAccess, so the local
+ // volumeToAttach object can be stale.
+ volumeToAttach = _volsDao.findById(volumeToAttach.getId());
Review Comment:
After grantAccess(), the code reloads the volume from the DB but doesn’t
handle the (unlikely but possible) case where _volsDao.findById(...) returns
null. That would cause a NullPointerException later when volumeToAttach is
dereferenced (e.g., getPath/get_iScsiName).
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageUtils.java:
##########
@@ -139,10 +139,15 @@ public static StorageStrategy
getStrategyByStoragePoolDetails(Map<String, String
}
}
- public static String getIgroupName(String svmName, String hostName) {
- //Igroup name format: cs_svmName_hostName
- String sanitizedHostName =
hostName.split("\\.")[0].replaceAll("[^a-zA-Z0-9_-]", "_");
- return OntapStorageConstants.CS + OntapStorageConstants.UNDERSCORE +
svmName + OntapStorageConstants.UNDERSCORE + sanitizedHostName;
+ public static String getIgroupName(String svmName, String hostUuid) {
+ //Igroup name format: cs_svmName_hostUuid
+ String sanitizedHostUuid = hostUuid.replaceAll("[^a-zA-Z0-9_-]", "_");
+ String igroupName = OntapStorageConstants.CS +
OntapStorageConstants.UNDERSCORE + svmName + OntapStorageConstants.UNDERSCORE +
sanitizedHostUuid;
+ // ONTAP igroup names are limited to 96 characters; truncate if longer.
+ if (igroupName.length() >
OntapStorageConstants.IGROUP_NAME_MAX_LENGTH) {
+ igroupName = igroupName.substring(0,
OntapStorageConstants.IGROUP_NAME_MAX_LENGTH);
+ }
+ return igroupName;
}
Review Comment:
getIgroupName() truncates the full "cs_<svm>_<hostUuid>" string as a whole.
If svmName is long, this can truncate away most or all of the hostUuid portion,
increasing the chance of igroup name collisions across different hosts (and
also throws a NullPointerException if hostUuid is null). Consider validating
inputs and truncating svmName first so the host UUID remains intact (or, if
hostUuid itself is too long, truncate only the UUID).
--
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]
