[nexus] add sled provision state #4520
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
Created using spr 1.3.5
sunshowers
changed the title
Created using spr 1.3.5
Created using spr 1.3.5
Created using spr 1.3.5
sunshowers
changed the title
sunshowers
changed the title
smklein
reviewed
Nov 28, 2023
There was a problem hiding this comment.
Looks great! I have a smattering of comments, but my big two are:
- We gotta do a minor rename to make the schema change work
- The transactional semantics of altering the
provisionablestate need some tweaking to make the return value consistent -- but I have a small essay discussing some of our options there
Created using spr 1.3.5
Created using spr 1.3.5
smklein
reviewed
Nov 29, 2023
smklein
reviewed
Nov 29, 2023
smklein
approved these changes
Nov 29, 2023
Created using spr 1.3.5
3 tasks
Created using spr 1.3.5
Created using spr 1.3.5
sunshowers
deleted the
sunshowers/spr/nexus-add-tracking-for-provision-state
branch
ahl
reviewed
Dec 4, 2023
Comment on lines
+13178
to
+13183
| "description": "This is a state that isn't known yet.\n\nThis is defined to avoid API breakage.", | ||
| "type": "string", | ||
| "enum": [ | ||
| "unknown" | ||
| ] | ||
| } |
There was a problem hiding this comment.
Is this how you intended this state to appear in the OpenAPI doc? I'm not clear on how this will prevent API breakage.
There was a problem hiding this comment.
Addressed in #4608
sunshowers
added a commit
that referenced
this pull request
Dec 8, 2023
…4573) While developing #4520, I observed that we were producing a number of error messages that were: * 503 Service Unavailable, * With only an internal message attached * But where the message is both safe and useful to display to clients. This is my attempt to make the situation slightly better. To achieve this, I made a few changes: 1. Make all the client errors carry a new `MessagePair` struct, which consists of an external message. (Along the way, correct the definition of e.g. the `Conflict` variant: it actually is an external message, not an internal one.) 2. Define a new `InsufficientCapacity` variant that consists of both an external and an internal message. This variant resolves to a 507 Insufficient Storage error, and has a more helpful message than just "Service Unavailable". 3. Turn some current 503 errors into client errors so that the message is available externally. Looking for feedback on this approach!
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.
Add the notion of a sled provision state to Nexus. Currently, we will only use this to prevent new resources and regions from being provisioned to sleds.
This PR includes:
nexus-db-queries.Questions:
TODO list
TODO:
db::model::Sledshould track provision state -- it seems like it is created by sled_agent API calls, which don't know the provision state. Butsled_listshould return the provision state. Do we need to split up the type into two, one for creation and one for listing? Are there existing patterns for this?db::model::Sledhas alast_used_addressparameter that is assigned here but not used insled_upsert. This seems maybe a bit dodgy?Fixes (maybe partially) #2483.