-
Notifications
You must be signed in to change notification settings - Fork 1k
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
base: main
Are you sure you want to change the base?
Conversation
1d4a7f8
to
3449172
Compare
|
||
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 { |
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 this testing only done for the log message? Because otherwise ~10 lines of code could be reduced to one. (new_mem_mb
will end up equal to aligned_request
no matter what.)
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.
@pmores it is for logging. I logged here what is logged in the go runtime for cloud hypervisor; we could consider reducing the amount of logging for both.
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 mind logging this at all, I just wanted to make sure this is a conscious decision. Thanks!
if new_mem_mb != aligned_request { | ||
info!( | ||
sl!(), | ||
"Aligning VM memory request {} to {}", new_mem_mb, aligned_request |
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.
Provided we have to have this log message, I'd propose including block_size_mg
as well so that complete information related to aligning is available to anyone trying to track down a problem.
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
if new_mem_mb == current_mem_size_mb { | ||
info!( | ||
sl!(), | ||
"VM already has requested memory after alignment {}", new_mem_mb |
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.
It's a matter of taste but frankly, this wording seem a bit cryptic to me, considering that we're just trying to say that nothing needed to be done since the VM has had the requested amount of memory already.
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 agree, it may not be necessary to log this.
let response = simple_api_full_command_and_response( | ||
&mut socket, | ||
"PUT", | ||
"vm.add-disk", |
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.
Shouldn't this rather be vm.resize
- copy/paste error?
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.
Yes. this was a copy/paste error, thank you for catching it!
3449172
to
64a9fc8
Compare
@pmores thanks for the review. I updated a few things based on your comments and also modified some of the code comments for clarity. For logging, I chose to log the same information that is logged in the go runtime for cloud hypervisor. We could consider reducing it or switching it to debug. |
} | ||
} | ||
|
||
// Additional checks after memory alignment |
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.
Hi @cmaf
Is the init default memory block size aligned? If the initial values are aligned to the block size, then it is enough to do the alignment first and then do these size checks. Is there no need to check before the alignment?
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.
That's a very interesting question. Correct me if I'm wrong but it would be hard to rely on memory_info.default_memory
being aligned since 1) it comes from the config file, ie. the user, and 2) the memory allocation granularity (the block size) is only determined at runtime via the agent.
So I think (and I might well be off here) that at least on the initial invocation of this function when current_hotplug_size_mb
is likely zero and current_mem_size_mb
thus consists purely of memory_info.default_memory
alignment of the initial values can hardly be guaranteed.
That said, I still think you're right suggesting that the pre-alignment checks could be left out. They seem to serve as an early-out to avoid the alignment computations if it's clear that they are not needed. This would make a lot of sense if they were considerably resource-heavy. However, since that doesn't seem the case I'd consider leaving the pre-alignment checks out, simplifying the function at the risk that a bunch of simple arithmetics might be performed unnecessarily at times.
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.
64a9fc8
to
3f7d1da
Compare
3f7d1da
to
35c8b56
Compare
35c8b56
to
173950e
Compare
Add resize_memory for Cloud Hypervisor and cloud_hypervisor_vm_resize for the call to the VM. Write unit tests checking failure cases. Fixes kata-containers#8801 Signed-off-by: Chelsea Mafrica <chelsea.e.mafrica@intel.com>
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, thanks @cmaf !
let mut capabilities = Capabilities::new(); | ||
capabilities.set( | ||
let mut caps = Capabilities::new(); | ||
caps.set( |
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.
Since you are adding support for memory hotplug, should'nt you be setting GuestMemoryProbe
in this bitset now?
) -> Result<(u32, MemoryConfig)> { | ||
// check for probe in memory_config | ||
let caps = &self.capabilities; | ||
if caps.is_mem_hotplug_probe_supported() { |
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.
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.
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.
Also since GuestMemoryProbe has not been added to the capability set yet, is this check passing currently? Or am I missing something?
Add resize_memory for Cloud Hypervisor and cloud_hypervisor_vm_resize for the call to the VM. Write unit tests checking failure cases.
Fixes #8801