-
Notifications
You must be signed in to change notification settings - Fork 767
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Sidecar terminator ignore the exit code of the sidecar container #1303
Conversation
Codecov ReportAttention: Patch coverage is
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
64b8f72
to
da6496f
Compare
pkg/controller/sidecarterminator/sidecar_terminator_controller_test.go
Outdated
Show resolved
Hide resolved
ad293ff
to
62b908d
Compare
pkg/controller/sidecarterminator/sidecar_terminator_controller.go
Outdated
Show resolved
Hide resolved
a1e7bd6
to
5da1e4f
Compare
@diannaowa Can we set |
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. |
will do |
048e9da
to
01f9580
Compare
// after the pod is terminated by the sidecar terminator, kubelet will kill the containers that are not in the terminal phase | ||
// 1. sidecar container terminate with non-zero exit code | ||
// 2. sidecar container is not in a terminal phase (still running or waiting) | ||
if !containersSucceeded(pod, sidecars) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
plz reverse the checking to reduce code indentation, e..g
if containersSucceeded(pod, sidecars) {
return nil
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
pkg/webhook/util/util.go
Outdated
) | ||
|
||
func GetHost() string { | ||
return os.Getenv("WEBHOOK_HOST") | ||
return os.Getenv("$1ST") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why change the env ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, my fault. Test code.
Done.
14ae299
to
a1465b5
Compare
/lgtm |
pkg/controller/sidecarterminator/sidecar_terminator_controller.go
Outdated
Show resolved
Hide resolved
pkg/controller/sidecarterminator/sidecar_terminator_controller.go
Outdated
Show resolved
Hide resolved
Signed-off-by: liuzhenwei <[email protected]> add ut Signed-off-by: liuzhenwei <[email protected]> add some comments and simplified some code Signed-off-by: liuzhenwei <[email protected]> remove unnecessary pod status operations Signed-off-by: liuzhenwei <[email protected]> change pod to terminal phase before create crr Signed-off-by: liuzhenwei <[email protected]> reverse the checking to reduce code indentation Signed-off-by: liuzhenwei <[email protected]> simplified some logic Signed-off-by: liuzhenwei <[email protected]> remove unesd code and rename function avoid misleading Signed-off-by: liuzhenwei <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve |
[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 |
…nkruise#1303) add ut add some comments and simplified some code remove unnecessary pod status operations change pod to terminal phase before create crr reverse the checking to reduce code indentation simplified some logic remove unesd code and rename function avoid misleading
Ⅰ. 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:
4, If the job pod is completed, we will skip terminating the pod. Check whether a job is complete by:
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