Skip to content

Commit 6f73cd0

Browse files
committed
addressed Copilot reviews
1 parent 642b20f commit 6f73cd0

5 files changed

Lines changed: 56 additions & 28 deletions

File tree

‎plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/NASBackupProviderTest.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -802,11 +802,11 @@ private void mockSourceVolume(String volUuid, String name) {
802802
Mockito.when(volumeDao.findByUuid(volUuid)).thenReturn(srcVolume);
803803
}
804804

805-
private void mockThinDiskOffering(Long diskOfferingId) {
805+
private void mockThinDiskOffering(String diskOfferingUuid, Long diskOfferingId) {
806806
DiskOfferingVO diskOffering = mock(DiskOfferingVO.class);
807807
Mockito.when(diskOffering.getId()).thenReturn(diskOfferingId);
808808
Mockito.when(diskOffering.getProvisioningType()).thenReturn(Storage.ProvisioningType.THIN);
809-
Mockito.when(diskOfferingDao.findByUuid(Mockito.anyString())).thenReturn(diskOffering);
809+
Mockito.when(diskOfferingDao.findByUuid(diskOfferingUuid)).thenReturn(diskOffering);
810810
}
811811

812812
private void mockStorPoolPoolByUuid(String dsUuid, Long poolId) {
@@ -877,7 +877,7 @@ public void restoreBackedUpVolumeUsesQcow2FormatAndRealPathForStorPool()
877877
String realStorPoolPath = "/dev/storpool-byid/t.t.t";
878878

879879
mockSourceVolume(volUuid, "data-sp");
880-
mockThinDiskOffering(6L);
880+
mockThinDiskOffering("disk-offering-uuid", 6L);
881881
mockStorPoolPoolByUuid(dsUuid, poolId);
882882
mockHostByIp(hostIp, 8L);
883883
mockNasRepository(backupOfferingId);

‎plugins/storage/volume/storpool/src/main/java/com/cloud/hypervisor/kvm/storage/StorPoolStorageAdaptor.java‎

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -481,6 +481,10 @@ public KVMPhysicalDisk createPhysicalDisk(String name, KVMStoragePool pool, Phys
481481
throw new CloudRuntimeException("Encrypted StorPool volumes are not supported");
482482
}
483483

484+
if (!PhysicalDiskFormat.RAW.equals(format)) {
485+
throw new CloudRuntimeException(String.format("StorPool volumes can only be created as RAW, %s was requested", format));
486+
}
487+
484488
if (size <= 0) {
485489
throw new CloudRuntimeException("Size must be greater than 0 to create a StorPool volume");
486490
}
@@ -507,9 +511,19 @@ public KVMPhysicalDisk createPhysicalDisk(String name, KVMStoragePool pool, Phys
507511

508512
String volumePath = StorPoolUtil.devPath(globalId);
509513
try {
510-
attachOrDetachVolume("attach", "volume", volumePath);
514+
if (!attachOrDetachVolume("attach", "volume", volumePath)) {
515+
// The attach succeeded but the device never showed up; StorPool refuses to delete an attached volume.
516+
attachOrDetachVolume("detach", "volume", volumePath);
517+
throw new CloudRuntimeException(String.format("StorPool volume %s for %s was attached but its device did not appear", volumePath, name));
518+
}
511519
} catch (CloudRuntimeException e) {
512-
volumeDelete("~" + globalId);
520+
try {
521+
volumeDelete("~" + globalId);
522+
} catch (CloudRuntimeException deleteEx) {
523+
SP_LOG("Failed to delete StorPool volume ~%s for %s after a failed attach: %s", globalId, name, deleteEx.getMessage());
524+
LOGGER.warn(String.format("Failed to delete StorPool volume ~%s for %s after a failed attach, it must be deleted manually: %s",
525+
globalId, name, deleteEx.getMessage()));
526+
}
513527
throw e;
514528
}
515529

‎scripts/vm/hypervisor/kvm/nasbackup.sh‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -566,8 +566,6 @@ print(gid)
566566
echo "$clone_path"
567567
}
568568

569-
# Deletes a volume previously returned by sp_create_backup_source_disk. Best-effort:
570-
# logs on failure rather than aborting the backup, matching the wrapper's old cleanup semantics.
571569
sp_delete_backup_source_disk() {
572570
local clone_name="$1"
573571
log -ne "StorPool: releasing backup source volume $clone_name"
@@ -582,7 +580,7 @@ sp_cleanup_backup_source_disks() {
582580
for clone_name in "${SP_CLEANUP_VOLUMES[@]:-}"; do
583581
[[ -z "$clone_name" ]] && continue
584582
log -ne "StorPool: cleaning up backup source volume $clone_name left over on script exit"
585-
sp_delete_backup_source_disk "$clone_name"
583+
sp_delete_backup_source_disk "$clone_name" || true
586584
done
587585
SP_CLEANUP_VOLUMES=()
588586
}
@@ -926,6 +924,11 @@ backup_stopped_vm() {
926924

927925
local -a disk_arr=()
928926
IFS=',' read -ra disk_arr <<< "$DISK_PATHS"
927+
if [[ ${#disk_arr[@]} -eq 0 ]]; then
928+
echo "No disk paths given for stopped VM $VM"
929+
cleanup
930+
exit 1
931+
fi
929932

930933
name="root"
931934
for disk in "${disk_arr[@]}"; do
@@ -951,7 +954,12 @@ backup_stopped_vm() {
951954
cleanup
952955
exit 1
953956
fi
954-
sp_clone_name=$(sp_volume_name_from_path "$read_disk")
957+
if ! sp_clone_name=$(sp_volume_name_from_path "$read_disk"); then
958+
log -ne "Failed to resolve the StorPool volume name of backup source volume $read_disk, it must be deleted manually"
959+
echo "Failed to resolve the StorPool volume name of backup source volume $read_disk"
960+
cleanup
961+
exit 1
962+
fi
955963
# Register the clone here, in the parent shell, right after the command substitution
956964
# above returns — sp_create_backup_source_disk cannot do this itself (see its comment).
957965
SP_CLEANUP_VOLUMES+=("$sp_clone_name")

‎test/integration/plugins/storpool/TestNasBackupStorPool.py‎

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -28,10 +28,8 @@
2828

2929
from marvin.cloudstackTestCase import cloudstackTestCase
3030
from marvin.codes import FAILED, KVM
31-
from marvin.lib.base import (Account,
32-
Backup,
31+
from marvin.lib.base import (Backup,
3332
BackupOffering,
34-
BackupRepository,
3533
DiskOffering,
3634
SecurityGroup,
3735
ServiceOffering,
@@ -89,7 +87,8 @@ def _setup_common(cls):
8987
StorPoolHelper.logger = cls
9088

9189
zone_cfg = config.zones[0]
92-
assert zone_cfg is not None
90+
if zone_cfg is None:
91+
raise Exception("No zone configured in the Marvin config")
9392
cls.zone_cfg = zone_cfg
9493

9594
testClient = super(TestNasBackupStorPool, cls).getClsTestClient()
@@ -106,8 +105,10 @@ def _setup_common(cls):
106105

107106
cls.services = testClient.getParsedTestDataConfig()
108107
cls.domain = get_domain(cls.apiclient)
109-
cls.zone = list_zones(cls.apiclient, name=zone_cfg.name)[0]
110-
assert cls.zone is not None
108+
zones = list_zones(cls.apiclient, name=zone_cfg.name)
109+
if not zones:
110+
raise Exception("Zone %s not found" % zone_cfg.name)
111+
cls.zone = zones[0]
111112
cls.services["mode"] = cls.zone.networktype
112113

113114
cls.hypervisor = get_hypervisor_type(cls.apiclient)
@@ -127,7 +128,7 @@ def _setup_common(cls):
127128

128129
template = get_template(cls.apiclient, cls.zone.id, account="system")
129130
if template == FAILED:
130-
assert False, "get_template() failed to return a system template"
131+
raise Exception("get_template() failed to return a system template")
131132
cls.template = template
132133

133134
offerings = BackupOffering.listByZone(cls.apiclient, cls.zone.id) or []
@@ -202,7 +203,7 @@ def _setup_common(cls):
202203
@classmethod
203204
def _get_or_create_disk_offering(cls, tag, displaytext):
204205
disk_offerings = list_disk_offering(cls.apiclient, name=tag)
205-
if disk_offerings is None:
206+
if not disk_offerings:
206207
disk_offering = DiskOffering.create(
207208
cls.apiclient,
208209
{
@@ -237,7 +238,8 @@ def _create_test_vm(cls, name, diskofferingid):
237238
@classmethod
238239
def _verify_setup_volumes(cls, vm):
239240
volumes = Volume.list(cls.apiclient, virtualmachineid=vm.id, listall=True)
240-
assert isinstance(volumes, list) and len(volumes) >= 1
241+
if not isinstance(volumes, list) or not volumes:
242+
raise Exception("No volumes found for VM %s" % vm.id)
241243
for vol in volumes:
242244
if cls.nfs_storage_pool and vol.storageid == cls.nfs_storage_pool.id:
243245
continue
@@ -678,7 +680,7 @@ def _setup_nfs_environment(cls):
678680
@classmethod
679681
def _get_or_create_nfs_service_offering(cls):
680682
service_offerings = list_service_offering(cls.apiclient, name=cls.nfs_tag)
681-
if service_offerings is None:
683+
if not service_offerings:
682684
service_offering = ServiceOffering.create(
683685
cls.apiclient,
684686
{

‎test/integration/plugins/storpool/sp_util.py‎

Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@
2121
from marvin.lib.base import (Account,
2222
Cluster,
2323
Configurations,
24-
ImageStore,
2524
ServiceOffering,
2625
Snapshot,
2726
StoragePool,
@@ -66,6 +65,7 @@
6665
import subprocess
6766
import json
6867
from storpool import spapi
68+
from storpool.spapi import ApiError
6969
from storpool import sptypes
7070

7171
class TestData():
@@ -696,7 +696,7 @@ def get_storpool_volume_by_path(cls, spapi, path):
696696
name = "~" + parts[3]
697697
try:
698698
volumes = spapi.volumeList(volumeName=name)
699-
except Exception as err:
699+
except ApiError as err:
700700
raise Exception("StorPool volume not found for path %s: %s" % (path, err))
701701
if not volumes:
702702
raise Exception("StorPool volume not found for path %s" % path)
@@ -706,20 +706,24 @@ def get_storpool_volume_by_path(cls, spapi, path):
706706
def check_storpool_volume_size(cls, volume, spapi):
707707
spvolume = cls.get_storpool_volume_by_path(spapi, volume.path)
708708
if spvolume.size != volume.size:
709-
raise Exception("Storpool volume size is not the same as CloudStack db size")
709+
raise Exception("StorPool volume size is not the same as CloudStack db size")
710710

711711
@classmethod
712712
def check_storpool_volume_iops(cls, spapi, volume,):
713-
spvolume = cls.get_storpool_volume_by_path(spapi, volume.path)
714-
logging.debug(spvolume.iops)
715-
logging.debug(volume.maxiops)
716-
if spvolume.iops != volume.maxiops:
717-
raise Exception("Storpool volume size is not the same as CloudStack db size")
713+
name = volume.path.split("/")[3]
714+
try:
715+
spvolume = spapi.volumeList(volumeName = "~" + name)
716+
logging.debug(spvolume[0].iops)
717+
logging.debug(volume.maxiops)
718+
if spvolume[0].iops != volume.maxiops:
719+
raise Exception("Storpool volume size is not the same as CloudStack db size")
720+
except spapi.ApiError as err:
721+
raise Exception(err)
718722

719723
@classmethod
720724
def verify_storpool_volume(cls, spapi, volume, check_size=True):
721725
"""Assert the CloudStack volume exists on StorPool; optionally compare size."""
722-
if not volume.path or not volume.path.startswith("/dev/storpool"):
726+
if not volume.path or not volume.path.startswith("/dev/storpool-byid/"):
723727
raise Exception("Volume %s does not have a StorPool device path: %s" %
724728
(volume.id, volume.path))
725729
spvolume = cls.get_storpool_volume_by_path(spapi, volume.path)

0 commit comments

Comments
 (0)