Saga pantry attach/detach: fail if pantry is removed from DNS #6866
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
Removal from DNS should correspond with the pantry zone's expungement, which should result in a saga node failure.
jgallagher
added a commit
that referenced
this pull request
Oct 14, 2024
This builds on #6866. After its changes, there was only caller of `retry_until_known_result` left; this PR removes it. We keep the retry loop, but instead of retrying for ever, we bail out if the sled we're trying to reach is "gone", as determined by "is it no longer in-service", which in practice means it's been expunged.
jmpesp
approved these changes
Oct 15, 2024
Comment on lines
+1287
to
+1292
| // Importantly, _do not use `call_pantry_attach_for_disk`_! That retries | ||
| // as long as `pantry_address` is still resolvable in DNS, which we _do not | ||
| // want here_. The Pantry attach can fail if there's a racing Volume | ||
| // checkout to be sent to Propolis. Additionally, that call uses `attach` | ||
| // instead of `attach_activate_background`, which means it will hang on the | ||
| // activation. |
There was a problem hiding this comment.
I would update this message to only
Importantly, _do not use `call_pantry_attach_for_disk`_! That call uses `attach` instead of `attach_activate_background`, which means it will hang on the activation.
At some point, some sort of retry should be added here, but that shouldn't block this PR.
davepacheco
approved these changes
Oct 16, 2024
| Ok(entries) => entries, | ||
| Err(err) => { | ||
| warn!( | ||
| log, "Failed to resolve Crucible pantry in DNS"; |
There was a problem hiding this comment.
Suggested change
| log, "Failed to resolve Crucible pantry in DNS"; | |
| log, "Failed to query DNS for Crucible pantry"; |
super nitty but sometimes "failed to resolve X in DNS" means "I successfully reached DNS and X wasn't there", which isn't the case here
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.
Removal from DNS should correspond with the pantry zone's expungement, which should result in a saga node failure.
I'm marking this as a draft because it's completely untested. @jmpesp any suggestions for testing? I skipped over the Nexus integration tests for a test to expand or emulate and didn't see anything in this area, but definitely could've missed it (they are legion!). I could construct a local unit test of the attach/detach functions, but it would require a fair bit of setup machinery (a fake crucible pantry, modifying the DNS records, ...). Maybe worth it if there's no easy way to test this?