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..3547aa34 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`). @@ -21,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 0a0c85b3..74bc45a8 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 @@ -133,6 +135,7 @@ def __init__( vpc: dict, resource_group_id: str, subnet: Optional[_Subnet] = None, + created_subnet: Optional[_Subnet] = None, **_kwargs, ): """Init a `VPC`.""" @@ -140,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 @@ -200,7 +204,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, @@ -210,16 +214,18 @@ 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"]) + 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"], ) + created_subnet = subnet return cls( *args, @@ -227,6 +233,7 @@ def from_existing( vpc=vpc, resource_group_id=resource_group_id, subnet=subnet, + created_subnet=created_subnet, **kwargs, ) @@ -277,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/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_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"), + ] 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: