diff --git a/capi_janitor/openstack/operator.py b/capi_janitor/openstack/operator.py index d67a721..1197550 100644 --- a/capi_janitor/openstack/operator.py +++ b/capi_janitor/openstack/operator.py @@ -154,8 +154,15 @@ async def try_delete(logger, resource, instances, **kwargs): return check_required -async def purge_openstack_resources( - logger, clouds, cloud_name, cacert, name, include_volumes, include_appcred +async def purge_openstack_resources( # noqa: C901 + 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: 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