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
New histogram: Pod start SLI duration #111930
New histogram: Pod start SLI duration #111930
Conversation
Please note that we're already in Test Freeze for the Fast forwards are scheduled to happen every 6 hours, whereas the most recent run was: Fri Aug 19 07:42:40 UTC 2022. |
/retest |
a654b2e
to
7bba13f
Compare
pkg/kubelet/config/config.go
Outdated
@@ -235,6 +240,10 @@ func (s *podStorage) merge(source string, change interface{}) (adds, updates, de | |||
ref.Annotations = make(map[string]string) | |||
} | |||
ref.Annotations[kubetypes.ConfigSourceAnnotationKey] = source | |||
// ignore static pods | |||
if ref.ResourceVersion != "" { | |||
s.podStartupLatencyTracker.ObservedPodOnWatch(ref, time.Now()) |
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.
'when' value: can we set this to time.Now() stored when the object has been received from watch (as soon as possible) to reduce impact of possible process delay?
Or are you suggesting that this code is always happening directly after we receive the watch event?
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 see your point and I do agree, the time could (should?) be captured as soon as we observed the Pod Update, so most likely here: https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/config/apiserver.go#L64
On the other hand, the kubetypes.PodUpdate
is a main/base struct (used in 54 places in 14 files), so adding an eventTimestamp is not such a small change - why don't we wait for some input from sig-node
people?
I'm happy to make such a change, if there's a possibility, that could be a real time difference between those two options.
ea008c6
to
3a1a000
Compare
3a1a000
to
c4a93c2
Compare
/lgtm |
How hard would it be to export a separate graph with "watch latency" for pods graph only? It's quite unrelated to the change, but would be extremely useful. |
/retest |
/assign @logicalhan |
49a0902
to
2b45c9f
Compare
/retest |
Few small comments, overall LGTM. Looks like it needs a rebase though since prow is failing due to merge conflict. |
2b45c9f
to
89c8822
Compare
89c8822
to
492f5fa
Compare
Thanks for the updates! /lgtm |
Thanks David - now, the PR has the LGTM from all people involved :) |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: azylinski, dashpole, smarterclayton 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 |
What type of PR is this?
/kind feature
/sig node
/area kubelet
What this PR does / why we need it:
A new histogram will cover the gap for Pod Startup SLI/SLO measurement, ref:
https://github.com/kubernetes/community/blob/master/sig-scalability/slos/pod_startup_latency.md
This could replace the slo-monitor component.
Does this PR introduce a user-facing change?