Add omdb,db,nexus api support for chicken switches #8449
Conversation
This is an initial commit to support chicken switches that allow runtime reconfigurator configuration. There is only one initial chicken switch which can be used to enable the planner background task to run. A follow up PR will be made to add a background task that reads the chicken switch DB table and makes it available to nexus via a watcher. This PR will complete #8253 . Support for listing history was added at the datastore level, but not made available yet in OMDB, because of the way pagination works. This will likely come in a follow up.
nexus/db-queries/src/db/datastore/reconfigurator_chicken_switches.rs
Outdated
Show resolved
Hide resolved
nexus/db-queries/src/db/datastore/reconfigurator_chicken_switches.rs
Outdated
Show resolved
Hide resolved
| .await | ||
| } | ||
|
|
||
| async fn reconfigurator_chicken_switches_show( |
There was a problem hiding this comment.
Do we need to fetch individual settings by version number? I know we definitely want:
- show the latest
- show the history
but I'm not sure why I'd want to fetch a particular not-the-latest config by its version number. Maaaaybe something like "show me what the config was at timestamp X"? But looking at the history is probably going to be more common for any kind of historical debugging like that, I'd assume (based on how we've used the history of blueprints).
There was a problem hiding this comment.
I almost got rid of this before opening the PR, but left it for one reason only. I figured that an operator might want to view the previous configuration, which could be done easily enough with this API. The history is likely going to be under omdb reconfigurator, and it doesn't exist yet, so figured this might be worth it.
I can remove this easily enough though if it really doesn't make sense.
There was a problem hiding this comment.
My comment was definitely still through the lens of "do we need client-supplied versions at all". No objection to keeping this around; it might be useful, and it seems easy to remove it later if we find that the history is enough (or remove it now and add it later if we realize the history is not enough, if that's your preference).
There was a problem hiding this comment.
I'm going to keep it for now, but may remove it later, once I implement history.
jgallagher
left a comment
jgallagher
left a comment
There was a problem hiding this comment.
Looks great! Excited for the followup.
| opctx: &OpContext, | ||
| switches: &ReconfiguratorChickenSwitches, | ||
| ) -> Result<usize, Error> { | ||
| assert!(switches.version >= 1); |
There was a problem hiding this comment.
Ok, you've convinced me that version needs to be supplied by the client. But that means this needs to return a bad_request error instead of panicking, I think? I realize omdb will never be evil here, but we shouldn't panic if we get a request with version=0.
There was a problem hiding this comment.
This method is private to the module and only called from reconfigurator_chicken_switches_insert_latest_version where we do return an error in this case.
Funny enough, I also had the same thought for a fleeting second last night and then realized this method was private. Maybe I should change the name to insert_latest_version_internal or something.
There was a problem hiding this comment.
As a result of a chat with @jgallagher, I moved the check from the public method to the private method and renamed the private method in 9ac5fe8
| .await | ||
| } | ||
|
|
||
| async fn reconfigurator_chicken_switches_show( |
There was a problem hiding this comment.
My comment was definitely still through the lens of "do we need client-supplied versions at all". No objection to keeping this around; it might be useful, and it seems easy to remove it later if we find that the history is enough (or remove it now and add it later if we realize the history is not enough, if that's your preference).
This is an initial commit to support chicken switches that allow runtime reconfigurator configuration.
There is only one initial chicken switch which can be used to enable the planner background task to run.
A follow up PR will be made to add a background task that reads the chicken switch DB table and makes it available to nexus via a watcher. This PR will complete #8253 .
Support for listing history was added at the datastore level, but not made available yet in OMDB, because of the way pagination works. This will likely come in a follow up.