add negative test + small fixes

pkg/virt-config/config-map.go

@@ -21,7 +21,6 @@ package virtconfig
 
 import (
 	"os"
-	"strings"
 	"time"
 
 	k8sv1 "k8s.io/api/core/v1"
@@ -37,14 +36,15 @@ import (
 const (
 	configMapName         = "kubevirt-config"
 	featureGateEnvVar     = "FEATURE_GATES"
+	FeatureGatesKey       = "feature-gates"
 	emulatedMachineEnvVar = "VIRT_EMULATED_MACHINES"
 )
 
 // We cannot rely on automatic invocation of 'init' method because this initialization
 // code assumes a cluster is available to pull the configmap from
 func Init() {
 	cfgMap := getConfigMap()
-	if val, ok := cfgMap.Data["feature-gates"]; ok {
+	if val, ok := cfgMap.Data[FeatureGatesKey]; ok {
 		os.Setenv(featureGateEnvVar, val)
 	}
 	if val, ok := cfgMap.Data["emulated-machines"]; ok {
@@ -87,7 +87,3 @@ func getConfigMap() *k8sv1.ConfigMap {
 
 	return cfgMap
 }
-
-func CPUNodeDiscoveryEnabled() bool {
-	return strings.Contains(os.Getenv(featureGateEnvVar), CPUNodeDiscoveryGate)
-}

pkg/virt-config/feature-gates.go

@@ -25,10 +25,11 @@ import (
 )
 
 const (
-	dataVolumesGate   = "DataVolumes"
-	cpuManager        = "CPUManager"
-	liveMigrationGate = "LiveMigration"
-	SRIOVGate         = "SRIOV"
+	dataVolumesGate      = "DataVolumes"
+	cpuManager           = "CPUManager"
+	liveMigrationGate    = "LiveMigration"
+	SRIOVGate            = "SRIOV"
+	CPUNodeDiscoveryGate = "CPUNodeDiscovery"
 )
 
 func DataVolumesEnabled() bool {

pkg/virt-controller/services/template.go

@@ -35,14 +35,14 @@ import (
 	v1 "kubevirt.io/kubevirt/pkg/api/v1"
 	"kubevirt.io/kubevirt/pkg/config"
 	containerdisk "kubevirt.io/kubevirt/pkg/container-disk"
-	"kubevirt.io/kubevirt/pkg/feature-gates"
 	"kubevirt.io/kubevirt/pkg/hooks"
 	"kubevirt.io/kubevirt/pkg/kubecli"
 	"kubevirt.io/kubevirt/pkg/log"
 	"kubevirt.io/kubevirt/pkg/precond"
 	"kubevirt.io/kubevirt/pkg/util"
 	"kubevirt.io/kubevirt/pkg/util/net/dns"
 	"kubevirt.io/kubevirt/pkg/util/types"
+	"kubevirt.io/kubevirt/pkg/virt-config"
 )
 
 const configMapName = "kubevirt-config"
@@ -62,7 +62,7 @@ const LibvirtStartupDelay = 10
 
 //This is a perfix for node feature discovery, used in a NodeSelector on the pod
 //to match a VirtualMachineInstance CPU model(Family) to nodes that support this model.
-const NFD_CPU_FAMILY_PREFIX = "feature.node.kubernetes.io/cpu-model-"
+const NFD_CPU_MODEL_PREFIX = "feature.node.kubernetes.io/cpu-model-"
 
 const MULTUS_RESOURCE_NAME_ANNOTATION = "k8s.v1.cni.cncf.io/resourceName"
 
@@ -150,14 +150,23 @@ func isSRIOVVmi(vmi *v1.VirtualMachineInstance) bool {
 }
 
 func IsCPUNodeDiscoveryEnabled(store cache.Store) bool {
-	var value string
-	value, _ = getConfigMapEntry(store, featuregates.FEATURE_GATES_KEY)
-	if strings.Contains(value, featuregates.CPUNodeDiscoveryGate) {
+	if value, err := getConfigMapEntry(store, virtconfig.FeatureGatesKey); err != nil {
+		return false
+	} else if strings.Contains(value, virtconfig.CPUNodeDiscoveryGate) {
 		return true
 	}
 	return false
 }
 
+func CPUModelLabelFromCPUModel(vmi *v1.VirtualMachineInstance) (label string, err error) {
+	if vmi.Spec.Domain.CPU == nil || vmi.Spec.Domain.CPU.Model == "" {
+		err = fmt.Errorf("Cannot create CPU Model label, vmi spec is mising CPU model")
+		return
+	}
+	label = NFD_CPU_MODEL_PREFIX + vmi.Spec.Domain.CPU.Model
+	return
+}
+
 func (t *templateService) RenderLaunchManifest(vmi *v1.VirtualMachineInstance) (*k8sv1.Pod, error) {
 	precond.MustNotBeNil(vmi)
 	domain := precond.MustNotBeEmpty(vmi.GetObjectMeta().GetName())
@@ -627,9 +636,9 @@ func (t *templateService) RenderLaunchManifest(vmi *v1.VirtualMachineInstance) (
 
 	}
 	if IsCPUNodeDiscoveryEnabled(t.configMapStore) {
-		if vmi.Spec.Domain.CPU != nil && vmi.Spec.Domain.CPU.Model != "" {
+		if cpuModelLabel, err := CPUModelLabelFromCPUModel(vmi); err == nil {
 			if vmi.Spec.Domain.CPU.Model != v1.CPUModeHostModel && vmi.Spec.Domain.CPU.Model != v1.CPUModeHostPassthrough {
-				nodeSelector[NFD_CPU_FAMILY_PREFIX+vmi.Spec.Domain.CPU.Model] = "true"
+				nodeSelector[cpuModelLabel] = "true"
 			}
 		}
 	}

pkg/virt-controller/services/template_test.go

@@ -38,10 +38,10 @@ import (
 	"k8s.io/client-go/tools/cache"
 
 	v1 "kubevirt.io/kubevirt/pkg/api/v1"
-	"kubevirt.io/kubevirt/pkg/feature-gates"
 	"kubevirt.io/kubevirt/pkg/hooks"
 	"kubevirt.io/kubevirt/pkg/kubecli"
 	"kubevirt.io/kubevirt/pkg/log"
+	"kubevirt.io/kubevirt/pkg/virt-config"
 )
 
 const namespaceKubevirt = "kubevirt"
@@ -257,7 +257,7 @@ var _ = Describe("Template", func() {
 						Namespace: namespaceKubevirt,
 						Name:      "kubevirt-config",
 					},
-					Data: map[string]string{featuregates.FEATURE_GATES_KEY: featuregates.CPUNodeDiscoveryGate},
+					Data: map[string]string{virtconfig.FeatureGatesKey: virtconfig.CPUNodeDiscoveryGate},
 				}
 				cmCache.Add(&cfgMap)
 
@@ -276,9 +276,12 @@ var _ = Describe("Template", func() {
 					},
 				}
 
+				cpuModelLabel, err := CPUModelLabelFromCPUModel(&vmi)
+				Expect(err).ToNot(HaveOccurred())
 				pod, err := svc.RenderLaunchManifest(&vmi)
 				Expect(err).ToNot(HaveOccurred())
-				Expect(pod.Spec.NodeSelector).Should(HaveKeyWithValue(NFD_CPU_FAMILY_PREFIX+"Conroe", "true"))
+
+				Expect(pod.Spec.NodeSelector).Should(HaveKeyWithValue(cpuModelLabel, "true"))
 			})
 
 			It("should add default cpu/memory resources to the sidecar container if cpu pinning was requested", func() {

tests/utils.go

@@ -60,10 +60,10 @@ import (
 	cdiv1 "kubevirt.io/containerized-data-importer/pkg/apis/datavolumecontroller/v1alpha1"
 	v1 "kubevirt.io/kubevirt/pkg/api/v1"
 	"kubevirt.io/kubevirt/pkg/controller"
-	"kubevirt.io/kubevirt/pkg/feature-gates"
 	"kubevirt.io/kubevirt/pkg/kubecli"
 	"kubevirt.io/kubevirt/pkg/log"
 	"kubevirt.io/kubevirt/pkg/util/net/dns"
+	"kubevirt.io/kubevirt/pkg/virt-config"
 	"kubevirt.io/kubevirt/pkg/virt-controller/services"
 	"kubevirt.io/kubevirt/pkg/virtctl"
 	vmsgen "kubevirt.io/kubevirt/tools/vms-generator/utils"
@@ -2617,7 +2617,7 @@ func HasCDI() bool {
 	options := metav1.GetOptions{}
 	cfgMap, err := virtClient.CoreV1().ConfigMaps(KubeVirtInstallNamespace).Get("kubevirt-config", options)
 	if err == nil {
-		val, ok := cfgMap.Data[featuregates.FEATURE_GATES_KEY]
+		val, ok := cfgMap.Data[virtconfig.FeatureGatesKey]
 		if !ok {
 			return hasCDI
 		}

tests/vmi_lifecycle_test.go

@@ -39,8 +39,8 @@ import (
 	"k8s.io/apimachinery/pkg/util/rand"
 
 	v1 "kubevirt.io/kubevirt/pkg/api/v1"
-	"kubevirt.io/kubevirt/pkg/feature-gates"
 	"kubevirt.io/kubevirt/pkg/kubecli"
+	"kubevirt.io/kubevirt/pkg/virt-config"
 	"kubevirt.io/kubevirt/pkg/virt-controller/services"
 	"kubevirt.io/kubevirt/pkg/virt-controller/watch"
 	"kubevirt.io/kubevirt/pkg/virt-launcher/virtwrap/api"
@@ -640,20 +640,24 @@ var _ = Describe("VMIlifecycle", func() {
 			BeforeEach(func() {
 				options = metav1.GetOptions{}
 				cfgMap, err = virtClient.CoreV1().ConfigMaps(namespaceKubevirt).Get("kubevirt-config", options)
-				ExpectWithOffset(1, err).ToNot(HaveOccurred())
-				originalData = cfgMap.Data[featuregates.FEATURE_GATES_KEY]
-				cfgMap.Data[featuregates.FEATURE_GATES_KEY] = featuregates.CPUNodeDiscoveryGate
+				Expect(err).ToNot(HaveOccurred())
+				originalData = cfgMap.Data[virtconfig.FeatureGatesKey]
+				cfgMap.Data[virtconfig.FeatureGatesKey] = virtconfig.CPUNodeDiscoveryGate
 				_, err = virtClient.CoreV1().ConfigMaps(namespaceKubevirt).Update(cfgMap)
-				ExpectWithOffset(1, err).ToNot(HaveOccurred())
+				Expect(err).ToNot(HaveOccurred())
+				//FIXME improve the detection if virt-controller already received the config-map change
+				time.Sleep(time.Millisecond * 500)
 
 			})
 
 			AfterEach(func() {
 				cfgMap, err = virtClient.CoreV1().ConfigMaps(namespaceKubevirt).Get("kubevirt-config", options)
-				ExpectWithOffset(1, err).ToNot(HaveOccurred())
-				cfgMap.Data[featuregates.FEATURE_GATES_KEY] = originalData
+				Expect(err).ToNot(HaveOccurred())
+				cfgMap.Data[virtconfig.FeatureGatesKey] = originalData
 				_, err = virtClient.CoreV1().ConfigMaps(namespaceKubevirt).Update(cfgMap)
-				ExpectWithOffset(1, err).ToNot(HaveOccurred())
+				Expect(err).ToNot(HaveOccurred())
+				//FIXME improve the detection if virt-controller already received the config-map change
+				time.Sleep(time.Millisecond * 500)
 			})
 
 			It("the vmi with cpu.model matching a nfd label on a node should be scheduled", func() {
@@ -663,17 +667,20 @@ var _ = Describe("VMIlifecycle", func() {
 				Expect(err).ToNot(HaveOccurred(), "Should list nodes")
 				Expect(nodes.Items).ToNot(BeEmpty(), "There should be some nodes")
 
-				NFD_CPU_LABEL := services.NFD_CPU_FAMILY_PREFIX + "Conroe"
-				node := &nodes.Items[0]
-				node, err = virtClient.CoreV1().Nodes().Patch(node.Name, types.StrategicMergePatchType,
-					[]byte(fmt.Sprintf(`{"metadata": { "labels": {"%s": "true"}}}`, NFD_CPU_LABEL)))
-				Expect(err).ToNot(HaveOccurred(), "Should patch node successfully")
-
 				vmi := tests.NewRandomVMIWithEphemeralDiskAndUserdata(tests.ContainerDiskFor(tests.ContainerDiskCirros), "#!/bin/bash\necho 'hello'\n")
 				vmi.Spec.Domain.CPU = &v1.CPU{
 					Cores: 1,
 					Model: "Conroe",
 				}
+
+				cpuModelLabel, err := services.CPUModelLabelFromCPUModel(vmi)
+				Expect(err).ToNot(HaveOccurred(), "CPU model label should have been retrieved successfully")
+
+				node := &nodes.Items[0]
+				node, err = virtClient.CoreV1().Nodes().Patch(node.Name, types.StrategicMergePatchType,
+					[]byte(fmt.Sprintf(`{"metadata": { "labels": {"%s": "true"}}}`, cpuModelLabel)))
+				Expect(err).ToNot(HaveOccurred(), "Should patch node successfully")
+
 				_, err = virtClient.VirtualMachineInstance(vmi.Namespace).Create(vmi)
 				Expect(err).ToNot(HaveOccurred(), "Should create VMI")
 				tests.WaitForSuccessfulVMIStart(vmi)
@@ -684,6 +691,43 @@ var _ = Describe("VMIlifecycle", func() {
 
 			})
 
+			It("the vmi with cpu.model that cannot match an nfd label on node should not be scheduled", func() {
+
+				nodes, err := virtClient.CoreV1().Nodes().List(metav1.ListOptions{})
+				Expect(err).ToNot(HaveOccurred(), "Should list nodes")
+				Expect(nodes.Items).ToNot(BeEmpty(), "There should be some nodes")
+
+				vmi := tests.NewRandomVMIWithEphemeralDiskAndUserdata(tests.ContainerDiskFor(tests.ContainerDiskCirros), "#!/bin/bash\necho 'hello'\n")
+				vmi.Spec.Domain.CPU = &v1.CPU{
+					Cores: 1,
+					Model: "Conroe",
+				}
+
+				cpuModelLabel, err := services.CPUModelLabelFromCPUModel(vmi)
+				Expect(err).ToNot(HaveOccurred(), "CPU model label should have been retrieved successfully")
+
+				node := &nodes.Items[0]
+				node, err = virtClient.CoreV1().Nodes().Patch(node.Name, types.StrategicMergePatchType,
+					[]byte(fmt.Sprintf(`{"metadata": { "labels": {"%s": "false"}}}`, cpuModelLabel)))
+				Expect(err).ToNot(HaveOccurred(), "Should patch node successfully")
+
+				//Make sure the vmi should try to be scheduled only on master node
+				vmi.Spec.NodeSelector = map[string]string{"kubernetes.io/hostname": node.Name}
+
+				_, err = virtClient.VirtualMachineInstance(vmi.Namespace).Create(vmi)
+				Expect(err).ToNot(HaveOccurred(), "Should create VMI")
+
+				By("Waiting for the VirtualMachineInstance to be unschedulable")
+				Eventually(func() string {
+					curVMI, err := virtClient.VirtualMachineInstance(vmi.Namespace).Get(vmi.Name, &metav1.GetOptions{})
+					Expect(err).ToNot(HaveOccurred(), "Should get vmi")
+					if curVMI.Status.Conditions != nil {
+						return curVMI.Status.Conditions[0].Reason
+					}
+					return ""
+				}, 60*time.Second, 1*time.Second).Should(Equal("Unschedulable"), "VMI should be unchedulable")
+			})
+
 		})
 
 		Context("with non default namespace", func() {