server: make clients parse config TOMLs #810
Open
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 migrate-from-base tests' unhappiness is expected here, since downlevel propolis-server won't grok the args uplevel PHD is passing it. (This is a case where even HTTP server API versioning wouldn't have saved us!) |
gjcolombo
force-pushed
the
gjcolombo/death-of-a-config-file
branch
from
360192d
to
986264a
Compare
hawkw
reviewed
Nov 18, 2024
There was a problem hiding this comment.
AFAICT, this code hasn't changed meaningfully, just moved --- but if there are changes here that are worth a closer look, let me know?
There was a problem hiding this comment.
Yeah, the logic in this and the old spec/config_toml.rs should be about the same. The main difference is that the new code generates a map of ComponentV0s instead of an internal-to-Propolis-server ParsedConfig type.
hawkw
approved these changes
Nov 18, 2024
Remove the propolis-server logic to add devices from the config TOML when fielding an instance ensure request. This logic never adds any devices in production anyway because the zone image's config TOML never specifies any. The config TOML does specify the path to the bootrom the server should load (and an optional version string to display to the guest). Supply these arguments on the command line instead of getting them from the TOML. Once this is done, the config TOML in the zone image is unnecessary, so remove it.
Now that config TOMLs no longer configure Propolis servers, rename the propolis-server-config crate to propolis-config-toml, then add code to convert a config TOML into a set of instance spec configuration options. Most of this code already existed in propolis-server's config TOML processor, which is now deleted. Also tweak a couple of field names in the SoftNPU port spec type to clarify that the name that a port has *within a SoftNPU setup* is distinct from its instance spec component key.
gjcolombo
force-pushed
the
gjcolombo/migration-failure-injection-spec
branch
from
0ac3bf1
to
15ad57c
Compare
gjcolombo
force-pushed
the
gjcolombo/death-of-a-config-file
branch
from
986264a
to
8c0f774
Compare
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.
Stacked on #809. Related to #804 and #806.
Remove config TOML processing from propolis-server. The only thing the config TOML is used for in Omicron is specifying a bootrom path (and possibly a version string); turn this into a propolis-server command-line argument instead. The instance spec-based ensure API already ignored the server's config TOML, and the non-spec ensure API is about to be removed anyway, so this is a good time to make this shift.
To avoid breaking existing config TOML users, move the TOML-to-spec processing logic into the propolis-config-toml crate, and have it output propolis-client's generated instance spec types. (This would have been a layering violation previously, but is OK now that propolis-server no longer uses this crate.) Teach propolis-cli to use this crate to process TOMLs and to use the instance spec-based ensure endpoint to create and migrate VMs.
Remove the config TOML from the Propolis zone image and change its command line invocation. Also change PHD's propolis-server invocations as needed.
Finally, tweak the propolis-server and propolis-cli READMEs to describe the new state of affairs.
Tests: cargo test, PHD, created and migrated ad-hoc VMs (including with propolis-cli's
--crucible-disksoption).