From 39136ae72ba93314a2b254bfb7288979ef484289 Mon Sep 17 00:00:00 2001 From: John Garbutt Date: Mon, 28 Apr 2025 16:20:15 +0100 Subject: [PATCH 01/13] Test operator calls purge --- capi_janitor/openstack/operator.py | 33 +++++--- capi_janitor/tests/openstack/test_operator.py | 75 +++++++++++++++++++ 2 files changed, 99 insertions(+), 9 deletions(-) diff --git a/capi_janitor/openstack/operator.py b/capi_janitor/openstack/operator.py index e862813..4ebb5b4 100644 --- a/capi_janitor/openstack/operator.py +++ b/capi_janitor/openstack/operator.py @@ -389,14 +389,13 @@ async def _on_openstackcluster_event_impl( # Get the cloud credential from the cluster and use it to delete dangling # resources created by OpenStack integrations on the cluster - secrets = await ekclient.api("v1").resource("secrets") - try: - clouds_secret = await secrets.fetch( - spec["identityRef"]["name"], namespace=namespace - ) - except easykube.ApiError as exc: - if exc.status_code != 404: - raise + clouds_secret = await _get_clouds_secret( + spec["identityRef"]["name"], namespace=namespace + ) + if clouds_secret is None: + # TODO(johngarbutt): fail better when secret not found? + logger.error(f"clouds.yaml not found for: {clustername}") + else: clouds = yaml.safe_load(base64.b64decode(clouds_secret.data["clouds.yaml"])) if "cacert" in clouds_secret.data: @@ -424,6 +423,7 @@ async def _on_openstackcluster_event_impl( CREDENTIAL_ANNOTATION ) remove_appcred = credential_annotation_value == CREDENTIAL_ANNOTATION_DELETE + await purge_openstack_resources( logger, clouds, @@ -436,7 +436,7 @@ async def _on_openstackcluster_event_impl( # If we get to here, OpenStack resources have been successfully deleted # So we can remove the appcred secret if we are the last actor if remove_appcred and len(finalizers) == 1: - await secrets.delete(clouds_secret.metadata.name, namespace=namespace) + await _delete_secret(clouds_secret.metadata["name"], namespace) logger.info("cloud credential secret deleted") elif remove_appcred: # If the annotation says delete but other controllers are still acting, go round again @@ -451,7 +451,22 @@ async def _on_openstackcluster_event_impl( logger.info("removed janitor finalizer from cluster") +async def _delete_secret(name, namespace): + secrets = await ekclient.api("v1").resource("secrets") + await secrets.delete(name, namespace=namespace) + + async def _get_os_cluster_client(): capoapi = await ekclient.api_preferred_version(CAPO_API_GROUP) openstackclusters = await capoapi.resource("openstackclusters") return openstackclusters + + +async def _get_clouds_secret(secret_name, namespace): + secrets = await ekclient.api("v1").resource("secrets") + try: + return await secrets.fetch(secret_name, namespace=namespace) + except easykube.ApiError as exc: + if exc.status_code != 404: + raise + # TODO(johngarbutt): fail better when not found? diff --git a/capi_janitor/tests/openstack/test_operator.py b/capi_janitor/tests/openstack/test_operator.py index 04b06a3..329c085 100644 --- a/capi_janitor/tests/openstack/test_operator.py +++ b/capi_janitor/tests/openstack/test_operator.py @@ -1,3 +1,5 @@ +import base64 +import yaml import unittest from unittest import mock @@ -67,3 +69,76 @@ async def test_on_openstackcluster_event_skip_no_finalizers( logger.info.assert_has_calls( [mock.call("janitor finalizer not present, skipping cleanup")] ) + + @mock.patch.object(operator, "_delete_secret") + @mock.patch.object(operator, "_get_clouds_secret") + @mock.patch.object(operator, "purge_openstack_resources") + @mock.patch.object(operator, "patch_finalizers") + @mock.patch.object(operator, "_get_os_cluster_client") + async def test_on_openstackcluster_event_calls_purge( + self, + mock_get_client, + mock_patch_finalizers, + mock_purge, + mock_clouds_secret, + mock_delete_secret, + ): + logger = mock.Mock() + mock_get_client.return_value = "mock_client" + mock_secret = mock.Mock() + mock_clouds_secret.return_value = mock_secret + clouds_yaml_data = { + "openstack": { + "auth": { + "auth_url": "https://example.com:5000/v3", + "username": "user", + "password": "pass", + "project_name": "project", + "user_domain_name": "Default", + "project_domain_name": "Default", + } + } + } + mock_secret.data = { + "clouds.yaml": base64.b64encode(yaml.dump(clouds_yaml_data).encode("utf-8")) + } + mock_secret.metadata = { + "annotations": {"janitor.capi.stackhpc.com/credential-policy": "delete"}, + "name": "appcred42", + } + + await operator._on_openstackcluster_event_impl( + name="mycluster", + namespace="namespace1", + meta={ + "deletionTimestamp": "2023-10-01T00:00:00Z", + "finalizers": ["janitor.capi.stackhpc.com"], + "annotations": { + "janitor.capi.stackhpc.com/volumes-policy": "delete", + }, + }, + labels={}, + spec={"identityRef": {"name": "appcred42"}}, + logger=logger, + body={}, + ) + + mock_purge.assert_awaited_once_with( + logger, + clouds_yaml_data, + "openstack", + None, + "mycluster", + True, + True, + ) + mock_delete_secret.assert_awaited_once_with("appcred42", "namespace1") + mock_patch_finalizers.assert_awaited_once_with( + "mock_client", "mycluster", "namespace1", [] + ) + logger.debug.assert_has_calls( + [mock.call("cluster name that will be used for cleanup: 'mycluster'")] + ) + logger.info.assert_has_calls( + [mock.call("janitor finalizer not present, skipping cleanup")] + ) From 2a3abb06b7107b851e22154a5875e4c3e2537675 Mon Sep 17 00:00:00 2001 From: bertiethorpe Date: Mon, 28 Apr 2025 16:39:02 +0100 Subject: [PATCH 02/13] fix logging assertions --- capi_janitor/tests/openstack/test_operator.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/capi_janitor/tests/openstack/test_operator.py b/capi_janitor/tests/openstack/test_operator.py index 329c085..e98c7e0 100644 --- a/capi_janitor/tests/openstack/test_operator.py +++ b/capi_janitor/tests/openstack/test_operator.py @@ -140,5 +140,8 @@ async def test_on_openstackcluster_event_calls_purge( [mock.call("cluster name that will be used for cleanup: 'mycluster'")] ) logger.info.assert_has_calls( - [mock.call("janitor finalizer not present, skipping cleanup")] + [ + mock.call("cloud credential secret deleted"), + mock.call("removed janitor finalizer from cluster"), + ] ) From 546a4351c4620ddaff08c1547dc7d042cdcaa682 Mon Sep 17 00:00:00 2001 From: bertiethorpe Date: Mon, 28 Apr 2025 18:04:55 +0100 Subject: [PATCH 03/13] add test purge_openstack_resources --- capi_janitor/tests/openstack/test_operator.py | 41 +++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/capi_janitor/tests/openstack/test_operator.py b/capi_janitor/tests/openstack/test_operator.py index e98c7e0..b9797bb 100644 --- a/capi_janitor/tests/openstack/test_operator.py +++ b/capi_janitor/tests/openstack/test_operator.py @@ -6,6 +6,7 @@ import easykube from capi_janitor.openstack import operator +from capi_janitor.openstack import openstack class TestOperator(unittest.IsolatedAsyncioTestCase): @@ -145,3 +146,43 @@ async def test_on_openstackcluster_event_calls_purge( mock.call("removed janitor finalizer from cluster"), ] ) + + @mock.patch.object(openstack.Cloud, "from_clouds") + async def test_purge_openstack_resources_raises(self, mock_from_clouds): + mock_from_clouds.return_value = mock.AsyncMock() + mock_networkapi = mock.AsyncMock() + mock_networkapi.resource.side_effect = lambda resource: resource + async with mock_from_clouds() as mock_cloud: + mock_cloud.is_authenticated = False + mock_cloud.current_user_id = "user" + mock_cloud.api_client.return_value = mock_networkapi + logger = mock.Mock() + clouds_yaml_data = { + "clouds": { + "openstack": { + "auth": { + "auth_url": "https://example.com:5000/v3", + "application_credential_id": "user", + "application_credential_secret": "pass", + }, + "region_name": "RegionOne", + "interface": "public", + "identity_api_version": 3, + "auth_type": "v3applicationcredential", + } + } + } + with self.assertRaises(openstack.AuthenticationError) as e: + await operator.purge_openstack_resources( + logger, + clouds_yaml_data, + "openstack", + None, + "mycluster", + True, + False, + ) + self.assertEqual( + str(e.exception), + "failed to authenticate as user: user", + ) From d2741ce2ac445655753139842de38ed2451342e1 Mon Sep 17 00:00:00 2001 From: bertiethorpe Date: Thu, 1 May 2025 10:32:41 +0100 Subject: [PATCH 04/13] WIP: happy path --- capi_janitor/tests/openstack/test_operator.py | 94 ++++++++++++++++++- requirements.txt | 2 +- 2 files changed, 90 insertions(+), 6 deletions(-) diff --git a/capi_janitor/tests/openstack/test_operator.py b/capi_janitor/tests/openstack/test_operator.py index b9797bb..da8af3c 100644 --- a/capi_janitor/tests/openstack/test_operator.py +++ b/capi_janitor/tests/openstack/test_operator.py @@ -149,13 +149,18 @@ async def test_on_openstackcluster_event_calls_purge( @mock.patch.object(openstack.Cloud, "from_clouds") async def test_purge_openstack_resources_raises(self, mock_from_clouds): - mock_from_clouds.return_value = mock.AsyncMock() + mock_networkapi = mock.AsyncMock() mock_networkapi.resource.side_effect = lambda resource: resource - async with mock_from_clouds() as mock_cloud: - mock_cloud.is_authenticated = False - mock_cloud.current_user_id = "user" - mock_cloud.api_client.return_value = mock_networkapi + + mock_cloud = mock.AsyncMock() + mock_cloud.__aenter__.return_value = mock_cloud + mock_cloud.is_authenticated = False + mock_cloud.current_user_id = "user" + mock_cloud.api_client.return_value = mock_networkapi + + mock_from_clouds.return_value = mock_cloud + logger = mock.Mock() clouds_yaml_data = { "clouds": { @@ -186,3 +191,82 @@ async def test_purge_openstack_resources_raises(self, mock_from_clouds): str(e.exception), "failed to authenticate as user: user", ) + + @mock.patch.object(openstack.Cloud, "from_clouds") + async def test_purge_openstack_resources_success(self, mock_from_clouds): + # Mocking the cloud object and API clients + mock_cloud = mock.AsyncMock() + mock_networkapi = mock.AsyncMock() + mock_lbapi = mock.AsyncMock() + mock_volumeapi = mock.AsyncMock() + mock_identityapi = mock.AsyncMock() + + # Mock the __aenter__ to return the mock_cloud when using the async context manager + mock_cloud.__aenter__.return_value = mock_cloud + mock_cloud.is_authenticated = True + mock_cloud.current_user_id = "user" + mock_cloud.api_client.return_value = mock_networkapi + + # Return mock clients for different services when requested + mock_from_clouds.return_value = mock_cloud + + # Mocking the resources for Network, Load Balancer, and Volume APIs + mock_networkapi.resource.side_effect = lambda resource: { + "floatingips": mock.AsyncMock(), + "security-groups": mock.AsyncMock(), + }.get(resource, None) + + mock_lbapi.resource.side_effect = lambda resource: { + "loadbalancers": mock.AsyncMock(), + }.get(resource, None) + + mock_volumeapi.resource.side_effect = lambda resource: { + "snapshots/detail": mock.AsyncMock(), + "snapshots": mock.AsyncMock(), + "volumes/detail": mock.AsyncMock(), + "volumes": mock.AsyncMock(), + }.get(resource, None) + + mock_identityapi.resource.side_effect = lambda resource: { + "application_credentials": mock.AsyncMock(), + }.get(resource, None) + + # Mock logger + logger = mock.Mock() + + clouds_yaml_data = { + "clouds": { + "openstack": { + "auth": { + "auth_url": "https://example.com:5000/v3", + "application_credential_id": "user", + "application_credential_secret": "pass", + }, + "region_name": "RegionOne", + "interface": "public", + "identity_api_version": 3, + "auth_type": "v3applicationcredential", + } + } + } + + # Simulate the purge_openstack_resources method behavior + await operator.purge_openstack_resources( + logger, + clouds_yaml_data, # Pass the mock cloud config + "openstack", + None, + "mycluster", + True, + False, + ) + + # Add assertions here based on expected behavior + # Example: Check that the resources were interacted with + mock_networkapi.resource.assert_any_call("floatingips") + mock_lbapi.resource.assert_any_call("loadbalancers") + mock_volumeapi.resource.assert_any_call("snapshots") + mock_volumeapi.resource.assert_any_call("volumes") + + # Example: Validate if appcred deletion was attempted + mock_identityapi.resource.assert_any_call("application_credentials") diff --git a/requirements.txt b/requirements.txt index dceab9f..069a5f4 100644 --- a/requirements.txt +++ b/requirements.txt @@ -10,7 +10,7 @@ easykube==0.5.0 exceptiongroup==1.2.2 frozenlist==1.5.0 h11==0.16.0 -httpcore==1.0.7 +httpcore==1.0.9 httpx==0.28.1 idna==3.10 iso8601==2.1.0 From e94f6ef56183adf3619b5b6a97990682bb67f338 Mon Sep 17 00:00:00 2001 From: bertiethorpe Date: Wed, 7 May 2025 16:31:15 +0100 Subject: [PATCH 05/13] test helper functions --- capi_janitor/openstack/operator.py | 7 +- capi_janitor/tests/openstack/test_operator.py | 227 ++++++++++++------ 2 files changed, 153 insertions(+), 81 deletions(-) diff --git a/capi_janitor/openstack/operator.py b/capi_janitor/openstack/operator.py index 4ebb5b4..bb2f08b 100644 --- a/capi_janitor/openstack/operator.py +++ b/capi_janitor/openstack/operator.py @@ -72,12 +72,13 @@ async def fips_for_cluster(resource, cluster): """ Async iterator for FIPs belonging to the specified cluster. """ - async for fip in resource.list(): - if not fip.description.startswith( + fips = await resource.list() + async for fip in fips: + if not fip["description"].startswith( "Floating IP for Kubernetes external service" ): continue - if not fip.description.endswith(f"from cluster {cluster}"): + if not fip["description"].endswith(f"from cluster {cluster}"): continue yield fip diff --git a/capi_janitor/tests/openstack/test_operator.py b/capi_janitor/tests/openstack/test_operator.py index da8af3c..5a359a0 100644 --- a/capi_janitor/tests/openstack/test_operator.py +++ b/capi_janitor/tests/openstack/test_operator.py @@ -9,6 +9,24 @@ from capi_janitor.openstack import openstack +# Helper to create an async iterable +def aiter(iterable): + class AsyncIter: + def __init__(self, it): + self.it = iter(it) + + def __aiter__(self): + return self + + async def __anext__(self): + try: + return next(self.it) + except StopIteration: + raise StopAsyncIteration + + return AsyncIter(iterable) + + class TestOperator(unittest.IsolatedAsyncioTestCase): async def test_operator(self): @@ -19,6 +37,59 @@ async def test_operator(self): mock_easykube.aclose.assert_awaited_once_with() + async def test_fips_for_cluster(self): + fips = [ + { + "description": "Floating IP for Kubernetes external service from cluster mycluster", + "id": 1, + }, + { + "description": "Floating IP for Kubernetes external service from cluster othercluster", + "id": 2, + }, + {"description": "Some other description", "id": 3}, + { + "description": "Floating IP for Kubernetes external service from cluster mycluster", + "id": 4, + }, + ] + + resource_mock = mock.Mock() + resource_mock.list = mock.AsyncMock(return_value=aiter(fips)) + + result = [] + async for fip in operator.fips_for_cluster(resource_mock, "mycluster"): + result.append(fip) + + self.assertEqual(len(result), 2) + self.assertEqual(result[0]["id"], 1) + self.assertEqual(result[1]["id"], 4) + + # async def test_lbs_for_cluster(self): + # # Create mock load balancers + # lb1 = mock.Mock(name="lb1", id=1) + # lb1.name = "kube_service_mycluster_api" + + # lb2 = mock.Mock(name="lb2", id=2) + # lb2.name = "kube_service_othercluster_ui" + + # lb3 = mock.Mock(name="lb3", id=3) + # lb3.name = "kube_service_mycluster_ui" + + # # Mock resource with async list() + # resource = mock.Mock() + # resource.list.return_value = aiter([lb1, lb2, lb3]) + + # # Run the function under test + # result = [] + # async for lb in lbs_for_cluster(resource, "mycluster"): + # result.append(lb) + + # # Assert results + # self.assertEqual(len(result), 2) + # self.assertEqual(result[0].id, 1) + # self.assertEqual(result[1].id, 3) + @mock.patch.object(operator, "patch_finalizers") @mock.patch.object(operator, "_get_os_cluster_client") async def test_on_openstackcluster_event_adds_finalizers( @@ -192,81 +263,81 @@ async def test_purge_openstack_resources_raises(self, mock_from_clouds): "failed to authenticate as user: user", ) - @mock.patch.object(openstack.Cloud, "from_clouds") - async def test_purge_openstack_resources_success(self, mock_from_clouds): - # Mocking the cloud object and API clients - mock_cloud = mock.AsyncMock() - mock_networkapi = mock.AsyncMock() - mock_lbapi = mock.AsyncMock() - mock_volumeapi = mock.AsyncMock() - mock_identityapi = mock.AsyncMock() - - # Mock the __aenter__ to return the mock_cloud when using the async context manager - mock_cloud.__aenter__.return_value = mock_cloud - mock_cloud.is_authenticated = True - mock_cloud.current_user_id = "user" - mock_cloud.api_client.return_value = mock_networkapi - - # Return mock clients for different services when requested - mock_from_clouds.return_value = mock_cloud - - # Mocking the resources for Network, Load Balancer, and Volume APIs - mock_networkapi.resource.side_effect = lambda resource: { - "floatingips": mock.AsyncMock(), - "security-groups": mock.AsyncMock(), - }.get(resource, None) - - mock_lbapi.resource.side_effect = lambda resource: { - "loadbalancers": mock.AsyncMock(), - }.get(resource, None) - - mock_volumeapi.resource.side_effect = lambda resource: { - "snapshots/detail": mock.AsyncMock(), - "snapshots": mock.AsyncMock(), - "volumes/detail": mock.AsyncMock(), - "volumes": mock.AsyncMock(), - }.get(resource, None) - - mock_identityapi.resource.side_effect = lambda resource: { - "application_credentials": mock.AsyncMock(), - }.get(resource, None) - - # Mock logger - logger = mock.Mock() - - clouds_yaml_data = { - "clouds": { - "openstack": { - "auth": { - "auth_url": "https://example.com:5000/v3", - "application_credential_id": "user", - "application_credential_secret": "pass", - }, - "region_name": "RegionOne", - "interface": "public", - "identity_api_version": 3, - "auth_type": "v3applicationcredential", - } - } - } - - # Simulate the purge_openstack_resources method behavior - await operator.purge_openstack_resources( - logger, - clouds_yaml_data, # Pass the mock cloud config - "openstack", - None, - "mycluster", - True, - False, - ) - - # Add assertions here based on expected behavior - # Example: Check that the resources were interacted with - mock_networkapi.resource.assert_any_call("floatingips") - mock_lbapi.resource.assert_any_call("loadbalancers") - mock_volumeapi.resource.assert_any_call("snapshots") - mock_volumeapi.resource.assert_any_call("volumes") - - # Example: Validate if appcred deletion was attempted - mock_identityapi.resource.assert_any_call("application_credentials") + # @mock.patch.object(openstack.Cloud, "from_clouds") + # async def test_purge_openstack_resources_success(self, mock_from_clouds): + # # Mocking the cloud object and API clients + # mock_cloud = mock.AsyncMock() + # mock_networkapi = mock.AsyncMock() + # mock_lbapi = mock.AsyncMock() + # mock_volumeapi = mock.AsyncMock() + # mock_identityapi = mock.AsyncMock() + + # # Mock the __aenter__ to return the mock_cloud when using the async context manager + # mock_cloud.__aenter__.return_value = mock_cloud + # mock_cloud.is_authenticated = True + # mock_cloud.current_user_id = "user" + # mock_cloud.api_client.return_value = mock_networkapi + + # # Return mock clients for different services when requested + # mock_from_clouds.return_value = mock_cloud + + # # Mocking the resources for Network, Load Balancer, and Volume APIs + # mock_networkapi.resource.side_effect = lambda resource: { + # "floatingips": mock.AsyncMock(), + # "security-groups": mock.AsyncMock(), + # }.get(resource, None) + + # mock_lbapi.resource.side_effect = lambda resource: { + # "loadbalancers": mock.AsyncMock(), + # }.get(resource, None) + + # mock_volumeapi.resource.side_effect = lambda resource: { + # "snapshots/detail": mock.AsyncMock(), + # "snapshots": mock.AsyncMock(), + # "volumes/detail": mock.AsyncMock(), + # "volumes": mock.AsyncMock(), + # }.get(resource, None) + + # mock_identityapi.resource.side_effect = lambda resource: { + # "application_credentials": mock.AsyncMock(), + # }.get(resource, None) + + # # Mock logger + # logger = mock.Mock() + + # clouds_yaml_data = { + # "clouds": { + # "openstack": { + # "auth": { + # "auth_url": "https://example.com:5000/v3", + # "application_credential_id": "user", + # "application_credential_secret": "pass", + # }, + # "region_name": "RegionOne", + # "interface": "public", + # "identity_api_version": 3, + # "auth_type": "v3applicationcredential", + # } + # } + # } + + # # Simulate the purge_openstack_resources method behavior + # await operator.purge_openstack_resources( + # logger, + # clouds_yaml_data, # Pass the mock cloud config + # "openstack", + # None, + # "mycluster", + # True, + # False, + # ) + + # # Add assertions here based on expected behavior + # # Example: Check that the resources were interacted with + # mock_networkapi.resource.assert_any_call("floatingips") + # mock_lbapi.resource.assert_any_call("loadbalancers") + # mock_volumeapi.resource.assert_any_call("snapshots") + # mock_volumeapi.resource.assert_any_call("volumes") + + # # Example: Validate if appcred deletion was attempted + # mock_identityapi.resource.assert_any_call("application_credentials") From 8b18ff79c1ef44eed0099bed064e3d46c17548a9 Mon Sep 17 00:00:00 2001 From: bertiethorpe Date: Thu, 8 May 2025 13:35:12 +0100 Subject: [PATCH 06/13] add tests for operator helper functions --- capi_janitor/openstack/operator.py | 20 +-- capi_janitor/tests/openstack/test_operator.py | 137 +++++++++++++----- 2 files changed, 114 insertions(+), 43 deletions(-) diff --git a/capi_janitor/openstack/operator.py b/capi_janitor/openstack/operator.py index bb2f08b..5f6c0c9 100644 --- a/capi_janitor/openstack/operator.py +++ b/capi_janitor/openstack/operator.py @@ -69,16 +69,14 @@ def __init__(self, resource, cluster): async def fips_for_cluster(resource, cluster): - """ - Async iterator for FIPs belonging to the specified cluster. - """ + """Async iterator for FIPs belonging to the specified cluster.""" fips = await resource.list() async for fip in fips: - if not fip["description"].startswith( + if not fip.description.startswith( "Floating IP for Kubernetes external service" ): continue - if not fip["description"].endswith(f"from cluster {cluster}"): + if not fip.description.endswith(f"from cluster {cluster}"): continue yield fip @@ -87,7 +85,8 @@ async def lbs_for_cluster(resource, cluster): """ Async iterator for loadbalancers belonging to the specified cluster. """ - async for lb in resource.list(): + lbs = await resource.list() + async for lb in lbs: if lb.name.startswith(f"kube_service_{cluster}_"): yield lb @@ -96,7 +95,8 @@ async def secgroups_for_cluster(resource, cluster): """ Async iterator for security groups belonging to the specified cluster. """ - async for sg in resource.list(): + sgs = await resource.list() + async for sg in sgs: if not sg.description.startswith("Security Group for"): continue if not sg.description.endswith(f"Service LoadBalancer in cluster {cluster}"): @@ -108,7 +108,8 @@ async def volumes_for_cluster(resource, cluster): """ Async iterator for volumes belonging to the specified cluster. """ - async for vol in resource.list(): + vols = await resource.list() + async for vol in vols: # CSI Cinder sets metadata on the volumes that we can look for owner = vol.metadata.get("cinder.csi.openstack.org/cluster") if owner and owner == cluster: @@ -119,7 +120,8 @@ async def snapshots_for_cluster(resource, cluster): """ Async iterator for snapshots belonging to the specified cluster. """ - async for snapshot in resource.list(): + snapshots = await resource.list() + async for snapshot in snapshots: # CSI Cinder sets metadata on the volumes that we can look for owner = snapshot.metadata.get("cinder.csi.openstack.org/cluster") if owner and owner == cluster: diff --git a/capi_janitor/tests/openstack/test_operator.py b/capi_janitor/tests/openstack/test_operator.py index 5a359a0..3e74ab6 100644 --- a/capi_janitor/tests/openstack/test_operator.py +++ b/capi_janitor/tests/openstack/test_operator.py @@ -39,19 +39,19 @@ async def test_operator(self): async def test_fips_for_cluster(self): fips = [ - { - "description": "Floating IP for Kubernetes external service from cluster mycluster", - "id": 1, - }, - { - "description": "Floating IP for Kubernetes external service from cluster othercluster", - "id": 2, - }, - {"description": "Some other description", "id": 3}, - { - "description": "Floating IP for Kubernetes external service from cluster mycluster", - "id": 4, - }, + mock.Mock( + description="Floating IP for Kubernetes external service from cluster mycluster", + id=1, + ), + mock.Mock( + description="Floating IP for Kubernetes external service from cluster othercluster", + id=2, + ), + mock.Mock(description="Some other description", id=3), + mock.Mock( + description="Floating IP for Kubernetes external service from cluster mycluster", + id=4, + ), ] resource_mock = mock.Mock() @@ -62,33 +62,102 @@ async def test_fips_for_cluster(self): result.append(fip) self.assertEqual(len(result), 2) - self.assertEqual(result[0]["id"], 1) - self.assertEqual(result[1]["id"], 4) + self.assertEqual(result[0].id, 1) + self.assertEqual(result[1].id, 4) + + async def test_lbs_for_cluster(self): + lbs = [ + mock.Mock(name="lb1", id=1), + mock.Mock(name="lb2", id=2), + mock.Mock(name="lb3", id=3), + mock.Mock(name="lb4", id=4), + ] + lbs[0].name = "kube_service_mycluster_api" + lbs[1].name = "kube_service_othercluster_api" + lbs[2].name = "fake_service_mycluster_api" + lbs[3].name = "kube_service_mycluster_ui" + + resource_mock = mock.Mock() + resource_mock.list = mock.AsyncMock(return_value=aiter(lbs)) + + result = [] + async for lb in operator.lbs_for_cluster(resource_mock, "mycluster"): + result.append(lb) + + self.assertEqual(len(result), 2) + self.assertEqual(result[0].id, 1) + self.assertEqual(result[1].id, 4) + + async def test_secgroups_for_cluster(self): + secgroups = [ + mock.Mock( + description="Security Group for Service LoadBalancer in cluster mycluster", + id=1, + ), + mock.Mock( + description="Security Group for Service LoadBalancer in cluster othercluster", + id=2, + ), + mock.Mock(description="Other description", id=3), + mock.Mock( + description="Security Group for Service LoadBalancer in cluster mycluster", + id=4, + ), + ] + + resource_mock = mock.Mock() + resource_mock.list = mock.AsyncMock(return_value=aiter(secgroups)) + + result = [] + async for sg in operator.secgroups_for_cluster(resource_mock, "mycluster"): + result.append(sg) + + self.assertEqual(len(result), 2) + self.assertEqual(result[0].id, 1) + self.assertEqual(result[1].id, 4) + + async def test_volumes_and_snapshots_for_cluster(self): + # Mock volumes with metadata containing the cluster owner information + resources = [ + mock.Mock(id=1, metadata={"cinder.csi.openstack.org/cluster": "mycluster"}), + mock.Mock( + id=2, metadata={"cinder.csi.openstack.org/cluster": "othercluster"} + ), + mock.Mock(id=3, metadata={"cinder.csi.openstack.org/cluster": "mycluster"}), + mock.Mock( + id=4, metadata={"cinder.csi.openstack.org/cluster": "othercluster"} + ), + # Volumes with invalid metadata + mock.Mock(id=5, metadata={"another_key": "value"}), + ] + + resource_mock = mock.Mock() + resource_mock.list = mock.AsyncMock(return_value=aiter(resources)) + + volumes_result = [] + async for vol in operator.volumes_for_cluster(resource_mock, "mycluster"): + volumes_result.append(vol) - # async def test_lbs_for_cluster(self): - # # Create mock load balancers - # lb1 = mock.Mock(name="lb1", id=1) - # lb1.name = "kube_service_mycluster_api" + self.assertEqual(len(volumes_result), 2) + self.assertEqual(volumes_result[0].id, 1) + self.assertEqual(volumes_result[1].id, 3) - # lb2 = mock.Mock(name="lb2", id=2) - # lb2.name = "kube_service_othercluster_ui" + # Reset mock values + resource_mock.list = mock.AsyncMock(return_value=aiter(resources)) - # lb3 = mock.Mock(name="lb3", id=3) - # lb3.name = "kube_service_mycluster_ui" + snapshots_result = [] + async for vol in operator.snapshots_for_cluster(resource_mock, "mycluster"): + snapshots_result.append(vol) - # # Mock resource with async list() - # resource = mock.Mock() - # resource.list.return_value = aiter([lb1, lb2, lb3]) + self.assertEqual(len(snapshots_result), 2) + self.assertEqual(snapshots_result[0].id, 1) + self.assertEqual(snapshots_result[1].id, 3) - # # Run the function under test - # result = [] - # async for lb in lbs_for_cluster(resource, "mycluster"): - # result.append(lb) + async def test_empty_iterator(self): + self.assertTrue(await operator.empty(aiter([]))) - # # Assert results - # self.assertEqual(len(result), 2) - # self.assertEqual(result[0].id, 1) - # self.assertEqual(result[1].id, 3) + async def test_non_empty_iterator(self): + self.assertFalse(await operator.empty(aiter([1, 2, 3]))) @mock.patch.object(operator, "patch_finalizers") @mock.patch.object(operator, "_get_os_cluster_client") From 6919ab670ad33161ab0700de816e93f8d5566e55 Mon Sep 17 00:00:00 2001 From: John Garbutt Date: Thu, 8 May 2025 14:13:44 +0100 Subject: [PATCH 07/13] Fix up a few of the tests --- capi_janitor/tests/openstack/test_operator.py | 77 ++++++++----------- 1 file changed, 33 insertions(+), 44 deletions(-) diff --git a/capi_janitor/tests/openstack/test_operator.py b/capi_janitor/tests/openstack/test_operator.py index c5ba361..508b124 100644 --- a/capi_janitor/tests/openstack/test_operator.py +++ b/capi_janitor/tests/openstack/test_operator.py @@ -6,27 +6,21 @@ import easykube from easykube.rest.util import PropertyDict -from capi_janitor.openstack import operator, openstack -from capi_janitor.openstack.operator import OPENSTACK_USER_VOLUMES_RECLAIM_PROPERTY from capi_janitor.openstack import openstack +from capi_janitor.openstack import operator +from capi_janitor.openstack.operator import OPENSTACK_USER_VOLUMES_RECLAIM_PROPERTY # Helper to create an async iterable -def aiter(iterable): - class AsyncIter: - def __init__(self, it): - self.it = iter(it) - - def __aiter__(self): - return self +class AsyncIterList: + def __init__(self, items): + self.items = items + self.kwargs = None - async def __anext__(self): - try: - return next(self.it) - except StopIteration: - raise StopAsyncIteration - - return AsyncIter(iterable) + async def list(self, **kwargs): + self.kwargs = kwargs + for item in self.items: + yield item class TestOperator(unittest.IsolatedAsyncioTestCase): @@ -39,55 +33,50 @@ async def test_operator(self): mock_easykube.aclose.assert_awaited_once_with() async def test_fips_for_cluster(self): + prefix = "Floating IP for Kubernetes external service from cluster" fips = [ mock.Mock( - description="Floating IP for Kubernetes external service from cluster mycluster", - id=1, + description=f"{prefix} mycluster", ), mock.Mock( - description="Floating IP for Kubernetes external service from cluster othercluster", - id=2, + description=f"{prefix} asdf", ), - mock.Mock(description="Some other description", id=3), + mock.Mock(description="Some other description"), mock.Mock( - description="Floating IP for Kubernetes external service from cluster mycluster", - id=4, + description=f"{prefix} mycluster", ), ] + resource_mock = AsyncIterList(fips) - resource_mock = mock.Mock() - resource_mock.list = mock.AsyncMock(return_value=aiter(fips)) - - result = [] - async for fip in operator.fips_for_cluster(resource_mock, "mycluster"): - result.append(fip) + result = [ + fip async for fip in operator.fips_for_cluster(resource_mock, "mycluster") + ] self.assertEqual(len(result), 2) - self.assertEqual(result[0].id, 1) - self.assertEqual(result[1].id, 4) + self.assertEqual(result[0], fips[0]) + self.assertEqual(result[1], fips[3]) async def test_lbs_for_cluster(self): lbs = [ - mock.Mock(name="lb1", id=1), - mock.Mock(name="lb2", id=2), - mock.Mock(name="lb3", id=3), - mock.Mock(name="lb4", id=4), + mock.Mock(name="lb0"), + mock.Mock(name="lb1"), + mock.Mock(name="lb2"), + mock.Mock(name="lb3"), ] + # can't pass name into mock.Mock() to do this lbs[0].name = "kube_service_mycluster_api" lbs[1].name = "kube_service_othercluster_api" lbs[2].name = "fake_service_mycluster_api" lbs[3].name = "kube_service_mycluster_ui" + resource_mock = AsyncIterList(lbs) - resource_mock = mock.Mock() - resource_mock.list = mock.AsyncMock(return_value=aiter(lbs)) - - result = [] - async for lb in operator.lbs_for_cluster(resource_mock, "mycluster"): - result.append(lb) - + result = [ + lb async for lb in operator.lbs_for_cluster(resource_mock, "mycluster") + ] + self.assertEqual(len(result), 2) - self.assertEqual(result[0].id, 1) - self.assertEqual(result[1].id, 4) + self.assertEqual(result[0], lbs[0]) + self.assertEqual(result[1], lbs[3]) async def test_secgroups_for_cluster(self): secgroups = [ From f3decf8a8272eca09ec235c333a98df6db975826 Mon Sep 17 00:00:00 2001 From: John Garbutt Date: Thu, 8 May 2025 14:17:01 +0100 Subject: [PATCH 08/13] Make sure ruff formats the code --- capi_janitor/tests/openstack/test_operator.py | 3 +-- tox.ini | 1 - 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/capi_janitor/tests/openstack/test_operator.py b/capi_janitor/tests/openstack/test_operator.py index 508b124..59f4420 100644 --- a/capi_janitor/tests/openstack/test_operator.py +++ b/capi_janitor/tests/openstack/test_operator.py @@ -73,7 +73,7 @@ async def test_lbs_for_cluster(self): result = [ lb async for lb in operator.lbs_for_cluster(resource_mock, "mycluster") ] - + self.assertEqual(len(result), 2) self.assertEqual(result[0], lbs[0]) self.assertEqual(result[1], lbs[3]) @@ -279,7 +279,6 @@ async def test_on_openstackcluster_event_calls_purge( @mock.patch.object(openstack.Cloud, "from_clouds") async def test_purge_openstack_resources_raises(self, mock_from_clouds): - mock_networkapi = mock.AsyncMock() mock_networkapi.resource.side_effect = lambda resource: resource diff --git a/tox.ini b/tox.ini index 1043309..08f67de 100644 --- a/tox.ini +++ b/tox.ini @@ -17,7 +17,6 @@ commands = stestr run {posargs} [testenv:pep8] commands = - ruff check {tox_root} ruff format {tox_root} codespell {tox_root} -w flake8 {postargs} From 7aef793e07edbbfd3519a448ca78cf01edacd961 Mon Sep 17 00:00:00 2001 From: John Garbutt Date: Thu, 8 May 2025 14:20:49 +0100 Subject: [PATCH 09/13] Assert calls to list() --- capi_janitor/tests/openstack/test_operator.py | 1 + 1 file changed, 1 insertion(+) diff --git a/capi_janitor/tests/openstack/test_operator.py b/capi_janitor/tests/openstack/test_operator.py index 59f4420..955d395 100644 --- a/capi_janitor/tests/openstack/test_operator.py +++ b/capi_janitor/tests/openstack/test_operator.py @@ -55,6 +55,7 @@ async def test_fips_for_cluster(self): self.assertEqual(len(result), 2) self.assertEqual(result[0], fips[0]) self.assertEqual(result[1], fips[3]) + self.assertEqual(resource_mock.kwargs, {}) async def test_lbs_for_cluster(self): lbs = [ From 43a0aec54380d1e33974aaab64affb81e2c1ab57 Mon Sep 17 00:00:00 2001 From: bertiethorpe Date: Thu, 8 May 2025 17:16:04 +0100 Subject: [PATCH 10/13] clean up helper function unit tests --- capi_janitor/tests/openstack/test_operator.py | 127 +++++++++--------- 1 file changed, 64 insertions(+), 63 deletions(-) diff --git a/capi_janitor/tests/openstack/test_operator.py b/capi_janitor/tests/openstack/test_operator.py index 955d395..eba696f 100644 --- a/capi_janitor/tests/openstack/test_operator.py +++ b/capi_janitor/tests/openstack/test_operator.py @@ -22,6 +22,16 @@ async def list(self, **kwargs): for item in self.items: yield item + def __aiter__(self): + self._iter = iter(self.items) + return self + + async def __anext__(self): + try: + return next(self._iter) + except StopIteration: + raise StopAsyncIteration + class TestOperator(unittest.IsolatedAsyncioTestCase): async def test_operator(self): @@ -35,16 +45,10 @@ async def test_operator(self): async def test_fips_for_cluster(self): prefix = "Floating IP for Kubernetes external service from cluster" fips = [ - mock.Mock( - description=f"{prefix} mycluster", - ), - mock.Mock( - description=f"{prefix} asdf", - ), + mock.Mock(description=f"{prefix} mycluster"), + mock.Mock(description=f"{prefix} asdf"), mock.Mock(description="Some other description"), - mock.Mock( - description=f"{prefix} mycluster", - ), + mock.Mock(description=f"{prefix} mycluster"), ] resource_mock = AsyncIterList(fips) @@ -80,75 +84,72 @@ async def test_lbs_for_cluster(self): self.assertEqual(result[1], lbs[3]) async def test_secgroups_for_cluster(self): + prefix = "Security Group for Service LoadBalancer in cluster" secgroups = [ - mock.Mock( - description="Security Group for Service LoadBalancer in cluster mycluster", - id=1, - ), - mock.Mock( - description="Security Group for Service LoadBalancer in cluster othercluster", - id=2, - ), - mock.Mock(description="Other description", id=3), - mock.Mock( - description="Security Group for Service LoadBalancer in cluster mycluster", - id=4, - ), + mock.Mock(description=f"{prefix} mycluster"), + mock.Mock(description=f"{prefix} othercluster"), + mock.Mock(description="Other description"), + mock.Mock(description=f"{prefix} mycluster"), ] - - resource_mock = mock.Mock() - resource_mock.list = mock.AsyncMock(return_value=aiter(secgroups)) + resource_mock = AsyncIterList(secgroups) result = [] async for sg in operator.secgroups_for_cluster(resource_mock, "mycluster"): result.append(sg) self.assertEqual(len(result), 2) - self.assertEqual(result[0].id, 1) - self.assertEqual(result[1].id, 4) - - async def test_volumes_and_snapshots_for_cluster(self): - # Mock volumes with metadata containing the cluster owner information - resources = [ - mock.Mock(id=1, metadata={"cinder.csi.openstack.org/cluster": "mycluster"}), - mock.Mock( - id=2, metadata={"cinder.csi.openstack.org/cluster": "othercluster"} - ), - mock.Mock(id=3, metadata={"cinder.csi.openstack.org/cluster": "mycluster"}), - mock.Mock( - id=4, metadata={"cinder.csi.openstack.org/cluster": "othercluster"} - ), + self.assertEqual(result[0], secgroups[0]) + self.assertEqual(result[1], secgroups[3]) + + async def test_filtered_volumes_for_cluster(self): + volumes = [ + mock.Mock(metadata={"cinder.csi.openstack.org/cluster": "mycluster"}), + mock.Mock(metadata={"cinder.csi.openstack.org/cluster": "othercluster"}), + mock.Mock(metadata={"cinder.csi.openstack.org/cluster": "mycluster"}), + mock.Mock(metadata={"cinder.csi.openstack.org/cluster": "othercluster"}), # Volumes with invalid metadata - mock.Mock(id=5, metadata={"another_key": "value"}), + mock.Mock(metadata={"another_key": "value"}), ] + resource_mock = AsyncIterList(volumes) - resource_mock = mock.Mock() - resource_mock.list = mock.AsyncMock(return_value=aiter(resources)) - - volumes_result = [] - async for vol in operator.volumes_for_cluster(resource_mock, "mycluster"): - volumes_result.append(vol) - - self.assertEqual(len(volumes_result), 2) - self.assertEqual(volumes_result[0].id, 1) - self.assertEqual(volumes_result[1].id, 3) + result = [] + async for vol in operator.filtered_volumes_for_cluster( + resource_mock, "mycluster" + ): + result.append(vol) - # Reset mock values - resource_mock.list = mock.AsyncMock(return_value=aiter(resources)) + self.assertEqual(len(result), 2) + self.assertEqual(result[0], volumes[0]) + self.assertEqual(result[1], volumes[2]) + + async def test_snapshots_for_cluster(self): + snapshots = [ + mock.Mock(metadata={"cinder.csi.openstack.org/cluster": "mycluster"}), + mock.Mock(metadata={"cinder.csi.openstack.org/cluster": "othercluster"}), + mock.Mock(metadata={"cinder.csi.openstack.org/cluster": "mycluster"}), + mock.Mock(metadata={"cinder.csi.openstack.org/cluster": "othercluster"}), + # Volumes with invalid metadata + mock.Mock(metadata={"another_key": "value"}), + ] + resource_mock = AsyncIterList(snapshots) - snapshots_result = [] - async for vol in operator.snapshots_for_cluster(resource_mock, "mycluster"): - snapshots_result.append(vol) + result = [] + async for snap in operator.snapshots_for_cluster(resource_mock, "mycluster"): + result.append(snap) - self.assertEqual(len(snapshots_result), 2) - self.assertEqual(snapshots_result[0].id, 1) - self.assertEqual(snapshots_result[1].id, 3) + self.assertEqual(len(result), 2) + self.assertEqual(result[0], snapshots[0]) + self.assertEqual(result[1], snapshots[2]) - async def test_empty_iterator(self): - self.assertTrue(await operator.empty(aiter([]))) + async def test_empty_iterator_returns_true(self): + async_iter = AsyncIterList([]).__aiter__() + result = await operator.empty(async_iter) + self.assertTrue(result) - async def test_non_empty_iterator(self): - self.assertFalse(await operator.empty(aiter([1, 2, 3]))) + async def test_non_empty_iterator_returns_false(self): + async_iter = AsyncIterList([1, 2, 3]).__aiter__() + result = await operator.empty(async_iter) + self.assertFalse(result) @mock.patch.object(operator, "patch_finalizers") @mock.patch.object(operator, "_get_os_cluster_client") @@ -331,7 +332,7 @@ async def test_purge_openstack_resources_raises(self, mock_from_clouds): # mock_volumeapi = mock.AsyncMock() # mock_identityapi = mock.AsyncMock() - # # Mock the __aenter__ to return the mock_cloud when using the async context manager + # # Mock the __aenter__ to return the mock_cloud # mock_cloud.__aenter__.return_value = mock_cloud # mock_cloud.is_authenticated = True # mock_cloud.current_user_id = "user" From 0a45b91266504de5bd0852ff1534a0fd79b92456 Mon Sep 17 00:00:00 2001 From: bertiethorpe Date: Fri, 9 May 2025 13:57:46 +0100 Subject: [PATCH 11/13] extend unit tests for more helpers, put volumes test in line with current patterns --- capi_janitor/tests/openstack/test_operator.py | 129 +++++++++++------- 1 file changed, 76 insertions(+), 53 deletions(-) diff --git a/capi_janitor/tests/openstack/test_operator.py b/capi_janitor/tests/openstack/test_operator.py index eba696f..2ec82f4 100644 --- a/capi_janitor/tests/openstack/test_operator.py +++ b/capi_janitor/tests/openstack/test_operator.py @@ -4,7 +4,7 @@ import yaml import easykube -from easykube.rest.util import PropertyDict +import httpx from capi_janitor.openstack import openstack from capi_janitor.openstack import operator @@ -103,12 +103,30 @@ async def test_secgroups_for_cluster(self): async def test_filtered_volumes_for_cluster(self): volumes = [ - mock.Mock(metadata={"cinder.csi.openstack.org/cluster": "mycluster"}), - mock.Mock(metadata={"cinder.csi.openstack.org/cluster": "othercluster"}), - mock.Mock(metadata={"cinder.csi.openstack.org/cluster": "mycluster"}), - mock.Mock(metadata={"cinder.csi.openstack.org/cluster": "othercluster"}), - # Volumes with invalid metadata - mock.Mock(metadata={"another_key": "value"}), + mock.Mock( + metadata={ + "cinder.csi.openstack.org/cluster": "mycluster", + OPENSTACK_USER_VOLUMES_RECLAIM_PROPERTY: "false", + } + ), + mock.Mock( + metadata={ + "cinder.csi.openstack.org/cluster": "mycluster", + OPENSTACK_USER_VOLUMES_RECLAIM_PROPERTY: "true", + } + ), + mock.Mock( + metadata={ + "cinder.csi.openstack.org/cluster": "othercluster", + OPENSTACK_USER_VOLUMES_RECLAIM_PROPERTY: "false", + } + ), + mock.Mock( + metadata={ + "cinder.csi.openstack.org/cluster": "mycluster", + } + ), + mock.Mock(metadata={"other_key": "value"}), ] resource_mock = AsyncIterList(volumes) @@ -120,7 +138,7 @@ async def test_filtered_volumes_for_cluster(self): self.assertEqual(len(result), 2) self.assertEqual(result[0], volumes[0]) - self.assertEqual(result[1], volumes[2]) + self.assertEqual(result[1], volumes[3]) async def test_snapshots_for_cluster(self): snapshots = [ @@ -151,6 +169,56 @@ async def test_non_empty_iterator_returns_false(self): result = await operator.empty(async_iter) self.assertFalse(result) + async def test_try_delete_success(self): + instances = [mock.Mock(id=1), mock.Mock(id=2)] + resource = mock.Mock() + resource.delete = mock.AsyncMock() + resource.singular_name = "test_resource" + logger = mock.Mock() + + result = await operator.try_delete(logger, resource, AsyncIterList(instances)) + + self.assertTrue(result) + resource.delete.assert_any_await(1) + resource.delete.assert_any_await(2) + logger.warn.assert_not_called() + + async def test_try_delete_with_400_and_409_errors(self): + instances = [mock.Mock(id=1), mock.Mock(id=2)] + resource = mock.Mock() + resource.singular_name = "test_resource" + logger = mock.Mock() + + resource.delete = mock.AsyncMock( + side_effect=[ + httpx.HTTPStatusError( + "error", request=mock.Mock(), response=mock.Mock(status_code=400) + ), + httpx.HTTPStatusError( + "error", request=mock.Mock(), response=mock.Mock(status_code=409) + ), + ] + ) + result = await operator.try_delete(logger, resource, AsyncIterList(instances)) + + self.assertTrue(result) + self.assertEqual(resource.delete.await_count, 2) + self.assertEqual(logger.warn.call_count, 2) + + async def test_delete_with_unexpected_error_raises(self): + instances = [mock.Mock(id=1)] + resource = mock.Mock() + resource.singular_name = "test_resource" + logger = mock.Mock() + + resource.delete = mock.AsyncMock( + side_effect=httpx.HTTPStatusError( + "error", request=mock.Mock(), response=mock.Mock(status_code=500) + ) + ) + with self.assertRaises(httpx.HTTPStatusError): + await operator.try_delete(logger, resource, AsyncIterList(instances)) + @mock.patch.object(operator, "patch_finalizers") @mock.patch.object(operator, "_get_os_cluster_client") async def test_on_openstackcluster_event_adds_finalizers( @@ -401,48 +469,3 @@ async def test_purge_openstack_resources_raises(self, mock_from_clouds): # # Example: Validate if appcred deletion was attempted # mock_identityapi.resource.assert_any_call("application_credentials") - - @mock.patch.object(openstack, "Resource") - async def test_user_keep_volumes_filter(self, mock_volumes_resource): - # Arrange - async def _list_volumes(): - test_volumes = [ - { - "id": "123", - "name": "volume-1", - "metadata": { - "cinder.csi.openstack.org/cluster": "cluster-1", - OPENSTACK_USER_VOLUMES_RECLAIM_PROPERTY: "anything-but-true", - }, - }, - { - "id": "456", - "name": "volume-2", - "metadata": { - "cinder.csi.openstack.org/cluster": "cluster-1", - OPENSTACK_USER_VOLUMES_RECLAIM_PROPERTY: "true", - }, - }, - { - "id": "789", - "name": "volume-3", - "metadata": { - "cinder.csi.openstack.org/cluster": "cluster-2", - OPENSTACK_USER_VOLUMES_RECLAIM_PROPERTY: "true", - }, - }, - ] - for volume in map(PropertyDict, test_volumes): - yield volume - - mock_volumes_resource.list.return_value = _list_volumes() - # Act - filtered_volumes = [ - v - async for v in operator.filtered_volumes_for_cluster( - mock_volumes_resource, "cluster-1" - ) - ] - # Assert - self.assertEqual(len(filtered_volumes), 1) - self.assertEqual(filtered_volumes[0].get("name"), "volume-1") From 0fbc0d8760d28b1d8835849cf6533bb926ae3a26 Mon Sep 17 00:00:00 2001 From: bertiethorpe Date: Fri, 9 May 2025 15:33:53 +0100 Subject: [PATCH 12/13] test patch_finalizers directly --- capi_janitor/tests/openstack/test_operator.py | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/capi_janitor/tests/openstack/test_operator.py b/capi_janitor/tests/openstack/test_operator.py index 2ec82f4..b08d7b8 100644 --- a/capi_janitor/tests/openstack/test_operator.py +++ b/capi_janitor/tests/openstack/test_operator.py @@ -5,6 +5,7 @@ import easykube import httpx +import kopf from capi_janitor.openstack import openstack from capi_janitor.openstack import operator @@ -469,3 +470,34 @@ async def test_purge_openstack_resources_raises(self, mock_from_clouds): # # Example: Validate if appcred deletion was attempted # mock_identityapi.resource.assert_any_call("application_credentials") + + async def test_successful_patch(self): + resource = mock.AsyncMock() + await operator.patch_finalizers( + resource, "test-name", "default", ["finalizer.k8s.io"] + ) + resource.patch.assert_awaited_once_with( + "test-name", + {"metadata": {"finalizers": ["finalizer.k8s.io"]}}, + namespace="default", + ) + + async def test_patch_finalizers_error_handling(self): + resource = mock.AsyncMock() + + response_mock = mock.Mock() + response_mock.status_code = 422 + response_mock.json.return_value = {"message": "error"} + + api_error_422 = easykube.ApiError(mock.Mock(response=response_mock)) + api_error_422.status_code = 422 + resource.patch.side_effect = api_error_422 + + with self.assertRaises(kopf.TemporaryError): + await operator.patch_finalizers(resource, "name", "ns", []) + + exc_404 = easykube.ApiError("not found") + exc_404.status_code = 404 + resource.patch.side_effect = exc_404 + + await operator.patch_finalizers(resource, "name", "ns", []) \ No newline at end of file From c141c518540400ed5654172c78154cf091b3ea58 Mon Sep 17 00:00:00 2001 From: bertiethorpe Date: Fri, 9 May 2025 15:39:25 +0100 Subject: [PATCH 13/13] revert patch_finalizers test --- capi_janitor/tests/openstack/test_operator.py | 32 ------------------- 1 file changed, 32 deletions(-) diff --git a/capi_janitor/tests/openstack/test_operator.py b/capi_janitor/tests/openstack/test_operator.py index b08d7b8..2ec82f4 100644 --- a/capi_janitor/tests/openstack/test_operator.py +++ b/capi_janitor/tests/openstack/test_operator.py @@ -5,7 +5,6 @@ import easykube import httpx -import kopf from capi_janitor.openstack import openstack from capi_janitor.openstack import operator @@ -470,34 +469,3 @@ async def test_purge_openstack_resources_raises(self, mock_from_clouds): # # Example: Validate if appcred deletion was attempted # mock_identityapi.resource.assert_any_call("application_credentials") - - async def test_successful_patch(self): - resource = mock.AsyncMock() - await operator.patch_finalizers( - resource, "test-name", "default", ["finalizer.k8s.io"] - ) - resource.patch.assert_awaited_once_with( - "test-name", - {"metadata": {"finalizers": ["finalizer.k8s.io"]}}, - namespace="default", - ) - - async def test_patch_finalizers_error_handling(self): - resource = mock.AsyncMock() - - response_mock = mock.Mock() - response_mock.status_code = 422 - response_mock.json.return_value = {"message": "error"} - - api_error_422 = easykube.ApiError(mock.Mock(response=response_mock)) - api_error_422.status_code = 422 - resource.patch.side_effect = api_error_422 - - with self.assertRaises(kopf.TemporaryError): - await operator.patch_finalizers(resource, "name", "ns", []) - - exc_404 = easykube.ApiError("not found") - exc_404.status_code = 404 - resource.patch.side_effect = exc_404 - - await operator.patch_finalizers(resource, "name", "ns", []) \ No newline at end of file