Respect controllers on PVCs for retention policy #122499

mattcary · 2023-12-27T20:57:27Z

/kind bug

StatefulSet autodelete will respect controlling owners on PVC claims as described in https://github.com/kubernetes/enhancements/pull/4375

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

KEP #1847

mattcary · 2023-12-27T20:58:23Z

/sig apps
/priority-soon

mattcary · 2023-12-27T20:58:55Z

/triage accepted

mattcary · 2023-12-27T21:00:41Z

/priority important-soon

msau42 · 2023-12-28T19:07:45Z

/lgtm
/approve

k8s-ci-robot · 2023-12-28T19:07:52Z

LGTM label has been added.

Git tree hash: 3af13a5ff2cc1cd46ea27a3174fd1f2f9597b125

carlory · 2024-01-08T07:33:58Z

/assign @soltysh

atiratree

@mattcary I found couple of problems with the current implementation

I feel that hasOwnerRef should have a bool checkController parameter. And we should not check the controller flag if we want to delete the owner ref, but we should check it if we want to introduce it / overwrite it.

I think this goes for all the cases in both claimOwnerMatchesSetAndPod and updateClaimOwnerRefForSetAndPod functions.

+ would be good to document why we are doing it

atiratree · 2024-01-12T15:49:07Z

pkg/controller/statefulset/stateful_set_utils.go

@@ -305,6 +333,7 @@ func setOwnerRef(target, owner metav1.Object, ownerType *metav1.TypeMeta) bool {
 			Kind:       ownerType.Kind,
 			Name:       owner.GetName(),
 			UID:        owner.GetUID(),
+			Controller: ptr.To(true),


If there is already existing owner reference, this append will duplicate it. So there will be one reference without a controller and one with. Can we deduplicate it?

~~hasOwnerRef checks that there is not an owner with the same UID, regardless of the value of the controller.~~ Oops wait I did add that check to controller in there. Thx

Although, it occurs to me, if the StatefulSet is updated, the UID will change. So we probably want hasOwnerRef to check on api version + kind + name + namespace, which will be unique?

What do you mean by updated? Normally the UID shouldn't change and should be present and is used by the garbage collector. And if a person deletes the StatefulSet using the orphan strategy it should also remove the owner reference.

Ah I didn't know the ownerref was removed with the orphan strategy.

Mostly I think I got confused.

I guess the problem could be a pvc that's slow to delete, and a statefulset that's deleted and recreated (the same case the stale pod claim check deals with). In that case though a new pod won't bind to a deleted PVC, and so it should eventually delete and the statefulset controller will recreate a new PVC.

So yeah, we should just look at UIDs (except for the stale pod case).

atiratree · 2024-01-12T15:55:03Z

pkg/controller/statefulset/stateful_set_utils.go

@@ -305,6 +333,7 @@ func setOwnerRef(target, owner metav1.Object, ownerType *metav1.TypeMeta) bool {
 			Kind:       ownerType.Kind,
 			Name:       owner.GetName(),
 			UID:        owner.GetUID(),
+			Controller: ptr.To(true),
 		})
 	target.SetOwnerReferences(ownerRefs)
 	return true


also removeOwnerRef seems it will not work correctly if there is an owner reference in the old format because hasOwnerRef is more strict now

+1 this will be fixed

atiratree · 2024-01-12T16:32:05Z

pkg/controller/statefulset/stateful_pod_control.go

+		case hasUnexpectedController(claim, set, pod):
+			// Do not set any ownerRefs if there is an unexpected controller.
+			msg := fmt.Sprintf("Claim %s has a conflicting controller, the retention policy is ignored for this claim", claimName)
+			spc.recorder.Event(set, v1.EventTypeWarning, "ConflictingController", msg)


also this part from the KEP seems not to be satisfied and should apply for all policy combinations:

If there is another controller, the existing non-controller StatefulSet or Pod reference will be removed.

Thanks for taking a look at this! You make some good points.

Let's resolve the conversation on the KEP first about what the proper semantics should be, then I'll work through your comments here -- you raise some excellent points.

argh, you are correct. Thank you.

Change is in progress. It was better to make a little more complete re-vamp of the logic to keep it from getting too complicated.

Thanks for looking at that. And yeah, there are a lot of cases that need to be covered. It would be nice to have them all covered by the tests.

atiratree

@mattcary there are still some nits and testing considerations

pkg/controller/statefulset/stateful_set_utils_test.go

atiratree · 2024-04-30T19:22:12Z

pkg/controller/statefulset/stateful_set_utils.go

+// nonController returns true if the pod or set is an owner but not controller of the claim.
+func nonController(claim *v1.PersistentVolumeClaim, set *apps.StatefulSet, pod *v1.Pod) bool {
+	for _, ownerRef := range claim.GetOwnerReferences() {
+		if ownerRef.UID == set.GetUID() || ownerRef.UID == pod.GetUID() {


good, although it would be better to have it reflected in the function name as well

pkg/controller/statefulset/stateful_set_utils.go

atiratree · 2024-04-30T19:45:07Z

pkg/controller/statefulset/stateful_set_utils_test.go

+		claim := v1.PersistentVolumeClaim{}
+		claim.SetOwnerReferences(tc.originalRefs)
+		updateClaimOwnerRefForSetAndPod(logger, &claim, &set, &pod)
+		if ownerRefsChanged(tc.expectedRefs, claim.GetOwnerReferences()) {


Okay. In that case, I think it would be appropriate to have a unit test for the new function.

atiratree · 2024-04-30T19:55:17Z

pkg/controller/statefulset/stateful_set_utils.go

+	}
+
+	if hasNonController(claim, set, pod) {
+		return false


Some other resource besides our set or pod has an owner ref

It seems that other resources do not necessarily have to be registered as owners in the references to get to this line. That is, it can be just one owner reference (e.g. our set or pod).

atiratree · 2024-04-30T22:04:37Z

test/e2e/apps/statefulset.go

+					BlockOwnerDeletion: ptr.To(true),
+				},
+			}
+			ginkgo.By("Expect claims 0 and 2 to have ownerRefs to the statefulset, and 3 to have a stale reference")


TY. Let's also change the description:

Suggested change

ginkgo.By("Expect claims 0 and 2 to have ownerRefs to the statefulset, and 3 to have a stale reference")

ginkgo.By("Expect claims 0,1 and 2 to have ownerRefs to the statefulset, and 3 to have a stale reference")

Oops, thx, done.

test/e2e/apps/statefulset.go

atiratree

There are 2 nits, but the PR lgtm.

Nevertheless, it would be good to get another set of eyes on it.

cc @soltysh

test/e2e/apps/statefulset.go

atiratree · 2024-05-08T09:20:56Z

unit test flaking due to #124526
/test pull-kubernetes-unit

soltysh

A few questions and suggestions to resolve before the merge.

soltysh · 2024-06-04T10:18:00Z

test/e2e/apps/statefulset.go

+
+			ginkgo.By("Remove PVC 1 owner ref")
+			pvc.OwnerReferences = nil
+			_, err = c.CoreV1().PersistentVolumeClaims(ns).Update(ctx, pvc, metav1.UpdateOptions{})


I've tried running this test locally and the first two invocations I hit:

[FAILED] Operation cannot be fulfilled on persistentvolumeclaims "datadir-ss-1": the object has been modified; please apply your changes to the latest version and try again In [It] at: k8s.io/kubernetes/test/e2e/apps/statefulset.go:1402 @ 06/04/24 12:13:52.29

so I'd suggest to go through all the PVCs update operations and make them retriable. Similarly StatefulSet updates should be changed to use updateStatefrulSetWithRetries most likely.

Done.

Interesting, I've been using kind and never hit that. But +1 to fewer flakes.

soltysh · 2024-06-04T10:43:54Z

pkg/controller/statefulset/stateful_set_utils.go

-// claimOwnerMatchesSetAndPod returns false if the ownerRefs of the claim are not set consistently with the
+// matchesRef returns true when the object matches the owner reference, that is the name and GVK are the same.
+func matchesRef(ref *metav1.OwnerReference, obj metav1.Object, gvk schema.GroupVersionKind) bool {
+	return gvk.GroupVersion().String() == ref.APIVersion && gvk.Kind == ref.Kind && ref.Name == obj.GetName()


Why not using IsControlledBy from k8s.io/apimachinery/pkg/apis/meta/v1

That matches on the UID, not on the GVK & name. So if, eg, the statefulset was updated, IsControlledBy won't return true until the UID is updated? In particular, matchRef is used to update stale UIDs.

soltysh · 2024-06-04T10:46:39Z

pkg/controller/statefulset/stateful_set_utils.go

+				return true
+			}
+			continue // This is us.
+		}


Why do we need to match both statefulset and pod in the loop above?

PVC are controlled by the statefulset for the WhenDeleted retention type, and controlled by the pod for the WhenScaled retention type.

soltysh · 2024-06-04T10:48:56Z

pkg/controller/statefulset/stateful_set_utils.go

+		return true
+	}
+
+	if hasNonControllerOwner(claim, set, pod) {


I don't understand why we should react in such a situation? It doesn't matter that we don't own it, and furthermore who owns it. We just don't react on it.

At most we can warn the user about such situation.

The implementation before this change makes statefulsets & pods owners but not controllers -- that's what this PR is fixing. So we have to handle at least that case to correctly update existing statefulsets from before this bugfix.

soltysh

/lgtm
/approve

k8s-ci-robot · 2024-06-07T15:25:41Z

LGTM label has been added.

Git tree hash: ba293b2cf2ee1f935175f95b7d0ef82769516a45

k8s-ci-robot · 2024-06-07T15:26:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mattcary, msau42, soltysh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/controller/statefulset/OWNERS~~ [soltysh]
~~test/e2e/apps/OWNERS~~ [msau42,soltysh]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mattcary · 2024-06-07T18:05:34Z

/cherry-pick release-1.30

^^ This didn't work, #125389 created manually instead.

mattcary · 2024-08-07T16:17:46Z

/cherry-pick release-1.29

^^ I guess this doesn't work. #126580 created

mattcary · 2024-08-07T16:17:54Z

/cherry-pick release-1.28

^^ #126581 created.

…se-1.30 Cherry pick #122499 to release-1.30

k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Dec 27, 2023

k8s-ci-robot assigned msau42 Dec 27, 2023

k8s-ci-robot requested review from mortent and smarterclayton December 27, 2023 20:58

k8s-ci-robot added sig/apps Categorizes an issue or PR as relevant to SIG Apps. area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Dec 27, 2023

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Dec 27, 2023

k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Dec 27, 2023

mattcary force-pushed the oref branch from 38b3402 to a9946f4 Compare December 27, 2023 21:47

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 28, 2023

k8s-ci-robot assigned soltysh Jan 8, 2024

atiratree reviewed Jan 12, 2024

View reviewed changes

mattcary force-pushed the oref branch from a9946f4 to e17aced Compare January 21, 2024 00:35

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 26, 2024

mattcary force-pushed the oref branch from a5d3cae to 63e8a8e Compare April 29, 2024 16:48

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 30, 2024

atiratree reviewed Apr 30, 2024

View reviewed changes

mattcary force-pushed the oref branch 3 times, most recently from fc472e6 to 8400017 Compare May 7, 2024 19:46

atiratree reviewed May 7, 2024

View reviewed changes

test/e2e/apps/statefulset.go Outdated Show resolved Hide resolved

test/e2e/apps/statefulset.go Show resolved Hide resolved

mattcary force-pushed the oref branch from 8400017 to 2a5595d Compare May 7, 2024 21:43

soltysh reviewed Jun 4, 2024

View reviewed changes

mattcary force-pushed the oref branch 2 times, most recently from 6b706a7 to a42e6e4 Compare June 5, 2024 18:16

Respect controllers on PVCs for retention policy

d386d68

mattcary force-pushed the oref branch from a42e6e4 to d386d68 Compare June 5, 2024 19:07

soltysh approved these changes Jun 7, 2024

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 7, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 7, 2024

k8s-ci-robot merged commit c6b5191 into kubernetes:master Jun 7, 2024
15 checks passed

k8s-ci-robot added this to the v1.31 milestone Jun 7, 2024

mattcary mentioned this pull request Jun 7, 2024

Cherry pick #122499 to release-1.30 #125389

Merged

This was referenced Aug 7, 2024

Respect controllers on PVCs for retention policy #126580

Merged

Respect controllers on PVCs for retention policy #126581

Merged

k8s-ci-robot added a commit that referenced this pull request Aug 9, 2024

Merge pull request #125389 from mattcary/cherry-pick-#122499-to-relea…

4e3e56d

…se-1.30 Cherry pick #122499 to release-1.30

atiratree mentioned this pull request Aug 26, 2024

Auto remove PVCs created by StatefulSet kubernetes/enhancements#1847

Open

8 tasks

	ginkgo.By("Expect claims 0 and 2 to have ownerRefs to the statefulset, and 3 to have a stale reference")
	ginkgo.By("Expect claims 0,1 and 2 to have ownerRefs to the statefulset, and 3 to have a stale reference")

Respect controllers on PVCs for retention policy #122499

Respect controllers on PVCs for retention policy #122499

Conversation

mattcary commented Dec 27, 2023 • edited Loading

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

mattcary commented Dec 27, 2023

mattcary commented Dec 27, 2023

mattcary commented Dec 27, 2023

msau42 commented Dec 28, 2023

k8s-ci-robot commented Dec 28, 2023

carlory commented Jan 8, 2024

atiratree left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattcary Jan 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atiratree left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atiratree left a comment

Choose a reason for hiding this comment

atiratree commented May 8, 2024

soltysh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattcary Jun 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soltysh left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 7, 2024

k8s-ci-robot commented Jun 7, 2024

mattcary commented Jun 7, 2024 • edited Loading

mattcary commented Aug 7, 2024 • edited Loading

mattcary commented Aug 7, 2024 • edited Loading

mattcary commented Dec 27, 2023 •

edited

Loading

atiratree left a comment •

edited

Loading

mattcary Jan 16, 2024 •

edited

Loading

mattcary Jun 5, 2024 •

edited

Loading

mattcary commented Jun 7, 2024 •

edited

Loading

mattcary commented Aug 7, 2024 •

edited

Loading

mattcary commented Aug 7, 2024 •

edited

Loading