feat: safer cleanup for resources

AGENTS.md

@@ -31,6 +31,7 @@ This file is the constitution for this project. It captures persistent principle
 4. **volume-operator** hooks into the ReplicaSet lifecycle. For the active ReplicaSet (revision matches Deployment), it creates:
     - An **`AzureVolumePopulator` CR** (the AVP)
     - A **source PVC** with `DataSourceRef` pointing to the AVP — both named by the hash
+    - Both carry an **`ownerReference` to the ReplicaSet** (see "Cleanup on full deletion" below)
 5. **azure-volume-populator** sees the AVP CR and spawns a **populator pod** that copies data from Azure Blob Storage into the source PVC.
 6. The source PVC reaches **`Bound`** state.
 7. Kubernetes detects that the ephemeral volume's `DataSource` PVC is now Bound and triggers native **volume cloning** — each pod gets its own ephemeral clone.
@@ -41,9 +42,23 @@ This file is the constitution for this project. It captures persistent principle
 When a Deployment rolls out a new ReplicaSet (new revision), on every reconcile of any ReplicaSet in the set:
 - volume-operator deletes the **AVP and PVC** (not the ReplicaSet itself — ReplicaSet lifecycle belongs to Kubernetes/the Deployment controller, not this operator) for old ReplicaSets that are scaled to 0 replicas.
 - A resource is only deleted if no other ReplicaSet still references the same `resource-suffix` hash with `replicas > 0` (the hash can be shared across revisions when `blobPrefix`/`volumeMountPath`/`storageCapacity` are unchanged — see "Hash-based deduplication" below).
-- **Safety gate:** cleanup is skipped entirely unless the ReplicaSet matching the Deployment's *current* revision has `AvailableReplicas > 0` — i.e. the new rollout must actually be serving before old storage is torn down (`currentReplicaSetIsAvailable` in `internal/controller/util.go`). This guards against deleting the old PVC/AVP while the new one is still failing to come up.
+- **Cleanup runs on a reconcile of *any* ReplicaSet in the set**, not just the current-revision one — an old ReplicaSet scaling to 0 is itself the trigger that makes it eligible. Resource *provisioning* still only happens on the current-revision ReplicaSet (revision match + required annotations); the `else` branch falls through to cleanup.
+- **Safety gate:** deletion is skipped unless the ReplicaSet matching the Deployment's *current* revision has `AvailableReplicas > 0` — i.e. the new rollout must actually be serving before old storage is torn down (`currentReplicaSetIsAvailable` in `internal/controller/util.go`). This guards against deleting the old PVC/AVP while the new one is still failing to come up.
+- **Requeue while blocked:** when the safety gate holds cleanup back but eligible old resources still exist, `Reconcile` returns `RequeueAfter: cleanupRetryInterval` (1 min) so it converges once the new rollout serves, instead of waiting for an incidental later event. `cleanUpOldReplicaSets` returns `(requeue bool, err error)` for this; `replicaSetHasResources` checks whether the AVP/PVC still exist, and fails safe (reports `true`/still-pending) on any unexpected, non-`NotFound` `Get` error rather than silently treating it as "resource gone".
+- **The Deployment's current hash is never deleted:** an old ReplicaSet whose `resource-suffix` equals the Deployment's current one is skipped, regardless of replica counts. `resourceIsUsedByOtherReplicaSet` only counts *other* ReplicaSets with `replicas > 0`, so without this a Deployment scaled to 0 (current ReplicaSet `Spec.Replicas == 0` while `AvailableReplicas` still lags at >0 as pods terminate) would have its live source PVC/AVP deleted and re-populated from blob on scale-up. It also keeps the requeue from firing forever for a scaled-to-0 Deployment.
 - The new and old PVCs are **independent** (different names if the hash changed), so there is no conflict during transition.
-- Cleanup logic lives in `cleanUpOldReplicaSets` / `deleteResourcesForReplicaSet` in `internal/controller/volume_controller.go`, backed by `hasReplicas`, `resourceIsUsedByOtherReplicaSet`, and `currentReplicaSetIsAvailable` in `internal/controller/util.go`.
+- Cleanup logic lives in `cleanUpOldReplicaSets` / `deleteResourcesForReplicaSet` / `replicaSetHasResources` in `internal/controller/volume_controller.go`, backed by `hasReplicas`, `resourceIsUsedByOtherReplicaSet`, and `currentReplicaSetIsAvailable` in `internal/controller/util.go`.
+
+### Cleanup on full deletion (owner references)
+
+The rollout cleanup above only fires on a live reconcile of *some* ReplicaSet in the set — but when the whole Deployment (and therefore every one of its ReplicaSets) is deleted outright (e.g. the owning OGCAPI CR itself is deleted), there is no ReplicaSet left to reconcile at all: `Reconcile` sees `Get` return `NotFound` and returns immediately, before `cleanUpOldReplicaSets` ever runs. This used to leave the AVP and PVC permanently orphaned (confirmed live against a test AKS cluster: deploy → delete the OGCAPI → Deployment/ReplicaSet gone in seconds, AVP/PVC left `Bound` indefinitely).
+
+The fix is a safety net independent of any reconcile: `createAvpIfNotExists` / `createPvcIfNotExists` set (or, if the resource already exists via hash-reuse, add to) an `ownerReference` from the AVP/PVC to the owning ReplicaSet, via `ensureOwnerReference` in `internal/controller/volume_controller.go`. Kubernetes' own garbage collector then deletes the AVP/PVC once **every** owning ReplicaSet is gone — no reconcile required.
+
+- Ownership is **additive, not exclusive**: because the resource-suffix hash can be shared across ReplicaSet revisions (see "Hash-based deduplication"), a reused AVP/PVC can carry owner references to more than one live ReplicaSet at once. `ensureOwnerReference` relies on `controllerutil.SetOwnerReference`, which upserts by (group, kind, name) rather than replacing the whole list, so an older revision's ownership survives until that specific ReplicaSet is actually gone.
+- `ensureOwnerReference` re-reads the object and sends a **merge patch with an optimistic lock** (`MergeFromWithOptimisticLock`), only when the owner list actually changed, retrying on `Conflict` (`retry.RetryOnConflict`). The lock is required, not cosmetic: a merge patch replaces the whole `ownerReferences` list, so without a `resourceVersion` precondition a stale cache read could drop another ReplicaSet's ownership. Patching rather than `Update` also avoids writing back the whole object, which would prune CRD fields unknown to the vendored AVP types. The client needs `patch` on PVCs and AVPs (this repo has no RBAC markers and `config/rbac/role.yaml` has no PVC rules, so check the ClusterRole where it is actually defined).
+- This is a *complement* to the explicit rollout cleanup above, not a replacement — the explicit path still deletes eagerly on scale-to-0 rather than waiting for a ReplicaSet object to eventually disappear (e.g. via `revisionHistoryLimit` trimming).
+- `createAvpIfNotExists` / `createPvcIfNotExists` also tolerate `AlreadyExists` on `Create` as a non-error: two near-simultaneous reconciles for the same new ReplicaSet can both read a not-yet-cached AVP/PVC as `NotFound` (informer cache lag) and race to create it — the loser's `Create` legitimately fails with `AlreadyExists`, which isn't a real failure since the desired object already exists. Without this, that race surfaced as noisy `ERROR Reconciler error` log lines (with stack traces) on effectively every normal rollout — confirmed live during the same debugging session.
 
 ### Hash-based deduplication
 

cmd/main.go

@@ -1,19 +1,3 @@
-/*
-Copyright 2025.
-
-Licensed under the Apache License, Version 2.0 (the "License");
-you may not use this file except in compliance with the License.
-You may obtain a copy of the License at
-
-    http://www.apache.org/licenses/LICENSE-2.0
-
-Unless required by applicable law or agreed to in writing, software
-distributed under the License is distributed on an "AS IS" BASIS,
-WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-See the License for the specific language governing permissions and
-limitations under the License.
-*/
-
 package main
 
 import (

internal/controller/suite_test.go

@@ -1,19 +1,3 @@
-/*
-Copyright 2025.
-
-Licensed under the Apache License, Version 2.0 (the "License");
-you may not use this file except in compliance with the License.
-You may obtain a copy of the License at
-
-    http://www.apache.org/licenses/LICENSE-2.0
-
-Unless required by applicable law or agreed to in writing, software
-distributed under the License is distributed on an "AS IS" BASIS,
-WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-See the License for the specific language governing permissions and
-limitations under the License.
-*/
-
 package controller
 
 import (

internal/controller/util_test.go

@@ -0,0 +1,106 @@
+package controller
+
+import (
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+
+	"github.com/PDOK/volume-operator/internal/config"
+	appsv1 "k8s.io/api/apps/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+func helperReplicaSet(name, revision, suffix string, replicas *int32, available int32) appsv1.ReplicaSet {
+	return appsv1.ReplicaSet{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      name,
+			Namespace: testNamespace,
+			Annotations: map[string]string{
+				config.RevisionAnnotation:       revision,
+				config.ResourceSuffixAnnotation: suffix,
+			},
+		},
+		Spec:   appsv1.ReplicaSetSpec{Replicas: replicas},
+		Status: appsv1.ReplicaSetStatus{AvailableReplicas: available},
+	}
+}
+
+func int32Ptr(i int32) *int32 { return &i }
+
+var _ = Describe("Controller helpers", func() {
+	Describe("hasReplicas", func() {
+		It("is false when Replicas is nil", func() {
+			Expect(hasReplicas(helperReplicaSet("rs", "1", "s", nil, 0))).To(BeFalse())
+		})
+
+		It("is false when Replicas is zero", func() {
+			Expect(hasReplicas(helperReplicaSet("rs", "1", "s", int32Ptr(0), 0))).To(BeFalse())
+		})
+
+		It("is true when Replicas is greater than zero", func() {
+			Expect(hasReplicas(helperReplicaSet("rs", "1", "s", int32Ptr(2), 0))).To(BeTrue())
+		})
+	})
+
+	Describe("currentReplicaSetIsAvailable", func() {
+		It("is false when the deployment revision is empty", func() {
+			list := appsv1.ReplicaSetList{Items: []appsv1.ReplicaSet{
+				helperReplicaSet("rs", "1", "s", int32Ptr(1), 1),
+			}}
+			Expect(currentReplicaSetIsAvailable(list, "")).To(BeFalse())
+		})
+
+		It("is false when no ReplicaSet carries the deployment revision", func() {
+			list := appsv1.ReplicaSetList{Items: []appsv1.ReplicaSet{
+				helperReplicaSet("rs", "1", "s", int32Ptr(1), 1),
+			}}
+			Expect(currentReplicaSetIsAvailable(list, "2")).To(BeFalse())
+		})
+
+		It("is false when the matching ReplicaSet has no available replicas", func() {
+			list := appsv1.ReplicaSetList{Items: []appsv1.ReplicaSet{
+				helperReplicaSet("rs", "2", "s", int32Ptr(1), 0),
+			}}
+			Expect(currentReplicaSetIsAvailable(list, "2")).To(BeFalse())
+		})
+
+		It("is true when the matching ReplicaSet has available replicas", func() {
+			list := appsv1.ReplicaSetList{Items: []appsv1.ReplicaSet{
+				helperReplicaSet("rs", "2", "s", int32Ptr(1), 3),
+			}}
+			Expect(currentReplicaSetIsAvailable(list, "2")).To(BeTrue())
+		})
+	})
+
+	Describe("resourceIsUsedByOtherReplicaSet", func() {
+		current := helperReplicaSet("current", "1", "shared", int32Ptr(0), 0)
+
+		It("ignores the ReplicaSet with the same name", func() {
+			list := appsv1.ReplicaSetList{Items: []appsv1.ReplicaSet{
+				helperReplicaSet("current", "1", "shared", int32Ptr(5), 5),
+			}}
+			Expect(resourceIsUsedByOtherReplicaSet(list, current)).To(BeFalse())
+		})
+
+		It("is false when the shared suffix is empty", func() {
+			empty := helperReplicaSet("current", "1", "", int32Ptr(0), 0)
+			list := appsv1.ReplicaSetList{Items: []appsv1.ReplicaSet{
+				helperReplicaSet("other", "1", "", int32Ptr(5), 5),
+			}}
+			Expect(resourceIsUsedByOtherReplicaSet(list, empty)).To(BeFalse())
+		})
+
+		It("is false when the other ReplicaSet with the same suffix has no replicas", func() {
+			list := appsv1.ReplicaSetList{Items: []appsv1.ReplicaSet{
+				helperReplicaSet("other", "2", "shared", int32Ptr(0), 0),
+			}}
+			Expect(resourceIsUsedByOtherReplicaSet(list, current)).To(BeFalse())
+		})
+
+		It("is true when another ReplicaSet with the same suffix still has replicas", func() {
+			list := appsv1.ReplicaSetList{Items: []appsv1.ReplicaSet{
+				helperReplicaSet("other", "2", "shared", int32Ptr(1), 1),
+			}}
+			Expect(resourceIsUsedByOtherReplicaSet(list, current)).To(BeTrue())
+		})
+	})
+})