enable cloneset deleting pvc when pod hanging #1113

willise · 2022-11-01T11:55:12Z

Ⅰ. Describe what this PR does

fixes #1099

Ⅱ. Does this pull request fix one issue?

fixes #1099

Ⅲ. Describe how to verify it

Create a cloneset with setting "cleanUpPVC=false" and PVC
Edit a pod created by cloneset with a finalizer
Delete the above pod and verify the new pod can't be created with name conflict
Edit cloneset with setting "cleanUpPVC=true"
Verify a new pod with new PVC has been created and old pod hangs in terminating state.

Ⅳ. Special notes for reviews

kruise-bot · 2022-11-01T11:55:28Z

Welcome @willise! It looks like this is your first PR to openkruise/kruise 🎉

codecov-commenter · 2022-11-01T12:06:33Z

Codecov Report

Merging #1113 (ec8d173) into master (53fb7e1) will decrease coverage by 0.39%.
The diff coverage is 18.91%.

@@            Coverage Diff             @@
##           master    #1113      +/-   ##
==========================================
- Coverage   49.73%   49.33%   -0.40%     
==========================================
  Files         137      138       +1     
  Lines       19331    19589     +258     
==========================================
+ Hits         9614     9664      +50     
- Misses       8667     8868     +201     
- Partials     1050     1057       +7

Flag	Coverage Δ
unittests	`49.33% <18.91%> (-0.40%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/control/pubcontrol/pub_control.go	`25.23% <0.00%> (-0.98%)`	⬇️
pkg/control/pubcontrol/pub_control_utils.go	`50.99% <0.00%> (-1.04%)`	⬇️
pkg/controller/cloneset/sync/cloneset_scale.go	`28.25% <ø> (ø)`
pkg/controller/cloneset/utils/cloneset_utils.go	`0.00% <0.00%> (ø)`
...tentpodstate/persistent_pod_state_event_handler.go	`0.00% <0.00%> (ø)`
...availablebudget/podunavailablebudget_controller.go	`38.03% <0.00%> (-1.19%)`	⬇️
...ller/podunavailablebudget/pub_pod_event_handler.go	`26.78% <0.00%> (+0.31%)`	⬆️
...g/webhook/pod/validating/pod_unavailable_budget.go	`68.83% <ø> (+5.73%)`	⬆️
pkg/controller/cloneset/cloneset_controller.go	`44.95% <11.53%> (-7.99%)`	⬇️
...sistentpodstate/persistent_pod_state_controller.go	`31.18% <18.18%> (-1.68%)`	⬇️
... and 9 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

veophi · 2022-11-02T02:39:18Z

How about taking the PVCs related with Competed Pods(Phase=Failed || Phase=Succeeded)?

zmberg · 2022-11-02T03:06:09Z

@willise If the Pod is in the Terminating, it is still risky to delete the PVC directly. The difficulty with this way is that we can't quite tell if it's a normal process or a Hang. I agree with @FillZpp 's idea of adding AlwaysCreatePVC in spec.scaleStrategy to decide whether it should reuse the instanceId of existing free PVCs when creating new Pod.

willise · 2022-11-02T06:48:40Z

How about taking the PVCs related with Competed Pods(Phase=Failed || Phase=Succeeded)?

Thank you for your reply. I am not sure if I understand correctly. Now the Scale function has "active" pods and "active" pvcs as parameters. If the pod is in terminating state, it's a little hard to get it except I have another function to get all pods.

willise · 2022-11-02T06:56:13Z

@willise If the Pod is in the Terminating, it is still risky to delete the PVC directly. The difficulty with this way is that we can't quite tell if it's a normal process or a Hang. I agree with @FillZpp 's idea of adding AlwaysCreatePVC in spec.scaleStrategy to decide whether it should reuse the instanceId of existing free PVCs when creating new Pod.

I got what you mean and I think it is a good advice. But if AlwaysCreatePVC is set, cloneset controller only creates new pod with new PVC. So what to do with existing free PVCs? Like mentioned in the issue, should cloneset controller leave these PVCs unprocessed? @zmberg

zmberg · 2022-11-02T07:07:11Z

Your current cleanup logic works, except that you need to add an additional judgment that only Pods that really don't exist anymore can be cleaned up.

apis/apps/v1alpha1/cloneset_types.go

willise · 2022-11-03T11:01:31Z

Also I find the main logic when scaling in, pod deletes the pvc directly. Maybe I can update the ownerreference of the pod from cloneset to the pod and if the pod is deleted, pvc will be deleted by background cascading deletion later after the pod disappears finally. Just like https://github.com/kubernetes/kubernetes/blob/4086b45af3761d59cb82af6ee427d2d6557c1cbc/pkg/controller/statefulset/stateful_set_utils.go#L231 does.

FillZpp · 2022-11-04T06:59:27Z

Also I find the main logic when scaling in, pod deletes the pvc directly. Maybe I can update the ownerreference of the pod from cloneset to the pod and if the pod is deleted, pvc will be deleted by background cascading deletion later after the pod disappears finally. Just like https://github.com/kubernetes/kubernetes/blob/4086b45af3761d59cb82af6ee427d2d6557c1cbc/pkg/controller/statefulset/stateful_set_utils.go#L231 does.

+1, I think it's a good idea. WDYT @zmberg @veophi

zmberg · 2022-11-07T08:17:02Z

Also I find the main logic when scaling in, pod deletes the pvc directly. Maybe I can update the ownerreference of the pod from cloneset to the pod and if the pod is deleted, pvc will be deleted by background cascading deletion later after the pod disappears finally. Just like https://github.com/kubernetes/kubernetes/blob/4086b45af3761d59cb82af6ee427d2d6557c1cbc/pkg/controller/statefulset/stateful_set_utils.go#L231 does.

Good idea, and please add UT.

pkg/controller/cloneset/utils/cloneset_utils.go

willise · 2022-11-09T12:32:16Z

@zmberg @FillZpp I finish the code and please review :).

zmberg · 2022-12-02T09:44:12Z

pkg/controller/cloneset/sync/cloneset_scale.go

+	// the existing pvc first. Then it looks like the pod
+	// is deleted by controller, new pod can be created.
+	if updateCS.Spec.ScaleStrategy.DisablePVCReuse {
+		uselessPVCs := getUselessPVCs(pods, pvcs)


Should remove uselessPVCs in pvcs? then, pvcs only contains used pvcs.

@zmberg If there are uselessPVCs, cleanupPVCs should always set modified to true then return, so maybe there is no need to update pvcs?

zmberg · 2022-12-02T09:48:37Z

pkg/controller/cloneset/sync/cloneset_scale.go

+	}
+
+	for _, pvc := range uselessPVCs {
+		if clonesetutils.IsTerminating(pvc) {


I don’t think it’s necessary to getInactivePods in the front, just get the pod here, update the owner ref if it exists, delete it if it doesn’t exist.

zmberg · 2022-12-02T10:51:27Z

pkg/controller/cloneset/sync/cloneset_scale.go

+
+func updateClaimOwnerRefToPod(pvc *v1.PersistentVolumeClaim, cs *appsv1alpha1.CloneSet, pod *v1.Pod) bool {
+	needsUpdate := false
+	updateMeta := func(tm *metav1.TypeMeta) {


Please use metav1.NewControllerRef() and you can realize the common function in utils like https://github.com/openkruise/rollouts/blob/master/pkg/util/condition.go.

zmberg · 2022-12-23T06:29:12Z

pkg/controller/cloneset/sync/cloneset_scale.go

+	activeIDs := getInstanceIDsFromPods(pods)
+
+	useless := map[string]*v1.PersistentVolumeClaim{}
+	for _, pvc := range pvcs {


Writing in this way may have some problems with pointers. The suggestions are as follows:

for i := range pvcs { pvc := pvcs[i] id := clonesetutils.GetInstanceID(pvc) if id != "" && !activeIDs.Has(id) { useless[id] = pvc } }

zmberg · 2022-12-23T06:35:10Z

pkg/controller/cloneset/utils/cloneset_utils.go

+// GetAllPods returns all pods in this namespace.
+func GetAllPods(reader client.Reader, opts *client.ListOptions) ([]*v1.Pod, error) {
+	podList := &v1.PodList{}
+	if err := reader.List(context.TODO(), podList, opts); err != nil {


if err := reader.List(context.TODO(), podList, opts, , utilclient.DisableDeepCopy); err != nil {

zmberg · 2022-12-23T06:46:26Z

pkg/util/ownerref.go

+	}
+	ownerRefs := append(
+		target.GetOwnerReferences(),
+		metav1.OwnerReference{


*metav1.NewControllerRef(owner, ownerType.GroupVersionKind()))

zmberg · 2022-12-23T06:56:02Z

pkg/util/pods.go

@@ -343,3 +343,12 @@ func SetPodReadyCondition(pod *v1.Pod) {

 	SetPodCondition(pod, newPodReady)
 }
+
+func UpdatePodMeta(tm *metav1.TypeMeta) {


I think you can remove the function.

zmberg · 2022-12-23T06:58:22Z

pkg/controller/cloneset/sync/cloneset_scale.go

+	needsUpdate = util.RemoveOwnerRef(pvc, cs)
+	podMeta := &pod.TypeMeta
+	util.UpdatePodMeta(podMeta)
+	return util.SetOwnerRef(pvc, pod, podMeta) || needsUpdate


return util.SetOwnerRef(pvc, pod, metav1.TypeMeta{Kind: "Pod", APIVersion: "v1"}) || needsUpdate

zmberg · 2022-12-23T06:58:38Z

pkg/util/ownerref.go

+	return true
+}
+
+func SetOwnerRef(target, owner metav1.Object, ownerType *metav1.TypeMeta) bool {


func SetOwnerRef(target, owner metav1.Object, ownerType metav1.TypeMeta) bool {

zmberg · 2022-12-23T07:05:04Z

pkg/controller/cloneset/sync/cloneset_scale.go

+		instanceID := clonesetutils.GetInstanceID(pod)
+		if pvc, ok := uselessPVCs[instanceID]; ok {
+			if updateClaimOwnerRefToPod(pvc, cs, pod) {
+				if modified, err := r.updatePVC(cs, pvc); err != nil && !errors.IsNotFound(err) {


if modified, err = r.updatePVC(cs, pvc); err != nil && !errors.IsNotFound(err) {

zmberg · 2022-12-23T07:07:27Z

pkg/controller/cloneset/sync/cloneset_scale.go

+	// If useless pvc owner pod does not exist, the pvc can be deleted directly,
+	// else update pvc's ownerreference to pod.
+	for _, pod := range pods {
+		instanceID := clonesetutils.GetInstanceID(pod)


if kubecontroller.IsPodActive(pod) { continue }

zmberg · 2022-12-23T07:26:47Z

pkg/controller/cloneset/sync/cloneset_scale.go

+
+	for _, pvc := range uselessPVCs {
+		// It's safe to delete pvc that has no pod found.
+		if modified, err := r.deletePVC(cs, pvc); err != nil && !errors.IsNotFound(err) {


if modified, err = r.deletePVC(cs, pvc); err != nil && !errors.IsNotFound(err) {

zmberg · 2022-12-23T07:29:31Z

@willise In addition, Can you add e2e for this?

zmberg · 2022-12-23T07:37:17Z

apis/apps/v1alpha1/cloneset_types.go

@@ -93,6 +93,10 @@ type CloneSetScaleStrategy struct {
 	// The scale will fail if the number of unavailable pods were greater than this MaxUnavailable at scaling up.
 	// MaxUnavailable works only when scaling up.
 	MaxUnavailable *intstr.IntOrString `json:"maxUnavailable,omitempty"`
+
+	// Indicate if cloneset will reuse aleady existed pvc to


aleady -> already

zmberg · 2022-12-23T07:44:36Z

pkg/controller/cloneset/utils/cloneset_utils.go

 		return nil, err
 	}

 	// Ignore inactive pods
 	var activePods []*v1.Pod
-	for i, pod := range podList.Items {
+	for i, pod := range podList {


I'm very worried that there will be problems with pointers, so I suggest:

for i := range podList { pod := podList[i] // Consider all rebuild pod as active pod, should not recreate if kubecontroller.IsPodActive(pod) { activePods = append(activePods, pod) } }

zmberg · 2022-12-23T07:47:50Z

pkg/util/ownerref_test.go

@@ -0,0 +1,154 @@
+/*
+Copyright 2020 The Kruise Authors.


zmberg · 2022-12-23T07:48:09Z

pkg/util/ownerref.go

@@ -0,0 +1,66 @@
+/*
+Copyright 2021 The Kruise Authors.


pkg/controller/cloneset/sync/cloneset_scale.go

Signed-off-by: willise <[email protected]>

furykerry · 2023-01-03T04:10:47Z

/lgtm

furykerry · 2023-01-03T04:11:11Z

/approve

kruise-bot · 2023-01-03T04:11:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: furykerry

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:

~~OWNERS~~ [furykerry]

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

kruise-bot requested review from furykerry and hellolijj November 1, 2022 11:55

kruise-bot added the size/M size/M: 30-99 label Nov 1, 2022

FillZpp reviewed Nov 2, 2022

View reviewed changes

apis/apps/v1alpha1/cloneset_types.go Outdated Show resolved Hide resolved

willise force-pushed the fix/enable-clean-up-pvc branch from a447a1a to 0b57d3f Compare November 2, 2022 13:53

kruise-bot added size/L size/L: 100-499 and removed size/M size/M: 30-99 labels Nov 2, 2022

willise force-pushed the fix/enable-clean-up-pvc branch from 0b57d3f to 516cda9 Compare November 7, 2022 12:18

sonatype-lift bot reviewed Nov 7, 2022

View reviewed changes

pkg/controller/cloneset/utils/cloneset_utils.go Outdated Show resolved Hide resolved

willise force-pushed the fix/enable-clean-up-pvc branch 2 times, most recently from 4965c17 to 897bde2 Compare November 9, 2022 09:27

willise force-pushed the fix/enable-clean-up-pvc branch 2 times, most recently from d3f36cc to 854616b Compare November 24, 2022 07:53

zmberg reviewed Dec 2, 2022

View reviewed changes

willise force-pushed the fix/enable-clean-up-pvc branch 2 times, most recently from 852a53c to 74de5e1 Compare December 22, 2022 08:48

kruise-bot added size/XL size/XL: 500-999 and removed size/L size/L: 100-499 labels Dec 22, 2022

willise force-pushed the fix/enable-clean-up-pvc branch from 74de5e1 to df1f121 Compare December 22, 2022 09:15

willise requested review from zmberg and removed request for furykerry and hellolijj December 23, 2022 02:47

zmberg reviewed Dec 23, 2022

View reviewed changes

furykerry reviewed Dec 26, 2022

View reviewed changes

pkg/controller/cloneset/sync/cloneset_scale.go Outdated Show resolved Hide resolved

pkg/controller/cloneset/sync/cloneset_scale.go Outdated Show resolved Hide resolved

pkg/controller/cloneset/sync/cloneset_scale.go Outdated Show resolved Hide resolved

willise force-pushed the fix/enable-clean-up-pvc branch from df1f121 to 6871ed3 Compare December 26, 2022 13:22

enable cloneset deleting pvc when pod hanging

ec8d173

Signed-off-by: willise <[email protected]>

willise force-pushed the fix/enable-clean-up-pvc branch from 6871ed3 to ec8d173 Compare December 29, 2022 13:03

kruise-bot added size/L size/L: 100-499 and removed size/XL size/XL: 500-999 labels Dec 29, 2022

kruise-bot assigned furykerry Jan 3, 2023

kruise-bot added the lgtm label Jan 3, 2023

kruise-bot added the approved label Jan 3, 2023

kruise-bot merged commit 7b23f68 into openkruise:master Jan 3, 2023

willise changed the title ~~enable mcloneset deleting pvc when pod hanging~~ enable cloneset deleting pvc when pod hanging Jan 4, 2023

ppbits pushed a commit to ppbits/kruise that referenced this pull request Apr 4, 2024

enable cloneset deleting pvc when pod hanging (openkruise#1113)

151206c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enable cloneset deleting pvc when pod hanging #1113

enable cloneset deleting pvc when pod hanging #1113

willise commented Nov 1, 2022

kruise-bot commented Nov 1, 2022

codecov-commenter commented Nov 1, 2022 •

edited

Loading

veophi commented Nov 2, 2022

zmberg commented Nov 2, 2022

willise commented Nov 2, 2022

willise commented Nov 2, 2022

zmberg commented Nov 2, 2022

willise commented Nov 3, 2022

FillZpp commented Nov 4, 2022

zmberg commented Nov 7, 2022

willise commented Nov 9, 2022

zmberg Dec 2, 2022

willise Dec 6, 2022 •

edited

Loading

zmberg Dec 2, 2022

zmberg Dec 2, 2022

zmberg Dec 23, 2022

zmberg Dec 23, 2022

zmberg Dec 23, 2022

zmberg Dec 23, 2022

zmberg Dec 23, 2022

zmberg Dec 23, 2022

zmberg Dec 23, 2022

zmberg Dec 23, 2022

zmberg Dec 23, 2022

zmberg commented Dec 23, 2022

zmberg Dec 23, 2022

zmberg Dec 23, 2022

zmberg Dec 23, 2022

zmberg Dec 23, 2022

furykerry commented Jan 3, 2023

furykerry commented Jan 3, 2023

kruise-bot commented Jan 3, 2023

enable cloneset deleting pvc when pod hanging #1113

enable cloneset deleting pvc when pod hanging #1113

Conversation

willise commented Nov 1, 2022

Ⅰ. Describe what this PR does

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how to verify it

Ⅳ. Special notes for reviews

kruise-bot commented Nov 1, 2022

codecov-commenter commented Nov 1, 2022 • edited Loading

Codecov Report

veophi commented Nov 2, 2022

zmberg commented Nov 2, 2022

willise commented Nov 2, 2022

willise commented Nov 2, 2022

zmberg commented Nov 2, 2022

willise commented Nov 3, 2022

FillZpp commented Nov 4, 2022

zmberg commented Nov 7, 2022

willise commented Nov 9, 2022

Choose a reason for hiding this comment

willise Dec 6, 2022 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zmberg commented Dec 23, 2022

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

furykerry commented Jan 3, 2023

furykerry commented Jan 3, 2023

kruise-bot commented Jan 3, 2023

codecov-commenter commented Nov 1, 2022 •

edited

Loading

willise Dec 6, 2022 •

edited

Loading