Drop SYS_RESOURCE capability #5558

ormergi · 2021-04-28T17:52:31Z

What this PR does / why we need it:
Currently virt-launcher runs with SYS_RESOURCE capability on VMs with SRIOV devices when SRIOVLiveMigation feature-gate enabled.
Having this capability enables Libvirt to adjust QEMU process memory lock limits (memlock rlimit) in case its already exists
As occurs when SRIOV VM is migrated and the SRIOV host-device is re-attached to VM on target.

With Libvirt and above, Libvirt does best effort to adjusts QEMU process limits and wont try to change the limits if they are high enough.
In order to prevent using SYS_RESOURCE cap on containerized environments [1] as Kubevirt use case.

With this PR changes Kubevirt will adjust QEMU process rlimits as it does for libvirtd.
This will prevent libvirt from adjusting QEMU process rlimits (it will detect that limits are sufficient).
Then virt-launcher resources will be adjusted on post migration, in order to enable hot plugging VFIO devices

This PR also remove SYS_RESOURCE capability.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:
Rebased on #5328

virt-handler is privileged while virt-launcher is not.
Before this change, it was already done for the libvirtd process, the only difference here is that we now need the qemu one.

Release note:

Drop virt-launcher SYS_RESOURCE capability

kubevirt-bot · 2021-04-28T17:52:34Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

ormergi · 2021-04-28T17:52:52Z

/test pull-kubevirt-e2e-kind-1.17-sriov

ormergi · 2021-04-29T07:41:04Z

/test pull-kubevirt-e2e-kind-1.17-sriov

ormergi · 2021-04-29T07:46:50Z

/test pull-kubevirt-e2e-kind-1.17-sriov

ormergi · 2021-04-29T08:46:28Z

/cc @EdDev

EdDev

gracefully shutdown in case of virt-launcher crash [1][2].

The links are not permanent, so these will become incorrect when the code change.

EdDev · 2021-05-02T08:10:05Z

pkg/virt-handler/isolation/isolation.go

 	_, _, errno := unix.RawSyscall6(unix.SYS_PRLIMIT64,
 		uintptr(pid),
-		limit,
-		uintptr(unsafe.Pointer(rlimit)), // #nosec used in unix RawSyscall6
+		uintptr(unix.RLIMIT_MEMLOCK), // #nosec used in unix RawSyscall6


Why the comment of #nosec moved to a different command? Are you sure it is not needed now for both now?

#nosec comments are added where unsafe pointers are used #4446
This comment no longer needed, I will remove it.

I just realized we still need this comment for line 145, I will just move it

EdDev · 2021-05-02T08:22:29Z

pkg/virt-handler/isolation/isolation.go

@@ -98,13 +99,19 @@ func (s *socketBasedIsolationDetector) DetectForSocket(vm *v1.VirtualMachineInst

 	}

+	if process, err := ps.FindProcess(pid); err != nil {
+		return nil, err
+	} else {


It is common in the code not to do this. How about:

process, err := ps.FindProcess(pid) if err != nil { return ... } ppid := process.PPid()

EdDev

< With Libvirt 7 (Coper)

I do not understand how the Coper part relevant here? That is the source of the package, we mainly care about the version. If you want to be specific, give a more detailed version.

EdDev

commit post migration: Adjust QEMU and libvirt processes memlock limits:

The change in this commit includes both logic addition and refactoring of the existing code. Please split the refactoring (and naming changes) to a separate commit so it will be easier to see what logic changed.

EdDev · 2021-05-02T08:47:16Z

pkg/virt-handler/isolation/isolation.go

-		if err != nil {
-			return err
+func filterVirtLaunchersLibvirtProcesses(isolationResult IsolationResult, processes []ps.Process) []ps.Process {
+	var filteredProcesses []ps.Process


Why do you need this? On line 208 you can just use :=, no?

It may look redundant here because there are only two assignments.
I think its easier to track where a variable was declared when there are more then one assignment (the first is always :=).

EdDev · 2021-05-02T08:49:11Z

pkg/virt-handler/isolation/isolation.go

+func filterVirtLaunchersLibvirtProcesses(isolationResult IsolationResult, processes []ps.Process) []ps.Process {
+	var filteredProcesses []ps.Process
+
+	// consider all processes that are virt-launcher children


Can you please explain it all as a doctext? Having inline comments is hard to keep up-to-date and read the code.

EdDev · 2021-05-02T08:52:33Z

pkg/virt-handler/isolation/isolation.go

+
+	filteredProcesses = filterProcessesByParentPID(processes, []int{launcherPid, launcherParentPid})
+	// libvirtd process sets the memory lock limit before fork/exec-ing into qemu
+	// Its necessary to change QMEU process limits if its already exists (e.g: post migration just before hostdev)


What if we are in a reconcile? Are you sure we can reach this only on post-migration?

We will reach this also from reconcile, although adjust resources is called only when VM is not running
https://github.com/kubevirt/kubevirt/blob/master/pkg/virt-handler/vm.go#L2420
https://github.com/kubevirt/kubevirt/blob/master/pkg/virt-handler/vm.go#L2446
Do we want to restrict changing QEMU process limits only on post migration?
If so I can create AdjsutResourcesOnPostMigration function
that will be called on finalizeMigration (instead of regular AdjustResources)
@EdDev What you think?

I think it is less risky if the change is done per the specific need known to us. The qemu process memlock limit needs to be changed only when the VMI has a VFIO device and the state is post-migration.
If this is open to other scenarios, we may get into corner cases where we should not be.

If so I can create AdjsutResourcesOnPostMigration function

As I see it, a function name should express what it does, not who or when it is called.

I pushed new changes so that only QEMU process will be adjusted on post migration

oshoval · 2021-05-02T13:42:51Z

pkg/virt-handler/isolation/isolation.go

-		}
+	// bump libvirtd and qemu processes locked memory limits
+	libvirtProcesses := filterVirtLaunchersLibvirtProcesses(isolationResult, processes)
+	log.Log.Info("debug: adjust memlock: filtered processes:")


there are some debug logs just fyi (i guess its known)

yup, please use logs verbosity, log.Log.V(X).Info instead of log.Log.Info("debug: - KubeVirt uses verbosity levels, so you can print different messages at various levels.

I added these logs just for debugging I was going to remove them (all logs that stats with 'debug')

maybe it is still valuable to keep it? up to you, but I think if it was valuable for you to debug, it might be useful when things go wrong ;)

vladikr · 2021-05-03T20:23:39Z

pkg/virt-handler/isolation/isolation.go

-			Cur: uint64(memlockSize),
-		}
-		err = prLimit(process.Pid(), unix.RLIMIT_MEMLOCK, &rLimit)
+	for _, process := range libvirtProcesses {


can you please combine this loop with the one on line 190?

vladikr · 2021-05-03T20:25:02Z

pkg/virt-handler/isolation/isolation.go

+	libvirtProcesses := filterVirtLaunchersLibvirtProcesses(isolationResult, processes)
+	log.Log.Info("debug: adjust memlock: filtered processes:")
+	for _, process := range libvirtProcesses {
+		log.Log.Infof("debug: pid %d, ppid %d, exec %s", process.Pid(), process.PPid(), process.Executable())


please make the debug prints more informative. Something like: "found libvirt process with pid %d...."

vladikr · 2021-05-03T20:38:40Z

pkg/virt-handler/vm.go

@@ -2641,6 +2641,13 @@ func (d *VirtualMachineController) finalizeMigration(vmi *v1.VirtualMachineInsta
 	if err != nil {
 		return err
 	}
+
+	err = d.podIsolationDetector.AdjustResources(vmi)


In reality, a failure here doesn't affect the result of migration.
The migration will still show as succeeded. However, perhaps we do need to react differently in this case?
Maybe, we should at least send an alert in case we didn't manage to plug the devices back.

You right, its wrong to return on AdjustResources error, thanks for the heads up.
I will log the error instead of returning, then in case a device is failed to plug it will be logged.

vladikr · 2021-05-26T16:44:37Z

/retest

vladikr · 2021-05-26T18:01:27Z

/approve
Thanks.

kubevirt-bot · 2021-05-26T18:01:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vladikr

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [vladikr]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

kubevirt-bot · 2021-05-26T19:04:34Z

@ormergi: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-kubevirt-e2e-k8s-1.20-sig-compute	`0adf11b`	link	`/test pull-kubevirt-e2e-k8s-1.20-sig-compute`

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

vladikr · 2021-05-26T19:11:56Z

/lgtm

Following kubevirt/kubevirt#5558 Kubevirt is not longer requires SYS_RESOURCE capability in order to enable SRIOV VM's migration. We still want this feature-gate to enable the user to choose whether to enable SRIOV VM migration or not until this feature is maturated. Signed-off-by: Nahshon Unna-Tsameret <nunnatsa@redhat.com>

Following [1], it is no longer needed to add SYS_RESOURCE capability to virt-launcher, SRIOVLiveMigration feature-gate is redundant. SR-IOV live migration in controlled by 'LiveMigration' feature gate. [1] kubevirt#5558 Signed-off-by: Or Mergi <ormergi@redhat.com>

Following [1] virt-launcher doesn't execute with the `SYS_RESOURCE` capability anymore, `SRIOVLiveMigration` feature-gate is redundant. SR-IOV live migration will be controlled by `LiveMigration` feature-gate similar to non-SR-IOV VMs. [1] kubevirt#5558 Signed-off-by: Or Mergi <ormergi@redhat.com>

Following [1] virt-launcher doesn't execute with the `SYS_RESOURCE` capability anymore, `SRIOVLiveMigration` feature-gate is redundant. [1] kubevirt#5558 Signed-off-by: Or Mergi <ormergi@redhat.com>

kubevirt-bot requested review from maiqueb, rmohr and rnetser April 28, 2021 17:52

kubevirt-bot added the size/XXL label Apr 28, 2021

kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 28, 2021

ormergi marked this pull request as ready for review April 28, 2021 19:53

ormergi marked this pull request as draft April 28, 2021 19:55

ormergi force-pushed the drop-sys-resource-cap branch from 168076a to adf265c Compare April 29, 2021 07:39

ormergi force-pushed the drop-sys-resource-cap branch from adf265c to c9016f0 Compare April 29, 2021 07:45

kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 29, 2021

ormergi marked this pull request as ready for review April 29, 2021 08:46

kubevirt-bot requested a review from EdDev April 29, 2021 08:46

EdDev suggested changes May 2, 2021

View reviewed changes

kubevirt-bot assigned EdDev May 2, 2021

EdDev reviewed May 2, 2021

View reviewed changes

EdDev suggested changes May 2, 2021

View reviewed changes

EdDev reviewed May 2, 2021

View reviewed changes

oshoval reviewed May 2, 2021

View reviewed changes

vladikr reviewed May 3, 2021

View reviewed changes

kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 4, 2021

ormergi requested a review from vladikr May 24, 2021 14:48

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 26, 2021

kubevirt-bot assigned vladikr May 26, 2021

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label May 26, 2021

kubevirt-bot merged commit b723dac into kubevirt:master May 26, 2021

ormergi mentioned this pull request Jun 4, 2021

Enable SRIOVLiveMigration feature-gate by default kubevirt/hyperconverged-cluster-operator#1375

Closed

11 tasks

nunnatsa mentioned this pull request Jun 16, 2021

Enable SRIOVLiveMigration feature-gate by default kubevirt/hyperconverged-cluster-operator#1401

Merged

11 tasks

ormergi deleted the drop-sys-resource-cap branch October 10, 2021 12:34

ormergi mentioned this pull request Dec 1, 2021

CNV-12277: Removed mention of sriovLiveMigration feature gate openshift/openshift-docs#38918

Merged

ormergi mentioned this pull request Jun 27, 2022

Remove SRIOVLiveMigration feature-gate #7990

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop SYS_RESOURCE capability #5558

Drop SYS_RESOURCE capability #5558

ormergi commented Apr 28, 2021 •

edited

kubevirt-bot commented Apr 28, 2021

ormergi commented Apr 28, 2021

ormergi commented Apr 29, 2021

ormergi commented Apr 29, 2021

ormergi commented Apr 29, 2021

EdDev left a comment

EdDev May 2, 2021

ormergi May 2, 2021

ormergi May 2, 2021

EdDev May 2, 2021

EdDev left a comment

EdDev left a comment

EdDev May 2, 2021

ormergi May 2, 2021 •

edited

EdDev May 2, 2021

EdDev May 2, 2021

ormergi May 2, 2021 •

edited

EdDev May 2, 2021

ormergi May 5, 2021

oshoval May 2, 2021

vladikr May 3, 2021 •

edited

ormergi May 4, 2021 •

edited

vladikr May 4, 2021

vladikr May 3, 2021

vladikr May 3, 2021

vladikr May 3, 2021

ormergi May 4, 2021

vladikr commented May 26, 2021

vladikr commented May 26, 2021

kubevirt-bot commented May 26, 2021

kubevirt-bot commented May 26, 2021 •

edited

vladikr commented May 26, 2021

Drop SYS_RESOURCE capability #5558

Drop SYS_RESOURCE capability #5558

Conversation

ormergi commented Apr 28, 2021 • edited

kubevirt-bot commented Apr 28, 2021

ormergi commented Apr 28, 2021

ormergi commented Apr 29, 2021

ormergi commented Apr 29, 2021

ormergi commented Apr 29, 2021

EdDev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EdDev left a comment

Choose a reason for hiding this comment

EdDev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ormergi May 2, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ormergi May 2, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladikr May 3, 2021 • edited

Choose a reason for hiding this comment

ormergi May 4, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladikr commented May 26, 2021

vladikr commented May 26, 2021

kubevirt-bot commented May 26, 2021

kubevirt-bot commented May 26, 2021 • edited

vladikr commented May 26, 2021

ormergi commented Apr 28, 2021 •

edited

ormergi May 2, 2021 •

edited

ormergi May 2, 2021 •

edited

vladikr May 3, 2021 •

edited

ormergi May 4, 2021 •

edited

kubevirt-bot commented May 26, 2021 •

edited