Skip to content

Commit

Permalink
Respect controllers on PVCs for retention policy
Browse files Browse the repository at this point in the history
  • Loading branch information
@mattcary
mattcary committed Apr 29, 2024
1 parent 72524fa commit 63e8a8e
Show file tree
Hide file tree
Showing 6 changed files with 1,407 additions and 270 deletions.
21 changes: 11 additions & 10 deletions pkg/controller/statefulset/stateful_pod_control.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
Expand All @@ -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)
}
}
}
Expand All @@ -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
}
}
Expand Down
106 changes: 72 additions & 34 deletions pkg/controller/statefulset/stateful_pod_control_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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{}
Expand Down Expand Up @@ -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)
}
}
}
}
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
Expand Down
3 changes: 3 additions & 0 deletions pkg/controller/statefulset/stateful_set.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 63e8a8e

Please sign in to comment.