Fixes BZ-2123209: Real time VMs fail to change vCPU scheduler and priority in non-root deployments #8750

jordigilh · 2022-11-09T16:17:52Z

This PR addresses an issue found during the deployment of real time workloads in non-root deployments where libvirt was unable to change the scheduling and priorities of the vCPU threads due to lack of CAP_SYS_NICE.
The solution proposed here is to re-implement the logic that changes the scheduling and priority in virt-handler for non-root deployments. The old logic that adds the libvirt XML knobs remains there for root based deployments.

Release note:

Fixes an issue that prevented running real time workloads in non-root configurations due to libvirt's dependency on CAP_SYS_NICE to change the vcpu's thread's scheduling and priority to FIFO and 1. The change of priority and scheduling is now executed in the virt-launcher for both root and non-root configurations, removing the dependency in libvirt.

…e <cpusched> knobs when running in a non-root environment or the VM labeled as non-root Signed-off-by: Jordi Gil <jgil@redhat.com>

… would fail during the manifest validation Signed-off-by: Jordi Gil <jgil@redhat.com>

…t environment Signed-off-by: Jordi Gil <jgil@redhat.com>

jordigilh · 2022-11-16T21:27:47Z

@vladikr @iholder-redhat @rmohr PTAL.

jordigilh · 2022-11-16T21:29:06Z

The old logic that adds the libvirt XML knobs remains there for root based deployments.

I'm unsure whether to keep the old logic or not. Preferably it would be better to stick to one logic (this PR's implementation for both non-root and root use cases), but I'd like your input on this.

rmohr · 2022-11-17T09:35:15Z

I'm unsure whether to keep the old logic or not. Preferably it would be better to stick to one logic (this PR's implementation for both non-root and root use cases), but I'd like your input on this.

Does this mean that with your change we could drop CAP_SYS_NICE for root and non-root? If the logic you introduce works for both cases, yep let's converge :)

rmohr · 2022-11-17T09:32:57Z

pkg/virt-handler/isolation/detector.go

 	}
 	log.Log.V(5).Object(vmi).Infof("set process %+v memlock rlimits to: Cur: %[2]d Max:%[2]d",
 		qemuProcess, memlockSize.Value())

 	return nil
 }

+// GetQEMUProcess encapsulates and exposes the logic to retrieve the QEMU process ID
+func GetQEMUProcess(parentPID int) (ps.Process, error) {


Looks to me like this should be more of a method on the isolation result, than a general purpose method where you pass in an arbitrary pid?

Makes sense. I will move this to be a func method to IsolationResult

rmohr · 2022-11-17T09:35:46Z

pkg/virt-api/webhooks/validating-webhook/admitters/vmi-create-admitter.go

-				Field: field.Child("domain", "cpu", "dedicatedCpuPlacement").String(),
-			})
-		}
+		// if nonroot {


oops.. good catch! 😄

iholder101

Awesome work @jordigilh! +1!
Left mostly nits and small comments

iholder101 · 2022-11-17T11:59:12Z

pkg/virt-handler/realtime.go

+type SchedParam C.sched_param
+type Policy uint32


Can these be unexposed?

Suggested change

type SchedParam C.sched_param

type Policy uint32

type schedParam C.sched_param

type policy uint32

Absolutely!. I will change their scope 😄

iholder101 · 2022-11-17T12:16:06Z

pkg/virt-handler/realtime.go

+// except that in this case it uses a map[string]maskType instead of a bit array.
+func parseCPUMask(mask string) (map[string]maskType, error) {
+
+	if len(strings.TrimSpace(mask)) == 0 {


nit:

Suggested change

if len(strings.TrimSpace(mask)) == 0 {

if strings.TrimSpace(mask) != "" {

Looking at the code I should return an empty map if the mask is either empty or full of spaces. Only return error if the parsing fails.

vcpus := make(map[string]maskType) if strings.TrimSpace(mask) == "" { return vcpus, nil }

The reason is that mask is a string and will never be nil, so it should always treat the empty case as default.

Agreed!
Just one note, you can return a nil map instead of an initialized one. It would save the space of allocating an empty map. isRealtimeVCPU() will work with that as the len of a nil map is 0.

if strings.TrimSpace(mask) == "" { return nil, nil }

great, thanks for the suggestion! 😄

After further thought I think a mask filled with spaces should be considered invalid, compared with an empty string.

if len(mask) == 0 { return nil, nil }

So basically:

Entry("Empty mask", "", nil, nil), Entry("Empty mask with spaces", " ", nil, fmt.Errorf("invalid mask value ' ' in ' '")),

WDYT?

iholder101 · 2022-11-17T12:19:55Z

pkg/virt-handler/realtime.go

+			if startID < 0 {
+				return nil, fmt.Errorf("invalid vcpu mask start index `%d`", startID)
+			}
+			if endID < 0 {


Can endID be zero? Maybe switch with endID <= 0?

yeah, good point. I will add a test case to cover this 0-0 range as well.

after adding the test it seems to be working already as it is:

Entry("Correctly extracts a single range with one core", "0-0", map[string]maskType{"0": enabled}, nil),

Are you OK to leave the logic as it is or you'd feel more comfortable changing it as you initially suggested?

Interesting.
Well, as long as it works + covered with a unit test I guess we're fine :)

iholder101 · 2022-11-17T12:27:20Z

pkg/virt-handler/realtime.go

+func schedSetScheduler(pid int, policy Policy, param SchedParam) error {
+	_, _, e1 := unix.Syscall(unix.SYS_SCHED_SETSCHEDULER, uintptr(pid), uintptr(policy), uintptr(unsafe.Pointer(&param)))
+	if e1 != 0 {
+		return syscall.Errno(e1)


nit: cast is unnecessary

Suggested change

return syscall.Errno(e1)

return e1

iholder101 · 2022-11-17T12:33:37Z

pkg/virt-handler/realtime_test.go

+	. "github.com/onsi/gomega"
+)
+
+var _ = Describe("Set pthread scheduling type and priority", func() {


nit: I suggest to have one Describe with different Contexts inside so it would look a bit clearer

I'm not following you here. This Describe section has only 5 entries/tests to run in the same context. Are you talking in general or just to this Describe section?

In general, I'm not sure why we need 3 deffierent Describes, each with a Context and a DescribeTable. I'd go with one Describe and 3 Contexts.

But of course it's a styling manner and might be subjective. Feel free to ignore if you don't feel the same as I :)

Sounds good, makes sense to me. I will migrate the Describes into Context and add a top Describe.

done, please take a look.

iholder101 · 2022-11-17T12:35:33Z

pkg/virt-handler/realtime_test.go

+
+var _ = Describe("Set pthread scheduling type and priority", func() {
+	Context("when parsing the thread command for CPU ID", func() {
+		DescribeTable("extracts the CPU ID", func(comm []byte, cpuID string, parseOK bool) {


nits:

would change comm []byte to comm string and cast inside

cpuID -> expectedCpuID, parseOK -> expectParseOK

sounds good

one thing is that I need it to be []byte inside the function:

kubevirt/pkg/virt-handler/realtime.go

Line 100 in 315a5fb

v := vcpuRegex.FindSubmatch(comm)

So casting it to string when the source is already a byte array and then recasting it to []byte doesn't sound that good. WDYT?

Yeah, that's a good point.

jordigilh · 2022-11-17T12:48:41Z

I'm unsure whether to keep the old logic or not. Preferably it would be better to stick to one logic (this PR's implementation for both non-root and root use cases), but I'd like your input on this.

Does this mean that with your change we could drop CAP_SYS_NICE for root and non-root? If the logic you introduce works for both cases, yep let's converge :)

Terrific ... I will remove the old logic 😄

iholder101 · 2022-12-01T14:20:21Z

@jordigilh seems like a bazel file isn't configured correctly. make should solve that

Signed-off-by: Jordi Gil <jgil@redhat.com>

iholder101 · 2022-12-01T15:18:23Z

/lgtm
/hold cancel

thanks @jordigilh!

jordigilh · 2022-12-01T15:47:16Z

/test pull-kubevirt-e2e-k8s-1.24-sig-compute-realtime

jordigilh · 2022-12-01T15:47:22Z

/test pull-kubevirt-e2e-k8s-1.24-sig-compute-realtime-root

jordigilh · 2022-12-01T19:57:39Z

/retest

jordigilh · 2022-12-01T20:39:25Z

/retest

jordigilh · 2022-12-01T23:59:04Z

/retest

jordigilh · 2022-12-02T01:21:18Z

/retest

kubevirt-commenter-bot · 2022-12-02T06:54:52Z

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

jordigilh · 2022-12-02T11:59:27Z

/retest

EdDev · 2022-12-04T15:01:17Z

Hello there,

Unfortunately, this PR seems to have broken my local setup from building/running the e2e test binary.
The change here required a CGO linkage at the virt-handler package.

Sadly, this in turn exposed a nasty dependency from the e2e tests to the package here.

We seem to have several issues we better fix soon:

Avoid CGO linkage on virt-handler except for very special cases. I am not sure if this was discussed here directly.
Do not depend in the e2e test on internal packages which are not dedicated to be formal API/s. It is preferred to have duplication over such dependency.
Add a build job for the e2e test, which can detect such issues. Or any other alternative to try and avoid such cases in the future.

I will try to send a quick fix later today, but it is a small workaround, not fixing the core problem.
I hope we could avoid the C binding here.

EdDev · 2022-12-04T15:56:13Z

For completeness, the errors seen while building the e2e tests are these:

/lib64/libc.so.6: version `GLIBC_2.32'
/lib64/libc.so.6: version `GLIBC_2.34'

xpivarc · 2023-02-01T12:45:47Z

/cherry-pick release-0.58

kubevirt-bot · 2023-02-01T12:46:29Z

@xpivarc: #8750 failed to apply on top of branch "release-0.58":

Applying: Disabled admission check for real time manifest and avoid defining the <cpusched> knobs when running in a non-root environment or the VM labeled as non-root
Using index info to reconstruct a base tree...
M	pkg/virt-api/webhooks/validating-webhook/admitters/vmi-create-admitter.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/virt-api/webhooks/validating-webhook/admitters/vmi-create-admitter.go
CONFLICT (content): Merge conflict in pkg/virt-api/webhooks/validating-webhook/admitters/vmi-create-admitter.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Disabled admission check for real time manifest and avoid defining the <cpusched> knobs when running in a non-root environment or the VM labeled as non-root
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-0.58

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.

kubevirt-bot added dco-signoff: no Indicates the PR's author has not DCO signed all their commits. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. size/L labels Nov 9, 2022

jordigilh changed the title ~~[DRAFT] Fixes BG2123209: Realtime VMs fail to change vCPU scheduler and priority in non-root deployments~~ [DRAFT] Fixes BZ2123209: Realtime VMs fail to change vCPU scheduler and priority in non-root deployments Nov 9, 2022

jordigilh changed the title ~~[DRAFT] Fixes BZ2123209: Realtime VMs fail to change vCPU scheduler and priority in non-root deployments~~ [DRAFT] Fixes BZ-2123209: Realtime VMs fail to change vCPU scheduler and priority in non-root deployments Nov 9, 2022

kubevirt-bot requested review from marceloamaral and stu-gott November 9, 2022 16:18

jordigilh force-pushed the fix/BG2123209_RT_VM_nonroot_fail_bootup branch 2 times, most recently from 8e2ae88 to e47f95f Compare November 9, 2022 17:32

kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. and removed dco-signoff: no Indicates the PR's author has not DCO signed all their commits. labels Nov 9, 2022

jordigilh changed the title ~~[DRAFT] Fixes BZ-2123209: Realtime VMs fail to change vCPU scheduler and priority in non-root deployments~~ [DRAFT] Fixes BZ-2123209: Real time VMs fail to change vCPU scheduler and priority in non-root deployments Nov 9, 2022

jordigilh force-pushed the fix/BG2123209_RT_VM_nonroot_fail_bootup branch from e47f95f to 092ea36 Compare November 9, 2022 17:45

jordigilh force-pushed the fix/BG2123209_RT_VM_nonroot_fail_bootup branch from 092ea36 to ed8058e Compare November 16, 2022 19:33

kubevirt-bot added dco-signoff: no Indicates the PR's author has not DCO signed all their commits. and removed dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Nov 16, 2022

jordigilh changed the title ~~[DRAFT] Fixes BZ-2123209: Real time VMs fail to change vCPU scheduler and priority in non-root deployments~~ Fixes BZ-2123209: Real time VMs fail to change vCPU scheduler and priority in non-root deployments Nov 16, 2022

jordigilh force-pushed the fix/BG2123209_RT_VM_nonroot_fail_bootup branch from ed8058e to efb6134 Compare November 16, 2022 21:21

jordigilh added 3 commits November 16, 2022 16:24

Disabled admission check for real time manifest and avoid defining th…

eb295e7

…e <cpusched> knobs when running in a non-root environment or the VM labeled as non-root Signed-off-by: Jordi Gil <jgil@redhat.com>

Removed test to validate real time workloads in a non-root deployment…

a1b37a4

… would fail during the manifest validation Signed-off-by: Jordi Gil <jgil@redhat.com>

[BZ-2123209] Add support for Real Time workloads running in a non-roo…

315a5fb

…t environment Signed-off-by: Jordi Gil <jgil@redhat.com>

jordigilh force-pushed the fix/BG2123209_RT_VM_nonroot_fail_bootup branch from efb6134 to 315a5fb Compare November 16, 2022 21:25

kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. and removed dco-signoff: no Indicates the PR's author has not DCO signed all their commits. labels Nov 16, 2022

rmohr reviewed Nov 17, 2022

View reviewed changes

iholder101 reviewed Nov 17, 2022

View reviewed changes

kubevirt-bot added dco-signoff: no Indicates the PR's author has not DCO signed all their commits. size/L and removed dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/XL labels Dec 1, 2022

jordigilh force-pushed the fix/BG2123209_RT_VM_nonroot_fail_bootup branch from 9b4c891 to ada476f Compare December 1, 2022 13:38

kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. and removed dco-signoff: no Indicates the PR's author has not DCO signed all their commits. labels Dec 1, 2022

jordigilh force-pushed the fix/BG2123209_RT_VM_nonroot_fail_bootup branch from ada476f to 29f371c Compare December 1, 2022 14:37

Single setsched.go file for both amd64 and arm64

f365f1d

Signed-off-by: Jordi Gil <jgil@redhat.com>

jordigilh force-pushed the fix/BG2123209_RT_VM_nonroot_fail_bootup branch from 29f371c to f365f1d Compare December 1, 2022 14:37

kubevirt-bot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Dec 1, 2022

kubevirt-bot merged commit 458a768 into kubevirt:main Dec 2, 2022

EdDev mentioned this pull request Dec 4, 2022

virt-handler: Replace CGO linking with a Go alternative #8903

Merged

xpivarc mentioned this pull request Feb 6, 2023

[release-0.58] RT fixes #9174

Merged

jordigilh mentioned this pull request Mar 14, 2023

Add RuntimeClassName to vmi spec #9402

Closed

	if len(strings.TrimSpace(mask)) == 0 {
	if strings.TrimSpace(mask) != "" {

Fixes BZ-2123209: Real time VMs fail to change vCPU scheduler and priority in non-root deployments #8750

Fixes BZ-2123209: Real time VMs fail to change vCPU scheduler and priority in non-root deployments #8750

Conversation

jordigilh commented Nov 9, 2022 • edited

jordigilh commented Nov 16, 2022

jordigilh commented Nov 16, 2022

rmohr commented Nov 17, 2022

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

iholder101 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

jordigilh Nov 17, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jordigilh Nov 17, 2022 • edited

Choose a reason for hiding this comment

iholder101 Nov 17, 2022 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jordigilh commented Nov 17, 2022

iholder101 commented Dec 1, 2022

iholder101 commented Dec 1, 2022

jordigilh commented Dec 1, 2022

jordigilh commented Dec 1, 2022

jordigilh commented Dec 1, 2022

jordigilh commented Dec 1, 2022

jordigilh commented Dec 1, 2022

jordigilh commented Dec 2, 2022

kubevirt-commenter-bot commented Dec 2, 2022

jordigilh commented Dec 2, 2022

EdDev commented Dec 4, 2022

EdDev commented Dec 4, 2022 • edited

xpivarc commented Feb 1, 2023

kubevirt-bot commented Feb 1, 2023

jordigilh commented Nov 9, 2022 •

edited

jordigilh Nov 17, 2022 •

edited

jordigilh Nov 17, 2022 •

edited

iholder101 Nov 17, 2022 •

edited

EdDev commented Dec 4, 2022 •

edited