CSI: improve controller RPC reliability

[tgross]

The CSI specification says that we "SHOULD" send no more than one in-flight request per volume at a time, with an allowance for losing state (ex. leadership transitions) which the plugins "SHOULD" handle gracefully. We mostly successfully serialize node and controller RPCs for the same volume, except when Nomad clients are lost. (See also container-storage-interface/spec#512)

These concurrency requirements in the spec fall short because Storage Provider APIs aren't necessarily safe to call concurrently on the same host even for different volumes. For example, concurrently attaching AWS EBS volumes to an EC2 instance results in a race for device names, which results in failure to attach (because the device name is taken already and the API call fails) and confused results when releasing claims. So in practice many CSI plugins rely on k8s-specific sidecars for serializing storage provider API calls globally. As a result, we have to be much more conservative about concurrency in Nomad than the spec allows.

This changeset includes four major changes to fix this:

• Add a serializer method to the CSI volume RPC handler. When the RPC handler makes a destructive CSI Controller RPC, we send the RPC thru this serializer and only one RPC is sent at a time. Any other RPCs in flight will block.
• Ensure that requests go to the same controller plugin instance whenever possible by sorting by lowest client ID out of the plugin instances.
• Ensure that requests go to healthy plugin instances only.
• Ensure that requests for controllers can go to a controller on any live node, not just ones eligible for scheduling (which CSI controllers don't care about)

Fixes: #15415

[tgross]

I've done some end-to-end testing of this new function with reschedules but I want to follow-up with some additional testing of drains, broken plugin configurations, etc., to make sure that everything is recoverable even in the corner cases.

[gulducat]

Looks good! I think it'll cover a lot of our bases, and should also make it easier to debug any future issues.

Mainly though I'm not clear on some math in the test (which makes me feel a little not-smart, since it seems like basic multiplication?), so either something to fix there or I have a facepalm in my future.

[gulducat]

I think we're good, but sharing here some of my thought process: For this to work, all such RPCs need to be run on a single server to block in memory. Looking at what I think are the relevant RPCs, it seems like they are all(?) v.srv.forward()ed (to the leader). I suppose this is what you meant by "with an allowance for losing state (ex. leadership transitions)" - i.e. this serialization could be broken by switching leaders? and we can't reasonably avoid that caveat.

[tgross]

Right, that's exactly it.

[gulducat]

I find myself wanting simpler map thread-safety, and a sync.Map seems just about right instead of a manual mutex, but its auto-locking-for-you behavior might not be compatible with your concern here?

[tgross]

Yeah we're locking around both access to the map and the construction/cancellation of contexts. With a sync.Map that would end up looking like this:

// spin up a context goroutine whether we need it or not!
future, futureDone := context.WithCancel(v.srv.shutdownCtx)
defer futureDone()

for {
	ok := v.srv.volumeControllerFutures.CompareAndSwap(pluginID, nil, future)
	if ok {
		// success path
		err := fn()
		futureDone()
		v.srv.volumeControllerFutures.CompareAndDelete(pluginID, future)
		return err
	} else {
		// wait path

		// racy! what if it's been deleted?
		waitOn, ok := v.srv.volumeControllerFutures.Load(pluginID) 
		if !ok {
			continue // lost the race with the delete, start again
		}
		select {
			case <-waitOn.Done():
				continue // other goroutine is done
			case <-v.srv.shutdownCh:
				return structs.ErrNoLeader
		}
	}
}

There are a few reasons why this isn't the best solution:

• A sync.Map doesn't let us compare-and-swap unless we've already constructed the object we're intending to write. So we end up awkwardly constructing the context outside the loop.
• In the "success path" we need to cancel the context before deleting it from the map, otherwise someone else can start their future before we've closed our context (and any downstream contexts we add in the future). But doing so will unblock the "wait path" before the CompareAndSwap will succeed, potentially leading to that goroutine trying to grab the CompareAndSwap multiple times before it can succeed, which is wasteful.
• CompareAndSwap doesn't return the existing value, so we need to Load it the value and it may have been deleted in the meanwhile. This adds more contention around that lock.
• The CompareAndSwap operation returns false when it hasn't been previously set! We could probably hack around that by creating a dummy context that's always treated as "unused" but then we've spun up a goroutine per plugin just to hold that spot.

[gulducat]

Nice, thanks for the mock-up and detailed explanation!

[gulducat]

Worth wrapping this with any more info about where it came from, or does that happen upstream of here?

[tgross]

The caller's logger will wrap it sufficiently. "No leader" is fairly foundational and there's nothing we can meaningfully add.

[gulducat]

I see the test passes, but how does 2 * 50 milliseconds add up to > 150 ? "plugin1" should only be blocked behind itself, so "plugin2"'s 50ms shouldn't be included, right?

[tgross]

For plugin1: the goroutine on line 2002 runs for 50ms. The goroutine on line 2004 waits at least 50ms for the other to finish and then runs for 50ms, for a total of 100ms. We're summing up the elapsed time of all the goroutines for a given plugin, because that way we're asserting the wait time of the method under test from the perspective of each goroutine and not just the elapsed wall clock time. So it's a minimum of 50ms + 100ms.

The maximum value of 200ms is assuming that any overhead is (far) less than 50ms, so that asserts that we're not accidentally waiting behind plugin2 as well.

[tgross]

In addition to the tests added above, I've done some end-to-end testing here:

• client restart on a node that has a job that mounts a volume and a controller + node plugin
• rescheduling a job that mounts a volume
• draining a node with a job that mounts a volume and a controller plugin on it