Copilot commented on code in PR #13050:
URL: https://github.com/apache/cloudstack/pull/13050#discussion_r3117291035
##########
plugins/storage/volume/flasharray/src/main/java/org/apache/cloudstack/storage/datastore/adapter/flasharray/FlashArrayAdapter.java:
##########
@@ -453,18 +453,46 @@ public void disconnect() {
@Override
public ProviderVolumeStorageStats getManagedStorageStats() {
FlashArrayPod pod = getVolumeNamespace(this.pod);
- // just in case
- if (pod == null || pod.getFootprint() == 0) {
+ if (pod == null) {
return null;
}
Long capacityBytes = pod.getQuotaLimit();
- Long usedBytes = pod.getQuotaLimit() - (pod.getQuotaLimit() -
pod.getFootprint());
+ if (capacityBytes == null || capacityBytes == 0) {
+ // Pod has no explicit quota set; report the array total physical
+ // capacity so the CloudStack allocator has a real ceiling to plan
+ // against rather than bailing out with a zero-capacity pool.
+ capacityBytes = getArrayTotalCapacity();
+ }
+ if (capacityBytes == null || capacityBytes == 0) {
+ return null;
+ }
+ Long usedBytes = pod.getFootprint();
+ if (usedBytes == null) {
+ usedBytes = 0L;
+ }
ProviderVolumeStorageStats stats = new ProviderVolumeStorageStats();
stats.setCapacityInBytes(capacityBytes);
stats.setActualUsedInBytes(usedBytes);
return stats;
}
+ private Long getArrayTotalCapacity() {
+ try {
+ FlashArrayList<Map<String, Object>> list =
GET("/arrays?space=true",
+ new TypeReference<FlashArrayList<Map<String, Object>>>() {
+ });
+ if (list != null && list.getItems() != null &&
!list.getItems().isEmpty()) {
+ Object cap = list.getItems().get(0).get("capacity");
+ if (cap instanceof Number) {
+ return ((Number) cap).longValue();
+ }
+ }
+ } catch (Exception e) {
+ logger.warn("Could not retrieve array total capacity", e);
Review Comment:
The warning log in `getArrayTotalCapacity()` doesn’t include any identifying
context (e.g., array URL / configured pod). When multiple FlashArray pools are
configured, this makes it hard to trace which backend is failing. Consider
including `url` (and possibly `pod`) in the message, and avoid logging a full
stack trace at warn level if this is expected to be retried frequently.
```suggestion
logger.warn("Could not retrieve array total capacity for
configured pod [{}]: {}", this.pod, e.getMessage());
logger.debug("Error retrieving array total capacity for
configured pod [{}]", this.pod, e);
```
##########
plugins/storage/volume/flasharray/src/main/java/org/apache/cloudstack/storage/datastore/adapter/flasharray/FlashArrayAdapter.java:
##########
@@ -453,18 +453,46 @@ public void disconnect() {
@Override
public ProviderVolumeStorageStats getManagedStorageStats() {
FlashArrayPod pod = getVolumeNamespace(this.pod);
- // just in case
- if (pod == null || pod.getFootprint() == 0) {
+ if (pod == null) {
return null;
}
Long capacityBytes = pod.getQuotaLimit();
- Long usedBytes = pod.getQuotaLimit() - (pod.getQuotaLimit() -
pod.getFootprint());
+ if (capacityBytes == null || capacityBytes == 0) {
+ // Pod has no explicit quota set; report the array total physical
+ // capacity so the CloudStack allocator has a real ceiling to plan
+ // against rather than bailing out with a zero-capacity pool.
+ capacityBytes = getArrayTotalCapacity();
+ }
Review Comment:
Calling `GET("/arrays?space=true")` inside `getManagedStorageStats()` can
add a second REST request on every storage-stats refresh whenever a pod has no
quota. Since `FlashArrayAdapterFactory` constructs a new adapter per call, this
likely won’t be amortized/cached and could create avoidable API load. Consider
memoizing the array capacity (e.g., static cache keyed by URL with a TTL) or
persisting the discovered capacity into CloudStack pool details so subsequent
stats calls don’t need to re-query `/arrays` each time.
--
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]
