Introduce Leader Election for Gloo #6926

sam-heilbron · 2022-08-10T18:48:02Z

Description

Introduce leader election to Gloo component

Context

Introduce leader election if using Kubernetes as the source of truth for resources.

Solution

Initially I place the leader election code inside the Gloo SetupFunc, which is executed each time Settings change. The effect was that each time the SetupFunc was called, the election process was cancelled (since we use a new context for each setup), a new leader was elected, and inconsistent behavior was observed.

I moved this election logic to the main setup portion so that it happens once during startup, and if leadership is lost, we kill the application.

Technical Debt

This is called out loudly in a code comment in https://github.com/solo-io/gloo/blob/master/projects/gloo/pkg/api/converters/kube/artifact_converter.go, where the debt is incurred.

We need to ignore the configmap (or whatever kube resource maintains the state of the leader) during translation. Since it is updated on an interval (2 seconds) if it's processed by Gloo controllers, we will resync the entire state of the world continually.

Ideally, we ignore configmaps with a particular label, but that isn't supported in solo-kit. The faster solution, is to ignore it explicitly in code for now, and handle the more robust solution in a follow-up.

Follow Up Work

Solo-kit enhancements to resource filtering
Helm hardening around other HA features

Enterprise Follow-Up

To support this in Enterprise, the following work will be required: https://github.com/solo-io/solo-projects/pull/3974

Checklist:

I included a concise, user-facing changelog (for details, see https://github.com/solo-io/go-utils/tree/master/changelogutils) which references the issue that is resolved.
If I updated APIs (our protos) or helm values, I ran make -B install-go-tools generated-code to ensure there will be no code diff
I followed guidelines laid out in the Gloo Edge contribution guide
I opened a draft PR or added the work in progress label if my PR is not ready for review
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works

projects/gloo/pkg/syncer/translator_syncer.go

pkg/bootstrap/leaderelector/election_factory.go

projects/gloo/pkg/syncer/setup/setup_syncer.go

pkg/bootstrap/leaderelector/kube/factory.go

github-actions · 2022-08-11T04:17:30Z

Visit the preview URL for this PR (updated for commit 81a3ac5):

https://gloo-edge--pr6926-ha-part-3-g0d9pb36.web.app

_{(expires Mon, 22 Aug 2022 18:36:04 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

"

go.mod

jenshu · 2022-08-11T12:45:51Z

pkg/bootstrap/leaderelector/kube/factory.go

+	// Create the resource Lock interface necessary for leader election.
+	// Controller runtime requires an event handler provider, but that package is
+	// internal so for right now we pass a noop handler.
+	resourceLock, err := leaderelection.NewResourceLock(f.restCfg, NewNoopProvider(), leOpts)


what is the effect of using a noop handler? will we be missing some functionality?

A noop handler is just the implementation for leader election that does nothing (ie what we have today, where we only support a single replica). Is there anything I can do to help make that clearer?

jenshu · 2022-08-11T12:49:17Z

pkg/utils/namespaces.go

+	if podNamespace := os.Getenv("POD_NAMESPACE"); podNamespace != "" {
+		return podNamespace
+	}
+	return "gloo-system"


i think we already have a const defined for this somewhere

Yeah the challenge I ran into, is that that constant is defined in some projects/gloo/pkg/utils, which causes a circular dependency. I would like in the future (and I tried in my other cleanup PR) to define constants at the top level, without dependencies so they can be imported easily.

jenshu · 2022-08-11T12:58:21Z

pkg/bootstrap/leaderelector/kube/factory.go

+					config.OnStoppedLeading()
+				},
+				OnNewLeader: func(identity string) {
+					contextutils.LoggerFrom(ctx).Debugf("New Leader Elected with Identity: %s", identity)


will this print any info that helps identify which pod is the leader? looks like identity only contains a bool

Since this is the base implementation, I tried to keep it as lightweight as possible. I could certainly, add information about the identity of the leader in here, or we could configure Gloo elections to define their "OnNewLeader" callback to print this information. What do you think?

jenshu · 2022-08-11T13:03:35Z

test/kube2e/gateway/gateway_suite_test.go

@@ -129,6 +129,9 @@ settings:
  replaceInvalidRoutes: true
 gateway:
  persistProxySpec: true
+gloo:
+  deployment:
+    replicas: 2


will you be adding some e2e testing around making sure that leader election works with the 2 replicas?

Yeah first I want to make sure that existing regression tests just all work with multiple replicas.

Ideally I'd like to come up with a more explicit strategy to test that leader election is working, but I'm still thinking about that.

solo-changelog-bot · 2022-08-11T17:48:45Z

Issues linked to changelog:
#5795

test/kube2e/util.go

kdorosh · 2022-08-12T17:29:29Z

projects/gloo/pkg/syncer/translator_syncer.go

+	if s.identity.IsLeader() {
+		// Only leaders will write reports


i would think only leaders would do translation etc too? otherwise we may use a bunch of CPU / RAM for no reason? also a bunch of checks like this all over the codebase will be very very hard to maintain rather than a single check at the top level that disables everything else until elected as leader

should we even be serving validation requests either? do we really want to have watches duplicated everywhere (can we stress out or require k8s or dependencies to scale?) simultaneously? i thought most of the value of having the follower ready was to ensure that the pod was ready / was already scheduled

open to more discussion here; there are some valid points against but i think it will be easier and perhaps better to just short circuit everything at the top level

Being able to scale validation seems pretty nice but the point about watches is a good one it might even be behavior one could opt into. That being said if the watches arent in place is the pod really ready to go?

that's a fair point; probably good to have the watches ready and warmed before being marked as ready and thus available for failover. i may just be paranoid; depends on how much strain gloo alone puts on the k8s apiserver itself and thus scaling it could become an issue. might be nice to get input from the field here to determine if this is warranted / should be configurable behavior

Everyone should be doing translation and validation. Envoy/k8s will reach out to the service and the request will be Load Balanced, so all must be ready to serve the latest.

are we solving for scaling gloo, or just HA? I agree, if we want to scale gloo and load balance validation / xds requests we need to run translation (really everything but status reporting) on every pod (or have followers to talk leader to get state, similar to how zookeeper HA works). that said, i'm also nervous about scaling gloo given how many requests it can make, particularly in the consul case. I don't think having 3-5 gloo pods will be all that aggressive on the k8s apiserver because of the natural watch api, but for backends like consul we implement watches by querying for every single endpoint individually this puts a lot of strain on the consul server, and scaling gloo to multiple replicas could cause problems there when we may just want to solve for HA / ensuring pods can be scheduled.

really feels like more of a product requirement decision here than an engineering one to me. is the customer concerned about just scheduling pods? or about scaling gloo itself (is CPU / RAM pegged?)

also i don't know enough about the leader election libraries used here, but have we reasoned about what happens in error scenarios? usually in failure there can be (briefly) two or zero leaders..

It's possible for there to be 2 (fencing not implemented by library) or 0 (leader dies and there is a delay before a new elector is chosen). In both cases, the status reporting is affected, but will not cause issues pushing configuration to Envoy.
given that, I think we're ok in both scenarios

pkg/bootstrap/leaderelector/election_factory.go

projects/gloo/pkg/api/converters/kube/artifact_converter.go

kdorosh · 2022-08-15T16:44:59Z

before we merge i'd recommend reading https://aws.amazon.com/builders-library/leader-election-in-distributed-systems/ and following best practices here, in particular:

add metrics for leaders
reason about lease expiration, especially in the case of garbage collection (which is a real concern for us as a cpu intensive application)

sam-heilbron · 2022-08-15T17:03:17Z

before we merge i'd recommend reading https://aws.amazon.com/builders-library/leader-election-in-distributed-systems/ and following best practices here, in particular:

add metrics for leaders

reason about lease expiration, especially in the case of garbage collection (which is a real concern for us as a cpu intensive application)

Good call. I had looked too quickly and seen that the library exposes metrics, but the default is a noop. I'll update to include metrics.

Do you think lease expiration should just be configurable given the differences based on users environments?

kdorosh · 2022-08-15T17:22:25Z

I think the defaults are sane; i don't think we will hit multi second issues unless we have deadlock, kernel errors, almost total network failure.. the kinds of things that should result in a new leader regardless

test/kube2e/gateway/gateway_test.go

nfuden · 2022-08-15T19:23:52Z

I am content with current state.

* add leaderelector module * use single replica as placeholder * delete unused * kill gloo on lost leadership * add support in e2e testss * include metrics, skip statuses * set the leader election factory based on the settings * fix bad test * add kube lease rbac * update rbac and tests * fix rbac" " * generated code * set 2 replicas in proper place * skip config map with leader election annotation * move go.mod * add changelog * codegen * bad comment * udate consistent state check to allow for more jitter, which occurs during startup * Adding changelog file to new location * Deleting changelog file from old location * make metrics check more consistent * move leader election to setup so it runs once * preoprly quit on lost leadership, update comment to reflect * go mod tidy * fix kube2e setupfunc * bump timeout for proxy creation * print debug logs on gateway kube2e test failure * if leader election could not be started, error * different prefail handler * realseoNcancel * add informative description to expect * remove ReleaseOnCancel * fix timeout * add metrics provider * change metric name * go mod tidy * codegen * increase status timeout Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com> Co-authored-by: changelog-bot <changelog-bot>

* Introduce Leader Election for Gloo (#6926) * add leaderelector module * use single replica as placeholder * delete unused * kill gloo on lost leadership * add support in e2e testss * include metrics, skip statuses * set the leader election factory based on the settings * fix bad test * add kube lease rbac * update rbac and tests * fix rbac" " * generated code * set 2 replicas in proper place * skip config map with leader election annotation * move go.mod * add changelog * codegen * bad comment * udate consistent state check to allow for more jitter, which occurs during startup * Adding changelog file to new location * Deleting changelog file from old location * make metrics check more consistent * move leader election to setup so it runs once * preoprly quit on lost leadership, update comment to reflect * go mod tidy * fix kube2e setupfunc * bump timeout for proxy creation * print debug logs on gateway kube2e test failure * if leader election could not be started, error * different prefail handler * realseoNcancel * add informative description to expect * remove ReleaseOnCancel * fix timeout * add metrics provider * change metric name * go mod tidy * codegen * increase status timeout Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com> Co-authored-by: changelog-bot <changelog-bot> * fix(tls_inspector): added AggregateListener support "envoy.filters.listener.tls_inspector" is an essential listener_filter when attempting to use TLS. For _most_ gateways, we would previously always consider logic on whether or not to add it based on the presence of a user-provided SslConfig field. For the new _AggregateListener_, we neglected to add support, originally. This caused TLS to be unable to function when using the "isolateVirtualHostsBySslConfig" feature flag. related #6677 * changelog: condense * leader: Attempt to see if leader lease time is the breaking part * Revert "fix(tls_inspector): added AggregateListener support" This reverts commit 278187d. * Revert "Revert "fix(tls_inspector): added AggregateListener support"" This reverts commit 4d38e2b. Co-authored-by: Sam Heilbron <[email protected]> Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com> Co-authored-by: Gunnar <[email protected]>

timflannagan · 2024-08-20T22:19:41Z

pkg/bootstrap/leaderelector/kube/metrics.go

@@ -0,0 +1,37 @@
+package kube


@sam-heilbron Do you remember why we manually vendored this file from the c-r or client-go repository?

sam-heilbron added 5 commits August 10, 2022 13:41

add leaderelector module

1698f4d

use single replica as placeholder

280bce3

Merge branch 'master' into ha/part-3

04f851d

delete unused

e27daf2

kill gloo on lost leadership

68dfeb7

github-actions bot added the keep pr updated signals bulldozer to keep pr up to date with base branch label Aug 10, 2022

add support in e2e testss

a1e5592

EItanya reviewed Aug 10, 2022

View reviewed changes

projects/gloo/pkg/syncer/translator_syncer.go Outdated Show resolved Hide resolved

nfuden reviewed Aug 10, 2022

View reviewed changes

pkg/bootstrap/leaderelector/election_factory.go Show resolved Hide resolved

projects/gloo/pkg/syncer/setup/setup_syncer.go Outdated Show resolved Hide resolved

jackstine reviewed Aug 10, 2022

View reviewed changes

pkg/bootstrap/leaderelector/kube/factory.go Outdated Show resolved Hide resolved

soloio-bulldozer bot and others added 3 commits August 10, 2022 19:46

Merge refs/heads/master into ha/part-3

a0cb0af

include metrics, skip statuses

46847ed

Merge branch 'ha/part-3' of ssh:https://github.com/solo-io/gloo into ha/part-3

b63e47d

sam-heilbron marked this pull request as ready for review August 11, 2022 02:56

sam-heilbron added 3 commits August 10, 2022 23:11

set the leader election factory based on the settings

ffcf61e

fix bad test

0eaf098

add kube lease rbac

86ccd6f

sam-heilbron added 4 commits August 11, 2022 00:32

update rbac and tests

52b75e1

fix rbac"

23ced0f

"

generated code

f7eebdd

set 2 replicas in proper place

03db68d

sam-heilbron requested a review from a team as a code owner August 11, 2022 05:25

jenshu reviewed Aug 11, 2022

View reviewed changes

sam-heilbron added 3 commits August 11, 2022 13:40

skip config map with leader election annotation

d29c6c6

move go.mod

0af932f

add changelog

05fcb65

sam-heilbron changed the title ~~[DNM] Introduce Leader Election for Gloo~~ Introduce Leader Election for Gloo Aug 11, 2022

sam-heilbron requested a review from jackstine August 11, 2022 17:53

elcasteel reviewed Aug 12, 2022

View reviewed changes

test/kube2e/util.go Show resolved Hide resolved

sam-heilbron requested a review from elcasteel August 12, 2022 16:30

add informative description to expect

7f046f1

kdorosh reviewed Aug 12, 2022

View reviewed changes

EItanya reviewed Aug 12, 2022

View reviewed changes

pkg/bootstrap/leaderelector/election_factory.go Outdated Show resolved Hide resolved

EItanya reviewed Aug 12, 2022

View reviewed changes

projects/gloo/pkg/api/converters/kube/artifact_converter.go Show resolved Hide resolved

soloio-bulldozer bot and others added 4 commits August 12, 2022 20:39

Merge refs/heads/master into ha/part-3

334d0a2

remove ReleaseOnCancel

9c96bc9

Merge branch 'ha/part-3' of ssh:https://github.com/solo-io/gloo into ha/part-3

1a30001

fix timeout

feb97ef

sam-heilbron requested review from kdorosh and EItanya August 15, 2022 14:03

Merge refs/heads/master into ha/part-3

1764f48

sam-heilbron added 6 commits August 15, 2022 13:27

add metrics provider

a34d6f0

Merge branch 'ha/part-3' of ssh:https://github.com/solo-io/gloo into ha/part-3

5b02526

change metric name

bef6270

go mod tidy

7abdd3b

codegen

c152281

increase status timeout

81a3ac5

nfuden reviewed Aug 15, 2022

View reviewed changes

test/kube2e/gateway/gateway_test.go Show resolved Hide resolved

nfuden approved these changes Aug 15, 2022

View reviewed changes

soloio-bulldozer bot merged commit dd3ac2d into master Aug 15, 2022

soloio-bulldozer bot deleted the ha/part-3 branch August 15, 2022 19:57

timflannagan reviewed Aug 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce Leader Election for Gloo #6926

Introduce Leader Election for Gloo #6926

sam-heilbron commented Aug 10, 2022 •

edited

Loading

github-actions bot commented Aug 11, 2022 •

edited

Loading

jenshu Aug 11, 2022

sam-heilbron Aug 11, 2022

jenshu Aug 11, 2022

sam-heilbron Aug 11, 2022

jenshu Aug 11, 2022

sam-heilbron Aug 11, 2022

jenshu Aug 11, 2022

sam-heilbron Aug 11, 2022

solo-changelog-bot bot commented Aug 11, 2022

kdorosh Aug 12, 2022

kdorosh Aug 12, 2022

nfuden Aug 12, 2022

kdorosh Aug 12, 2022

EItanya Aug 12, 2022

kdorosh Aug 15, 2022 •

edited

Loading

kdorosh Aug 15, 2022

sam-heilbron Aug 15, 2022 •

edited

Loading

kdorosh commented Aug 15, 2022

sam-heilbron commented Aug 15, 2022

kdorosh commented Aug 15, 2022

nfuden commented Aug 15, 2022

timflannagan Aug 20, 2024

		if s.identity.IsLeader() {
		// Only leaders will write reports

Introduce Leader Election for Gloo #6926

Introduce Leader Election for Gloo #6926

Conversation

sam-heilbron commented Aug 10, 2022 • edited Loading

Description

Context

Solution

Technical Debt

Follow Up Work

Enterprise Follow-Up

Checklist:

github-actions bot commented Aug 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

solo-changelog-bot bot commented Aug 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdorosh Aug 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sam-heilbron Aug 15, 2022 • edited Loading

Choose a reason for hiding this comment

kdorosh commented Aug 15, 2022

sam-heilbron commented Aug 15, 2022

kdorosh commented Aug 15, 2022

nfuden commented Aug 15, 2022

Choose a reason for hiding this comment

sam-heilbron commented Aug 10, 2022 •

edited

Loading

github-actions bot commented Aug 11, 2022 •

edited

Loading

kdorosh Aug 15, 2022 •

edited

Loading

sam-heilbron Aug 15, 2022 •

edited

Loading