Copilot commented on code in PR #14405:
URL: https://github.com/apache/cloudstack/pull/14405#discussion_r4281192410
##########
api/src/main/java/org/apache/cloudstack/api/command/admin/storage/PreparePrimaryStorageForMaintenanceCmd.java:
##########
@@ -33,8 +33,9 @@
import com.cloud.storage.StoragePool;
import com.cloud.user.Account;
-@APICommand(name = "enableStorageMaintenance", description = "Puts storage
pool into maintenance state", responseObject = StoragePoolResponse.class,
- requestHasSensitiveInfo = false, responseHasSensitiveInfo = false)
+@APICommand(name = "enableStorageMaintenance",
+ description = "Puts storage pool into maintenance state. The storage
pool must be Up or Disabled",
Review Comment:
The API description says the pool must be \"Up or Disabled\", but the
backend check also allows \"ErrorInMaintenance\" (see StorageManagerImpl). This
is user-visible API documentation, so it should match actual accepted states.
Either update the description to include ErrorInMaintenance, or tighten the
backend validation to match the documented states.
##########
server/src/main/java/com/cloud/storage/StorageManagerImpl.java:
##########
@@ -2606,6 +2608,19 @@ public void
doInTransactionWithoutResult(TransactionStatus status) {
}
}
+ /**
+ * Remembers a Disabled pool's status so that cancelling maintenance puts
it back to Disabled instead of Up
+ * (restored in PrimaryDataStoreHelper#cancelMaintain). A retry from
ErrorInMaintenance keeps what the
+ * first attempt recorded.
+ */
+ protected void recordStatusBeforeMaintenance(StoragePoolVO pool) {
+ if (pool.getStatus() == StoragePoolStatus.Disabled) {
+ _storagePoolDetailsDao.addDetail(pool.getId(),
PrimaryDataStoreLifeCycle.STATUS_BEFORE_MAINTENANCE,
StoragePoolStatus.Disabled.name(), false);
+ } else if (pool.getStatus() == StoragePoolStatus.Up) {
+ _storagePoolDetailsDao.removeDetail(pool.getId(),
PrimaryDataStoreLifeCycle.STATUS_BEFORE_MAINTENANCE);
+ }
+ }
Review Comment:
`recordStatusBeforeMaintenance` is not idempotent for Disabled pools:
calling it multiple times while the pool remains Disabled will call
`addDetail(...)` repeatedly, which can violate unique constraints or create
duplicates depending on DAO behavior. Make this safe to call multiple times by
first removing any existing detail for the same key, or by using an
update/upsert-style DAO method (e.g., find + update, or removeDetail +
addDetail).
##########
server/src/main/java/com/cloud/storage/StorageManagerImpl.java:
##########
@@ -2550,14 +2550,15 @@ public PrimaryDataStoreInfo
preparePrimaryStorageForMaintenance(Long primaryStor
throw new InvalidParameterValueException(msg);
}
- if (!primaryStorage.getStatus().equals(StoragePoolStatus.Up) &&
!primaryStorage.getStatus().equals(StoragePoolStatus.ErrorInMaintenance)) {
+ if (!Arrays.asList(StoragePoolStatus.Up, StoragePoolStatus.Disabled,
StoragePoolStatus.ErrorInMaintenance).contains(primaryStorage.getStatus())) {
Review Comment:
This creates a new list instance each call just to do a membership check
(same pattern also appears in `cancelPrimaryStorageForMaintenance`). Prefer a
static `EnumSet` (or equivalent) or a `switch`/explicit comparisons to avoid
repeated allocations and make the allowed states easier to reuse consistently.
--
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]
