runtime-rs: Implement resize memory for CH #9564
Conversation
cmaf
force-pushed
the
runtime-rs-resize-mem-1
branch
2 times, most recently
from
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.
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.
@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.
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.
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.
| 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.
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.
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.
Shouldn't this rather be vm.resize - copy/paste error?
There was a problem hiding this comment.
Yes. this was a copy/paste error, thank you for catching it!
cmaf
force-pushed
the
runtime-rs-resize-mem-1
branch
from
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.
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.
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.
cmaf
force-pushed
the
runtime-rs-resize-mem-1
branch
from
64a9fc8
to
3f7d1da
Compare
cmaf
added
wip
cmaf
force-pushed
the
runtime-rs-resize-mem-1
branch
from
3f7d1da
to
35c8b56
Compare
cmaf
removed
the
wip
cmaf
force-pushed
the
runtime-rs-resize-mem-1
branch
from
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>
| let mut capabilities = Capabilities::new(); | ||
| capabilities.set( | ||
| let mut caps = Capabilities::new(); | ||
| caps.set( |
There was a problem hiding this comment.
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.
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.
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