Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

populate SMBIOS system version with Git metadata #702

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented May 7, 2024

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 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 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.

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)
@luqmana
Copy link
Contributor

luqmana commented May 7, 2024

Version in SMBIOS tables aside for now, I like having this available via just --version. Great for double checking one-off binaries laying about!

lib/propolis/src/lib.rs Outdated Show resolved Hide resolved
@hawkw hawkw marked this pull request as ready for review May 7, 2024 18:49
@hawkw hawkw requested a review from pfmooney May 7, 2024 18:49
@hawkw
Copy link
Member Author

hawkw commented May 7, 2024

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}")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we remove the bhyve API version from the string, we could include this condition as part of the version() unit tests

@rmustacc
Copy link

rmustacc commented May 8, 2024

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:

  • qemu: Version: pc-i440fx-jammy
  • AWS Nitro doesn't use it at all
  • On both Gigabyte and Dell systems (AMD and Intel respectively) I see it as being the string 001/01.

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.

hawkw added a commit that referenced this pull request May 8, 2024
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.
@hawkw
Copy link
Member Author

hawkw commented May 8, 2024

@rmustacc I'm also not totally sure what the intended use-case of this is --- perhaps @luqmana (who opened issue #701 for adding this) has thoughts about why we would want to expose Propolis versions in SMBIOS?

hawkw added a commit that referenced this pull request May 8, 2024
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.
@luqmana
Copy link
Contributor

luqmana commented May 8, 2024

@rmustacc I'm also not totally sure what the intended use-case of this is --- perhaps @luqmana (who opened issue #701 for adding this) has thoughts about why we would want to expose Propolis versions in SMBIOS?

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 --version output. In which case, let's drop the bhyve api version from it as well. @pfmooney made the good point that that's not some fixed version tied to the binary itself. If anything, ApiVersion::current() would be more useful as quick way to figure out what a random propolis binary was built against without having to find the corresponding commit first.

@hawkw
Copy link
Member Author

hawkw commented May 8, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants