populate SMBIOS system version with Git metadata #702
Conversation
Currently, the SMBIOS system (type 1) table has a hard-coded version field. It would be nice if this was instead populated with details about the Propolis version, as described in issue #701. This branch adds a `build.rs`` script that uses [the `vergen` crate][1] to emit information about the Git revision that Propolis was build from. Now, we can generate a version string that describes the git branch, commit hash, and commit depth, as described in [this comment][2]. This is generated in a `propolis::version()` function, which also includes the Bhyve API version (detected at runtime). This results in version strings like: ``` Propolis v0.1.0-658 (0d8efa1) eliza/somebios-version, <unknown Bhyve API version> ``` (on a Linux machine; the Bhyve version would be present on illumos) In addition to populating the SMBIOS system version, this commit also sets the Clap version for CLI commands, so that the `--version` flag will print out the same value that's set in SMBIOS. [1]: https://docs.rs/vergen [2]: #701 (comment)
|
Version in SMBIOS tables aside for now, I like having this available via just |
|
Since this shouldn't actually result in differing versions after live migration (per @pfmooney's comment in #701 (comment)), I'm marking this as ready for review. |
| } | ||
| match bhyve_api::api_version() { | ||
| Ok(v) => { | ||
| write!(version, "Bhyve API v{v}") |
There was a problem hiding this comment.
With everything else in the version being fixed at compile time, do we want to include runtime data (the bhyve api version) in the string?
There was a problem hiding this comment.
I'm happy to remove it if you think that's better --- it would certainly make this code much simpler!
| } | ||
| match bhyve_api::api_version() { | ||
| Ok(v) => { | ||
| write!(version, "Bhyve API v{v}") |
There was a problem hiding this comment.
nit: bhyve should not be capitalized
| manufacturer: SmbString::default(), | ||
| product_name: SmbString::default(), | ||
| version: SmbString::try_from(crate::version()) | ||
| .expect("version string should not contain NULs"), |
There was a problem hiding this comment.
If we remove the bhyve API version from the string, we could include this condition as part of the version() unit tests
|
So I'm not sure if we want this going in SMBIOS per se. I get the benefit as a developer, but I'm less sold from a customer perspective. What are the semantics of the version that they should use? Are we committing to the format? This leaks information about the host version in a much more concrete way and I'll admit I expected this to be something about the virtual machine itself or something about the overall system as opposed to the virtualization part of it. For example:
I think if we want to say something about propolis itself, then this would be where we want the actual hardware platform revision to go in as this seems much more about the virtual platform. |
Presently, the BIOS version string in Propolis' SMBIOS tables is hardcoded to a default value. It would be nice to instead use the OVMF version for the BIOS version string. Because Propolis' understanding of bootroms is just a path on the filesystem to some kind of file, it's not aware of the OVMF version, or, indeed, that the bootrom even *is* OVMF (and it could conceivably be anything). Therefore, the bootrom version must be provided externally, such as by the Oxide control plane in the case of `propolis-server`, or by the user when running standalone. This PR adds a config field `bootrom_version` to the TOML config files for `propolis-server` and `propolis-standalone` which can be used to provide a value for the bootrom version string. If the version string is not provided, Propolis will continue to use the current default values. I considered changing the config format to move the `bootrom` path field and `bootrom_version` string field into a `bootrom` table, as in: ```toml [bootrom] path = "/path/to/OVMF_CODE.fd" version = "edk2-stable202402" ``` However, this would break existing configs, and I don't think it's so much nicer than ```toml bootrom = "/path/to/OVMF_CODE.fd" bootrom_version = "edk2-stable202402" ```` to justify breakage. I'm happy to change the format if others disagree. Along with #702, this branch implements the changes described in #701.
Presently, the BIOS version string in Propolis' SMBIOS tables is hardcoded to a default value. It would be nice to instead use the OVMF version for the BIOS version string. Because Propolis' understanding of bootroms is just a path on the filesystem to some kind of file, it's not aware of the OVMF version, or, indeed, that the bootrom even *is* OVMF (and it could conceivably be anything). Therefore, the bootrom version must be provided externally, such as by the Oxide control plane in the case of `propolis-server`, or by the user when running standalone. This PR adds a config field `bootrom_version` to the TOML config files for `propolis-server` and `propolis-standalone` which can be used to provide a value for the bootrom version string. If the version string is not provided, Propolis will continue to use the current default values. I considered changing the config format to move the `bootrom` path field and `bootrom_version` string field into a `bootrom` table, as in: ```toml [bootrom] path = "/path/to/OVMF_CODE.fd" version = "edk2-stable202402" ``` However, this would break existing configs, and I don't think it's so much nicer than ```toml bootrom = "/path/to/OVMF_CODE.fd" bootrom_version = "edk2-stable202402" ```` to justify breakage. I'm happy to change the format if others disagree. Along with #702, this branch implements the changes described in #701.
No particular use case beyond just tracking whether we wanted to hardcode it at all. I do think @rmustacc's point about identifying the "Virtual Platform" revision would make more sense. Either way, there's clearly a couple ways to go here. Thanks for putting this together so quickly @hawkw and I think if we drop the SMBIOS bits it's still worth merging just to have the |
|
I'll go ahead and back out the SMBIOS changes, I agree that it's nice to get commit information in the CLI version output regardless of what we do about SMBIOS. And I think including the bhyve version we were built against, rather than the current version detected at runtime, seems like a good call. |
Currently, the SMBIOS system (type 1) table has a hard-coded version field. It would be nice if this was instead populated with details about the Propolis version, as described in issue #701.
This branch adds a
build.rsscript that uses thevergencrate to emit information about the Git revision that Propolis was build from. Now, we can generate a version string that describes the git branch, commit hash, and commit depth, as described in this comment. This is generated in apropolis::version()function, which also includes the Bhyve API version (detected at runtime). This results in version strings like:(on a Linux machine; the Bhyve version would be present on illumos)
In addition to populating the SMBIOS system version, this commit also sets the Clap version for CLI commands, so that the
--versionflag will print out the same value that's set in SMBIOS.