From e93e4bb1b8db5f5a4f95ff877ca9943f86cdcda2 Mon Sep 17 00:00:00 2001 From: Pavel Tishkov Date: Wed, 17 Jun 2026 12:43:19 +0300 Subject: [PATCH 1/4] fix(vmclass): recompute cpu features so vms can schedule on listed available nodes VirtualMachineClass with cpu.type=Discovery kept CPU features from the very first reconcile in Status.CpuFeatures.Enabled. When nodes were added or drained later, the cached feature list was never updated, so the class kept advertising features that no longer existed on availableNodes. This caused VMs stuck in Pending because the virt-launcher pod required labels that no node provided. Compute features from availableNodes on every reconcile so the list always matches the current node set filtered by spec.nodeSelector. Signed-off-by: Pavel Tishkov --- .../controller/vmclass/internal/discovery.go | 38 +++----- .../vmclass/internal/discovery_test.go | 97 +++++++++++++++++++ 2 files changed, 112 insertions(+), 23 deletions(-) diff --git a/images/virtualization-artifact/pkg/controller/vmclass/internal/discovery.go b/images/virtualization-artifact/pkg/controller/vmclass/internal/discovery.go index de4882f3b4..c9a83dde0f 100644 --- a/images/virtualization-artifact/pkg/controller/vmclass/internal/discovery.go +++ b/images/virtualization-artifact/pkg/controller/vmclass/internal/discovery.go @@ -19,7 +19,6 @@ package internal import ( "context" "fmt" - "slices" "sort" "strings" @@ -80,38 +79,33 @@ func (h *DiscoveryHandler) Handle(ctx context.Context, s state.VirtualMachineCla availableNodeNames[i] = n.GetName() } - var ( - featuresEnabled []string - featuresNotEnabled []string - ) + var featuresEnabled []string switch cpuType { case v1alpha2.CPUTypeDiscovery: - if fs := current.Status.CpuFeatures.Enabled; len(fs) > 0 { - featuresEnabled = fs - break - } - featuresEnabled = h.discoveryCommonFeatures(nodes) + // Always recompute features from availableNodes so the result reflects + // the current set of nodes filtered by spec.nodeSelector. Caching + // Status.CpuFeatures.Enabled would lock the class to whatever node + // composition existed at first reconcile and break when nodes are + // added/removed later. + featuresEnabled = h.discoveryCommonFeatures(availableNodes) case v1alpha2.CPUTypeFeatures: featuresEnabled = current.Spec.CPU.Features } - if cpuType == v1alpha2.CPUTypeDiscovery || cpuType == v1alpha2.CPUTypeFeatures { - commonFeatures := h.discoveryCommonFeatures(availableNodes) - for _, cf := range commonFeatures { - if !slices.Contains(featuresEnabled, cf) { - featuresNotEnabled = append(featuresNotEnabled, cf) - } - } - } - cb := conditions.NewConditionBuilder(vmclasscondition.TypeDiscovered).Generation(current.GetGeneration()) switch cpuType { case v1alpha2.CPUTypeDiscovery: + if len(availableNodes) == 0 { + cb.Message("No nodes match the class nodeSelector; skipping feature discovery."). + Reason(vmclasscondition.ReasonDiscoveryFailed). + Status(metav1.ConditionFalse) + break + } if len(featuresEnabled) > 0 { cb.Message("").Reason(vmclasscondition.ReasonDiscoverySucceeded).Status(metav1.ConditionTrue) break } - cb.Message("No common features are discovered on nodes."). + cb.Message("No common CPU features are discovered across available nodes."). Reason(vmclasscondition.ReasonDiscoveryFailed). Status(metav1.ConditionFalse) default: @@ -123,7 +117,6 @@ func (h *DiscoveryHandler) Handle(ctx context.Context, s state.VirtualMachineCla sort.Strings(availableNodeNames) sort.Strings(featuresEnabled) - sort.Strings(featuresNotEnabled) addedNodes, removedNodes := NodeNamesDiff(current.Status.AvailableNodes, availableNodeNames) if len(addedNodes) > 0 || len(removedNodes) > 0 { @@ -150,8 +143,7 @@ func (h *DiscoveryHandler) Handle(ctx context.Context, s state.VirtualMachineCla changed.Status.AvailableNodes = availableNodeNames changed.Status.MaxAllocatableResources = h.maxAllocatableResources(availableNodes) changed.Status.CpuFeatures = v1alpha2.CpuFeatures{ - Enabled: featuresEnabled, - NotEnabledCommon: featuresNotEnabled, + Enabled: featuresEnabled, } return reconcile.Result{}, nil diff --git a/images/virtualization-artifact/pkg/controller/vmclass/internal/discovery_test.go b/images/virtualization-artifact/pkg/controller/vmclass/internal/discovery_test.go index 0fa47ed3aa..3998111337 100644 --- a/images/virtualization-artifact/pkg/controller/vmclass/internal/discovery_test.go +++ b/images/virtualization-artifact/pkg/controller/vmclass/internal/discovery_test.go @@ -519,5 +519,102 @@ var _ = Describe("DiscoveryHandler", func() { Expect(changed.Status.CpuFeatures.Enabled).To(ConsistOf("vmx", "svm")) Expect(changed.Status.CpuFeatures.Enabled).NotTo(ContainElement("lm")) }) + + It("should recompute features when available nodes change, not reuse cached status", func() { + // Regression test: previously the handler cached + // Status.CpuFeatures.Enabled forever, so when a node with a rare + // CPU feature disappeared from availableNodes, the class kept + // advertising that feature and VMs refused to schedule. + nodeOld := newNodeWithLabels("node-old", map[string]string{ + virtv1.CPUFeatureLabel + "vmx": "true", + virtv1.CPUFeatureLabel + "avx": "true", + virtv1.CPUFeatureLabel + "hle": "true", + }) + nodeNew := newNodeWithLabels("node-new", map[string]string{ + virtv1.CPUFeatureLabel + "vmx": "true", + virtv1.CPUFeatureLabel + "avx": "true", + virtv1.CPUFeatureLabel + "fma": "true", + }) + handlerOld := newVirtHandlerPod("node-old") + handlerNew := newVirtHandlerPod("node-new") + + vmc := newVMClass("test-cache-staleness", v1alpha2.CPUTypeDiscovery, nil, nil) + vmcState, resource := setupDiscoveryEnvironment(vmc, + nodeOld, nodeNew, + handlerOld, handlerNew) + + ctx := context.Background() + mockRecorder := &eventrecord.EventRecorderLoggerMock{ + EventfFunc: func(involved client.Object, eventtype, reason, messageFmt string, args ...any) {}, + } + handler := NewDiscoveryHandler(mockRecorder) + + // Two passes are required: first sets the Discovered condition to + // Unknown (addAllUnknown requeue), second performs the actual + // discovery and writes Status.CpuFeatures. + _, err := handler.Handle(ctx, vmcState) + Expect(err).NotTo(HaveOccurred()) + _, err = handler.Handle(ctx, vmcState) + Expect(err).NotTo(HaveOccurred()) + + changed := resource.Changed() + Expect(changed.Status.AvailableNodes).To(ConsistOf("node-old", "node-new")) + Expect(changed.Status.CpuFeatures.Enabled).To(ConsistOf("vmx", "avx")) + + // Simulate node-old disappearing (e.g. drained, deleted) by + // re-running discovery with only node-new in availableNodes. The + // next reconcile must drop hle/rtm from Enabled without manual + // intervention on the class status. + vmcState, resource = setupDiscoveryEnvironment(vmc, + nodeNew, + handlerNew) + // Replay the same reconcile pattern again on the fresh state. + _, err = handler.Handle(ctx, vmcState) + Expect(err).NotTo(HaveOccurred()) + _, err = handler.Handle(ctx, vmcState) + Expect(err).NotTo(HaveOccurred()) + + changed = resource.Changed() + Expect(changed.Status.AvailableNodes).To(ConsistOf("node-new")) + Expect(changed.Status.CpuFeatures.Enabled).To(ConsistOf("vmx", "avx", "fma")) + }) + + It("should mark Discovered=False when available nodes have no common CPU features", func() { + // Pick two labels that exist on opposite nodes so there is no + // common CPU feature between availableNodes. + node1 := newNodeWithLabels("node1", map[string]string{ + virtv1.CPUFeatureLabel + "hle": "true", + }) + node2 := newNodeWithLabels("node2", map[string]string{ + virtv1.CPUFeatureLabel + "rtm": "true", + }) + handler1 := newVirtHandlerPod("node1") + handler2 := newVirtHandlerPod("node2") + + vmc := newVMClass("test-no-common", v1alpha2.CPUTypeDiscovery, nil, nil) + vmcState, resource := setupDiscoveryEnvironment(vmc, + node1, node2, + handler1, handler2) + + ctx := context.Background() + mockRecorder := &eventrecord.EventRecorderLoggerMock{ + EventfFunc: func(involved client.Object, eventtype, reason, messageFmt string, args ...any) {}, + } + handler := NewDiscoveryHandler(mockRecorder) + + _, err := handler.Handle(ctx, vmcState) + Expect(err).NotTo(HaveOccurred()) + _, err = handler.Handle(ctx, vmcState) + Expect(err).NotTo(HaveOccurred()) + + changed := resource.Changed() + Expect(changed.Status.AvailableNodes).To(ConsistOf("node1", "node2")) + Expect(changed.Status.CpuFeatures.Enabled).To(BeEmpty()) + + cond := conditions.FindStatusCondition(changed.Status.Conditions, vmclasscondition.TypeDiscovered.String()) + Expect(cond).NotTo(BeNil()) + Expect(cond.Status).To(Equal(metav1.ConditionFalse)) + Expect(cond.Reason).To(Equal(vmclasscondition.ReasonDiscoveryFailed.String())) + }) }) }) From 3ec74ed4bda0ae49b00449779b42bc548dbafb99 Mon Sep 17 00:00:00 2001 From: Pavel Tishkov Date: Sat, 20 Jun 2026 22:13:34 +0300 Subject: [PATCH 2/4] test(vmclass): silence unparam on newVMClass helper Adding two Discovery regression tests tipped unparam past its call-count threshold and it started flagging newVMClass because cpuType is always CPUTypeDiscovery. The helper keeps a generic signature for reuse across test cases, so silence the linter locally instead of shrinking the API. Signed-off-by: Pavel Tishkov --- .../pkg/controller/vmclass/internal/discovery_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/images/virtualization-artifact/pkg/controller/vmclass/internal/discovery_test.go b/images/virtualization-artifact/pkg/controller/vmclass/internal/discovery_test.go index 3998111337..9cc95f5bec 100644 --- a/images/virtualization-artifact/pkg/controller/vmclass/internal/discovery_test.go +++ b/images/virtualization-artifact/pkg/controller/vmclass/internal/discovery_test.go @@ -69,6 +69,7 @@ func newVirtHandlerPod(nodeName string) *corev1.Pod { } } +//nolint:unparam // helper preserves generic signature for reuse across test cases func newVMClass(name string, cpuType v1alpha2.CPUType, nodeSelector *v1alpha2.NodeSelector, discoveryNodeSelector *metav1.LabelSelector) *v1alpha2.VirtualMachineClass { vmc := &v1alpha2.VirtualMachineClass{ TypeMeta: metav1.TypeMeta{ From 28f27d7144131227020f5642bb2a5349f860fb37 Mon Sep 17 00:00:00 2001 From: Pavel Tishkov Date: Mon, 22 Jun 2026 11:02:39 +0300 Subject: [PATCH 3/4] fix(vmclass): separate discovery pool from schedulable nodes discovery.nodeSelector was applied as a label selector in Nodes(), which made it restrict availableNodes instead of only scoping the feature-discovery pool. As a result a class with spec.cpu.discovery.nodeSelector={hostname: pt-0} and spec.nodeSelector={} reported availableNodes=[pt-0] even though pt-1/pt-2 expose the same CPU features and should be schedulable. Per the CRD, discovery.nodeSelector selects nodes used as the basis for building a universal CPU model; it must not constrain scheduling. Now features are intersected from the discovery pool (DiscoveryNodes), and availableNodes is derived from spec.nodeSelector filtered down to nodes exposing every discovered feature. - state.go: add DiscoveryNodes for the discovery pool; Nodes() no longer filters by discovery.nodeSelector for cpu.type=Discovery. - discovery.go: compute features from DiscoveryNodes, filter availableNodes by discovered features, update Discovered=False branch messages to reflect the discovery pool. - discovery_test.go: rewrite filter tests for the new semantics; add pool-separation and feature-mismatch exclusion tests. Signed-off-by: Pavel Tishkov --- .../controller/vmclass/internal/discovery.go | 60 +++++++-- .../vmclass/internal/discovery_test.go | 127 +++++++++++++++++- .../vmclass/internal/state/state.go | 62 +++++++-- 3 files changed, 225 insertions(+), 24 deletions(-) diff --git a/images/virtualization-artifact/pkg/controller/vmclass/internal/discovery.go b/images/virtualization-artifact/pkg/controller/vmclass/internal/discovery.go index c9a83dde0f..b0105c2a07 100644 --- a/images/virtualization-artifact/pkg/controller/vmclass/internal/discovery.go +++ b/images/virtualization-artifact/pkg/controller/vmclass/internal/discovery.go @@ -74,29 +74,33 @@ func (h *DiscoveryHandler) Handle(ctx context.Context, s state.VirtualMachineCla return reconcile.Result{}, err } - availableNodeNames := make([]string, len(availableNodes)) - for i, n := range availableNodes { - availableNodeNames[i] = n.GetName() - } - var featuresEnabled []string + var discoveryPool []corev1.Node switch cpuType { case v1alpha2.CPUTypeDiscovery: - // Always recompute features from availableNodes so the result reflects - // the current set of nodes filtered by spec.nodeSelector. Caching - // Status.CpuFeatures.Enabled would lock the class to whatever node - // composition existed at first reconcile and break when nodes are - // added/removed later. - featuresEnabled = h.discoveryCommonFeatures(availableNodes) + // Discover features from the discovery nodeSelector pool, then restrict + // schedulable nodes to those that expose every discovered feature so + // VMs can only land on nodes compatible with the universal CPU model. + discoveryPool, err = s.DiscoveryNodes(ctx) + if err != nil { + return reconcile.Result{}, err + } + featuresEnabled = h.discoveryCommonFeatures(discoveryPool) + availableNodes = h.nodesWithAllFeatures(availableNodes, featuresEnabled) case v1alpha2.CPUTypeFeatures: featuresEnabled = current.Spec.CPU.Features } + availableNodeNames := make([]string, len(availableNodes)) + for i, n := range availableNodes { + availableNodeNames[i] = n.GetName() + } + cb := conditions.NewConditionBuilder(vmclasscondition.TypeDiscovered).Generation(current.GetGeneration()) switch cpuType { case v1alpha2.CPUTypeDiscovery: - if len(availableNodes) == 0 { - cb.Message("No nodes match the class nodeSelector; skipping feature discovery."). + if len(discoveryPool) == 0 { + cb.Message("No nodes match the discovery nodeSelector; skipping feature discovery."). Reason(vmclasscondition.ReasonDiscoveryFailed). Status(metav1.ConditionFalse) break @@ -105,7 +109,7 @@ func (h *DiscoveryHandler) Handle(ctx context.Context, s state.VirtualMachineCla cb.Message("").Reason(vmclasscondition.ReasonDiscoverySucceeded).Status(metav1.ConditionTrue) break } - cb.Message("No common CPU features are discovered across available nodes."). + cb.Message("No common CPU features are discovered across discovery nodes."). Reason(vmclasscondition.ReasonDiscoveryFailed). Status(metav1.ConditionFalse) default: @@ -203,6 +207,34 @@ func (h *DiscoveryHandler) discoveryCommonFeatures(nodes []corev1.Node) []string return features } +// nodesWithAllFeatures keeps only nodes whose labels confirm every feature is +// present. When features is empty no filtering is applied and nodes are +// returned as-is. +func (h *DiscoveryHandler) nodesWithAllFeatures(nodes []corev1.Node, features []string) []corev1.Node { + if len(features) == 0 { + return nodes + } + required := make(map[string]struct{}, len(features)) + for _, f := range features { + required[virtv1.CPUFeatureLabel+f] = struct{}{} + } + result := make([]corev1.Node, 0, len(nodes)) + for _, n := range nodes { + labels := n.GetLabels() + ok := true + for k := range required { + if labels[k] != "true" { + ok = false + break + } + } + if ok { + result = append(result, n) + } + } + return result +} + func (h *DiscoveryHandler) maxAllocatableResources(nodes []corev1.Node) corev1.ResourceList { var ( resourceList corev1.ResourceList = make(map[corev1.ResourceName]resource.Quantity) diff --git a/images/virtualization-artifact/pkg/controller/vmclass/internal/discovery_test.go b/images/virtualization-artifact/pkg/controller/vmclass/internal/discovery_test.go index 9cc95f5bec..00b4a15232 100644 --- a/images/virtualization-artifact/pkg/controller/vmclass/internal/discovery_test.go +++ b/images/virtualization-artifact/pkg/controller/vmclass/internal/discovery_test.go @@ -411,7 +411,7 @@ var _ = Describe("DiscoveryHandler", func() { Expect(cond.Reason).To(Equal(vmclasscondition.ReasonDiscoverySucceeded.String())) }) - It("should filter nodes by discovery.nodeSelector.matchLabels", func() { + It("should derive features from discovery.nodeSelector.matchLabels pool", func() { node1 := newNodeWithLabels("node1", map[string]string{ "node.deckhouse.io/group": "worker", virtv1.CPUFeatureLabel + "vmx": "true", @@ -459,11 +459,16 @@ var _ = Describe("DiscoveryHandler", func() { changed := resource.Changed() + // Features come from the discovery pool (worker nodes only), so lm + // is excluded even though node3 has it. Expect(changed.Status.CpuFeatures.Enabled).To(ConsistOf("vmx", "svm")) Expect(changed.Status.CpuFeatures.Enabled).NotTo(ContainElement("lm")) + // spec.nodeSelector is empty, so every node exposing the discovered + // features is schedulable, including node3 from outside the pool. + Expect(changed.Status.AvailableNodes).To(ConsistOf("node1", "node2", "node3")) }) - It("should filter nodes by discovery.nodeSelector.matchExpressions with In operator", func() { + It("should derive features from discovery.nodeSelector.matchExpressions pool", func() { node1 := newNodeWithLabels("node1", map[string]string{ "node.deckhouse.io/group": "worker", virtv1.CPUFeatureLabel + "vmx": "true", @@ -519,6 +524,124 @@ var _ = Describe("DiscoveryHandler", func() { Expect(changed.Status.CpuFeatures.Enabled).To(ConsistOf("vmx", "svm")) Expect(changed.Status.CpuFeatures.Enabled).NotTo(ContainElement("lm")) + // spec.nodeSelector is empty, so node3 also remains schedulable as it + // exposes all discovered features. + Expect(changed.Status.AvailableNodes).To(ConsistOf("node1", "node2", "node3")) + }) + + It("should keep discovery pool and schedulable nodes separate", func() { + // discovery.nodeSelector narrows the feature-discovery pool, while + // spec.nodeSelector narrows where VMs schedule. A node outside the + // discovery pool but matching spec.nodeSelector and exposing the + // discovered features must appear in availableNodes. + node1 := newNodeWithLabels("node1", map[string]string{ + "node.deckhouse.io/group": "worker", + virtv1.CPUFeatureLabel + "vmx": "true", + virtv1.CPUFeatureLabel + "svm": "true", + }) + node2 := newNodeWithLabels("node2", map[string]string{ + "node.deckhouse.io/group": "compute", + virtv1.CPUFeatureLabel + "vmx": "true", + virtv1.CPUFeatureLabel + "svm": "true", + }) + + virtHandler1 := newVirtHandlerPod("node1") + virtHandler2 := newVirtHandlerPod("node2") + + // Discovery pool is worker only; spec.nodeSelector allows compute too. + vmc := newVMClass("test-pool-separation", + v1alpha2.CPUTypeDiscovery, + &v1alpha2.NodeSelector{ + MatchExpressions: []corev1.NodeSelectorRequirement{ + { + Key: "node.deckhouse.io/group", + Operator: corev1.NodeSelectorOpIn, + Values: []string{"worker", "compute"}, + }, + }, + }, + &metav1.LabelSelector{ + MatchLabels: map[string]string{"node.deckhouse.io/group": "worker"}, + }, + ) + + vmcState, resource := setupDiscoveryEnvironment(vmc, + node1, node2, + virtHandler1, virtHandler2) + + ctx := context.Background() + + mockRecorder := &eventrecord.EventRecorderLoggerMock{ + EventfFunc: func(involved client.Object, eventtype, reason, messageFmt string, args ...any) {}, + } + handler := NewDiscoveryHandler(mockRecorder) + + _, err := handler.Handle(ctx, vmcState) + Expect(err).NotTo(HaveOccurred()) + _, err = handler.Handle(ctx, vmcState) + Expect(err).NotTo(HaveOccurred()) + + changed := resource.Changed() + + // Features discovered from the worker pool only. + Expect(changed.Status.CpuFeatures.Enabled).To(ConsistOf("vmx", "svm")) + // Both nodes match spec.nodeSelector and expose the discovered + // features, so both are schedulable even though node2 is outside the + // discovery pool. + Expect(changed.Status.AvailableNodes).To(ConsistOf("node1", "node2")) + }) + + It("should exclude schedulable nodes missing a discovered feature", func() { + // A node matching spec.nodeSelector but lacking a feature present in + // the discovery pool must not be schedulable, since the universal CPU + // model would not run on it. + node1 := newNodeWithLabels("node1", map[string]string{ + "kubernetes.io/hostname": "node1", + "node.deckhouse.io/group": "worker", + virtv1.CPUFeatureLabel + "vmx": "true", + virtv1.CPUFeatureLabel + "svm": "true", + }) + node2 := newNodeWithLabels("node2", map[string]string{ + "node.deckhouse.io/group": "worker", + virtv1.CPUFeatureLabel + "vmx": "true", + }) + + virtHandler1 := newVirtHandlerPod("node1") + virtHandler2 := newVirtHandlerPod("node2") + + // Discovery pool is node1 only; spec.nodeSelector is empty so both + // nodes are candidates for scheduling. + vmc := newVMClass("test-feature-mismatch", + v1alpha2.CPUTypeDiscovery, + nil, + &metav1.LabelSelector{ + MatchLabels: map[string]string{"kubernetes.io/hostname": "node1"}, + }, + ) + + vmcState, resource := setupDiscoveryEnvironment(vmc, + node1, node2, + virtHandler1, virtHandler2) + + ctx := context.Background() + + mockRecorder := &eventrecord.EventRecorderLoggerMock{ + EventfFunc: func(involved client.Object, eventtype, reason, messageFmt string, args ...any) {}, + } + handler := NewDiscoveryHandler(mockRecorder) + + _, err := handler.Handle(ctx, vmcState) + Expect(err).NotTo(HaveOccurred()) + _, err = handler.Handle(ctx, vmcState) + Expect(err).NotTo(HaveOccurred()) + + changed := resource.Changed() + + // Discovered model is derived from node1 only. + Expect(changed.Status.CpuFeatures.Enabled).To(ConsistOf("vmx", "svm")) + // node2 misses svm, so it cannot run the universal CPU model and is + // excluded from availableNodes even though it matches spec.nodeSelector. + Expect(changed.Status.AvailableNodes).To(ConsistOf("node1")) }) It("should recompute features when available nodes change, not reuse cached status", func() { diff --git a/images/virtualization-artifact/pkg/controller/vmclass/internal/state/state.go b/images/virtualization-artifact/pkg/controller/vmclass/internal/state/state.go index b9eafb90c6..145fcbed0b 100644 --- a/images/virtualization-artifact/pkg/controller/vmclass/internal/state/state.go +++ b/images/virtualization-artifact/pkg/controller/vmclass/internal/state/state.go @@ -38,6 +38,7 @@ type VirtualMachineClassState interface { VirtualMachineClass() *reconciler.Resource[*v1alpha2.VirtualMachineClass, v1alpha2.VirtualMachineClassStatus] VirtualMachines(ctx context.Context) ([]v1alpha2.VirtualMachine, error) Nodes(ctx context.Context) ([]corev1.Node, error) + DiscoveryNodes(ctx context.Context) ([]corev1.Node, error) AvailableNodes(nodes []corev1.Node) ([]corev1.Node, error) } @@ -98,14 +99,10 @@ func (s *state) Nodes(ctx context.Context) ([]corev1.Node, error) { case v1alpha2.CPUTypeHost, v1alpha2.CPUTypeHostPassthrough: // Node is always has the "Host" CPU type, no additional filters required. case v1alpha2.CPUTypeDiscovery: - var matchExpressions []metav1.LabelSelectorRequirement - if discovery := curr.Spec.CPU.Discovery; discovery != nil { - matchLabels = discovery.NodeSelector.MatchLabels - matchExpressions = discovery.NodeSelector.MatchExpressions - } - filters = append(filters, func(node *corev1.Node) bool { - return annotations.MatchExpressions(node.GetLabels(), matchExpressions) - }) + // Discovery selects the feature-discovery pool via DiscoveryNodes; + // here we list all virt-handler nodes so AvailableNodes can derive + // schedulable nodes from spec.nodeSelector filtered by discovered + // features. case v1alpha2.CPUTypeModel: matchLabels = map[string]string{virtv1.CPUModelLabel + curr.Spec.CPU.Model: "true"} case v1alpha2.CPUTypeFeatures: @@ -129,6 +126,55 @@ func (s *state) Nodes(ctx context.Context) ([]corev1.Node, error) { return nodeFilter(nodes.Items, filters...), nil } +// DiscoveryNodes returns the pool of virt-handler nodes selected by +// spec.cpu.discovery.nodeSelector. This pool is the basis for intersecting +// CPU features into a universal CPU model; it does not restrict where VMs +// can schedule. Schedulable nodes are derived in AvailableNodes from +// spec.nodeSelector and filtered by the discovered features by the handler. +func (s *state) DiscoveryNodes(ctx context.Context) ([]corev1.Node, error) { + if s.vmClass == nil || s.vmClass.IsEmpty() { + return nil, nil + } + curr := s.vmClass.Current() + if curr.Spec.CPU.Type != v1alpha2.CPUTypeDiscovery { + return nil, nil + } + + var ( + matchLabels map[string]string + matchExpressions []metav1.LabelSelectorRequirement + ) + if discovery := curr.Spec.CPU.Discovery; discovery != nil { + matchLabels = discovery.NodeSelector.MatchLabels + matchExpressions = discovery.NodeSelector.MatchExpressions + } + + virtHandlerNodes, err := s.getVirtHandlerNodeNames(ctx) + if err != nil { + return nil, err + } + filters := []array.FilterFunc[corev1.Node]{ + func(node *corev1.Node) bool { + _, found := virtHandlerNodes[node.GetName()] + return found + }, + func(node *corev1.Node) bool { + return annotations.MatchExpressions(node.GetLabels(), matchExpressions) + }, + } + + nodes := &corev1.NodeList{} + err = s.client.List( + ctx, + nodes, + client.MatchingLabelsSelector{Selector: labels.SelectorFromSet(matchLabels)}) + if err != nil { + return nil, fmt.Errorf("failed to list nodes: %w", err) + } + + return nodeFilter(nodes.Items, filters...), nil +} + func (s *state) getVirtHandlerNodeNames(ctx context.Context) (map[string]struct{}, error) { pods := &corev1.PodList{} err := s.client.List(ctx, pods, client.InNamespace(s.controllerNamespace), From d649cfd601e1f23f1a0399191856a05a10126a76 Mon Sep 17 00:00:00 2001 From: Pavel Tishkov Date: Mon, 22 Jun 2026 23:02:57 +0300 Subject: [PATCH 4/4] fix(vmclass): restore notEnabledCommon cpu features for Discovery and Features types Commit bb87e014b accidentally removed the calculation of status.cpuFeatures.notEnabledCommon when fixing the stale feature cache. The field stayed in the CRD/types/conversion but was always empty. Restore variant B semantics: notEnabledCommon = commonFeatures(availableNodes) minus featuresEnabled. For Features this surfaces CPU instructions common to all schedulable nodes that the user did not request in spec.cpu.features. For Discovery it reports features common to the (possibly shrunk) schedulable set beyond the discovered model. Cover with unit tests: UF1-UFP for Features via Handle (previously untested), UF4/UF6 for notEnabledCommon on Features/Discovery, UF5 empty-case for Discovery. Signed-off-by: Pavel Tishkov --- .../controller/vmclass/internal/discovery.go | 19 +- .../vmclass/internal/discovery_test.go | 290 +++++++++++++++++- 2 files changed, 307 insertions(+), 2 deletions(-) diff --git a/images/virtualization-artifact/pkg/controller/vmclass/internal/discovery.go b/images/virtualization-artifact/pkg/controller/vmclass/internal/discovery.go index b0105c2a07..0eb89414f4 100644 --- a/images/virtualization-artifact/pkg/controller/vmclass/internal/discovery.go +++ b/images/virtualization-artifact/pkg/controller/vmclass/internal/discovery.go @@ -19,6 +19,7 @@ package internal import ( "context" "fmt" + "slices" "sort" "strings" @@ -91,6 +92,20 @@ func (h *DiscoveryHandler) Handle(ctx context.Context, s state.VirtualMachineCla featuresEnabled = current.Spec.CPU.Features } + // notEnabledCommon: CPU features common to every available node but not + // part of the enabled model. For Discovery these are features present on + // all schedulable nodes beyond the discovered intersection; for Features + // these are features the nodes support but the user did not request. + var featuresNotEnabled []string + if cpuType == v1alpha2.CPUTypeDiscovery || cpuType == v1alpha2.CPUTypeFeatures { + commonFeatures := h.discoveryCommonFeatures(availableNodes) + for _, cf := range commonFeatures { + if !slices.Contains(featuresEnabled, cf) { + featuresNotEnabled = append(featuresNotEnabled, cf) + } + } + } + availableNodeNames := make([]string, len(availableNodes)) for i, n := range availableNodes { availableNodeNames[i] = n.GetName() @@ -121,6 +136,7 @@ func (h *DiscoveryHandler) Handle(ctx context.Context, s state.VirtualMachineCla sort.Strings(availableNodeNames) sort.Strings(featuresEnabled) + sort.Strings(featuresNotEnabled) addedNodes, removedNodes := NodeNamesDiff(current.Status.AvailableNodes, availableNodeNames) if len(addedNodes) > 0 || len(removedNodes) > 0 { @@ -147,7 +163,8 @@ func (h *DiscoveryHandler) Handle(ctx context.Context, s state.VirtualMachineCla changed.Status.AvailableNodes = availableNodeNames changed.Status.MaxAllocatableResources = h.maxAllocatableResources(availableNodes) changed.Status.CpuFeatures = v1alpha2.CpuFeatures{ - Enabled: featuresEnabled, + Enabled: featuresEnabled, + NotEnabledCommon: featuresNotEnabled, } return reconcile.Result{}, nil diff --git a/images/virtualization-artifact/pkg/controller/vmclass/internal/discovery_test.go b/images/virtualization-artifact/pkg/controller/vmclass/internal/discovery_test.go index 00b4a15232..a676703d13 100644 --- a/images/virtualization-artifact/pkg/controller/vmclass/internal/discovery_test.go +++ b/images/virtualization-artifact/pkg/controller/vmclass/internal/discovery_test.go @@ -69,7 +69,6 @@ func newVirtHandlerPod(nodeName string) *corev1.Pod { } } -//nolint:unparam // helper preserves generic signature for reuse across test cases func newVMClass(name string, cpuType v1alpha2.CPUType, nodeSelector *v1alpha2.NodeSelector, discoveryNodeSelector *metav1.LabelSelector) *v1alpha2.VirtualMachineClass { vmc := &v1alpha2.VirtualMachineClass{ TypeMeta: metav1.TypeMeta{ @@ -740,5 +739,294 @@ var _ = Describe("DiscoveryHandler", func() { Expect(cond.Status).To(Equal(metav1.ConditionFalse)) Expect(cond.Reason).To(Equal(vmclasscondition.ReasonDiscoveryFailed.String())) }) + + It("should populate enabled from spec.cpu.features and keep all matching nodes for Features type", func() { + // UF1: Features type takes enabled features verbatim from spec; no + // discovery happens and every node exposing the requested features is + // schedulable. + node1 := newNodeWithLabels("node1", map[string]string{ + virtv1.CPUFeatureLabel + "vmx": "true", + virtv1.CPUFeatureLabel + "svm": "true", + }) + node2 := newNodeWithLabels("node2", map[string]string{ + virtv1.CPUFeatureLabel + "vmx": "true", + virtv1.CPUFeatureLabel + "svm": "true", + }) + node3 := newNodeWithLabels("node3", map[string]string{ + virtv1.CPUFeatureLabel + "vmx": "true", + virtv1.CPUFeatureLabel + "svm": "true", + }) + + virtHandler1 := newVirtHandlerPod("node1") + virtHandler2 := newVirtHandlerPod("node2") + virtHandler3 := newVirtHandlerPod("node3") + + vmc := newVMClass("test-features-basic", v1alpha2.CPUTypeFeatures, nil, nil) + vmc.Spec.CPU.Features = []string{"vmx", "svm"} + + vmcState, resource := setupDiscoveryEnvironment(vmc, + node1, node2, node3, + virtHandler1, virtHandler2, virtHandler3) + + ctx := context.Background() + + mockRecorder := &eventrecord.EventRecorderLoggerMock{ + EventfFunc: func(involved client.Object, eventtype, reason, messageFmt string, args ...any) {}, + } + handler := NewDiscoveryHandler(mockRecorder) + + _, err := handler.Handle(ctx, vmcState) + Expect(err).NotTo(HaveOccurred()) + _, err = handler.Handle(ctx, vmcState) + Expect(err).NotTo(HaveOccurred()) + + changed := resource.Changed() + + Expect(changed.Status.AvailableNodes).To(ConsistOf("node1", "node2", "node3")) + Expect(changed.Status.CpuFeatures.Enabled).To(ConsistOf("vmx", "svm")) + Expect(changed.Status.CpuFeatures.NotEnabledCommon).To(BeEmpty()) + + cond := conditions.FindStatusCondition(changed.Status.Conditions, vmclasscondition.TypeDiscovered.String()) + Expect(cond).NotTo(BeNil()) + Expect(cond.Status).To(Equal(metav1.ConditionFalse)) + Expect(cond.Reason).To(Equal(vmclasscondition.ReasonDiscoverySkip.String())) + }) + + It("should exclude nodes missing a requested feature for Features type", func() { + // UF2: Nodes() filters via matchLabels on every spec feature, so a + // node lacking any requested feature never reaches availableNodes. + node1 := newNodeWithLabels("node1", map[string]string{ + virtv1.CPUFeatureLabel + "vmx": "true", + virtv1.CPUFeatureLabel + "svm": "true", + }) + node2 := newNodeWithLabels("node2", map[string]string{ + virtv1.CPUFeatureLabel + "vmx": "true", + }) + + virtHandler1 := newVirtHandlerPod("node1") + virtHandler2 := newVirtHandlerPod("node2") + + vmc := newVMClass("test-features-missing", v1alpha2.CPUTypeFeatures, nil, nil) + vmc.Spec.CPU.Features = []string{"vmx", "svm"} + + vmcState, resource := setupDiscoveryEnvironment(vmc, + node1, node2, + virtHandler1, virtHandler2) + + ctx := context.Background() + + mockRecorder := &eventrecord.EventRecorderLoggerMock{ + EventfFunc: func(involved client.Object, eventtype, reason, messageFmt string, args ...any) {}, + } + handler := NewDiscoveryHandler(mockRecorder) + + _, err := handler.Handle(ctx, vmcState) + Expect(err).NotTo(HaveOccurred()) + _, err = handler.Handle(ctx, vmcState) + Expect(err).NotTo(HaveOccurred()) + + changed := resource.Changed() + + Expect(changed.Status.AvailableNodes).To(ConsistOf("node1")) + Expect(changed.Status.CpuFeatures.Enabled).To(ConsistOf("vmx", "svm")) + Expect(changed.Status.CpuFeatures.NotEnabledCommon).To(BeEmpty()) + }) + + It("should narrow availableNodes by spec.nodeSelector for Features type", func() { + // UF3: spec.nodeSelector restricts scheduling independently of the + // feature match performed by Nodes(). + node1 := newNodeWithLabels("node1", map[string]string{ + "node.deckhouse.io/group": "worker", + virtv1.CPUFeatureLabel + "vmx": "true", + }) + node2 := newNodeWithLabels("node2", map[string]string{ + "node.deckhouse.io/group": "worker", + virtv1.CPUFeatureLabel + "vmx": "true", + }) + node3 := newNodeWithLabels("node3", map[string]string{ + "node.deckhouse.io/group": "master", + virtv1.CPUFeatureLabel + "vmx": "true", + }) + + virtHandler1 := newVirtHandlerPod("node1") + virtHandler2 := newVirtHandlerPod("node2") + virtHandler3 := newVirtHandlerPod("node3") + + vmc := newVMClass("test-features-nodeselector", v1alpha2.CPUTypeFeatures, &v1alpha2.NodeSelector{ + MatchExpressions: []corev1.NodeSelectorRequirement{ + { + Key: "node.deckhouse.io/group", + Operator: corev1.NodeSelectorOpIn, + Values: []string{"worker"}, + }, + }, + }, nil) + vmc.Spec.CPU.Features = []string{"vmx"} + + vmcState, resource := setupDiscoveryEnvironment(vmc, + node1, node2, node3, + virtHandler1, virtHandler2, virtHandler3) + + ctx := context.Background() + + mockRecorder := &eventrecord.EventRecorderLoggerMock{ + EventfFunc: func(involved client.Object, eventtype, reason, messageFmt string, args ...any) {}, + } + handler := NewDiscoveryHandler(mockRecorder) + + _, err := handler.Handle(ctx, vmcState) + Expect(err).NotTo(HaveOccurred()) + _, err = handler.Handle(ctx, vmcState) + Expect(err).NotTo(HaveOccurred()) + + changed := resource.Changed() + + Expect(changed.Status.AvailableNodes).To(ConsistOf("node1", "node2")) + Expect(changed.Status.CpuFeatures.Enabled).To(ConsistOf("vmx")) + Expect(changed.Status.CpuFeatures.NotEnabledCommon).To(BeEmpty()) + }) + + It("should report notEnabledCommon for Features type when nodes share extra features", func() { + // UF4: notEnabledCommon (variant B) = commonFeatures(availableNodes) + // minus spec.cpu.features. Every schedulable node supports sse2 but + // the user did not request it, so it must be reported as not enabled. + node1 := newNodeWithLabels("node1", map[string]string{ + virtv1.CPUFeatureLabel + "vmx": "true", + virtv1.CPUFeatureLabel + "sse2": "true", + }) + node2 := newNodeWithLabels("node2", map[string]string{ + virtv1.CPUFeatureLabel + "vmx": "true", + virtv1.CPUFeatureLabel + "sse2": "true", + }) + + virtHandler1 := newVirtHandlerPod("node1") + virtHandler2 := newVirtHandlerPod("node2") + + vmc := newVMClass("test-features-notenabled", v1alpha2.CPUTypeFeatures, nil, nil) + vmc.Spec.CPU.Features = []string{"vmx"} + + vmcState, resource := setupDiscoveryEnvironment(vmc, + node1, node2, + virtHandler1, virtHandler2) + + ctx := context.Background() + + mockRecorder := &eventrecord.EventRecorderLoggerMock{ + EventfFunc: func(involved client.Object, eventtype, reason, messageFmt string, args ...any) {}, + } + handler := NewDiscoveryHandler(mockRecorder) + + _, err := handler.Handle(ctx, vmcState) + Expect(err).NotTo(HaveOccurred()) + _, err = handler.Handle(ctx, vmcState) + Expect(err).NotTo(HaveOccurred()) + + changed := resource.Changed() + + Expect(changed.Status.AvailableNodes).To(ConsistOf("node1", "node2")) + Expect(changed.Status.CpuFeatures.Enabled).To(ConsistOf("vmx")) + Expect(changed.Status.CpuFeatures.NotEnabledCommon).To(ConsistOf("sse2")) + }) + + It("should leave notEnabledCommon empty for Discovery when availableNodes match the discovered model", func() { + // UF5: for Discovery, availableNodes are filtered by nodesWithAllFeatures + // against the discovered intersection, so commonFeatures(availableNodes) + // collapses back to the enabled set and notEnabledCommon stays empty. + // A non-empty result is only possible when spec.nodeSelector excludes + // part of the discovery pool, shrinking the common set below the model. + node1 := newNodeWithLabels("node1", map[string]string{ + virtv1.CPUFeatureLabel + "vmx": "true", + virtv1.CPUFeatureLabel + "avx": "true", + virtv1.CPUFeatureLabel + "sse2": "true", + }) + node2 := newNodeWithLabels("node2", map[string]string{ + virtv1.CPUFeatureLabel + "vmx": "true", + virtv1.CPUFeatureLabel + "avx": "true", + virtv1.CPUFeatureLabel + "sse2": "true", + }) + + virtHandler1 := newVirtHandlerPod("node1") + virtHandler2 := newVirtHandlerPod("node2") + + vmc := newVMClass("test-discovery-notenabled-empty", v1alpha2.CPUTypeDiscovery, nil, nil) + + vmcState, resource := setupDiscoveryEnvironment(vmc, + node1, node2, + virtHandler1, virtHandler2) + + ctx := context.Background() + + mockRecorder := &eventrecord.EventRecorderLoggerMock{ + EventfFunc: func(involved client.Object, eventtype, reason, messageFmt string, args ...any) {}, + } + handler := NewDiscoveryHandler(mockRecorder) + + _, err := handler.Handle(ctx, vmcState) + Expect(err).NotTo(HaveOccurred()) + _, err = handler.Handle(ctx, vmcState) + Expect(err).NotTo(HaveOccurred()) + + changed := resource.Changed() + + Expect(changed.Status.AvailableNodes).To(ConsistOf("node1", "node2")) + Expect(changed.Status.CpuFeatures.Enabled).To(ConsistOf("vmx", "avx", "sse2")) + Expect(changed.Status.CpuFeatures.NotEnabledCommon).To(BeEmpty()) + }) + + It("should report notEnabledCommon for Discovery when spec.nodeSelector shrinks availableNodes below the discovery pool", func() { + // UF6: discovery pool is {node1,node2} with common features + // {vmx,avx,sse2}, but spec.nodeSelector keeps only node1, whose labels + // add clwb. commonFeatures(availableNodes={node1}) = {vmx,avx,sse2,clwb}, + // so clwb is common to the (shrunk) available set yet not part of the + // discovered model and must surface in notEnabledCommon. + node1 := newNodeWithLabels("node1", map[string]string{ + "node.deckhouse.io/group": "worker", + virtv1.CPUFeatureLabel + "vmx": "true", + virtv1.CPUFeatureLabel + "avx": "true", + virtv1.CPUFeatureLabel + "sse2": "true", + virtv1.CPUFeatureLabel + "clwb": "true", + }) + node2 := newNodeWithLabels("node2", map[string]string{ + "node.deckhouse.io/group": "compute", + virtv1.CPUFeatureLabel + "vmx": "true", + virtv1.CPUFeatureLabel + "avx": "true", + virtv1.CPUFeatureLabel + "sse2": "true", + }) + + virtHandler1 := newVirtHandlerPod("node1") + virtHandler2 := newVirtHandlerPod("node2") + + vmc := newVMClass("test-discovery-notenabled-shrink", v1alpha2.CPUTypeDiscovery, &v1alpha2.NodeSelector{ + MatchExpressions: []corev1.NodeSelectorRequirement{ + { + Key: "node.deckhouse.io/group", + Operator: corev1.NodeSelectorOpIn, + Values: []string{"worker"}, + }, + }, + }, &metav1.LabelSelector{}) + + vmcState, resource := setupDiscoveryEnvironment(vmc, + node1, node2, + virtHandler1, virtHandler2) + + ctx := context.Background() + + mockRecorder := &eventrecord.EventRecorderLoggerMock{ + EventfFunc: func(involved client.Object, eventtype, reason, messageFmt string, args ...any) {}, + } + handler := NewDiscoveryHandler(mockRecorder) + + _, err := handler.Handle(ctx, vmcState) + Expect(err).NotTo(HaveOccurred()) + _, err = handler.Handle(ctx, vmcState) + Expect(err).NotTo(HaveOccurred()) + + changed := resource.Changed() + + Expect(changed.Status.AvailableNodes).To(ConsistOf("node1")) + Expect(changed.Status.CpuFeatures.Enabled).To(ConsistOf("vmx", "avx", "sse2")) + Expect(changed.Status.CpuFeatures.NotEnabledCommon).To(ConsistOf("clwb")) + }) }) })