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
pkg/kubelet/pleg: do not include exited and dead pods in running_pod_count metric #92187
Conversation
Welcome @paulfantom! |
Hi @paulfantom. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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/test-infra repository. |
/ok-to-test |
cc @kubernetes/sig-instrumentation-pr-reviews |
Looks good from instrumentation side. /lgtm Sig node needs to approve. cc @kubernetes/sig-node-bugs |
You should also correct test case, and by the way, |
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 change is not correct as it will break the ability to properly computeEvents
in the pleg.
Please update as requested.
/hold
pkg/kubelet/pleg/generic.go
Outdated
@@ -200,7 +200,7 @@ func (g *GenericPLEG) relist() { | |||
}() | |||
|
|||
// Get all the pods. | |||
podList, err := g.runtime.GetPods(true) | |||
podList, err := g.runtime.GetPods(false) |
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 change has bad side-effects. It is intentional that all pods are fetched in this context so we can properly build the pleg. The change should be that updateRunningPodAndContainerMetrics
should do the desired filtering.
a228369
to
1b27d07
Compare
New changes are detected. LGTM label has been removed. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: paulfantom 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 |
I changed it to base |
1b27d07
to
43b2431
Compare
43b2431
to
9052521
Compare
@paulfantom: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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/test-infra repository. I understand the commands that are listed here. |
|
||
for _, pod := range pods { | ||
// Count only Running pods | ||
p := pod.ToAPIPod() |
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.
Will this method ToAPIPod
update the Status
? I cannot find where it does it:
kubernetes/pkg/kubelet/container/runtime.go
Lines 598 to 613 in 5ed7b1a
// ToAPIPod converts Pod to v1.Pod. Note that if a field in v1.Pod has no | |
// corresponding field in Pod, the field would not be populated. | |
func (p *Pod) ToAPIPod() *v1.Pod { | |
var pod v1.Pod | |
pod.UID = p.ID | |
pod.Name = p.Name | |
pod.Namespace = p.Namespace | |
for _, c := range p.Containers { | |
var container v1.Container | |
container.Name = c.Name | |
container.Image = c.Image | |
pod.Spec.Containers = append(pod.Spec.Containers, container) | |
} | |
return &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 was hoping it did, but apparently it doesn't which can be seen in failing bazel-test. Any ideas how to get pod status/phase without iterating over all containers and thus duplicating logic?
Closing in favor of #85983 |
What type of PR is this?
/kind bug
What this PR does / why we need it:
Stop including exited and dead pods in the
kubelet_running_pod_count
metric.Which issue(s) this PR fixes:
Fixes #92180
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: