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

runtime-rs: Implement resize memory for CH #9564

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
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
25 changes: 25 additions & 0 deletions src/runtime-rs/crates/hypervisor/ch-config/src/ch_api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,31 @@ pub async fn cloud_hypervisor_vm_blockdev_add(
.await?
}

#[derive(Deserialize, Serialize, Default, Debug)]
pub struct VmResizeData {
pub desired_vcpus: Option<u8>,
pub desired_ram: Option<u64>,
pub desired_balloon: Option<u64>,
}

pub async fn cloud_hypervisor_vm_resize(
mut socket: UnixStream,
resize_data: VmResizeData,
) -> Result<Option<String>> {
task::spawn_blocking(move || -> Result<Option<String>> {
let response = simple_api_full_command_and_response(
&mut socket,
"PUT",
"vm.resize",
Some(&serde_json::to_string(&resize_data)?),
)
.map_err(|e| anyhow!(e))?;

Ok(response)
})
.await?
}

pub async fn cloud_hypervisor_vm_netdev_add(
mut socket: UnixStream,
net_config: NetConfig,
Expand Down
2 changes: 1 addition & 1 deletion src/runtime-rs/crates/hypervisor/ch-config/src/convert.rs
Original file line number Diff line number Diff line change
Expand Up @@ -289,7 +289,7 @@ impl TryFrom<(MemoryInfo, GuestProtection)> for MemoryConfig {
// method is available in the rust language.
//
// See: https://github.com/rust-lang/rust/issues/88581
fn checked_next_multiple_of(value: u64, multiple: u64) -> Option<u64> {
pub fn checked_next_multiple_of(value: u64, multiple: u64) -> Option<u64> {
match value.checked_rem(multiple) {
None => Some(value),
Some(r) => value.checked_add(multiple - r),
Expand Down
19 changes: 11 additions & 8 deletions src/runtime-rs/crates/hypervisor/src/ch/inner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ pub struct CloudHypervisorInner {
/// List of devices that will be added to the VM once it boots
pub(crate) pending_devices: Vec<DeviceType>,

pub(crate) _capabilities: Capabilities,
pub(crate) capabilities: Capabilities,

pub(crate) shutdown_tx: Option<Sender<bool>>,
pub(crate) shutdown_rx: Option<Receiver<bool>>,
Expand All @@ -74,16 +74,18 @@ pub struct CloudHypervisorInner {
// None.
pub(crate) ch_features: Option<Vec<String>>,

/// Size of memory block of guest OS in MB (currently unused)
pub(crate) _guest_memory_block_size_mb: u32,
// hotplug memory size
pub(crate) mem_hotplug_size_mb: u32,
/// Size of memory block of guest OS in MB
pub(crate) guest_memory_block_size_mb: u32,
}

const CH_DEFAULT_TIMEOUT_SECS: u32 = 10;

impl CloudHypervisorInner {
pub fn new() -> Self {
let mut capabilities = Capabilities::new();
capabilities.set(
let mut caps = Capabilities::new();
caps.set(
Copy link
Member

Choose a reason for hiding this comment

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

Since you are adding support for memory hotplug, should'nt you be setting GuestMemoryProbe in this bitset now?

CapabilityBits::BlockDeviceSupport
| CapabilityBits::BlockDeviceHotplugSupport
| CapabilityBits::FsSharingSupport
Expand All @@ -109,13 +111,14 @@ impl CloudHypervisorInner {
netns: None,
pending_devices: vec![],
device_ids: HashMap::<String, String>::new(),
_capabilities: capabilities,
capabilities: caps,
shutdown_tx: Some(tx),
shutdown_rx: Some(rx),
tasks: None,
guest_protection_to_use: GuestProtection::NoProtection,
ch_features: None,
_guest_memory_block_size_mb: 0,
mem_hotplug_size_mb: 0,
guest_memory_block_size_mb: 0,
}
}

Expand Down Expand Up @@ -183,7 +186,7 @@ impl Persist for CloudHypervisorInner {

..Default::default()
};
ch._capabilities = ch.capabilities().await?;
ch.capabilities = ch.capabilities().await?;

Ok(ch)
}
Expand Down
218 changes: 210 additions & 8 deletions src/runtime-rs/crates/hypervisor/src/ch/inner_hypervisor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,10 @@ use crate::VM_ROOTFS_DRIVER_PMEM;
use crate::{VcpuThreadIds, VmmState};
use anyhow::{anyhow, Context, Result};
use ch_config::ch_api::{
cloud_hypervisor_vm_create, cloud_hypervisor_vm_start, cloud_hypervisor_vmm_ping,
cloud_hypervisor_vmm_shutdown,
cloud_hypervisor_vm_create, cloud_hypervisor_vm_resize, cloud_hypervisor_vm_start,
cloud_hypervisor_vmm_ping, cloud_hypervisor_vmm_shutdown, VmResizeData,
};
use ch_config::convert::checked_next_multiple_of;
use ch_config::{guest_protection_is_tdx, NamedHypervisorConfig, VmConfig};
use core::future::poll_fn;
use futures::executor::block_on;
Expand Down Expand Up @@ -761,17 +762,109 @@ impl CloudHypervisorInner {
}

pub(crate) fn set_guest_memory_block_size(&mut self, size: u32) {
self._guest_memory_block_size_mb = size;
self.guest_memory_block_size_mb = size;
Copy link
Contributor

Choose a reason for hiding this comment

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

From my tests it looks like the value passed to this function is actually in bytes, not megabytes - you might want to double-check that...

}

pub(crate) fn guest_memory_block_size_mb(&self) -> u32 {
self._guest_memory_block_size_mb
self.guest_memory_block_size_mb
}

pub(crate) fn resize_memory(&self, _new_mem_mb: u32) -> Result<(u32, MemoryConfig)> {
warn!(sl!(), "CH memory resize not implemented - see https://github.com/kata-containers/kata-containers/issues/8801");
pub(crate) async fn resize_memory(
&mut self,
mut new_mem_mb: u32,
) -> Result<(u32, MemoryConfig)> {
// check for probe in memory_config
let caps = &self.capabilities;
if caps.is_mem_hotplug_probe_supported() {
Copy link
Member

Choose a reason for hiding this comment

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

Since we know that cloud-hypervisor supports memory hotplug, should be perform the check here?
I feel that this check needs to be done at a higher abstraction level (hypervisor level ) before this gets called at all.

Copy link
Member

Choose a reason for hiding this comment

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

Also since GuestMemoryProbe has not been added to the capability set yet, is this check passing currently? Or am I missing something?

warn!(sl!(), "probe memory is not supported for cloud hypervisor");
return Ok((0, MemoryConfig::default()));
}

// check for resizing to 0
if new_mem_mb == 0 {
error!(sl!(), "Cannot resize memory to 0");
return Ok((0, MemoryConfig::default()));
}

// get hotplug size
let max_hotplug_size_mb = self.hypervisor_config().memory_info.default_maxmemory;

// get mem size
let default_mem_size_mb = self.hypervisor_config().memory_info.default_memory;
let current_hotplug_size_mb = self.mem_hotplug_size_mb;
let current_mem_size_mb = default_mem_size_mb + current_hotplug_size_mb;

if new_mem_mb < current_mem_size_mb {
warn!(sl!(), "Remove memory is not supported, nothing to do");
return Ok((current_mem_size_mb, MemoryConfig::default()));
}

let block_size_mb = self.guest_memory_block_size_mb;

let new_hotplug_size_mb = new_mem_mb - current_mem_size_mb;

Ok((0, MemoryConfig::default()))
// checked_next multiple_of() will align 0 to the block size, which we don't want here,
// so run check to avoid that
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that the function will do that not just for zero but for any input value that is aligned already.

let mut aligned_new_hotplug_size_mb = None;
if new_hotplug_size_mb != 0 {
aligned_new_hotplug_size_mb =
checked_next_multiple_of(new_hotplug_size_mb.into(), block_size_mb.into());
}

if let Some(aligned_value_mb) = aligned_new_hotplug_size_mb {
let aligned_request = current_mem_size_mb + aligned_value_mb as u32;
if new_mem_mb < aligned_request {
info!(
sl!(),
"Aligning VM memory request {} to {} with block size {}", new_mem_mb, aligned_request, block_size_mb;
);
new_mem_mb = aligned_request;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if aligned_new_hotplug_size_mb is None, i.e. if aligning fails? Do we want to continue with the old unaligned size?


// Checks after memory alignment
if new_mem_mb == current_mem_size_mb {
info!(sl!(), "VM already has requested memory {}", new_mem_mb);
return Ok((current_mem_size_mb, MemoryConfig::default()));
}
if new_mem_mb > max_hotplug_size_mb {
warn!(
sl!(),
"Memory size unchanged; memory requested {} is greater than max memory {}",
new_mem_mb,
max_hotplug_size_mb
);
return Ok((current_mem_size_mb, MemoryConfig::default()));
}

// update hotplug size
self.mem_hotplug_size_mb = new_mem_mb - self.hypervisor_config().memory_info.default_memory;

// resize to bytes for VM resize
let resize_mem = new_mem_mb as u64 * 1024 * 1024;

let socket = self
.api_socket
.as_ref()
.ok_or("missing socket")
.map_err(|e| anyhow!(e))?;

let resize_mem_data = VmResizeData {
desired_ram: Some(resize_mem),
..Default::default()
};

let response = cloud_hypervisor_vm_resize(
socket.try_clone().context("failed to clone socket")?,
resize_mem_data,
)
.await?;

if let Some(detail) = response {
debug!(sl!(), "vm resize memory response: {:?}", detail);
}

Ok((new_mem_mb, MemoryConfig::default()))
}
}

Expand Down Expand Up @@ -967,7 +1060,9 @@ mod tests {
#[cfg(target_arch = "x86_64")]
use kata_sys_util::protection::TDX_SYS_FIRMWARE_DIR;

use kata_types::config::hypervisor::{Hypervisor as HypervisorConfig, SecurityInfo};
use kata_types::config::hypervisor::{
Hypervisor as HypervisorConfig, MemoryInfo, SecurityInfo,
};
use serial_test::serial;
#[cfg(target_arch = "x86_64")]
use std::path::PathBuf;
Expand Down Expand Up @@ -1209,6 +1304,113 @@ mod tests {
set_fake_guest_protection(None);
}

#[actix_rt::test]
async fn test_resize_memory() {
#[derive(Debug)]
struct TestData {
new_mem_mb: u32,
probe: bool,
default_maxmemory: u32,
default_memory: u32,
mem_hotplug_size_mb: u32,
guest_memory_block_size_mb: u32,
result: u32,
}

let tests = &[
// tests for failure cases
// probe failure
TestData {
new_mem_mb: 1024,
probe: true,
default_maxmemory: 2048,
default_memory: 512,
mem_hotplug_size_mb: 0,
guest_memory_block_size_mb: 128,
result: 0,
},
// resize to 0 failure
TestData {
new_mem_mb: 0,
probe: false,
default_maxmemory: 2048,
default_memory: 512,
mem_hotplug_size_mb: 0,
guest_memory_block_size_mb: 128,
result: 0,
},
// new_mem_mb > max_hotplug_size_mb failure, no alignment
TestData {
new_mem_mb: 4096,
probe: false,
default_maxmemory: 2048,
default_memory: 512,
mem_hotplug_size_mb: 0,
guest_memory_block_size_mb: 128,
result: 512,
},
// new_mem_mb == current_mem_size_mb failure, no alignment
TestData {
new_mem_mb: 1024,
probe: false,
default_maxmemory: 2048,
default_memory: 512,
mem_hotplug_size_mb: 512,
guest_memory_block_size_mb: 128,
result: 1024,
},
// new_mem_mb > max_hotplug_size_mb failure after alignment
TestData {
new_mem_mb: 1023,
probe: false,
default_maxmemory: 2048,
default_memory: 512,
mem_hotplug_size_mb: 512,
guest_memory_block_size_mb: 128,
result: 1024,
},
// new_mem_mb == current_mem_size_mb failure after alignment
TestData {
new_mem_mb: 2048,
probe: false,
default_maxmemory: 2048,
default_memory: 1023,
mem_hotplug_size_mb: 0,
guest_memory_block_size_mb: 128,
result: 1023,
},
];

for (i, d) in tests.iter().enumerate() {
let msg = format!("test[{}]: {:?}", i, d);

let mut ch = CloudHypervisorInner::default();
let mut hypervisor_config = HypervisorConfig::default();
let mut mem_info = MemoryInfo::default();
let mut capabilities = Capabilities::new();

mem_info.default_maxmemory = d.default_maxmemory;
mem_info.default_memory = d.default_memory;

hypervisor_config.memory_info = mem_info;
ch.config = Some(hypervisor_config);

ch.mem_hotplug_size_mb = d.mem_hotplug_size_mb;
ch.guest_memory_block_size_mb = d.guest_memory_block_size_mb;

if d.probe {
capabilities.set(CapabilityBits::GuestMemoryProbe);
}

ch.capabilities = capabilities;

let result = ch.resize_memory(d.new_mem_mb).await.unwrap();
let (mem_mb, _) = result;

assert_eq!(d.result, mem_mb, "{}", msg);
}
}

#[actix_rt::test]
async fn test_get_kernel_params() {
#[derive(Debug)]
Expand Down
4 changes: 2 additions & 2 deletions src/runtime-rs/crates/hypervisor/src/ch/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -179,8 +179,8 @@ impl Hypervisor for CloudHypervisor {
}

async fn resize_memory(&self, new_mem_mb: u32) -> Result<(u32, MemoryConfig)> {
let inner = self.inner.read().await;
inner.resize_memory(new_mem_mb)
let mut inner = self.inner.write().await;
inner.resize_memory(new_mem_mb).await
}

async fn get_passfd_listener_addr(&self) -> Result<(String, u32)> {
Expand Down
Loading