Add mark pod not ready feature to Lifecycle hooks #979

veophi · 2022-05-18T08:16:47Z

Signed-off-by: mingzhou.swx mingzhou.swx@alibaba-inc.com

Ⅰ. Describe what this PR does

Add mark pod not ready feature to Lifecycle hooks;
CloneSet status.availableReplicas consider lifecycle state, only the pods at normal state are available.

Ⅱ. Does this pull request fix one issue?

#944

pkg/util/lifecycle/lifecycle_utils.go

pkg/util/podreadiness/pod_readiness_utils.go

pkg/controller/containerrecreaterequest/crr_controller.go

pkg/util/lifecycle/lifecycle_utils.go

pkg/util/inplaceupdate/inplace_update.go

sonatype-lift · 2022-05-18T15:56:10Z

pkg/controller/cloneset/sync/cloneset_scale.go

+	// Actually, there is a bug cased by this transformation from PreparingDelete to Normal,
+	// i.e., Lifecycle Updated Hook may be lost if the pod was transformed from Updating state
+	// to PreparingDelete.
+	if cs.Spec.Lifecycle != nil || lifecycle.IsHookRequiredPodNotReady(cs.Spec.Lifecycle.PreDelete) {


opt.semgrep.bad-nil-guard: Useless nil guard

(at-me in a reply with help or ignore)

Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

codecov-commenter · 2022-05-19T02:29:56Z

Codecov Report

Merging #979 (8fa6413) into master (cf95157) will increase coverage by 0.01%.
The diff coverage is 58.82%.

@@            Coverage Diff             @@
##           master     #979      +/-   ##
==========================================
+ Coverage   49.93%   49.95%   +0.01%     
==========================================
  Files         120      121       +1     
  Lines       11558    11579      +21     
==========================================
+ Hits         5772     5784      +12     
- Misses       4914     4924      +10     
+ Partials      872      871       -1

Flag	Coverage Δ
unittests	`49.95% <58.82%> (+0.01%)`	⬆️

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

Impacted Files	Coverage Δ
pkg/controller/cloneset/sync/cloneset_scale.go	`25.74% <0.00%> (-0.39%)`	⬇️
pkg/controller/daemonset/daemonset_controller.go	`41.80% <0.00%> (-0.09%)`	⬇️
pkg/controller/cloneset/sync/cloneset_update.go	`47.29% <25.00%> (-0.96%)`	⬇️
pkg/controller/statefulset/stateful_set_control.go	`61.89% <50.00%> (+0.18%)`	⬆️
...kg/controller/cloneset/sync/cloneset_sync_utils.go	`87.03% <66.66%> (ø)`
pkg/util/podreadiness/pod_readiness_utils.go	`77.38% <76.00%> (+0.27%)`	⬆️
pkg/util/podreadiness/pod_readiness.go	`84.61% <84.61%> (ø)`
pkg/controller/cloneset/cloneset_status.go	`90.47% <100.00%> (ø)`
...roller/uniteddeployment/uniteddeployment_update.go	`72.28% <0.00%> (-1.87%)`	⬇️
pkg/controller/uniteddeployment/revision.go	`67.93% <0.00%> (+1.52%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cf95157...8fa6413. Read the comment docs.

sonatype-lift · 2022-05-19T02:46:54Z

pkg/controller/containerrecreaterequest/crr_controller.go

 	return &ReconcileContainerRecreateRequest{
-		Client: util.NewClientFromManager(mgr, "containerrecreaterequest-controller"),
+		Client: cli,


gofmt: File is not gofmt-ed with -s

(at-me in a reply with help or ignore)

Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

sonatype-lift · 2022-05-19T02:46:57Z

pkg/util/lifecycle/lifecycle_utils.go

 // Interface for managing pods lifecycle.
 type Interface interface {
-	UpdatePodLifecycle(pod *v1.Pod, state appspub.LifecycleStateType) (bool, *v1.Pod, error)
+	UpdatePodLifecycle(pod *v1.Pod, state appspub.LifecycleStateType, requiredPodNotReady bool) (bool, *v1.Pod, error)
 	UpdatePodLifecycleWithHandler(pod *v1.Pod, state appspub.LifecycleStateType, inPlaceUpdateHandler *appspub.LifecycleHook) (bool, *v1.Pod, error)
 }

 type realControl struct {
 	adp podadapter.Adapter


gofmt: File is not gofmt-ed with -s

(at-me in a reply with help or ignore)

Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

sonatype-lift · 2022-05-19T02:46:58Z

pkg/controller/cloneset/sync/cloneset_scale.go

+	// Actually, there is a bug cased by this transformation from PreparingDelete to Normal,
+	// i.e., Lifecycle Updated Hook may be lost if the pod was transformed from Updating state
+	// to PreparingDelete.
+	if cs.Spec.Lifecycle != nil && (


gofmt: File is not gofmt-ed with -s

(at-me in a reply with help or ignore)

Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

furykerry · 2022-05-19T07:52:44Z

pkg/controller/daemonset/daemonset_controller.go

@@ -755,7 +755,8 @@ func (dsc *ReconcileDaemonSet) syncWithPreparingDelete(ds *appsv1alpha1.DaemonSe
 			podsCanDelete = append(podsCanDelete, podName)
 			continue
 		}
-		if updated, gotPod, err := dsc.lifecycleControl.UpdatePodLifecycle(pod, appspub.LifecycleStatePreparingDelete); err != nil {
+		requiredPodNotReady := ds.Spec.Lifecycle.PreDelete.MarkPodNotReady


can we rename all requiredPodNotReady variables/comments to markPodNotReady

apis/apps/pub/lifecycle.go

furykerry · 2022-05-19T10:14:36Z

/lgtm

Signed-off-by: mingzhou.swx <mingzhou.swx@alibaba-inc.com>

FillZpp

/lgtm

kruise-bot · 2022-05-20T05:58:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: FillZpp

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~~ [FillZpp]

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

Signed-off-by: mingzhou.swx <mingzhou.swx@alibaba-inc.com> Co-authored-by: mingzhou.swx <mingzhou.swx@alibaba-inc.com> Signed-off-by: Liu Zhenwei <zwliu@thoughtworks.com>

Co-authored-by: mingzhou.swx <mingzhou.swx@alibaba-inc.com>

kruise-bot requested review from FillZpp and shiyan2016 May 18, 2022 08:16

kruise-bot added the size/L size/L: 100-499 label May 18, 2022

FillZpp reviewed May 18, 2022

View reviewed changes

veophi force-pushed the lifecycle-notready branch from 906d86d to e06c854 Compare May 18, 2022 15:40

sonatype-lift bot reviewed May 18, 2022

View reviewed changes

veophi force-pushed the lifecycle-notready branch from e06c854 to 39f9303 Compare May 19, 2022 02:19

veophi force-pushed the lifecycle-notready branch from 39f9303 to 7a778df Compare May 19, 2022 02:31

sonatype-lift bot reviewed May 19, 2022

View reviewed changes

veophi force-pushed the lifecycle-notready branch from 7a778df to 7f9b557 Compare May 19, 2022 05:46

furykerry reviewed May 19, 2022

View reviewed changes

apis/apps/pub/lifecycle.go Show resolved Hide resolved

veophi force-pushed the lifecycle-notready branch 2 times, most recently from c57492b to 3b67a4f Compare May 19, 2022 09:38

kruise-bot assigned furykerry May 19, 2022

kruise-bot added the lgtm label May 19, 2022

veophi force-pushed the lifecycle-notready branch from 3b67a4f to f959515 Compare May 20, 2022 03:52

kruise-bot removed the lgtm label May 20, 2022

add lifecycle mark pod not ready feature

8fa6413

Signed-off-by: mingzhou.swx <mingzhou.swx@alibaba-inc.com>

veophi force-pushed the lifecycle-notready branch from f959515 to 8fa6413 Compare May 20, 2022 04:00

FillZpp approved these changes May 20, 2022

View reviewed changes

kruise-bot assigned FillZpp May 20, 2022

kruise-bot added the lgtm label May 20, 2022

kruise-bot added the approved label May 20, 2022

kruise-bot merged commit 2fd19a4 into openkruise:master May 20, 2022

veophi mentioned this pull request May 20, 2022

Mark pod not ready quickly when add not ready key #980

Merged

veophi mentioned this pull request Jun 1, 2022

update cloneset statefulset daemonset docs for kruise 1.2.0 openkruise/openkruise.io#60

Merged

FillZpp mentioned this pull request Jun 7, 2022

[feature request] set pod not ready in PreparingUpdate and PreparingDelete lifecycle hook #944

Closed

FillZpp mentioned this pull request Jul 12, 2022

[BUG] preDelete of Advanced StatefulSet not working #950

Closed

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

add lifecycle mark pod not ready feature (openkruise#979)

6d522f3

Co-authored-by: mingzhou.swx <mingzhou.swx@alibaba-inc.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add mark pod not ready feature to Lifecycle hooks #979

Add mark pod not ready feature to Lifecycle hooks #979

veophi commented May 18, 2022

sonatype-lift bot May 18, 2022

codecov-commenter commented May 19, 2022 •

edited

Loading

sonatype-lift bot May 19, 2022

sonatype-lift bot May 19, 2022

sonatype-lift bot May 19, 2022

furykerry May 19, 2022

veophi May 19, 2022

furykerry commented May 19, 2022

FillZpp left a comment

kruise-bot commented May 20, 2022

Add mark pod not ready feature to Lifecycle hooks #979

Add mark pod not ready feature to Lifecycle hooks #979

Conversation

veophi commented May 18, 2022

Ⅰ. Describe what this PR does

Ⅱ. Does this pull request fix one issue?

sonatype-lift bot May 18, 2022

Choose a reason for hiding this comment

codecov-commenter commented May 19, 2022 • edited Loading

Codecov Report

sonatype-lift bot May 19, 2022

Choose a reason for hiding this comment

sonatype-lift bot May 19, 2022

Choose a reason for hiding this comment

sonatype-lift bot May 19, 2022

Choose a reason for hiding this comment

furykerry May 19, 2022

Choose a reason for hiding this comment

veophi May 19, 2022

Choose a reason for hiding this comment

furykerry commented May 19, 2022

FillZpp left a comment

Choose a reason for hiding this comment

kruise-bot commented May 20, 2022

codecov-commenter commented May 19, 2022 •

edited

Loading