Copilot commented on code in PR #11931:
URL: https://github.com/apache/cloudstack/pull/11931#discussion_r4275118843
##########
test/integration/smoke/test_direct_download.py:
##########
Review Comment:
Previously, the SharedMountPoint case deployed the VM with an explicit
`networkids=self.l2_network.id`, while the other cases used `deployVM(...)`.
After refactor, SharedMountPoint also goes through `deployVM(...)`. If
`deployVM` doesn’t set the required network for this scenario, SharedMountPoint
coverage may start failing (or stop exercising the intended network path).
Consider preserving the SharedMountPoint-specific VM creation parameters (or
enhancing `deployVM` to accept/handle the needed network selection).
##########
test/integration/smoke/test_backup_recovery_nas.py:
##########
@@ -594,3 +662,79 @@ def test_stopped_vm_falls_back_to_full(self):
finally:
self._set_full_every(original_full_every)
self.backup_offering.removeOffering(self.apiclient, self.vm.id)
+
+ # ------------------------------------------------------------------
+ # Restore-volume-and-attach regression (PR apache/cloudstack#14007)
+ # ------------------------------------------------------------------
+ # This test exercises the fixed path end to end via
restoreVolumeFromBackupAndAttachToVM,
+ # on whichever primary storage this environment is running with (NFS or
Ceph/RBD).
+
+ @attr(tags=["advanced", "backup"], required_hardware="true")
+ def test_restore_volume_and_attach_to_vm(self):
+ """
+ Test restoring the ROOT and DATADISK volumes of a backup and attaching
them
+ to a different Instance (restoreVolumeFromBackupAndAttachToVM).
+ """
+ target_vm = None
+ self.backup_offering.assignOffering(self.apiclient, self.vm.id)
+ try:
+ ssh_client_vm = self.vm.get_ssh_client(reconnect=True)
+ ssh_client_vm.execute("echo restore-attach-marker >
/root/restore_attach_marker.txt; sync")
+
+ Backup.create(self.apiclient, self.vm.id, "restore_attach_backup")
+
+ backups = Backup.list(self.apiclient, self.vm.id)
+ self.assertEqual(len(backups), 1, "There should exist only one
backup for the VM")
+ backup = backups[0]
Review Comment:
Asserting `len(backups) == 1` is brittle if prior tests (or a previously
interrupted run) left backups behind, even when this test’s new backup is
created successfully. To avoid flakes, identify the backup you just created
(e.g., by filtering on name/metadata if exposed by the API, or by selecting the
most recent matching entry) rather than requiring the VM to have exactly one
backup total.
##########
test/integration/smoke/test_vm_snapshots.py:
##########
@@ -47,6 +47,11 @@ def setUpClass(cls):
cls.unsupportedHypervisor = True
return
+ list_volume_pool_response = list_storage_pools(cls.apiclient)
+ volume_pool = list_volume_pool_response[0]
+ if volume_pool.type == "RBD":
+ cls.skipTest(cls, reason="VM snapshot is unsupported for VMs on
RBD storage pool")
Review Comment:
This assumes `list_storage_pools(cls.apiclient)` always returns a non-empty
list. If it returns `None` or `[]` in a misconfigured/empty environment,
`list_volume_pool_response[0]` will raise and fail the suite rather than
skipping with a helpful reason. Add a guard to skip (or fail with a clear
message) when no storage pools are returned.
##########
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, so this test can pass even
if the filter returns only zone-wide pools (or even an empty list) and never
validates the `clusterid` constraint. Consider asserting that at least one
cluster-scoped pool was observed (when the environment is expected to have one)
or reintroduce a minimal non-emptiness/expectation check so the test can’t pass
vacuously.
##########
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 cluster filter: `pod_scoped_found` is never asserted, so
the test may pass without validating any pod-scoped pool behavior. Add an
assertion (or an explicit expectation) so the test fails when it doesn’t
exercise the intended podid-filter semantics.
##########
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 changed from a case-normalized comparison (`type.lower()`) to a
case-sensitive one. If pool types ever come back with different casing (or if
future providers follow a different naming convention), the test may
incorrectly skip/fail. Consider normalizing `volume_pool.type` (e.g., via
`.lower()`) and comparing against normalized constants, as is done elsewhere in
this PR.
--
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]
