Respect controllers on PVCs for retention policy

Author: mattcary

pkg/controller/statefulset/stateful_pod_control.go

@@ -220,7 +220,7 @@ func (spc *StatefulPodControl) ClaimsMatchRetentionPolicy(ctx context.Context, s
 		case err != nil:
 			return false, fmt.Errorf("Could not retrieve claim %s for %s when checking PVC deletion policy", claimName, pod.Name)
 		default:
-			if !claimOwnerMatchesSetAndPod(logger, claim, set, pod) {
+			if !isClaimOwnerUpToDate(logger, claim, set, pod) {
 				return false, nil
 			}
 		}
@@ -242,14 +242,16 @@ func (spc *StatefulPodControl) UpdatePodClaimForRetentionPolicy(ctx context.Cont
 		case err != nil:
 			return fmt.Errorf("Could not retrieve claim %s not found for %s when checking PVC deletion policy: %w", claimName, pod.Name, err)
 		default:
-			if !claimOwnerMatchesSetAndPod(logger, claim, set, pod) {
+			if hasUnexpectedController(claim, set, pod) {
+				// Add an event so the user knows they're in a strange configuration. The claim will be cleaned up below.
+				msg := fmt.Sprintf("PersistentVolumeClaim %s has a conflicting OwnerReference that acts as a manging controller, the retention policy is ignored for this claim", claimName)
+				spc.recorder.Event(set, v1.EventTypeWarning, "ConflictingController", msg)
+			}
+			if !isClaimOwnerUpToDate(logger, claim, set, pod) {
 				claim = claim.DeepCopy() // Make a copy so we don't mutate the shared cache.
-				needsUpdate := updateClaimOwnerRefForSetAndPod(logger, claim, set, pod)
-				if needsUpdate {
-					err := spc.objectMgr.UpdateClaim(claim)
-					if err != nil {
-						return fmt.Errorf("Could not update claim %s for delete policy ownerRefs: %w", claimName, err)
-					}
+				updateClaimOwnerRefForSetAndPod(logger, claim, set, pod)
+				if err := spc.objectMgr.UpdateClaim(claim); err != nil {
+					return fmt.Errorf("could not update claim %s for delete policy ownerRefs: %w", claimName, err)
 				}
 			}
 		}
@@ -275,8 +277,7 @@ func (spc *StatefulPodControl) PodClaimIsStale(set *apps.StatefulSet, pod *v1.Po
 		case err != nil:
 			return false, err
 		case err == nil:
-			// A claim is stale if it doesn't match the pod's UID, including if the pod has no UID.
-			if hasStaleOwnerRef(pvc, pod) {
+			if hasStaleOwnerRef(pvc, pod, podKind) {
 				return true, nil
 			}
 		}

pkg/controller/statefulset/stateful_pod_control_test.go

@@ -41,6 +41,7 @@ import (
 	_ "k8s.io/kubernetes/pkg/apis/apps/install"
 	_ "k8s.io/kubernetes/pkg/apis/core/install"
 	"k8s.io/kubernetes/pkg/features"
+	"k8s.io/utils/ptr"
 )
 
 func TestStatefulPodControlCreatesPods(t *testing.T) {
@@ -502,7 +503,7 @@ func TestStatefulPodControlDeleteFailure(t *testing.T) {
 }
 
 func TestStatefulPodControlClaimsMatchDeletionPolcy(t *testing.T) {
-	// The claimOwnerMatchesSetAndPod is tested exhaustively in stateful_set_utils_test; this
+	// The isClaimOwnerUpToDate is tested exhaustively in stateful_set_utils_test; this
 	// test is for the wiring to the method tested there.
 	_, ctx := ktesting.NewTestContext(t)
 	fakeClient := &fake.Clientset{}
@@ -542,38 +543,64 @@ func TestStatefulPodControlUpdatePodClaimForRetentionPolicy(t *testing.T) {
 	testFn := func(t *testing.T) {
 		_, ctx := ktesting.NewTestContext(t)
 		featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StatefulSetAutoDeletePVC, true)
-		fakeClient := &fake.Clientset{}
-		indexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{cache.NamespaceIndex: cache.MetaNamespaceIndexFunc})
-		claimLister := corelisters.NewPersistentVolumeClaimLister(indexer)
-		fakeClient.AddReactor("update", "persistentvolumeclaims", func(action core.Action) (bool, runtime.Object, error) {
-			update := action.(core.UpdateAction)
-			indexer.Update(update.GetObject())
-			return true, update.GetObject(), nil
-		})
-		set := newStatefulSet(3)
-		set.GetObjectMeta().SetUID("set-123")
-		pod := newStatefulSetPod(set, 0)
-		claims := getPersistentVolumeClaims(set, pod)
-		for k := range claims {
-			claim := claims[k]
-			indexer.Add(&claim)
-		}
-		control := NewStatefulPodControl(fakeClient, nil, claimLister, &noopRecorder{})
-		set.Spec.PersistentVolumeClaimRetentionPolicy = &apps.StatefulSetPersistentVolumeClaimRetentionPolicy{
-			WhenDeleted: apps.DeletePersistentVolumeClaimRetentionPolicyType,
-			WhenScaled:  apps.RetainPersistentVolumeClaimRetentionPolicyType,
-		}
-		if err := control.UpdatePodClaimForRetentionPolicy(ctx, set, pod); err != nil {
-			t.Errorf("Unexpected error for UpdatePodClaimForRetentionPolicy (retain): %v", err)
+
+		testCases := []struct {
+			name      string
+			ownerRef  []metav1.OwnerReference
+			expectRef bool
+		}{
+			{
+				name:      "bare PVC",
+				expectRef: true,
+			},
+			{
+				name:      "PVC already controller",
+				ownerRef:  []metav1.OwnerReference{{Controller: ptr.To(true), Name: "foobar"}},
+				expectRef: false,
+			},
 		}
-		expectRef := utilfeature.DefaultFeatureGate.Enabled(features.StatefulSetAutoDeletePVC)
-		for k := range claims {
-			claim, err := claimLister.PersistentVolumeClaims(claims[k].Namespace).Get(claims[k].Name)
-			if err != nil {
-				t.Errorf("Unexpected error getting Claim %s/%s: %v", claim.Namespace, claim.Name, err)
+
+		for _, tc := range testCases {
+			fakeClient := &fake.Clientset{}
+			indexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{cache.NamespaceIndex: cache.MetaNamespaceIndexFunc})
+			claimLister := corelisters.NewPersistentVolumeClaimLister(indexer)
+			fakeClient.AddReactor("update", "persistentvolumeclaims", func(action core.Action) (bool, runtime.Object, error) {
+				update := action.(core.UpdateAction)
+				if err := indexer.Update(update.GetObject()); err != nil {
+					t.Fatalf("could not update index: %v", err)
+				}
+				return true, update.GetObject(), nil
+			})
+			set := newStatefulSet(3)
+			set.GetObjectMeta().SetUID("set-123")
+			pod0 := newStatefulSetPod(set, 0)
+			claims0 := getPersistentVolumeClaims(set, pod0)
+			for k := range claims0 {
+				claim := claims0[k]
+				if tc.ownerRef != nil {
+					claim.SetOwnerReferences(tc.ownerRef)
+				}
+				if err := indexer.Add(&claim); err != nil {
+					t.Errorf("Could not add claim %s: %v", k, err)
+				}
+			}
+			control := NewStatefulPodControl(fakeClient, nil, claimLister, &noopRecorder{})
+			set.Spec.PersistentVolumeClaimRetentionPolicy = &apps.StatefulSetPersistentVolumeClaimRetentionPolicy{
+				WhenDeleted: apps.DeletePersistentVolumeClaimRetentionPolicyType,
+				WhenScaled:  apps.RetainPersistentVolumeClaimRetentionPolicyType,
+			}
+			if err := control.UpdatePodClaimForRetentionPolicy(ctx, set, pod0); err != nil {
+				t.Errorf("Unexpected error for UpdatePodClaimForRetentionPolicy (retain), pod0: %v", err)
 			}
-			if hasOwnerRef(claim, set) != expectRef {
-				t.Errorf("Claim %s/%s bad set owner ref", claim.Namespace, claim.Name)
+			expectRef := tc.expectRef && utilfeature.DefaultFeatureGate.Enabled(features.StatefulSetAutoDeletePVC)
+			for k := range claims0 {
+				claim, err := claimLister.PersistentVolumeClaims(claims0[k].Namespace).Get(claims0[k].Name)
+				if err != nil {
+					t.Errorf("Unexpected error getting Claim %s/%s: %v", claim.Namespace, claim.Name, err)
+				}
+				if hasOwnerRef(claim, set) != expectRef {
+					t.Errorf("%s: Claim %s/%s bad set owner ref", tc.name, claim.Namespace, claim.Name)
+				}
 			}
 		}
 	}
@@ -663,12 +690,22 @@ func TestPodClaimIsStale(t *testing.T) {
 				claimIndexer.Add(&claim)
 			case stale:
 				claim.SetOwnerReferences([]metav1.OwnerReference{
-					{Name: "set-3", UID: types.UID("stale")},
+					{
+						Name:       "set-3",
+						UID:        types.UID("stale"),
+						APIVersion: "v1",
+						Kind:       "Pod",
+					},
 				})
 				claimIndexer.Add(&claim)
 			case withRef:
 				claim.SetOwnerReferences([]metav1.OwnerReference{
-					{Name: "set-3", UID: types.UID("123")},
+					{
+						Name:       "set-3",
+						UID:        types.UID("123"),
+						APIVersion: "v1",
+						Kind:       "Pod",
+					},
 				})
 				claimIndexer.Add(&claim)
 			}
@@ -710,7 +747,8 @@ func TestStatefulPodControlRetainDeletionPolicyUpdate(t *testing.T) {
 		}
 		for k := range claims {
 			claim := claims[k]
-			setOwnerRef(&claim, set, &set.TypeMeta) // This ownerRef should be removed in the update.
+			// This ownerRef should be removed in the update.
+			claim.SetOwnerReferences(addControllerRef(claim.GetOwnerReferences(), set, controllerKind))
 			claimIndexer.Add(&claim)
 		}
 		control := NewStatefulPodControl(fakeClient, podLister, claimLister, recorder)

pkg/controller/statefulset/stateful_set.go

@@ -49,6 +49,9 @@ import (
 // controllerKind contains the schema.GroupVersionKind for this controller type.
 var controllerKind = apps.SchemeGroupVersion.WithKind("StatefulSet")
 
+// podKind contains the schema.GroupVersionKind for pods.
+var podKind = v1.SchemeGroupVersion.WithKind("Pod")
+
 // StatefulSetController controls statefulsets.
 type StatefulSetController struct {
 	// client interface