You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by "suryag1201 (via GitHub)" <gi...@apache.org> on 2026/07/10 11:32:15 UTC
[PR] Bugfix/cstackex 216 Public PR Review Comment: Igroup Name Length truncation [cloudstack]
suryag1201 opened a new pull request, #13585:
URL: https://github.com/apache/cloudstack/pull/13585
### Description
Issue: "getIgroupName() truncates the full "cs__" 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)."
Fix: In typical deployments, truncation likely never triggers bcz total length will remain less than 96. But to avoid any issues for future where svm limits get exceeded, will change the igroup format to cs_hostUuid_svmName
<!--- ******************************************************************************* -->
<!--- NOTE: AUTOMATION USES THE DESCRIPTIONS TO SET LABELS AND PRODUCE DOCUMENTATION. -->
<!--- PLEASE PUT AN 'X' in only **ONE** box -->
<!--- ******************************************************************************* -->
### Types of changes
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Bug fix (non-breaking change which fixes an issue)
- [ ] Enhancement (improves an existing feature and functionality)
- [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
- [ ] Build/CI
- [ ] Test (unit or integration test code)
### Feature/Enhancement Scale or Bug Severity
#### Feature/Enhancement Scale
- [ ] Major
- [ ] Minor
#### Bug Severity
- [ ] BLOCKER
- [ ] Critical
- [ ] Major
- [ ] Minor
- [ ] Trivial
### Screenshots (if appropriate):
<img width="1524" height="780" alt="image" src="https://github.com/user-attachments/assets/b899bb78-8ae4-4f40-b978-deb554161bca" />
<img width="1515" height="310" alt="image" src="https://github.com/user-attachments/assets/21a9a016-3bc9-47a3-99aa-986f1e6da2b5" />
### How Has This Been Tested?
1- Created 1st VM on ISCSI Storage pool - Igroup got create with new name format attached a screen shot and lun got mapped
2- Create 2nd VM on same storage pool - Igroup got reused and lun got mapped
3- Storage Pool to enter maintenance mode - All Luns got unmapped and igroup also got deleted
4- Storage Pool to cancel maintenance mode - New Igroup got created with same name format and All Luns got mapped again.
5- Delete All VMs - Igroup got deleted
#### How did you try to break this feature and the system with this change?
<!-- see how your change affects other areas of the code, etc. -->
<!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md) document -->
--
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
Re: [PR] Bugfix/cstackex 216 Public PR Review Comment: Igroup Name Length truncation [cloudstack]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13585:
URL: https://github.com/apache/cloudstack/pull/13585#discussion_r3558595194
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageUtils.java:
##########
@@ -36,41 +42,201 @@
public class OntapStorageUtils {
private static final Logger logger = LogManager.getLogger(OntapStorageUtils.class);
-
private static final String BASIC = "Basic";
private static final String AUTH_HEADER_COLON = ":";
+ /**
+ * Method generates authentication headers using storage backend credentials passed as normal string
+ *
+ * @param username -->> username of the storage backend
+ * @param password -->> normal decoded password of the storage backend
+ * @return
+ */
public static String generateAuthHeader (String username, String password) {
byte[] encodedBytes = Base64Utils.encode((username + AUTH_HEADER_COLON + password).getBytes(StandardCharsets.UTF_8));
return BASIC + StringUtils.SPACE + new String(encodedBytes);
}
+ public static CloudStackVolume createCloudStackVolumeRequestByProtocol(StoragePoolVO storagePool, Map<String, String> details, DataObject volumeObject) {
+ CloudStackVolume cloudStackVolumeRequest = null;
+
+ String protocol = details.get(OntapStorageConstants.PROTOCOL);
+ ProtocolType protocolType = ProtocolType.valueOf(protocol);
+ switch (protocolType) {
+ case NFS3:
+ cloudStackVolumeRequest = new CloudStackVolume();
+ cloudStackVolumeRequest.setDatastoreId(String.valueOf(storagePool.getId()));
+ cloudStackVolumeRequest.setVolumeInfo(volumeObject);
+ break;
+ case ISCSI:
+ Svm svm = new Svm();
+ svm.setName(details.get(OntapStorageConstants.SVM_NAME));
+ cloudStackVolumeRequest = new CloudStackVolume();
+ Lun lunRequest = new Lun();
+ lunRequest.setSvm(svm);
+
+ LunSpace lunSpace = new LunSpace();
+ lunSpace.setSize(volumeObject.getSize());
+ lunRequest.setSpace(lunSpace);
+ //Lun name is full path like in unified "/vol/VolumeName/LunName"
+ String lunName = volumeObject.getName().replace(OntapStorageConstants.HYPHEN, OntapStorageConstants.UNDERSCORE);
+ if(!isValidName(lunName)) {
+ String errMsg = "createAsync: Invalid dataObject name [" + lunName + "]. It must start with a letter and can only contain letters, digits, and underscores, and be up to 200 characters long.";
+ throw new InvalidParameterValueException(errMsg);
+ }
+ String lunFullName = getLunName(storagePool.getName(), lunName);
+ lunRequest.setName(lunFullName);
+
+ String osType = getOSTypeFromHypervisor(storagePool.getHypervisor().name());
+ lunRequest.setOsType(Lun.OsTypeEnum.valueOf(osType));
+
+ cloudStackVolumeRequest.setLun(lunRequest);
+ break;
+ default:
+ throw new CloudRuntimeException("Unsupported protocol " + protocol);
+
+ }
+ return cloudStackVolumeRequest;
+ }
+
+ public static boolean isValidName(String name) {
+ // Check for null and length constraint first
+ if (name == null || name.length() > 200) {
+ return false;
+ }
+ // Regex: Starts with a letter, followed by letters, digits, or underscores
+ return name.matches(OntapStorageConstants.ONTAP_NAME_REGEX);
+ }
+
+ public static String getOSTypeFromHypervisor(String hypervisorType) {
+ switch (hypervisorType) {
+ case OntapStorageConstants.KVM:
+ return Lun.OsTypeEnum.LINUX.name();
+ default:
+ String errMsg = "getOSTypeFromHypervisor : Unsupported hypervisor type " + hypervisorType + " for ONTAP storage";
+ logger.error(errMsg);
+ throw new CloudRuntimeException(errMsg);
+ }
+ }
+
+ /**
+ * Returns a connected {@link StorageStrategy} for operations on an existing pool (snapshots,
+ * delete, revert, grant/revoke). Does not require aggregate free space for the full pool size.
+ */
public static StorageStrategy getStrategyByStoragePoolDetails(Map<String, String> details) {
+ return getStrategyByStoragePoolDetails(details, false);
+ }
+
+ public static StorageStrategy getStrategyByStoragePoolDetails(Map<String, String> details,
+ boolean validateAggregatesForVolumeCreation) {
if (details == null || details.isEmpty()) {
logger.error("getStrategyByStoragePoolDetails: Storage pool details are null or empty");
- throw new CloudRuntimeException("getStrategyByStoragePoolDetails: Storage pool details are null or empty");
+ throw new CloudRuntimeException("Storage pool details are null or empty");
}
String protocol = details.get(OntapStorageConstants.PROTOCOL);
OntapStorage ontapStorage = new OntapStorage(details.get(OntapStorageConstants.USERNAME), details.get(OntapStorageConstants.PASSWORD),
- details.get(OntapStorageConstants.MANAGEMENT_LIF), details.get(OntapStorageConstants.SVM_NAME), Long.parseLong(details.get(OntapStorageConstants.SIZE)),
- ProtocolType.valueOf(protocol),
- Boolean.parseBoolean(details.get(OntapStorageConstants.IS_DISAGGREGATED)));
+ details.get(OntapStorageConstants.STORAGE_IP), details.get(OntapStorageConstants.SVM_NAME), Long.parseLong(details.get(OntapStorageConstants.SIZE)),
+ ProtocolType.valueOf(protocol));
StorageStrategy storageStrategy = StorageProviderFactory.getStrategy(ontapStorage);
- boolean isValid = storageStrategy.connect();
+ boolean isValid = storageStrategy.connect(validateAggregatesForVolumeCreation);
if (isValid) {
logger.info("Connection to Ontap SVM [{}] successful", details.get(OntapStorageConstants.SVM_NAME));
return storageStrategy;
} else {
logger.error("getStrategyByStoragePoolDetails: Connection to Ontap SVM [" + details.get(OntapStorageConstants.SVM_NAME) + "] failed");
- throw new CloudRuntimeException("getStrategyByStoragePoolDetails: Connection to Ontap SVM [" + details.get(OntapStorageConstants.SVM_NAME) + "] failed");
+ throw new CloudRuntimeException("Connection to Ontap SVM [" + details.get(OntapStorageConstants.SVM_NAME) + "] failed");
}
}
- public static String getIgroupName(String svmName, ScopeType scopeType, Long scopeId) {
- return OntapStorageConstants.CS + OntapStorageConstants.UNDERSCORE + svmName + OntapStorageConstants.UNDERSCORE + scopeType.toString().toLowerCase() + OntapStorageConstants.UNDERSCORE + scopeId;
+ public static String getIgroupName(String svmName, String hostUuid) {
+ //Igroup name format: cs_hostUuid_svmName
+ String sanitizedHostUuid = hostUuid.replaceAll("[^a-zA-Z0-9_-]", "_");
+ String igroupName = OntapStorageConstants.CS + OntapStorageConstants.UNDERSCORE + sanitizedHostUuid + OntapStorageConstants.UNDERSCORE + svmName;
+ // 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() still throws NullPointerException when hostUuid is null (hostUuid.replaceAll(...)). The PR description calls out this NPE; this method should validate inputs (and ideally fail fast with a clear error) before sanitizing/truncating.
##########
ui/src/views/infra/zone/ZoneWizardLaunchZone.vue:
##########
@@ -1604,6 +1604,18 @@ export default {
params.url = this.powerflexURL(this.prefillContent.powerflexGateway, this.prefillContent.powerflexGatewayUsername,
this.prefillContent.powerflexGatewayPassword, this.prefillContent.powerflexStoragePool)
}
+ if (this.prefillContent.provider === 'NetApp ONTAP') {
+ params['details[0].storageIP'] = this.prefillContent.ontapIP
+ params['details[0].username'] = this.prefillContent.ontapUsername
+ params['details[0].password'] = btoa(this.prefillContent.ontapPassword)
+ params['details[0].svmName'] = this.prefillContent.ontapSvmName
+ params['details[0].protocol'] = this.prefillContent.primaryStorageProtocol
Review Comment:
Using btoa() directly for the ONTAP password is fragile: it throws for non-Latin1 input and bypasses the UI’s existing base64+UTF-8 helper used elsewhere. Consider using $toBase64AndURIEncoded (ui/src/utils/plugins.js) for consistent encoding.
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/provider/StorageProviderFactory.java:
##########
@@ -36,23 +39,23 @@ public class StorageProviderFactory {
public static StorageStrategy getStrategy(OntapStorage ontapStorage) {
ProtocolType protocol = ontapStorage.getProtocol();
logger.info("Initializing StorageProviderFactory with protocol: " + protocol);
+ String decodedPassword = new String(java.util.Base64.getDecoder().decode(ontapStorage.getPassword()), StandardCharsets.UTF_8);
+ ontapStorage = new OntapStorage(
+ ontapStorage.getUsername(),
+ decodedPassword,
+ ontapStorage.getStorageIP(),
+ ontapStorage.getSvmName(),
+ ontapStorage.getSize(),
+ protocol);
Review Comment:
StorageProviderFactory decodes ontapStorage.getPassword() as Base64 unconditionally. If the stored value is not valid Base64 (e.g. created by older code paths / API clients), this will throw IllegalArgumentException and break all ONTAP operations at runtime.
##########
ui/src/views/infra/AddPrimaryStorage.vue:
##########
@@ -890,6 +935,14 @@ export default {
params['details[0].api_username'] = values.flashArrayUsername
params['details[0].api_password'] = values.flashArrayPassword
url = values.flashArrayURL
+ } else if (values.provider === 'NetApp ONTAP') {
+ params['details[0].storageIP'] = values.ontapIP
+ params['details[0].username'] = values.ontapUsername
+ params['details[0].password'] = btoa(values.ontapPassword)
+ params['details[0].svmName'] = values.ontapSvmName
+ params['details[0].protocol'] = values.protocol
+ values.managed = true
+ url = this.ontapURL(values.ontapIP)
}
Review Comment:
ONTAP password is encoded with btoa(), which is not UTF-8 safe and can throw for non-Latin1 characters. The UI already provides $toBase64AndURIEncoded for consistent base64 encoding across forms.
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java:
##########
@@ -207,67 +217,54 @@ private long validateInitializeInputs(Long capacityBytes, Long podId, Long clust
capacityBytes = ONTAP_MIN_VOLUME_SIZE_IN_BYTES;
}
- // Scope (pod/cluster/zone) validation
+ // Validate scope
if (podId == null ^ clusterId == null) {
throw new CloudRuntimeException("Cluster Id or Pod Id is null, cannot create primary storage");
}
- if (podId == null && clusterId == null) {
- if (zoneId != null) {
- logger.info("Both Pod Id and Cluster Id are null, Primary storage pool will be associated with a Zone");
- } else {
- throw new CloudRuntimeException("Pod Id, Cluster Id and Zone Id are all null, cannot create primary storage");
- }
+
+ if (podId == null && zoneId == null) {
+ throw new CloudRuntimeException("Pod Id, Cluster Id and Zone Id are all null, cannot create primary storage");
+ }
+
+ if (podId == null) {
+ logger.info("Both Pod Id and Cluster Id are null, Primary storage pool will be associated with a Zone");
}
- // Basic parameter validation
if (StringUtils.isBlank(storagePoolName)) {
throw new CloudRuntimeException("Storage pool name is null or empty, cannot create primary storage");
}
+
if (StringUtils.isBlank(providerName)) {
throw new CloudRuntimeException("Provider name is null or empty, cannot create primary storage");
}
+
logger.debug("ONTAP primary storage will be created as " + (managed ? "managed" : "unmanaged"));
if (!managed) {
throw new CloudRuntimeException("ONTAP primary storage must be managed");
}
- // Details key validation
+ //Required ONTAP detail keys
Set<String> requiredKeys = Set.of(
OntapStorageConstants.USERNAME,
OntapStorageConstants.PASSWORD,
OntapStorageConstants.SVM_NAME,
OntapStorageConstants.PROTOCOL,
- OntapStorageConstants.MANAGEMENT_LIF
- );
- Set<String> optionalKeys = Set.of(
- OntapStorageConstants.IS_DISAGGREGATED
+ OntapStorageConstants.STORAGE_IP
);
- Set<String> allowedKeys = new java.util.HashSet<>(requiredKeys);
- allowedKeys.addAll(optionalKeys);
-
- if (StringUtils.isNotBlank(url)) {
- for (String segment : url.split(OntapStorageConstants.SEMICOLON)) {
- if (segment.isEmpty()) {
- continue;
- }
- String[] kv = segment.split(OntapStorageConstants.EQUALS, 2);
- if (kv.length == 2) {
- details.put(kv[0].trim(), kv[1].trim());
- }
- }
- }
+ // Validate existing entries (reject unexpected keys, empty values)
for (Map.Entry<String, String> e : details.entrySet()) {
String key = e.getKey();
String val = e.getValue();
- if (!allowedKeys.contains(key)) {
+ if (!requiredKeys.contains(key)) {
throw new CloudRuntimeException("Unexpected ONTAP detail key in URL: " + key);
}
if (StringUtils.isBlank(val)) {
throw new CloudRuntimeException("ONTAP primary storage creation failed, empty detail: " + key);
}
Review Comment:
validateInitializeInputs() throws "Unexpected ONTAP detail key in URL" but the code no longer parses ONTAP details from the URL; it validates the provided details map. This error message is misleading, and details can also be null (causing NPE) since it is not validated before iterating.
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedNASStrategy.java:
##########
@@ -87,6 +97,19 @@ CloudStackVolume updateCloudStackVolume(CloudStackVolume cloudstackVolume) {
@Override
public void deleteCloudStackVolume(CloudStackVolume cloudstackVolume) {
+ logger.info("deleteCloudStackVolume: Delete cloudstack volume " + cloudstackVolume);
+ try {
+ // Step 1: Send command to KVM host to delete qcow2 file using qemu-img
+ Answer answer = deleteVolumeOnKVMHost(cloudstackVolume.getVolumeInfo());
+ if (answer == null || !answer.getResult()) {
+ String errMsg = answer != null ? answer.getDetails() : "Failed to delete qcow2 on KVM host";
+ logger.error("deleteCloudStackVolume: " + errMsg);
+ throw new CloudRuntimeException(errMsg);
+ }
+ } catch (Exception e) {
+ logger.error("deleteCloudStackVolume: error occured " + e);
+ throw new CloudRuntimeException(e);
+ }
Review Comment:
Same logging issue as createCloudStackVolume(): the catch block logs "error occured" (typo) and drops stack trace by string concatenation. Use logger.error(..., e).
##########
engine/api/src/main/java/org/apache/cloudstack/engine/subsystem/api/storage/DataStoreProvider.java:
##########
@@ -29,6 +29,11 @@ public interface DataStoreProvider {
String SAMPLE_IMAGE = "Sample";
String SMB = "NFS";
String DEFAULT_PRIMARY = "DefaultPrimary";
+ /**
+ * Primary storage provider name for NetApp ONTAP ({@code OntapPrimaryDatastoreProvider#getName}).
+ * Single canonical value; use this instead of duplicating the string across modules.
+ */
+ String ONTAP_PLUGIN_NAME = "NetApp ONTAP";
Review Comment:
This introduces a new canonical provider name value ("NetApp ONTAP"). If any existing installations have storage_pool.storage_provider_name persisted as the previous ONTAP plugin name (the ONTAP plugin previously used "ONTAP" in OntapStorageConstants), equality checks against DataStoreProvider.ONTAP_PLUGIN_NAME will stop matching and can break snapshot/managed-storage logic after upgrade. Consider supporting both legacy and new names (or adding a DB migration) to avoid upgrade regressions.
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedNASStrategy.java:
##########
@@ -77,7 +72,22 @@ public void setOntapStorage(OntapStorage ontapStorage) {
@Override
public CloudStackVolume createCloudStackVolume(CloudStackVolume cloudstackVolume) {
- return null;
+ logger.info("createCloudStackVolume: Create cloudstack volume " + cloudstackVolume);
+ try {
+ // Step 1: set cloudstack volume metadata
+ updateCloudStackVolumeMetadata(cloudstackVolume.getDatastoreId(), cloudstackVolume.getVolumeInfo());
+ // Step 2: Send command to KVM host to create qcow2 file using qemu-img
+ Answer answer = createVolumeOnKVMHost(cloudstackVolume.getVolumeInfo());
+ if (answer == null || !answer.getResult()) {
+ String errMsg = answer != null ? answer.getDetails() : "Failed to create qcow2 on KVM host";
+ logger.error("createCloudStackVolume: " + errMsg);
+ throw new CloudRuntimeException(errMsg);
+ }
+ return cloudstackVolume;
+ }catch (Exception e) {
+ logger.error("createCloudStackVolume: error occured " + e);
+ throw new CloudRuntimeException(e);
+ }
Review Comment:
The catch block logs "error occured" (typo) and drops the stack trace by concatenating the exception into the message. Prefer structured logging with the throwable so the stack trace is preserved.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/IscsiAdmStorageAdaptor.java:
##########
@@ -158,6 +159,56 @@ public boolean connectPhysicalDisk(String volumeUuid, KVMStoragePool pool, Map<S
return true;
}
+ /**
+ * Checks the result of an iscsiadm node-create command.
+ * Returns true if the node was created or already exists, false on failure.
+ */
+ boolean handleNodeCreateResult(String result, String volumeUuid) {
+ if (result == null) {
+ logger.debug("Successfully added iSCSI node for target {}", volumeUuid);
+ return true;
+ }
+ String msg = result.toLowerCase();
+ if (msg.contains("already exists") || msg.contains("database exists") || msg.contains("exists")) {
+ logger.debug("iSCSI node already exists for target {}, proceeding", volumeUuid);
+ return true;
+ }
Review Comment:
handleNodeCreateResult() treats any iscsiadm output containing "exists" as a success. This can incorrectly mark failures as success for messages like "does not exist" (contains "exist"). Restrict the check to the known benign node-exists strings.
--
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
Re: [PR] Bugfix/cstackex 216 Public PR Review Comment: Igroup Name Length truncation [cloudstack]
Posted by "suryag1201 (via GitHub)" <gi...@apache.org>.
suryag1201 closed pull request #13585: Bugfix/cstackex 216 Public PR Review Comment: Igroup Name Length truncation
URL: https://github.com/apache/cloudstack/pull/13585
--
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