Copilot commented on code in PR #13053:
URL: https://github.com/apache/cloudstack/pull/13053#discussion_r3510494159
##########
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:
`btoa()` will throw for non‑ASCII passwords (it only accepts Latin‑1), which
would break ONTAP primary storage creation in the zone wizard for unicode
passwords. Use the existing UTF‑8-safe base64 helper instead of `btoa`.
##########
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:
`btoa()` will throw for non‑ASCII passwords (it only accepts Latin‑1), which
would break ONTAP primary storage creation for users with unicode passwords.
Use the existing UTF‑8-safe base64 helper instead of calling `btoa` directly.
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java:
##########
@@ -411,23 +386,49 @@ private boolean
validateProtocolSupportAndFetchHostsIdentifier(List<HostVO> host
for (HostVO host : hosts) {
if (host != null) {
ip = host.getStorageIpAddress() != null ?
host.getStorageIpAddress().trim() : "";
- if (ip.isEmpty()) {
- if (host.getPrivateIpAddress() == null ||
host.getPrivateIpAddress().trim().isEmpty()) {
- return false;
- }
- ip = host.getPrivateIpAddress().trim();
+ if (ip.isEmpty() &&
StringUtils.isBlank(host.getPrivateIpAddress() )) {
+ // TODO we will inform customer through alert for
excluded host because of protocol enabled on host
+ continue;
+ } else {
+ ip = ip.isEmpty() ?
host.getPrivateIpAddress().trim() : ip;
}
+ hostIdentifiers.add(ip);
}
- hostIdentifiers.add(ip);
}
break;
default:
- throw new
CloudRuntimeException("validateProtocolSupportAndFetchHostsIdentifier :
Unsupported protocol: " + protocolType.name());
+ throw new CloudRuntimeException("Unsupported protocol: " +
protocolType.name());
}
logger.info("validateProtocolSupportAndFetchHostsIdentifier: All hosts
support the protocol: " + protocolType.name());
return true;
}
+ /**
+ * Creates an NFS export policy (access group) on the ONTAP storage if the
protocol is NFS3
+ * and there are eligible hosts. Skipped for iSCSI (igroups are created
per-host in grantAccess).
+ */
+ private void createNfsAccessGroupIfNeeded(Map<String, String> details,
List<String> hostsIdentifier,
+ List<HostVO> hostsToConnect,
Scope scope,
+ StoragePoolVO storagePool,
StorageStrategy strategy) {
+ if
(!ProtocolType.NFS3.name().equalsIgnoreCase(details.get(OntapStorageConstants.PROTOCOL)))
{
+ return;
+ }
+ if (hostsIdentifier.isEmpty()) {
+ // No eligible hosts — export policy will be created later via
HostListener when hosts come up
+ return;
+ }
+ try {
+ AccessGroup accessGroupRequest = new AccessGroup();
+ accessGroupRequest.setHostsToConnect(hostsToConnect);
+ accessGroupRequest.setScope(scope);
+ accessGroupRequest.setStoragePoolId(storagePool.getId());
+ strategy.createAccessGroup(accessGroupRequest);
Review Comment:
`createNfsAccessGroupIfNeeded` passes the full `hostsToConnect` list into
`createAccessGroup`, but `validateProtocolSupportAndFetchHostsIdentifier` only
*skips* hosts without a usable storage/private IP and does not filter them out
of `hostsToConnect`. This can lead to export-policy rules like `null/32` being
generated for those hosts. Filter `hostsToConnect` down to hosts with a usable
IP before creating the access group.
##########
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 the exception by concatenating it into the message,
which drops the stack trace, and the message contains a typo ("occured"). Log
with the throwable and throw a clearer `CloudRuntimeException` message.
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedNASStrategy.java:
##########
@@ -300,4 +331,153 @@ private ExportPolicy
createExportPolicyRequest(AccessGroup accessGroup,String sv
return exportPolicy;
}
+
+ private String updateCloudStackVolumeMetadata(String dataStoreId,
DataObject volumeInfo) {
+ logger.info("updateCloudStackVolumeMetadata called with datastoreID:
{} volumeInfo: {} ", dataStoreId, volumeInfo );
+ try {
+ VolumeObject volumeObject = (VolumeObject) volumeInfo;
+ long volumeId = volumeObject.getId();
+ logger.info("updateCloudStackVolumeMetadata: VolumeInfo ID from
VolumeObject: {}", volumeId);
+ VolumeVO volume = volumeDao.findById(volumeId);
+ if (volume == null) {
+ throw new CloudRuntimeException("Volume not found with id: " +
volumeId);
+ }
+ String volumeUuid = volumeInfo.getUuid();
+ volume.setPoolType(Storage.StoragePoolType.NetworkFilesystem);
+ volume.setPoolId(Long.parseLong(dataStoreId));
+ volume.setPath(volumeUuid); // Filename for qcow2 file
+ volumeDao.update(volume.getId(), volume);
+ logger.info("Updated volume path to {} for volume ID {}",
volumeUuid, volumeId);
+ return volumeUuid;
+ } catch (Exception e){
+ logger.error("updateCloudStackVolumeMetadata: Exception while
updating volumeInfo: {} in volume: {}", dataStoreId, volumeInfo.getUuid(), e);
+ throw new CloudRuntimeException("Exception while updating
volumeInfo: " + e.getMessage());
+ }
+ }
+
+ private Answer createVolumeOnKVMHost(DataObject volumeInfo) {
+ logger.info("createVolumeOnKVMHost called with volumeInfo: {} ",
volumeInfo);
+
+ try {
+ logger.info("createVolumeOnKVMHost: Sending CreateObjectCommand to
KVM agent for volume: {}", volumeInfo.getUuid());
+ CreateObjectCommand cmd = new
CreateObjectCommand(volumeInfo.getTO());
+ EndPoint ep = epSelector.select(volumeInfo);
+ if (ep == null) {
+ String errMsg = "No remote endpoint to send
CreateObjectCommand, check if host is up";
+ logger.error(errMsg);
+ return new Answer(cmd, false, errMsg);
+ }
+ logger.info("createVolumeOnKVMHost: Sending command to endpoint:
{}", ep.getHostAddr());
+ Answer answer = ep.sendMessage(cmd);
+ if (answer != null && answer.getResult()) {
+ logger.info("createVolumeOnKVMHost: Successfully created qcow2
file on KVM host");
+ } else {
+ logger.error("createVolumeOnKVMHost: Failed to create qcow2
file: {}",
+ answer != null ? answer.getDetails() : "null answer");
+ }
+ return answer;
+ } catch (Exception e) {
+ logger.error("createVolumeOnKVMHost: Exception sending
CreateObjectCommand", e);
+ return new Answer(null, false, e.toString());
+ }
Review Comment:
On exception, `createVolumeOnKVMHost` returns `new Answer(null, ...)`,
losing the original command context. Returning an `Answer` tied to the
`CreateObjectCommand` makes failures easier to diagnose and keeps behavior
consistent with the `ep == null` branch.
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedNASStrategy.java:
##########
@@ -280,13 +311,13 @@ private ExportPolicy
createExportPolicyRequest(AccessGroup accessGroup,String sv
String ip = (hostStorageIp != null && !hostStorageIp.isEmpty())
? hostStorageIp
: host.getPrivateIpAddress();
- String ipToUse = ip + "/31";
+ String ipToUse = ip + "/32";
ExportRule.ExportClient exportClient = new
ExportRule.ExportClient();
exportClient.setMatch(ipToUse);
exportClients.add(exportClient);
Review Comment:
`createExportPolicyRequest` assumes every host has either `storageIpAddress`
or `privateIpAddress`. If both are null/empty, this will emit an invalid client
match like `null/32` into the export policy. Skip such hosts (and also guard
against `null` host entries).
--
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]
