Copilot commented on code in PR #13578:
URL: https://github.com/apache/cloudstack/pull/13578#discussion_r3569406386
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java:
##########
@@ -135,31 +136,42 @@ public boolean connect() {
logger.error("No aggregates are assigned to SVM " + svmName);
throw new CloudRuntimeException("No aggregates are assigned to
SVM " + svmName);
}
+ // Collect all online aggregates assigned to the SVM.
Capacity-based selection is
+ // intentionally deferred to createStorageVolume(name, size),
which validates the
+ // available space against the actual requested volume size.
+ List<Aggregate> eligibleAggregates = new ArrayList<>();
for (Aggregate aggr : aggrs) {
logger.debug("Found aggregate: " + aggr.getName() + " with
UUID: " + aggr.getUuid());
Aggregate aggrResp =
aggregateFeignClient.getAggregateByUUID(authHeader, aggr.getUuid());
if (aggrResp == null) {
logger.warn("Aggregate details response is null for
aggregate " + aggr.getName() + ". Skipping.");
- break;
+ continue;
}
if (!Objects.equals(aggrResp.getState(),
Aggregate.StateEnum.ONLINE)) {
logger.warn("Aggregate " + aggr.getName() + " is not in
online state. Skipping this aggregate.");
continue;
- } else if (aggrResp.getSpace() == null ||
aggrResp.getAvailableBlockStorageSpace() == null ||
- aggrResp.getAvailableBlockStorageSpace() <=
storage.getSize().doubleValue()) {
- logger.warn("Aggregate " + aggr.getName() + " does not
have sufficient available space. Skipping this aggregate.");
- continue;
}
- logger.info("Selected aggregate: " + aggr.getName() + " for
volume operations.");
- this.aggregates = List.of(aggr);
- break;
+ logger.debug("Aggregate " + aggr.getName() + " is online and
eligible for volume operations.");
+ eligibleAggregates.add(aggr);
}
- if (this.aggregates == null || this.aggregates.isEmpty()) {
- logger.error("No suitable aggregates found on SVM " + svmName
+ " for volume creation.");
- throw new CloudRuntimeException("No suitable aggregates found
on SVM " + svmName + " for volume creation.");
+ if (eligibleAggregates.isEmpty()) {
+ logger.error("No suitable aggregates found on SVM " + svmName
+ " for volume operations.");
+ throw new CloudRuntimeException("No suitable aggregates found
on SVM " + svmName + " for volume operations.");
}
+ this.aggregates = eligibleAggregates;
+ logger.info("Found " + eligibleAggregates.size() + " online
aggregate(s) on SVM " + svmName + " for volume operations.");
logger.info("Successfully connected to ONTAP cluster and validated
ONTAP details provided");
+ } catch (FeignException.Unauthorized e) {
+ logger.error("Authentication failed while connecting to ONTAP
cluster at " + storage.getStorageIP() +
+ ". Please verify the username and password.", e);
+ throw new CloudRuntimeException("Authentication failed: Invalid
credentials for ONTAP cluster at " +
+ storage.getStorageIP() + ". Please verify the username and
password.");
Review Comment:
The Unauthorized handler logs the underlying FeignException but then throws
a new CloudRuntimeException without preserving the cause, which makes
downstream troubleshooting (and error aggregation) harder when logs are not
available. Prefer using the (message, cause) constructor so the original
exception is retained.
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java:
##########
@@ -135,31 +136,42 @@ public boolean connect() {
logger.error("No aggregates are assigned to SVM " + svmName);
throw new CloudRuntimeException("No aggregates are assigned to
SVM " + svmName);
}
+ // Collect all online aggregates assigned to the SVM.
Capacity-based selection is
+ // intentionally deferred to createStorageVolume(name, size),
which validates the
+ // available space against the actual requested volume size.
+ List<Aggregate> eligibleAggregates = new ArrayList<>();
for (Aggregate aggr : aggrs) {
logger.debug("Found aggregate: " + aggr.getName() + " with
UUID: " + aggr.getUuid());
Aggregate aggrResp =
aggregateFeignClient.getAggregateByUUID(authHeader, aggr.getUuid());
if (aggrResp == null) {
logger.warn("Aggregate details response is null for
aggregate " + aggr.getName() + ". Skipping.");
- break;
+ continue;
}
if (!Objects.equals(aggrResp.getState(),
Aggregate.StateEnum.ONLINE)) {
logger.warn("Aggregate " + aggr.getName() + " is not in
online state. Skipping this aggregate.");
continue;
- } else if (aggrResp.getSpace() == null ||
aggrResp.getAvailableBlockStorageSpace() == null ||
- aggrResp.getAvailableBlockStorageSpace() <=
storage.getSize().doubleValue()) {
- logger.warn("Aggregate " + aggr.getName() + " does not
have sufficient available space. Skipping this aggregate.");
- continue;
}
- logger.info("Selected aggregate: " + aggr.getName() + " for
volume operations.");
- this.aggregates = List.of(aggr);
- break;
+ logger.debug("Aggregate " + aggr.getName() + " is online and
eligible for volume operations.");
+ eligibleAggregates.add(aggr);
}
- if (this.aggregates == null || this.aggregates.isEmpty()) {
- logger.error("No suitable aggregates found on SVM " + svmName
+ " for volume creation.");
- throw new CloudRuntimeException("No suitable aggregates found
on SVM " + svmName + " for volume creation.");
+ if (eligibleAggregates.isEmpty()) {
+ logger.error("No suitable aggregates found on SVM " + svmName
+ " for volume operations.");
+ throw new CloudRuntimeException("No suitable aggregates found
on SVM " + svmName + " for volume operations.");
}
+ this.aggregates = eligibleAggregates;
+ logger.info("Found " + eligibleAggregates.size() + " online
aggregate(s) on SVM " + svmName + " for volume operations.");
logger.info("Successfully connected to ONTAP cluster and validated
ONTAP details provided");
+ } catch (FeignException.Unauthorized e) {
+ logger.error("Authentication failed while connecting to ONTAP
cluster at " + storage.getStorageIP() +
+ ". Please verify the username and password.", e);
+ throw new CloudRuntimeException("Authentication failed: Invalid
credentials for ONTAP cluster at " +
+ storage.getStorageIP() + ". Please verify the username and
password.");
+ } catch (FeignException.Forbidden e) {
+ logger.error("Authorization failed while connecting to ONTAP
cluster at " + storage.getStorageIP() +
+ ". The user does not have sufficient privileges.", e);
+ throw new CloudRuntimeException("Authorization failed: User does
not have sufficient privileges on ONTAP cluster at " +
+ storage.getStorageIP() + ". Please verify user
permissions.");
Review Comment:
Same as the Unauthorized case: the Forbidden handler discards the original
exception when rethrowing, which loses stack/context for callers. Preserve the
cause in the CloudRuntimeException.
##########
server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java:
##########
@@ -5092,6 +5093,13 @@ 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.
+
if(DataStoreProvider.ONTAP_PLUGIN_NAME.equals(volumeToAttachStoragePool.getStorageProviderName())){
+ volumeToAttach = _volsDao.findById(volumeToAttach.getId());
+ }
Review Comment:
Minor formatting: this `if` is missing the usual space after the keyword
(`if (`), which is the prevailing style in this file (see e.g. `if (host !=
null)` above).
--
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]
