dynamic host volumes: serialize ops per volume

[gulducat]

Let only one of any create/register/delete run at a time per volume ID.

• DHV plugins can assume that Nomad will not run concurrent operations for the same volume
• We avoid interleaving client RPCs with raft writes

[tgross]

I was a little worried this was going to be tough to do without either a coarse-grained mutex or a hot loop, but your implementation elegantly avoids both problems. Nice work on this so far.

[tgross]

LGTM!

[gulducat]

I refactored it (again), and while it's still convoluted, it does actually test the serialization.

instead of these mock client methods blocking on a receive, they block on a send. then unblockCurrent() unblocks them by receiving, and it returns what it got, i.e. whichever client RPC was being called at the time.

then the test code makes sure that matches which server RPC completed, and we can check the state as appropriate for the operation type.

aside from me thinking it works, I can tell it does because if I don't use the same volume ID as initial create, then stuff happens in random orders and there are chaotic test errors.

[tgross]

LGTM!

We can probably reuse this logic for tightening up the serialization of CSI operations too, so it's great that we're spending the effort to make this solid.

[tgross]

This will deadlock the test if we break while there are any pending funcs (i.e. if the first or second times out), because we'll never unblock the channel for the remaining functions but call funcs.Wait() after the loop breaks. A deadlocked test manifests as the whole test run blowing up in a panic after the 20min timeout, which is pretty awful to debug on GHA because it's not obvious which test is panicking without a lot of rummaging in the logs.

We could fix this by having a shutdown context on the mockVolumeClient. Just before we break the loop we can close that context. And then the send looks something like this:

	// block until something runs unblockCurrent()
	if bc := v.getBlockChan(); bc != nil {
		select {
			case bc <- "delete":
			case <-v.shutdownCtx.Done():
				v.setBlockChan(nil)
		}
	}

That would allow the funcs.Wait() to run to completion and close out the test. We'd still end up failing the test because of the t.Error here.

All that being said, I'll 👍 this PR as-is, as it'd be a bit of effort to fix and only matters in the case we break the test. Up to you.

[gulducat]

I thought I had caught all the deadlocks! I caused enough of them while writing this 😋 but you're totally right. I pushed another commit to cover it.

so it's great that we're spending the effort to make this solid.

thanks for saying so; I was worried I was deadlocked myself, spending too much time on it 😅

[tgross]

LGTM!