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
proxy: followup to last-queued-change metric #90972
proxy: followup to last-queued-change metric #90972
Conversation
/sig network |
/cc @danwinship @dcbw |
But there's still a race condition at startup even with this patch isn't there? If you look at the metrics before the initial sync, you'll get nonsense? I'm not sure what the expected behavior for timestamp metrics in this case is... Kube-proxy always forces a resync shortly after startup, so arguably just starting up kube-proxy counts as requesting a proxy sync, and it might be reasonable to just set |
It is indeed awkward for all "gauge" metrics. Now that Prometheus handles stale metrics better, the convention is to not report an unobserved metric. Unfortunately, the existing library (and the k8s wrapper around it) makes this somewhat awkward. I'm asking around to see if there's a reasonable way to do this.
Yeah, that might be the right thing to do. Arguably, for a running system, the user doesn't care about the difference between a slow informer sync and a slow iptables sync. Either way, there's latency. |
Fixes two small issues with the metric added in kubernetes#90175: 1. Bump the timestamp on initial informer sync. Otherwise it remains 0 if restarting kube-proxy in a quiescent cluster, which isn't quite right. 2. Bump the timestamp even if no healthz server is specified.
cf0cfc6
to
042daa2
Compare
OK, updated to bump the timestamp on proxy startup. |
/lgtm |
/retest |
Update: I asked around, and prometheus client-go doesn't currently have an ergonomic way of exposing a Gauge (timestamp) that has no value until first observed. In this particular case, we don't care anymore since we're bumping the value on startup. |
oh forgot that I have to say |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danwinship, squeed 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 |
/retest Review the full test history for this PR. Silence the bot with an |
/retest Review the full test history for this PR. Silence the bot with an |
1 similar comment
/retest Review the full test history for this PR. Silence the bot with an |
@squeed: The following test 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. |
Fixes two small issues with the metric added in #90175:
/kind cleanup
What this PR does / why we need it:
The metrics introduced in #90175 don't quite match expectations.
Does this PR introduce a user-facing change?:
No, because this has not yet been released.