Copilot commented on code in PR #14362:
URL: https://github.com/apache/cloudstack/pull/14362#discussion_r4219076465
##########
test/integration/smoke/test_vm_lifecycle_with_snapshot_or_volume.py:
##########
@@ -177,16 +195,16 @@ def
test_03_deploy_vm_with_existing_volume_deleted_template(self):
template = Template.create_from_snapshot(self.apiclient,
self.snapshot, services)
self._cleanup.append(template)
virtual_machine = VirtualMachine.create(self.apiclient,
- {"name": "Test-%s" %
uuid.uuid4()},
+ self.services['small'],
accountid=self.account.name,
domainid=self.account.domainid,
zoneid=self.zone.id,
serviceofferingid=self.service_offering.id,
templateid=template.id,
- mode="basic",
+ mode=self.services['mode'],
)
Review Comment:
Replacing the per-VM unique `{"name": "Test-%s" % uuid.uuid4()}` with a
shared `self.services['small']` dict risks non-unique VM names across runs (and
potential mutation/side-effects if `VirtualMachine.create` writes into the
passed dict). Consider copying the service dict for each create and overriding
`name` (and any other identity fields) to keep tests isolated and deterministic.
##########
test/integration/smoke/test_vm_lifecycle_with_snapshot_or_volume.py:
##########
@@ -40,11 +40,19 @@
from marvin.codes import FAILED, PASS
from nose.plugins.attrib import attr
-import uuid
import unittest
class TestDeployVMFromSnapshotOrVolume(cloudstackTestCase):
+ def rbd_snapshot_cannot_outlive_source_volume(self):
+ '''
+ On RBD, a snapshot lives on primary storage with its parent
+ volume, so with snapshot.backup.to.secondary=false, deleting the
+ parent volume destroys the snapshot too - it can no longer be
+ used to deploy a new volume/VM afterward.
+ '''
+ return self.storage_pool.type == 'RBD'
Review Comment:
`setUpClass` uses `getattr(pool, "type", None)` when filtering candidate
pools, but this helper method accesses `self.storage_pool.type` directly. If
the returned pool object lacks a `type` attribute (or it’s `None`), this will
raise `AttributeError` and hard-fail the test. Use `getattr(self.storage_pool,
"type", None)` (and optionally normalize case) to make the check consistent and
safe.
##########
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##########
@@ -7011,8 +7011,9 @@ private VolumeInfo getVolume(long id, Long templateId,
boolean isSnapshot) {
if (volume != null) {
if (!isSnapshot
&& (volume.getDataStore() == null
- ||
!ScopeType.ZONE.equals(volume.getDataStore().getScope().getScopeType()))) {
- throw new InvalidParameterValueException("Deployment of
virtual machine is supported only for Zone-wide storage pools");
+ ||
(!ScopeType.ZONE.equals(volume.getDataStore().getScope().getScopeType())
+ &&
!ScopeType.CLUSTER.equals(volume.getDataStore().getScope().getScopeType())))) {
+ throw new InvalidParameterValueException("Deployment of
virtual machine is supported only for Zone-wide or Cluster-wide storage pools");
Review Comment:
This change introduces a new supported behavior (accept `ScopeType.CLUSTER`
for deploy-from-volume), but the updated unit tests shown only assert the
updated error message for failure paths. Add a positive unit test that verifies
VM deployment from a volume on a CLUSTER-scoped primary store succeeds (i.e.,
does not throw `InvalidParameterValueException`) to prevent regressions.
##########
test/integration/smoke/test_vm_lifecycle_with_snapshot_or_volume.py:
##########
@@ -80,23 +88,33 @@ def setUpClass(cls):
cls._cleanup.append(cls.account)
cls.debug(cls.account.id)
+ # Prefer a zone-wide pool, but also accept a cluster-scope NFS/RBD
pool.
storage_pools_response = list_storage_pools(cls.apiclient,
zoneid=cls.zone.id,
scope="ZONE")
+ if not storage_pools_response:
+ cluster_storage_pools = list_storage_pools(cls.apiclient,
+ zoneid=cls.zone.id,
+ scope="CLUSTER") or []
+ storage_pools_response = [
+ pool for pool in cluster_storage_pools
+ if getattr(pool, "type", None) in ("NetworkFilesystem", "RBD")
+ ]
+
if storage_pools_response:
- cls.zone_wide_storage = storage_pools_response[0]
+ cls.storage_pool = storage_pools_response[0]
Review Comment:
The CLUSTER-scope path filters to `("NetworkFilesystem", "RBD")`, but the
ZONE-scope path takes the first pool unfiltered. In mixed environments, this
can select a ZONE-wide pool type that doesn’t match the assumptions of these
tests and lead to brittle failures rather than a clear skip. Consider applying
the same pool-type filtering to the ZONE-scope result (or otherwise preferring
known-supported types) before selecting `storage_pools_response[0]`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
