Skip to content

Commit

Permalink
Optimize CPU Model Feature Validation for Node Labeller
Browse files Browse the repository at this point in the history
**Issue:**
Libvirt validation fails when a CPU model is usable
by QEMU but lacks features listed in
`/usr/share/libvirt/cpu_map/[CPU Model].xml` on a node.

**Resolution:**
Nodes are now labeled with a CPU model only if all
features listed in the corresponding `cpu_map` XML file
are present and the model is usable by QEMU.
This prevents validation issues caused by missing features.

**Libvirt's Recommendation:**
Libvirt advises against using its internal data files for
feature validation.
As recommended, we're exploring the use only Libvirt's API
to identify usable CPU models, avoiding direct `cpu_map`
file dependency.

**Workaround:**
To avoid the validation error mentioned above we will disable
some of the features in the `/usr/share/libvirt/cpu_map/[CPU Model].xml` files:

- disable svm for Opteron_G2 to avoid:
https://bugzilla.redhat.com/show_bug.cgi?id=2122283
- disable mpx for Skylake, Cascadelake, and Icelake to avoid:
https://gitlab.com/libvirt/libvirt/-/issues/304

These features are disabled only if not explicitly required by the user,
allowing broader CPU model support when svm or mpx features are absent.

In addition, for future issues we allow setting
a config map called "cpu-model-to-features-to-disable"
in the installation namespace for quick problematic
features disablement.

Signed-off-by: bmordeha <bmordeha@redhat.com>
  • Loading branch information
Barakmor1 committed Mar 3, 2024
1 parent 13f3655 commit 29c1163
Show file tree
Hide file tree
Showing 10 changed files with 306 additions and 162 deletions.
1 change: 1 addition & 0 deletions cmd/virt-handler/virt-handler.go
Expand Up @@ -350,6 +350,7 @@ func (app *virtHandlerApp) Run() {
app.KubeletPodsDir,
vmiSourceInformer,
vmiTargetInformer,
factory.CpuModelToFeaturesToDisableConfigMap(),
domainSharedInformer,
int(app.WatchdogTimeoutDuration.Seconds()),
app.MaxDevices,
Expand Down
12 changes: 12 additions & 0 deletions pkg/controller/virtinformers.go
Expand Up @@ -165,6 +165,9 @@ type KubeInformerFactory interface {
// Watches for the export route config map
ExportRouteConfigMap() cache.SharedIndexInformer

// Watches for cpu model config map
CpuModelToFeaturesToDisableConfigMap() cache.SharedIndexInformer

// Watches for the kubevirt export service
ExportService() cache.SharedIndexInformer

Expand Down Expand Up @@ -892,6 +895,15 @@ func (f *kubeInformerFactory) ExportRouteConfigMap() cache.SharedIndexInformer {
})
}

func (f *kubeInformerFactory) CpuModelToFeaturesToDisableConfigMap() cache.SharedIndexInformer {
return f.getInformer("cpuModelToFeaturesToDisable", func() cache.SharedIndexInformer {
restClient := f.clientSet.CoreV1().RESTClient()
fieldSelector := fields.OneTermEqualSelector("metadata.name", "cpu-model-to-features-to-disable")
lw := cache.NewListWatchFromClient(restClient, "configmaps", f.kubevirtNamespace, fieldSelector)
return cache.NewSharedIndexInformer(lw, &k8sv1.ConfigMap{}, f.defaultResync, cache.Indexers{cache.NamespaceIndex: cache.MetaNamespaceIndexFunc})
})
}

func (f *kubeInformerFactory) ExportService() cache.SharedIndexInformer {
return f.getInformer("exportService", func() cache.SharedIndexInformer {
// Watch all service with the kubevirt app label
Expand Down
304 changes: 167 additions & 137 deletions pkg/handler-launcher-com/cmd/v1/cmd.pb.go

Large diffs are not rendered by default.

5 changes: 5 additions & 0 deletions pkg/handler-launcher-com/cmd/v1/cmd.proto
Expand Up @@ -104,6 +104,10 @@ message InterfaceBindingMigration{
string Method = 1;
}

message FeaturesToDisable{
repeated string features = 1;
}

message VirtualMachineOptions {
SMBios VirtualMachineSMBios = 1;
uint32 MemBalloonStatsPeriod = 2;
Expand All @@ -115,6 +119,7 @@ message VirtualMachineOptions {
ClusterConfig clusterConfig = 7;
map<string, string> interfaceDomainAttachment = 8;
map<string, InterfaceBindingMigration> interfaceMigration = 9;
map<string, FeaturesToDisable> cpuModelToFeaturesToDisable = 10;
}

message VMIRequest {
Expand Down
1 change: 1 addition & 0 deletions pkg/virt-config/BUILD.bazel
Expand Up @@ -10,6 +10,7 @@ go_library(
importpath = "kubevirt.io/kubevirt/pkg/virt-config",
visibility = ["//visibility:public"],
deps = [
"//pkg/handler-launcher-com/cmd/v1:go_default_library",
"//pkg/pointer:go_default_library",
"//pkg/virt-config/deprecation:go_default_library",
"//staging/src/kubevirt.io/api/core/v1:go_default_library",
Expand Down
76 changes: 74 additions & 2 deletions pkg/virt-config/virt-config.go
Expand Up @@ -25,13 +25,16 @@ package virtconfig

import (
"fmt"

"kubevirt.io/client-go/log"
"strings"

k8sv1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/resource"
"k8s.io/client-go/tools/cache"

v1 "kubevirt.io/api/core/v1"
"kubevirt.io/client-go/log"

v12 "kubevirt.io/kubevirt/pkg/handler-launcher-com/cmd/v1"
)

const (
Expand Down Expand Up @@ -101,6 +104,58 @@ func IsPPC64(arch string) bool {
return arch == "ppc64le"
}

func (c *ClusterConfig) GetCpuModelToFeaturesToDisableMap(cpuModelConfigMapInformer cache.SharedIndexInformer) map[string]*v12.FeaturesToDisable {
/*
Libvirt validation fails when a CPU model is usable
by QEMU but lacks features listed in
`/usr/share/libvirt/cpu_map/[CPU Model].xml` on a node
To avoid the validation error mentioned above we will disable
some of the features in the `/usr/share/libvirt/cpu_map/[CPU Model].xml` files
for some of the cpu models.
Examples of validation error:
https://bugzilla.redhat.com/show_bug.cgi?id=2122283
https://gitlab.com/libvirt/libvirt/-/issues/304
Additionally, to handle future issues, we allow setting a config map named
“cpu-model-to-features-to-disable” in the installation namespace.
This enables quick and targeted disabling of problematic features.
*/

cpuModelToFeaturesToDisable := map[string]*v12.FeaturesToDisable{
"Opteron_G2": {Features: []string{"svm"}},
"Cascadelake-Server-noTSX": {Features: []string{"mpx"}},
"Cascadelake-Server": {Features: []string{"mpx"}},
"Icelake-Client-noTSX": {Features: []string{"mpx"}},
"Icelake-Client": {Features: []string{"mpx"}},
"Icelake-Server-noTSX": {Features: []string{"mpx"}},
"Icelake-Server": {Features: []string{"mpx"}},
"Skylake-Client-IBRS": {Features: []string{"mpx"}},
"Skylake-Client-noTSX-IBRS": {Features: []string{"mpx"}},
"Skylake-Client": {Features: []string{"mpx"}},
"Skylake-Server-IBRS": {Features: []string{"mpx"}},
"Skylake-Server-noTSX-IBRS": {Features: []string{"mpx"}},
"Skylake-Server": {Features: []string{"mpx"}},
}

cachedObjs := cpuModelConfigMapInformer.GetIndexer().List()

if len(cachedObjs) != 1 {
return cpuModelToFeaturesToDisable
}
cpuModelToFeaturesToDisableCM := cachedObjs[0].(*k8sv1.ConfigMap)

for model, featuresString := range cpuModelToFeaturesToDisableCM.Data {
features := strings.Split(featuresString, ",")
knowFeatures := []string{}
if _, ok := cpuModelToFeaturesToDisable[model]; ok {
knowFeatures = cpuModelToFeaturesToDisable[model].Features
}
cpuModelToFeaturesToDisable[model] = &v12.FeaturesToDisable{Features: mergeStringSlices(knowFeatures, features)}
}
return cpuModelToFeaturesToDisable
}

func (c *ClusterConfig) GetMemBalloonStatsPeriod() uint32 {
return *c.GetConfig().MemBalloonStatsPeriod
}
Expand Down Expand Up @@ -476,3 +531,20 @@ func (c *ClusterConfig) GetNetworkBindings() map[string]v1.InterfaceBindingPlugi
}
return nil
}

func mergeStringSlices(slice1, slice2 []string) []string {
mergedMap := make(map[string]bool)
for _, s := range slice1 {
mergedMap[s] = true
}

for _, s := range slice2 {
mergedMap[s] = true
}

mergedSlice := make([]string, 0, len(mergedMap))
for s := range mergedMap {
mergedSlice = append(mergedSlice, s)
}
return mergedSlice
}
51 changes: 28 additions & 23 deletions pkg/virt-handler/vm.go
Expand Up @@ -199,6 +199,7 @@ func NewController(
kubeletPodsDir string,
vmiSourceInformer cache.SharedIndexInformer,
vmiTargetInformer cache.SharedIndexInformer,
cpuModelConfigMapInformer cache.SharedIndexInformer,
domainInformer cache.SharedInformer,
watchdogTimeoutSeconds int,
maxDevices int,
Expand All @@ -222,6 +223,7 @@ func NewController(
vmiSourceInformer: vmiSourceInformer,
vmiTargetInformer: vmiTargetInformer,
domainInformer: domainInformer,
cpuModelConfigMapInformer: cpuModelConfigMapInformer,
heartBeatInterval: 1 * time.Minute,
watchdogTimeoutSeconds: watchdogTimeoutSeconds,
migrationProxy: migrationProxy,
Expand Down Expand Up @@ -298,27 +300,28 @@ func NewController(
}

type VirtualMachineController struct {
recorder record.EventRecorder
clientset kubecli.KubevirtClient
host string
migrationIpAddress string
virtShareDir string
virtPrivateDir string
Queue workqueue.RateLimitingInterface
vmiSourceInformer cache.SharedIndexInformer
vmiTargetInformer cache.SharedIndexInformer
domainInformer cache.SharedInformer
launcherClients virtcache.LauncherClientInfoByVMI
heartBeatInterval time.Duration
watchdogTimeoutSeconds int
deviceManagerController *device_manager.DeviceController
migrationProxy migrationproxy.ProxyManager
podIsolationDetector isolation.PodIsolationDetector
containerDiskMounter container_disk.Mounter
hotplugVolumeMounter hotplug_volume.VolumeMounter
clusterConfig *virtconfig.ClusterConfig
sriovHotplugExecutorPool *executor.RateLimitedExecutorPool
downwardMetricsManager downwardMetricsManager
recorder record.EventRecorder
clientset kubecli.KubevirtClient
host string
migrationIpAddress string
virtShareDir string
virtPrivateDir string
Queue workqueue.RateLimitingInterface
vmiSourceInformer cache.SharedIndexInformer
vmiTargetInformer cache.SharedIndexInformer
domainInformer cache.SharedInformer
cpuModelConfigMapInformer cache.SharedIndexInformer
launcherClients virtcache.LauncherClientInfoByVMI
heartBeatInterval time.Duration
watchdogTimeoutSeconds int
deviceManagerController *device_manager.DeviceController
migrationProxy migrationproxy.ProxyManager
podIsolationDetector isolation.PodIsolationDetector
containerDiskMounter container_disk.Mounter
hotplugVolumeMounter hotplug_volume.VolumeMounter
clusterConfig *virtconfig.ClusterConfig
sriovHotplugExecutorPool *executor.RateLimitedExecutorPool
downwardMetricsManager downwardMetricsManager

netConf netconf
netStat netstat
Expand Down Expand Up @@ -1621,7 +1624,9 @@ func (c *VirtualMachineController) Run(threadiness int, stopCh chan struct{}) {

go c.downwardMetricsManager.Run(stopCh)

cache.WaitForCacheSync(stopCh, c.domainInformer.HasSynced, c.vmiSourceInformer.HasSynced, c.vmiTargetInformer.HasSynced)
go c.cpuModelConfigMapInformer.Run(stopCh)

cache.WaitForCacheSync(stopCh, c.domainInformer.HasSynced, c.vmiSourceInformer.HasSynced, c.vmiTargetInformer.HasSynced, c.cpuModelConfigMapInformer.HasSynced)

// Queue keys for previous Domains on the host that no longer exist
// in the cache. This ensures we perform local cleanup of deleted VMs.
Expand Down Expand Up @@ -3095,8 +3100,8 @@ func (d *VirtualMachineController) vmUpdateHelperDefault(origVMI *v1.VirtualMach

smbios := d.clusterConfig.GetSMBIOS()
period := d.clusterConfig.GetMemBalloonStatsPeriod()

options := virtualMachineOptions(smbios, period, preallocatedVolumes, d.capabilities, disksInfo, d.clusterConfig)
options.CpuModelToFeaturesToDisable = d.clusterConfig.GetCpuModelToFeaturesToDisableMap(d.cpuModelConfigMapInformer)
options.InterfaceDomainAttachment = domainspec.DomainAttachmentByInterfaceName(vmi.Spec.Domain.Devices.Interfaces, d.clusterConfig.GetNetworkBindings())

err = client.SyncVirtualMachine(vmi, options)
Expand Down
3 changes: 3 additions & 0 deletions pkg/virt-handler/vm_test.go
Expand Up @@ -95,6 +95,7 @@ var _ = Describe("VirtualMachineInstance", func() {
var vmiSource *framework.FakeControllerSource
var vmiSourceInformer cache.SharedIndexInformer
var vmiTargetInformer cache.SharedIndexInformer
var cpuModelCMInformer cache.SharedIndexInformer
var domainSource *framework.FakeControllerSource
var domainInformer cache.SharedIndexInformer
var mockQueue *testutils.MockWorkQueue
Expand Down Expand Up @@ -177,6 +178,7 @@ var _ = Describe("VirtualMachineInstance", func() {

vmiSourceInformer, vmiSource = testutils.NewFakeInformerFor(&v1.VirtualMachineInstance{})
vmiTargetInformer, _ = testutils.NewFakeInformerFor(&v1.VirtualMachineInstance{})
cpuModelCMInformer, _ = testutils.NewFakeInformerFor(&k8sv1.ConfigMap{})
domainInformer, domainSource = testutils.NewFakeInformerFor(&api.Domain{})
recorder = record.NewFakeRecorder(100)
recorder.IncludeObject = true
Expand Down Expand Up @@ -225,6 +227,7 @@ var _ = Describe("VirtualMachineInstance", func() {
podsDir,
vmiSourceInformer,
vmiTargetInformer,
cpuModelCMInformer,
domainInformer,
1,
10,
Expand Down
14 changes: 14 additions & 0 deletions pkg/virt-launcher/virtwrap/converter/converter.go
Expand Up @@ -134,6 +134,7 @@ type ConverterContext struct {
BochsForEFIGuests bool
SerialConsoleLog bool
DomainAttachmentByInterfaceName map[string]string
CpuModelToFeaturesToDisable map[string]*cmdv1.FeaturesToDisable
}

func contains(volumes []string, name string) bool {
Expand Down Expand Up @@ -1827,15 +1828,28 @@ func Convert_v1_VirtualMachineInstance_To_api_Domain(vmi *v1.VirtualMachineInsta
}

// Set VM CPU features
existingFeatures := make(map[string]struct{})
if vmi.Spec.Domain.CPU.Features != nil {
for _, feature := range vmi.Spec.Domain.CPU.Features {
existingFeatures[feature.Name] = struct{}{}
domain.Spec.CPU.Features = append(domain.Spec.CPU.Features, api.CPUFeature{
Name: feature.Name,
Policy: feature.Policy,
})
}
}

if featuresToDisable, ok := c.CpuModelToFeaturesToDisable[vmi.Spec.Domain.CPU.Model]; ok {
for _, feature := range featuresToDisable.Features {
if _, exists := existingFeatures[feature]; !exists {
domain.Spec.CPU.Features = append(domain.Spec.CPU.Features, api.CPUFeature{
Name: feature,
Policy: "disable",
})
}
}
}

// Adjust guest vcpu config. Currently will handle vCPUs to pCPUs pinning
if vmi.IsCPUDedicated() {
err = vcpu.AdjustDomainForTopologyAndCPUSet(domain, vmi, c.Topology, c.CPUSet, useIOThreads)
Expand Down
1 change: 1 addition & 0 deletions pkg/virt-launcher/virtwrap/manager.go
Expand Up @@ -983,6 +983,7 @@ func (l *LibvirtDomainManager) generateConverterContext(vmi *v1.VirtualMachineIn
c.SerialConsoleLog = isSerialConsoleLogEnabled(options.GetClusterConfig().GetSerialConsoleLogDisabled(), vmi)
}

c.CpuModelToFeaturesToDisable = options.CpuModelToFeaturesToDisable
c.DomainAttachmentByInterfaceName = options.GetInterfaceDomainAttachment()
}
c.DisksInfo = l.disksInfo
Expand Down

0 comments on commit 29c1163

Please sign in to comment.