virtiofs: Allow memory hotplug with virtiofs #1810
Conversation
|
This PR depends on kata-containers/govmm#96 and once that lands, we can vendor in the changes. Getting this out here for review while we wait for govmm change to land. |
ganeshmaharaj
force-pushed
the
virtiofs-hotplug
branch
2 times, most recently
from
b87a7a1
to
bc8614a
Compare
virtcontainers/qemu.go
Outdated
| share = true | ||
| } else if q.config.SharedFS == config.VirtioFS || q.config.FileBackedMemRootDir != "" { | ||
| target = q.qemuConfig.Memory.Path | ||
| memory_back = "memory-backend-file" |
There was a problem hiding this comment.
This is the default virtio-fs case right.
There was a problem hiding this comment.
yes. this is the default case.
| if q.qemuConfig.Knobs.HugePages { | ||
| // we are setting all the bits that govmm sets when hugepages are enabled. | ||
| // https://github.com/intel/govmm/blob/master/qemu/qemu.go#L1677 | ||
| target = "/dev/hugepages" |
There was a problem hiding this comment.
Should we just go away from explicit huge pages and use the memory-backend-file method long term.
There was a problem hiding this comment.
I think so. Even if this can be the most performant way to use virtio-fs, it can have expectation on the host that most people don't have, therefore I think only people who know what they're doing should enable it.
There was a problem hiding this comment.
Today from master and 1.7.x release, virtio-fs uses /dev/shm and is no longer using hugepages. this change is only making sure we are compatible with hugepages and memory hotplug.
|
@ganeshmaharaj @egernst with this change we can use virtio-fs freely with kubernetes right? and also support dynamic updates to container memory sizes. |
ganeshmaharaj
force-pushed
the
virtiofs-hotplug
branch
from
bc8614a
to
81b9380
Compare
|
/test-nemu |
|
/test |
Codecov Report
@@ Coverage Diff @@
## master #1810 +/- ##
=========================================
Coverage ? 52.46%
=========================================
Files ? 108
Lines ? 13963
Branches ? 0
=========================================
Hits ? 7325
Misses ? 5768
Partials ? 870 |
|
@GabyCT @chavafg the cri-containerd tests are failing with seems like a new issue with the teardown process of k8s. anything that stands out at either of you from that error log? |
|
@ganeshmaharaj In addition, on the initrd job I see that there was a failure on a docker test: |
|
opened an issue for the test failure, as I've seen this on other PRs as well @ganeshmaharaj @chavafg: kata-containers/tests#1745 /cc @GabyCT |
|
Any update on this @ganeshmaharaj? |
we still have some issues running memory hotplug using virtiofs, so many of the cri-o and k8s tests do not run as expected. This should be resolved on kata-containers/runtime#1810, but in the meantime run only docker related tests. Signed-off-by: Salvador Fuentes <[email protected]>
we still have some issues running memory hotplug using virtiofs, so many of the cri-o and k8s tests do not run as expected. This should be resolved on kata-containers/runtime#1810, but in the meantime run only docker related tests. Signed-off-by: Salvador Fuentes <[email protected]>
|
I've tested this PR, and can now specify memory limits/requests in Kubernetes and they work properly with virtio-fs. Looking forward to this being released. |
|
@ganeshmaharaj any updates? |
|
@jodh-intel @raravena80 the patch is good to merge. I am currently out with very limited access to a PC for the next few weeks. I am not able to make sense of my the initrd job fails to run a container. The other crio issue seems to no longer exist. |
|
@ganeshmaharaj The initrd job fails consistently on the |
|
@chavafg ohh.. I am curious why that's failing. This change shouldn't affect that. If anyone can get to this sooner than 2 weeks, please do. Will be able to look at this again in a couple of weeks when I am back with decent connectivity. |
|
may be worth splitting out the govmm vendoring, then, since the runtime changes shouldn't impact this now-consistently-failing-for-this-PR test. |
|
Nack: the vendoring commit + adjustments to use it should be standalone, ie, should be able to bisect. I see compiler errors with just the first 3 commits. |
ganeshmaharaj
force-pushed
the
virtiofs-hotplug
branch
from
81b9380
to
4e33379
Compare
@chavafg Just rebased the branch and locally tested initrd and i am not able to reproduce the issue. This is the logs i got from my test. Setup
@egernst fixed the patch for this comment. Now all commits are bisectable. |
|
/test-initrd |
|
/test |
|
@chavafg that issue happens only when i try to hotplug more than 8 devices. In the failing case the error logs look like this whereas without the patch, partial logs show up but the device is plugged |
|
any idea @devimc? |
|
this change makes it work. |
|
@ganeshmaharaj we need disable-modern on azure |
@devimc With the current code in master, |
|
Once #1868 lands, I will rebase this change onto master. @egernst @jodh-intel @grahamwhaley I think these two changes should be backported to 1.7.x branch as virtio-fs feature needs this. WDYT? |
Kata with virtio-fs fails to do memory hotplugging. This is caused by the fact that hot plugged memory is always backed by 'memory-backend-ram' while virtio-fs expects it to be backed by file and shared for it to be able to use the system the way it is intended. This chnage allows using file based memory backend for virtio-fs, hugepages or when the user prefers to use a file backed memory Fixes: kata-containers#1745 Signed-off-by: Ganesh Maharaj Mahalingam <[email protected]>
ganeshmaharaj
force-pushed
the
virtiofs-hotplug
branch
from
4e33379
to
d392b22
Compare
|
/test |
|
Verified that this patch, after rebase works as expected with virtiofs. @jcvenegas @mcastelino @devimc |
|
Looks like #1868 has recently been merged. Are there any other dependancies on this @ganeshmaharaj ? We're super-keen to see this merged and give it a go so we can finally move away from 9p. |
|
@coreyjohnston nope.. none that i can think of. This patch should be ready to land. once this hits master, i will backport this 1.7.x tree to make sure we have a stable with this fix. @egernst @devimc @mcastelino @grahamwhaley @jodh-intel any objections? |
|
merging as it is already approved and all CI passed, thanks @ganeshmaharaj |
|
Brilliant, thanks @ganeshmaharaj ! |
|
@coreyjohnston we will be backporting this to 1.8 release, but not to 1.7. Hope that is alright. Since this is still an experimental feature, backporting a vendor change to 1.7 was decided to be much of a risk on the stability front. |
Kata with virtio-fs fails to do memory hotplugging. This is caused by
the fact that hot plugged memory is always backed by
'memory-backend-ram' while virtio-fs expects it to be backed by file and
shared for it to be able to use the system the way it is intended. This
chnage allows using file based memory backend for virtio-fs, hugepages
or when the user prefers to use a file backed memory
Fixes: #1745
Signed-off-by: Ganesh Maharaj Mahalingam [email protected]