feat:operator:kcc: MultiClusterLease integration #5420

acpana · 2025-10-13T22:28:27Z

Integrates into operator and kcc https://github.com/gke-labs/multicluster-leader-election.
Automated testing is happening in wip:tests: MCL integ #5490

google-oss-prow · 2025-10-13T22:28:42Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign justinsb for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Signed-off-by: Alex Pana <[email protected]>

acpana · 2025-10-22T00:08:50Z

.github/license-lint-config.yml

+  # todo
+  - github.com/gke-labs/multicluster-leader-election


will need to specify the license in https://github.com/gke-labs/multicluster-leader-election and follow up here

I approved so we should be able to rebase

acpana · 2025-10-22T00:10:49Z

cmd/manager/main.go

+		// err is nil
+		// todo acpana add more defense in depth here
+		logging.ExitInfo("might've lost leader election")


walter had suggested adding some defense in depth here where we as the kccmanager process check our leadership status and bail out if we are no longer a leader.

however, AFAICT, we get that guarantee for free if mgr.Start returns, meaning we are no longer a leader if err is nil.

Is that actually what happens with the default lease objects? If/when we lose the lease then Start simply returns?

I think we should be able to listen for a cancel on the context used for leader election and exist when it is called.

acpana · 2025-10-22T00:12:01Z

go.mod

 	github.com/cenkalti/backoff v2.2.1+incompatible
 	github.com/fatih/color v1.18.0
 	github.com/ghodss/yaml v1.0.0
+	github.com/gke-labs/multicluster-leader-election v0.0.0-20250923220528-0bf41dc7fecc


All go mod changes are coming out of there. Unfortunately, I think bumping the controller-runtime lib really upset the pkg/cli/preview as it introduces an interface change. I added an unimplemtend func for now for the preview cli.

That sounds OK. It is a good reason not to update library versions in libraries. Also a good reason to stick to keep dependencies minimal in libraries. I wonder if the MCL library should be using dynamic.Interface or a similar dependency-light library. (Does one exist?)

Signed-off-by: Alex Pana <[email protected]>

gemmahou · 2025-10-22T20:53:29Z

cmd/manager/main.go

+		ManagerOptions: manager.Options{
+			Cache: cache.Options{
+				DefaultNamespaces: map[string]cache.Config{
+					scopedNamespace: {},


nit: I see this is a copy of existing code, but I'm curious shouldn't this be scopedNamespace: scopedNamespace so it takes the flag value?

actually no I think this is a great catch! I tried to do away with that level of indirection (newManager) func but I will revert that refactor to keep things less confusing and make sure I don't miss things like these!

Lets not conflate

gemmahou · 2025-10-22T22:36:04Z

operator/pkg/controllers/configconnector/experiments.go

+
+			// Add --multi-cluster-election=true flag
+			args, _, _ := unstructured.NestedStringSlice(container, "args")
+			args = append(args, "--multi-cluster-election=true")


QQ: just for self-education purpose, how this flag will be used to enable the feature? asking because I don't see we pass it anywhere, and seems like we check the existence of "cc.Spec.Experiments.LeaderElection" to determine whether to enable the feature(line 134-148 in kccmanager.go)

justinsb · 2025-10-23T10:21:33Z

operator/config/crd/bases/core.cnrm.cloud.google.com_configconnectors.yaml

                  It's recommended to use `googleServiceAccount` when running ConfigConnector in Google Kubernetes Engine (GKE) clusters with Workload Identity enabled.
                  This field cannot be specified together with `googleServiceAccount`.
                type: string
+              experiments:


Is this going to be per-namespace or per-cluster?

Seems like we should start with per-cluster level. I think namespace (or sharding) is more complicated to build, maintain, configure and understand. Things like the syncer also get more complicated. Lets ship the easier version and then work on it.

justinsb · 2025-10-23T10:22:34Z

operator/pkg/apis/core/v1beta1/configconnector_types.go

+	Namespace string `json:"namespace"`
+	// The unique name of the global lock, which must be shared by all KCC replicas
+	// and workloads across all clusters that are part of the same election.
+	GlobalLockName string `json:"globalLockName"`


We could allow limited templating in this name, e.g. gs://my-cluster-locks/kcc/prod/{namespace}

justinsb · 2025-10-23T10:23:59Z

operator/pkg/controllers/configconnector/experiments.go

+func (r *Reconciler) applyMultiClusterLeaderElection(ctx context.Context, obj *manifest.Objects) error {
+	log := log.FromContext(ctx)
+	for _, item := range obj.Items {
+		if item.Kind != "StatefulSet" || !strings.HasPrefix(item.GetName(), "cnrm-controller-manager") {


Nit: we might want to extract this if statement into a shared function, it's both a little less fragile / more maintainable and it documents more clearly what we are doing.

e.g. IsControllerManagerStatefulSet

justinsb · 2025-10-23T10:25:11Z

operator/pkg/controllers/configconnector/experiments.go

+		log.Info("enabling multi-cluster leader election for StatefulSet", "name", item.GetName())
+		if err := item.MutateContainers(func(container map[string]interface{}) error {
+			name, found, _ := unstructured.NestedString(container, "name")
+			if !found || name != "manager" {


Nit: if !found, then name will be empty, so you don't need to check found

justinsb · 2025-10-23T10:26:27Z

pkg/controller/kccmanager/kccmanager.go

 	"sigs.k8s.io/controller-runtime/pkg/manager"
-
-	// Register direct controllers
-	_ "github.com/GoogleCloudPlatform/k8s-config-connector/pkg/controller/direct"


I think you need this (?). If we don't, that's interesting and it should be a separate PR I think

justinsb · 2025-10-23T10:26:52Z

pkg/controller/kccmanager/kccmanager.go

 // This serves as the entry point for the in-cluster main and the Borg service main. Any changes made should be done
 // with care.
 func New(ctx context.Context, restConfig *rest.Config, cfg Config) (manager.Manager, error) {
+	krmtotf.SetUserAgentForTerraformProvider()


Also looks unrelated to the MCL integration (?)

justinsb · 2025-10-23T10:31:21Z

pkg/controller/kccmanager/kccmanager.go

 		return nil, fmt.Errorf("error adding schemes: %w", err)
 	}

+	// Create a temporary client to read the ConfigConnector object for leader election config.


Hmmm, this is clever but I just don't know. We should at least split it out into its own function to avoid accidentally reusing the client.

I think because we aren't going to watch this object, we should rely on the operator to manage the restarts / rollout.

Maybe invent something like a JDBC URL for leader election syntax to make it a one liner, e.g.

gs://<bucket>/<path>?timeout=15s&namespace=foo

I'm also not sure who should create the lease objects, should we simply accept a pointer to a pre-configured lease object?

justinsb · 2025-10-23T10:33:27Z

go.mod

 	github.com/cenkalti/backoff v2.2.1+incompatible
 	github.com/fatih/color v1.18.0
 	github.com/ghodss/yaml v1.0.0
+	github.com/gke-labs/multicluster-leader-election v0.0.0-20250923220528-0bf41dc7fecc


That sounds OK. It is a good reason not to update library versions in libraries. Also a good reason to stick to keep dependencies minimal in libraries. I wonder if the MCL library should be using dynamic.Interface or a similar dependency-light library. (Does one exist?)

google-oss-prow bot added the do-not-merge/work-in-progress label Oct 13, 2025

acpana force-pushed the acpana/integrate-mcl branch 2 times, most recently from ed9b767 to ff8b3e1 Compare October 20, 2025 19:28

acpana force-pushed the acpana/integrate-mcl branch 2 times, most recently from 1277beb to 8fe8d41 Compare October 21, 2025 22:37

acpana added 2 commits October 21, 2025 22:51

feat:operator: MCL integration

8ee6392

Signed-off-by: Alex Pana <[email protected]>

hack: license lint

f79c640

Signed-off-by: Alex Pana <[email protected]>

acpana force-pushed the acpana/integrate-mcl branch from 8fe8d41 to ee7d7b7 Compare October 21, 2025 22:52

acpana changed the title ~~[WIP]: feat:operator: MCL integration~~ feat:operator: MCL integration Oct 21, 2025

acpana force-pushed the acpana/integrate-mcl branch 3 times, most recently from 64b7a95 to 11aff87 Compare October 21, 2025 23:50

feat:kcc: MCL integration

cfe58c7

Signed-off-by: Alex Pana <[email protected]>

acpana force-pushed the acpana/integrate-mcl branch from 11aff87 to cfe58c7 Compare October 22, 2025 00:00

acpana commented Oct 22, 2025

View reviewed changes

hack: unimplemeneted

564600e

Signed-off-by: Alex Pana <[email protected]>

acpana marked this pull request as ready for review October 22, 2025 20:02

google-oss-prow bot removed the do-not-merge/work-in-progress label Oct 22, 2025

acpana changed the title ~~feat:operator: MCL integration~~ feat:operator:kcc: MCL integration Oct 22, 2025

gemmahou reviewed Oct 22, 2025

View reviewed changes

justinsb reviewed Oct 23, 2025

View reviewed changes

cheftako changed the title ~~feat:operator:kcc: MCL integration~~ feat:operator:kcc: MultiClusterLease integration Oct 24, 2025

feat:operator:kcc: MultiClusterLease integration #5420

Are you sure you want to change the base?

feat:operator:kcc: MultiClusterLease integration #5420

Uh oh!

Conversation

acpana commented Oct 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

google-oss-prow bot commented Oct 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acpana Oct 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemmahou Oct 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinsb Oct 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

acpana commented Oct 13, 2025 •

edited

Loading

acpana Oct 22, 2025 •

edited

Loading

gemmahou Oct 22, 2025 •

edited

Loading

justinsb Oct 23, 2025 •

edited

Loading