Copilot commented on code in PR #13193:
URL: https://github.com/apache/cloudstack/pull/13193#discussion_r3278131520
##########
server/src/main/java/com/cloud/storage/StorageManagerImpl.java:
##########
@@ -1301,15 +1305,89 @@ public PrimaryDataStoreInfo
updateStoragePool(UpdateStoragePoolCmd cmd) throws I
}
if (cmd.getUrl() != null) {
details.put("url", cmd.getUrl());
+
+ // Updating host/path/port and propagating the remount to
agents is only
+ // supported for NFS and Gluster pools. For other types of
storage pools, the URL is just informational and won't be used for actual
connection, so we don't need to parse and propagate it.
+ StoragePoolType poolType = storagePool.getPoolType();
+ if (poolType == StoragePoolType.NetworkFilesystem ||
poolType == StoragePoolType.Gluster) {
+ if (!storagePool.isInMaintenance()) {
+ throw new InvalidParameterValueException("Storage
pool must be in Maintenance state before its URL can be changed. " +
+ "Please put the pool into maintenance
first.");
+ }
+ URI newUri;
+ try {
+ newUri = new URI(cmd.getUrl());
+ } catch (URISyntaxException e) {
+ throw new InvalidParameterValueException("Invalid
URL format: " + cmd.getUrl());
+ }
+ storagePool.setHostAddress(newUri.getHost());
+ storagePool.setPath(newUri.getPath());
+ if (newUri.getPort() != -1) {
+ storagePool.setPort(newUri.getPort());
+ }
+ }
}
_storagePoolDao.update(id, storagePool);
_storagePoolDao.updateDetails(id, details);
+ ((PrimaryDataStoreLifeCycle)
dataStoreLifeCycle).updateStoragePool(storagePool, details);
+ StoragePoolType poolType = storagePool.getPoolType();
+ if (cmd.getUrl() != null &&
+ (poolType == StoragePoolType.NetworkFilesystem ||
poolType == StoragePoolType.Gluster)) {
+ propagateStoragePoolUrlChangeToHosts(storagePool);
+ }
}
}
return (PrimaryDataStoreInfo)_dataStoreMgr.getDataStore(pool.getId(),
DataStoreRole.Primary);
}
+ /**
+ * Propagates a storage pool URL change to all currently connected hosts
by sending
+ * a {@link ModifyStoragePoolCommand} with the updated connection details.
+ * This is called after the pool URL has been persisted to the DB while
the pool is
+ * in Maintenance state (so no VMs are actively using it).
+ *
+ * @param updatedPool the {@link StoragePoolVO} whose
hostAddress/path/port have already been updated
+ */
+ private void propagateStoragePoolUrlChangeToHosts(StoragePoolVO
updatedPool) {
+ List<Long> poolIds = List.of(updatedPool.getId());
+ List<Long> connectedHostIds =
_storagePoolHostDao.findHostsConnectedToPools(poolIds);
+ if (connectedHostIds.isEmpty()) {
+ logger.debug("No connected hosts found for storage pool [{}];
nothing to propagate for URL change.", updatedPool.getName());
+ return;
+ }
+
+ // Fetch fresh DataStore so that StoragePool delegates
(getHostAddress, getPath, getPort)
+ // return the newly saved values.
+ StoragePool freshPool = (StoragePool)
_dataStoreMgr.getDataStore(updatedPool.getId(), DataStoreRole.Primary);
+
+ Pair<Map<String, String>, Boolean> nfsMountOpts =
getStoragePoolNFSMountOpts(freshPool, null);
+ Map<String, String> mountDetails = nfsMountOpts != null ?
nfsMountOpts.first() : new java.util.HashMap<>();
+
+ logger.info("Propagating new URL for storage pool [{}] to {} connected
host(s).", updatedPool.getName(), connectedHostIds.size());
+
+ for (Long hostId : connectedHostIds) {
+ HostVO host = _hostDao.findById(hostId);
+ if (host == null) {
+ logger.warn("Host [{}] not found; skipping URL propagation for
pool [{}].", hostId, updatedPool.getName());
+ continue;
+ }
+
+ ModifyStoragePoolCommand cmd = new ModifyStoragePoolCommand(true,
freshPool, mountDetails);
+ Answer answer = _agentMgr.easySend(hostId, cmd);
+
+ if (answer == null || !answer.getResult()) {
+ String detail = (answer == null) ? "no answer returned" :
answer.getDetails();
+ logger.warn("Failed to propagate new URL for storage pool [{}]
to host [{}]: {}",
+ updatedPool.getName(), host.getName(), detail);
+ } else {
Review Comment:
`propagateStoragePoolUrlChangeToHosts` logs and continues when a host fails
to apply the new URL. This can leave the pool URL changed in the DB while some
connected hosts still have the old mount, creating inconsistent state that is
hard to detect. Consider failing the API call (or at least returning a
warning/marking the pool for manual remediation) when any propagation attempt
fails, so operators know the change was only partially applied.
##########
server/src/main/java/com/cloud/storage/StorageManagerImpl.java:
##########
@@ -1301,15 +1305,89 @@ public PrimaryDataStoreInfo
updateStoragePool(UpdateStoragePoolCmd cmd) throws I
}
if (cmd.getUrl() != null) {
details.put("url", cmd.getUrl());
+
+ // Updating host/path/port and propagating the remount to
agents is only
+ // supported for NFS and Gluster pools. For other types of
storage pools, the URL is just informational and won't be used for actual
connection, so we don't need to parse and propagate it.
+ StoragePoolType poolType = storagePool.getPoolType();
+ if (poolType == StoragePoolType.NetworkFilesystem ||
poolType == StoragePoolType.Gluster) {
+ if (!storagePool.isInMaintenance()) {
+ throw new InvalidParameterValueException("Storage
pool must be in Maintenance state before its URL can be changed. " +
+ "Please put the pool into maintenance
first.");
+ }
+ URI newUri;
+ try {
+ newUri = new URI(cmd.getUrl());
+ } catch (URISyntaxException e) {
+ throw new InvalidParameterValueException("Invalid
URL format: " + cmd.getUrl());
+ }
+ storagePool.setHostAddress(newUri.getHost());
+ storagePool.setPath(newUri.getPath());
Review Comment:
URL updates for NFS/Gluster pools are parsed with `new URI(cmd.getUrl())`,
but this bypasses the existing `extractUriParamsAsMap`/`UriUtils.getUriInfo`
validation/encoding used elsewhere in this class (e.g. host/path non-blank
checks and proper URI encoding). This can accept invalid URLs (host/path null)
or reject previously accepted URLs with characters needing encoding. Consider
reusing the existing URI parsing/validation helper here and explicitly validate
scheme/host/path (and normalize path) before persisting and propagating.
##########
api/src/main/java/org/apache/cloudstack/api/command/admin/storage/UpdateStoragePoolCmd.java:
##########
@@ -73,7 +73,12 @@ public class UpdateStoragePoolCmd extends BaseCmd {
@Parameter(name = ApiConstants.URL,
type = CommandType.STRING,
required = false,
- description = "the URL of the storage pool",
+ description = "the URL of the storage pool.
Supported only for NFS and Gluster pool type." +
+ "The pool must be in Maintenance mode
before changing the URL. WARNING: use this parameter" +
+ "with caution. It is intended for failover
scenarios where the storage content is already " +
+ "fully mirrored at the new location.
Pointing to a new location without ensuring complete " +
+ "data parity will result in data loss or
corruption. After the URL is updated, the new mount" +
+ "is applied to all connected hosts or
restart the Management server",
Review Comment:
The URL parameter description is built by concatenating string literals
without spaces at the join points (e.g. "type." + "The pool...", "parameter" +
"with caution", "new mount" + "is applied..."). This will render as merged
words in API docs/help output. Add the missing spaces/punctuation so the
description reads correctly.
##########
server/src/main/java/com/cloud/storage/StorageManagerImpl.java:
##########
@@ -4129,18 +4207,25 @@ public ImageStore migrateToObjectStore(String name,
String url, String providerN
@Override
public ImageStore updateImageStore(UpdateImageStoreCmd cmd) {
- return updateImageStoreStatus(cmd.getId(), cmd.getName(),
cmd.getReadonly(), cmd.getCapacityBytes());
+ return updateImageStoreStatus(cmd.getId(), cmd.getName(),
cmd.getReadonly(), cmd.getCapacityBytes(), cmd.getUrl());
}
@Override
@ActionEvent(eventType = EventTypes.EVENT_UPDATE_IMAGE_STORE_ACCESS_STATE,
eventDescription = "image store access updated")
- public ImageStore updateImageStoreStatus(Long id, String name, Boolean
readonly, Long capacityBytes) {
+ public ImageStore updateImageStoreStatus(Long id, String name, Boolean
readonly, Long capacityBytes, String url) {
// Input validation
ImageStoreVO imageStoreVO = _imageStoreDao.findById(id);
if (imageStoreVO == null) {
throw new IllegalArgumentException("Unable to find image store
with ID: " + id);
}
+ if (url != null) {
+ if (!imageStoreVO.isReadonly()) {
+ throw new InvalidParameterValueException("Image store must be
set to read-only (maintenance) state before its URL can be changed. " +
+ "Please set readOnly=true on the image store first.");
+ }
+ imageStoreVO.setUrl(url);
+ }
Review Comment:
`updateImageStoreStatus` sets the new URL without validating it against the
store's existing protocol/provider expectations (e.g. `ImageStoreVO.protocol`)
or even basic host/path presence. A malformed or mismatched URL can be
persisted and later break SSVM mounts or other components using the URL.
Consider validating the URL similarly to existing secondary storage update
logic (scheme/protocol match, host/path non-empty, URI encoding) before
persisting.
##########
server/src/main/java/com/cloud/storage/StorageManagerImpl.java:
##########
@@ -1301,15 +1305,89 @@ public PrimaryDataStoreInfo
updateStoragePool(UpdateStoragePoolCmd cmd) throws I
}
if (cmd.getUrl() != null) {
details.put("url", cmd.getUrl());
+
+ // Updating host/path/port and propagating the remount to
agents is only
+ // supported for NFS and Gluster pools. For other types of
storage pools, the URL is just informational and won't be used for actual
connection, so we don't need to parse and propagate it.
+ StoragePoolType poolType = storagePool.getPoolType();
+ if (poolType == StoragePoolType.NetworkFilesystem ||
poolType == StoragePoolType.Gluster) {
+ if (!storagePool.isInMaintenance()) {
+ throw new InvalidParameterValueException("Storage
pool must be in Maintenance state before its URL can be changed. " +
+ "Please put the pool into maintenance
first.");
+ }
+ URI newUri;
+ try {
+ newUri = new URI(cmd.getUrl());
+ } catch (URISyntaxException e) {
+ throw new InvalidParameterValueException("Invalid
URL format: " + cmd.getUrl());
+ }
+ storagePool.setHostAddress(newUri.getHost());
+ storagePool.setPath(newUri.getPath());
+ if (newUri.getPort() != -1) {
+ storagePool.setPort(newUri.getPort());
+ }
+ }
}
_storagePoolDao.update(id, storagePool);
_storagePoolDao.updateDetails(id, details);
+ ((PrimaryDataStoreLifeCycle)
dataStoreLifeCycle).updateStoragePool(storagePool, details);
+ StoragePoolType poolType = storagePool.getPoolType();
+ if (cmd.getUrl() != null &&
+ (poolType == StoragePoolType.NetworkFilesystem ||
poolType == StoragePoolType.Gluster)) {
+ propagateStoragePoolUrlChangeToHosts(storagePool);
Review Comment:
New behavior for updating primary storage URLs (maintenance-state
requirement, URI parsing, and host propagation via ModifyStoragePoolCommand) is
introduced here, but there are no corresponding unit tests in
`server/src/test/java/com/cloud/storage/StorageManagerImplTest.java` covering
the success and failure paths (e.g. non-maintenance -> exception, invalid URL
-> exception, connected-host propagation invoked, empty-host list -> no
propagation). Adding tests would help prevent regressions in this high-risk
operational workflow.
##########
server/src/main/java/com/cloud/storage/StorageManagerImpl.java:
##########
@@ -1301,15 +1305,89 @@ public PrimaryDataStoreInfo
updateStoragePool(UpdateStoragePoolCmd cmd) throws I
}
if (cmd.getUrl() != null) {
details.put("url", cmd.getUrl());
+
+ // Updating host/path/port and propagating the remount to
agents is only
+ // supported for NFS and Gluster pools. For other types of
storage pools, the URL is just informational and won't be used for actual
connection, so we don't need to parse and propagate it.
+ StoragePoolType poolType = storagePool.getPoolType();
+ if (poolType == StoragePoolType.NetworkFilesystem ||
poolType == StoragePoolType.Gluster) {
+ if (!storagePool.isInMaintenance()) {
+ throw new InvalidParameterValueException("Storage
pool must be in Maintenance state before its URL can be changed. " +
+ "Please put the pool into maintenance
first.");
+ }
+ URI newUri;
+ try {
+ newUri = new URI(cmd.getUrl());
+ } catch (URISyntaxException e) {
+ throw new InvalidParameterValueException("Invalid
URL format: " + cmd.getUrl());
+ }
+ storagePool.setHostAddress(newUri.getHost());
+ storagePool.setPath(newUri.getPath());
+ if (newUri.getPort() != -1) {
+ storagePool.setPort(newUri.getPort());
+ }
Review Comment:
When changing a pool URL, `storagePool.setPort(...)` is only called if the
new URI explicitly includes a port; if the old URL had a port and the new one
omits it, the previous port value will be retained in the DB and sent to
agents. For NFS/Gluster this can result in propagating an incorrect port.
Consider resetting the port to the default/"unset" value (e.g. 0/-1 per
existing conventions) when `newUri.getPort()` is -1.
##########
server/src/main/java/com/cloud/storage/StorageManagerImpl.java:
##########
@@ -4129,18 +4207,25 @@ public ImageStore migrateToObjectStore(String name,
String url, String providerN
@Override
public ImageStore updateImageStore(UpdateImageStoreCmd cmd) {
- return updateImageStoreStatus(cmd.getId(), cmd.getName(),
cmd.getReadonly(), cmd.getCapacityBytes());
+ return updateImageStoreStatus(cmd.getId(), cmd.getName(),
cmd.getReadonly(), cmd.getCapacityBytes(), cmd.getUrl());
}
@Override
@ActionEvent(eventType = EventTypes.EVENT_UPDATE_IMAGE_STORE_ACCESS_STATE,
eventDescription = "image store access updated")
- public ImageStore updateImageStoreStatus(Long id, String name, Boolean
readonly, Long capacityBytes) {
+ public ImageStore updateImageStoreStatus(Long id, String name, Boolean
readonly, Long capacityBytes, String url) {
// Input validation
ImageStoreVO imageStoreVO = _imageStoreDao.findById(id);
if (imageStoreVO == null) {
throw new IllegalArgumentException("Unable to find image store
with ID: " + id);
}
+ if (url != null) {
+ if (!imageStoreVO.isReadonly()) {
+ throw new InvalidParameterValueException("Image store must be
set to read-only (maintenance) state before its URL can be changed. " +
+ "Please set readOnly=true on the image store first.");
+ }
+ imageStoreVO.setUrl(url);
Review Comment:
The read-only precondition for changing an image store URL checks only the
current DB value (`imageStoreVO.isReadonly()`). This rejects a request that
sets `readOnly=true` and `url=...` in the same API call, even though the end
state would be valid. Consider checking the effective target state (e.g. allow
the change if `readonly == Boolean.TRUE` or the store is already read-only) and
apply `readonly` before validating URL updates.
##########
server/src/main/java/com/cloud/storage/StorageManagerImpl.java:
##########
@@ -4129,18 +4207,25 @@ public ImageStore migrateToObjectStore(String name,
String url, String providerN
@Override
public ImageStore updateImageStore(UpdateImageStoreCmd cmd) {
- return updateImageStoreStatus(cmd.getId(), cmd.getName(),
cmd.getReadonly(), cmd.getCapacityBytes());
+ return updateImageStoreStatus(cmd.getId(), cmd.getName(),
cmd.getReadonly(), cmd.getCapacityBytes(), cmd.getUrl());
}
@Override
@ActionEvent(eventType = EventTypes.EVENT_UPDATE_IMAGE_STORE_ACCESS_STATE,
eventDescription = "image store access updated")
- public ImageStore updateImageStoreStatus(Long id, String name, Boolean
readonly, Long capacityBytes) {
+ public ImageStore updateImageStoreStatus(Long id, String name, Boolean
readonly, Long capacityBytes, String url) {
// Input validation
ImageStoreVO imageStoreVO = _imageStoreDao.findById(id);
if (imageStoreVO == null) {
throw new IllegalArgumentException("Unable to find image store
with ID: " + id);
}
+ if (url != null) {
+ if (!imageStoreVO.isReadonly()) {
+ throw new InvalidParameterValueException("Image store must be
set to read-only (maintenance) state before its URL can be changed. " +
+ "Please set readOnly=true on the image store first.");
+ }
Review Comment:
The new `url` update path for image stores (read-only enforcement + URL
persistence) is not covered by unit tests. Since `StorageManagerImplTest`
already exists, consider adding tests for: rejecting URL change when not
read-only, allowing when already read-only, and allowing when `readonly=true`
is provided in the same request as `url` (once fixed), plus URL validation
behavior.
--
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]
