StatefulSet PVC auto-delete implementation #99728

mattcary · 2021-03-03T21:21:16Z

/kind feature
/sig apps

What this PR does / why we need it:

This contains the implementation for the StatefulSet PVC auto-delete KEP

Which issue(s) this PR address:

Special notes for your reviewer:

This PR is unfortunately rather large.

The first two api commits were originally submitted in #99378, which was reverted when the implementation didn't get finished in time for 1.22.

The next three are this change: the implementation, unit tests, and finally the feature gate.

The unit tests required a bit of refactoring as the faking was done at the wrong level to efficiently test the new behavior. So it's quite large, but splitting it up would be very difficult at the point :-(

The final commit is the feature gate. I think I'm doing it the right way based on other feature gate implementations, but please let me know if I missed some key bit.

Does this PR introduce a user-facing change?

Adds a feature gate StatefulSetAutoDeletePVC, which allows PVCs automatically created for StatefulSet pods to be automatically deleted.

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

- KEP: https://github.com/kubernetes/enhancements/tree/master/keps/sig-apps/1847-autoremove-statefulset-pvcs

mattcary · 2021-03-03T21:21:36Z

/cc @kk-src
/cc @msau42

k8s-ci-robot · 2021-03-03T21:21:37Z

@mattcary: GitHub didn't allow me to request PR reviews from the following users: kk-src.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @kk-src
/cc @msau42

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

fejta-bot · 2021-03-03T22:18:29Z

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

msau42 · 2021-03-08T22:39:38Z

pkg/controller/statefulset/stateful_pod_control.go

+		claimName := getPersistentVolumeClaimName(set, &templates[i], ordinal)
+		claim, err := spc.objectMgr.GetClaim(templates[i].Namespace, claimName)
+		switch {
+		case apierrors.IsNotFound(err):


I'm wondering if we should error or we should log a warning and continue. This would most likely happen if someone deleted the object out of band?

This also happens when everything is getting created IIUC. In UpdateStatefulPod, above, we ignore an error from this method when appropriate.

I think it's nicer to do it that way, where the caller knows when it's ok to ignore missing things, than to plumb that logic down here.

I may eat my words on this when we do more complicated test cases, but for the time being at least to me it makes it easier to reason about.

If objectMgr is fed by an informer, this in an inherently racy check (create object A, check cache until B shows up). Generally we don't warn in those cases, because that's creating log spam (the race is roughly guaranteed to always happen at least once on the first pass, and depending on watch latency, possibly longer).

If a PVC doesn't exist, retention is irrelevant. So I'd probably say apierrors.IsNotFound should not cause an error to be returned, and should log at a high level (i.e. not be visible except during debugging such as 4 or 5). It doesn't matter whether it's a race on startup, a user driven deletion, or a bug elsewhere in the controller - the controller has to accept that cross object comparisons driven by inherently laggy caches can't be done transactionally, and the way we do it is break our checks into simple invariants (if no PVC exists but should -> try to create, if PVC shouldn't exist but does -> try to destroy) and minimizing the amount of noise we create in logs and events.

I think that makes sense. Done.

msau42 · 2021-03-08T22:41:08Z

pkg/controller/statefulset/stateful_pod_control.go

+		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) {
+				fmt.Printf("%s/%s on %s/%v\n", pod.Name, pod.GetUID(), pvc.Name, pvc.GetOwnerReferences())


Should use klog to log

Yikes this is printf debugging that leaked out from the commit with the test :-P. fixed.

msau42 · 2021-03-08T22:44:11Z

pkg/controller/statefulset/stateful_set_control.go

+			if isStale, err := ssc.podControl.PodClaimIsStale(set, replicas[i]); err != nil {
+				return &status, err
+			} else if isStale {
+				// If a pod has a stale PVC, no more work can be done this round.


What if someone manually created the PVC and didn't put an ownerref on it?

That case of manual pod creation will be caught in UpdateStatefulPod.

A stale owner ref is where there's a ref to a different pod, meaning the race condition during a scale-down followed immediately by a scale back up before the PVC can get deleted. Since things may be in process of getting unmounted it seems best to just wait for everything to settle down.

msau42 · 2021-03-08T22:44:45Z

pkg/controller/statefulset/stateful_pod_control.go

@@ -108,23 +145,42 @@ func (spc *realStatefulPodControl) UpdateStatefulPod(set *apps.StatefulSet, pod
 				return err
 			}
 		}
+		// if the Pod's PVCs are not consistent with the StatefulSet's PVC deletion policy, update the PVC


Any new logic in the controller should be feature gated.

Oops I'm reviewing a commit at a time and missed 89caf6d

Sorry, I thought it would be easier to split that out. I'm overthinking it!

The feature gate checks happen in this commit now (see getPersistentVolumeClaimDeletePolicy in stateful-set_utils.go).

Also note that we will need to update claim ownerRefs if the feature gate is rolled back. So the feature gate check has to happen deeper down anyway.

From a code organization perspective the feature gate is buried deeply in the utils. Its hard to read the code and determine the controller behavior when its enabled/disabled.

I see with the StatefulSetMinReadySeconds that the feature gate testing is hoisted up as you describe. So I've done the same with this feature.

msau42 · 2021-03-08T22:47:48Z

pkg/controller/statefulset/stateful_pod_control.go

+			// Otherwise the error may be due to some other update, and we'll have to reconcile the next time around.
+			// TODO: does the reconcile need to be sped up?
+		} else if !match {
+			if err := spc.UpdatePodClaimForDeletionPolicy(set, pod); err != nil {


Maybe something to consider as a future optimization, but we could save an update if we create+update in one call.

yes, although it gets complicated because if the pod is new as well it may not have the UID necessary for the owner ref.

So +1 to future optimization :-)

msau42 · 2021-03-08T22:59:52Z