kruise icon indicating copy to clipboard operation
kruise copied to clipboard

Sidecar terminator ignore the exit code of the sidecar container

Open diannaowa opened this issue 1 year ago • 5 comments

Ⅰ. Describe what this PR does

Pods are not allowed to transition out of terminal phases when kubelet report the pod status.(ref) Base on this context, the proposal as bellow: In the sidecar terminator controller: 1,Calculate the Pod phase from the exit-code of the main container: the pod phase will be Succeeded, if all main containers are succeeded . the pod phase will be failed ,if at least one main container is failed(the exitCode is not zero) . 2, Terminate it if the job pod is complete.

3, Terminate the pod and set the condition of the pod as the bellow:

- lastProbeTime: null
    lastTransitionTime: "2023-06-06T14:06:08Z"
    status: "True"
    type: SidecarTerminated

4, If the job pod is completed, we will skip terminating the pod. Check whether a job is complete by:

func isJobPodCompleted(pod *corev1.Pod) bool {
	mainContainers := getMain(pod)
	if !containersCompleted(pod, mainContainers) {
		return false
	}
	if containersSucceeded(pod, mainContainers) {
		return pod.Status.Phase == corev1.PodSucceeded
	} else {
		return pod.Status.Phase == corev1.PodFailed
	}
}

Native controller actions:

1 .Job controller update the status of the job when pod phase is updated. 2. As we said above, kubelet only update the container status and do not update the pod phase since it is terminal phase. And the pod will reach a final state. 3. Kubelet will kill the containers that are not in terminal phase, after the pod the terminated by sidecar terminator.

Ⅱ. Does this pull request fix one issue?

fixes #1285

Ⅲ. Describe how to verify it

Ⅳ. Special notes for reviews

diannaowa avatar Jun 02 '23 00:06 diannaowa

Codecov Report

Attention: Patch coverage is 79.06977% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 47.89%. Comparing base (209d476) to head (a66d98f).

Files Patch % Lines
...sidecarterminator/sidecar_terminator_controller.go 77.50% 6 Missing and 3 partials :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1303      +/-   ##
==========================================
+ Coverage   47.82%   47.89%   +0.06%     
==========================================
  Files         161      161              
  Lines       23395    23428      +33     
==========================================
+ Hits        11188    11220      +32     
- Misses      10993    10995       +2     
+ Partials     1214     1213       -1     
Flag Coverage Δ
unittests 47.89% <79.06%> (+0.06%) :arrow_up:

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Jun 02 '23 00:06 codecov-commenter

@diannaowa Can we set Pod.Status.Phase to Succeeded/Failed BEFORE killing sidecars?

veophi avatar Oct 30 '23 12:10 veophi

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Jan 29 '24 04:01 stale[bot]

will do

diannaowa avatar Jan 29 '24 05:01 diannaowa

/lgtm

zmberg avatar Mar 12 '24 12:03 zmberg

/approve

furykerry avatar Mar 13 '24 05:03 furykerry

[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:

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

kruise-bot avatar Mar 13 '24 05:03 kruise-bot