Skip to content
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 #124936

Open
SergeyKanzhelev opened this issue May 17, 2024 · 2 comments · May be fixed by #124952
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug. priority/backlog Higher priority than priority/awaiting-more-evidence. sig/apps Categorizes an issue or PR as relevant to SIG Apps. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@SergeyKanzhelev
Copy link
Member

What happened?

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. This is not a huge issue, mostly a quality of life improvement.

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.

There are 2 implementations and 5 comparison interfaces.

Implementatons in:

  • pkg/controller/controller_utils.go
  • staging/src/k8s.io/kubectl/pkg/util/podutils/podutils.go
func maxContainerRestarts(pod *v1.Pod) int {
	maxRestarts := 0
	for _, c := range pod.Status.ContainerStatuses {
		maxRestarts = max(maxRestarts, int(c.RestartCount))
	}
	return maxRestarts
}

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.

/kind bug

What did you expect to happen?

Comparison of max container restarts account for init containers as well as regular containers.

How can we reproduce it (as minimally and precisely as possible)?

Two pods - one with the sidecar container in constant restart loop and one is running sucessfully. Random one will be picked to get logs from.

Anything else we need to know?

/sig apps
/priority backlog

KEP: kubernetes/enhancements#753

@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. sig/apps Categorizes an issue or PR as relevant to SIG Apps. priority/backlog Higher priority than priority/awaiting-more-evidence. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels May 17, 2024
@AxeZhan
Copy link
Member

AxeZhan commented May 19, 2024

/assign

@gjkim42
Copy link
Member

gjkim42 commented May 20, 2024

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels May 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. priority/backlog Higher priority than priority/awaiting-more-evidence. sig/apps Categorizes an issue or PR as relevant to SIG Apps. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
Status: Needs Triage
4 participants