Copilot commented on code in PR #13119:
URL: https://github.com/apache/cloudstack/pull/13119#discussion_r3202204355
##########
plugins/storage/volume/storpool/src/main/java/com/cloud/hypervisor/kvm/storage/StorPoolStoragePool.java:
##########
@@ -53,12 +54,13 @@ public class StorPoolStoragePool implements KVMStoragePool {
private String _localPath;
private String storageNodeId = getStorPoolConfigParam("SP_OURID");
- public StorPoolStoragePool(String uuid, String host, int port,
StoragePoolType storagePoolType, StorageAdaptor storageAdaptor) {
+ public StorPoolStoragePool(String uuid, String host, int port,
StoragePoolType storagePoolType, StorageAdaptor storageAdaptor, Map<String,
String> details) {
_uuid = uuid;
_sourceHost = host;
_sourcePort = port;
_storagePoolType = storagePoolType;
_storageAdaptor = storageAdaptor;
+ _authUsername = details.get(StorPoolUtil.SP_TEMPLATE);
Review Comment:
`details` is dereferenced without a null-check; if the storage pool is
created with `details == null` (or without `SP_TEMPLATE`), this will throw or
propagate a null template name to later flows. Consider defaulting `details` to
an empty map (or explicitly validating and failing fast with a clear message),
and providing a backward-compatible fallback for pre-migration pools if
applicable.
##########
plugins/storage/volume/storpool/src/main/java/com/cloud/hypervisor/kvm/storage/StorPoolStorageAdaptor.java:
##########
@@ -409,7 +409,7 @@ public KVMPhysicalDisk
createTemplateFromDirectDownloadFile(String templateFileP
srcFile = new QemuImgFile(srcTemplateFilePath, srcFileFormat);
- String spTemplate = destPool.getUuid().split(";")[0];
+ String spTemplate = destPool.getAuthUserName();
Review Comment:
`getAuthUserName()` is now being used to return the StorPool template name
(set in `StorPoolStoragePool`), which is semantically confusing and can easily
regress other call sites that expect an actual username. Prefer introducing an
explicit field/accessor for the StorPool template name (e.g.,
`getTemplateName()`), or retrieving it from a dedicated details structure, to
keep behavior clear and avoid cross-feature coupling.
##########
engine/schema/src/main/java/com/cloud/upgrade/dao/Upgrade42210to42300.java:
##########
@@ -51,6 +62,56 @@ public InputStream[] getPrepareScripts() {
@Override
public void performDataMigration(Connection conn) {
unhideJsInterpretationEnabled(conn);
+ normalizeStorPoolPrimaryStorageUuids();
+ }
+
+ protected PrimaryDataStoreDao getStorageDao() {
+ if (storageDao == null) {
+ storageDao = new PrimaryDataStoreDaoImpl();
+ }
+ return storageDao;
+ }
+
+ /**
+ * StorPool primary storage used {@code templateName + ";" + uuid} as
{@code storage_pool.uuid}.
+ * Normalize to a plain UUID form so API and validation treat {@code id}
like other pools.
+ * Template name remains in {@code storage_pool_details} ({@code
SP_TEMPLATE}).
+ */
+ protected void normalizeStorPoolPrimaryStorageUuids() {
+ SearchBuilder<StoragePoolVO> sb =
getStorageDao().createSearchBuilder();
+ sb.and("poolType", sb.entity().getPoolType(), SearchCriteria.Op.EQ);
+ sb.and("uuid", sb.entity().getUuid(), SearchCriteria.Op.LIKE);
+ sb.done();
+ SearchCriteria<StoragePoolVO> sc = sb.create();
+ sc.setParameters("poolType", StoragePoolType.StorPool);
+ sc.setParameters("uuid", "%;%");
+ List<StoragePoolVO> pools = getStorageDao().search(sc, null);
+ int updated = 0;
+ for (StoragePoolVO pool : pools) {
+ final String templatePrefixedPoolUuid = pool.getUuid();
+ if (templatePrefixedPoolUuid == null) {
+ continue;
+ }
+ final String[] parts = templatePrefixedPoolUuid.split(";");
+ if (parts.length < 2) {
+ continue;
+ }
+ final String realUuid = parts[1].trim();
Review Comment:
Parsing the legacy UUID with `split(\";\")` and taking `parts[1]` will
produce the wrong UUID if the StorPool template name itself contains `;` (since
the UUID is intended to be the suffix). A safer extraction is to take the
substring after the last `;` (e.g., using `lastIndexOf(';')`) so the migration
reliably normalizes all legacy values.
##########
engine/schema/src/main/java/com/cloud/upgrade/dao/Upgrade42210to42300.java:
##########
@@ -51,6 +62,56 @@ public InputStream[] getPrepareScripts() {
@Override
public void performDataMigration(Connection conn) {
unhideJsInterpretationEnabled(conn);
+ normalizeStorPoolPrimaryStorageUuids();
+ }
Review Comment:
`performDataMigration` receives a `Connection`, but
`normalizeStorPoolPrimaryStorageUuids()` uses a DAO that will likely open/use
its own transaction/connection. This can break expected upgrade transaction
boundaries and can behave differently depending on upgrade runtime wiring.
Prefer performing this migration using the provided `conn` (JDBC
`PreparedStatement`s) or otherwise ensuring the DAO uses the same upgrade
connection/transaction context.
--
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]
