Restart external-attacher faster by releasing leader election lease on sigterm

[rhrmo]

What type of PR is this?
/kind bug

What this PR does / why we need it:
This PR makes external-attacher restart faster when it is terminating because of sigterm. This is accomplished by releasing leader election lease on sigterm and then it does not have to wait for lease to expire but it proceeds with restart right away. This is the first PR of series of PRs that fix #609

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:
This PR is the first of the series of PRs that fix #609 and I want to use it to agree on a solution - if you have any suggestions, or find anything wrong, please add a comment and let me know.
I had to import newer version of csi-lib-utils library, so that it would contain changes from kubernetes-csi/csi-lib-utils#191 (go get github.com/kubernetes-csi/csi-lib-utils@f77445f75e9c4954062cdddcef4863adab21947b), so before merging this we would also need to have new version of csi-lib-utils released.

Does this PR introduce a user-facing change?:

NONE

[k8s-ci-robot]

Hi @rhrmo. Thanks for your PR.

I'm waiting for a kubernetes-csi member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[jsafrane]

/ok-to-test

[jsafrane]

I tested in on a cluster.

What's great is that when the attacher gets SIGTERM, it exits immediately (this PR does not change that) + release its lease, so a newly started attacher starts immediately. This is taken when SIGTERM was received in the middle of ControllerPublish:

I0310 15:57:31.102801  133591 connection.go:264] "GRPC call" VolumeAttachment="csi-043731379aa11361179f9e22a17f1fda8d2b0716696126267433bf49892e92b2" method="/csi.v1.Controller/ControllerPublishVolume" request="{\"node_id\":\"192.168.124.90\",\"volume_capability\":{\"access_mode\":{\"mode\":\"SINGLE_NODE_MULTI_WRITER\"},\"mount\":{}},\"volume_context\":{\"storage.kubernetes.io/csiProvisionerIdentity\":\"1741618771172-2394-hostpath.csi.k8s.io\"},\"volume_id\":\"5a4de638-fdc0-11ef-b375-5ed7a0a04918\"}"

[waiting for ControllerPublishVolumeResponse]
^C

I0310 15:57:32.373544  133591 main.go:292] Received SIGTERM or SIGINT signal, shutting down controller.
I0310 15:57:32.373594  133591 controller.go:151] "Shutting CSI attacher"
I0310 15:57:32.373682  133591 reflector.go:319] Stopping reflector *v1.CSINode (10m0s) from k8s.io/client-go/informers/factory.go:160
I0310 15:57:32.373719  133591 reflector.go:319] Stopping reflector *v1.PersistentVolume (10m0s) from k8s.io/client-go/informers/factory.go:160
I0310 15:57:32.373748  133591 reflector.go:319] Stopping reflector *v1.VolumeAttachment (10m0s) from k8s.io/client-go/informers/factory.go:160
I0310 15:57:32.373719  133591 connection.go:270] "GRPC response" VolumeAttachment="csi-043731379aa11361179f9e22a17f1fda8d2b0716696126267433bf49892e92b2" response="{}" err="rpc error: code = Canceled desc = context canceled"
I0310 15:57:32.373761  133591 csi_handler.go:611] "Saving attach error" VolumeAttachment="csi-043731379aa11361179f9e22a17f1fda8d2b0716696126267433bf49892e92b2"
I0310 15:57:32.374421  133591 csi_handler.go:267] "Failed to save attach error to VolumeAttachment" VolumeAttachment="csi-043731379aa11361179f9e22a17f1fda8d2b0716696126267433bf49892e92b2" err="client rate limiter Wait returned an error: context canceled"
I0310 15:57:32.374448  133591 csi_handler.go:243] "Error processing" VolumeAttachment="csi-043731379aa11361179f9e22a17f1fda8d2b0716696126267433bf49892e92b2" err="failed to attach: rpc error: code = Canceled desc = context canceled"
E0310 15:57:32.376055  133591 event.go:464] "Unable to record event (will not retry!)" err="broadcaster already stopped"
E0310 15:57:32.376066  133591 leader_election.go:198] "Stopped leading"

What's not so great is that the CSI driver does not know about any of this. It got ControllerPublishVolume, it is still attaching the volume and eventually it may succeed. But at the same time, another external-attacher might get the lease very quickly and call the same ControllerPublishVolume in parallel.

This could happen also today, however, before this PR, the new attacher would need to wait for the old attacher lease to expire (15 seconds in the default config). Which could give the old CSI driver enough time either complete the attachment or die (because it's unlikely that the attacher got SIGTERM and the CSI driver in the same Pod did not).

[msau42]

What's not so great is that the CSI driver does not know about any of this. It got ControllerPublishVolume, it is still attaching the volume and eventually it may succeed. But at the same time, another external-attacher might get the lease very quickly and call the same ControllerPublishVolume in parallel.

Instead of shutting down immediately on a sigterm, can we wait until all ongoing requests are finished? It would probably require changes to our reconciler.

[jsafrane]

Instead of shutting down immediately on a sigterm, can we wait until all ongoing requests are finished? It would probably require changes to our reconciler.

We would need two contexts then and it might get messy which context should be used when.

@rhrmo can you explore this idea? I think the only place that should unblock when a signal is received would be here (to call ShutDown on all workqueues, which will then wait for existing work to finish):

external-attacher/pkg/controller/controller.go

Line 150 in fbf2dba

<-ctx.Done()

And here to finish periodic reconcile:

external-attacher/pkg/controller/controller.go

Line 142 in fbf2dba

go wait.UntilWithContext(ctx, func(ctx context.Context) {

Maybe you find other places. But it will be different in each sidecar.

[rhrmo]

Sure thing!

[jsafrane]

What's not so great is that the CSI driver does not know about any of this. It got ControllerPublishVolume, it is still attaching the volume and eventually it may succeed. But at the same time, another external-attacher might get the lease very quickly and call the same ControllerPublishVolume in parallel.

Instead of shutting down immediately on a sigterm, can we wait until all ongoing requests are finished? It would probably require changes to our reconciler.

I can see there is a code that waits for workers to finish before exiting, still, they should finish with a lot of context canceled errors, because it's the main context that gets cancelled.

[rhrmo]

/retest-required

[jsafrane]

Just adding few comments for my future self:

Suggested change

	if utilfeature.DefaultFeatureGate.Enabled(features.ReleaseLeaderElectionOnExit) {
	var wg sync.WaitGroup
	stopCh := shutdownHandler
	factory.Start(stopCh)
	ctrl.Run(controllerCtx, int(*workerThreads), &wg)
	if signalReceived {
	wg.Wait()
	terminate()
	}
	if utilfeature.DefaultFeatureGate.Enabled(features.ReleaseLeaderElectionOnExit) {
	var wg sync.WaitGroup
	stopCh := shutdownHandler
	factory.Start(stopCh)
	ctrl.Run(controllerCtx, int(*workerThreads), &wg)
	if signalReceived {
	// Wait for the controllers to finish
	wg.Wait()
	// Finish the overall context and release leader election.
	terminate()
	}

[jsafrane]

Can Run() above finish without a signal received? Assuming it could, what would be the right handling? IMO releasing the leader election + exit (i.e. terminate()) would be correct in all cases where Run() finishes.

[jsafrane]

I.e. IMO we could remove the whole variable signalReceived

[rhrmo]

I think Run() should be able to finish without signal but I am not entirely sure now that you ask, I will test it. I agree with releasing the leader election + exit after Run() finishes.

[rhrmo]

I removed the whole signalReceived variable, testing Run() is bit tricky though because it waits for the context to be cancelled before it returns, by trying to call return at it's end instead <-ctx.Done() it finished without any issues.

[jsafrane]

/lgtm

[jsafrane]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsafrane, rhrmo

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [jsafrane]

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