[nicolaskagami]

Fixes: #11549

Context

An operator completes a multirack join that triggers a request via nexus to sled-agent during rack adoption. Between the time the setup status was queried and that completion request is sent, the operator on the wicket side can upload a new RackNetworkConfig and bump the generation number of the bootstore. This race is why we can't just return the generation number in the status request with the other information.

When the multirack join service receives the completion request it atomically stops allowing updates to the bootstore. We should therefore return the latest generation as the response to this request. However, since the the response can get lost we want to make this idempotent in case of retries. Therefore we'll need to memoize this value in the RssStatus::Completed field.

What does this PR change?

• multirack-join-complete returns the latest bootstore_generation written by the join service. Repeated requests are idempotent.
• multirack-join-complete waits for the join service to finish, so Nexus can definitively know the bootstore generation was correctly written. This may also be useful in the future to signal other stuff to Nexus. However, we may not want to wait (which may include finishing start_sled_agents).
• Sled-agent assigns the bootstore generation when it accepts a join request, under the status lock. This means the generation numbers written to the bootstore can skip, which is fine.
• Found a race where the multirack-join-complete could arrive while a new configuration had been accepted but not written yet. Decided with @andrewjstone to deny the join-complete request and apply the accepted config. Returns 409 MultirackJoinPendingConfigUpdate in that case and Nexus should retry later.
• After sled-agent restarts, multirack-join-complete returns 409 MultirackJoinGenerationUnavailable, since the generation is only kept in memory. Multirack Join Completion response should return the bootstore generation number #11549 mentions this is out of scope.
• Older API versions (56-62) redirect to the same implementation (ignoring the actual gen number), so now they also wait and can get the 409s. I could make them behave like before (not waiting) but it doesn't seem worth the extra machinery.

Details

• Tested with 🤖 help on a single-rack Voxel deployment that ran the multirack join instead of RSS. Checked happy path, idempotency and the two new errors.

[andrewjstone]

Great stuff. Love the tests.

[nickelization]

Neat trick using shared to allow polling this in multiple places at once, I've actually never encountered this before!

[nicolaskagami]

Thanks!

[nickelization]

Why send_replace in these tests instead of just send? Since we're not looking at the return value, I think effectively the only difference is that unlike send it won't fail if there are no receivers, but it seems like that would be unexpected enough that maybe such an error should trigger a test failure.

[nicolaskagami]

I was using send_replace in tests simply because that's what the prod code is doing (where we don't want to fail if the receivers are dead). But you're right, that should not be true in these tests so it's just a tighter test. Fix incoming.

[nickelization]

I'm wondering if this sync lock becomes dangerous now that this is being changed to an async function that can block waiting for the multirack join result - do we need a manual drop call after the match?

Or...maybe it's fine because we're matching on the result of the lock() call and it goes out of scope at the end of the match block, before we call wait_for_multirack_join_result? I can't remember off the top of my head exactly how the drop rules work in this kind of situation. Even if this is actually fine though I'm thinking we might want to put in a comment to warn anyone who modifies this code in the future.

[nicolaskagami]

Yeah. I wasn't super happy with the visibility here, but it's definitely safe because the lock would be dropped before. I'll change it to something clearer.

[nicolaskagami]

Ah, now I remember why I did it this way. I can't have status declared outside the result_rx statement like it was before because it would make the future !Send, even if I drop it before the yield point (rust-lang/rust#57478). And if I put it inside the block I'd have to re-indent the whole thing (which I don't like much for visibility). I think it's own function would be overkill as it would leave complete_multirack_join basically empty. Anyway, I prefer just adding a comment on how it's safe.

[nickelization]

The uses of now_or_never in these tests do make me a little nervous. It feels like it's possible these tests could be racy, because it's not easy to tell if the futures are guaranteed to resolve or if the tests just happen to work because of the way the timing (usually?) lines up. Is this just being done to time out quickly if something goes wrong? I wonder if using more conventional timeouts might be safer.

[nicolaskagami]

No. Actually, these are strictly more reliable than with timeouts. If you look at how complete_multirack_join works you'll see that every use of it in tests is deterministic. That should be obvious for the Some cases, but the None cases also assert that they haven't failed yet (and poll once, which makes them ready to proceed). I think the only problem with it is that it's slightly implementation-gnostic.

[nickelization]

Great stuff!