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

Configuring the VMM reservoir for dev setups could be easier. #4137

Merged
merged 4 commits into from
Nov 10, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions sled-agent/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,9 @@ pub struct Config {
pub sidecar_revision: SidecarRevision,
/// Optional percentage of DRAM to reserve for guest memory
pub vmm_reservoir_percentage: Option<u8>,
/// Optional DRAM to reserve for guest memory in MiB (mutually exclusive
/// option with vmm_reservoir_percentage).
pub vmm_reservoir_size_mb: Option<u32>,
/// Optional swap device size in GiB
pub swap_device_size_gb: Option<u32>,
/// Optional VLAN ID to be used for tagging guest VNICs.
Expand Down
78 changes: 56 additions & 22 deletions sled-agent/src/instance_manager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,13 +43,22 @@ pub enum Error {
#[error("Failed to create reservoir: {0}")]
Reservoir(#[from] vmm_reservoir::Error),

#[error("Invalid reservoir configuration: {0}")]
ReservoirConfig(String),

#[error("Cannot find data link: {0}")]
Underlay(#[from] sled_hardware::underlay::Error),

#[error("Zone bundle error")]
ZoneBundle(#[from] BundleError),
}

pub enum ReservoirMode {
None,
Size(u32),
Percentage(u8),
}

struct InstanceManagerInternal {
log: Logger,
nexus_client: NexusClientWithResolver,
Expand Down Expand Up @@ -108,44 +117,69 @@ impl InstanceManager {
})
}

/// Sets the VMM reservoir size to the requested (nonzero) percentage of
/// usable physical RAM, rounded down to nearest aligned size required by
/// the control plane.
/// Sets the VMM reservoir to the requested percentage of usable physical
/// RAM or to a size in MiB. Either mode will round down to the nearest
/// aligned size required by the control plane.
pub fn set_reservoir_size(
&self,
hardware: &sled_hardware::HardwareManager,
target_percent: u8,
mode: ReservoirMode,
) -> Result<(), Error> {
assert!(
target_percent > 0 && target_percent < 100,
"target_percent {} must be nonzero and < 100",
target_percent
);
let hardware_physical_ram_bytes = hardware.usable_physical_ram_bytes();
let req_bytes = match mode {
ReservoirMode::None => return Ok(()),
ReservoirMode::Size(mb) => {
let bytes = ByteCount::from_mebibytes_u32(mb).to_bytes();
if bytes > hardware_physical_ram_bytes {
return Err(Error::ReservoirConfig(format!(
"cannot specify a reservoir of {bytes} bytes when \
physical memory is {hardware_physical_ram_bytes} bytes",
)));
}
bytes
}
ReservoirMode::Percentage(percent) => {
if !matches!(percent, 1..=99) {
return Err(Error::ReservoirConfig(format!(
"VMM reservoir percentage of {} must be between 0 and \
100",
percent
)));
};
(hardware_physical_ram_bytes as f64 * (percent as f64 / 100.0))
.floor() as u64
}
};

let req_bytes = (hardware.usable_physical_ram_bytes() as f64
* (target_percent as f64 / 100.0))
.floor() as u64;
let req_bytes_aligned = vmm_reservoir::align_reservoir_size(req_bytes);

if req_bytes_aligned == 0 {
warn!(
self.inner.log,
"Requested reservoir size of {} bytes < minimum aligned size of {} bytes",
req_bytes, vmm_reservoir::RESERVOIR_SZ_ALIGN);
"Requested reservoir size of {} bytes < minimum aligned size \
of {} bytes",
req_bytes,
vmm_reservoir::RESERVOIR_SZ_ALIGN
);
return Ok(());
}

// The max ByteCount value is i64::MAX, which is ~8 million TiB. As this
// value is a percentage of DRAM, constructing this should always work.
// The max ByteCount value is i64::MAX, which is ~8 million TiB.
// As this value is either a percentage of DRAM or a size in MiB
// represented as a u32, constructing this should always work.
let reservoir_size = ByteCount::try_from(req_bytes_aligned).unwrap();
if let ReservoirMode::Percentage(percent) = mode {
info!(
self.inner.log,
"{}% of {} physical ram = {} bytes)",
percent,
hardware_physical_ram_bytes,
req_bytes,
);
}
info!(
self.inner.log,
"Setting reservoir size to {} bytes \
({}% of {} total = {} bytes requested)",
reservoir_size,
target_percent,
hardware.usable_physical_ram_bytes(),
req_bytes,
"Setting reservoir size to {reservoir_size} bytes"
);
vmm_reservoir::ReservoirControl::set(reservoir_size)?;

Expand Down
42 changes: 27 additions & 15 deletions sled-agent/src/sled_agent.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use crate::bootstrap::early_networking::{
};
use crate::bootstrap::params::StartSledAgentRequest;
use crate::config::Config;
use crate::instance_manager::InstanceManager;
use crate::instance_manager::{InstanceManager, ReservoirMode};
use crate::metrics::MetricsManager;
use crate::nexus::{NexusClientWithResolver, NexusRequestQueue};
use crate::params::{
Expand Down Expand Up @@ -387,21 +387,33 @@ impl SledAgent {
storage.zone_bundler().clone(),
)?;

match config.vmm_reservoir_percentage {
Some(sz) if sz > 0 && sz < 100 => {
instances.set_reservoir_size(&hardware, sz).map_err(|e| {
error!(log, "Failed to set VMM reservoir size: {e}");
e
})?;
}
Some(0) => {
warn!(log, "Not using VMM reservoir (size 0 bytes requested)");
}
None => {
warn!(log, "Not using VMM reservoir");
// Configure the VMM reservoir as either a percentage of DRAM or as an
// exact size in MiB.
let reservoir_mode = match (
config.vmm_reservoir_percentage,
config.vmm_reservoir_size_mb,
) {
(None, None) => ReservoirMode::None,
(Some(p), None) => ReservoirMode::Percentage(p),
(None, Some(mb)) => ReservoirMode::Size(mb),
(Some(_), Some(_)) => panic!(
"only one of vmm_reservoir_percentage and \
vmm_reservoir_size_mb is allowed"
),
};

match reservoir_mode {
ReservoirMode::None => warn!(log, "Not using VMM reservoir"),
ReservoirMode::Size(0) | ReservoirMode::Percentage(0) => {
warn!(log, "Not using VMM reservoir (size 0 bytes requested)")
}
Some(sz) => {
panic!("invalid requested VMM reservoir percentage: {}", sz);
Comment on lines -403 to -404
Copy link
Contributor

@jordanhendricks jordanhendricks Sep 25, 2023

Choose a reason for hiding this comment

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

This check was here to verify that the agent would panic if it was configured with a percentage > 100. It looks like this got moved into an error into the callee, which I think is fine, but it would be good to put testing bad values in the config in some testing notes for the PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked to see if there was a test that mocked some of this so that I could pass some values to the function but it doesn't look like such a setup exists. Maybe we can use this as an excuse to jump on a bench gimlet and test this together?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, there's no unit test for that behavior. When I merged this PR initially, I just tested it ad hoc (since it's a pretty straightforward thing to test, and only matters for startup of the agent).

but yes, let's do that!

_ => {
instances
.set_reservoir_size(&hardware, reservoir_mode)
.map_err(|e| {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be nice to use the tap crate for situations like this: tap_err()

error!(log, "Failed to setup VMM reservoir: {e}");
e
})?;
}
}

Expand Down
5 changes: 5 additions & 0 deletions smf/sled-agent/non-gimlet/config.toml
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,11 @@ zpools = [
# guest memory is pulled from.
vmm_reservoir_percentage = 50

# Optionally you can specify the size of the VMM reservoir in MiB.
# Note vmm_reservoir_percentage and vmm_reservoir_size_mb cannot be specified
# at the same time.
#vmm_reservoir_size_mb = 2048

# Swap device size for the system. The device is a sparsely allocated zvol on
# the internal zpool of the M.2 that we booted from.
#
Expand Down
Loading