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.

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

clustermesh-apiserver: add flag to disable external workloads support #25259

Merged
merged 1 commit into from
May 9, 2023
Merged

clustermesh-apiserver: add flag to disable external workloads support #25259

merged 1 commit into from
May 9, 2023

Conversation

giorio94
Copy link
Member

@giorio94 giorio94 commented May 4, 2023

Currently, the support for external workloads is always enabled in the clustermesh-apiserver. Yet, this feature requires the synchronization of all the services present in the cluster to the kvstore, including the corresponding backends (plain clustermesh instead only requires the synchronization of shared services and associated backends).

Given that full synchronization is quite onerous in large clusters, let's make it configurable through a dedicated flag. By default, it is enabled, in order not to modify the current behavior when unspecified (e.g., by the legacy Cilium CLI). When installing cilium through Helm, instead, it is configured according to the pre-existing externalWorkloads.enabled configuration entry.

Allow to disable external workloads support in clustermesh-apiserver to improve performance when not needed.

@giorio94
clustermesh-apiserver: add flag to disable external workloads support
361b750 
Currently, the support for external workloads is always enabled in the
clustermesh-apiserver. Yet, this feature requires the synchronization of
all the services present in the cluster to the kvstore, including the
corresponding backends (plain clustermesh instead only requires the
synchronization of shared services and associated backends).

Given that full synchronization is quite onerous in large clusters,
let's make it configurable through a dedicated flag. By default, it
is enabled, in order not to modify the current behavior when unspecified
(e.g., by the legacy Cilium CLI). When installing cilium through Helm,
instead, it is configured according to the pre-existing
externalWorkloads.enabled configuration entry.

Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
@giorio94 giorio94 added kind/performance There is a performance impact of this. 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. labels May 4, 2023
@giorio94 giorio94 requested review from a team as code owners May 4, 2023 13:02
@giorio94
Copy link
Member Author

giorio94 commented May 4, 2023

/test

@giorio94 giorio94 mentioned this pull request May 4, 2023
Closed
@giorio94
Copy link
Member Author

giorio94 commented May 4, 2023

/ci-gke

asauber
asauber approved these changes May 5, 2023
View reviewed changes
Copy link
Member

@asauber asauber left a comment

Choose a reason for hiding this comment

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

Have you tested this interactively?

@giorio94
Copy link
Member Author

giorio94 commented May 5, 2023 •
edited

/ci-gke

Failed due to yesterday's GitHub incident.

@giorio94
Copy link
Member Author

giorio94 commented May 5, 2023 •
edited

/test-1.26-net-next

Hit known flake #24697

@giorio94
Copy link
Member Author

giorio94 commented May 5, 2023 •
edited

/test-runtime

Hit #25276 and #25281

@giorio94 giorio94 mentioned this pull request May 5, 2023
Closed
@giorio94
Copy link
Member Author

giorio94 commented May 5, 2023

Have you tested this interactively?

@asauber Do you mean using the cilium CLI? The legacy one uses the hard-coded manifests, hence triggering the previous behavior as the flag is not specified. The new one, instead, sets the flag to false, which is the default in helm (this is something that we should keep in mind for the cilium clustermesh externalworkloads commands, but it already applies also for the certificates). The only issue with the new CLI was that I couldn't specify the clustermesh-apiserver image version through cilium clustermesh enable (I had to configure that in advance while installing cilium).

@asauber
Copy link
Member

asauber commented May 5, 2023

@giorio94 Have you compiled this locally and tested the flag? (is what I was asking)

@giorio94
Copy link
Member Author

giorio94 commented May 8, 2023

@giorio94 Have you compiled this locally and tested the flag? (is what I was asking)

Yep, I've run it both with and without the flag set.

thorn3r
thorn3r approved these changes May 9, 2023
View reviewed changes
clustermesh-apiserver/main.go
@@ -248,6 +249,11 @@ func runApiserver() error {
flags.Bool(option.K8sEnableEndpointSlice, defaults.K8sEnableEndpointSlice, "Enable support of Kubernetes EndpointSlice")
option.BindEnv(vp, option.K8sEnableEndpointSlice)

// The default values is set to true to match the existing behavior in case
// the flag is not configured (for instance by the legacy cilium CLI).
flags.BoolVar(&cfg.enableExternalWorkloads, option.EnableExternalWorkloads, true, "Enable support for external workloads")
Copy link
Contributor

Choose a reason for hiding this comment

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

could this default value go in in defaults.EnableExternalWorkloads?

Copy link
Member Author

Choose a reason for hiding this comment

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

I personally find flags a bit easier to read when the value is inline rather than defined as a constant, since it does not require to navigate through the code to find it (assuming that it is not reused in multiple places, otherwise having a constant ensures consistency).

I don't have any strong preference though, and the other flags defined don't seem to be consistent in that regard.

Copy link
Contributor

Choose a reason for hiding this comment

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

seems like the other flags using defaults are also used elsewhere, so seems like it makes sense to keep it as is 👍

@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 9, 2023
@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 9, 2023
@michi-covalent michi-covalent merged commit e772ba8 into cilium:main May 9, 2023
57 of 58 checks passed
This was referenced May 12, 2023
Closed
Merged
@giorio94 giorio94 mentioned this pull request Jun 14, 2023
Closed
2 tasks
@giorio94 giorio94 deleted the mio/clustermesh-apiserver-external-workloads branch June 20, 2023 06:41
@giorio94 giorio94 added the needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch label Jun 20, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.13.3 Jun 20, 2023
@giorio94
Copy link
Member Author

Marked as needs-backport for v1.13: #26049 (comment)

@nbusseneau nbusseneau mentioned this pull request Jun 22, 2023
Merged
19 tasks
@nbusseneau nbusseneau added backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. and removed needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch labels Jun 22, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from main to Backport pending to v1.13 in 1.13.3 Jun 22, 2023
@tklauser tklauser added backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. and removed backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. labels Jun 29, 2023
@georgew5656
Copy link

will the helm change be backported into the release branch? https://github.com/cilium/cilium/tree/v1.13.3/install/kubernetes/cilium

@giorio94
Copy link
Member Author

Yes, this change has been backported to v1.13 (although it is not yet part of any release).

@gentoo-root gentoo-root mentioned this pull request Jul 26, 2023
Merged
@nathanjsweet nathanjsweet removed this from Backport pending to v1.13 in 1.13.3 Nov 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thorn3r thorn3r thorn3r approved these changes

@gandro gandro gandro approved these changes

@nathanjsweet nathanjsweet nathanjsweet approved these changes

@marseel marseel marseel approved these changes

@asauber asauber asauber approved these changes

@YutaroHayakawa YutaroHayakawa YutaroHayakawa approved these changes

Assignees
No one assigned
Labels
area/clustermesh Relates to multi-cluster routing functionality in Cilium. backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. kind/performance There is a performance impact of this. 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.

None yet