Copilot commented on code in PR #14203:
URL: https://github.com/apache/cloudstack/pull/14203#discussion_r4322706696
##########
plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java:
##########
@@ -773,11 +801,18 @@ private Pair<Boolean, String>
restoreVMBackup(VirtualMachine vm, Backup backup)
private List<String> getBackupFiles(List<Backup.VolumeInfo> backedVolumes)
{
List<String> backupFiles = new ArrayList<>();
for (Backup.VolumeInfo backedVolume : backedVolumes) {
- backupFiles.add(backedVolume.getPath());
+ backupFiles.add(getBackupFileIdentifier(backedVolume.getPath()));
}
return backupFiles;
}
+ // nasbackup.sh names each qcow2 after the basename of the volume's disk
path. No-op for
+ // most drivers (bare volume path already); reduces StorPool's full device
path to match.
+ private String getBackupFileIdentifier(String volumePath) {
+ int idx = volumePath.lastIndexOf('/');
+ return idx >= 0 ? volumePath.substring(idx + 1) : volumePath;
+ }
Review Comment:
`getBackupFileIdentifier()` is applied unconditionally and will strip
everything before the last `/` even for non-filesystem identifiers that
legitimately contain `/` (e.g., RBD-style strings), potentially breaking
restore by changing the expected identifier. Restrict this normalization to
StorPool device paths (e.g. paths starting with `/dev/storpool-byid/`) or to
inputs that are known to be filesystem paths, and leave other volume
identifiers unchanged.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStoragePoolManager.java:
##########
@@ -443,6 +443,13 @@ public boolean disconnectPhysicalDisk(StoragePoolType
type, String poolUuid, Str
return adaptor.disconnectPhysicalDisk(volPath, pool);
}
+ public boolean cleanupCreatedPhysicalDisk(StoragePoolType type, String
poolUuid, String volPath) {
+ StorageAdaptor adaptor = getStorageAdaptor(type);
+ KVMStoragePool pool = adaptor.getStoragePool(poolUuid);
+
+ return adaptor.cleanupCreatedPhysicalDisk(volPath, pool);
+ }
Review Comment:
`adaptor.getStoragePool(poolUuid)` can return `null`, and the current code
passes that through to `cleanupCreatedPhysicalDisk`, which may lead to a
NullPointerException (and potentially mask the original restore failure).
Handle the `null` pool case explicitly (log + return `false`) before calling
into the adaptor.
##########
plugins/storage/volume/storpool/src/main/java/com/cloud/hypervisor/kvm/storage/StorPoolStorageAdaptor.java:
##########
@@ -454,7 +474,53 @@ public boolean createFolder(String uuid, String path,
String localPath) {
@Override
public KVMPhysicalDisk createPhysicalDisk(String name, KVMStoragePool
pool, PhysicalDiskFormat format,
ProvisioningType provisioningType, long size, byte[] passphrase) {
- return null;
+ SP_LOG("StorPoolStorageAdaptor.createPhysicalDisk: name=%s, pool=%s,
size=%d", name, pool.getUuid(), size);
Review Comment:
New host-side behaviors were added for provisioning (`createPhysicalDisk`)
and best-effort cleanup (`cleanupCreatedPhysicalDisk`), but there’s no
corresponding unit coverage here (unlike the wrapper-level tests added
elsewhere). Add tests that validate: (1) RAW-only enforcement and size checks,
(2) that a successfully created disk is attached and returns a correct
`/dev/storpool-byid/<gid>` path, and (3) that cleanup disconnects and deletes
(and returns `false` on non-StorPool paths).
##########
plugins/storage/volume/storpool/src/main/java/com/cloud/hypervisor/kvm/storage/StorPoolStorageAdaptor.java:
##########
@@ -327,6 +327,26 @@ public boolean deletePhysicalDisk(String volumeUuid,
KVMStoragePool pool, Storag
return true; // apparently ignored
}
+ @Override
+ public boolean cleanupCreatedPhysicalDisk(String volumePath,
KVMStoragePool pool) {
+ SP_LOG("StorPoolStorageAdaptor.cleanupCreatedPhysicalDisk: path=%s,
pool=%s", volumePath, pool);
+ final String name = getVolumeNameFromPath(volumePath, true);
Review Comment:
New host-side behaviors were added for provisioning (`createPhysicalDisk`)
and best-effort cleanup (`cleanupCreatedPhysicalDisk`), but there’s no
corresponding unit coverage here (unlike the wrapper-level tests added
elsewhere). Add tests that validate: (1) RAW-only enforcement and size checks,
(2) that a successfully created disk is attached and returns a correct
`/dev/storpool-byid/<gid>` path, and (3) that cleanup disconnects and deletes
(and returns `false` on non-StorPool paths).
##########
test/integration/plugins/storpool/sp_util.py:
##########
@@ -682,14 +683,30 @@ def destroy_vm(self, apiclient, virtualmachineid):
apiclient.destroyVirtualMachine(cmd)
@classmethod
- def check_storpool_volume_size(cls, volume, spapi):
- name = volume.path.split("/")[3]
+ def get_storpool_volume_by_path(cls, spapi, path):
+ """
+ Resolve a CloudStack volume path (/dev/storpool-byid/<globalId>) to a
+ StorPool volume.
+ """
+ if not path:
+ raise Exception("Volume path is empty")
+ parts = path.rstrip("/").split("/")
+ if len(parts) < 4:
+ raise Exception("Unexpected StorPool volume path: %s" % path)
+ name = "~" + parts[3]
try:
- spvolume = spapi.volumeList(volumeName = "~" + name)
- if spvolume[0].size != volume.size:
- raise Exception("Storpool volume size is not the same as
CloudStack db size")
- except spapi.ApiError as err:
- raise Exception(err)
+ volumes = spapi.volumeList(volumeName=name)
+ except Exception as err:
+ raise Exception("StorPool volume not found for path %s: %s" %
(path, err))
+ if not volumes:
+ raise Exception("StorPool volume not found for path %s" % path)
+ return volumes[0]
Review Comment:
Catching `Exception` here will also swallow unexpected/non-API errors (e.g.,
programming errors) and rethrow them as “volume not found”, making failures
harder to diagnose. Prefer catching the StorPool client’s explicit API
exception type (e.g. `spapi.ApiError`) and letting other exceptions bubble up
(or rethrowing with a message that preserves the original meaning).
##########
test/integration/plugins/storpool/sp_util.py:
##########
@@ -682,14 +683,30 @@ def destroy_vm(self, apiclient, virtualmachineid):
apiclient.destroyVirtualMachine(cmd)
@classmethod
- def check_storpool_volume_size(cls, volume, spapi):
- name = volume.path.split("/")[3]
+ def get_storpool_volume_by_path(cls, spapi, path):
+ """
+ Resolve a CloudStack volume path (/dev/storpool-byid/<globalId>) to a
+ StorPool volume.
+ """
+ if not path:
+ raise Exception("Volume path is empty")
+ parts = path.rstrip("/").split("/")
+ if len(parts) < 4:
+ raise Exception("Unexpected StorPool volume path: %s" % path)
+ name = "~" + parts[3]
try:
- spvolume = spapi.volumeList(volumeName = "~" + name)
- if spvolume[0].size != volume.size:
- raise Exception("Storpool volume size is not the same as
CloudStack db size")
- except spapi.ApiError as err:
- raise Exception(err)
+ volumes = spapi.volumeList(volumeName=name)
+ except Exception as err:
+ raise Exception("StorPool volume not found for path %s: %s" %
(path, err))
+ if not volumes:
+ raise Exception("StorPool volume not found for path %s" % path)
+ return volumes[0]
+
+ @classmethod
+ def check_storpool_volume_size(cls, volume, spapi):
+ spvolume = cls.get_storpool_volume_by_path(spapi, volume.path)
+ if spvolume.size != volume.size:
+ raise Exception("Storpool volume size is not the same as
CloudStack db size")
Review Comment:
Correct capitalization in the error message: “Storpool” → “StorPool”.
--
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]
