Do not 500 on snapshot delete for deleted region #1162
Merged
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
The code path that validates a snapshot delete request checks that a
region exists, and returns 404 if it does not:
match rc.context().get(&p.id) {
Some(_) => (),
None => {
return Err(HttpError::for_not_found(
None,
format!("region {:?} not found", p.id),
));
}
}
However there's a flaw in this check: if the region existed but was
deleted, this will not return a 404, and proceed into `delete_snapshot`.
`delete_snapshot` will check if a running snapshot exists, and if it
does will bail out: it's an invalid request to delete a snapshot for
which there's a read-only downstairs running for.
However, the very next thing the code does is try to make a ZFSDataset
object out of the region's data path. If the region was deleted this
will lead to `zfs list` exiting with a non-zero return code, and will
bubble up to the user as a 500.
Catch ZFSDataset returning Err, and if the region existed but was
deleted, we can assume the snapshot was deleted prior to this (as the
agent would not allow otherwise). If the region existed and isn't
deleted, and `zfs list` for that region path failed, then the agent's
datafile doesn't match reality.
leftwo
reviewed
Feb 15, 2024
leftwo
approved these changes
Feb 15, 2024
| @@ -95,7 +98,9 @@ impl ZFSDataset { | |||
| .output()?; | |||
|
|
|||
| if !cmd.status.success() { | |||
| bail!("zfs list failed!"); | |||
| let stderr = | |||
| String::from_utf8_lossy(&cmd.stderr).trim_end().to_string(); | |||
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.
The code path that validates a snapshot delete request checks that a region exists, and returns 404 if it does not:
However there's a flaw in this check: if the region existed but was deleted, this will not return a 404, and proceed into
delete_snapshot.delete_snapshotwill check if a running snapshot exists, and if it does will bail out: it's an invalid request to delete a snapshot for which there's a read-only downstairs running for.However, the very next thing the code does is try to make a ZFSDataset object out of the region's data path. If the region was deleted this will lead to
zfs listexiting with a non-zero return code, and will bubble up to the user as a 500.Catch ZFSDataset returning Err, and if the region existed but was deleted, we can assume the snapshot was deleted prior to this (as the agent would not allow otherwise). If the region existed and isn't deleted, and
zfs listfor that region path failed, then the agent's datafile doesn't match reality.