Skip to content

helm: kvstoremesh with external etcd #36216

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 12, 2025
Merged

helm: kvstoremesh with external etcd #36216

merged 1 commit into from
Feb 12, 2025

Conversation

balous
Copy link
Contributor

@balous balous commented Nov 27, 2024

When running clustermesh-apiserver, there should be an option to enable and disable containers that are not needed.
For example the etcd and apiserver containers are not necessary when running kvstoremesh with an external kvstore.

This commit adds the options to disable these containers.

This PR is revival of #34174, its comments were adressed and some fixes were made.

Added possibility to run kvstoremesh with external etcd.

@balous balous requested review from a team as code owners November 27, 2024 19:33
@balous balous requested review from youngnick, gandro, giorio94 and a user November 27, 2024 19:33
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Nov 27, 2024
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Nov 27, 2024
@balous balous mentioned this pull request Nov 27, 2024
Closed
@balous balous force-pushed the kvstoremode branch 2 times, most recently from 63a97ac to be68a63 Compare November 28, 2024 05:10
giorio94
giorio94 requested changes Nov 28, 2024
View reviewed changes
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 @balous.

I've left a bunch of comments inline. Overall, my main concern, as I had also commented in the original PR, is that with these new modes it is fairly easy for users to end-up in broken scenarios (e.g., if two replicas are active at the same time), although arguably the additional validation is already preventing a bunch of completely unsupported configuration. I think that one essential aspect here (once the main issues have been addressed) would be to clearly frame this functionality as for advanced users only, who have a lot of expertise in this area.

Another aspect I see completely missing is the testing plan, as otherwise there's no guarantee it will not break over time.

/cc @marseel @hemanthmalla @oblazek, as you commented on the original PR.

@giorio94 giorio94 added release-note/minor This PR changes functionality that users may find relevant to operating Cilium. area/clustermesh Relates to multi-cluster routing functionality in Cilium. area/kvstore Impacts the KVStore package interactions. labels Nov 28, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Nov 28, 2024
@gandro gandro marked this pull request as draft December 4, 2024 10:10
@gandro
Copy link
Member

gandro commented Dec 4, 2024

Coverted this into draft until the feedback is addressed. Feel free to mark this "Ready for review" when ready.

@marseel marseel mentioned this pull request Dec 11, 2024
Merged
8 tasks
@github-actions GitHub Actions
Copy link

This pull request has been automatically marked as stale because it
has not had recent activity. It will be closed if no further activity
occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Jan 10, 2025
@github-actions github-actions bot removed the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Jan 15, 2025
@balous balous force-pushed the kvstoremode branch 4 times, most recently from e7e8202 to 1887f23 Compare January 16, 2025 08:51
@balous balous marked this pull request as ready for review January 20, 2025 14:09
gandro
gandro approved these changes Jan 21, 2025
View reviewed changes
Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

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

Helm looks good to me structure-wise, thank you! I left one non-blocking question to make sure I understood things correctly, since I'm not familiar with kvstoremesh at all.

@gandro gandro requested review from a team and marseel and removed request for a team January 21, 2025 10:20
@giorio94 giorio94 self-requested a review January 21, 2025 10:31
@oblazek oblazek mentioned this pull request Jan 21, 2025
Closed
ghost
ghost reviewed Jan 21, 2025
View reviewed changes
giorio94
giorio94 reviewed Jan 28, 2025
View reviewed changes
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, I took a fresh look (and did a bit of local testing). Overall looks mostly good to me, just a few extra minor comments inline.

@marseel marseel removed their request for review January 28, 2025 13:10
@marseel
Copy link
Contributor

marseel commented Jan 28, 2025

Removing myself from review, Marco is already reviewing from sig-clustermesh perspective

@balous balous force-pushed the kvstoremode branch 2 times, most recently from 51b272d to 2e6d142 Compare January 30, 2025 10:01
giorio94
giorio94 reviewed Jan 31, 2025
View reviewed changes
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.

Looks almost ready to me 🚀. A couple more comments inline.

@balous balous force-pushed the kvstoremode branch 2 times, most recently from 7ae6804 to b29e3fe Compare February 3, 2025 15:13
giorio94
giorio94 approved these changes Feb 3, 2025
View reviewed changes
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

@giorio94
Copy link
Member

giorio94 commented Feb 3, 2025

/test

@giorio94 giorio94 requested review from a user and removed request for youngnick and a user February 3, 2025 16:13
@calcium6 @balous
helm: kvstoremesh with external etcd
ffd613d 
have the option to use external etcd with kvstoremesh

Signed-off-by: Jiri Vanura <jiri.vanura@firma.seznam.cz>
@ghost ghost removed their request for review February 3, 2025 22:52
@giorio94
Copy link
Member

giorio94 commented Feb 4, 2025

/test

@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 Feb 10, 2025
@joestringer joestringer added this pull request to the merge queue Feb 12, 2025
Merged via the queue into cilium:main with commit 14544e4 Feb 12, 2025
66 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gandro gandro gandro approved these changes

@giorio94 giorio94 giorio94 approved these changes

+1 more reviewer
Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
area/clustermesh Relates to multi-cluster routing functionality in Cilium. area/kvstore Impacts the KVStore package interactions. kind/community-contribution This was a contribution made by a community member. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@balous @gandro @giorio94 @marseel @joestringer @calcium6