Copilot commented on code in PR #11931:
URL: https://github.com/apache/cloudstack/pull/11931#discussion_r4182298972
##########
test/integration/smoke/test_backup_recovery_nas.py:
##########
@@ -39,19 +43,56 @@ def setUpClass(cls):
cls.services["mode"] = cls.zone.networktype
cls.hypervisor = cls.testClient.getHypervisorInfo()
cls.domain = get_domain(cls.api_client)
+ cls._cleanup = []
+
+ if cls.hypervisor.lower() != 'kvm':
+ cls.skipTest(cls, reason="Test can be run only on KVM hypervisor")
+
cls.template = get_template(cls.api_client, cls.zone.id,
cls.services["ostype"])
if cls.template == FAILED:
assert False, "get_template() failed to return template with
description %s" % cls.services["ostype"]
cls.services["small"]["zoneid"] = cls.zone.id
cls.services["small"]["template"] = cls.template.id
- cls._cleanup = []
- if cls.hypervisor.lower() != 'kvm':
- cls.skipTest(cls, reason="Test can be run only on KVM hypervisor")
-
- cls.storage_pool = StoragePool.list(cls.api_client)[0]
- if cls.storage_pool.type.lower() != 'networkfilesystem':
- cls.skipTest(cls, reason="Test can be run only if the primary
storage is of type NFS")
+ # Pick a pool that's actually usable, not just list()[0] --
environments that
+ # added Ceph/RBD storage after the zone's original NFS primary storage
keep the
+ # old NFS pool around in Disabled state, and it still sorts first.
Falling back
+ # to index 0 there silently exercises the disabled NFS pool's path as
if it were
+ # the primary storage in use, rather than the RBD pool VMs actually
deploy on.
+ storage_pools = StoragePool.list(cls.api_client)
+ usable_pools = [p for p in storage_pools if getattr(p, 'state', 'Up')
== 'Up']
+ cls.storage_pool = usable_pools[0] if usable_pools else
storage_pools[0]
+ if cls.storage_pool.type.lower() not in
SUPPORTED_PRIMARY_STORAGE_POOL_TYPES:
+ cls.skipTest(cls, reason="Test can be run only if the primary
storage is of type NFS or RBD (Ceph)")
Review Comment:
Filtering only by state still lets an unrelated pool determine whether this
suite runs. For example, an Up Filesystem pool listed before an Up RBD pool
skips all tests despite supported storage being available. The global listing
can also select another zone's pool for the default NAS export. Restrict
candidates to Up NFS/RBD pools in the test zone, and skip when none exist
instead of falling back to the first global result.
##########
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]
+
+ volumes = Volume.list(self.apiclient, virtualmachineid=self.vm.id,
listall=True)
+ self.assertTrue(isinstance(volumes, list), "List volumes should
return a valid list")
+ root_disk_id = None
+ data_disk_id = None
+ for volume in volumes:
+ if volume.type == 'ROOT':
+ root_disk_id = volume.id
+ elif volume.type == 'DATADISK':
+ data_disk_id = volume.id
+ self.assertIsNotNone(root_disk_id, "The backed up VM should have a
ROOT volume")
+
+ # Target Instance that will receive the restored volumes. The nas
provider
+ # (unlike KBOSS) requires the target Instance to be stopped before
a volume
+ # can be restored and attached to it.
+ target_vm = VirtualMachine.create(
+ self.apiclient, self.services["small"],
accountid=self.account.name,
+ domainid=self.account.domainid,
serviceofferingid=self.offering.id,
+ mode=self.services["mode"]
+ )
+ target_vm.stop(self.apiclient, forced=True)
+
+ # Restore and attach the ROOT volume backup as an extra disk.
+ Backup.restoreVolumeFromBackupAndAttachToVM(
+ self.apiclient, backupid=backup.id, volumeid=root_disk_id,
virtualmachineid=target_vm.id
+ )
+ target_volumes = Volume.list(self.apiclient,
virtualmachineid=target_vm.id, listall=True)
+ self.assertTrue(isinstance(target_volumes, list), "List volumes
should return a valid list")
+ self.assertEqual(2, len(target_volumes),
+ "Target Instance should have its own ROOT volume plus the
restored volume")
+
+ if data_disk_id:
+ # Restore and attach the DATADISK volume backup as well.
+ Backup.restoreVolumeFromBackupAndAttachToVM(
+ self.apiclient, backupid=backup.id, volumeid=data_disk_id,
virtualmachineid=target_vm.id
+ )
+ target_volumes = Volume.list(self.apiclient,
virtualmachineid=target_vm.id, listall=True)
+ self.assertEqual(3, len(target_volumes),
+ "Target Instance should have 3 volumes after restoring
both the ROOT and DATADISK backups")
+
+ # Start the target Instance to verify the restored disk(s) are
actually
+ # usable and libvirt accepted the attach-device/attach-disk calls.
+ target_vm.start(self.apiclient)
+
+ Backup.delete(self.apiclient, backup.id)
+ finally:
Review Comment:
Backup deletion runs only on success. If a restore, assertion, or target VM
startup fails, `finally` removes the offering but leaves the backup behind: NAS
explicitly preserves backups on offering removal. This contaminates later tests
sharing `self.vm` and can block repository cleanup. Initialize a backup
variable before `try`, capture the object returned by `Backup.create()`, and
delete that backup in `finally`, while still ensuring VM cleanup and offering
removal are attempted.
##########
test/integration/smoke/test_direct_download.py:
##########
@@ -218,7 +218,7 @@ def setUpClass(cls):
zoneid=cls.zone.id
)
for pool in storage_pools:
- if not cls.nfsStorageFound and pool.type ==
"NetworkFilesystem":
+ if not cls.nfsStorageFound and pool.type in
("NetworkFilesystem", "RBD"):
Review Comment:
A disabled legacy NFS pool can be selected before an active RBD pool. The
test then tags that disabled pool and requires the same tag in its service
offering, so deployment fails instead of exercising Ceph. Require an Up pool
before recording this candidate.
##########
test/integration/smoke/test_backup_recovery_nas.py:
##########
@@ -303,12 +352,26 @@ def _backup_type(self, backup):
# Backup objects expose `type`; for chained backups it's
"INCREMENTAL", else "FULL".
return getattr(backup, 'type', 'FULL') or 'FULL'
+ def _require_incremental_capable_storage(self):
+ """
+ Incremental NAS backups rely on QEMU dirty bitmaps / libvirt
checkpoints, which
+ only exist on file-based qcow2 storage -- see
+ NASBackupProvider.allVolumesOnCheckpointCapableStorage(), which forces
every VM
+ on RBD/Ceph (or Linstor) onto the legacy full-only path server-side.
On such
+ storage every backup comes back FULL regardless of cadence, so these
chain/type
+ assertions can't pass (and some would pass vacuously without
exercising the
+ chain logic at all). Skip rather than fail when running against RBD.
+ """
+ if self.storage_pool.type.lower() == 'rbd':
+ self.skipTest("Incremental backups are not supported on RBD/Ceph
primary Storage")
Review Comment:
`self.storage_pool` is selected before deployment, and the VM's volumes are
not pinned to it. With NFS listed first but either VM disk on RBD, this guard
runs incremental tests even though the provider returns only FULL backups; the
reverse incorrectly skips supported tests. Resolve every attached volume's
actual pool and use the same capability predicate as
`NASBackupProvider.allVolumesOnCheckpointCapableStorage()`.
##########
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 checks the first pool across all zones, not the pool holding the test
VM. A disabled NFS pool listed before active RBD storage lets unsupported
snapshot tests run; an unrelated RBD pool can instead skip valid NFS coverage.
Remove this early check and gate each test in `setUp()` using
`self.volume_pool`, which is already resolved from the deployed VM's root
volume. Keeping the skip in `setUp()` also allows class resource cleanup to run.
##########
test/integration/smoke/test_volumes.py:
##########
@@ -1111,6 +1117,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="Volume encryption is unsupported for
volumes on RBD storage pool")
Review Comment:
The first global pool does not establish whether encryption is supported in
the selected test zone. An unrelated RBD pool can skip the entire suite despite
available encryption-capable storage, while a disabled NFS pool listed first
leaves a Ceph-only zone unskipped and encrypted deployment fails. Resolve the
test zone first and check eligible encryption-capable pools against the
offerings' placement constraints before creating encrypted volumes.
--
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]
