[inventory] Add full OmicronSledConfig and fields for upcoming config reconciler#8188
Merged
jgallagher merged 25 commits intomainfrom May 22, 2025
Merged
Conversation
Contributor
Author
|
Just went through an upgrade to this branch on dublin. After the mupdate, while Nexus is blocked waiting for schema migrations, we had a few collections from before the update: After performing the schema migration, all of them were removed (as expected). About 2 minutes later, the first post-upgrade collection showed up. (I think from watching Nexus logs, most of that 2 minutes was in Nexus's backoff waiting for the schema migration.) This is what the |
sunshowers
reviewed
May 20, 2025
Contributor
sunshowers
left a comment
sunshowers
left a comment
There was a problem hiding this comment.
(partway through reviewing)
| pub remove_mupdate_override: Option<MupdateOverrideUuid>, | ||
| } | ||
|
|
||
| impl Default for OmicronSledConfig { |
Contributor
There was a problem hiding this comment.
Thoughts on deriving this?
Contributor
Author
There was a problem hiding this comment.
Hmm, that would require Generation to impl Default, which it currently does not. I can't think of a reason why it shouldn't, but will ask around.
Contributor
Author
There was a problem hiding this comment.
This generated a lot of discussion. 😅 Generation not implementing Default is intentional, so I'll leave this as-is.
| } | ||
| } | ||
|
|
||
| // Insert rows for the all the sled configs we found. |
Contributor
There was a problem hiding this comment.
deeply admire your tenacity
sunshowers
approved these changes
May 20, 2025
Contributor
sunshowers
left a comment
sunshowers
left a comment
There was a problem hiding this comment.
This is absolutely incredible work.
Accepting modulo a few comments.
| @@ -0,0 +1,2 @@ | |||
| ALTER TABLE omicron.public.inv_sled_agent | |||
| ADD COLUMN IF NOT EXISTS reconciler_status_kind inv_config_reconciler_status_kind NOT NULL; | |||
Contributor
There was a problem hiding this comment.
Does this need to set a default value (not-yet-run?) in this migration, and then remove the default in a future one?
Contributor
Author
There was a problem hiding this comment.
I was worried it might, but it doesn't, because we've explicitly removed all the rows from inv_sled_agent in up01.sql.
| @@ -0,0 +1,13 @@ | |||
| CREATE TABLE IF NOT EXISTS omicron.public.inv_omicron_sled_config_zone_nic ( | |||
Contributor
There was a problem hiding this comment.
I guess these are just copied from inv_omicron_zone_nic etc?
Contributor
Author
There was a problem hiding this comment.
Yes: a few of these "new" tables are really the old table with a new column, except that new column is part of the primary key. I think it's easier to reason about the migration as "drop the table, recreate the new table" instead of having to deal with migrating the primary key indices.
…g-reconciler-inventory-integration
…g-reconciler-inventory-integration
jgallagher
deleted the
john/sled-agent-config-reconciler-inventory-integration
branch
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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 primary change here is replacing these inventory fields (a subset of
OmicronSledConfig):with these:
Once #8064 lands, all three of these will be filled in meaningfully; as of this PR, only
ledgered_sled_configis populated. (reconciler_statusis alwaysNotYetRunandlast_reconciliationis alwaysNone, since there is no reconciler yet.) The rest of the changes are all fallout from changing inventory:omdbprintingledgered_sled_configfor now, but will need to be updated on [sled-agent] Integrate config-reconciler #8064 once other fields are populatedomicron_zonesto a fullOmicronSledConfig. The first few schema migrations take care of this.Before merging I'll go through an upgrade on a racklette and confirm things come back up okay after the schema migration blows away all the pre-update inventory collections. (We think this is fine, but it'd be good to confirm.) But I think this is close enough that it's reviewable.
Couple other minor changes that came along for the ride:
inv_sled_omicron_zonesis gone now)image_sourcecolumns to the inventory zone config table, so we don't loseImageSource::Artifact { hash }values reported by sled-agent)