You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by "weizhouapache (via GitHub)" <gi...@apache.org> on 2025/10/29 14:50:03 UTC
[PR] smoke test: support ceph storage pools [cloudstack]
weizhouapache opened a new pull request, #11931:
URL: https://github.com/apache/cloudstack/pull/11931
### Description
This PR improves and fixes some tests on ceph storage
- enable direct download tests on ceph
- enable over provisioning tests on ceph
- enable volume snapshot tests on ceph
- fix test_human_readable_logs.py by increasing timeout of restarting to 120 seconds
- skip live vm migration for VMs with volume migration between ceph storage pools
- skip vm snapshots for VMs on ceph
- skip volume migration test between ceph storage pools
- skip volume encryption tests on ceph
<!--- Describe your changes in DETAIL - And how has behaviour functionally changed. -->
<!-- For new features, provide link to FS, dev ML discussion etc. -->
<!-- In case of bug fix, the expected and actual behaviours, steps to reproduce. -->
<!-- When "Fixes: #<id>" is specified, the issue/PR will automatically be closed when this PR gets merged -->
<!-- For addressing multiple issues/PRs, use multiple "Fixes: #<id>" -->
<!-- Fixes: # -->
<!--- ******************************************************************************* -->
<!--- NOTE: AUTOMATION USES THE DESCRIPTIONS TO SET LABELS AND PRODUCE DOCUMENTATION. -->
<!--- PLEASE PUT AN 'X' in only **ONE** box -->
<!--- ******************************************************************************* -->
### Types of changes
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Bug fix (non-breaking change which fixes an issue)
- [ ] Enhancement (improves an existing feature and functionality)
- [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
- [ ] Build/CI
- [x] Test (unit or integration test code)
### Feature/Enhancement Scale or Bug Severity
#### Feature/Enhancement Scale
- [ ] Major
- [ ] Minor
#### Bug Severity
- [ ] BLOCKER
- [ ] Critical
- [ ] Major
- [ ] Minor
- [ ] Trivial
### Screenshots (if appropriate):
### How Has This Been Tested?
<!-- Please describe in detail how you tested your changes. -->
<!-- Include details of your testing environment, and the tests you ran to -->
#### How did you try to break this feature and the system with this change?
<!-- see how your change affects other areas of the code, etc. -->
<!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md) document -->
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] smoke test: support ceph storage pools [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #11931:
URL: https://github.com/apache/cloudstack/pull/11931#issuecomment-3468265823
<b>[SF] Trillian test result (tid-14741)</b>
Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8
Total time taken: 71700 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr11931-t14741-kvm-ol8.zip
Smoke tests completed. 147 look OK, 2 have errors, 0 did not run
Only failed and skipped tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_02_migrate_VM_with_two_data_disks | `Error` | 38.80 | test_vm_life_cycle.py
ContextSuite context=TestVmSnapshot>:teardown | `Error` | 288.49 | test_vm_snapshots.py
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] smoke test: support ceph storage pools [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #11931:
URL: https://github.com/apache/cloudstack/pull/11931#issuecomment-6464776068
> Hi @weizhouapache can you please check if the Copilot comments are relevant?
addressed
thanks @nvazquez
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] smoke test: support ceph storage pools [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #11931:
URL: https://github.com/apache/cloudstack/pull/11931#issuecomment-3477396619
<b>[SF] Trillian test result (tid-14743)</b>
Environment: vmware-70u3 (x2), zone: Advanced Networking with Mgmt server ol8
Total time taken: 292096 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr11931-t14743-vmware-70u3.zip
Smoke tests completed. 136 look OK, 13 have errors, 0 did not run
Only failed and skipped tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_08_upgrade_kubernetes_ha_cluster | `Failure` | 3693.33 | test_kubernetes_clusters.py
test_list_vms_metrics_admin | `Error` | 3633.14 | test_metrics_api.py
test_list_vms_metrics_history | `Error` | 25.23 | test_metrics_api.py
test_list_volumes_metrics_history | `Error` | 3606.95 | test_metrics_api.py
ContextSuite context=TestISOUsage>:setup | `Error` | 0.00 | test_usage.py
test_01_vpn_usage | `Error` | 1.22 | test_usage.py
test_01_deploy_vm_on_specific_host | `Error` | 26.64 | test_vm_deployment_planner.py
test_02_deploy_vm_on_specific_cluster | `Error` | 3604.05 | test_vm_deployment_planner.py
test_03_deploy_vm_on_specific_pod | `Error` | 2.66 | test_vm_deployment_planner.py
test_04_deploy_vm_on_host_override_pod_and_cluster | `Error` | 4.61 | test_vm_deployment_planner.py
test_05_deploy_vm_on_cluster_override_pod | `Error` | 3.61 | test_vm_deployment_planner.py
test_09_expunge_vm | `Failure` | 424.88 | test_vm_life_cycle.py
ContextSuite context=TestVmSnapshot>:teardown | `Error` | 299.52 | test_vm_snapshots.py
test_01_migrate_vm_strict_tags_success | `Error` | 3607.74 | test_vm_strict_host_tags.py
test_02_migrate_vm_strict_tags_failure | `Error` | 4.65 | test_vm_strict_host_tags.py
test_01_restore_vm_strict_tags_success | `Error` | 20.56 | test_vm_strict_host_tags.py
test_02_restore_vm_strict_tags_failure | `Error` | 3607.25 | test_vm_strict_host_tags.py
test_01_scale_vm_strict_tags_success | `Error` | 23.20 | test_vm_strict_host_tags.py
test_02_scale_vm_strict_tags_failure | `Error` | 3606.02 | test_vm_strict_host_tags.py
test_01_deploy_vm_on_specific_host_without_strict_tags | `Error` | 27.80 | test_vm_strict_host_tags.py
test_02_deploy_vm_on_any_host_without_strict_tags | `Error` | 3605.43 | test_vm_strict_host_tags.py
test_03_deploy_vm_on_specific_host_with_strict_tags_success | `Error` | 3603.58 | test_vm_strict_host_tags.py
test_04_deploy_vm_on_any_host_with_strict_tags_success | `Error` | 3608.30 | test_vm_strict_host_tags.py
test_04_deploy_vnf_appliance | `Error` | 2022.66 | test_vnf_templates.py
ContextSuite context=TestVnfTemplates>:teardown | `Error` | 1817.67 | test_vnf_templates.py
ContextSuite context=TestCreateVolume>:teardown | `Error` | 2264.70 | test_volumes.py
test_01_verify_ipv6_vpc | `Error` | 3791.85 | test_vpc_ipv6.py
test_01_verify_ipv6_vpc | `Error` | 3792.19 | test_vpc_ipv6.py
test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL | `Error` | 1924.77 | test_vpc_redundant.py
test_02_redundant_VPC_default_routes | `Error` | 1923.29 | test_vpc_redundant.py
test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers | `Error` | 1819.42 | test_vpc_redundant.py
test_04_rvpc_network_garbage_collector_nics | `Error` | 1925.69 | test_vpc_redundant.py
test_05_rvpc_multi_tiers | `Error` | 1915.19 | test_vpc_redundant.py
test_01_VPC_nics_after_destroy | `Failure` | 5719.14 | test_vpc_router_nics.py
test_01_redundant_vpc_site2site_vpn | `Failure` | 1912.43 | test_vpc_vpn.py
test_01_redundant_vpc_site2site_vpn | `Error` | 1912.58 | test_vpc_vpn.py
test_01_vpc_site2site_vpn_multiple_options | `Error` | 2564.26 | test_vpc_vpn.py
test_01_vpc_remote_access_vpn | `Error` | 1937.20 | test_vpc_vpn.py
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] smoke test: support ceph storage pools [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #11931:
URL: https://github.com/apache/cloudstack/pull/11931#issuecomment-3895004240
@sureshanaparti 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.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] smoke test: support ceph storage pools [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #11931:
URL: https://github.com/apache/cloudstack/pull/11931#issuecomment-3462013741
@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.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] smoke test: support ceph storage pools [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #11931:
URL: https://github.com/apache/cloudstack/pull/11931#issuecomment-3462003689
@blueorangutan package
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] smoke test: support ceph storage pools [cloudstack]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] smoke test: support ceph storage pools [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on code in PR #11931:
URL: https://github.com/apache/cloudstack/pull/11931#discussion_r4398495665
##########
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:
removed
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] smoke test: support ceph storage pools [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #11931:
URL: https://github.com/apache/cloudstack/pull/11931#issuecomment-3469794976
@blueorangutan package
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] smoke test: support ceph storage pools [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #11931:
URL: https://github.com/apache/cloudstack/pull/11931#issuecomment-3470743399
@blueorangutan test
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] smoke test: support ceph storage pools [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #11931:
URL: https://github.com/apache/cloudstack/pull/11931#issuecomment-6468872984
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19799
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] smoke test: support ceph storage pools [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #11931:
URL: https://github.com/apache/cloudstack/pull/11931#issuecomment-3470210057
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 15597
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] smoke test: support ceph storage pools [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #11931:
URL: https://github.com/apache/cloudstack/pull/11931#issuecomment-3471466330
<b>[SF] Trillian test result (tid-14744)</b>
Environment: xcpng82 (x2), zone: Advanced Networking with Mgmt server ol9
Total time taken: 129543 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr11931-t14744-xcpng82.zip
Smoke tests completed. 139 look OK, 10 have errors, 0 did not run
Only failed and skipped tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_01_events_resource | `Error` | 822.70 | test_events_resource.py
test_08_upgrade_kubernetes_ha_cluster | `Failure` | 3787.76 | test_kubernetes_clusters.py
test_01_non_strict_host_anti_affinity | `Error` | 520.75 | test_nonstrict_affinity_group.py
test_02_non_strict_host_affinity | `Error` | 130.06 | test_nonstrict_affinity_group.py
test_02_list_snapshots_with_removed_data_store | `Error` | 18.51 | test_snapshots.py
test_02_list_snapshots_with_removed_data_store | `Error` | 18.52 | test_snapshots.py
test_01_ssl_offloading_isolated_network | `Failure` | 949.48 | test_ssl_offloading.py
test_01_vpn_usage | `Error` | 1.18 | test_usage.py
test_01_scale_up_verify | `Failure` | 579.72 | test_vm_autoscaling.py
test_11_destroy_vm_and_volumes | `Error` | 30.02 | test_vm_life_cycle.py
ContextSuite context=TestVmSnapshot>:teardown | `Error` | 314.04 | test_vm_snapshots.py
test_01_migrate_vm_strict_tags_success | `Error` | 78.07 | test_vm_strict_host_tags.py
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] smoke test: support ceph storage pools [cloudstack]
Posted by "sureshanaparti (via GitHub)" <gi...@apache.org>.
sureshanaparti commented on PR #11931:
URL: https://github.com/apache/cloudstack/pull/11931#issuecomment-3895006445
> ready for review and testing
@weizhouapache can you advise on testing, should these smoke tests run against env with only ceph as primary storage?
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] smoke test: support ceph storage pools [cloudstack]
Posted by "sureshanaparti (via GitHub)" <gi...@apache.org>.
sureshanaparti commented on PR #11931:
URL: https://github.com/apache/cloudstack/pull/11931#issuecomment-3894998712
@blueorangutan package
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] smoke test: support ceph storage pools [cloudstack]
Posted by "nvazquez (via GitHub) via commits" <co...@cloudstack.apache.org>.
nvazquez commented on PR #11931:
URL: https://github.com/apache/cloudstack/pull/11931#issuecomment-6564122623
@blueorangutan test
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] smoke test: support ceph storage pools [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #11931:
URL: https://github.com/apache/cloudstack/pull/11931#issuecomment-3895177748
Packaging result [SF]: ✖️ el8 ✖️ el9 ✔️ debian ✖️ suse15. SL-JID 16822
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] smoke test: support ceph storage pools [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #11931:
URL: https://github.com/apache/cloudstack/pull/11931#issuecomment-3895836823
@sureshanaparti 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.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] smoke test: support ceph storage pools [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #11931:
URL: https://github.com/apache/cloudstack/pull/11931#issuecomment-3462484755
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 15584
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] smoke test: support ceph storage pools [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #11931:
URL: https://github.com/apache/cloudstack/pull/11931#issuecomment-3470745280
@weizhouapache a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] smoke test: support ceph storage pools [cloudstack]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] smoke test: support ceph storage pools [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #11931:
URL: https://github.com/apache/cloudstack/pull/11931#issuecomment-3810075356
ready for review and testing
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] smoke test: support ceph storage pools [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #11931:
URL: https://github.com/apache/cloudstack/pull/11931#issuecomment-3462706121
@blueorangutan test matrix
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] smoke test: support ceph storage pools [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #11931:
URL: https://github.com/apache/cloudstack/pull/11931#issuecomment-3474078564
<b>[SF] Trillian test result (tid-14753)</b>
Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8
Total time taken: 58456 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr11931-t14753-kvm-ol8.zip
Smoke tests completed. 149 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] smoke test: support ceph storage pools [cloudstack]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #11931:
URL: https://github.com/apache/cloudstack/pull/11931#discussion_r4218641817
##########
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:
If `BackupOffering.listExternal(...)` returns `None` or an empty list, indexing `[0]` will raise. Add an explicit assertion/skip with a clear reason when no external offerings are available (e.g., provider not properly configured) before attempting to select a matching 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, so the test can pass vacuously even if the filter returns only zone-wide pools (or returns unrelated pools with `clusterid=None`). Add an assertion that at least one cluster-scoped pool was returned (or explicitly `skipTest` when the environment has no cluster-scoped pools) so the `clusterid` filter behavior is actually validated.
##########
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 test: `pod_scoped_found` is never asserted, so the test may pass without validating filtering. Assert `pod_scoped_found` (or skip when the environment lacks pod-scoped pools) to ensure the `podid` filter is meaningfully tested.
##########
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:
Two reliability issues: (1) `assertEqual(len(backups), 1)` will fail if the environment already has backups for the VM (e.g., from previous incomplete runs). Prefer selecting the backup by name/ID returned from `Backup.create` (or filtering by `name == \"restore_attach_backup\"`). (2) `Backup.delete` is not in the `finally` block, so a failure before line 733 leaves the backup behind and can pollute subsequent runs. Move deletion into `finally` with a guard that `backup` was created.
##########
test/integration/smoke/test_list_storage_pools.py:
##########
@@ -394,3 +404,64 @@ def test_11_list_storage_pools_no_filter(self):
0,
"Length of storage pools should greater than 0"
)
+
+ @attr(tags=["advanced", "advancedns", "smoke", "basic"], required_hardware="false")
+ def test_12_list_storage_pools_zone_wide(self):
+ """ Test listing zone-wide vs cluster-scoped storage pools
+ Zone-wide pools (scope=ZONE) are not bound to a specific cluster.
+ Cluster-scoped pools are bound to clusters.
+ """
+ # List all storage pools and categorize them
+ storage_pools = StoragePool.list(self.apiclient)
+ self.assertTrue(
+ isinstance(storage_pools, list),
+ "Storage pool response type should be a list"
+ )
+
+ 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"]
+
Review Comment:
Pools without a `scope` attribute are silently ignored and won’t be validated by either branch, which can make the test pass while missing (or regressing) an expected API field. Consider asserting that all returned pools expose `scope` (if it’s required), or include pools without `scope` in one category and fail with a clear message so regressions don’t get masked.
##########
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:
Both checks are now case-sensitive, but earlier code used `volume_pool.type.lower()`, suggesting API responses may vary in casing. This can cause incorrect skips/failures if the type comes back as e.g. `networkfilesystem`/`rbd`. Normalize case (e.g., compare `type.lower()` against lowercased allow-list) consistently for both `volume_pool.type` and `storage.type`.
##########
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:
Same issue as above: using the first storage pool to decide whether to skip can incorrectly skip (or not skip) in environments with multiple pools. Use the storage pool actually backing the VM under test (derive from the deployed VM’s root volume `storageid`), or pick an `Up` pool and ensure it represents the pool the test will consume. Add a defensive check for an empty result to avoid `IndexError`.
##########
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:
+ if target_vm is not None:
+ try:
+ target_vm.delete(self.apiclient)
+ except Exception:
+ pass
+ self.backup_offering.removeOffering(self.apiclient, self.vm.id)
Review Comment:
Two reliability issues: (1) `assertEqual(len(backups), 1)` will fail if the environment already has backups for the VM (e.g., from previous incomplete runs). Prefer selecting the backup by name/ID returned from `Backup.create` (or filtering by `name == \"restore_attach_backup\"`). (2) `Backup.delete` is not in the `finally` block, so a failure before line 733 leaves the backup behind and can pollute subsequent runs. Move deletion into `finally` with a guard that `backup` was created.
##########
test/integration/smoke/test_snapshots.py:
##########
@@ -291,9 +291,8 @@ def test_02_list_snapshots_with_removed_data_store(self):
'Up',
"Check primary storage state"
)
- self.assertEqual(
- storage.type,
- 'NetworkFilesystem',
+ self.assertTrue(
+ storage.type in ("NetworkFilesystem", "RBD"),
"Check storage pool type"
Review Comment:
Both checks are now case-sensitive, but earlier code used `volume_pool.type.lower()`, suggesting API responses may vary in casing. This can cause incorrect skips/failures if the type comes back as e.g. `networkfilesystem`/`rbd`. Normalize case (e.g., compare `type.lower()` against lowercased allow-list) consistently for both `volume_pool.type` and `storage.type`.
##########
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:
This skips the entire class based on `list_storage_pools()[0]`, which can be the wrong pool in mixed setups (or even a disabled/non-default pool), causing false skips. Prefer determining the pool used by the test VM/volume (e.g., create the volume/VM first and check its `storageid`), or at minimum select an `Up` pool (and only skip when the relevant pool type is RBD). Also guard against an empty `list_storage_pools` response to avoid `IndexError`.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] smoke test: support ceph storage pools [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on code in PR #11931:
URL: https://github.com/apache/cloudstack/pull/11931#discussion_r4398454112
##########
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:
let's make simple, do not consider live vm migration with RBD volumes
it will be addressed in future releases
```
- if source_pool.type == 'RBD' and target_pools[0].type == 'RBD':
- self.skipTest("Live VM migration between RBD pools is unsupported")
+ if source_pool.type == 'RBD':
+ self.skipTest("Live VM migration from RBD pool is unsupported")
```
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] smoke test: support ceph storage pools [cloudstack]
Posted by "blueorangutan (via GitHub) via commits" <co...@cloudstack.apache.org>.
blueorangutan commented on PR #11931:
URL: https://github.com/apache/cloudstack/pull/11931#issuecomment-6581436465
<b>[SF] Trillian test result (tid-17426)</b>
Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8
Total time taken: 61818 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr11931-t17426-kvm-ol8.zip
Smoke tests completed. 155 look OK, 2 have errors, 0 did not run
Only failed and skipped tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_01_RVR_Network_FW_PF_SSH_default_routes_egress_true | `Failure` | 203.44 | test_routers_network_ops.py
test_03_ssvm_internals | `Failure` | 6.40 | test_ssvm.py
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] smoke test: support ceph storage pools [cloudstack]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #11931:
URL: https://github.com/apache/cloudstack/pull/11931#issuecomment-3462055335
## [Codecov](https://app.codecov.io/gh/apache/cloudstack/pull/11931?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 3.58%. Comparing base ([`2dbc86a`](https://app.codecov.io/gh/apache/cloudstack/commit/2dbc86abfaea70653d36926527648d013fbc2c92?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`248bc6c`](https://app.codecov.io/gh/apache/cloudstack/commit/248bc6c58a8ee9a562975b9237130c89b5cf4297?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
> :exclamation: There is a different number of reports uploaded between BASE (2dbc86a) and HEAD (248bc6c). Click for more details.
>
> <details><summary>HEAD has 1 upload less than BASE</summary>
>
>| Flag | BASE (2dbc86a) | HEAD (248bc6c) |
>|------|------|------|
>|unittests|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #11931 +/- ##
=============================================
- Coverage 17.56% 3.58% -13.98%
=============================================
Files 5909 445 -5464
Lines 529058 37532 -491526
Branches 64617 6901 -57716
=============================================
- Hits 92922 1346 -91576
+ Misses 425683 36022 -389661
+ Partials 10453 164 -10289
```
| [Flag](https://app.codecov.io/gh/apache/cloudstack/pull/11931/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [uitests](https://app.codecov.io/gh/apache/cloudstack/pull/11931/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `3.58% <ø> (ø)` | |
| [unittests](https://app.codecov.io/gh/apache/cloudstack/pull/11931/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/cloudstack/pull/11931?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] smoke test: support ceph storage pools [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #11931:
URL: https://github.com/apache/cloudstack/pull/11931#issuecomment-3469459848
<b>[SF] Trillian test result (tid-14742)</b>
Environment: kvm-ubuntu22 (x2), zone: Advanced Networking with Mgmt server u22
Total time taken: 87202 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr11931-t14742-kvm-ubuntu22.zip
Smoke tests completed. 146 look OK, 3 have errors, 0 did not run
Only failed and skipped tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_02_migrate_VM_with_two_data_disks | `Error` | 38.29 | test_vm_life_cycle.py
ContextSuite context=TestVmSnapshot>:teardown | `Error` | 291.59 | test_vm_snapshots.py
test_hostha_enable_ha_when_host_disabled | `Error` | 3.67 | test_hostha_kvm.py
test_hostha_enable_ha_when_host_in_maintenance | `Error` | 305.91 | test_hostha_kvm.py
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] smoke test: support ceph storage pools [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #11931:
URL: https://github.com/apache/cloudstack/pull/11931#issuecomment-3896335932
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 16825
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] smoke test: support ceph storage pools [cloudstack]
Posted by "sureshanaparti (via GitHub)" <gi...@apache.org>.
sureshanaparti commented on PR #11931:
URL: https://github.com/apache/cloudstack/pull/11931#issuecomment-3895822274
@blueorangutan package
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] smoke test: support ceph storage pools [cloudstack]
Posted by "blueorangutan (via GitHub) via commits" <co...@cloudstack.apache.org>.
blueorangutan commented on PR #11931:
URL: https://github.com/apache/cloudstack/pull/11931#issuecomment-6564135110
@nvazquez a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] smoke test: support ceph storage pools [cloudstack]
Posted by "nvazquez (via GitHub)" <gi...@apache.org>.
nvazquez commented on PR #11931:
URL: https://github.com/apache/cloudstack/pull/11931#issuecomment-6458971341
Hi @weizhouapache can you please check if the Copilot comments are relevant?
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] smoke test: support ceph storage pools [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #11931:
URL: https://github.com/apache/cloudstack/pull/11931#issuecomment-3462725463
@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
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] smoke test: support ceph storage pools [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #11931:
URL: https://github.com/apache/cloudstack/pull/11931#issuecomment-3469801258
@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.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] smoke test: support ceph storage pools [cloudstack]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] smoke test: support ceph storage pools [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on code in PR #11931:
URL: https://github.com/apache/cloudstack/pull/11931#discussion_r4398546542
##########
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:
addressed the line too
```
secret_type = virshxml_root.findall(".devices/disk/encryption/secret[@type='passphrase']")
self.assertGreater(len(secret_type), 0,
"No encryption secret of type passphrase in the domain XML of %s" % instance_name)
```
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] smoke test: support ceph storage pools [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on code in PR #11931:
URL: https://github.com/apache/cloudstack/pull/11931#discussion_r4398396735
##########
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:
it is not considered in the orignal code
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] smoke test: support ceph storage pools [cloudstack]
Posted by "nvazquez (via GitHub)" <gi...@apache.org>.
nvazquez commented on PR #11931:
URL: https://github.com/apache/cloudstack/pull/11931#issuecomment-6467148802
@blueorangutan package
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] smoke test: support ceph storage pools [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #11931:
URL: https://github.com/apache/cloudstack/pull/11931#issuecomment-6467162479
@nvazquez a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org