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

v1.14 backports 2023-11-20 #29270

Merged
merged 3 commits into from
Nov 24, 2023
Merged

v1.14 backports 2023-11-20 #29270

merged 3 commits into from
Nov 24, 2023

Conversation

sayboras
Copy link
Member

@sayboras sayboras commented Nov 20, 2023

Once this PR is merged, you can update the PR labels via:

$ for pr in 27007 29178 29189; do contrib/backporting/set-labels.py $pr done 1.14; done

[ upstream commit 0eba8bc ]

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

Signed-off-by: Yusuke Suzuki <yusuke-suzuki@cybozu.co.jp>
Signed-off-by: Tam Mach <tam.mach@cilium.io>
@maintainer-s-little-helper maintainer-s-little-helper bot added backport/1.14 This PR represents a backport for Cilium 1.14.x of a PR that was merged to main. kind/backports This PR provides functionality previously merged into master. labels Nov 20, 2023
[ upstream commit 1fe2281 ]

The same connectivity tests should be run by ci-ipsec-e2e.

Suggested-by: Paul Chaignon <paul.chaignon@gmail.com>
Signed-off-by: Martynas Pumputis <m@lambda.lt>
Signed-off-by: Tam Mach <tam.mach@cilium.io>
[ upstream commit 38a705b ]

See cilium/cilium-cli#2110.

Signed-off-by: Martynas Pumputis <m@lambda.lt>
Signed-off-by: Tam Mach <tam.mach@cilium.io>
@sayboras sayboras marked this pull request as ready for review November 20, 2023 11:12
@sayboras sayboras requested review from a team as code owners November 20, 2023 11:12
@sayboras
Copy link
Member Author

/test-backport-1.14

Copy link
Member

@brb brb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@brb
Copy link
Member

brb commented Nov 22, 2023

/ci-ipsec-upgrade

Copy link
Member

@ysksuzuki ysksuzuki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! LGTM

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Nov 23, 2023
@lmb lmb merged commit 2948102 into v1.14 Nov 24, 2023
204 checks passed
@lmb lmb deleted the pr/v1.14-backport-2023-11-20 branch November 24, 2023 10:32
@ysksuzuki ysksuzuki mentioned this pull request Nov 30, 2023
14 tasks
@christarazi
Copy link
Member

@lmb Just a heads up when merging backport PR to remember to run the scripts to set the labels as described in the PR description. Our release scripts rely on this process and may cause at worst missed PRs into releases and at best, missing release notes for PRs that did land.

@lmb
Copy link
Contributor

lmb commented Dec 1, 2023

Ok!

@aanm
Copy link
Member

aanm commented Dec 1, 2023

@lmb Just a heads up when merging backport PR to remember to run the scripts to set the labels as described in the PR description. Our release scripts rely on this process and may cause at worst missed PRs into releases and at best, missing release notes for PRs that did land.

@christarazi we already have automation that does this https://github.com/cilium/cilium/actions/workflows/call-backport-label-updater.yaml

/cc @lmb

@christarazi
Copy link
Member

@aanm Oh I didn't realize that. That's great and I wonder why it didn't properly work for this PR. I had to manually fix up some backport labels on PRs, so not sure what went wrong.

@sayboras
Copy link
Member Author

sayboras commented Dec 2, 2023

Thanks all.

It seems like the backport script didn't throw the errors, or I overlooked them, hence the pending-backport/1.14 label was not set for mentioned PRs.

Will be more cautious next time 🙇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.14 This PR represents a backport for Cilium 1.14.x of a PR that was merged to main. kind/backports This PR provides functionality previously merged into master. ready-to-merge This PR has passed all tests and received consensus from code owners to merge.
Projects
No open projects
Status: Released
Development

Successfully merging this pull request may close these issues.

None yet

8 participants