From 697f99ef4370e3c317950aba353880281c0e41fe Mon Sep 17 00:00:00 2001 From: Scott Davidson Date: Tue, 22 Jul 2025 12:09:02 +0100 Subject: [PATCH 1/2] Skip load balancer clean up when none were created --- capi_janitor/openstack/operator.py | 42 ++++++++++++++++++++++++------ 1 file changed, 34 insertions(+), 8 deletions(-) diff --git a/capi_janitor/openstack/operator.py b/capi_janitor/openstack/operator.py index d67a721..f879fb6 100644 --- a/capi_janitor/openstack/operator.py +++ b/capi_janitor/openstack/operator.py @@ -155,7 +155,14 @@ async def try_delete(logger, resource, instances, **kwargs): async def purge_openstack_resources( - logger, clouds, cloud_name, cacert, name, include_volumes, include_appcred + logger, + clouds, + cloud_name, + cacert, + name, + include_volumes, + include_loadbalancers, + include_appcred, ): """Cleans up the OpenStack resources created by the OCCM and CSI for a cluster.""" # Use the credential to delete external resources as required @@ -180,10 +187,15 @@ async def purge_openstack_resources( # Delete any loadbalancers associated with loadbalancer services for the cluster lbapi = cloud.api_client("load-balancer", "/v2/lbaas/") loadbalancers = lbapi.resource("loadbalancers") - check_lbs = await try_delete( - logger, loadbalancers, lbs_for_cluster(loadbalancers, name), cascade="true" - ) - logger.info("deleted load balancers for LoadBalancer services") + check_lbs = False + if include_loadbalancers: + check_lbs = await try_delete( + logger, + loadbalancers, + lbs_for_cluster(loadbalancers, name), + cascade="true" + ) + logger.info("deleted load balancers for LoadBalancer services") # Delete security groups associated with loadbalancer services for the cluster secgroups = networkapi.resource("security-groups") @@ -344,15 +356,15 @@ async def wrapper(**kwargs): @kopf.on.event(CAPO_API_GROUP, "openstackclusters") @retry_event async def on_openstackcluster_event( - name, namespace, meta, labels, spec, logger, **kwargs + name, namespace, meta, labels, spec, status, logger, **kwargs ): await _on_openstackcluster_event_impl( - name, namespace, meta, labels, spec, logger, **kwargs + name, namespace, meta, labels, spec, status, logger, **kwargs ) async def _on_openstackcluster_event_impl( - name, namespace, meta, labels, spec, logger, **kwargs + name, namespace, meta, labels, spec, status, logger, **kwargs ): """Executes whenever an event occurs for an OpenStack cluster.""" # Get the resource for manipulating OpenStackClusters at the preferred version @@ -421,6 +433,18 @@ async def _on_openstackcluster_event_impl( ) remove_appcred = credential_annotation_value == CREDENTIAL_ANNOTATION_DELETE + # Handle the case where the API server load balancer is enabled but was + # never successfully created (possibly due to LB permissions error) + # meaning that there cannot be any other LBs to remove + # (since API server was never online due to missing load balancer) + remove_loadbalancers = ( + # Default to false since this is default CAPO behaviour if + # ApiServerLoadBalancer field is omitted from OpenStackClusterSpec + # https://github.com/kubernetes-sigs/cluster-api-provider-openstack/blob/57ae27ee114bda3606d92163397697b640272673/api/v1beta1/openstackcluster_types.go#L99-L101 + spec.get('apiServerLoadBalancer', {}).get('enabled', False) and + status.get('apiServerLoadBalancer', {}).get('id', '') != '' + ) + await purge_openstack_resources( logger, clouds, @@ -428,8 +452,10 @@ async def _on_openstackcluster_event_impl( cacert, clustername, volumes_annotation_value == VOLUMES_ANNOTATION_DELETE, + remove_loadbalancers, remove_appcred and len(finalizers) == 1, ) + # 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: From 004dc9c7e8d4a09ebe072b6b02891399e8883658 Mon Sep 17 00:00:00 2001 From: Scott Davidson Date: Tue, 22 Jul 2025 14:01:44 +0100 Subject: [PATCH 2/2] Update unit tests and fix linter errors --- capi_janitor/openstack/operator.py | 8 ++++---- capi_janitor/tests/openstack/test_operator.py | 5 +++++ tox.ini | 4 ++-- 3 files changed, 11 insertions(+), 6 deletions(-) diff --git a/capi_janitor/openstack/operator.py b/capi_janitor/openstack/operator.py index f879fb6..1197550 100644 --- a/capi_janitor/openstack/operator.py +++ b/capi_janitor/openstack/operator.py @@ -154,7 +154,7 @@ async def try_delete(logger, resource, instances, **kwargs): return check_required -async def purge_openstack_resources( +async def purge_openstack_resources( # noqa: C901 logger, clouds, cloud_name, @@ -193,7 +193,7 @@ async def purge_openstack_resources( logger, loadbalancers, lbs_for_cluster(loadbalancers, name), - cascade="true" + cascade="true", ) logger.info("deleted load balancers for LoadBalancer services") @@ -441,8 +441,8 @@ async def _on_openstackcluster_event_impl( # Default to false since this is default CAPO behaviour if # ApiServerLoadBalancer field is omitted from OpenStackClusterSpec # https://github.com/kubernetes-sigs/cluster-api-provider-openstack/blob/57ae27ee114bda3606d92163397697b640272673/api/v1beta1/openstackcluster_types.go#L99-L101 - spec.get('apiServerLoadBalancer', {}).get('enabled', False) and - status.get('apiServerLoadBalancer', {}).get('id', '') != '' + spec.get("apiServerLoadBalancer", {}).get("enabled", False) + and status.get("apiServerLoadBalancer", {}).get("id", "") != "" ) await purge_openstack_resources( diff --git a/capi_janitor/tests/openstack/test_operator.py b/capi_janitor/tests/openstack/test_operator.py index d423622..5792d8d 100644 --- a/capi_janitor/tests/openstack/test_operator.py +++ b/capi_janitor/tests/openstack/test_operator.py @@ -232,6 +232,7 @@ async def test_on_openstackcluster_event_adds_finalizers( meta={}, labels={}, spec={}, + status={}, logger=logger, body={}, ) @@ -258,6 +259,7 @@ async def test_on_openstackcluster_event_skip_no_finalizers( meta={"deletionTimestamp": "2023-10-01T00:00:00Z"}, labels={}, spec={}, + status={}, logger=logger, body={}, ) @@ -319,6 +321,7 @@ async def test_on_openstackcluster_event_calls_purge( }, labels={}, spec={"identityRef": {"name": "appcred42"}}, + status={}, logger=logger, body={}, ) @@ -330,6 +333,7 @@ async def test_on_openstackcluster_event_calls_purge( None, "mycluster", True, + False, True, ) mock_delete_secret.assert_awaited_once_with("appcred42", "namespace1") @@ -383,6 +387,7 @@ async def test_purge_openstack_resources_raises(self, mock_from_clouds): None, "mycluster", True, + True, False, ) self.assertEqual( diff --git a/tox.ini b/tox.ini index c78309f..eaaa863 100644 --- a/tox.ini +++ b/tox.ini @@ -20,7 +20,7 @@ commands = stestr run {posargs} [testenv:autofix] commands = ruff format {tox_root} - codespell {tox_root} -w + codespell {tox_root} -w --skip venv ruff check {tox_root} --fix [testenv:black] @@ -29,7 +29,7 @@ commands = commands = black {tox_root} --check [testenv:codespell] -commands = codespell {posargs} +commands = codespell --skip ./venv {posargs} [testenv:ruff] description = Run Ruff checks