Store NVMe firmware metadata in inventory collection #6275
Merged
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
papertigers
force-pushed
the
mike/nvme-firmware-crdb
branch
from
5d7df4b to
ded63b4
Compare
|
Example output from the updated omdb command: |
papertigers
commented
Aug 12, 2024
papertigers
commented
Aug 13, 2024
smklein
reviewed
Aug 27, 2024
papertigers
force-pushed
the
mike/nvme-firmware-crdb
branch
from
5c8bf92 to
3e59254
Compare
papertigers
commented
Sep 25, 2024
Comment on lines
+1012
to
1034
| pub fn current_version(&self) -> Option<&str> { | ||
| match self.active_slot.0 { | ||
| // be paranoid that we have a value within the NVMe spec | ||
| slot @ 1..=7 => self | ||
| .slot_firmware_versions | ||
| .get(usize::from(slot) - 1) | ||
| .and_then(|v| v.as_deref()), | ||
| _ => None, | ||
| } | ||
| } | ||
|
|
||
| /// Attempt to read the staged firmware version that will be active upon | ||
| /// next device reset. | ||
| pub fn next_version(&self) -> Option<&str> { | ||
| match self.next_active_slot { | ||
| // be paranoid that we have a value within the NVMe spec | ||
| Some(slot) if slot.0 <= 7 && slot.0 >= 1 => self | ||
| .slot_firmware_versions | ||
| .get(usize::from(slot.0) - 1) | ||
| .and_then(|v| v.as_deref()), | ||
| _ => None, | ||
| } | ||
| } |
There was a problem hiding this comment.
These checks should be unnecessary now that the only way to construct a InvNvmeDiskFirmware is through the "new" method, but I kind of like the extra safety net. Thoughts on converting this to Result<&str, InvNvmeDiskFirmwareError> and Result<Option<&str>, InvNvmeDiskFirmwareError>
There was a problem hiding this comment.
I think an Option is fine -- I'm not sure we'd yet want a caller to treat a Result::Err case differently from None anyway.
smklein
approved these changes
Sep 25, 2024
| @@ -1551,3 +1740,218 @@ impl InvOmicronZoneNic { | |||
| zone_nic.into_network_interface_for_zone(zone_id) | |||
| } | |||
| } | |||
|
|
|||
| #[cfg(test)] | |||
There was a problem hiding this comment.
Great tests, thanks for adding these!
Comment on lines
+1012
to
1034
| pub fn current_version(&self) -> Option<&str> { | ||
| match self.active_slot.0 { | ||
| // be paranoid that we have a value within the NVMe spec | ||
| slot @ 1..=7 => self | ||
| .slot_firmware_versions | ||
| .get(usize::from(slot) - 1) | ||
| .and_then(|v| v.as_deref()), | ||
| _ => None, | ||
| } | ||
| } | ||
|
|
||
| /// Attempt to read the staged firmware version that will be active upon | ||
| /// next device reset. | ||
| pub fn next_version(&self) -> Option<&str> { | ||
| match self.next_active_slot { | ||
| // be paranoid that we have a value within the NVMe spec | ||
| Some(slot) if slot.0 <= 7 && slot.0 >= 1 => self | ||
| .slot_firmware_versions | ||
| .get(usize::from(slot.0) - 1) | ||
| .and_then(|v| v.as_deref()), | ||
| _ => None, | ||
| } | ||
| } |
There was a problem hiding this comment.
I think an Option is fine -- I'm not sure we'd yet want a caller to treat a Result::Err case differently from None anyway.
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.
With #5706 merged and later reenabled in #6026 we now want to store this data in CRDB so that it can be used by blueprints and reconfigurator.
nexus_types::inventory::PhysicalDisknow has afirmwarefield that is exposed as an enum calledPhysicalDiskFirmware. Today we only haveUnknownandNvmevariants but this can later be expanded upon if we ever start using additional disk types such as SCSI/SATA etc.Finally I have included an updated version of
omdb db inventory physical-disksthat will include the currently active firmware as well as the next active firmware if a new version has been staged.