-
Notifications
You must be signed in to change notification settings - Fork 38.6k
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 Containers] Pods comparison by maxContainerRestarts should account for sidecar containers #124952
base: master
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: AxeZhan The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
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.
I guess we need to check the feature gate to change the existing behavior.
@@ -190,14 +199,32 @@ func podReadyTime(pod *corev1.Pod) *metav1.Time { | |||
return &metav1.Time{} | |||
} | |||
|
|||
func maxContainerRestarts(pod *corev1.Pod) int { | |||
func maxContainerRestarts(cs []corev1.ContainerStatus) int { |
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 don't we get the pod here and take restartable init containers into account inside of this function to remove the redundancy?
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.
You mean a function like maxContainerRestarts(p1 *corev1.Pod, p2 *corev1.Pod) int
?
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.
I thought about the function that returns the maximum container restarts for regular containers AND sidecar containers of the given pod.
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.
I think it's a good idea :)
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 for the confusion.
I meant one maximum count across all containers including regular containers and sidecar containers.
What do you think?
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.
This is inconsistent with what we originally designed.
I think the desireable behavior will be to check regular containers max restart count first. And compare this.
Then compare max restart count for restarteable init containers.
/cc @SergeyKanzhelev
I'm not sure about this also: If the pod has no sidecar containers, then existing logic won't be broken here.
I tend to compare it as long as there are sidecar containers in the pod. |
959b9fb
to
426bb40
Compare
I guess we have applied the feature gate since we cannot guarantee that it never break anything. However, I am ok with not applying it as the patch looks simple and predictable. |
I have no strong view either, let's wait for other reviewers:) |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Today, there are a few uses of the function maxContainerRestarts - mostly to compare pods to decide which one is
better to delete or which logs to get.
The code only look at Container Statuses, but likely need to look at init container statuses as well.
Especially in case of sidecar containers that may behave exactly as regular containers.
We may need to be careful including all init container statuses. If a Pod was failing to start for a while
because of Init container failures and now it is running OK, it is likely not important. However, including
the restartable containers (sidecars) restart count is important.
I think the desireable behavior will be to check regular containers max restart count first. And compare this.
Then compare max restart count for restarteable init containers.
Which issue(s) this PR fixes:
Fixes #124936
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: