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

[1.14] operator: propagate CiliumClusterConfig when in kvstore mode #32349

Merged
merged 1 commit into from
May 7, 2024

Conversation

hemanthmalla
Copy link
Member

@hemanthmalla hemanthmalla commented May 3, 2024

1.14 backport of #27109 to support running kvstoremesh in kvstore identity allocation mode. Currently cluster-config in local kvstore is only set by apiserver component of clustermesh-apiserver. In this config, we don't need to run apiserver component. So, without this commit remote clusters cannot discover cluster-id and will default to using 0 as cluster ID.

@hemanthmalla hemanthmalla requested a review from a team as a code owner May 3, 2024 18:18
@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 May 3, 2024
@hemanthmalla
Copy link
Member Author

/test

Copy link
Member

@giorio94 giorio94 left a comment

Choose a reason for hiding this comment

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

The backport looks reasonable to me, being straightforward and extremely unlikely to introduce regressions.

Could you please just edit the commit message to link the upstream commit for consistency?

[ upstream commit 45174b3c97779a4288b59f6650d4d4b7d6380418 ]

You can also use ./contrib/backporting/cherry-pick 45174b3c9777 to automatically add it while cherry-picking the patch. And also fix the link in the PR description? The correct one is #27109.

Copy link
Contributor

@youngnick youngnick left a comment

Choose a reason for hiding this comment

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

LGTM once @giorio94's changes are done.

@giorio94
Copy link
Member

giorio94 commented May 6, 2024

(i forgot to mention earlier, FYI the trigger phrase on the v1.14 branch is test-backport-1.14)

@hemanthmalla
Copy link
Member Author

/test-backport-1.14

@hemanthmalla
Copy link
Member Author

@giorio94 Thanks for catching that. Updated the commit and PR description.

Copy link
Member

@giorio94 giorio94 left a comment

Choose a reason for hiding this comment

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

Thanks!

[ upstream commit 45174b3 ]

5e5a26e ("clustermesh: Implement a basic connect-time validation")
introduced the CiliumClusterConfig, which is stored in the kvstore by
the clustermesh-apiserver at startup time to propagate information about
the current cluster (e.g., its cluster ID). The same operation, though,
was not performed when cilium is configured in kvstore mode (the
clustermesh-apiserver is not enabled in this case). Let's fix this
deviation implementing the same logic also in the cilium operator.
No capabilities are set in this case, because sync canaries are not
supported (and not needed given that the kvstore is expected to leverage
persistent storage in this case).

Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
@hemanthmalla
Copy link
Member Author

Rebased to pull in c5ab76b

@hemanthmalla
Copy link
Member Author

/test-backport-1.14

@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 May 6, 2024
@ldelossa ldelossa merged commit 52c600d into cilium:v1.14 May 7, 2024
61 checks passed
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

4 participants