Skip to content

Commit c6b5191

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request kubernetes#122499 from mattcary/oref
Respect controllers on PVCs for retention policy
2 parents 51f89c3 + d386d68 commit c6b5191

File tree

6 files changed

+1519
-275
lines changed

6 files changed

+1519
-275
lines changed

‎pkg/controller/statefulset/stateful_pod_control.go‎

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -220,7 +220,7 @@ func (spc *StatefulPodControl) ClaimsMatchRetentionPolicy(ctx context.Context, s
220220
case err != nil:
221221
return false, fmt.Errorf("Could not retrieve claim %s for %s when checking PVC deletion policy", claimName, pod.Name)
222222
default:
223-
if !claimOwnerMatchesSetAndPod(logger, claim, set, pod) {
223+
if !isClaimOwnerUpToDate(logger, claim, set, pod) {
224224
return false, nil
225225
}
226226
}
@@ -242,14 +242,16 @@ func (spc *StatefulPodControl) UpdatePodClaimForRetentionPolicy(ctx context.Cont
242242
case err != nil:
243243
return fmt.Errorf("Could not retrieve claim %s not found for %s when checking PVC deletion policy: %w", claimName, pod.Name, err)
244244
default:
245-
if !claimOwnerMatchesSetAndPod(logger, claim, set, pod) {
245+
if hasUnexpectedController(claim, set, pod) {
246+
// Add an event so the user knows they're in a strange configuration. The claim will be cleaned up below.
247+
msg := fmt.Sprintf("PersistentVolumeClaim %s has a conflicting OwnerReference that acts as a manging controller, the retention policy is ignored for this claim", claimName)
248+
spc.recorder.Event(set, v1.EventTypeWarning, "ConflictingController", msg)
249+
}
250+
if !isClaimOwnerUpToDate(logger, claim, set, pod) {
246251
claim = claim.DeepCopy() // Make a copy so we don't mutate the shared cache.
247-
needsUpdate := updateClaimOwnerRefForSetAndPod(logger, claim, set, pod)
248-
if needsUpdate {
249-
err := spc.objectMgr.UpdateClaim(claim)
250-
if err != nil {
251-
return fmt.Errorf("Could not update claim %s for delete policy ownerRefs: %w", claimName, err)
252-
}
252+
updateClaimOwnerRefForSetAndPod(logger, claim, set, pod)
253+
if err := spc.objectMgr.UpdateClaim(claim); err != nil {
254+
return fmt.Errorf("could not update claim %s for delete policy ownerRefs: %w", claimName, err)
253255
}
254256
}
255257
}
@@ -275,8 +277,7 @@ func (spc *StatefulPodControl) PodClaimIsStale(set *apps.StatefulSet, pod *v1.Po
275277
case err != nil:
276278
return false, err
277279
case err == nil:
278-
// A claim is stale if it doesn't match the pod's UID, including if the pod has no UID.
279-
if hasStaleOwnerRef(pvc, pod) {
280+
if hasStaleOwnerRef(pvc, pod, podKind) {
280281
return true, nil
281282
}
282283
}

‎pkg/controller/statefulset/stateful_pod_control_test.go‎

Lines changed: 72 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ import (
4141
_ "k8s.io/kubernetes/pkg/apis/apps/install"
4242
_ "k8s.io/kubernetes/pkg/apis/core/install"
4343
"k8s.io/kubernetes/pkg/features"
44+
"k8s.io/utils/ptr"
4445
)
4546

4647
func TestStatefulPodControlCreatesPods(t *testing.T) {
@@ -502,7 +503,7 @@ func TestStatefulPodControlDeleteFailure(t *testing.T) {
502503
}
503504

504505
func TestStatefulPodControlClaimsMatchDeletionPolcy(t *testing.T) {
505-
// The claimOwnerMatchesSetAndPod is tested exhaustively in stateful_set_utils_test; this
506+
// The isClaimOwnerUpToDate is tested exhaustively in stateful_set_utils_test; this
506507
// test is for the wiring to the method tested there.
507508
_, ctx := ktesting.NewTestContext(t)
508509
fakeClient := &fake.Clientset{}
@@ -542,38 +543,64 @@ func TestStatefulPodControlUpdatePodClaimForRetentionPolicy(t *testing.T) {
542543
testFn := func(t *testing.T) {
543544
_, ctx := ktesting.NewTestContext(t)
544545
featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StatefulSetAutoDeletePVC, true)
545-
fakeClient := &fake.Clientset{}
546-
indexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{cache.NamespaceIndex: cache.MetaNamespaceIndexFunc})
547-
claimLister := corelisters.NewPersistentVolumeClaimLister(indexer)
548-
fakeClient.AddReactor("update", "persistentvolumeclaims", func(action core.Action) (bool, runtime.Object, error) {
549-
update := action.(core.UpdateAction)
550-
indexer.Update(update.GetObject())
551-
return true, update.GetObject(), nil
552-
})
553-
set := newStatefulSet(3)
554-
set.GetObjectMeta().SetUID("set-123")
555-
pod := newStatefulSetPod(set, 0)
556-
claims := getPersistentVolumeClaims(set, pod)
557-
for k := range claims {
558-
claim := claims[k]
559-
indexer.Add(&claim)
560-
}
561-
control := NewStatefulPodControl(fakeClient, nil, claimLister, &noopRecorder{})
562-
set.Spec.PersistentVolumeClaimRetentionPolicy = &apps.StatefulSetPersistentVolumeClaimRetentionPolicy{
563-
WhenDeleted: apps.DeletePersistentVolumeClaimRetentionPolicyType,
564-
WhenScaled: apps.RetainPersistentVolumeClaimRetentionPolicyType,
565-
}
566-
if err := control.UpdatePodClaimForRetentionPolicy(ctx, set, pod); err != nil {
567-
t.Errorf("Unexpected error for UpdatePodClaimForRetentionPolicy (retain): %v", err)
546+
547+
testCases := []struct {
548+
name string
549+
ownerRef []metav1.OwnerReference
550+
expectRef bool
551+
}{
552+
{
553+
name: "bare PVC",
554+
expectRef: true,
555+
},
556+
{
557+
name: "PVC already controller",
558+
ownerRef: []metav1.OwnerReference{{Controller: ptr.To(true), Name: "foobar"}},
559+
expectRef: false,
560+
},
568561
}
569-
expectRef := utilfeature.DefaultFeatureGate.Enabled(features.StatefulSetAutoDeletePVC)
570-
for k := range claims {
571-
claim, err := claimLister.PersistentVolumeClaims(claims[k].Namespace).Get(claims[k].Name)
572-
if err != nil {
573-
t.Errorf("Unexpected error getting Claim %s/%s: %v", claim.Namespace, claim.Name, err)
562+
563+
for _, tc := range testCases {
564+
fakeClient := &fake.Clientset{}
565+
indexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{cache.NamespaceIndex: cache.MetaNamespaceIndexFunc})
566+
claimLister := corelisters.NewPersistentVolumeClaimLister(indexer)
567+
fakeClient.AddReactor("update", "persistentvolumeclaims", func(action core.Action) (bool, runtime.Object, error) {
568+
update := action.(core.UpdateAction)
569+
if err := indexer.Update(update.GetObject()); err != nil {
570+
t.Fatalf("could not update index: %v", err)
571+
}
572+
return true, update.GetObject(), nil
573+
})
574+
set := newStatefulSet(3)
575+
set.GetObjectMeta().SetUID("set-123")
576+
pod0 := newStatefulSetPod(set, 0)
577+
claims0 := getPersistentVolumeClaims(set, pod0)
578+
for k := range claims0 {
579+
claim := claims0[k]
580+
if tc.ownerRef != nil {
581+
claim.SetOwnerReferences(tc.ownerRef)
582+
}
583+
if err := indexer.Add(&claim); err != nil {
584+
t.Errorf("Could not add claim %s: %v", k, err)
585+
}
586+
}
587+
control := NewStatefulPodControl(fakeClient, nil, claimLister, &noopRecorder{})
588+
set.Spec.PersistentVolumeClaimRetentionPolicy = &apps.StatefulSetPersistentVolumeClaimRetentionPolicy{
589+
WhenDeleted: apps.DeletePersistentVolumeClaimRetentionPolicyType,
590+
WhenScaled: apps.RetainPersistentVolumeClaimRetentionPolicyType,
591+
}
592+
if err := control.UpdatePodClaimForRetentionPolicy(ctx, set, pod0); err != nil {
593+
t.Errorf("Unexpected error for UpdatePodClaimForRetentionPolicy (retain), pod0: %v", err)
574594
}
575-
if hasOwnerRef(claim, set) != expectRef {
576-
t.Errorf("Claim %s/%s bad set owner ref", claim.Namespace, claim.Name)
595+
expectRef := tc.expectRef && utilfeature.DefaultFeatureGate.Enabled(features.StatefulSetAutoDeletePVC)
596+
for k := range claims0 {
597+
claim, err := claimLister.PersistentVolumeClaims(claims0[k].Namespace).Get(claims0[k].Name)
598+
if err != nil {
599+
t.Errorf("Unexpected error getting Claim %s/%s: %v", claim.Namespace, claim.Name, err)
600+
}
601+
if hasOwnerRef(claim, set) != expectRef {
602+
t.Errorf("%s: Claim %s/%s bad set owner ref", tc.name, claim.Namespace, claim.Name)
603+
}
577604
}
578605
}
579606
}
@@ -663,12 +690,22 @@ func TestPodClaimIsStale(t *testing.T) {
663690
claimIndexer.Add(&claim)
664691
case stale:
665692
claim.SetOwnerReferences([]metav1.OwnerReference{
666-
{Name: "set-3", UID: types.UID("stale")},
693+
{
694+
Name: "set-3",
695+
UID: types.UID("stale"),
696+
APIVersion: "v1",
697+
Kind: "Pod",
698+
},
667699
})
668700
claimIndexer.Add(&claim)
669701
case withRef:
670702
claim.SetOwnerReferences([]metav1.OwnerReference{
671-
{Name: "set-3", UID: types.UID("123")},
703+
{
704+
Name: "set-3",
705+
UID: types.UID("123"),
706+
APIVersion: "v1",
707+
Kind: "Pod",
708+
},
672709
})
673710
claimIndexer.Add(&claim)
674711
}
@@ -710,7 +747,8 @@ func TestStatefulPodControlRetainDeletionPolicyUpdate(t *testing.T) {
710747
}
711748
for k := range claims {
712749
claim := claims[k]
713-
setOwnerRef(&claim, set, &set.TypeMeta) // This ownerRef should be removed in the update.
750+
// This ownerRef should be removed in the update.
751+
claim.SetOwnerReferences(addControllerRef(claim.GetOwnerReferences(), set, controllerKind))
714752
claimIndexer.Add(&claim)
715753
}
716754
control := NewStatefulPodControl(fakeClient, podLister, claimLister, recorder)

‎pkg/controller/statefulset/stateful_set.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,9 @@ import (
4949
// controllerKind contains the schema.GroupVersionKind for this controller type.
5050
var controllerKind = apps.SchemeGroupVersion.WithKind("StatefulSet")
5151

52+
// podKind contains the schema.GroupVersionKind for pods.
53+
var podKind = v1.SchemeGroupVersion.WithKind("Pod")
54+
5255
// StatefulSetController controls statefulsets.
5356
type StatefulSetController struct {
5457
// client interface

0 commit comments

Comments
 (0)