Remove SYS_RESOURCE capability from launcher pod #2584
Conversation
kubevirt-bot
added
do-not-merge/release-note-label-needed
kubevirt-bot
added
release-note
|
/hold Need to consider calculating the exact limit size in handler (the formula is not trivial but we can do a reasonable estimate). |
kubevirt-bot
added
the
do-not-merge/hold
|
heads up, we just merged a PR which introduces kubevirt specific SCCs instead of using the privileged SCC. The capability can be removed there as well I guess: |
booxter
force-pushed
the
ulimit
branch
3 times, most recently
from
795ca3b
to
87dae49
Compare
|
Unit test coverage is somewhat down because the code dealing with processes / ulimits is not unit tested. But it should be enough to cover it for regressions with existing functional tests: as long as VMIs still start correctly and have SR-IOV interfaces inside the guest, it should be enough. There is little value in validating that the capability is indeed not present anymore because it's trivial to double check it's not referenced anywhere in the code anymore. |
|
/hold cancel |
kubevirt-bot
removed
the
do-not-merge/hold
|
|
||
| func (s *socketBasedIsolationDetector) AdjustResources(vm *v1.VirtualMachineInstance) error { | ||
| // bump memlock ulimit for libvirtd | ||
| res, err := s.Detect(vm) |
There was a problem hiding this comment.
could we use a more specific name than result?
There was a problem hiding this comment.
(won't block the PR on this one, if you answer no no no to my comments, this PR is good to go)
There was a problem hiding this comment.
it's of "IsolationResult" type so I thought it's a good enough name. I am ok with renaming. What would be a better name?
There was a problem hiding this comment.
it is being used only in this block and i don't really know. since the other comments are resolved, i wont bother
|
/retest |
1 similar comment
|
/retest |
pkg/virt-handler/vm_test.go
Outdated
| @@ -1286,6 +1288,24 @@ func (m *MockGracefulShutdown) TriggerShutdown(vmi *v1.VirtualMachineInstance) { | |||
| Expect(err).NotTo(HaveOccurred()) | |||
| } | |||
|
|
|||
| type MockIsolationDetector struct{} | |||
There was a problem hiding this comment.
This should be auto generated?
There was a problem hiding this comment.
OK I've found a way to make it work without manually writing a fake.
|
/retest |
|
/retest |
|
/test pull-kubevirt-e2e-k8s-multus-1.13.3 |
|
/approved |
|
/hold cancel Now the ulimit adjustment happens only for VFIO attached VMIs. Also rebased the branch to latest master in hope that some job failures go away. |
kubevirt-bot
removed
the
do-not-merge/hold
|
/retest |
|
@booxter Travis is red, please run make generate |
kubevirt-bot
added
the
needs-rebase
kubevirt-bot
removed
the
needs-rebase
|
/retest I couldn't reproduce the sriov job failure locally; let's see if it is consistent or a fluke. |
|
/retest |
This syscall is implemented in libraries we use anyway, so we can easily avoid dealing with unsafe pointers etc.
Instead, set the (unlimited) limit for libvirtd from handler pod that is already privileged.
Instead of setting to unlimited, try to estimate the actual amount libvirtd may need for the VM. The actual formula in libvirtd code is very complex and hard to reproduce (it involves estimating necessary resources based on NUMA topology, number of CPUs, platform specific requirements for memory alignment etc.) We are not going to reproduce it in kubevirt, instead making our best conservative guess and then allowing libvirtd to set the actual calculated value (that should work as long as the value used by libvirtd is lower than the limit we set in kubevirt).
Libvirtd configures the limit in particular domain configurations only. This limit adjustment is of no use for VMIs not attached to VFIO.
|
/retest |
|
/lgtm |
|
/retest |
|
/retest |
3 similar comments
|
/retest |
|
/retest |
|
/retest |
Instead, set the (unlimited) limit for libvirtd from handler pod that
is already privileged.