Skip volumes with the keep property set to true in volumes_for_cluster (#176)

* Skip volumes with the keep property set to true in volumes_for_cluster

See zonca/jupyterhub-deploy-kubernetes-jetstream#93 (comment)

* Run auto-formatter

* Use top-level variable for volume property

* Add section on user-configurable volume cleanup behaviour

* Fix formatting

* Add unit test for volume property filter

* Rename volumes_for_cluster -> filtered_volumes_for_cluster

* Fix comment typos

* Make formatter happy

---------

Co-authored-by: sd109 <[email protected]>

Author: zonca

.github/workflows/test-pr.yaml

@@ -73,11 +73,11 @@ jobs:
       - name: Provision Azimuth
         uses: azimuth-cloud/azimuth-config/.github/actions/provision@devel
 
-      # # Run the tests
+      # Run the tests
       - name: Run Azimuth tests
         uses: azimuth-cloud/azimuth-config/.github/actions/test@devel
 
-      # Tear down the environment
+      # Tear down the environment
       - name: Destroy Azimuth
         uses: azimuth-cloud/azimuth-config/.github/actions/destroy@devel
         if: ${{ always() }}

README.md

@@ -56,6 +56,21 @@ metadata:
 
 > **NOTE**: Any value other than `delete` means volumes will be kept.
 
+### User-configurable behaviour
+
+Annotations on the Kubernetes resources are only available to administrators with
+access to the Cluster API management cluster's Kubernetes API; therefore, the Janitor
+also provides an alternative user-facing mechanism for marking volumes which should
+not be deleted during cluster clean up. This is done by adding a property to the
+OpenStack volume using:
+
+```
+openstack volume set --property janitor.capi.azimuth-cloud.com/keep='true' <volume-name-or-id>
+```
+
+Any value other than 'true' will result in the volume being deleted when the workload
+cluster is deleted.
+
 ## How it works
 
 `cluster-api-janitor-openstack` watches for `OpenStackCluster`s being created and adds its
@@ -66,7 +81,7 @@ Cluster API `Cluster`, from being removed until the finalizer is removed.
 (specifically, it waits for the `deletionTimestamp` to be set, indicating that a deletion
 has been requested), at which point it uses the credential from
 `OpenStackCluster.spec.identityRef` to remove any dangling resources that were created by
-the OCCM or Cinder CSI with the same cluster name as the cluster being deleted. 
+the OCCM or Cinder CSI with the same cluster name as the cluster being deleted.
 The cluster name is determined by the `cluster.x-k8s.io/cluster-name` label on the
 OpenStackCluster resource, if present.
 If the label is not set, the name of the OpenStackCluster resource (`metadata.name`) is

capi_janitor/openstack/operator.py

@@ -31,6 +31,11 @@
 RETRY_ANNOTATION = "janitor.capi.stackhpc.com/retry"
 RETRY_DEFAULT_DELAY = int(os.environ.get("CAPI_JANITOR_RETRY_DEFAULT_DELAY", "60"))
 
+# The property on the OpenStack volume resource which, if set to 'true',
+# will instruct the Janitor to ignore this volume when cleaning up cluster
+# resources.
+OPENSTACK_USER_VOLUMES_RECLAIM_PROPERTY = "janitor.capi.azimuth-cloud.com/keep"
+
 
 @kopf.on.startup()
 async def on_startup(**kwargs):
@@ -103,14 +108,19 @@ async def secgroups_for_cluster(resource, cluster):
         yield sg
 
 
-async def volumes_for_cluster(resource, cluster):
+async def filtered_volumes_for_cluster(resource, cluster):
     """
     Async iterator for volumes belonging to the specified cluster.
     """
     async for vol in resource.list():
         # 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:
+        # Skip volumes with the keep property set to true
+        if (
+            owner
+            and owner == cluster
+            and vol.metadata.get(OPENSTACK_USER_VOLUMES_RECLAIM_PROPERTY) != "true"
+        ):
             yield vol
 
 
@@ -228,7 +238,7 @@ async def purge_openstack_resources(
             )
             logger.info("deleted snapshots for persistent volume claims")
             check_volumes = await try_delete(
-                logger, volumes, volumes_for_cluster(volumes_detail, name)
+                logger, volumes, filtered_volumes_for_cluster(volumes_detail, name)
             )
             logger.info("deleted volumes for persistent volume claims")
 
@@ -239,7 +249,9 @@ async def purge_openstack_resources(
             raise ResourcesStillPresentError("loadbalancers", name)
         if check_secgroups and not await empty(secgroups_for_cluster(secgroups, name)):
             raise ResourcesStillPresentError("security-groups", name)
-        if check_volumes and not await empty(volumes_for_cluster(volumes_detail, name)):
+        if check_volumes and not await empty(
+            filtered_volumes_for_cluster(volumes_detail, name)
+        ):
             raise ResourcesStillPresentError("volumes", name)
         if check_snapshots and not await empty(
             snapshots_for_cluster(snapshots_detail, name)

capi_janitor/tests/openstack/test_operator.py

@@ -4,8 +4,10 @@
 from unittest import mock
 
 import easykube
+from easykube.rest.util import PropertyDict
 
-from capi_janitor.openstack import operator
+from capi_janitor.openstack import operator, openstack
+from capi_janitor.openstack.operator import OPENSTACK_USER_VOLUMES_RECLAIM_PROPERTY
 
 
 class TestOperator(unittest.IsolatedAsyncioTestCase):
@@ -145,3 +147,48 @@ async def test_on_openstackcluster_event_calls_purge(
                 mock.call("removed janitor finalizer from cluster"),
             ]
         )
+
+    @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")