Repository navigation
smoke test: support ceph storage pools - #11931
Open
weizhouapache wants to merge 11 commits into
Open
weizhouapache wants to merge 11 commits into
weizhouapache wants to merge 11 commits into
Conversation
boring-cyborg
Bot
added
component:integration-test
Python
Member
Author
|
@blueorangutan package |
|
@weizhouapache a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #11931 +/- ##
============================================
+ Coverage 19.78% 20.21% +0.43%
- Complexity 19995 20751 +756
============================================
Files 6371 6426 +55
Lines 575909 580181 +4272
Branches 70509 71033 +524
============================================
+ Hits 113950 117293 +3343
- Misses 449526 450173 +647
- Partials 12433 12715 +282
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 15584 |
Member
Author
|
@blueorangutan test matrix |
|
@weizhouapache a [SL] Trillian-Jenkins matrix job (EL8 mgmt + EL8 KVM, Ubuntu22 mgmt + Ubuntu22 KVM, EL8 mgmt + VMware 7.0u3, EL9 mgmt + XCP-ng 8.2 ) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-14741)
|
|
[SF] Trillian test result (tid-14742)
|
Member
Author
|
@blueorangutan package |
|
@weizhouapache a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 15597 |
Member
Author
|
@blueorangutan test |
|
@weizhouapache a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-14744)
|
|
[SF] Trillian test result (tid-14753)
|
|
[SF] Trillian test result (tid-14743)
|
weizhouapache
force-pushed
the
4.22-smoke-tests-on-ceph
branch
from
December 9, 2025 10:38
ddfdacf to
844de76
Compare
weizhouapache
marked this pull request as ready for review
January 28, 2026 09:30
Member
Author
|
ready for review and testing |
Contributor
|
@blueorangutan package |
test_backup_recovery_nas.py only allowed NFS primary storage, since it reused the primary storage pool's own path as the NAS backup repository address, and always required incremental-backup semantics that only qcow2/NFS storage can provide. Neither holds on Ceph/RBD. - setUpClass now accepts RBD alongside NFS as the primary storage pool type, picking a pool that's actually Up rather than list()[0] -- environments that added Ceph/RBD after the zone's original NFS primary storage keep that old pool around in Disabled state, and it still sorts first, silently exercising its path as if it were the storage VMs actually deploy on. - The NAS backup repository's NFS export address is resolved independently of the primary storage: when the primary pool isn't NFS, reuse the nfs test data entry (services[nfs][url]) -- the same temporary NFS mount point test_primary_storage.py uses for its temporary NFS primary storage pool, and something every marvin environment already has configured. An explicit nas_backup_repository_address test data entry or NAS_BACKUP_REPO_ADDRESS environment variable, if set, takes precedence. - The external offering imported in setUpClass is matched to the repository just created 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, whose backups didn't get cleaned up so its own teardown couldn't remove it either, sorts alongside the new one with no guarantee of which comes first. - Incremental NAS backups require QEMU dirty bitmaps / libvirt checkpoints, which only exist on file-based qcow2 storage (NASBackupProvider.allVolumesOnCheckpointCapableStorage). The six incremental-chain tests now skip on RBD/Ceph, where the provider always falls back to full-only backups server-side, rather than failing on assertions that storage type can never satisfy. - Added test_restore_volume_and_attach_to_vm, which exercises restoreVolumeFromBackupAndAttachToVM end-to-end (restoring a backed-up ROOT and DATADISK volume onto a second, stopped Instance) -- the API that drives the restore-and-attach code fixed by the previous commit (apache#14007). The target Instance is stopped with forced=True: a graceful ACPI stop was observed to time out (~2 minutes) before falling back to a hard destroy anyway, and once forced to a hard destroy the domain drops out of libvirt entirely, so the periodic ping-based PowerState sync the restore call depends on falls back to a much slower heuristic well past any reasonable wait. A forced stop destroys the domain immediately and deterministically.
weizhouapache
force-pushed
the
4.22-smoke-tests-on-ceph
branch
from
September 9, 2026 07:51
844de76 to
fff8e56
Compare
14 tasks
A zone-wide storage pool (e.g. RBD configured with scope=ZONE) isn't bound to a single pod/cluster, so a volume on it has no clustername/ clusterid/podid/podname. test_10_list_volumes asserted these were always non-None, which fails whenever storage is zone-scoped rather than cluster-scoped. Now look up the volume's actual storage pool and only assert pod/cluster attributes when its scope isn't ZONE.
When a host has no explicit tags initially, cls.host.hosttags is None. Passing None to the API does not set the hosttags parameter, so tags are never cleared during cleanup. Convert None to empty string to properly clear tags that were set during the test.
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Storage selection can incorrectly run or skip tests, and failed restore tests can leave backups behind.
Review effort: Balanced
Findings: 6
Open (6)
What changed in this PR
Extends CloudStack’s smoke tests to support Ceph/RBD storage while skipping unsupported operations.
Changes:
- Enables RBD coverage for direct downloads, overprovisioning, snapshots, and NAS backups.
- Adds storage-aware checks and skips.
- Adds offline backup-volume restore-and-attach coverage.
| File | Description |
|---|---|
| test/integration/smoke/test_volumes.py | Adjusts zone-wide pool assertions and encryption skips. |
| test/integration/smoke/test_vm_snapshots.py | Adds an RBD snapshot skip. |
| test/integration/smoke/test_vm_life_cycle.py | Skips live migration between RBD pools. |
| test/integration/smoke/test_snapshots.py | Allows RBD snapshot coverage. |
| test/integration/smoke/test_over_provisioning.py | Includes RBD overprovisioning tests. |
| test/integration/smoke/test_host_tags.py | Sends empty strings to clear host tags. |
| test/integration/smoke/test_direct_download.py | Includes RBD in shared-storage coverage. |
| test/integration/smoke/test_backup_recovery_nas.py | Adds Ceph support, repository selection, incremental skips, and restore coverage. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+62
to
+66
| 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)") |
Comment on lines
+365
to
+366
| if self.storage_pool.type.lower() == 'rbd': | ||
| self.skipTest("Incremental backups are not supported on RBD/Ceph primary Storage") |
Comment on lines
+733
to
+734
| Backup.delete(self.apiclient, backup.id) | ||
| finally: |
| ) | ||
| for pool in storage_pools: | ||
| if not cls.nfsStorageFound and pool.type == "NetworkFilesystem": | ||
| if not cls.nfsStorageFound and pool.type in ("NetworkFilesystem", "RBD"): |
Comment on lines
+50
to
+53
| 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") |
Comment on lines
+1120
to
+1123
| 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") |
Zone-wide storage pools (scope=ZONE) are not bound to clusters or pods. When filtering by clusterid/podid, these pools may be included in results with clusterid=None and podid=None. Updated tests to: - test_01: Accept zone-wide pools in clusterid filter results - test_07: Accept zone-wide pools in podid filter results - test_12: New test validating zone-wide vs cluster-scoped pools Both invalid clusterid/podid filters now correctly return zone-wide pools, and assertions skip None values for zone-wide pools while validating cluster-scoped pools have their expected scope identifiers. Validated on two environments: - Zone-wide only (10.1.112.155): 12/12 tests passed - Cluster-scoped only (10.0.112.202): 12/12 tests passed
Contributor
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
16 open findings
BackupOffering.listExternal(...)returnsNoneor an empty list, indexing[0]will raise.… · NewassertEqual(len(backups), 1)will fail if the environment already has… · NewassertEqual(len(backups), 1)will fail if the environment already has… · Newcluster_scoped_foundis set but never asserted, so the test can pass vacuously even if the filter… · Newpod_scoped_foundis never asserted, so the test may pass… · Newscopeattribute are silently ignored and won’t be validated by either branch,… · Newvolume_pool.type.lower(), suggesting… · Newvolume_pool.type.lower(), suggesting… · Newlist_storage_pools()[0], which can be the wrong pool in… · New
🧠 Review effort: Lite
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
Comment on lines
127
to
+131
| 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) |
Comment on lines
+684
to
+688
| 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] |
Comment on lines
+733
to
+740
| Backup.delete(self.apiclient, backup.id) | ||
| finally: | ||
| if target_vm is not None: | ||
| try: | ||
| target_vm.delete(self.apiclient) | ||
| except Exception: | ||
| pass | ||
| self.backup_offering.removeOffering(self.apiclient, self.vm.id) |
| self.cluster.id, | ||
| "Cluster id should be equal to the cluster id passed in the filter" | ||
| ) | ||
|
|
| self.cluster.podid, | ||
| "Pod id should be equal to the pod id passed in the filter" | ||
| ) | ||
|
|
Comment on lines
+421
to
+423
| zone_wide_pools = [p for p in storage_pools if hasattr(p, 'scope') and p.scope == "ZONE"] | ||
| cluster_scoped_pools = [p for p in storage_pools if hasattr(p, 'scope') and p.scope != "ZONE"] | ||
|
|
| 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"): |
Comment on lines
+294
to
296
| self.assertTrue( | ||
| storage.type in ("NetworkFilesystem", "RBD"), | ||
| "Check storage pool type" |
Comment on lines
+50
to
+53
| 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") |
Comment on lines
+1120
to
+1123
| 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") |
Copilot stopped reviewing on behalf of
weizhouapache due to an error
October 8, 2026 12:15
TestVolumeEncryption previously skipped entirely on RBD storage pools. Now that RBD encryption is supported (PR apache#13556), extend it instead of skipping: - setUpClass no longer hard-skips RBD pools; records storage_pool_type for use by the assertions below. - check_volume_encryption() branches on pool type: RBD volumes are encrypted natively by librbd, so the domain XML carries <encryption format='luks2' engine='librbd'>; other pool types keep the existing qemu-native <encryption format='luks'> check. - does_host_with_encryption_support_exists() gained an RBD-specific path: the API's "encryptionsupported" field only reflects qemu-native LUKS support, so librbd capability is read from the host_details table (host.volume.encryption.rbd), joined on the host's uuid. - get_expected_guest_visible_size() centralizes the existing PowerFlex size-overhead adjustment and adds the equivalent for RBD: encrypted RBD volumes reserve a 16 MiB LUKS2 header out of the volume's own reported size, so the guest sees size - header. - test_03_root_and_data_volume_encryption now also exercises resize of the attached encrypted data volume, reusing its VM instead of spinning up a separate one: - online (VM stays running): verified via the volume's own API state and size, since picking up a live virtio-blk capacity change in the guest depends on the guest OS/kernel (the CentOS 5.5 template used here predates reliable live-resize notification) and is orthogonal to whether CloudStack/libvirt/qemu actually grew the volume. - offline (detach, resize, reattach): verified against the guest's fdisk output, matched by the expected byte count rather than a device letter derived from deviceid, since libvirt is not guaranteed to reuse the same target letter after a detach then reattach.
Deploy a VM from the direct download template on each type of storage found (NFS, RBD, SharedMountPoint and local) in a single test, using the storage pool type as the storage tag.
A disk-only VM snapshot does not contain the guest page cache, so the file written by the test may be missing after the revert.
Contributor
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
22 open findings
len(backups) == 1is brittle if prior tests (or a previously interrupted run) left… · Newcluster_scoped_foundis set but never asserted, so this test can pass even if the filter returns… · Newpod_scoped_foundis never asserted, so the test may pass… · Newlist_storage_pools(cls.apiclient)always returns a non-empty list. If it returns… · Newlist_storage_pools()[0], which can be the wrong pool in…volume_pool.type.lower(), suggesting…volume_pool.type.lower(), suggesting…scopeattribute are silently ignored and won’t be validated by either branch,…pod_scoped_foundis never asserted, so the test may pass…cluster_scoped_foundis set but never asserted, so the test can pass vacuously even if the filter…assertEqual(len(backups), 1)will fail if the environment already has…assertEqual(len(backups), 1)will fail if the environment already has…BackupOffering.listExternal(...)returnsNoneor an empty list, indexing[0]will raise.…
And 2 more that still need to be addressed.
🧠 Review effort: Lite
Comment on lines
+684
to
+688
| 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] |
Comment on lines
307
to
317
| 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 |
Comment on lines
+65
to
+75
| # 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" | ||
| ) |
| storage_pool.podid, | ||
| self.cluster.podid, | ||
| "Pod id should be equal to the pod id passed in the filter" | ||
| ) |
Comment on lines
+50
to
+53
| 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") |
| 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"): |
Contributor
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
30 open findings
BackupOffering.listExternal(...)can returnNone/empty in some environments or transiently;… · Newpool_type.lower()as the temporary storage tag is very generic (e.g.,rbd,… · Newcluster_scoped_foundis set but never asserted/used, so the test can pass even when the cluster… · Newpod_scoped_foundis never asserted/used, so the test can… · Newtarget_pools[0]. If the first candidate happens to be an RBD… · Newtype.lower() == ...) and case-sensitive… · NewElementTree.findall(...)always returns a list (possibly empty), so… · Newlist_storage_pools(cls.apiclient)always returns a non-empty list. If it returns…pod_scoped_foundis never asserted, so the test may pass…cluster_scoped_foundis set but never asserted, so this test can pass even if the filter returns…len(backups) == 1is brittle if prior tests (or a previously interrupted run) left…list_storage_pools()[0], which can be the wrong pool in…volume_pool.type.lower(), suggesting…volume_pool.type.lower(), suggesting…scopeattribute are silently ignored and won’t be validated by either branch,…pod_scoped_foundis never asserted, so the test may pass…cluster_scoped_foundis set but never asserted, so the test can pass vacuously even if the filter…assertEqual(len(backups), 1)will fail if the environment already has…
And 10 more that still need to be addressed.
🧠 Review effort: Lite
Comment on lines
127
to
+131
| 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) |
Comment on lines
+310
to
+312
| # Create service offering for the storage using the storage pool type as storage tag | ||
| tags = self.getCurrentStoragePoolTags(pool_id) | ||
| test_tag = pool_type.lower() |
Comment on lines
+65
to
+75
| # 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" | ||
| ) |
| self.cluster.podid, | ||
| "Pod id should be equal to the pod id passed in the filter" | ||
| ) | ||
|
|
Comment on lines
+1719
to
+1720
| if source_pool.type == 'RBD' and target_pools[0].type == 'RBD': | ||
| self.skipTest("Live VM migration between RBD pools is unsupported") |
Comment on lines
+1687
to
+1690
| 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): |
Comment on lines
+1748
to
+1750
| 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) |
| 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"): |
Copilot stopped reviewing on behalf of
weizhouapache due to an error
October 14, 2026 20:57
Copilot stopped reviewing on behalf of
weizhouapache due to an error
October 14, 2026 21:21
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR improves and fixes some tests on ceph storage
skip volume migration test between ceph storage pools(fixed by ceph: fix offline volume migration between ceph pools #12103)Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?