Make leader election a top level pilot component, use it for singleton components by howardjohn · Pull Request #20809 · istio/istio

howardjohn · 2020-02-03T21:09:47Z

This PR does a few things, which I attempted to decompose into different commits

Add a new leaderelection component. This is largely a copy of the ingress leader election (which in turn is directly copied from some other implementation)
Switch ingress status to use the new component
Make validation and namespace controller use the new leader election

nmittler

was there previous leader election code in Istio? I was expecting to see moves/deletes

howardjohn · 2020-02-03T21:22:02Z

Yes, you can see it moved in ingress/status.go. The code is just calling a k8s library, so its not complex/long code or anything.

pilot/pkg/bootstrap/validation.go

ayj · 2020-02-03T21:47:23Z

pilot/pkg/leaderelection/leaderelection.go

Is there any chance of losing the lock without shutting down? Is this what line 100 below is referring to?

There is - at the very least if someone took away our rbac permission for config map, api server went down, etc we would. In this case, the context will be canceled, so the functions will stop (the library does this, not us).

I was confused looking at this original since I thought we just did nothing when we lose the lock, but its already handled. Probably warrants a comment.

The L100 comment is just for if WE explicitly cancel the context, we also drop the lock immediately. This allows another instance to take over immediately when we exit

nmittler

looks good overall ... took a first pass.

nmittler · 2020-02-03T21:54:03Z

pilot/pkg/leaderelection/leaderelection.go

I assume this will only be called before it's Run? If so, maybe we should just have a separate builder. Then we could do something like:

le := leaderelection.NewBuilder(). Namespace(ns). Name(name). Client(client). WithRun(runFn1). WithRun(runFn2). Build() ... le.Run(...)

@howardjohn there are a few options here:

lock access to the runFns array wherever it's used

Use a separate builder so that the API makes it clear that runFns can not be mutated after start.

Use a separate Config object that you pass to New... and this is the thing that you build up before you create the elector.

Personally, I think my preference is 3, it's probably the simplest since you could just have a simple struct with no additional methods, like a builder would require. It also makes the New method clean.

Also, this method should probably be renamed to something like RunWhenLeader.

nmittler · 2020-02-03T21:54:24Z

pilot/pkg/leaderelection/leaderelection.go

Any reason to use a stop channel as opposed to context.Context? Seems like it would simplify the cancel code below.

All of pilot is doing the stop channel passing. So at some point, we will need to convert stop channel into context? So I think its reasonable to do here rather than outside this function?

I feel like there should be a simpler way though..

nmittler · 2020-02-03T21:57:05Z

pilot/pkg/leaderelection/leaderelection.go

Make this a constant at the top?

Also, does this name need to be different for each component (e.g. webhook vs ingress)?

No, we will have a single leader election for all components. Technically we could split it up, but I see no reason to have 4+ locks when we can have one?

also I moved to a constant

pilot/pkg/leaderelection/leaderelection.go

howardjohn · 2020-02-04T00:55:14Z

/retest

hzxuzhonghu

Mostly i am concerned when leader lost, will it renew?

hzxuzhonghu · 2020-02-04T02:00:49Z

pilot/pkg/bootstrap/options.go

by default hostname equals podname.

@howardjohn looks like you're just using pod name for Identity in the election config? What are the requirements for this identity? Does it have to be a pod name?

pilot/pkg/bootstrap/validation.go

howardjohn · 2020-02-04T02:28:14Z

I think it will renew but I will check the library

howardjohn · 2020-02-04T02:52:13Z

Neither Istio's current implementation nor nginx-ingress do anything special to handle leader being lost: https://github.com/nginxinc/kubernetes-ingress/blob/5d4c5ddc70c2a40e78c803d0000908bfb2e6b63f/internal/k8s/controller.go#L310. So based on this I assume we have nothing to worry about here?

howardjohn · 2020-02-04T02:54:12Z

also looked at the code, it basically does

for {
  renew()
}

so if we lose it we can get it back again

hzxuzhonghu · 2020-02-04T03:07:10Z

I remember in kube-controller-manager, the OnStoppedLeading is set to panic and exit.

// Run starts the leader election loop
func (le *LeaderElector) Run(ctx context.Context) {
	defer func() {
		runtime.HandleCrash()
		le.config.Callbacks.OnStoppedLeading()
	}()
	if !le.acquire(ctx) {
		return // ctx signalled done
	}
	ctx, cancel := context.WithCancel(ctx)
	defer cancel()
	go le.config.Callbacks.OnStartedLeading(ctx)
	le.renew(ctx)
}

howardjohn · 2020-02-04T03:13:40Z

But for us we do nothing for OnStoppedLeading, so I don't think there is an issue?

hzxuzhonghu · 2020-02-04T03:20:56Z

We do nothing just print a log. But this goroutine will exit. There is no way to re-acquire the lock.

howardjohn · 2020-02-04T03:59:16Z

@hzxuzhonghu thanks for pushing back on this, i misunderstood the code. I simulated loosing the lock (by removing RBAC permission) and found

With ReleaseOnCancel, a panic from kubernetes (seems like a bug): leaderelection panic when failing to renew lease kubernetes/kubernetes#87800
Without release on cancel, no panic, but we don't ever renew

So I think you are right, we need some looping to acquire leadership again

howardjohn · 2020-02-05T23:16:15Z

/hold

I added a test that fails due to the k8s crashing bug

hzxuzhonghu · 2020-02-07T03:56:09Z

pilot/pkg/bootstrap/server.go

nmittler · 2020-02-07T16:03:59Z

pilot/pkg/bootstrap/options.go

@howardjohn looks like you're just using pod name for Identity in the election config? What are the requirements for this identity? Does it have to be a pod name?

nmittler · 2020-02-07T16:12:05Z

pilot/pkg/leaderelection/leaderelection.go

@howardjohn there are a few options here:

lock access to the runFns array wherever it's used

Use a separate builder so that the API makes it clear that runFns can not be mutated after start.

Use a separate Config object that you pass to New... and this is the thing that you build up before you create the elector.

Personally, I think my preference is 3, it's probably the simplest since you could just have a simple struct with no additional methods, like a builder would require. It also makes the New method clean.

Also, this method should probably be renamed to something like RunWhenLeader.

nmittler · 2020-02-07T16:15:37Z

pilot/pkg/leaderelection/leaderelection.go

nit: the name creates a stutter with the package name leaderelection.LeaderElection. Also, I think Elector is a little better than Election. Perhaps the package could be leader and the type could be Elector, so that callsites would see leader.Elector? WDYT?

istio-testing · 2020-02-07T17:14:36Z

In response to a cherrypick label: new pull request created: #20939

…n components (istio#20809) * Add initial leader election * Cleanup * Add unit tests * user leader election for ns controller, validation controller * Log about ingress * Add graceful shutdown * Fix tests, add injector controller * Fix lint * Add renewal when lease is droppped, failing test * wip * Get everything working * Fix build * lint

howardjohn requested a review from a team as a code owner February 3, 2020 21:09

googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Feb 3, 2020

istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 3, 2020

nmittler reviewed Feb 3, 2020

View reviewed changes

ayj reviewed Feb 3, 2020

View reviewed changes

pilot/pkg/bootstrap/validation.go Outdated Show resolved Hide resolved

ayj reviewed Feb 3, 2020

View reviewed changes

nmittler reviewed Feb 3, 2020

View reviewed changes

hzxuzhonghu reviewed Feb 4, 2020

View reviewed changes

howardjohn force-pushed the pilot/leader-election branch from 4163c65 to df198c2 Compare February 5, 2020 23:16

howardjohn requested a review from a team as a code owner February 5, 2020 23:16

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Feb 5, 2020

howardjohn force-pushed the pilot/leader-election branch from df198c2 to c3ceabd Compare February 6, 2020 02:10

istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Feb 6, 2020

howardjohn force-pushed the pilot/leader-election branch 2 times, most recently from 4801164 to be71e58 Compare February 6, 2020 23:28

hzxuzhonghu approved these changes Feb 7, 2020

View reviewed changes

pilot/pkg/bootstrap/server.go Outdated

Copy link
Copy Markdown

Member

hzxuzhonghu Feb 7, 2020

Choose a reason for hiding this comment

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

defer

howardjohn added 4 commits February 7, 2020 08:23

Add initial leader election

52dea04

Cleanup

21bb503

Add unit tests

b5a300c

user leader election for ns controller, validation controller

8317966

howardjohn added 9 commits February 7, 2020 08:23

Log about ingress

937360c

Add graceful shutdown

9d8c29e

Fix tests, add injector controller

200b0dc

Fix lint

29d3128

Add renewal when lease is droppped, failing test

168df6a

wip

9aed33f

Get everything working

a38562b

Fix build

70880f2

lint

a745085

howardjohn force-pushed the pilot/leader-election branch from 275bf86 to a745085 Compare February 7, 2020 16:23

howardjohn added the cherrypick/release-1.5 label Feb 7, 2020

nmittler reviewed Feb 7, 2020

View reviewed changes

istio-testing merged commit f8b56e9 into istio:master Feb 7, 2020

istio-testing mentioned this pull request Feb 7, 2020

[release-1.5] Make leader election a top level pilot component, use it for singleton components #20939

Merged

Conversation

howardjohn commented Feb 3, 2020

Uh oh!

nmittler left a comment

Choose a reason for hiding this comment

Uh oh!

howardjohn commented Feb 3, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nmittler left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

howardjohn commented Feb 4, 2020

Uh oh!

hzxuzhonghu left a comment

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!

Uh oh!

howardjohn commented Feb 4, 2020

Uh oh!

howardjohn commented Feb 4, 2020

Uh oh!

howardjohn commented Feb 4, 2020

Uh oh!

hzxuzhonghu commented Feb 4, 2020

Uh oh!

howardjohn commented Feb 4, 2020

Uh oh!

hzxuzhonghu commented Feb 4, 2020

Uh oh!

howardjohn commented Feb 4, 2020

Uh oh!

howardjohn commented Feb 5, 2020

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!

istio-testing commented Feb 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants