underhill_core: keepalive cli arguments for nvme follow the "disabled,host,privatepool" pattern #2473
Conversation
|
This PR modifies files containing For more on why we check whole files, instead of just diffs, check out the Rustonomicon |
| Ok(v) => v, | ||
| Err(e) => { | ||
| tracing::warn!( | ||
| "failed to parse OPENHCL_NVME_KEEP_ALIVE ('{s}'): {e}. Nvme keepalive will be disabled." |
There was a problem hiding this comment.
NOTE: Decided to keep this here as opposed to defaulting to disabled in parse. That way we can more accurately log which variable parsing is failing (nvme vs mana)
gurasinghMS
changed the title
There was a problem hiding this comment.
Pull request overview
This PR updates NVMe keepalive configuration to use a verbose pattern-based approach (matching the existing MANA keepalive implementation) instead of a simple boolean, providing better visibility into why keepalive is enabled or disabled.
Key Changes:
- Changed
nvme_keep_alivefrombooltoKeepAliveConfigenum across the codebase - Updated command line generation in
openhcl_bootto produce verbose patterns like "disabled,host,privatepool" - Added parsing support for the "disabled,*" pattern to handle manual override cases
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| openhcl/underhill_core/src/worker.rs | Changed nvme_keep_alive field type from bool to KeepAliveConfig and updated usage to call .is_enabled() |
| openhcl/underhill_core/src/options.rs | Added documentation for NVMe keepalive patterns, updated parsing to handle "disabled,*" patterns, improved error messages |
| openhcl/underhill_core/src/emuplat/netvsp.rs | Reorganized import of KeepAliveConfig to follow better ordering |
| openhcl/underhill_core/src/dispatch/mod.rs | Updated all references to use KeepAliveConfig type, replaced boolean flag with config object, added .is_enabled() calls, improved logging |
| openhcl/openhcl_boot/src/main.rs | Rewrote NVMe keepalive command line generation to produce verbose "disabled,host,privatepool" style patterns instead of simple "1" flag |
| write!(cmdline, "privatepool ")?; | ||
| } else { | ||
| write!(cmdline, "noprivatepool ")?; | ||
| } |
There was a problem hiding this comment.
The space after "privatepool" and "noprivatepool" will become part of the environment variable value, causing parsing to fail. The FromStr implementation for KeepAliveConfig (line 93 in options.rs) does not trim whitespace before parsing.
Move the space outside the environment variable value:
if vtl2_pool_supported {
write!(cmdline, "privatepool")?;
} else {
write!(cmdline, "noprivatepool")?;
}
write!(cmdline, " ")?;| write!(cmdline, "privatepool ")?; | |
| } else { | |
| write!(cmdline, "noprivatepool ")?; | |
| } | |
| write!(cmdline, "privatepool")?; | |
| } else { | |
| write!(cmdline, "noprivatepool")?; | |
| } | |
| write!(cmdline, " ")?; |
| write!(cmdline, "disabled,")?; | ||
| } | ||
|
|
||
| if partition_info.nvme_keepalive { | ||
| write!(cmdline, "host,")?; | ||
| } else { | ||
| write!(cmdline, "nohost,")?; | ||
| } | ||
|
|
||
| if vtl2_pool_supported { | ||
| write!(cmdline, "privatepool ")?; | ||
| } else { | ||
| write!(cmdline, "noprivatepool ")?; |
There was a problem hiding this comment.
When disable_nvme_keep_alive is false, the command line will not include the "disabled," prefix. However, when it's true, the pattern becomes "disabled,host,privatepool" or "disabled,nohost,noprivatepool", etc.
The FromStr implementation at line 97 handles strings starting with "disabled," but doesn't have explicit matches for patterns like "disabled,host,privatepool". While the catch-all at line 97 will map these to Disabled, this means the host and privatepool information is ignored even though it's being generated.
Consider either:
- Not generating the host/privatepool parts when disabled is true (since they're ignored)
- Or adding explicit parsing for these patterns if the information is meant to be preserved for logging/debugging
| write!(cmdline, "disabled,")?; | |
| } | |
| if partition_info.nvme_keepalive { | |
| write!(cmdline, "host,")?; | |
| } else { | |
| write!(cmdline, "nohost,")?; | |
| } | |
| if vtl2_pool_supported { | |
| write!(cmdline, "privatepool ")?; | |
| } else { | |
| write!(cmdline, "noprivatepool ")?; | |
| write!(cmdline, "disabled ")?; | |
| } else { | |
| if partition_info.nvme_keepalive { | |
| write!(cmdline, "host,")?; | |
| } else { | |
| write!(cmdline, "nohost,")?; | |
| } | |
| if vtl2_pool_supported { | |
| write!(cmdline, "privatepool ")?; | |
| } else { | |
| write!(cmdline, "noprivatepool ")?; | |
| } |
There was a problem hiding this comment.
Ignoring the host and private pool information when disable has explicitly been passed in should be fine because that's exactly what the "disable" is. it is an override for everything else. So I don't think this needs to be changed.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| "host,privatepool" => Ok(KeepAliveConfig::EnabledHostAndPrivatePoolPresent), | ||
| "nohost,privatepool" => Ok(KeepAliveConfig::DisabledHostAndPrivatePoolPresent), | ||
| "nohost,noprivatepool" => Ok(KeepAliveConfig::Disabled), | ||
| x if x.starts_with("disabled,") => Ok(KeepAliveConfig::Disabled), |
There was a problem hiding this comment.
I don't think we should add this disabled,.. variant. Instead, I think we should add a host,noprivatepool variant to the parsing that maps to a new enum variant KeepAliveConfig::EnabledHostAndPrivatePoolMissing that maps to the scenario where the host supports keepalive but it's been explicitly disabled.
4 participants
Nvme keepalive will now use more verbose language for cli updates sent to the user mode. This will give us visibility in to why nvme keepalive is being enabled or disabled.