Copilot commented on code in PR #13518:
URL: https://github.com/apache/cloudstack/pull/13518#discussion_r3499135319
##########
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:
Using `btoa` to encode the ONTAP password can throw for non‑Latin1 (unicode)
passwords. Prefer the existing `$toBase64AndURIEncoded` helper used elsewhere
in the UI (e.g. RegisterUserData.vue) to avoid runtime errors and ensure safe
query encoding.
##########
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` to encode the ONTAP password can throw for non-Latin1 (unicode)
passwords and can also lead to inconsistent encoding. The UI already provides
`$toBase64AndURIEncoded` for safely base64+URI encoding user input (see e.g.
ui/src/views/compute/RegisterUserData.vue:233).
##########
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:
`Base64.getDecoder().decode(ontapStorage.getPassword())` will throw
`IllegalArgumentException` if the password is not base64-encoded (e.g. API
clients providing a raw password, or older DB entries). This makes pool
operations fail hard. Consider decoding defensively with a fallback to the raw
value.
##########
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 as above: concatenating the exception into the log message drops the
stack trace, and the message has a typo ("occured"). Prefer `logger.error(msg,
e)` for actionable logs.
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/model/CloudStackVolume.java:
##########
@@ -25,10 +25,34 @@
public class CloudStackVolume {
+ /**
+ * Filed used for request:
+ * a. snapshot workflows will get source file details from it.
+ */
Review Comment:
Typo in Javadoc: "Filed" → "Field".
##########
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;
+ }
+ logger.debug("Failed to add iSCSI node for target {}: {}", volumeUuid,
result);
Review Comment:
`handleNodeCreateResult` treats any iscsiadm output containing the substring
"exists" as success. This is overly broad and can mask real failures whose
message happens to include that word. It’s safer to only treat known benign
phrases (e.g. "already exists" / "database exists") as idempotent success.
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/model/CloudStackVolume.java:
##########
@@ -25,10 +25,34 @@
public class CloudStackVolume {
+ /**
+ * Filed used for request:
+ * a. snapshot workflows will get source file details from it.
+ */
private FileInfo file;
+
+ /**
+ * Filed used for request:
+ * a. snapshot workflows will get source LUN details from it.
+ */
Review Comment:
Typo in Javadoc: "Filed" → "Field".
##########
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 by concatenating the exception into the message (`"..."
+ e`), which drops the stack trace and also contains a typo ("occured").
Logging with the throwable argument preserves stack traces for debugging.
--
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]
