Backport of CSI: persist previous mounts on client to restore during restart into release/1.3.x #17878
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hc-github-team-nomad-core
force-pushed
the
backport/csi-agent-restart-13028/accurately-optimum-monkfish
branch
2 times, most recently
from
25a59e9
to
de5c624
Compare
|
|
) When claiming a CSI volume, we need to ensure the CSI node plugin is running before we send any CSI RPCs. This extends even to the controller publish RPC because it requires the storage provider's "external node ID" for the client. This primarily impacts client restarts but also is a problem if the node plugin exits (and fingerprints) while the allocation that needs a CSI volume claim is being placed. Unfortunately there's no mapping of volume to plugin ID available in the jobspec, so we don't have enough information to wait on plugins until we either get the volume from the server or retrieve the plugin ID from data we've persisted on the client. If we always require getting the volume from the server before making the claim, a client restart for disconnected clients will cause all the allocations that need CSI volumes to fail. Even while connected, checking in with the server to verify the volume's plugin before trying to make a claim RPC is inherently racy, so we'll leave that case as-is and it will fail the claim if the node plugin needed to support a newly-placed allocation is flapping such that the node fingerprint is changing. This changeset persists a minimum subset of data about the volume and its plugin in the client state DB, and retrieves that data during the CSI hook's prerun to avoid re-claiming and remounting the volume unnecessarily. This changeset also updates the RPC handler to use the external node ID from the claim whenever it is available. Fixes: #13028
tgross
force-pushed
the
backport/csi-agent-restart-13028/accurately-optimum-monkfish
branch
from
f8165d8
to
33ce2c6
Compare
tgross
deleted the
backport/csi-agent-restart-13028/accurately-optimum-monkfish
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Backport
This PR is auto-generated from #17840 to be assessed for backporting due to the inclusion of the label backport/1.3.x.
WARNING automatic cherry-pick of commits failed. Commits will require human attention.
The below text is copied from the body of the original PR.
When claiming a CSI volume, we need to ensure the CSI node plugin is running before we send any CSI RPCs. This extends even to the controller publish RPC because it requires the storage provider's "external node ID" for the client. This primarily impacts client restarts because the client's initial fingerprint won't include restored plugins.
Unfortunately there's no mapping of volume to plugin ID available in the jobspec, so we don't have enough information to wait on plugins until we either get the volume from the server or retrieve the plugin ID from data we've persisted on the client.
If we always require getting the volume from the server before making the claim, a client restart for disconnected clients will cause all the allocations that need CSI volumes to fail. Even while connected, checking in with the server to verify the volume's plugin before trying to make a claim RPC is inherently racy, so we'll leave that case as-is and it will fail the claim if the node plugin needed to support a newly-placed allocation is flapping such that the node fingerprint is changing.
So instead of blocking for information from the server, this changeset persists a minimum subset of data about the volume and its plugin in the client state DB, and retrieves that data during the CSI hook's prerun to avoid re-claiming and remounting the volume unnecessarily. This fixes the client restart bug but also reduces Nomad RPC and CSI RPC traffic during client restarts.
This changeset also updates the RPC handler to use the external node ID from the claim whenever it is available.
Fixes: #13028
Note for reviewers: this looks like a bit of a monster of a PR because of all the plumbing required for adding a new client state DB object. The bulk of the non-plumbing changes are in
client/allocrunner/csi_hook.go, which GitHub is helpfully folding. 😀 An unfortunate amount of the churn in that file is having to break up theclaimVolumefunction a bit, but I think the top-level flow inPrerunis a lot more legible as a result. In addition to expanding the test coverage in the unit test, I've also end-to-end tested this with the AWS EBS plugin and restarting the client in various configurations. (Also note this will not do anything for #15415)