metrics: fix potential conflict on metrics registration #27007
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
maintainer-s-little-helper
bot
added
the
dont-merge/needs-release-note-label
|
context: #26412 (review) |
ysksuzuki
force-pushed
the
fix-client-go-metrics-conflict
branch
from
453cdf7
to
891e79b
Compare
|
/test |
pkg/k8s/watchers/watcher.go
Outdated
| @@ -93,13 +93,27 @@ func init() { | |||
| k8s.K8sErrorHandler, | |||
| } | |||
|
|
|||
| k8s_metrics.Register(k8s_metrics.RegisterOpts{ | |||
| // The client-go Register function can be called only once, | |||
| // but controller-runtime also calls it in its init function | |||
There was a problem hiding this comment.
nit: we can maybe generalize that any in case of transitive imports can cause this condition to happen (while also mentioning the specific controller-runtime scenario that was the motivation here).
There was a problem hiding this comment.
I rephrased it. Please take a look.
ysksuzuki
force-pushed
the
fix-client-go-metrics-conflict
branch
from
891e79b
to
a0d56f3
Compare
|
/test |
|
ginkgo build cache seems broken |
|
/test |
ysksuzuki
force-pushed
the
fix-client-go-metrics-conflict
branch
from
a0d56f3
to
f4c4ebc
Compare
In Cilium v1.13, K8s rest client metrics, such as cilium_k8s_client_api_latency_time_seconds and cilium_k8s_client_api_calls_total, are missing because of a conflict with controller-runtime on the metrics registration. controller-runtime(sigs.k8s.io/controller-runtime/pkg/metrics) also registers the client-go rest client metrics on its registry using metrics.Register method in its init function. The metrics.Register can be called only once, and subsequent calls will be ignored. cilium-agent doesn't use controller-runtime, but there's an indirect dependency from github.com/cilium/cilium/daemon to github.com/cilium/cilium/operator/metrics package. (operator uses controller-runtime in its Gateway API implementation) For example, github.com/cilium/cilium/daemon/cmd -> github.com/cilium/cilium/pkg/ipam -> github.com/cilium/cilium/pkg/ipam/metrics -> github.com/cilium/cilium/operator/metrics The init function of sigs.k8s.io/controller-runtime/pkg/metrics will be called because of this indirect dependency. The issue(missing client-go metrics) doesn't happen with v1.14 and later, but it can happen if the order of imports changes again. This commit is to prevent the issue from happning. This commit is a forward port of cilium@e4ff641 Signed-off-by: Yusuke Suzuki <yusuke-suzuki@cybozu.co.jp>
|
/test |
|
@tommyp1ckles Do you have any other feedback or question? |
tommyp1ckles
added
the
release-note/bug
maintainer-s-little-helper
bot
removed
the
dont-merge/needs-release-note-label
tommyp1ckles
approved these changes
Aug 25, 2023
maintainer-s-little-helper
bot
added
ready-to-merge
tommyp1ckles
added
the
area/metrics
|
@tommyp1ckles do we need backport labels? |
tommyp1ckles
added
backport/1.13
|
@borkmann 👍 This fixues a bug in 1.13, but this sounds like it may prevent future issues in 1.14 as well. |
tommyp1ckles
removed
backport/1.13
tommyp1ckles
added
needs-backport/1.14
github-actions
bot
added
the
backport-done/1.14
joamaki
added
backport-pending/1.14
christarazi
added
backport-done/1.14
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In Cilium v1.13, K8s rest client metrics, such as cilium_k8s_client_api_latency_time_seconds and cilium_k8s_client_api_calls_total, are missing because of a conflict with controller-runtime on the metrics registration.
controller-runtime(sigs.k8s.io/controller-runtime/pkg/metrics) also registers the client-go rest client metrics on its registry using metrics.Register method in its init function. The metrics.Register can be called only once, and subsequent calls will be ignored.
cilium-agent doesn't use controller-runtime, but there's an indirect dependency from github.com/cilium/cilium/daemon to github.com/cilium/cilium/operator/metrics package. (operator uses controller-runtime in its Gateway API implementation) For example,
github.com/cilium/cilium/daemon/cmd
-> github.com/cilium/cilium/pkg/ipam
-> github.com/cilium/cilium/pkg/ipam/metrics
-> github.com/cilium/cilium/operator/metrics
The init function of sigs.k8s.io/controller-runtime/pkg/metrics will be called because of this indirect dependency.
The issue(missing client-go metrics) doesn't happen with v1.14 and later, but it can happen if the order of imports changes again. This commit is to prevent the issue from happning.
This commit is a forward port of e4ff641
fixes: #25610
Please ensure your pull request adheres to the following guidelines:
description and a
Fixes: #XXXline if the commit addresses a particularGitHub issue.
Fixes: <commit-id>tag, thenplease add the commit author[s] as reviewer[s] to this issue.