-
Notifications
You must be signed in to change notification settings - Fork 42
runtime-rs: Port MSFT changes from msft-main in runtime-go #429
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
base: msft-preview
Are you sure you want to change the base?
Changes from all commits
f67e9a7
e5880d2
a415d69
0bfcd5a
85c57e6
00aa722
5e002c2
c993b6f
9580cf6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -143,6 +143,17 @@ impl InitialSizeManager { | |
| if self.resource.vcpu > 0.0 { | ||
| info!(sl!(), "resource with vcpu {}", self.resource.vcpu); | ||
| } | ||
|
|
||
| if config.runtime.static_sandbox_resource_mgmt { | ||
| if self.resource.mem_mb == 0 { | ||
| self.resource.mem_mb = config.runtime.static_sandbox_default_workload_mem; | ||
| } | ||
|
|
||
| if self.resource.vcpu == 0.0 { | ||
| self.resource.vcpu = config.runtime.static_sandbox_default_workload_vcpus; | ||
| } | ||
| } | ||
|
Redent0r marked this conversation as resolved.
|
||
|
|
||
| self.resource.orig_toml_default_mem = hv.memory_info.default_memory; | ||
| if self.resource.mem_mb > 0 { | ||
| // since the memory overhead introduced by kata-agent and system components | ||
|
|
@@ -152,12 +163,24 @@ impl InitialSizeManager { | |
| // use memory as they orignally expected, it would be easy to OOM.) | ||
| hv.memory_info.default_memory += self.resource.mem_mb; | ||
| } | ||
|
|
||
| if self.resource.vcpu > 0.0 { | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This block will probably generate discussion. This commit is trying to port over kata-containers@9af9844 . After doing that, I realized that, unlike the memory, the static CPU set in the config wasn't getting past to the guest VM as expected. I got some error in the lines of "not enough vcpus" and in the logs I was able to confirm the VM config passed to CH was using 0 vcpus. It looks like runtime-rs is missing https://github.com/fidencio/kata-containers/blob/e2476f587c472d5d217df9c75cdb80193dd85994/src/runtime/pkg/oci/utils.go#L1232 But then the question I have is: is upstream runtime-rs really not adding vcpus that come from limits or annotations? I must be missing something |
||
| hv.cpu_info.default_vcpus += self.resource.vcpu; | ||
| } | ||
| Ok(()) | ||
| } | ||
|
|
||
| pub fn get_orig_toml_default_mem(&self) -> u32 { | ||
| self.resource.orig_toml_default_mem | ||
| } | ||
|
|
||
| /// Returns the effective workload memory for the pod/container (in MiB). | ||
| /// This may be either explicitly requested in the spec or defaulted by static | ||
| /// sandbox resource management. 0 means no explicit limit was set and no default | ||
| /// workload memory was applied. | ||
| pub fn workload_mem_mb(&self) -> u32 { | ||
| self.resource.mem_mb | ||
| } | ||
| } | ||
|
|
||
| fn get_nr_vcpu(resource: &LinuxContainerCpuResources) -> f32 { | ||
|
|
@@ -197,6 +220,7 @@ fn get_sizing_info(annotation: Annotation) -> Result<(u64, i64, i64)> { | |
| mod tests { | ||
| use super::*; | ||
| use kata_types::annotations::cri_containerd; | ||
| use kata_types::config::Hypervisor; | ||
| use oci_spec::runtime::{LinuxBuilder, LinuxMemory, LinuxMemoryBuilder, LinuxResourcesBuilder}; | ||
| use std::collections::HashMap; | ||
| #[derive(Clone)] | ||
|
|
@@ -366,4 +390,72 @@ mod tests { | |
| ); | ||
| } | ||
| } | ||
|
|
||
| fn get_config_for_setup_tests( | ||
| base_vcpus: f32, | ||
| base_mem_mb: u32, | ||
| static_mgmt: bool, | ||
| default_workload_vcpus: f32, | ||
| default_workload_mem_mb: u32, | ||
| ) -> TomlConfig { | ||
| let hypervisor_name = "test-hv".to_string(); | ||
| let mut config = TomlConfig::default(); | ||
| config.runtime.hypervisor_name = hypervisor_name.clone(); | ||
| config.runtime.static_sandbox_resource_mgmt = static_mgmt; | ||
| config.runtime.static_sandbox_default_workload_vcpus = default_workload_vcpus; | ||
| config.runtime.static_sandbox_default_workload_mem = default_workload_mem_mb; | ||
|
|
||
| let mut hv = Hypervisor::default(); | ||
| hv.cpu_info.default_vcpus = base_vcpus; | ||
| hv.memory_info.default_memory = base_mem_mb; | ||
| config.hypervisor.insert(hypervisor_name, hv); | ||
|
|
||
| config | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_setup_config_static_defaults_unset_resources() { | ||
| let mut manager = InitialSizeManager { | ||
| resource: InitialSize { | ||
| vcpu: 0.0, | ||
| mem_mb: 0, | ||
| orig_toml_default_mem: 0, | ||
| }, | ||
| }; | ||
| let mut config = get_config_for_setup_tests(2.0, 256, true, 1.0, 512); | ||
|
|
||
| manager.setup_config(&mut config).unwrap(); | ||
|
|
||
| let hv = config | ||
| .hypervisor | ||
| .get(&config.runtime.hypervisor_name) | ||
| .unwrap(); | ||
| assert_eq!(hv.cpu_info.default_vcpus, 3.0); | ||
| assert_eq!(hv.memory_info.default_memory, 768); | ||
| assert_eq!(manager.get_orig_toml_default_mem(), 256); | ||
| assert_eq!(manager.workload_mem_mb(), 512); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_setup_config_static_preserves_explicit_resources() { | ||
| let mut manager = InitialSizeManager { | ||
| resource: InitialSize { | ||
| vcpu: 1.5, | ||
| mem_mb: 1024, | ||
| orig_toml_default_mem: 0, | ||
| }, | ||
| }; | ||
| let mut config = get_config_for_setup_tests(2.0, 256, true, 3.0, 512); | ||
|
|
||
| manager.setup_config(&mut config).unwrap(); | ||
|
|
||
| let hv = config | ||
| .hypervisor | ||
| .get(&config.runtime.hypervisor_name) | ||
| .unwrap(); | ||
| assert_eq!(hv.cpu_info.default_vcpus, 3.5); | ||
| assert_eq!(hv.memory_info.default_memory, 1280); | ||
| assert_eq!(manager.get_orig_toml_default_mem(), 256); | ||
| assert_eq!(manager.workload_mem_mb(), 1024); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -33,13 +33,13 @@ use netns_rs::{Env, NetNs}; | |||||
| use nix::{sys::statfs, unistd::User}; | ||||||
| use oci_spec::runtime as oci; | ||||||
| use persist::sandbox_persist::Persist; | ||||||
| use protobuf::Message as ProtobufMessage; | ||||||
| use resource::{ | ||||||
| cpu_mem::initial_size::InitialSizeManager, | ||||||
| network::{dan_config_path, generate_netns_name}, | ||||||
| }; | ||||||
| use runtime_spec as spec; | ||||||
| use shim_interface::shim_mgmt::ERR_NO_SHIM_SERVER; | ||||||
| use protobuf::Message as ProtobufMessage; | ||||||
| use std::{ | ||||||
| collections::HashMap, | ||||||
| env, | ||||||
|
|
@@ -218,6 +218,16 @@ impl RuntimeHandlerManagerInner { | |||||
| .setup_config(&mut config) | ||||||
| .context("failed to setup static resource mgmt config")?; | ||||||
|
|
||||||
| let mem_min = config.runtime.sandbox_workload_mem_min; | ||||||
| let workload_mem = initial_size_manager.workload_mem_mb(); | ||||||
| if workload_mem < mem_min { | ||||||
|
||||||
| if workload_mem < mem_min { | |
| if workload_mem != 0 && workload_mem < mem_min { |
Uh oh!
There was an error while loading. Please reload this page.