-
Notifications
You must be signed in to change notification settings - Fork 40
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
sled-agent needs to deal with SSD overprovisioning #5158
Conversation
/// The desired disk size for dealing with overprovisioning. | ||
size: Option<u32>, | ||
/// An override for the default 4k LBA formatting. | ||
lba_data_size_override: Option<u64>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing uses this today, so I am open to removing it from this PR in favor of not having dead code.
I have tested this on my bench gimlet and observed the proper behavior. I will run through that test one more time and paste the results on this PR. Currently the blocker for this is updating the buildiomat image (cc @jclulow) which means we will introduce a new omicron dependency on libnvme. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but I think I'd vote for a review from @smklein too since he's currently down in the weeds of physical disk management from Nexus.
|
||
fn preferred_nvme_device_settings( | ||
) -> &'static HashMap<&'static str, NvmeDeviceSettings> { | ||
PREFERRED_NVME_DEVICE_SETTINGS.get_or_init(|| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tiny nit: if having to do this dance is annoying, you could use https://docs.rs/once_cell/latest/once_cell/sync/struct.Lazy.html instead, so you could use PREFERRED_NVME_DEVICE_SETTINGS
directly instead of having to go through this helper. We use it elsewhere, and eventually it will land in the std lib.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't feel strongly about this one. I am okay leaving it as is if that works for you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, totally fine.
HashMap::from([ | ||
( | ||
"WUS4C6432DSP3X3", | ||
NvmeDeviceSettings { size: 3200, lba_data_size_override: None }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two thoughts, neither of which are necessarily relevant to landing this PR (and are kinda half baked; maybe @smklein has more thoughts here?):
- This list being built into sled-agent means adding support for more disks requires an OS update. Is that okay?
- If being baked into sled-agent is okay, do we want to define this in code, or would it make sense to put it in the sled-agent config.toml (which would require some work to pass it down into
sled-hardware
, admittedly).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those are good questions, @jgallagher.
Just to clarify, I think this means that if we want to support adopting a disk that needs non-default changes (i.e. something other than the 4K sector size) then we will need to do something. If we're going to move it at all, then we should probably make nexus own this list and tell sled-agent (not unreasonable per se). I think that'll fit better with the general firmware update and related flows that we've been discussing as well for disks. What I'd suggest is that for the moment we keep this here and then work to move it into nexus as part of the disk adoption path and make this something we can send down.
What I'd expect in terms of user interface flow over time is that we'd see something like:
- sled-agent detects a new disk and tells nexus.
- nexus will prompt the operator and if this is not a known / expected disk we will make the prompt to adopt very clear.
- At adoption time, nexus will send down instructions on how to transform the disk and things like resizing are things we may want to do in the future if we for example feel good undoing the overprovisioning.
- This may mean that a nexus update is required for us not to show the scarier unsupported disk warning and it may still require an OS update to fully update sled-agent on the transformations required.
I'm not sure it's worth an intermediate step to put it in the toml file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, I much prefer the nexus path laid out here to using the toml file. Thanks for the details @rmustacc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the helpful writeup @rmustacc. What you outlined here makes sense to me and likely ties into some of the same work that's going to happen with nvme firmware upgrades. I can keep this in the back of my mind as I start to figure out what that's going to look like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After #5172 merges, I'm hoping this pathway will get much easier, but I totally agree with Robert here. Expect that Nexus will end up sending a struct to Sled Agent, basically saying "Please use (and potentially format) these disks" -- and Sled Agent won't try writing to storage until that happens.
I'm okay with this being an explicit lookup table in Sled Agent until we get to that point. I don't think this PR needs to block on 5172
@@ -119,6 +191,11 @@ fn internal_ensure_partition_layout<GPT: gpt::LibEfiGpt>( | |||
}; | |||
match variant { | |||
DiskVariant::U2 => { | |||
// First we need to check that this disk is of the proper | |||
// size and correct logical block address formatting. | |||
ensure_size_and_formatting(log, identity)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly for my own education: what does this do with disks that already have data on them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't touch a disk with a label/zpool as changing the LBA format erases all data on the disk. This should only be preformed when StorageManger finds a new disk that has not been setup yet i.e first boot, add disk etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I expect that when we transform to an explicit adoption model then we'll want this to not occur until that adoption process is initiated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This only gets run if there is no GPT table. It's hidden by github above the diff.
@@ -154,13 +231,124 @@ fn internal_ensure_partition_layout<GPT: gpt::LibEfiGpt>( | |||
} | |||
} | |||
|
|||
fn ensure_size_and_formatting( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any problem running this over after failure on a reboot or restart of sled-agent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So while testing I found that if a disk is left in the unattached state then sled-agent will ignore it. That's due to these lines. So, if we were to detach blkdev from the disk and then take a sled-agent reboot we wouldn't be notified of that disk again I believe.
Now if we were in the process of formatting a disk and the box panicked or rebooted for some reason I don't know what would happen. Perhaps @rmustacc could shed some light on that failure scenario.
Regardless I don't know what the best course of action is for the first failure mode, maybe we need an enhancement in how we detect disks from devinfo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now if we were in the process of formatting a disk and the box panicked or rebooted for some reason I don't know what would happen. Perhaps @rmustacc could shed some light on that failure scenario.
There is nothing persistent about a blkdev attach/detach, so the kernel will attempt to if it can. Because we don't support namespace management and aren't doing anything here, this should likely be okay, though I can't comment on what happens if a device fails in the middle of say a format nvm command.
Regardless I don't know what the best course of action is for the first failure mode, maybe we need an enhancement in how we detect disks from devinfo?
I would suggest we file a follow up bug that moves this to looking for the NVMe node and not the blkdev node. This is for a few reasons:
- An NVMe device may not have any blkdev instances attached as mentioned to start this.
- An NVMe device may have the wrong number of namespaces created if it came from another system. This would show up as two distinct block devices or possibly no block devices, which would likely be confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filed this as #5241
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@papertigers Nice work. This looks good to me, although I'm mostly trusting you (and @rmustacc) on how the actual NVME stuff works. I think that's probably a safe bet :)
As long as the answer to my question is "it's fine", this LGTM.
I don't think this will affect what @smklein is doing as I believe this is at a much lower level. But it would be good to get a quick check.
Just wanted to dump some testing notes. SetupTwo of the new disks that Robert added to my bench gimlet for me.
I looped over them and configured them how we will likely see them in the wild from the manufacture:
Where LBA 0 looks like:
While I am here let us reset one of the deployed
Where LBA looks like:
1st pass sled-agentWhen we install omicron for the first time we expect to see the Install:
Check sled-agent's logs:
2nd pass sled-agentRemove omicron but don't modify the disks
Install omicron again and verify none of the disks are touched outside of zpool creation.
Ensure disks with existing pools are not touchedFollow the same cleanup steps from above. Then:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, with the caveats about how we can move some of this into Nexus -- but that doesn't need to be a blocker.
test-utils/src/dev/mod.rs
Outdated
|
||
/// Returns a DiskIdentity that can be passed to ensure_partition_layout when | ||
/// not operating on a real disk. | ||
pub fn mock_device_identity() -> DiskIdentity { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: disk_identity
- there are devices other than disks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in ea09e03
HashMap::from([ | ||
( | ||
"WUS4C6432DSP3X3", | ||
NvmeDeviceSettings { size: 3200, lba_data_size_override: None }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After #5172 merges, I'm hoping this pathway will get much easier, but I totally agree with Robert here. Expect that Nexus will end up sending a struct to Sled Agent, basically saying "Please use (and potentially format) these disks" -- and Sled Agent won't try writing to storage until that happens.
I'm okay with this being an explicit lookup table in Sled Agent until we get to that point. I don't think this PR needs to block on 5172
} else { | ||
info!( | ||
log, | ||
"There are no preferred NVMe settings for disk model {}; nothing to\ | ||
do for disk with serial {}", | ||
identity.model, | ||
identity.serial | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for handling this case -- this means that all the file-backed vdevs in testing will still work.
On main as of #5158, we unexpectedly get some binaries depending on `libnvme` that shouldn't. In release builds: ``` installinator omicron-dev omicron-package services-ledger-check-migrate sled-agent sled-agent-sim wicketd ``` and in debug builds, all of the above plus `omdb`. We don't really care about this for binaries that don't run on the rack, so stripping the list down to binaries we do care about: ``` installinator omdb (debug only) sled-agent wicketd ``` It's correct and expected that installinator and sled-agent depend on libnvme, but omdb shouldn't (and doesn't in release), and wicketd _must not_, as libnvme isn't available in the switch zone. This PR fixes that incorrect dependency by splitting the parts of the `sled-hardware` crate that wicketd and omdb depended on (directly or transitively) into a new `sled-hardware-types` crate. On this PR, we are left with only the following needing libnvme, in both debug and release: ``` installinator omicron-dev omicron-package services-ledger-check-migrate sled-agent sled-agent-sim ``` I assume with a bit more work we could trim out everything but `installinator` and `sled-agent`, and that might be worth doing, but we'd like to land this ASAP so as to not break any updates performed off of `main`. Separately, we could also imagine a CI check that we don't have unexpected library dependencies present in the final binaries; @papertigers is going to work on that.
After #5158 was integrated @Rain noticed that attempting to run a build of `omdb` in the switch zone suddenly stopped working and filed oxidecomputer/helios-omicron-brand#15. @jgallagher ended up fixing this by splitting out the sled-hardware types into their own crate in #5245. We decided it would be good if we added some sort of CI check to omicron to catch these library leakages earlier. This PR introduces that check and adds it to the helios build and test buildomat job. I have also added some notes to the readme for others that may end up adding a new library dependency. Locally I modified the allow list so that it would produce errors, those errors end up looking like: ``` $ cargo xtask verify-libraries Finished dev [unoptimized + debuginfo] target(s) in 0.42s Running `target/debug/xtask verify-libraries` Finished dev [unoptimized + debuginfo] target(s) in 4.11s Error: Found library issues with the following: installinator UNEXPECTED dependency on libipcc.so.1 omicron-dev UNEXPECTED dependency on libipcc.so.1 UNEXPECTED dependency on libresolv.so.2 sp-sim UNEXPECTED dependency on libipcc.so.1 UNEXPECTED dependency on libresolv.so.2 sled-agent NEEDS libnvme.so.1 but is not allowed mgs UNEXPECTED dependency on libipcc.so.1 UNEXPECTED dependency on libresolv.so.2 If depending on a new library was intended please add it to xtask.toml ```
This introduces the ability for omicron's storage manager to resize and format NVMe disks based upon a lookup table keyed off of model number to preferred settings.
Fixes #4619