support fix containerrecreaterequest #1182

BH4AWS · 2023-02-20T01:53:40Z

Ⅰ. Describe what this PR does

add the forceRecreate to container recreate request feature. When a crr resource is created with forceRecreate = true, the container will be kill once by forcibly and immediately.

Ⅱ. Does this pull request fix one issue?

yes, issue: #1177

Ⅲ. Describe how to verify it

using the container recreate request and the forceRecreate is set true.

Ⅳ. Special notes for reviews

codecov-commenter · 2023-02-20T02:04:12Z

Codecov Report

Merging #1182 (5e15dc1) into master (53fb7e1) will increase coverage by 0.32%.
The diff coverage is n/a.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##           master    #1182      +/-   ##
==========================================
+ Coverage   49.73%   50.05%   +0.32%     
==========================================
  Files         137      143       +6     
  Lines       19331    19906     +575     
==========================================
+ Hits         9614     9964     +350     
- Misses       8667     8848     +181     
- Partials     1050     1094      +44

Flag	Coverage Δ
unittests	`50.05% <ø> (+0.32%)`	⬆️

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

Impacted Files	Coverage Δ
pkg/controller/cloneset/cloneset_event_handler.go	`59.34% <0.00%> (-18.73%)`	⬇️
pkg/webhook/sidecarset/validating/utils.go	`56.66% <0.00%> (-18.34%)`	⬇️
pkg/controller/cloneset/sync/cloneset_update.go	`57.72% <0.00%> (-2.21%)`	⬇️
pkg/webhook/pod/mutating/persistent_pod_state.go	`64.21% <0.00%> (-1.73%)`	⬇️
...sistentpodstate/persistent_pod_state_controller.go	`31.18% <0.00%> (-1.68%)`	⬇️
pkg/control/pubcontrol/pub_control.go	`25.00% <0.00%> (-1.22%)`	⬇️
pkg/control/pubcontrol/pub_control_utils.go	`50.99% <0.00%> (-1.04%)`	⬇️
pkg/controller/statefulset/stateful_set_control.go	`63.32% <0.00%> (-0.59%)`	⬇️
pkg/control/sidecarcontrol/util.go	`53.48% <0.00%> (-0.39%)`	⬇️
pkg/controller/sidecarset/sidecarset_processor.go	`68.61% <0.00%> (-0.13%)`	⬇️
... and 30 more

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

pkg/daemon/containerrecreate/crr_daemon_util.go

furykerry · 2023-02-20T08:09:38Z

pkg/daemon/containerrecreate/crr_daemon_controller.go

@@ -362,10 +363,25 @@ func (c *Controller) manage(crr *appsv1alpha1.ContainerRecreateRequest) error {
 			}
 			return c.patchCRRContainerRecreateStates(crr, newCRRContainerRecreateStates)
 		}
+
+		if crr.Spec.Strategy.ForceRecreate {
+			newForceKilledContainerStatuses = append(newForceKilledContainerStatuses, appsv1alpha1.ContainerRecreateRequestForceKilledContainerStatus{


we don't have to patch "forcekill" status if the container do not have to be recreated forcefully

if crr.Spec.Strategy.ForceRecreate && state.ShouldBeKilledByForce {
state.IsForceKilled = true
state.ContainerID = kubeContainerStatus.ID.String()
}

furykerry · 2023-02-20T08:39:01Z

config/rbac/daemon_role.yaml

@@ -38,6 +38,7 @@ rules:
  - get
  - list
  - watch
+  - patch


it is recommended to avoid assign object patch privilege to daemons, so the container id of killed container can be saved in crr status

ai-driven · 2023-03-02T03:29:46Z

apis/apps/v1alpha1/containerrecreaterequest_types.go

+	// Name of the container.
+	Name string `json:"name"`
+	// Container is killed by force
+	IsForceKilled bool `json:"restartCount"`


struct tag for IsForceKilled should be isForceKilled or something similar. restartCount is considered as something different.

zmberg · 2023-03-08T03:33:28Z

apis/apps/v1alpha1/containerrecreaterequest_types.go

@@ -37,6 +37,9 @@ const (
 	// ContainerRecreateRequestUnreadyAcquiredKey indicates the Pod has been forced to not-ready.
 	// It is required if the unreadyGracePeriodSeconds is set in ContainerRecreateRequests.
 	ContainerRecreateRequestUnreadyAcquiredKey = "crr.apps.kruise.io/unready-acquired"
+
+	// It is a status key for containers which are be killed by crr
+	ContainerRecreateRequestForceKilledContainerStatusesKey = "crr.apps.kruise.io/force-killed-container-statuses"


Remove the unused key?

furykerry · 2023-03-08T05:36:57Z

apis/apps/v1alpha1/containerrecreaterequest_types.go

@@ -106,6 +109,8 @@ type ContainerRecreateRequestStrategy struct {
 	FailurePolicy ContainerRecreateRequestFailurePolicyType `json:"failurePolicy,omitempty"`
 	// OrderedRecreate indicates whether to recreate the next container only if the previous one has recreated completely.
 	OrderedRecreate bool `json:"orderedRecreate,omitempty"`
+	// ForceRecreate indicates whether to force kill the container even if the previous container is not running.


if the previous container is not running -> if the previous container is starting

furykerry · 2023-03-08T05:44:48Z

apis/apps/v1alpha1/containerrecreaterequest_types.go

@@ -158,6 +163,12 @@ type ContainerRecreateRequestContainerRecreateState struct {
 	Phase ContainerRecreateRequestPhase `json:"phase"`
 	// A human readable message indicating details about this state.
 	Message string `json:"message,omitempty"`
+	// Container should be killed by force
+	ShouldBeKilledByForce bool `json:"shouldBeKilledByForce"`


why not use crr.Spec.Strategy.ForceRecreate instead? it seems this status field unnecessary

ShouldBeKilledByForce has been removed in this solution

furykerry · 2023-03-08T05:45:41Z

apis/apps/v1alpha1/containerrecreaterequest_types.go

@@ -172,6 +183,16 @@ type ContainerRecreateRequestSyncContainerStatus struct {
 	ContainerID string `json:"containerID,omitempty"`
 }

+// ContainerRecreateRequestKilledContainerStatus contains the state of the last killed container.
+type ContainerRecreateRequestForceKilledContainerStatus struct {


remove unused code

furykerry · 2023-03-08T06:01:20Z

pkg/daemon/containerrecreate/crr_daemon_util.go

@@ -69,6 +69,8 @@ func getCurrentCRRContainersRecreateStates(
 	}

 	syncContainerStatuses := getCRRSyncContainerStatuses(crr)
+	// the statuses of force killed containers for this crr resource
+	forceKillContainerStatuses := getCRRForceKillContainersStatuses(crr)


getCRRForceKillContainersStatuses -> getKilledContainersStatuses

furykerry · 2023-03-08T06:26:00Z

pkg/daemon/containerrecreate/crr_daemon_util.go

@@ -128,6 +136,16 @@ func getCurrentCRRContainersRecreateStates(
 	return statuses
 }

+func containerIsNotForceKilled(recreateState *appsv1alpha1.ContainerRecreateRequestContainerRecreateState) bool {


containerIsNotForceKilled -> containerIsForceKilled
plz revert the func logic to make code more easy to understand

furykerry · 2023-03-08T06:32:39Z

pkg/daemon/containerrecreate/crr_daemon_util.go

-
+			if crr.Spec.Strategy.ForceRecreate && (len(forceKillContainerStatuses) == 0 ||
+				containerIsNotForceKilled(killedContainerStatus)) {
+				currentState.ShouldBeKilledByForce = true


ShouldBeKilledByForce -> IsForceKilled

new logic is changed in this code.

furykerry · 2023-03-08T06:42:29Z

pkg/daemon/containerrecreate/crr_daemon_controller.go

@@ -362,6 +362,11 @@ func (c *Controller) manage(crr *appsv1alpha1.ContainerRecreateRequest) error {
 			}
 			return c.patchCRRContainerRecreateStates(crr, newCRRContainerRecreateStates)
 		}
+
+		if crr.Spec.Strategy.ForceRecreate && state.ShouldBeKilledByForce {


we should populate ContainerID even if the container is not killed forcely

furykerry · 2023-03-08T06:43:06Z

pkg/daemon/containerrecreate/crr_daemon_util.go

@@ -114,7 +118,11 @@ func getCurrentCRRContainersRecreateStates(
 				syncContainerStatus.Ready {
 				currentState.Phase = appsv1alpha1.ContainerRecreateRequestSucceeded
 			}
-
+			if crr.Spec.Strategy.ForceRecreate && (len(forceKillContainerStatuses) == 0 ||


len(forceKillContainerStatuses) checking is not necessary, containerIsNotForceKilled will check whether recreateState == nil , plz remove len(forceKillContainerStatuses) checking

zmberg · 2023-03-13T08:08:39Z

pkg/daemon/containerrecreate/crr_daemon_util.go

+	if recreateState.IsForceKilled {
+		return true
+	}
+	return false


return recreateState != nil && recreateState.IsForceKilled

zmberg · 2023-03-13T08:14:50Z

apis/apps/v1alpha1/containerrecreaterequest_types.go

@@ -158,6 +160,8 @@ type ContainerRecreateRequestContainerRecreateState struct {
 	Phase ContainerRecreateRequestPhase `json:"phase"`
 	// A human readable message indicating details about this state.
 	Message string `json:"message,omitempty"`
+	// Container has been killed by force
+	IsForceKilled bool `json:"isForceKilled,omitempty"`


IsRealKilled bool ?

zmberg · 2023-03-13T08:37:59Z

pkg/daemon/containerrecreate/crr_daemon_util.go

@@ -93,13 +97,21 @@ func getCurrentCRRContainersRecreateStates(
 				Message: "not found container on Node",
 			}

+		} else if crr.Spec.Strategy.ForceRecreate && !containerIsForceKilled(killedContainerStatus) {


if crr.Spec.Strategy.ForceRecreate { if previousContainerRecreateState == nil || !previousContainerRecreateState.IsRealKilled { currentState = appsv1alpha1.ContainerRecreateRequestContainerRecreateState{ Name: c.Name, Phase: appsv1alpha1.ContainerRecreateRequestPending, Message: "No real kill container yet", } statuses = append(statuses, currentState) continue } }

apis/apps/v1alpha1/containerrecreaterequest_types.go

furykerry · 2023-03-15T06:16:42Z

pkg/daemon/containerrecreate/crr_daemon_util.go

+			}
+		}
+
+		// current: previousContainerRecreateState.IsRealKilled = true


IsRealKilled -> IsKilled

furykerry · 2023-03-15T06:23:57Z

pkg/daemon/containerrecreate/crr_daemon_util.go

@@ -85,40 +85,56 @@ func getCurrentCRRContainersRecreateStates(
 		kubeContainerStatus := podStatus.FindContainerStatusByName(c.Name)

 		var currentState appsv1alpha1.ContainerRecreateRequestContainerRecreateState
+
+		if crr.Spec.Strategy.ForceRecreate {


shall we skip crr with completed phase in L77.

Signed-off-by: jicheng.sk <jicheng.sk@alibaba-inc.com>

zmberg · 2023-03-16T09:53:18Z

/lgtm

furykerry

/approve

kruise-bot · 2023-03-16T12:33:40Z

[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

Signed-off-by: jicheng.sk <jicheng.sk@alibaba-inc.com> Co-authored-by: jicheng.sk <jicheng.sk@alibaba-inc.com>

Co-authored-by: jicheng.sk <jicheng.sk@alibaba-inc.com>

kruise-bot requested review from FillZpp and zmberg February 20, 2023 01:53

kruise-bot added the size/M size/M: 30-99 label Feb 20, 2023

zmberg added this to the v1.4 milestone Feb 20, 2023

furykerry reviewed Feb 20, 2023

View reviewed changes

ai-driven reviewed Mar 2, 2023

View reviewed changes

BH4AWS force-pushed the containerrecreate_add_forcekill branch from 303851d to 5e15dc1 Compare March 7, 2023 17:35

zmberg reviewed Mar 8, 2023

View reviewed changes

furykerry reviewed Mar 8, 2023

View reviewed changes

BH4AWS force-pushed the containerrecreate_add_forcekill branch 2 times, most recently from d367244 to 88baf65 Compare March 10, 2023 09:40

zmberg reviewed Mar 13, 2023

View reviewed changes

BH4AWS force-pushed the containerrecreate_add_forcekill branch from 88baf65 to ab2d8e9 Compare March 13, 2023 09:06

kruise-bot added size/S size/S 10-29 and removed size/M size/M: 30-99 labels Mar 13, 2023

BH4AWS force-pushed the containerrecreate_add_forcekill branch from ab2d8e9 to 7e43eb2 Compare March 13, 2023 17:05

kruise-bot added size/M size/M: 30-99 and removed size/S size/S 10-29 labels Mar 13, 2023

BH4AWS force-pushed the containerrecreate_add_forcekill branch from 7e43eb2 to 7a252af Compare March 14, 2023 16:41

furykerry reviewed Mar 15, 2023

View reviewed changes

BH4AWS force-pushed the containerrecreate_add_forcekill branch from 7a252af to d522f61 Compare March 15, 2023 17:38

kruise-bot added size/L size/L: 100-499 and removed size/M size/M: 30-99 labels Mar 15, 2023

BH4AWS force-pushed the containerrecreate_add_forcekill branch from d522f61 to 6f60438 Compare March 15, 2023 17:49

support fix containerrecreaterequest

f275bd3

Signed-off-by: jicheng.sk <jicheng.sk@alibaba-inc.com>

BH4AWS force-pushed the containerrecreate_add_forcekill branch from 6f60438 to f275bd3 Compare March 16, 2023 09:49

kruise-bot assigned zmberg Mar 16, 2023

kruise-bot added the lgtm label Mar 16, 2023

furykerry reviewed Mar 16, 2023

View reviewed changes

kruise-bot added the approved label Mar 16, 2023

kruise-bot merged commit eefcbb2 into openkruise:master Mar 16, 2023

diannaowa pushed a commit to diannaowa/kruise that referenced this pull request Jun 2, 2023

support fix containerrecreaterequest (openkruise#1182)

aded857

Signed-off-by: jicheng.sk <jicheng.sk@alibaba-inc.com> Co-authored-by: jicheng.sk <jicheng.sk@alibaba-inc.com>

diannaowa pushed a commit to diannaowa/kruise that referenced this pull request Jun 2, 2023

support fix containerrecreaterequest (openkruise#1182)

fd6560b

Signed-off-by: jicheng.sk <jicheng.sk@alibaba-inc.com> Co-authored-by: jicheng.sk <jicheng.sk@alibaba-inc.com>

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

support fix containerrecreaterequest (openkruise#1182)

92eafa9

Co-authored-by: jicheng.sk <jicheng.sk@alibaba-inc.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support fix containerrecreaterequest #1182

support fix containerrecreaterequest #1182

BH4AWS commented Feb 20, 2023

codecov-commenter commented Feb 20, 2023 •

edited

Loading

furykerry Feb 20, 2023

BH4AWS Mar 8, 2023

furykerry Feb 20, 2023

BH4AWS Mar 8, 2023

ai-driven Mar 2, 2023

BH4AWS Mar 8, 2023

zmberg Mar 8, 2023

BH4AWS Mar 8, 2023

furykerry Mar 8, 2023

BH4AWS Mar 8, 2023

furykerry Mar 8, 2023

BH4AWS Mar 10, 2023

furykerry Mar 8, 2023

BH4AWS Mar 8, 2023

furykerry Mar 8, 2023

BH4AWS Mar 8, 2023

furykerry Mar 8, 2023

furykerry Mar 8, 2023

BH4AWS Mar 10, 2023

furykerry Mar 8, 2023

furykerry Mar 8, 2023

zmberg Mar 13, 2023

zmberg Mar 13, 2023

zmberg Mar 13, 2023

furykerry Mar 15, 2023

furykerry Mar 15, 2023

zmberg commented Mar 16, 2023

furykerry left a comment

kruise-bot commented Mar 16, 2023

@@ @@ -38,6 +38,7 @@ rules: @@
                 - get
                 - list
                 - watch
+                - patch

support fix containerrecreaterequest #1182

support fix containerrecreaterequest #1182

Conversation

BH4AWS commented Feb 20, 2023

Ⅰ. Describe what this PR does

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how to verify it

Ⅳ. Special notes for reviews

codecov-commenter commented Feb 20, 2023 • edited Loading

Codecov Report

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zmberg commented Mar 16, 2023

furykerry left a comment

Choose a reason for hiding this comment

kruise-bot commented Mar 16, 2023

codecov-commenter commented Feb 20, 2023 •

edited

Loading