Merge pull request kubevirt#6251 from rmohr/cpu-pinning

Ensure optimal CPU pinning with dedicated CPUs
akalenyu · Sep 10, 2021 · b9ee32a · b9ee32a
2 parents 7324bbc + c3f8d20
commit b9ee32a
Show file tree

Hide file tree

Showing 23 changed files with 936 additions and 156 deletions.
diff --git a/pkg/handler-launcher-com/cmd/v1/cmd.pb.go b/pkg/handler-launcher-com/cmd/v1/cmd.pb.go
diff --git a/pkg/handler-launcher-com/cmd/v1/cmd.proto b/pkg/handler-launcher-com/cmd/v1/cmd.proto
@@ -33,6 +33,7 @@ message VMI {
 
 message CPU {
   uint32 id = 1;
+  repeated uint32 siblings = 2;
 }
 
 message Sibling {

diff --git a/pkg/util/hardware/hw_utils.go b/pkg/util/hardware/hw_utils.go
@@ -34,7 +34,7 @@ const (
 
 // Parse linux cpuset into an array of ints
 // See: http://man7.org/linux/man-pages/man7/cpuset.7.html#FORMATS
-func ParseCPUSetLine(cpusetLine string) (cpusList []int, err error) {
+func ParseCPUSetLine(cpusetLine string, limit int) (cpusList []int, err error) {
 	elements := strings.Split(cpusetLine, ",")
 	for _, item := range elements {
 		cpuRange := strings.Split(item, "-")
@@ -50,19 +50,30 @@ func ParseCPUSetLine(cpusetLine string) (cpusList []int, err error) {
 			}
 			// Add cpus to the list. Assuming it's a valid range.
 			for cpuNum := start; cpuNum <= end; cpuNum++ {
-				cpusList = append(cpusList, cpuNum)
+				if cpusList, err = safeAppend(cpusList, cpuNum, limit); err != nil {
+					return nil, err
+				}
 			}
 		} else {
 			cpuNum, err := strconv.Atoi(cpuRange[0])
 			if err != nil {
 				return nil, err
 			}
-			cpusList = append(cpusList, cpuNum)
+			if cpusList, err = safeAppend(cpusList, cpuNum, limit); err != nil {
+				return nil, err
+			}
 		}
 	}
 	return
 }
 
+func safeAppend(cpusList []int, cpu int, limit int) ([]int, error) {
+	if len(cpusList) > limit {
+		return nil, fmt.Errorf("rejecting expanding CPU array for safety reasons, limit is %v", limit)
+	}
+	return append(cpusList, cpu), nil
+}
+
 //GetNumberOfVCPUs returns number of vCPUs
 //It counts sockets*cores*threads
 func GetNumberOfVCPUs(cpuSpec *v1.CPU) int64 {

diff --git a/pkg/util/hardware/hw_utils_test.go b/pkg/util/hardware/hw_utils_test.go
@@ -32,11 +32,18 @@ var _ = Describe("Hardware utils test", func() {
 		It("shoud parse cpuset correctly", func() {
 			expectedList := []int{0, 1, 2, 7, 12, 13, 14}
 			cpusetLine := "0-2,7,12-14"
-			lst, err := ParseCPUSetLine(cpusetLine)
+			lst, err := ParseCPUSetLine(cpusetLine, 100)
 			Expect(err).ToNot(HaveOccurred())
 			Expect(len(lst)).To(Equal(7))
 			Expect(lst).To(Equal(expectedList))
 		})
+
+		It("should reject expanding arbitrary ranges which would overload a machine", func() {
+			cpusetLine := "0-100000000000"
+			_, err := ParseCPUSetLine(cpusetLine, 100)
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("safety"))
+		})
 	})
 
 	Context("count vCPUs", func() {

diff --git a/pkg/virt-api/webhooks/validating-webhook/admitters/vmi-create-admitter.go b/pkg/virt-api/webhooks/validating-webhook/admitters/vmi-create-admitter.go
@@ -981,6 +981,7 @@ func validateCpuPinning(field *k8sfield.Path, spec *v1.VirtualMachineInstanceSpe
 		causes = append(causes, validateRequestLimitOrCoresProvidedOnDedicatedCPUPlacement(field, spec)...)
 		causes = append(causes, validateRequestEqualsLimitOnDedicatedCPUPlacement(field, spec)...)
 		causes = append(causes, validateRequestOrLimitWithCoresProvidedOnDedicatedCPUPlacement(field, spec)...)
+		causes = append(causes, validateThreadCountOnDedicatedCPUPlacement(field, spec)...)
 	}
 	return causes
 }
@@ -1019,6 +1020,20 @@ func validateNUMA(field *k8sfield.Path, spec *v1.VirtualMachineInstanceSpec, con
 	return causes
 }
 
+func validateThreadCountOnDedicatedCPUPlacement(field *k8sfield.Path, spec *v1.VirtualMachineInstanceSpec) (causes []metav1.StatusCause) {
+	if spec.Domain.CPU != nil && spec.Domain.CPU.Threads > 2 {
+		causes = append(causes, metav1.StatusCause{
+			Type: metav1.CauseTypeFieldValueInvalid,
+			Message: fmt.Sprintf("Not more than two threads must be provided at %v (got %v) when DedicatedCPUPlacement is true",
+				field.Child("domain", "cpu", "threads").String(),
+				spec.Domain.CPU.Threads,
+			),
+			Field: field.Child("domain", "cpu", "dedicatedCpuPlacement").String(),
+		})
+	}
+	return causes
+}
+
 func validateRequestOrLimitWithCoresProvidedOnDedicatedCPUPlacement(field *k8sfield.Path, spec *v1.VirtualMachineInstanceSpec) (causes []metav1.StatusCause) {
 	if (spec.Domain.Resources.Requests.Cpu().Value() > 0 || spec.Domain.Resources.Limits.Cpu().Value() > 0) && hwutil.GetNumberOfVCPUs(spec.Domain.CPU) > 0 &&
 		spec.Domain.Resources.Requests.Cpu().Value() != hwutil.GetNumberOfVCPUs(spec.Domain.CPU) && spec.Domain.Resources.Limits.Cpu().Value() != hwutil.GetNumberOfVCPUs(spec.Domain.CPU) {

diff --git a/pkg/virt-api/webhooks/validating-webhook/admitters/vmi-create-admitter_test.go b/pkg/virt-api/webhooks/validating-webhook/admitters/vmi-create-admitter_test.go
@@ -2276,6 +2276,18 @@ var _ = Describe("Validating VMICreate Admitter", func() {
 			causes := ValidateVirtualMachineInstanceSpec(k8sfield.NewPath("fake"), &vmi.Spec, config)
 			Expect(causes).To(BeEmpty())
 		})
+		It("should reject specs with more than two threads", func() {
+			vmi.Spec.Domain.Memory = &v1.Memory{Hugepages: &v1.Hugepages{PageSize: "2Mi"}}
+			vmi.Spec.Domain.CPU.Cores = 4
+			vmi.Spec.Domain.CPU.Threads = 3
+			vmi.Spec.Domain.CPU.NUMA = &v1.NUMA{GuestMappingPassthrough: &v1.NUMAGuestMappingPassthrough{}}
+			vmi.Spec.Domain.Resources.Limits = k8sv1.ResourceList{
+				k8sv1.ResourceCPU: resource.MustParse("12"),
+			}
+			causes := ValidateVirtualMachineInstanceSpec(k8sfield.NewPath("fake"), &vmi.Spec, config)
+			Expect(causes).To(HaveLen(1))
+			Expect(causes[0].Message).To(ContainSubstring("Not more than two threads must be provided at fake.domain.cpu.threads (got 3) when DedicatedCPUPlacement is true"))
+		})
 		It("should reject specs without cpu reqirements", func() {
 			causes := ValidateVirtualMachineInstanceSpec(k8sfield.NewPath("fake"), &vmi.Spec, config)
 			Expect(len(causes)).To(Equal(1))