From ce1a39841cf1e939a21c9fa9b0dadef71fb7409b Mon Sep 17 00:00:00 2001 From: Gisaldjo Purbollari Date: Tue, 8 Sep 2026 10:27:50 -0400 Subject: [PATCH 1/2] fix(ibm): select custom VPC subnets in the instance zone Subnet discovery for a custom VPC picked the first subnet in the VPC, so instance creation failed when that subnet was in another zone. Select a subnet in the instance zone (explicit, configured, or -1) instead, creating --subnet if none exists. --- VERSION | 2 +- docs/clouds/ibm.md | 5 ++ pycloudlib/ibm/.kb/ibm.md | 1 + pycloudlib/ibm/instance.py | 16 +++--- tests/integration_tests/ibm/test_launch.py | 46 ++++++++++++++++ tests/unit_tests/ibm/test_instance.py | 63 +++++++++++++++++++++- 6 files changed, 124 insertions(+), 9 deletions(-) diff --git a/VERSION b/VERSION index 94acd658..d18358d5 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1!11.1.8 +1!11.1.9 diff --git a/docs/clouds/ibm.md b/docs/clouds/ibm.md index 4ec9a172..5f7db2eb 100644 --- a/docs/clouds/ibm.md +++ b/docs/clouds/ibm.md @@ -107,6 +107,11 @@ A pre-existent VPC can be set in the config file or be passed as argument to the ibm = IBM(vpc="my-custom-vpc", ...) ``` +For a custom VPC, pycloudlib selects a subnet in the requested zone or creates +one there if none exists. If no zone is provided, the zone defaults to +`{region}-1`. Newly created subnets in an existing VPC include the zone in +their name. + Another possibility is to create a custom VPC on the fly, then one can be created and then later used during instance creation. diff --git a/pycloudlib/ibm/.kb/ibm.md b/pycloudlib/ibm/.kb/ibm.md index d422c448..523be85a 100644 --- a/pycloudlib/ibm/.kb/ibm.md +++ b/pycloudlib/ibm/.kb/ibm.md @@ -14,6 +14,7 @@ Read the top-level `.kb/agents.md` file before continuing below. - **The `ibm-vpc` API version is pinned** in `ibm/cloud.py` (`VpcV1(..., version=...)` plus a `set_service_url` per region), and the SDK upper bound is constrained in `pyproject.toml` (`ibm-vpc >= 0.10, < 0.29.0`). When updating: check the SDK releases and update **both** the version string and the `<` upper bound together. - `resource_group_id` and `vpc` are lazy properties (looked up on first access); a missing resource group raises `IBMException`. See `ibm/cloud.py` for the `from_existing`/`from_default` VPC selection. +- An instance's subnet must be in its zone; when resolving a custom VPC, never fall back to a subnet in another zone. - `ibm/_util.py` provides the iteration/wait helpers used across the backend; `ibm/errors.py` defines `IBMException`. - mypy: `ibm_vpc.*`/`ibm_cloud_sdk_core.*`/`ibm_platform_services.*` are in `ignore_missing_imports`; `pycloudlib.ibm.instance` has `check_untyped_defs = false` (TODO overrides in `pyproject.toml`). diff --git a/pycloudlib/ibm/instance.py b/pycloudlib/ibm/instance.py index 0a0c85b3..1f9e9832 100644 --- a/pycloudlib/ibm/instance.py +++ b/pycloudlib/ibm/instance.py @@ -78,15 +78,17 @@ def from_default(cls, client: VpcV1, zone: str, vpc_id: str) -> "_Subnet": return cls.from_existing(client, f"{zone}-default-subnet", vpc_id) @classmethod - def discover(cls, client: VpcV1, vpc_id: str) -> "_Subnet": - """Discover a Subnet within a VPC.""" + def discover(cls, client: VpcV1, vpc_id: str, zone: str) -> "_Subnet": + """Discover a Subnet within a VPC and zone.""" subnet = _get_first( client.list_subnets, resource_name="subnets", - filter_fn=(lambda subnet: subnet["vpc"]["id"] == vpc_id), + filter_fn=( + lambda subnet: subnet["vpc"]["id"] == vpc_id and subnet["zone"]["name"] == zone + ), ) if subnet is None: - raise IBMException(f"No subnet associated to vpc found: {vpc_id}") + raise IBMException(f"No subnet associated to vpc found: {vpc_id} in zone {zone}") return cls(client, subnet) @property @@ -200,7 +202,7 @@ def from_existing( ) -> "VPC": """Find a VPC by name. - Try to discover a Subnet within it or create it if not found. + Try to discover a Subnet within it in the requested zone or create it if not found. """ vpc = _get_first( client.list_vpcs, @@ -211,11 +213,11 @@ def from_existing( raise IBMException(f"VPC not found: {name}") try: - subnet = _Subnet.discover(client, vpc_id=vpc["id"]) + subnet = _Subnet.discover(client, vpc_id=vpc["id"], zone=zone) except IBMException: subnet = _Subnet.create( client, - name=f"{name}-subnet", + name=f"{name}-{zone}-subnet", zone=zone, resource_group_id=resource_group_id, vpc_id=vpc["id"], diff --git a/tests/integration_tests/ibm/test_launch.py b/tests/integration_tests/ibm/test_launch.py index ffe6dc08..c22f784b 100644 --- a/tests/integration_tests/ibm/test_launch.py +++ b/tests/integration_tests/ibm/test_launch.py @@ -5,6 +5,8 @@ from google.cloud import compute_v1 import time +from pycloudlib.ibm._util import iter_resources + @pytest.fixture def ibm_cloud(): @@ -37,6 +39,50 @@ def manage_ssh_key(ibm: IBM, key_name): ) +def configured_vpc_subnets(ibm_cloud: IBM): + """Return subnets for the configured custom VPC.""" + vpc_name = ibm_cloud.config.get("vpc") + if not vpc_name: + pytest.skip("requires a custom VPC in the IBM test config") + + vpc = next( + ( + candidate + for candidate in iter_resources( + ibm_cloud._client.list_vpcs, + resource_name="vpcs", + ) + if candidate["name"] == vpc_name + ), + None, + ) + assert vpc is not None, f"Configured VPC not found: {vpc_name}" + return list( + iter_resources( + ibm_cloud._client.list_subnets, + resource_name="subnets", + filter_fn=lambda subnet: subnet["vpc"]["id"] == vpc["id"], + ) + ) + + +def test_ibm_custom_vpc_selects_subnet_in_zone(ibm_cloud: IBM): + """Select a subnet in the cloud's zone, not the first subnet listed.""" + subnets = configured_vpc_subnets(ibm_cloud) + matching_subnet = next( + (subnet for subnet in subnets if subnet["zone"]["name"] == ibm_cloud.zone), + None, + ) + if matching_subnet is None: + pytest.skip("requires an existing matching subnet to avoid creating resources") + if subnets[0]["id"] == matching_subnet["id"]: + pytest.skip("first listed subnet must be in another zone to reproduce the bug") + + selected_subnet = ibm_cloud._client.get_subnet(ibm_cloud.vpc.subnet_id).get_result() + + assert selected_subnet["id"] == matching_subnet["id"] + + def test_ibm_launch(ibm_cloud: IBM): """ Test launching an IBM instance. diff --git a/tests/unit_tests/ibm/test_instance.py b/tests/unit_tests/ibm/test_instance.py index ea573a07..7b8352d7 100644 --- a/tests/unit_tests/ibm/test_instance.py +++ b/tests/unit_tests/ibm/test_instance.py @@ -3,7 +3,7 @@ import pytest from unittest import mock -from pycloudlib.ibm.instance import IBMInstance, _IBMInstanceType, _Status +from pycloudlib.ibm.instance import IBMInstance, VPC, _IBMInstanceType, _Status SAMPLE_RAW_INSTANCE = { "id": "ibm1", @@ -12,6 +12,67 @@ "zone": {"name": "zone1"}, } M_PATH = "pycloudlib.ibm.instance." +VPC_ID = "vpc-1" + + +def _subnet(subnet_id, zone): + return { + "id": subnet_id, + "vpc": {"id": VPC_ID}, + "zone": {"name": zone}, + } + + +def _client_with_subnets(*subnets): + client = mock.Mock() + client.list_vpcs.return_value.get_result.return_value = { + "vpcs": [{"id": VPC_ID, "name": "custom-vpc"}] + } + client.list_subnets.return_value.get_result.return_value = {"subnets": list(subnets)} + return client + + +def _existing_vpc(client, **kwargs): + return VPC.from_existing( + None, + client=client, + name="custom-vpc", + resource_group_id="resource-group-id", + zone="us-south-1", + **kwargs, + ) + + +class TestVPC: + def test_from_existing_selects_subnet_by_zone(self): + """Select a custom VPC subnet in the configured zone.""" + client = _client_with_subnets( + _subnet("subnet-wrong-zone", "us-south-2"), + _subnet("subnet-requested-zone", "us-south-1"), + ) + + vpc = _existing_vpc(client) + + assert vpc.subnet_id == "subnet-requested-zone" + client.create_subnet.assert_not_called() + + def test_from_existing_creates_expected_subnet(self): + """Create a subnet in the resolved zone when none matches.""" + client = _client_with_subnets(_subnet("other-zone-subnet", "us-south-2")) + client.create_subnet.return_value.get_result.return_value = {"id": "created-subnet"} + + vpc = _existing_vpc(client) + + assert vpc.subnet_id == "created-subnet" + client.create_subnet.assert_called_once_with( + { + "name": "custom-vpc-us-south-1-subnet", + "resource_group": {"id": "resource-group-id"}, + "vpc": {"id": VPC_ID}, + "total_ipv4_address_count": 256, + "zone": {"name": "us-south-1"}, + } + ) class TestIBMInstance: From 43ebe1742dc6d832e3136a5bc1aebda9cc82f704 Mon Sep 17 00:00:00 2001 From: Gisaldjo Purbollari Date: Tue, 6 Oct 2026 09:45:36 -0400 Subject: [PATCH 2/2] fix(ibm): clean up subnets created in existing VPCs When a custom VPC has no subnet in the instance zone, one is created in it. That VPC is not tracked for deletion, so the subnet was left behind after clean(). Track these subnets in created_subnets and delete them in clean(), leaving the VPC and its other subnets intact. --- pycloudlib/ibm/.kb/ibm.md | 2 +- pycloudlib/ibm/cloud.py | 21 +++++++-- pycloudlib/ibm/instance.py | 10 ++++ tests/unit_tests/ibm/test_cloud.py | 75 ++++++++++++++++++++++++++++++ 4 files changed, 104 insertions(+), 4 deletions(-) diff --git a/pycloudlib/ibm/.kb/ibm.md b/pycloudlib/ibm/.kb/ibm.md index 523be85a..3547aa34 100644 --- a/pycloudlib/ibm/.kb/ibm.md +++ b/pycloudlib/ibm/.kb/ibm.md @@ -22,4 +22,4 @@ Read the top-level `.kb/agents.md` file before continuing below. # Architecture - `IAMAuthenticator(api_key)` authenticates both `VpcV1` and `ResourceManagerV2`. The `VPC` helper (in `ibm/instance.py`) pairs the IBM VPC resource with the resolved resource-group id, region, and zone; floating IPs are selected by `floating_ip_substring` when provided. -- `clean()` extends `BaseCloud.clean()` to tear down `created_vpcs`/`created_keys`. \ No newline at end of file +- `clean()` extends `BaseCloud.clean()` to tear down `created_subnets`/`created_vpcs`/`created_keys`. \ No newline at end of file diff --git a/pycloudlib/ibm/cloud.py b/pycloudlib/ibm/cloud.py index d0651193..e0ba911e 100644 --- a/pycloudlib/ibm/cloud.py +++ b/pycloudlib/ibm/cloud.py @@ -18,7 +18,7 @@ from pycloudlib.ibm._util import iter_resources as _iter_resources from pycloudlib.ibm._util import wait_until as _wait_until from pycloudlib.ibm.errors import IBMException -from pycloudlib.ibm.instance import VPC, IBMInstance +from pycloudlib.ibm.instance import VPC, IBMInstance, _Subnet from pycloudlib.instance import BaseInstance from pycloudlib.util import UBUNTU_RELEASE_VERSION_MAP @@ -56,6 +56,7 @@ def __init__( required_values=[resource_group, api_key, region], ) self.created_vpcs: List[VPC] = [] + self.created_subnets: List[_Subnet] = [] self.created_keys: List[str] = [] self._resource_group = ( @@ -108,7 +109,13 @@ def vpc(self) -> VPC: "zone": self.zone, } if self._vpc_name is not None: - self._vpc = VPC.from_existing(self.key_pair, name=self._vpc_name, **kwargs) + self._vpc = VPC.from_existing( + self.key_pair, + name=self._vpc_name, + **kwargs, + ) + if self._vpc.created_subnet is not None: + self.created_subnets.append(self._vpc.created_subnet) else: self._vpc = VPC.from_default(self.key_pair, **kwargs) @@ -244,11 +251,14 @@ def get_or_create_vpc(self, name: str) -> VPC: "zone": self.zone, } try: - return VPC.from_existing(*args, **kwargs) + vpc = VPC.from_existing(*args, **kwargs) except IBMException: vpc = VPC.create(*args, **kwargs) self.created_vpcs.append(vpc) return vpc + if vpc.created_subnet is not None: + self.created_subnets.append(vpc.created_subnet) + return vpc def launch( self, @@ -409,6 +419,11 @@ def clean(self) -> List[Exception]: # Not cleaning up floating ips here because they're 1:1 # with an instance and get cleaned up by the instance exceptions = super().clean() + for subnet in self.created_subnets: + try: + subnet.delete() + except Exception as error: + exceptions.append(error) for vpc in self.created_vpcs: try: vpc.delete() diff --git a/pycloudlib/ibm/instance.py b/pycloudlib/ibm/instance.py index 1f9e9832..74bc45a8 100644 --- a/pycloudlib/ibm/instance.py +++ b/pycloudlib/ibm/instance.py @@ -135,6 +135,7 @@ def __init__( vpc: dict, resource_group_id: str, subnet: Optional[_Subnet] = None, + created_subnet: Optional[_Subnet] = None, **_kwargs, ): """Init a `VPC`.""" @@ -142,6 +143,7 @@ def __init__( self._client = client self._vpc = vpc self._subnet = subnet + self._created_subnet = created_subnet self._resource_group_id = resource_group_id @classmethod @@ -212,6 +214,7 @@ def from_existing( if vpc is None: raise IBMException(f"VPC not found: {name}") + created_subnet = None try: subnet = _Subnet.discover(client, vpc_id=vpc["id"], zone=zone) except IBMException: @@ -222,6 +225,7 @@ def from_existing( resource_group_id=resource_group_id, vpc_id=vpc["id"], ) + created_subnet = subnet return cls( *args, @@ -229,6 +233,7 @@ def from_existing( vpc=vpc, resource_group_id=resource_group_id, subnet=subnet, + created_subnet=created_subnet, **kwargs, ) @@ -279,6 +284,11 @@ def subnet_id(self) -> str: raise IBMException("No subnet available") return self._subnet.id + @property + def created_subnet(self) -> Optional[_Subnet]: + """Subnet created in an existing VPC, or None if one was reused.""" + return self._created_subnet + def delete(self) -> None: """Delete VPC. diff --git a/tests/unit_tests/ibm/test_cloud.py b/tests/unit_tests/ibm/test_cloud.py index 81ef8ac3..ef9e5a3e 100644 --- a/tests/unit_tests/ibm/test_cloud.py +++ b/tests/unit_tests/ibm/test_cloud.py @@ -5,6 +5,7 @@ from unittest import mock import pytest +from ibm_cloud_sdk_core import ApiException from pycloudlib.errors import InvalidTagNameError, PycloudlibTimeoutError from pycloudlib.ibm.cloud import ( @@ -47,3 +48,77 @@ def test_validate_tag(tag: str, rules_failed: List[str]): assert tag in str(exc_info.value) for rule in rules_failed: assert rule in str(exc_info.value) + + +@pytest.mark.mock_ssh_keys +class TestIBM: + @pytest.fixture + def cloud(self): + with mock.patch("pycloudlib.ibm.cloud.VpcV1"): + cloud = IBM( + tag="test-subnet-cleanup", + api_key="api-key", + resource_group="resource-group", + region="us-south", + vpc="custom-vpc", + ) + cloud._resource_group_id = "resource-group-id" + cloud._client.list_vpcs.return_value.get_result.return_value = { + "vpcs": [{"id": "vpc-id", "name": "custom-vpc"}] + } + return cloud + + @pytest.mark.parametrize("lookup", ["vpc", "get_or_create_vpc"]) + @pytest.mark.parametrize("matching_subnet", [True, False]) + def test_clean_custom_vpc_subnets(self, cloud, lookup, matching_subnet): + """clean() deletes only a subnet created in an existing VPC.""" + client = cloud._client + client.list_subnets.return_value.get_result.return_value = { + "subnets": [ + { + "id": "existing-subnet", + "vpc": {"id": "vpc-id"}, + "zone": {"name": "us-south-1" if matching_subnet else "us-south-2"}, + } + ] + } + client.create_subnet.return_value.get_result.return_value = {"id": "created-subnet"} + client.get_subnet.side_effect = ApiException(404) + + vpc = cloud.vpc if lookup == "vpc" else cloud.get_or_create_vpc("custom-vpc") + + assert vpc.subnet_id == ("existing-subnet" if matching_subnet else "created-subnet") + assert cloud.clean() == [] + assert len(cloud.created_subnets) == (0 if matching_subnet else 1) + if matching_subnet: + client.create_subnet.assert_not_called() + client.delete_subnet.assert_not_called() + else: + client.delete_subnet.assert_called_once_with("created-subnet") + client.delete_vpc.assert_not_called() + + def test_clean_continues_after_subnet_failure(self, cloud): + """A failed subnet deletion is reported and the rest of cleanup still runs.""" + manager = mock.Mock() + instance = mock.Mock() + subnet = mock.Mock() + vpc = mock.Mock() + error = RuntimeError("subnet deletion failed") + subnet.delete.side_effect = error + cloud.created_instances.append(instance) + cloud.created_subnets.append(subnet) + cloud.created_vpcs.append(vpc) + cloud.created_keys.append("key-id") + manager.attach_mock(instance, "instance") + manager.attach_mock(subnet, "subnet") + manager.attach_mock(vpc, "vpc") + manager.attach_mock(cloud._client, "client") + + assert cloud.clean() == [error] + assert cloud.created_subnets == [subnet] + assert manager.mock_calls == [ + mock.call.instance.delete(), + mock.call.subnet.delete(), + mock.call.vpc.delete(), + mock.call.client.delete_key("key-id"), + ]