Copilot commented on code in PR #12812:
URL: https://github.com/apache/cloudstack/pull/12812#discussion_r2929998718
##########
test/integration/component/maint/test_redundant_router_deployment_planning.py:
##########
@@ -636,142 +638,144 @@ def test_RvR_multiprimarystorage(self):
self.apiclient.updateCluster(cmd)
self.debug("Enabled first cluster for testing..")
- # Creating network using the network offering created
- self.debug("Creating network with network offering: %s" %
- self.network_offering.id)
- network = Network.create(
- self.apiclient,
- self.services["network"],
- accountid=self.account.name,
- domainid=self.account.domainid,
- networkofferingid=self.network_offering.id,
- zoneid=self.zone.id
- )
- self.debug("Created network with ID: %s" % network.id)
-
- networks = Network.list(
- self.apiclient,
- id=network.id,
- listall=True
- )
- self.assertEqual(
- isinstance(networks, list),
- True,
- "List networks should return a valid response for created network"
- )
- nw_response = networks[0]
-
- self.debug("Network state: %s" % nw_response.state)
- self.assertEqual(
- nw_response.state,
- "Allocated",
- "The network should be in allocated state after creation"
- )
-
- self.debug("Listing routers for network: %s" % network.name)
- routers = Router.list(
- self.apiclient,
- networkid=network.id,
- listall=True
- )
- self.assertEqual(
- routers,
- None,
- "Routers should not be spawned when network is in allocated state"
- )
-
- self.debug("Retrieving the list of hosts in the cluster")
- hosts = Host.list(
- self.apiclient,
- clusterid=enabled_cluster.id,
- listall=True
- )
- self.assertEqual(
- isinstance(hosts, list),
- True,
- "List hosts should not return an empty response"
- )
- host = hosts[0]
-
- self.debug("Deploying VM in account: %s" % self.account.name)
-
- # Spawn an instance in that network
- virtual_machine = VirtualMachine.create(
- self.apiclient,
- self.services["virtual_machine"],
- accountid=self.account.name,
- domainid=self.account.domainid,
- serviceofferingid=self.service_offering.id,
- networkids=[str(network.id)],
- hostid=host.id
- )
- self.debug("Deployed VM in network: %s" % network.id)
+ try:
+ # Creating network using the network offering created
+ self.debug("Creating network with network offering: %s" %
+
self.network_offering.id)
+ network = Network.create(
+ self.apiclient,
+ self.services["network"],
+ accountid=self.account.name,
+ domainid=self.account.domainid,
+ networkofferingid=self.network_offering.id,
+ zoneid=self.zone.id
+ )
+ self.debug("Created network with ID: %s" % network.id)
+
+ networks = Network.list(
+ self.apiclient,
+ id=network.id,
+ listall=True
+ )
+ self.assertEqual(
+ isinstance(networks, list),
+ True,
+ "List networks should return a valid response for created
network"
+ )
+ nw_response = networks[0]
+
+ self.debug("Network state: %s" % nw_response.state)
+ self.assertEqual(
+ nw_response.state,
+ "Allocated",
+ "The network should be in allocated state after
creation"
+ )
- vms = VirtualMachine.list(
+ self.debug("Listing routers for network: %s" % network.name)
+ routers = Router.list(
self.apiclient,
- id=virtual_machine.id,
+ networkid=network.id,
listall=True
)
- self.assertEqual(
- isinstance(vms, list),
- True,
- "List Vms should return a valid list"
- )
- vm = vms[0]
- self.assertEqual(
- vm.state,
- "Running",
- "Vm should be in running state after deployment"
- )
-
- self.debug("Listing routers for network: %s" % network.name)
- routers = Router.list(
+ self.assertEqual(
+ routers,
+ None,
+ "Routers should not be spawned when network is in allocated
state"
+ )
+
+ self.debug("Retrieving the list of hosts in the cluster")
+ hosts = Host.list(
self.apiclient,
- networkid=network.id,
+ clusterid=enabled_cluster.id,
listall=True
)
- self.assertEqual(
- isinstance(routers, list),
- True,
- "list router should return Primary and backup routers"
- )
- self.assertEqual(
- len(routers),
- 2,
- "Length of the list router should be 2 (Backup & Primary)"
- )
- self.assertNotEqual(
- routers[0].hostid,
- routers[1].hostid,
- "Both the routers should be in different storage pools"
- )
- self.debug("Enabling remaining pods if any..")
- pods = Pod.list(
- self.apiclient,
- zoneid=self.zone.id,
- listall=True,
- allocationstate="Disabled"
+ self.assertEqual(
+ isinstance(hosts, list),
+ True,
+ "List hosts should not return an empty response"
+ )
+ host = hosts[0]
+
+ self.debug("Deploying VM in account: %s" % self.account.name)
+
+ # Spawn an instance in that network
+ virtual_machine = VirtualMachine.create(
+ self.apiclient,
+ self.services["virtual_machine"],
+ accountid=self.account.name,
+ domainid=self.account.domainid,
+
serviceofferingid=self.service_offering.id,
+ networkids=[str(network.id)],
+ hostid=host.id
+ )
+ self.debug("Deployed VM in network: %s" % network.id)
+
+ vms = VirtualMachine.list(
+ self.apiclient,
+ id=virtual_machine.id,
+ listall=True
+ )
+ self.assertEqual(
+ isinstance(vms, list),
+ True,
+ "List Vms should return a valid list"
+ )
+ vm = vms[0]
+ self.assertEqual(
+ vm.state,
+ "Running",
+ "Vm should be in running state after deployment"
+ )
+
+ self.debug("Listing routers for network: %s" % network.name)
+ routers = Router.list(
+ self.apiclient,
+ networkid=network.id,
+ listall=True
+ )
+ self.assertEqual(
+ isinstance(routers, list),
+ True,
+ "list router should return Primary and backup routers"
)
- if pods is not None:
- for pod in pods:
- cmd = updatePod.updatePodCmd()
- cmd.id = pod.id
- cmd.allocationstate = 'Enabled'
- self.apiclient.updatePod(cmd)
-
- clusters = Cluster.list(
- self.apiclient,
- allocationstate="Disabled",
- podid=enabled_pod.id,
- listall=True
+ self.assertEqual(
+ len(routers),
+ 2,
+ "Length of the list router should be 2 (Backup &
Primary)"
+ )
+ self.assertNotEqual(
+ routers[0].hostid,
+ routers[1].hostid,
+ "Both the routers should be in different storage pools"
)
-
- if clusters is not None:
- for cluster in clusters:
- cmd = updateCluster.updateClusterCmd()
- cmd.id = cluster.id
+ finally:
+ self.debug("Enabling remaining pods if any..")
+ pods = Pod.list(
+ self.apiclient,
+ zoneid=self.zone.id,
+ listall=True,
+ allocationstate="Disabled"
+ )
+ if pods is not None:
+ for pod in pods:
+ cmd = updatePod.updatePodCmd()
+ cmd.id = pod.id
cmd.allocationstate = 'Enabled'
- self.apiclient.updateCluster(cmd)
+ self.apiclient.updatePod(cmd)
+
+ clusters = Cluster.list(
+ self.apiclient,
+ allocationstate="Disabled",
+ podid=enabled_pod.id,
+ listall=True
+ )
+
+ if clusters is not None:
+ for cluster in clusters:
+ cmd = updateCluster.updateClusterCmd()
+ cmd.id = cluster.id
+ cmd.allocationstate = 'Enabled'
+ self.apiclient.updateCluster(cmd)
Review Comment:
The loop body under `for cluster in clusters:` is over-indented, which hurts
readability and can violate Python style/linting (even if it still executes).
Align the indentation of lines 775–778 to a single level inside the `for` block.
##########
test/integration/component/maint/test_redundant_router_deployment_planning.py:
##########
@@ -636,142 +638,144 @@ def test_RvR_multiprimarystorage(self):
self.apiclient.updateCluster(cmd)
self.debug("Enabled first cluster for testing..")
- # Creating network using the network offering created
- self.debug("Creating network with network offering: %s" %
- self.network_offering.id)
- network = Network.create(
- self.apiclient,
- self.services["network"],
- accountid=self.account.name,
- domainid=self.account.domainid,
- networkofferingid=self.network_offering.id,
- zoneid=self.zone.id
- )
- self.debug("Created network with ID: %s" % network.id)
-
- networks = Network.list(
- self.apiclient,
- id=network.id,
- listall=True
- )
- self.assertEqual(
- isinstance(networks, list),
- True,
- "List networks should return a valid response for created network"
- )
- nw_response = networks[0]
-
- self.debug("Network state: %s" % nw_response.state)
- self.assertEqual(
- nw_response.state,
- "Allocated",
- "The network should be in allocated state after creation"
- )
-
- self.debug("Listing routers for network: %s" % network.name)
- routers = Router.list(
- self.apiclient,
- networkid=network.id,
- listall=True
- )
- self.assertEqual(
- routers,
- None,
- "Routers should not be spawned when network is in allocated state"
- )
-
- self.debug("Retrieving the list of hosts in the cluster")
- hosts = Host.list(
- self.apiclient,
- clusterid=enabled_cluster.id,
- listall=True
- )
- self.assertEqual(
- isinstance(hosts, list),
- True,
- "List hosts should not return an empty response"
- )
- host = hosts[0]
-
- self.debug("Deploying VM in account: %s" % self.account.name)
-
- # Spawn an instance in that network
- virtual_machine = VirtualMachine.create(
- self.apiclient,
- self.services["virtual_machine"],
- accountid=self.account.name,
- domainid=self.account.domainid,
- serviceofferingid=self.service_offering.id,
- networkids=[str(network.id)],
- hostid=host.id
- )
- self.debug("Deployed VM in network: %s" % network.id)
+ try:
+ # Creating network using the network offering created
+ self.debug("Creating network with network offering: %s" %
+
self.network_offering.id)
+ network = Network.create(
+ self.apiclient,
+ self.services["network"],
+ accountid=self.account.name,
+ domainid=self.account.domainid,
+ networkofferingid=self.network_offering.id,
+ zoneid=self.zone.id
+ )
+ self.debug("Created network with ID: %s" % network.id)
+
+ networks = Network.list(
+ self.apiclient,
+ id=network.id,
+ listall=True
+ )
+ self.assertEqual(
+ isinstance(networks, list),
+ True,
+ "List networks should return a valid response for created
network"
+ )
+ nw_response = networks[0]
+
+ self.debug("Network state: %s" % nw_response.state)
+ self.assertEqual(
+ nw_response.state,
+ "Allocated",
+ "The network should be in allocated state after
creation"
+ )
- vms = VirtualMachine.list(
+ self.debug("Listing routers for network: %s" % network.name)
+ routers = Router.list(
self.apiclient,
- id=virtual_machine.id,
+ networkid=network.id,
listall=True
)
- self.assertEqual(
- isinstance(vms, list),
- True,
- "List Vms should return a valid list"
- )
- vm = vms[0]
- self.assertEqual(
- vm.state,
- "Running",
- "Vm should be in running state after deployment"
- )
-
- self.debug("Listing routers for network: %s" % network.name)
- routers = Router.list(
+ self.assertEqual(
+ routers,
+ None,
+ "Routers should not be spawned when network is in allocated
state"
+ )
+
+ self.debug("Retrieving the list of hosts in the cluster")
+ hosts = Host.list(
self.apiclient,
- networkid=network.id,
+ clusterid=enabled_cluster.id,
listall=True
)
- self.assertEqual(
- isinstance(routers, list),
- True,
- "list router should return Primary and backup routers"
- )
- self.assertEqual(
- len(routers),
- 2,
- "Length of the list router should be 2 (Backup & Primary)"
- )
- self.assertNotEqual(
- routers[0].hostid,
- routers[1].hostid,
- "Both the routers should be in different storage pools"
- )
- self.debug("Enabling remaining pods if any..")
- pods = Pod.list(
- self.apiclient,
- zoneid=self.zone.id,
- listall=True,
- allocationstate="Disabled"
+ self.assertEqual(
+ isinstance(hosts, list),
+ True,
+ "List hosts should not return an empty response"
+ )
+ host = hosts[0]
+
+ self.debug("Deploying VM in account: %s" % self.account.name)
+
+ # Spawn an instance in that network
+ virtual_machine = VirtualMachine.create(
+ self.apiclient,
+ self.services["virtual_machine"],
+ accountid=self.account.name,
+ domainid=self.account.domainid,
+
serviceofferingid=self.service_offering.id,
+ networkids=[str(network.id)],
+ hostid=host.id
+ )
+ self.debug("Deployed VM in network: %s" % network.id)
+
+ vms = VirtualMachine.list(
+ self.apiclient,
+ id=virtual_machine.id,
+ listall=True
+ )
+ self.assertEqual(
+ isinstance(vms, list),
+ True,
+ "List Vms should return a valid list"
+ )
+ vm = vms[0]
+ self.assertEqual(
+ vm.state,
+ "Running",
+ "Vm should be in running state after deployment"
+ )
+
+ self.debug("Listing routers for network: %s" % network.name)
+ routers = Router.list(
+ self.apiclient,
+ networkid=network.id,
+ listall=True
+ )
+ self.assertEqual(
+ isinstance(routers, list),
+ True,
+ "list router should return Primary and backup routers"
)
- if pods is not None:
- for pod in pods:
- cmd = updatePod.updatePodCmd()
- cmd.id = pod.id
- cmd.allocationstate = 'Enabled'
- self.apiclient.updatePod(cmd)
-
- clusters = Cluster.list(
- self.apiclient,
- allocationstate="Disabled",
- podid=enabled_pod.id,
- listall=True
+ self.assertEqual(
+ len(routers),
+ 2,
+ "Length of the list router should be 2 (Backup &
Primary)"
+ )
+ self.assertNotEqual(
+ routers[0].hostid,
+ routers[1].hostid,
+ "Both the routers should be in different storage pools"
Review Comment:
The assertion compares `hostid` values, but the failure message claims the
routers should be in different *storage pools*. Either update the message to
reflect the actual check (different hosts), or change the assertion to compare
the appropriate storage/pool identifier field if that’s the intent.
##########
test/integration/smoke/test_public_ip_range.py:
##########
@@ -286,20 +286,22 @@ def base_system_vm(self, services, systemvmtype):
cmd.allocationstate = 'Disabled'
self.apiclient.updateZone(cmd)
- # Delete System VM and IP range, so System VM can get IP from original
ranges
- self.debug("Destroying System VM: %s" % systemvm_id)
- cmd = destroySystemVm.destroySystemVmCmd()
- cmd.id = systemvm_id
- self.apiclient.destroySystemVm(cmd)
-
- domain_id = self.public_ip_range.vlan.domainid
- self.public_ip_range.delete(self.apiclient)
-
- # Enable Zone
- cmd = updateZone.updateZoneCmd()
- cmd.id = self.zone.id
- cmd.allocationstate = 'Enabled'
- self.apiclient.updateZone(cmd)
+ try:
+ # Delete System VM and IP range, so System VM can get IP from
original ranges
+ self.debug("Destroying System VM: %s" % systemvm_id)
+ cmd = destroySystemVm.destroySystemVmCmd()
+ cmd.id = systemvm_id
+ self.apiclient.destroySystemVm(cmd)
+
+ domain_id = self.public_ip_range.vlan.domainid
+ self.public_ip_range.delete(self.apiclient)
+
+ finally:
+ # Enable Zone
+ cmd = updateZone.updateZoneCmd()
+ cmd.id = self.zone.id
+ cmd.allocationstate = 'Enabled'
+ self.apiclient.updateZone(cmd)
Review Comment:
As written, an exception in the `finally` clean-up (e.g., `updateZone`) can
mask the original test failure/exception from the `try` block. Consider making
the clean-up best-effort (catch/log exceptions in `finally`) or preserving the
original exception (store it, attempt cleanup, then re-raise the original) so
failures remain actionable.
--
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]
