Basic guest vCPUs to pCPUs pinning #1381

vladikr · 2018-07-27T06:16:07Z

What this PR does / why we need it:
Replying on k8s CPUManager feature, this PR implements a basic guest vCPUs to a dedicated pods' pCPUs pinning.

In general, k8s CPU manager will pin pods' CPUs under certain conditions:

pods' QOS is Guaranteed
- resources requests and limits are equal
- all containers in the pod express cpu and memory resources
requested number of CPUs in integer

Once pods' cpus are dedicated and pinned, virt-launcher can pin VMs' vCPUS to the pods pCPUS

This PR also provides a mechanism that once dedicatedCpuPlacement policy has been requested, it will "enforce" the above conditions on the pod spec, so the k8s cpu manager will pin pods' cpus and reject inconsistent requirements.

The user may either express the vCPU requirements using cpu.cores to resources.[requests, limits].cpu
togethre with cpu.dedicatedCpuPlacement = true

spec:
  domain:
    cpu:
      cores: 2
      dedicatedCpuPlacement: true
    resources:                                                                                                                                                                                                     
      limits:
        memory: 2Gi

OR

spec:
  domain:
    cpu:
      dedicatedCpuPlacement: true
    resources:                                                                                                                                                                                                     
      limits:
        cpu: 2                                                                 
        memory: 2Gi

Special notes for your reviewer:
What's currently missing:

Functional tests to verify scheduling
validation webhooks
Defaults CPU/memory defaults for hooks

Functional tests are dependant on kubevirt/kubevirtci#37

Release note:

Pinning vCPU to a set of pCPUs is now possible

yanirq

Validations will be added to check that the VM pod resources and limits will conform with Guaranteed QoS class?

yanirq · 2018-08-09T10:08:43Z

pkg/registry-disk/registry-disk.go

-
+			resources := kubev1.ResourceRequirements{}
+			resources.Limits = make(kubev1.ResourceList)
+			resources.Limits[kubev1.ResourceCPU] = resource.MustParse("50m")


How was this value decided ?

hmm, just needed to start somewhere.
This container only mounts a path. - I'll make it a global default variable

cynepco3hahue

I like that we can introduce CPU pinning, but in my opinion it very limited, we still do not give any possibility to a user to control exact CPU pinning.

cynepco3hahue · 2018-08-09T12:08:01Z

pkg/virt-handler/vm.go

+func (d *VirtualMachineController) addNodeCpuManagerLabel() {
+	entries, err := filepath.Glob("/proc/*/cmdline")
+	if err != nil {
+		log.DefaultLogger().Reason(err).Errorf("failed to set a cpu manager label on host %s", d.host)


Can you make more specific messages? It will be easier to track it on debug

cynepco3hahue · 2018-08-09T12:12:09Z

pkg/virt-handler/vm.go

@@ -22,8 +22,11 @@ package virthandler
 import (


We need some renaming under this file from VM to VMI, but it must not be a part of the PR 😄

cynepco3hahue · 2018-08-09T12:12:56Z

pkg/virt-handler/vm.go

@@ -776,6 +783,33 @@ func (d *VirtualMachineController) heartBeat(interval time.Duration, stopCh chan
 	}
 }

+func (d *VirtualMachineController) addNodeCpuManagerLabel() {
+	entries, err := filepath.Glob("/proc/*/cmdline")


How many time can it take on some loaded machine? (20k process)

I don't 20k processes running anywhere. on my machine it with ~1k processes it took the following, but that also includes go compilation and getting its' stuck up:

time go run trym22.go real 0m0.143s user 0m0.115s sys 0m0.058s

I don't believe it should be a problem, however, this is just a workaround at the moment, until k8s addresses the issue: kubernetes/kubernetes#66525

cynepco3hahue · 2018-08-09T12:15:19Z

pkg/virt-launcher/virtwrap/cpu_utils.go

+	"kubevirt.io/kubevirt/pkg/virt-launcher/virtwrap/api"
+)
+
+const CPUSET_PATH = "/sys/fs/cgroup/cpuset/cpuset.cpus"


I think it must be lower case if you do not want to use it outside of the package.

vladikr · 2018-08-09T13:36:19Z

I like that we can introduce CPU pinning, but in my opinion it very limited, we still do not give any possibility to a user to control exact CPU pinning.

Thanks for the review!

Yea, it's because we have to rely on the k8s CPU manager, which is limited...
Hope it'll improve in the future or we'll come up with a better approach.
There was a survey from k8s that samples which cpu related features people are interested in:
https://docs.google.com/forms/d/e/1FAIpQLSest47hzQyKO_ENEbAsyZz_78aPCDpuToHjsyg7n75fF70ArA/viewanalytics

cynepco3hahue · 2018-08-13T14:30:31Z

pkg/virt-controller/services/template.go

+			resources.Limits[k8sv1.ResourceCPU] = *resource.NewQuantity(int64(cpus.Cores), resource.BinarySI)
+			resources.Requests[k8sv1.ResourceCPU] = *resource.NewQuantity(int64(cpus.Cores), resource.BinarySI)
+		} else {
+			if cpuLimit, ok := resources.Limits[k8sv1.ResourceCPU]; ok {


I just wonder if we need to check that resources can be converted to an integer because a user can to specify

cpus.DedicatedCPUPlacement

resources.Limits equal to something like 200m

In this case I think you will fail in converter, so better to add this check to validation webhook

Absolutely. I've added several validations but didn't post yet as I'm still struggling with the functional tests.

Also, I wanted to explicitly check, somewhere in the SyncVM that the pods QOS is Guaranteed, but didn't figure out yet how to get to pods informer at that stage.

vladikr · 2018-08-13T17:02:06Z

Added validations, still working on the functional tests.

yanirq · 2018-08-14T08:49:09Z

pkg/virt-api/validating-webhook/validating-webhook_test.go

+		})
+		It("should reject specs with non-integer cpu limits values", func() {
+			vmi.Spec.Domain.Resources.Limits = k8sv1.ResourceList{
+				k8sv1.ResourceCPU: resource.MustParse("800m"),


maybe add another negative test with a fraction value (cpu = 1.5) not in milli format.

@yanirq you don't trust resource.MustParse() to handle it? ;)

@vladikr if it does so i do :)

dcritch · 2018-08-15T14:46:19Z

I've been testing CPU manager as is with kubevirt and its working pretty good. Trying to understand what this change will improve. Seems like we can specify particular CPUs? Currently with CPU manager, the VM is pinned on whatever CPU the process happens to start on, but can't be specified.