Skip load balancer clean up when none were created (#218)

* Skip load balancer clean up when none were created

* Update unit tests and fix linter errors

Author: sd109

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,15 +433,29 @@ 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,
             cloud_name,
             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:

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(

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