Copilot commented on code in PR #11931:
URL: https://github.com/apache/cloudstack/pull/11931#discussion_r4275313436
##########
test/integration/smoke/test_direct_download.py:
##########
@@ -295,82 +292,36 @@ def deployVM(self, offering) :
self.cleanup.append(vm)
return vm
- @skipTestIf("nfsKvmNotAvailable")
+ @skipTestIf("storageKvmNotAvailable")
@attr(tags=["advanced", "basic", "eip", "advancedns", "sg"],
required_hardware="false")
- def test_01_deploy_vm_from_direct_download_template_nfs_storage(self):
- """Test Deploy VM from direct download template on NFS storage
+ def test_01_deploy_vm_from_direct_download_template(self):
+ """Test Deploy VM from direct download template on NFS, RBD, local
storage and shared mount point
"""
- # Create service offering for local storage using storage tags
- tags = self.getCurrentStoragePoolTags(self.nfsPoolId)
- test_tag = "marvin_test_nfs_storage_direct_download"
- self.updateStoragePoolTags(self.nfsPoolId, test_tag)
- nfs_storage_offering =
self.createServiceOffering("TestNFSStorageDirectDownload", "shared", test_tag)
-
- vm = self.deployVM(nfs_storage_offering)
- self.assertEqual(
- vm.state,
- "Running",
- "Check VM deployed from direct download template is running on NFS
storage"
- )
-
- # Revert storage tags for the storage pool used in this test
- self.updateStoragePoolTags(self.nfsPoolId, tags)
- self.cleanup.append(nfs_storage_offering)
- return
-
- @skipTestIf("localStorageKvmNotAvailable")
- @attr(tags=["advanced", "basic", "eip", "advancedns", "sg"],
required_hardware="false")
- def test_02_deploy_vm_from_direct_download_template_local_storage(self):
- """Test Deploy VM from direct download template on local storage
- """
-
- # Create service offering for local storage using storage tags
- tags = self.getCurrentStoragePoolTags(self.localPoolId)
- test_tag = "marvin_test_local_storage_direct_download"
- self.updateStoragePoolTags(self.localPoolId, test_tag)
- local_storage_offering =
self.createServiceOffering("TestLocalStorageDirectDownload", "local", test_tag)
-
- # Deploy VM
- vm = self.deployVM(local_storage_offering)
- self.assertEqual(
- vm.state,
- "Running",
- "Check VM deployed from direct download template is running on
local storage"
- )
-
- # Revert storage tags for the storage pool used in this test
- self.updateStoragePoolTags(self.localPoolId, tags)
- self.cleanup.append(local_storage_offering)
- return
-
- @skipTestIf("sharedMountPointKvmNotAvailable")
- @attr(tags=["advanced", "basic", "eip", "advancedns", "sg"],
required_hardware="false")
- def
test_03_deploy_vm_from_direct_download_template_shared_mount_point_storage(self):
- """Test Deploy VM from direct download template on shared mount point
- """
-
- # Create service offering for local storage using storage tags
- tags = self.getCurrentStoragePoolTags(self.sharedPoolId)
- test_tag = "marvin_test_shared_mount_point_storage_direct_download"
- self.updateStoragePoolTags(self.sharedPoolId, test_tag)
- shared_offering =
self.createServiceOffering("TestSharedMountPointStorageDirectDownload",
"shared", test_tag)
-
- # Deploy VM
- vm = VirtualMachine.create(
- self.apiclient,
- self.services["virtual_machine"],
- serviceofferingid=shared_offering.id,
- networkids=self.l2_network.id,
- )
- self.assertEqual(
- vm.state,
- "Running",
- "Check VM deployed from direct download template is running on
shared mount point"
- )
-
- # Revert storage tags for the storage pool used in this test
- self.updateStoragePoolTags(self.sharedPoolId, tags)
- self.cleanup.append(vm)
- self.cleanup.append(shared_offering)
- return
+ storages = []
+ for pool_type, pool_id in self.sharedPoolIds.items():
+ storages.append((pool_type, pool_id, "shared"))
+ if self.localStorageFound:
+ storages.append(("Filesystem", self.localPoolId, "local"))
+
+ for pool_type, pool_id, offering_type in storages:
+ self.debug("Deploying VM from direct download template on %s
storage" % pool_type)
+
+ # Create service offering for the storage using the storage pool
type as storage tag
+ tags = self.getCurrentStoragePoolTags(pool_id)
+ test_tag = pool_type.lower()
Review Comment:
Using `pool_type.lower()` as the temporary storage tag is very generic
(e.g., `rbd`, `networkfilesystem`, `filesystem`) and increases the chance of
colliding with existing environment tags or parallel test runs that also rely
on tags. Prefer a unique, test-scoped tag (e.g., prefix with
`marvin_test_direct_download_...` and optionally add a random suffix) while
still restoring the original tags in `finally`.
##########
test/integration/smoke/test_backup_recovery_nas.py:
##########
@@ -72,14 +113,22 @@ def setUpClass(cls):
cls._cleanup = [cls.account]
- # Create NAS backup repository and offering. Use the same directory as
the storage pool
+ # Create NAS backup repository and offering.
cls.backup_repository = BackupRepository.add(cls.api_client,
zoneid=cls.zone.id, name="Nas",
-
address=cls.storage_pool.ipaddress + ":" + cls.storage_pool.path,
+
address=cls.nas_repository_address,
provider="nas",
type="nfs",)
cls._cleanup.append(cls.backup_repository)
+ # Match the external offering to the repository just created above by
externalid
+ # (== the repository's own id for the nas provider) rather than
blindly taking
+ # index 0 -- a stray repository left over from an earlier interrupted
run (e.g.
+ # one whose backups didn't get cleaned up, so its own teardown
couldn't remove
+ # it either) sorts alongside the new one, and index 0 has no guarantee
of being
+ # the one this run owns.
cls.provider_offerings = BackupOffering.listExternal(cls.api_client,
cls.zone.id)
- cls.backup_offering = BackupOffering.importExisting(cls.api_client,
cls.zone.id, cls.provider_offerings[0].externalid,
-
cls.provider_offerings[0].name, cls.provider_offerings[0].description)
+ matching_offerings = [o for o in cls.provider_offerings if
o.externalid == cls.backup_repository.id]
+ provider_offering = matching_offerings[0] if matching_offerings else
cls.provider_offerings[0]
+ cls.backup_offering = BackupOffering.importExisting(cls.api_client,
cls.zone.id, provider_offering.externalid,
+
provider_offering.name, provider_offering.description)
Review Comment:
`BackupOffering.listExternal(...)` can return `None`/empty in some
environments or transiently; indexing `cls.provider_offerings[0]` will raise at
runtime. Add an explicit assertion/skip with a clear message when no external
offerings are returned, or poll/retry `listExternal` for a short period after
creating the repository before selecting the offering.
##########
test/integration/smoke/test_list_storage_pools.py:
##########
@@ -62,26 +62,31 @@ def test_01_list_storage_pools_clusterid_filter(self):
isinstance(storage_pools, list),
"Storage pool response type should be a list"
)
- self.assertGreater(
- len(storage_pools),
- 0,
- "Length of storage pools should greater than 0"
- )
+ # Filter may return zone-wide pools (with clusterid=None) alongside
cluster-scoped pools
+ cluster_scoped_found = False
for storage_pool in storage_pools:
- self.assertEqual(
- storage_pool.clusterid,
- self.cluster.id,
- "Cluster id should be equal to the cluster id passed in the
filter"
- )
+ if storage_pool.clusterid is not None:
+ # Only assert clusterid for cluster-scoped pools
+ cluster_scoped_found = True
+ self.assertEqual(
+ storage_pool.clusterid,
+ self.cluster.id,
+ "Cluster id should be equal to the cluster id passed in
the filter"
+ )
Review Comment:
`cluster_scoped_found` is set but never asserted/used, so the test can pass
even when the cluster filter returns only zone-wide pools (i.e., it becomes
non-validating). To preserve test value, either (a) `skipTest` when no
cluster-scoped pools exist in the environment, or (b) assert
`cluster_scoped_found` is true when `storage_pools` is non-empty and the
environment is expected to have cluster-scoped pools.
##########
test/integration/smoke/test_vm_life_cycle.py:
##########
@@ -1707,12 +1708,17 @@ def get_target_host(self, virtualmachineid):
return target_hosts[0]
- def get_target_pool(self, volid):
- target_pools = StoragePool.listForMigration(self.apiclient, id=volid)
+ def get_target_pool(self, vol):
+ target_pools = StoragePool.listForMigration(self.apiclient, id=vol.id)
if target_pools is None or len(target_pools) == 0:
self.skipTest("Not enough storage pools found for migration")
+ volume = Volume.list(self.apiclient, id=vol.id)[0]
+ source_pool = list_storage_pools(self.apiclient,
id=volume.storageid)[0]
+ if source_pool.type == 'RBD' and target_pools[0].type == 'RBD':
+ self.skipTest("Live VM migration between RBD pools is unsupported")
Review Comment:
The skip decision is based only on `target_pools[0]`. If the first candidate
happens to be an RBD pool but non-RBD candidates exist later in the list, the
test will be skipped even though a supported target is available. Consider
filtering `target_pools` to prefer a non-RBD target when the source is RBD, and
only skip if *all* candidates are RBD.
##########
test/integration/smoke/test_list_storage_pools.py:
##########
@@ -264,26 +269,31 @@ def test_07_list_storage_pools_podid_filter(self):
isinstance(storage_pools, list),
"Storage pool response type should be a list"
)
- self.assertGreater(
- len(storage_pools),
- 0,
- "Length of storage pools should greater than 0"
- )
+ # Filter may return zone-wide pools (with podid=None) alongside
pod-scoped pools
+ pod_scoped_found = False
for storage_pool in storage_pools:
- self.assertEqual(
- storage_pool.podid,
- self.cluster.podid,
- "Pod id should be equal to the pod id passed in the filter"
- )
+ if storage_pool.podid is not None:
+ # Only assert podid for pod-scoped pools
+ pod_scoped_found = True
+ self.assertEqual(
+ storage_pool.podid,
+ self.cluster.podid,
+ "Pod id should be equal to the pod id passed in the filter"
+ )
Review Comment:
Same issue as the clusterid filter test: `pod_scoped_found` is never
asserted/used, so the test can pass without validating the pod filter behavior.
Consider skipping when no pod-scoped pools are present, or assert that at least
one pod-scoped pool was returned for a valid `podid` filter.
##########
test/integration/smoke/test_volumes.py:
##########
@@ -1640,14 +1743,25 @@ def check_volume_encryption(self, virtual_machine,
volumes_count):
parser = etree.XMLParser(remove_blank_text=True)
virshxml_root = ET.fromstring(xml_as_str, parser=parser)
- encryption_format =
virshxml_root.findall(".devices/disk/encryption[@format='luks']")
- self.assertIsNotNone(encryption_format, "The volume encryption format
is not luks")
+ # RBD volumes are encrypted natively by librbd: <encryption
format='luks2' engine='librbd'>.
+ # Any other pool type keeps using qemu-native LUKS1: <encryption
format='luks'> (no engine).
+ encryption_format_value = 'luks2' if self.storage_pool_type == "RBD"
else 'luks'
+ encryption_format =
virshxml_root.findall(".devices/disk/encryption[@format='%s']" %
encryption_format_value)
+ self.assertIsNotNone(encryption_format, "The volume encryption format
is not %s" % encryption_format_value)
Review Comment:
`ElementTree.findall(...)` always returns a list (possibly empty), so
`assertIsNotNone(encryption_format, ...)` is ineffective and will never fail.
To make the failure mode clearer (and avoid relying on the later `len(...)`
assertion), assert that the list is non-empty (e.g., `assertGreater(len(...),
0)` / `assertTrue(encryption_format)`) and keep the more specific error message.
##########
test/integration/smoke/test_volumes.py:
##########
@@ -1599,10 +1609,90 @@ def test_03_root_and_data_volume_encryption(self):
self.check_volume_encryption(virtual_machine, 3)
+ # Resize the attached encrypted data volume, online then offline,
reusing this VM
+ # instead of provisioning a separate one just for resize coverage.
+ #
+ # Online (VM stays running): for RBD this is routed through virsh
blockresize; for
+ # any other pool type it goes through the same virsh blockresize
NOTIFYONLY path used
+ # for qemu-native LUKS.
+ disk_offering_encrypt_2gb = DiskOffering.create(
+ self.apiclient,
+ self.services["disk_offering"],
+ name="Encrypted-2GB",
+ disksize=2,
+ encrypt=True
+ )
+ self.cleanup.append(disk_offering_encrypt_2gb)
+
+ volume.resize(self.apiclient,
diskofferingid=disk_offering_encrypt_2gb.id)
+
+ # Online resize only grows the block device live; picking up the new
capacity in the
+ # guest still needs a rescan or reboot depending on the guest
OS/kernel (this template's
+ # CentOS 5.5 predates reliable virtio-blk live-capacity notification),
so verify success
+ # through the volume's own reported size here. Guest-visible size is
verified below after
+ # the offline resize, once the volume is freshly re-attached.
+ list_volume_response = Volume.list(self.apiclient, id=volume.id)
+ expected_raw_size = disk_offering_encrypt_2gb.disksize << 30
+ if self.storage_pool_type == "RBD" and
getattr(list_volume_response[0], "encryptformat", None):
+ expected_raw_size += (16 << 20)
+ self.assertEqual(
+ int(list_volume_response[0].size),
+ expected_raw_size,
+ "Check that the online resize grew the volume to the expected size"
+ )
+ self.assertEqual(list_volume_response[0].state, 'Ready', "Check that
the online-resized volume is Ready")
+
+ # Offline (volume detached first): for RBD this uses `rbd resize
+ # --encryption-passphrase-file`; for any other pool type it uses
qemu-img
+ # (LibvirtResizeVolumeCommandWrapper.resizeEncryptedQcowFile).
+ virtual_machine.detach_volume(self.apiclient, volume)
+
+ disk_offering_encrypt_3gb = DiskOffering.create(
+ self.apiclient,
+ self.services["disk_offering"],
+ name="Encrypted-3GB",
+ disksize=3,
+ encrypt=True
+ )
+ self.cleanup.append(disk_offering_encrypt_3gb)
+
+ volume.resize(self.apiclient,
diskofferingid=disk_offering_encrypt_3gb.id)
+
+ virtual_machine.attach_volume(self.apiclient, volume)
+ self.check_volume_encryption(virtual_machine, 3)
+
+ ssh = virtual_machine.get_ssh_client(reconnect=True)
+ list_volume_response = Volume.list(self.apiclient, id=volume.id)
+ vol_sz = self.get_expected_guest_visible_size(list_volume_response[0],
list_volume_response[0].size)
+ # After a detach + reattach, libvirt is not guaranteed to reuse the
same target device
+ # letter the volume had before (it may pick a fresh one), so the
offering's deviceid no
+ # longer reliably maps to /dev/vdX here. Match by the reported byte
count instead.
+ fdisk_output = ssh.execute("/sbin/fdisk -l | grep Disk")
+ self.debug("Offline resize - Volume Size Expected %s - guest disks:
%s" % (vol_sz, fdisk_output))
+ self.assertTrue(
+ any(" %s bytes" % vol_sz in line for line in fdisk_output),
+ "Check if the offline-resized encrypted volume size is available
in the guest"
+ )
+
virtual_machine.detach_volume(self.apiclient, volume)
- self.assertEqual(ret[0], SUCCESS, "Check if promised disk size
actually available")
time.sleep(self.services["sleep"])
+ def get_expected_guest_visible_size(self, volume, vol_sz):
+ """Some pool types/encryption mechanisms reserve overhead out of the
requested size, so the
+ guest sees fewer usable bytes than the volume's nominal size. Returns
the byte count the
+ guest should actually report for the given volume.
+ """
+ list_volume_pool_response = list_storage_pools(self.apiclient,
id=volume.storageid)
+ volume_pool = list_volume_pool_response[0]
+ vol_sz = int(vol_sz)
+ if volume_pool.type.lower() == "powerflex":
+ vol_sz = vol_sz - (128 << 20) - ((vol_sz >> 30) * 200704)
+ elif volume_pool.type == "RBD" and getattr(volume, "encryptformat",
None):
Review Comment:
Storage pool type comparisons mix case-insensitive (`type.lower() == ...`)
and case-sensitive (`type == \"RBD\"`) checks in the same helper. This makes
the test brittle to API casing changes and inconsistent with the rest of the
file’s usage. Normalize once (e.g., `pool_type = volume_pool.type.lower()`) and
compare using a single convention (`pool_type == \"rbd\"`).
##########
test/integration/smoke/test_snapshots.py:
##########
@@ -262,7 +262,7 @@ def test_02_list_snapshots_with_removed_data_store(self):
volume_pool_response = list_storage_pools(self.apiclient,
id=vol_res[0].storageid)
volume_pool = volume_pool_response[0]
- if volume_pool.type.lower() != 'networkfilesystem':
+ if volume_pool.type not in ("NetworkFilesystem", "RBD"):
Review Comment:
This check switched from case-insensitive (`type.lower()`) to case-sensitive
matching. Elsewhere in the tests, pool types are often normalized via
`.lower()`. For consistency and to avoid brittle behavior if the API returns
different casing, normalize before comparing (e.g., compare against lowercased
constants).
--
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]
