Copilot commented on code in PR #12678:
URL: https://github.com/apache/cloudstack/pull/12678#discussion_r2839025602
##########
server/src/main/java/com/cloud/storage/StorageManagerImpl.java:
##########
@@ -3990,6 +3993,37 @@ public ImageStore discoverImageStore(String name, String
url, String providerNam
throw new CloudRuntimeException("Failed to add data store: " +
e.getMessage(), e);
}
+ // Validate secondary storage mount immediately using SSVM
+if (zoneId != null) {
+ List<HostVO> ssvmHosts = _hostDao.listByType(Host.Type.SecondaryStorageVM);
+ boolean mountSuccess = false;
+ String failureReason = "No Secondary Storage VM available to validate the
NFS mount.";
+
+ for (HostVO ssvm : ssvmHosts) {
+ if (ssvm.getDataCenterId() != zoneId.longValue()) {
+ continue;
+ }
+
+ try {
+ boolean result = _ssVmMgr.generateSetupCommand(ssvm.getId());
+ if (result) {
+ mountSuccess = true;
+ break;
+ } else {
+ failureReason = "Secondary Storage VM failed to mount the NFS
secondary storage.";
+ }
+ } catch (Exception e) {
+ failureReason = e.getMessage();
+ }
+ }
+
+ if (!mountSuccess) {
+ // cleanup created store
+ _imageStoreDao.remove(store.getId());
+ throw new CloudRuntimeException("Invalid secondary storage mount: " +
failureReason);
+ }
+}
+
Review Comment:
SSVM selection uses _hostDao.listByType(SecondaryStorageVM) and only filters
by zone ID. This includes hosts in non-Up states and can result in false
failures (or an unhelpful "invalid mount" error) when the zone has no
Up/Connecting SSVM yet. Prefer using
SecondaryStorageVmManager.listUpAndConnectingSecondaryStorageVmHost(zoneId) (or
equivalent) and consider how to handle zones with no available SSVM (e.g.,
start one, retry, or skip validation with a clearer error).
```suggestion
if (zoneId != null) {
List<HostVO> ssvmHosts =
_ssVmMgr.listUpAndConnectingSecondaryStorageVmHost(zoneId);
if (CollectionUtils.isEmpty(ssvmHosts)) {
if (logger.isInfoEnabled()) {
logger.info("Skipping immediate secondary storage mount
validation: no Up/Connecting Secondary Storage VM available in zone " + zoneId);
}
} else {
boolean mountSuccess = false;
String failureReason = "No Secondary Storage VM was able to
validate the NFS mount.";
for (HostVO ssvm : ssvmHosts) {
try {
boolean result =
_ssVmMgr.generateSetupCommand(ssvm.getId());
if (result) {
mountSuccess = true;
break;
} else {
failureReason = "Secondary Storage VM failed to
mount the NFS secondary storage.";
}
} catch (Exception e) {
failureReason = e.getMessage();
}
}
if (!mountSuccess) {
// cleanup created store
_imageStoreDao.remove(store.getId());
throw new CloudRuntimeException("Invalid secondary
storage mount: " + failureReason);
}
}
}
```
##########
server/src/main/java/com/cloud/storage/StorageManagerImpl.java:
##########
@@ -3990,6 +3993,37 @@ public ImageStore discoverImageStore(String name, String
url, String providerNam
throw new CloudRuntimeException("Failed to add data store: " +
e.getMessage(), e);
}
+ // Validate secondary storage mount immediately using SSVM
+if (zoneId != null) {
+ List<HostVO> ssvmHosts = _hostDao.listByType(Host.Type.SecondaryStorageVM);
+ boolean mountSuccess = false;
+ String failureReason = "No Secondary Storage VM available to validate the
NFS mount.";
+
+ for (HostVO ssvm : ssvmHosts) {
+ if (ssvm.getDataCenterId() != zoneId.longValue()) {
+ continue;
+ }
+
+ try {
+ boolean result = _ssVmMgr.generateSetupCommand(ssvm.getId());
+ if (result) {
+ mountSuccess = true;
+ break;
+ } else {
+ failureReason = "Secondary Storage VM failed to mount the NFS
secondary storage.";
+ }
+ } catch (Exception e) {
+ failureReason = e.getMessage();
+ }
+ }
+
+ if (!mountSuccess) {
+ // cleanup created store
+ _imageStoreDao.remove(store.getId());
+ throw new CloudRuntimeException("Invalid secondary storage mount: " +
failureReason);
+ }
+}
+
Review Comment:
The added validation block isn’t indented consistently with the surrounding
method body (the `if (zoneId != null)` starts at column 1). This is likely to
violate the project’s formatting/checkstyle rules and makes the method harder
to read; please align indentation with the surrounding code style.
```suggestion
if (zoneId != null) {
List<HostVO> ssvmHosts =
_hostDao.listByType(Host.Type.SecondaryStorageVM);
boolean mountSuccess = false;
String failureReason = "No Secondary Storage VM available to
validate the NFS mount.";
for (HostVO ssvm : ssvmHosts) {
if (ssvm.getDataCenterId() != zoneId.longValue()) {
continue;
}
try {
boolean result =
_ssVmMgr.generateSetupCommand(ssvm.getId());
if (result) {
mountSuccess = true;
break;
} else {
failureReason = "Secondary Storage VM failed to
mount the NFS secondary storage.";
}
} catch (Exception e) {
failureReason = e.getMessage();
}
}
if (!mountSuccess) {
// cleanup created store
_imageStoreDao.remove(store.getId());
throw new CloudRuntimeException("Invalid secondary storage
mount: " + failureReason);
}
}
```
##########
server/src/main/java/com/cloud/storage/StorageManagerImpl.java:
##########
@@ -3990,6 +3993,37 @@ public ImageStore discoverImageStore(String name, String
url, String providerNam
throw new CloudRuntimeException("Failed to add data store: " +
e.getMessage(), e);
}
+ // Validate secondary storage mount immediately using SSVM
+if (zoneId != null) {
+ List<HostVO> ssvmHosts = _hostDao.listByType(Host.Type.SecondaryStorageVM);
+ boolean mountSuccess = false;
+ String failureReason = "No Secondary Storage VM available to validate the
NFS mount.";
+
+ for (HostVO ssvm : ssvmHosts) {
+ if (ssvm.getDataCenterId() != zoneId.longValue()) {
+ continue;
+ }
+
+ try {
+ boolean result = _ssVmMgr.generateSetupCommand(ssvm.getId());
+ if (result) {
+ mountSuccess = true;
+ break;
+ } else {
+ failureReason = "Secondary Storage VM failed to mount the NFS
secondary storage.";
+ }
+ } catch (Exception e) {
+ failureReason = e.getMessage();
+ }
+ }
+
+ if (!mountSuccess) {
+ // cleanup created store
+ _imageStoreDao.remove(store.getId());
+ throw new CloudRuntimeException("Invalid secondary storage mount: " +
failureReason);
+ }
+}
+
Review Comment:
The new validation calls
SecondaryStorageVmManager.generateSetupCommand(ssvmHostId), but that method
programs *all* NFS image stores in the zone (iterates getImageStoresByScope and
returns false if any setup fails). This can cause adding a new image store to
fail due to an unrelated existing store problem, and it also doesn’t
specifically validate the just-created store. Consider sending a
setup/validation command targeted only at the newly initialized store (or
extending SecondaryStorageVmManager with a method that validates a specific
store/URL) so the result reflects only the store being added.
```suggestion
```
##########
server/src/main/java/com/cloud/storage/StorageManagerImpl.java:
##########
@@ -3990,6 +3993,37 @@ public ImageStore discoverImageStore(String name, String
url, String providerNam
throw new CloudRuntimeException("Failed to add data store: " +
e.getMessage(), e);
}
+ // Validate secondary storage mount immediately using SSVM
+if (zoneId != null) {
+ List<HostVO> ssvmHosts = _hostDao.listByType(Host.Type.SecondaryStorageVM);
+ boolean mountSuccess = false;
+ String failureReason = "No Secondary Storage VM available to validate the
NFS mount.";
+
+ for (HostVO ssvm : ssvmHosts) {
+ if (ssvm.getDataCenterId() != zoneId.longValue()) {
+ continue;
+ }
+
+ try {
+ boolean result = _ssVmMgr.generateSetupCommand(ssvm.getId());
+ if (result) {
+ mountSuccess = true;
+ break;
+ } else {
+ failureReason = "Secondary Storage VM failed to mount the NFS
secondary storage.";
+ }
+ } catch (Exception e) {
+ failureReason = e.getMessage();
+ }
+ }
+
+ if (!mountSuccess) {
+ // cleanup created store
+ _imageStoreDao.remove(store.getId());
+ throw new CloudRuntimeException("Invalid secondary storage mount: " +
failureReason);
+ }
+}
Review Comment:
This change introduces new behavior (fail-fast validation + rollback) in
discoverImageStore, but there’s no accompanying unit test coverage for the new
success/failure paths. Please add tests that mock SSVM host selection and
generateSetupCommand results to assert: (1) invalid NFS causes an exception and
the store is cleaned up, and (2) valid NFS proceeds without deleting the store.
##########
server/src/main/java/com/cloud/storage/StorageManagerImpl.java:
##########
@@ -3990,6 +3993,37 @@ public ImageStore discoverImageStore(String name, String
url, String providerNam
throw new CloudRuntimeException("Failed to add data store: " +
e.getMessage(), e);
}
+ // Validate secondary storage mount immediately using SSVM
+if (zoneId != null) {
+ List<HostVO> ssvmHosts = _hostDao.listByType(Host.Type.SecondaryStorageVM);
+ boolean mountSuccess = false;
+ String failureReason = "No Secondary Storage VM available to validate the
NFS mount.";
+
+ for (HostVO ssvm : ssvmHosts) {
+ if (ssvm.getDataCenterId() != zoneId.longValue()) {
+ continue;
+ }
+
+ try {
+ boolean result = _ssVmMgr.generateSetupCommand(ssvm.getId());
+ if (result) {
+ mountSuccess = true;
+ break;
+ } else {
+ failureReason = "Secondary Storage VM failed to mount the NFS
secondary storage.";
+ }
+ } catch (Exception e) {
+ failureReason = e.getMessage();
+ }
+ }
+
+ if (!mountSuccess) {
+ // cleanup created store
+ _imageStoreDao.remove(store.getId());
Review Comment:
Rollback on mount validation failure only removes the image_store record. In
this codebase, image store deletion also explicitly deletes image_store_details
and other related records because delete-cascade won’t run (see
deleteImageStore). This failure path should perform the same cleanup (ideally
in a transaction) or call the appropriate lifecycle/service delete so the DB
doesn’t retain orphaned details/refs when validation fails.
```suggestion
```
--
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]
