Skip to content

Commit

Permalink
Merge pull request kubevirt#10872 from RamLavi/change_pcpu_to_global
Browse files Browse the repository at this point in the history 
IsolateEmulatorThread: Add cluster-wide parity completion setting
  • Loading branch information
@kubevirt-bot
kubevirt-bot authored Dec 19, 2023
2 parents ef91d5a + 99c4ec6 commit 33e6143
Show file tree
Hide file tree
Showing 12 changed files with 105 additions and 77 deletions.
12 changes: 12 additions & 0 deletions pkg/virt-api/webhooks/mutating-webhook/mutators/vmi-mutator.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,18 @@ func (mutator *VMIsMutator) Mutate(ar *admissionv1.AdmissionReview) *admissionv1
addNodeSelector(newVMI, v1.SEVESLabel)
}

if newVMI.Spec.Domain.CPU.IsolateEmulatorThread {
_, emulatorThreadCompleteToEvenParityAnnotationExists := mutator.ClusterConfig.GetConfigFromKubeVirtCR().Annotations[v1.EmulatorThreadCompleteToEvenParity]
if emulatorThreadCompleteToEvenParityAnnotationExists &&
mutator.ClusterConfig.AlignCPUsEnabled() {
log.Log.V(4).Infof("Copy %s annotation from Kubevirt CR", v1.EmulatorThreadCompleteToEvenParity)
if newVMI.Annotations == nil {
newVMI.Annotations = map[string]string{}
}
newVMI.Annotations[v1.EmulatorThreadCompleteToEvenParity] = ""
}
}

// Add foreground finalizer
newVMI.Finalizers = append(newVMI.Finalizers, v1.VirtualMachineInstanceFinalizer)

Expand Down
48 changes: 48 additions & 0 deletions pkg/virt-api/webhooks/mutating-webhook/mutators/vmi-mutator_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -530,6 +530,54 @@ var _ = Describe("VirtualMachineInstance Mutator", func() {
Expect(vmiSpec.Domain.Resources.Requests.Memory()).To(Equal(vmi.Spec.Domain.Resources.Requests.Memory()))
})

DescribeTable("should not copy the EmulatorThreadCompleteToEvenParity annotation to the VMI",
func(featureGate string, annotations map[string]string, isolateEmulatorThread bool) {
if featureGate != "" || annotations != nil {
testutils.UpdateFakeKubeVirtClusterConfig(kvInformer, &v1.KubeVirt{
ObjectMeta: k8smetav1.ObjectMeta{
Annotations: annotations,
},
Spec: v1.KubeVirtSpec{
Configuration: v1.KubeVirtConfiguration{
DeveloperConfiguration: &v1.DeveloperConfiguration{
FeatureGates: []string{featureGate},
},
},
},
})
}
vmi.Spec.Domain.CPU = &v1.CPU{IsolateEmulatorThread: isolateEmulatorThread}

vmiMeta, _, _ := getMetaSpecStatusFromAdmit(vmi.Spec.Architecture)
_, exist := vmiMeta.Annotations[v1.EmulatorThreadCompleteToEvenParity]
Expect(exist).To(BeFalse())
},
Entry("when the AlignCPUs featureGate is disabled", "", map[string]string{v1.EmulatorThreadCompleteToEvenParity: ""}, true),
Entry("when the EmulatorThreadCompleteToEvenParity annotation is not set on the kubevirt CR", virtconfig.AlignCPUsGate, nil, true),
Entry("when isolateEmulatorThread is disabled on the VMI spec", virtconfig.AlignCPUsGate, map[string]string{v1.EmulatorThreadCompleteToEvenParity: ""}, false),
)

It("should copy the EmulatorThreadCompleteToEvenParity annotation to the VMI", func() {
testutils.UpdateFakeKubeVirtClusterConfig(kvInformer, &v1.KubeVirt{
ObjectMeta: k8smetav1.ObjectMeta{
Annotations: map[string]string{v1.EmulatorThreadCompleteToEvenParity: ""},
},
Spec: v1.KubeVirtSpec{
Configuration: v1.KubeVirtConfiguration{
DeveloperConfiguration: &v1.DeveloperConfiguration{
FeatureGates: []string{virtconfig.AlignCPUsGate},
},
},
},
})

vmi.Spec.Domain.CPU = &v1.CPU{IsolateEmulatorThread: true}

vmiMeta, _, _ := getMetaSpecStatusFromAdmit(vmi.Spec.Architecture)
_, exist := vmiMeta.Annotations[v1.EmulatorThreadCompleteToEvenParity]
Expect(exist).To(BeTrue())
})

It("should convert CPU requests to sockets", func() {
vmi.Spec.Domain.CPU = &v1.CPU{Model: "EPYC"}
vmi.Spec.Domain.Resources.Requests = k8sv1.ResourceList{
Expand Down
9 changes: 0 additions & 9 deletions pkg/virt-api/webhooks/validating-webhook/admitters/vmi-create-admitter.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1717,15 +1717,6 @@ func ValidateVirtualMachineInstanceMetadata(field *k8sfield.Path, metadata *meta
}
}

if _, exists := annotations[v1.CPUManagerPolicyBetaOptionsAnnotation]; exists && !config.CPUManagerPolicyBetaOptionsEnabled() {
causes = append(causes, metav1.StatusCause{
Type: metav1.CauseTypeFieldValueInvalid,
Message: fmt.Sprintf("CPUManagerPolicyBetaOptions feature gate is not enabled in kubevirt-config, invalid entry %s",
field.Child("annotations").Child(v1.CPUManagerPolicyBetaOptionsAnnotation).String()),
Field: field.Child("annotations").String(),
})
}

return causes
}

Expand Down
8 changes: 0 additions & 8 deletions pkg/virt-api/webhooks/validating-webhook/admitters/vmi-create-admitter_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -480,10 +480,6 @@ var _ = Describe("Validating VMICreate Admitter", func() {
map[string]string{hooks.HookSidecarListAnnotationName: "[{'image': 'fake-image'}]"},
fmt.Sprintf("invalid entry metadata.annotations.%s", hooks.HookSidecarListAnnotationName),
),
Entry("without CPUManagerPolicyBetaOptions feature gate enabled",
map[string]string{v1.CPUManagerPolicyBetaOptionsAnnotation: string(v1.CPUManagerPolicyBetaOptionFullpCPUsOnly)},
fmt.Sprintf("invalid entry metadata.annotations.%s", v1.CPUManagerPolicyBetaOptionsAnnotation),
),
)

DescribeTable("should accept annotations which require feature gate enabled", func(annotations map[string]string, featureGate string) {
Expand All @@ -503,10 +499,6 @@ var _ = Describe("Validating VMICreate Admitter", func() {
map[string]string{hooks.HookSidecarListAnnotationName: "[{'image': 'fake-image'}]"},
virtconfig.SidecarGate,
),
Entry("with CPUManagerPolicyBetaOptions feature gate enabled",
map[string]string{v1.CPUManagerPolicyBetaOptionsAnnotation: string(v1.CPUManagerPolicyBetaOptionFullpCPUsOnly)},
virtconfig.CPUManagerPolicyBetaOptionsGate,
),
)
})

Expand Down
8 changes: 4 additions & 4 deletions pkg/virt-config/feature-gates.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,8 +81,8 @@ const (
//
// CommonInstancetypesDeploymentGate enables the deployment of common-instancetypes by virt-operator
CommonInstancetypesDeploymentGate = "CommonInstancetypesDeploymentGate"
// CPUManagerPolicyBetaOptionsGate allows conforming VM to static CPU-Manager Policies.
CPUManagerPolicyBetaOptionsGate = "CPUManagerPolicyBetaOptions"
// AlignCPUsGate allows emulator thread to assign two extra CPUs if needed to complete even parity.
AlignCPUsGate = "AlignCPUs"
)

var deprecatedFeatureGates = [...]string{
Expand Down Expand Up @@ -262,6 +262,6 @@ func (config *ClusterConfig) CommonInstancetypesDeploymentEnabled() bool {
return config.isFeatureGateEnabled(CommonInstancetypesDeploymentGate)
}

func (config *ClusterConfig) CPUManagerPolicyBetaOptionsEnabled() bool {
return config.isFeatureGateEnabled(CPUManagerPolicyBetaOptionsGate)
func (config *ClusterConfig) AlignCPUsEnabled() bool {
return config.isFeatureGateEnabled(AlignCPUsGate)
}
5 changes: 3 additions & 2 deletions pkg/virt-controller/services/renderresources.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ func WithAutoMemoryLimits(namespace string, namespaceStore cache.Store) Resource
}
}

func WithCPUPinning(cpu *v1.CPU, cpuManagerPolicyBetaOption v1.CPUManagerPolicyBetaOptions) ResourceRendererOption {
func WithCPUPinning(cpu *v1.CPU, annotations map[string]string) ResourceRendererOption {
return func(renderer *ResourceRenderer) {
vcpus := hardware.GetNumberOfVCPUs(cpu)
if vcpus != 0 {
Expand All @@ -217,7 +217,8 @@ func WithCPUPinning(cpu *v1.CPU, cpuManagerPolicyBetaOption v1.CPUManagerPolicyB
emulatorThreadCPUs := resource.NewQuantity(1, resource.BinarySI)

limits := renderer.calculatedLimits[k8sv1.ResourceCPU]
if cpuManagerPolicyBetaOption == v1.CPUManagerPolicyBetaOptionFullpCPUsOnly &&
_, emulatorThreadCompleteToEvenParityAnnotationExists := annotations[v1.EmulatorThreadCompleteToEvenParity]
if emulatorThreadCompleteToEvenParityAnnotationExists &&
limits.Value()%2 == 0 {
emulatorThreadCPUs = resource.NewQuantity(2, resource.BinarySI)
}
Expand Down
30 changes: 13 additions & 17 deletions pkg/virt-controller/services/renderresources_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -171,22 +171,21 @@ var _ = Describe("Resource pod spec renderer", func() {
})

Context("WithCPUPinning option", func() {
const cpuManagerPolicyBetaOptionsAnnotationDisabled = ""
userCPURequest := resource.MustParse("200m")
userSpecifiedCPU := kubev1.ResourceList{kubev1.ResourceCPU: userCPURequest}

It("the user requested CPU configs are *not* overriden", func() {
rr = NewResourceRenderer(nil, userSpecifiedCPU, WithCPUPinning(&v1.CPU{Cores: 5}, cpuManagerPolicyBetaOptionsAnnotationDisabled))
rr = NewResourceRenderer(nil, userSpecifiedCPU, WithCPUPinning(&v1.CPU{Cores: 5}, map[string]string{}))
Expect(rr.Requests()).To(HaveKeyWithValue(kubev1.ResourceCPU, userCPURequest))
})

It("carries over the CPU limits as requests when no CPUs are requested", func() {
rr = NewResourceRenderer(userSpecifiedCPU, nil, WithCPUPinning(&v1.CPU{}, cpuManagerPolicyBetaOptionsAnnotationDisabled))
rr = NewResourceRenderer(userSpecifiedCPU, nil, WithCPUPinning(&v1.CPU{}, map[string]string{}))
Expect(rr.Requests()).To(HaveKeyWithValue(kubev1.ResourceCPU, userCPURequest))
})

It("carries over the CPU requests as limits when no CPUs are requested", func() {
rr = NewResourceRenderer(nil, userSpecifiedCPU, WithCPUPinning(&v1.CPU{}, cpuManagerPolicyBetaOptionsAnnotationDisabled))
rr = NewResourceRenderer(nil, userSpecifiedCPU, WithCPUPinning(&v1.CPU{}, map[string]string{}))
Expect(rr.Requests()).To(HaveKeyWithValue(kubev1.ResourceCPU, userCPURequest))
})

Expand All @@ -196,7 +195,7 @@ var _ = Describe("Resource pod spec renderer", func() {
kubev1.ResourceCPU: userCPURequest,
kubev1.ResourceMemory: memoryRequest,
}
rr = NewResourceRenderer(nil, userSpecifiedCPU, WithCPUPinning(&v1.CPU{Cores: 5}, cpuManagerPolicyBetaOptionsAnnotationDisabled))
rr = NewResourceRenderer(nil, userSpecifiedCPU, WithCPUPinning(&v1.CPU{Cores: 5}, map[string]string{}))
Expect(rr.Requests()).To(HaveKeyWithValue(kubev1.ResourceCPU, resource.MustParse("200m")))
Expect(rr.Limits()).To(HaveKeyWithValue(kubev1.ResourceMemory, memoryRequest))
})
Expand All @@ -205,25 +204,22 @@ var _ = Describe("Resource pod spec renderer", func() {
userSpecifiedCPURequest := kubev1.ResourceList{kubev1.ResourceCPU: userCPURequest}

DescribeTable("requires additional EmulatorThread CPUs overhead, and additional CPUs added to the limits",
func(fullPCPUEnabled, defineUserSpecifiedCPULimit bool, cores uint32, expectedCPUOverhead string) {
func(vmiAnnotations map[string]string, defineUserSpecifiedCPULimit bool, cores uint32, expectedCPUOverhead string) {
cpuIsolatedEmulatorThreadOverhead := resource.MustParse(expectedCPUOverhead)
var userSpecifiedCPULimit kubev1.ResourceList
var cpuManagerPolicyBetaOption v1.CPUManagerPolicyBetaOptions

if defineUserSpecifiedCPULimit {
userSpecifiedCPULimit = kubev1.ResourceList{kubev1.ResourceCPU: userCPURequest}
}
if fullPCPUEnabled {
cpuManagerPolicyBetaOption = v1.CPUManagerPolicyBetaOptionFullpCPUsOnly
}

rr = NewResourceRenderer(
userSpecifiedCPULimit,
userSpecifiedCPURequest,
WithCPUPinning(&v1.CPU{
Cores: cores,
IsolateEmulatorThread: true,
},
cpuManagerPolicyBetaOption),
vmiAnnotations),
)
Expect(rr.Limits()).To(HaveKeyWithValue(
kubev1.ResourceCPU,
Expand All @@ -234,12 +230,12 @@ var _ = Describe("Resource pod spec renderer", func() {
addResources(userCPURequest, cpuIsolatedEmulatorThreadOverhead),
))
},
Entry("full-pcpu-only mode is disabled, only CPU requests set by the user", false, false, uint32(5), "1000m"),
Entry("full-pcpu-only mode is disabled, request and limits set by the user", false, true, uint32(5), "1000m"),
Entry("full-pcpu-only mode is enabled, only CPU requests set by the user, odd amount of cores is requested", true, false, uint32(5), "1000m"),
Entry("full-pcpu-only mode is enabled, only CPU requests set by the user, even amount of cores is requested", true, false, uint32(6), "2000m"),
Entry("full-pcpu-only mode is enabled, request and limits set by the user, odd amount of cores is requested", true, true, uint32(5), "1000m"),
Entry("full-pcpu-only mode is enabled, request and limits set by the user, even amount of cores is requested", true, true, uint32(6), "2000m"),
Entry("EmulatorThreadCompleteToEvenParity mode is disabled, only CPU requests set by the user", map[string]string{}, false, uint32(5), "1000m"),
Entry("EmulatorThreadCompleteToEvenParity mode is disabled, request and limits set by the user", map[string]string{}, true, uint32(5), "1000m"),
Entry("EmulatorThreadCompleteToEvenParity mode is enabled, only CPU requests set by the user, odd amount of cores is requested", map[string]string{v1.EmulatorThreadCompleteToEvenParity: ""}, false, uint32(5), "1000m"),
Entry("EmulatorThreadCompleteToEvenParity mode is enabled, only CPU requests set by the user, even amount of cores is requested", map[string]string{v1.EmulatorThreadCompleteToEvenParity: ""}, false, uint32(6), "2000m"),
Entry("EmulatorThreadCompleteToEvenParity mode is enabled, request and limits set by the user, odd amount of cores is requested", map[string]string{v1.EmulatorThreadCompleteToEvenParity: ""}, true, uint32(5), "1000m"),
Entry("EmulatorThreadCompleteToEvenParity mode is enabled, request and limits set by the user, even amount of cores is requested", map[string]string{v1.EmulatorThreadCompleteToEvenParity: ""}, true, uint32(6), "2000m"),
)
})
})
Expand Down
4 changes: 1 addition & 3 deletions pkg/virt-controller/services/template.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1464,12 +1464,10 @@ func (t *templateService) doesVMIRequireAutoCPULimits(vmi *v1.VirtualMachineInst
func (t *templateService) VMIResourcePredicates(vmi *v1.VirtualMachineInstance, networkToResourceMap map[string]string) VMIResourcePredicates {
memoryOverhead := GetMemoryOverhead(vmi, t.clusterConfig.GetClusterCPUArch(), t.clusterConfig.GetConfig().AdditionalGuestMemoryOverheadRatio)
withCPULimits := t.doesVMIRequireAutoCPULimits(vmi)
CPUManagerPolicyBetaOption := v1.CPUManagerPolicyBetaOptions(vmi.Annotations[v1.CPUManagerPolicyBetaOptionsAnnotation])

return VMIResourcePredicates{
vmi: vmi,
resourceRules: []VMIResourceRule{
NewVMIResourceRule(doesVMIRequireDedicatedCPU, WithCPUPinning(vmi.Spec.Domain.CPU, CPUManagerPolicyBetaOption)),
NewVMIResourceRule(doesVMIRequireDedicatedCPU, WithCPUPinning(vmi.Spec.Domain.CPU, vmi.Annotations)),
NewVMIResourceRule(not(doesVMIRequireDedicatedCPU), WithoutDedicatedCPU(vmi.Spec.Domain.CPU, t.clusterConfig.GetCPUAllocationRatio(), withCPULimits)),
NewVMIResourceRule(util.HasHugePages, WithHugePages(vmi.Spec.Domain.Memory, memoryOverhead)),
NewVMIResourceRule(not(util.HasHugePages), WithMemoryOverhead(vmi.Spec.Domain.Resources, memoryOverhead)),
Expand Down
4 changes: 2 additions & 2 deletions pkg/virt-controller/services/template_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1725,8 +1725,8 @@ var _ = Describe("Template", func() {
},
Entry("no annotation added, odd CPUs requested, should allocate one extra emulator CPU", map[string]string{}, uint32(3), "4"),
Entry("no annotation added, even CPUs requested, should allocate one extra emulator CPU", map[string]string{}, uint32(2), "3"),
Entry("full-pcpu-only annotation added, odd CPUs requested, should allocate one extra emulator CPU", map[string]string{v1.CPUManagerPolicyBetaOptionsAnnotation: string(v1.CPUManagerPolicyBetaOptionFullpCPUsOnly)}, uint32(3), "4"),
Entry("full-pcpu-only annotation added, even CPUs requested, should allocate two extra emulator CPU (to align SMT scheduling)", map[string]string{v1.CPUManagerPolicyBetaOptionsAnnotation: string(v1.CPUManagerPolicyBetaOptionFullpCPUsOnly)}, uint32(4), "6"),
Entry("EmulatorThreadCompleteToEvenParity annotation added, odd CPUs requested, should allocate one extra emulator CPU", map[string]string{v1.EmulatorThreadCompleteToEvenParity: ""}, uint32(3), "4"),
Entry("EmulatorThreadCompleteToEvenParity annotation added, even CPUs requested, should allocate two extra emulator CPU (to align SMT scheduling)", map[string]string{v1.EmulatorThreadCompleteToEvenParity: ""}, uint32(4), "6"),
)
It("should add node affinity to pod", func() {
config, kvInformer, svc = configFactory(defaultArch)
Expand Down
Loading

0 comments on commit 33e6143

Please sign in to comment.