From 325c8790075e49e59c0fea6a34494f13a332e67c Mon Sep 17 00:00:00 2001 From: Daniil Loktev Date: Mon, 23 Mar 2026 17:35:19 +0300 Subject: [PATCH 1/6] wip Signed-off-by: Daniil Loktev --- .../validators/pv_node_affinity_validator.go | 144 ++++++++++++ .../pv_node_affinity_validator_test.go | 214 ++++++++++++++++++ .../pkg/controller/vm/vm_webhook.go | 1 + .../validators/pv_node_affinity_validator.go | 123 ++++++++++ .../pkg/controller/vmbda/vmbda_webhook.go | 1 + 5 files changed, 483 insertions(+) create mode 100644 images/virtualization-artifact/pkg/controller/vm/internal/validators/pv_node_affinity_validator.go create mode 100644 images/virtualization-artifact/pkg/controller/vm/internal/validators/pv_node_affinity_validator_test.go create mode 100644 images/virtualization-artifact/pkg/controller/vmbda/internal/validators/pv_node_affinity_validator.go diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/validators/pv_node_affinity_validator.go b/images/virtualization-artifact/pkg/controller/vm/internal/validators/pv_node_affinity_validator.go new file mode 100644 index 0000000000..d8424411b2 --- /dev/null +++ b/images/virtualization-artifact/pkg/controller/vm/internal/validators/pv_node_affinity_validator.go @@ -0,0 +1,144 @@ +/* +Copyright 2026 Flant JSC + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package validators + +import ( + "context" + "fmt" + "reflect" + + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/types" + "k8s.io/component-helpers/scheduling/corev1/nodeaffinity" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + "github.com/deckhouse/virtualization-controller/pkg/common/object" + "github.com/deckhouse/virtualization/api/core/v1alpha2" +) + +type PVNodeAffinityValidator struct { + client client.Client +} + +func NewPVNodeAffinityValidator(client client.Client) *PVNodeAffinityValidator { + return &PVNodeAffinityValidator{client: client} +} + +func (v *PVNodeAffinityValidator) ValidateCreate(_ context.Context, _ *v1alpha2.VirtualMachine) (admission.Warnings, error) { + return nil, nil +} + +func (v *PVNodeAffinityValidator) ValidateUpdate(ctx context.Context, oldVM, newVM *v1alpha2.VirtualMachine) (admission.Warnings, error) { + if reflect.DeepEqual(oldVM.Spec.BlockDeviceRefs, newVM.Spec.BlockDeviceRefs) { + return nil, nil + } + + if newVM.Status.Node == "" { + return nil, nil + } + + oldRefs := make(map[string]struct{}, len(oldVM.Spec.BlockDeviceRefs)) + for _, ref := range oldVM.Spec.BlockDeviceRefs { + key := string(ref.Kind) + "/" + ref.Name + oldRefs[key] = struct{}{} + } + + node := &corev1.Node{} + if err := v.client.Get(ctx, types.NamespacedName{Name: newVM.Status.Node}, node); err != nil { + return nil, fmt.Errorf("failed to get node %q: %w", newVM.Status.Node, err) + } + + for _, ref := range newVM.Spec.BlockDeviceRefs { + key := string(ref.Kind) + "/" + ref.Name + if _, existed := oldRefs[key]; existed { + continue + } + + available, err := v.isDiskAvailableOnNode(ctx, ref, newVM.Namespace, node) + if err != nil { + return nil, err + } + if !available { + return nil, fmt.Errorf( + "unable to attach disk %q to VM %q: the disk is not available on node %q where the VM is running", + ref.Name, newVM.Name, newVM.Status.Node, + ) + } + } + + return nil, nil +} + +func (v *PVNodeAffinityValidator) isDiskAvailableOnNode(ctx context.Context, ref v1alpha2.BlockDeviceSpecRef, namespace string, node *corev1.Node) (bool, error) { + var pvcName string + + switch ref.Kind { + case v1alpha2.DiskDevice: + vd, err := object.FetchObject(ctx, types.NamespacedName{Name: ref.Name, Namespace: namespace}, v.client, &v1alpha2.VirtualDisk{}) + if err != nil { + return false, fmt.Errorf("failed to get VirtualDisk %q: %w", ref.Name, err) + } + if vd == nil { + return true, nil + } + pvcName = vd.Status.Target.PersistentVolumeClaim + case v1alpha2.ImageDevice: + vi, err := object.FetchObject(ctx, types.NamespacedName{Name: ref.Name, Namespace: namespace}, v.client, &v1alpha2.VirtualImage{}) + if err != nil { + return false, fmt.Errorf("failed to get VirtualImage %q: %w", ref.Name, err) + } + if vi == nil { + return true, nil + } + if vi.Spec.Storage == v1alpha2.StorageContainerRegistry { + return true, nil + } + pvcName = vi.Status.Target.PersistentVolumeClaim + case v1alpha2.ClusterImageDevice: + return true, nil + default: + return true, nil + } + + if pvcName == "" { + return true, nil + } + + pvc, err := object.FetchObject(ctx, types.NamespacedName{Name: pvcName, Namespace: namespace}, v.client, &corev1.PersistentVolumeClaim{}) + if err != nil { + return false, fmt.Errorf("failed to get PVC %q: %w", pvcName, err) + } + if pvc == nil || pvc.Spec.VolumeName == "" { + return true, nil + } + + pv, err := object.FetchObject(ctx, types.NamespacedName{Name: pvc.Spec.VolumeName}, v.client, &corev1.PersistentVolume{}) + if err != nil { + return false, fmt.Errorf("failed to get PV %q: %w", pvc.Spec.VolumeName, err) + } + if pv == nil || pv.Spec.NodeAffinity == nil || pv.Spec.NodeAffinity.Required == nil { + return true, nil + } + + selector, err := nodeaffinity.NewNodeSelector(pv.Spec.NodeAffinity.Required) + if err != nil { + return false, fmt.Errorf("failed to parse PV node selector: %w", err) + } + + return selector.Match(node), nil +} diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/validators/pv_node_affinity_validator_test.go b/images/virtualization-artifact/pkg/controller/vm/internal/validators/pv_node_affinity_validator_test.go new file mode 100644 index 0000000000..0990a4b459 --- /dev/null +++ b/images/virtualization-artifact/pkg/controller/vm/internal/validators/pv_node_affinity_validator_test.go @@ -0,0 +1,214 @@ +/* +Copyright 2026 Flant JSC + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package validators_test + +import ( + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + + "github.com/deckhouse/virtualization-controller/pkg/common/testutil" + "github.com/deckhouse/virtualization-controller/pkg/controller/vm/internal/validators" + "github.com/deckhouse/virtualization/api/core/v1alpha2" +) + +var _ = Describe("PVNodeAffinityValidator", func() { + const ( + ns = "test-ns" + node1 = "node-1" + node2 = "node-2" + ) + + makeNode := func(name string) *corev1.Node { + return &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Labels: map[string]string{"topology.kubernetes.io/node": name}, + }, + } + } + + makePV := func(name string, nodeNames ...string) *corev1.PersistentVolume { + pv := &corev1.PersistentVolume{ObjectMeta: metav1.ObjectMeta{Name: name}} + if len(nodeNames) > 0 { + pv.Spec.NodeAffinity = &corev1.VolumeNodeAffinity{ + Required: &corev1.NodeSelector{ + NodeSelectorTerms: []corev1.NodeSelectorTerm{{ + MatchExpressions: []corev1.NodeSelectorRequirement{{ + Key: "topology.kubernetes.io/node", + Operator: corev1.NodeSelectorOpIn, + Values: nodeNames, + }}, + }}, + }, + } + } + return pv + } + + makePVC := func(name, pvName string) *corev1.PersistentVolumeClaim { + return &corev1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: ns}, + Spec: corev1.PersistentVolumeClaimSpec{VolumeName: pvName}, + } + } + + makeVD := func(name, pvcName string) *v1alpha2.VirtualDisk { + return &v1alpha2.VirtualDisk{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: ns}, + Status: v1alpha2.VirtualDiskStatus{Target: v1alpha2.DiskTarget{PersistentVolumeClaim: pvcName}}, + } + } + + makeVM := func(name string, nodeName string, refs ...v1alpha2.BlockDeviceSpecRef) *v1alpha2.VirtualMachine { + return &v1alpha2.VirtualMachine{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: ns}, + Spec: v1alpha2.VirtualMachineSpec{BlockDeviceRefs: refs}, + Status: v1alpha2.VirtualMachineStatus{Node: nodeName}, + } + } + + It("should allow when VM is not running (no node)", func() { + oldVM := makeVM("vm", "", v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "disk1"}) + newVM := makeVM("vm", "", + v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "disk1"}, + v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "disk2"}, + ) + fakeClient := setupEnvironment(oldVM) + v := validators.NewPVNodeAffinityValidator(fakeClient) + _, err := v.ValidateUpdate(testutil.ContextBackgroundWithNoOpLogger(), oldVM, newVM) + Expect(err).ShouldNot(HaveOccurred()) + }) + + It("should allow when blockDeviceRefs unchanged", func() { + refs := []v1alpha2.BlockDeviceSpecRef{{Kind: v1alpha2.DiskDevice, Name: "disk1"}} + oldVM := makeVM("vm", node1, refs...) + newVM := makeVM("vm", node1, refs...) + fakeClient := setupEnvironment(oldVM, makeNode(node1)) + v := validators.NewPVNodeAffinityValidator(fakeClient) + _, err := v.ValidateUpdate(testutil.ContextBackgroundWithNoOpLogger(), oldVM, newVM) + Expect(err).ShouldNot(HaveOccurred()) + }) + + It("should allow adding a network disk (PV without nodeAffinity)", func() { + oldVM := makeVM("vm", node1, v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "disk1"}) + newVM := makeVM("vm", node1, + v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "disk1"}, + v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "net-disk"}, + ) + objs := []client.Object{ + oldVM, makeNode(node1), + makeVD("net-disk", "pvc-net"), + makePVC("pvc-net", "pv-net"), + makePV("pv-net"), // no nodeAffinity + } + fakeClient := setupEnvironment(objs...) + v := validators.NewPVNodeAffinityValidator(fakeClient) + _, err := v.ValidateUpdate(testutil.ContextBackgroundWithNoOpLogger(), oldVM, newVM) + Expect(err).ShouldNot(HaveOccurred()) + }) + + It("should allow adding a local disk available on VM node", func() { + oldVM := makeVM("vm", node1, v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "disk1"}) + newVM := makeVM("vm", node1, + v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "disk1"}, + v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "local-disk"}, + ) + objs := []client.Object{ + oldVM, makeNode(node1), + makeVD("local-disk", "pvc-local"), + makePVC("pvc-local", "pv-local"), + makePV("pv-local", node1), + } + fakeClient := setupEnvironment(objs...) + v := validators.NewPVNodeAffinityValidator(fakeClient) + _, err := v.ValidateUpdate(testutil.ContextBackgroundWithNoOpLogger(), oldVM, newVM) + Expect(err).ShouldNot(HaveOccurred()) + }) + + It("should reject adding a local disk NOT available on VM node", func() { + oldVM := makeVM("vm", node1, v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "disk1"}) + newVM := makeVM("vm", node1, + v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "disk1"}, + v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "local-disk"}, + ) + objs := []client.Object{ + oldVM, makeNode(node1), + makeVD("local-disk", "pvc-local"), + makePVC("pvc-local", "pv-local"), + makePV("pv-local", node2), // only on node2, VM on node1 + } + fakeClient := setupEnvironment(objs...) + v := validators.NewPVNodeAffinityValidator(fakeClient) + _, err := v.ValidateUpdate(testutil.ContextBackgroundWithNoOpLogger(), oldVM, newVM) + Expect(err).Should(HaveOccurred()) + Expect(err.Error()).Should(ContainSubstring("unable to attach disk")) + Expect(err.Error()).Should(ContainSubstring("local-disk")) + }) + + It("should allow adding a disk with pending PVC (WFFC, no PV yet)", func() { + oldVM := makeVM("vm", node1) + newVM := makeVM("vm", node1, + v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "new-disk"}, + ) + pvcPending := &corev1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{Name: "pvc-pending", Namespace: ns}, + Spec: corev1.PersistentVolumeClaimSpec{}, + } + objs := []client.Object{ + oldVM, makeNode(node1), + makeVD("new-disk", "pvc-pending"), + pvcPending, + } + fakeClient := setupEnvironment(objs...) + v := validators.NewPVNodeAffinityValidator(fakeClient) + _, err := v.ValidateUpdate(testutil.ContextBackgroundWithNoOpLogger(), oldVM, newVM) + Expect(err).ShouldNot(HaveOccurred()) + }) + + It("should reject when any of multiple new disks is incompatible", func() { + oldVM := makeVM("vm", node1) + newVM := makeVM("vm", node1, + v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "good-disk"}, + v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "bad-disk"}, + ) + objs := []client.Object{ + oldVM, makeNode(node1), + makeVD("good-disk", "pvc-good"), + makePVC("pvc-good", "pv-good"), + makePV("pv-good", node1), + makeVD("bad-disk", "pvc-bad"), + makePVC("pvc-bad", "pv-bad"), + makePV("pv-bad", node2), + } + fakeClient := setupEnvironment(objs...) + v := validators.NewPVNodeAffinityValidator(fakeClient) + _, err := v.ValidateUpdate(testutil.ContextBackgroundWithNoOpLogger(), oldVM, newVM) + Expect(err).Should(HaveOccurred()) + Expect(err.Error()).Should(ContainSubstring("bad-disk")) + }) + + It("should allow on create (VM not running yet)", func() { + vm := makeVM("vm", "", v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "disk1"}) + fakeClient := setupEnvironment(vm) + v := validators.NewPVNodeAffinityValidator(fakeClient) + _, err := v.ValidateCreate(testutil.ContextBackgroundWithNoOpLogger(), vm) + Expect(err).ShouldNot(HaveOccurred()) + }) +}) diff --git a/images/virtualization-artifact/pkg/controller/vm/vm_webhook.go b/images/virtualization-artifact/pkg/controller/vm/vm_webhook.go index 6af60254e2..e9e44a867e 100644 --- a/images/virtualization-artifact/pkg/controller/vm/vm_webhook.go +++ b/images/virtualization-artifact/pkg/controller/vm/vm_webhook.go @@ -57,6 +57,7 @@ func NewValidator(client client.Client, service *service.BlockDeviceService, fea validators.NewFirstDiskValidator(client), validators.NewUSBDevicesValidator(client, featureGate), validators.NewVMBDAConflictValidator(client), + validators.NewPVNodeAffinityValidator(client), }, log: log.With("webhook", "validation"), } diff --git a/images/virtualization-artifact/pkg/controller/vmbda/internal/validators/pv_node_affinity_validator.go b/images/virtualization-artifact/pkg/controller/vmbda/internal/validators/pv_node_affinity_validator.go new file mode 100644 index 0000000000..773e430423 --- /dev/null +++ b/images/virtualization-artifact/pkg/controller/vmbda/internal/validators/pv_node_affinity_validator.go @@ -0,0 +1,123 @@ +/* +Copyright 2026 Flant JSC + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package validators + +import ( + "context" + "fmt" + + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + "github.com/deckhouse/virtualization-controller/pkg/controller/service" + "github.com/deckhouse/virtualization/api/core/v1alpha2" +) + +type PVNodeAffinityValidator struct { + attacher *service.AttachmentService +} + +func NewPVNodeAffinityValidator(attacher *service.AttachmentService) *PVNodeAffinityValidator { + return &PVNodeAffinityValidator{attacher: attacher} +} + +func (v *PVNodeAffinityValidator) ValidateCreate(ctx context.Context, vmbda *v1alpha2.VirtualMachineBlockDeviceAttachment) (admission.Warnings, error) { + vm, err := v.attacher.GetVirtualMachine(ctx, vmbda.Spec.VirtualMachineName, vmbda.Namespace) + if err != nil { + return nil, fmt.Errorf("failed to get VirtualMachine %q: %w", vmbda.Spec.VirtualMachineName, err) + } + if vm == nil || vm.Status.Node == "" { + return nil, nil + } + + kvvmi, err := v.attacher.GetKVVMI(ctx, vm) + if err != nil { + return nil, fmt.Errorf("failed to get KVVMI for VM %q: %w", vm.Name, err) + } + if kvvmi == nil { + return nil, nil + } + + ad, err := v.resolveAttachmentDisk(ctx, vmbda) + if err != nil { + return nil, err + } + if ad == nil || ad.PVCName == "" { + return nil, nil + } + + pvc, err := v.attacher.GetPersistentVolumeClaim(ctx, ad) + if err != nil { + return nil, fmt.Errorf("failed to get PVC %q: %w", ad.PVCName, err) + } + if pvc == nil { + return nil, nil + } + + available, err := v.attacher.IsPVAvailableOnVMNode(ctx, pvc, kvvmi) + if err != nil { + return nil, fmt.Errorf("failed to check PV availability: %w", err) + } + + if !available { + return nil, fmt.Errorf( + "unable to attach disk %q to VM %q: the disk is not available on node %q where the VM is running", + vmbda.Spec.BlockDeviceRef.Name, vmbda.Spec.VirtualMachineName, vm.Status.Node, + ) + } + + return nil, nil +} + +func (v *PVNodeAffinityValidator) ValidateUpdate(_ context.Context, _, _ *v1alpha2.VirtualMachineBlockDeviceAttachment) (admission.Warnings, error) { + return nil, nil +} + +func (v *PVNodeAffinityValidator) resolveAttachmentDisk(ctx context.Context, vmbda *v1alpha2.VirtualMachineBlockDeviceAttachment) (*service.AttachmentDisk, error) { + ref := vmbda.Spec.BlockDeviceRef + + switch ref.Kind { + case v1alpha2.VMBDAObjectRefKindVirtualDisk: + vd, err := v.attacher.GetVirtualDisk(ctx, ref.Name, vmbda.Namespace) + if err != nil { + return nil, fmt.Errorf("failed to get VirtualDisk %q: %w", ref.Name, err) + } + if vd == nil { + return nil, nil + } + return service.NewAttachmentDiskFromVirtualDisk(vd), nil + case v1alpha2.VMBDAObjectRefKindVirtualImage: + vi, err := v.attacher.GetVirtualImage(ctx, ref.Name, vmbda.Namespace) + if err != nil { + return nil, fmt.Errorf("failed to get VirtualImage %q: %w", ref.Name, err) + } + if vi == nil { + return nil, nil + } + return service.NewAttachmentDiskFromVirtualImage(vi), nil + case v1alpha2.VMBDAObjectRefKindClusterVirtualImage: + cvi, err := v.attacher.GetClusterVirtualImage(ctx, ref.Name) + if err != nil { + return nil, fmt.Errorf("failed to get ClusterVirtualImage %q: %w", ref.Name, err) + } + if cvi == nil { + return nil, nil + } + return service.NewAttachmentDiskFromClusterVirtualImage(cvi), nil + default: + return nil, nil + } +} diff --git a/images/virtualization-artifact/pkg/controller/vmbda/vmbda_webhook.go b/images/virtualization-artifact/pkg/controller/vmbda/vmbda_webhook.go index b355b03710..1c3c976c63 100644 --- a/images/virtualization-artifact/pkg/controller/vmbda/vmbda_webhook.go +++ b/images/virtualization-artifact/pkg/controller/vmbda/vmbda_webhook.go @@ -46,6 +46,7 @@ func NewValidator(attachmentService *service.AttachmentService, service *service validators.NewSpecMutateValidator(), validators.NewAttachmentConflictValidator(attachmentService, log), validators.NewVMConnectLimiterValidator(service, log), + validators.NewPVNodeAffinityValidator(attachmentService), }, } } From afa75012db3f5ba170c06dbd5c4b8da908ef1b9e Mon Sep 17 00:00:00 2001 From: Daniil Loktev Date: Tue, 24 Mar 2026 11:07:59 +0300 Subject: [PATCH 2/6] wip Signed-off-by: Daniil Loktev --- .../validators/pv_node_affinity_validator.go | 104 ++++++++---------- .../pv_node_affinity_validator_test.go | 68 ++++++------ .../pkg/controller/vm/vm_controller.go | 2 +- .../pkg/controller/vm/vm_webhook.go | 6 +- 4 files changed, 86 insertions(+), 94 deletions(-) diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/validators/pv_node_affinity_validator.go b/images/virtualization-artifact/pkg/controller/vm/internal/validators/pv_node_affinity_validator.go index d8424411b2..1391634c9a 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/validators/pv_node_affinity_validator.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/validators/pv_node_affinity_validator.go @@ -21,22 +21,18 @@ import ( "fmt" "reflect" - corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/types" - "k8s.io/component-helpers/scheduling/corev1/nodeaffinity" - "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" - "github.com/deckhouse/virtualization-controller/pkg/common/object" + "github.com/deckhouse/virtualization-controller/pkg/controller/service" "github.com/deckhouse/virtualization/api/core/v1alpha2" ) type PVNodeAffinityValidator struct { - client client.Client + attacher *service.AttachmentService } -func NewPVNodeAffinityValidator(client client.Client) *PVNodeAffinityValidator { - return &PVNodeAffinityValidator{client: client} +func NewPVNodeAffinityValidator(attacher *service.AttachmentService) *PVNodeAffinityValidator { + return &PVNodeAffinityValidator{attacher: attacher} } func (v *PVNodeAffinityValidator) ValidateCreate(_ context.Context, _ *v1alpha2.VirtualMachine) (admission.Warnings, error) { @@ -52,27 +48,46 @@ func (v *PVNodeAffinityValidator) ValidateUpdate(ctx context.Context, oldVM, new return nil, nil } + kvvmi, err := v.attacher.GetKVVMI(ctx, newVM) + if err != nil { + return nil, fmt.Errorf("failed to get KVVMI for VM %q: %w", newVM.Name, err) + } + if kvvmi == nil { + return nil, nil + } + oldRefs := make(map[string]struct{}, len(oldVM.Spec.BlockDeviceRefs)) for _, ref := range oldVM.Spec.BlockDeviceRefs { key := string(ref.Kind) + "/" + ref.Name oldRefs[key] = struct{}{} } - node := &corev1.Node{} - if err := v.client.Get(ctx, types.NamespacedName{Name: newVM.Status.Node}, node); err != nil { - return nil, fmt.Errorf("failed to get node %q: %w", newVM.Status.Node, err) - } - for _, ref := range newVM.Spec.BlockDeviceRefs { key := string(ref.Kind) + "/" + ref.Name if _, existed := oldRefs[key]; existed { continue } - available, err := v.isDiskAvailableOnNode(ctx, ref, newVM.Namespace, node) + ad, err := v.resolveAttachmentDisk(ctx, ref, newVM.Namespace) if err != nil { return nil, err } + if ad == nil || ad.PVCName == "" { + continue + } + + pvc, err := v.attacher.GetPersistentVolumeClaim(ctx, ad) + if err != nil { + return nil, fmt.Errorf("failed to get PVC %q: %w", ad.PVCName, err) + } + if pvc == nil { + continue + } + + available, err := v.attacher.IsPVAvailableOnVMNode(ctx, pvc, kvvmi) + if err != nil { + return nil, fmt.Errorf("failed to check PV availability: %w", err) + } if !available { return nil, fmt.Errorf( "unable to attach disk %q to VM %q: the disk is not available on node %q where the VM is running", @@ -84,61 +99,36 @@ func (v *PVNodeAffinityValidator) ValidateUpdate(ctx context.Context, oldVM, new return nil, nil } -func (v *PVNodeAffinityValidator) isDiskAvailableOnNode(ctx context.Context, ref v1alpha2.BlockDeviceSpecRef, namespace string, node *corev1.Node) (bool, error) { - var pvcName string - +func (v *PVNodeAffinityValidator) resolveAttachmentDisk(ctx context.Context, ref v1alpha2.BlockDeviceSpecRef, namespace string) (*service.AttachmentDisk, error) { switch ref.Kind { case v1alpha2.DiskDevice: - vd, err := object.FetchObject(ctx, types.NamespacedName{Name: ref.Name, Namespace: namespace}, v.client, &v1alpha2.VirtualDisk{}) + vd, err := v.attacher.GetVirtualDisk(ctx, ref.Name, namespace) if err != nil { - return false, fmt.Errorf("failed to get VirtualDisk %q: %w", ref.Name, err) + return nil, fmt.Errorf("failed to get VirtualDisk %q: %w", ref.Name, err) } if vd == nil { - return true, nil + return nil, nil } - pvcName = vd.Status.Target.PersistentVolumeClaim + return service.NewAttachmentDiskFromVirtualDisk(vd), nil case v1alpha2.ImageDevice: - vi, err := object.FetchObject(ctx, types.NamespacedName{Name: ref.Name, Namespace: namespace}, v.client, &v1alpha2.VirtualImage{}) + vi, err := v.attacher.GetVirtualImage(ctx, ref.Name, namespace) if err != nil { - return false, fmt.Errorf("failed to get VirtualImage %q: %w", ref.Name, err) + return nil, fmt.Errorf("failed to get VirtualImage %q: %w", ref.Name, err) } if vi == nil { - return true, nil - } - if vi.Spec.Storage == v1alpha2.StorageContainerRegistry { - return true, nil + return nil, nil } - pvcName = vi.Status.Target.PersistentVolumeClaim + return service.NewAttachmentDiskFromVirtualImage(vi), nil case v1alpha2.ClusterImageDevice: - return true, nil + cvi, err := v.attacher.GetClusterVirtualImage(ctx, ref.Name) + if err != nil { + return nil, fmt.Errorf("failed to get ClusterVirtualImage %q: %w", ref.Name, err) + } + if cvi == nil { + return nil, nil + } + return service.NewAttachmentDiskFromClusterVirtualImage(cvi), nil default: - return true, nil - } - - if pvcName == "" { - return true, nil - } - - pvc, err := object.FetchObject(ctx, types.NamespacedName{Name: pvcName, Namespace: namespace}, v.client, &corev1.PersistentVolumeClaim{}) - if err != nil { - return false, fmt.Errorf("failed to get PVC %q: %w", pvcName, err) - } - if pvc == nil || pvc.Spec.VolumeName == "" { - return true, nil - } - - pv, err := object.FetchObject(ctx, types.NamespacedName{Name: pvc.Spec.VolumeName}, v.client, &corev1.PersistentVolume{}) - if err != nil { - return false, fmt.Errorf("failed to get PV %q: %w", pvc.Spec.VolumeName, err) - } - if pv == nil || pv.Spec.NodeAffinity == nil || pv.Spec.NodeAffinity.Required == nil { - return true, nil - } - - selector, err := nodeaffinity.NewNodeSelector(pv.Spec.NodeAffinity.Required) - if err != nil { - return false, fmt.Errorf("failed to parse PV node selector: %w", err) + return nil, nil } - - return selector.Match(node), nil } diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/validators/pv_node_affinity_validator_test.go b/images/virtualization-artifact/pkg/controller/vm/internal/validators/pv_node_affinity_validator_test.go index 0990a4b459..daa123590f 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/validators/pv_node_affinity_validator_test.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/validators/pv_node_affinity_validator_test.go @@ -21,9 +21,11 @@ import ( . "github.com/onsi/gomega" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + virtv1 "kubevirt.io/api/core/v1" "sigs.k8s.io/controller-runtime/pkg/client" "github.com/deckhouse/virtualization-controller/pkg/common/testutil" + "github.com/deckhouse/virtualization-controller/pkg/controller/service" "github.com/deckhouse/virtualization-controller/pkg/controller/vm/internal/validators" "github.com/deckhouse/virtualization/api/core/v1alpha2" ) @@ -84,14 +86,26 @@ var _ = Describe("PVNodeAffinityValidator", func() { } } + makeKVVMI := func(name, nodeName string) *virtv1.VirtualMachineInstance { + return &virtv1.VirtualMachineInstance{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: ns}, + Status: virtv1.VirtualMachineInstanceStatus{NodeName: nodeName}, + } + } + + makeValidator := func(objs ...client.Object) *validators.PVNodeAffinityValidator { + fakeClient := setupEnvironment(objs...) + attacher := service.NewAttachmentService(fakeClient, nil, "") + return validators.NewPVNodeAffinityValidator(attacher) + } + It("should allow when VM is not running (no node)", func() { oldVM := makeVM("vm", "", v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "disk1"}) newVM := makeVM("vm", "", v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "disk1"}, v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "disk2"}, ) - fakeClient := setupEnvironment(oldVM) - v := validators.NewPVNodeAffinityValidator(fakeClient) + v := makeValidator(oldVM) _, err := v.ValidateUpdate(testutil.ContextBackgroundWithNoOpLogger(), oldVM, newVM) Expect(err).ShouldNot(HaveOccurred()) }) @@ -100,8 +114,7 @@ var _ = Describe("PVNodeAffinityValidator", func() { refs := []v1alpha2.BlockDeviceSpecRef{{Kind: v1alpha2.DiskDevice, Name: "disk1"}} oldVM := makeVM("vm", node1, refs...) newVM := makeVM("vm", node1, refs...) - fakeClient := setupEnvironment(oldVM, makeNode(node1)) - v := validators.NewPVNodeAffinityValidator(fakeClient) + v := makeValidator(oldVM, makeNode(node1)) _, err := v.ValidateUpdate(testutil.ContextBackgroundWithNoOpLogger(), oldVM, newVM) Expect(err).ShouldNot(HaveOccurred()) }) @@ -112,14 +125,12 @@ var _ = Describe("PVNodeAffinityValidator", func() { v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "disk1"}, v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "net-disk"}, ) - objs := []client.Object{ - oldVM, makeNode(node1), + v := makeValidator( + oldVM, makeNode(node1), makeKVVMI("vm", node1), makeVD("net-disk", "pvc-net"), makePVC("pvc-net", "pv-net"), - makePV("pv-net"), // no nodeAffinity - } - fakeClient := setupEnvironment(objs...) - v := validators.NewPVNodeAffinityValidator(fakeClient) + makePV("pv-net"), + ) _, err := v.ValidateUpdate(testutil.ContextBackgroundWithNoOpLogger(), oldVM, newVM) Expect(err).ShouldNot(HaveOccurred()) }) @@ -130,14 +141,12 @@ var _ = Describe("PVNodeAffinityValidator", func() { v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "disk1"}, v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "local-disk"}, ) - objs := []client.Object{ - oldVM, makeNode(node1), + v := makeValidator( + oldVM, makeNode(node1), makeKVVMI("vm", node1), makeVD("local-disk", "pvc-local"), makePVC("pvc-local", "pv-local"), makePV("pv-local", node1), - } - fakeClient := setupEnvironment(objs...) - v := validators.NewPVNodeAffinityValidator(fakeClient) + ) _, err := v.ValidateUpdate(testutil.ContextBackgroundWithNoOpLogger(), oldVM, newVM) Expect(err).ShouldNot(HaveOccurred()) }) @@ -148,14 +157,12 @@ var _ = Describe("PVNodeAffinityValidator", func() { v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "disk1"}, v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "local-disk"}, ) - objs := []client.Object{ - oldVM, makeNode(node1), + v := makeValidator( + oldVM, makeNode(node1), makeKVVMI("vm", node1), makeVD("local-disk", "pvc-local"), makePVC("pvc-local", "pv-local"), - makePV("pv-local", node2), // only on node2, VM on node1 - } - fakeClient := setupEnvironment(objs...) - v := validators.NewPVNodeAffinityValidator(fakeClient) + makePV("pv-local", node2), + ) _, err := v.ValidateUpdate(testutil.ContextBackgroundWithNoOpLogger(), oldVM, newVM) Expect(err).Should(HaveOccurred()) Expect(err.Error()).Should(ContainSubstring("unable to attach disk")) @@ -171,13 +178,11 @@ var _ = Describe("PVNodeAffinityValidator", func() { ObjectMeta: metav1.ObjectMeta{Name: "pvc-pending", Namespace: ns}, Spec: corev1.PersistentVolumeClaimSpec{}, } - objs := []client.Object{ - oldVM, makeNode(node1), + v := makeValidator( + oldVM, makeNode(node1), makeKVVMI("vm", node1), makeVD("new-disk", "pvc-pending"), pvcPending, - } - fakeClient := setupEnvironment(objs...) - v := validators.NewPVNodeAffinityValidator(fakeClient) + ) _, err := v.ValidateUpdate(testutil.ContextBackgroundWithNoOpLogger(), oldVM, newVM) Expect(err).ShouldNot(HaveOccurred()) }) @@ -188,17 +193,15 @@ var _ = Describe("PVNodeAffinityValidator", func() { v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "good-disk"}, v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "bad-disk"}, ) - objs := []client.Object{ - oldVM, makeNode(node1), + v := makeValidator( + oldVM, makeNode(node1), makeKVVMI("vm", node1), makeVD("good-disk", "pvc-good"), makePVC("pvc-good", "pv-good"), makePV("pv-good", node1), makeVD("bad-disk", "pvc-bad"), makePVC("pvc-bad", "pv-bad"), makePV("pv-bad", node2), - } - fakeClient := setupEnvironment(objs...) - v := validators.NewPVNodeAffinityValidator(fakeClient) + ) _, err := v.ValidateUpdate(testutil.ContextBackgroundWithNoOpLogger(), oldVM, newVM) Expect(err).Should(HaveOccurred()) Expect(err.Error()).Should(ContainSubstring("bad-disk")) @@ -206,8 +209,7 @@ var _ = Describe("PVNodeAffinityValidator", func() { It("should allow on create (VM not running yet)", func() { vm := makeVM("vm", "", v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "disk1"}) - fakeClient := setupEnvironment(vm) - v := validators.NewPVNodeAffinityValidator(fakeClient) + v := makeValidator(vm) _, err := v.ValidateCreate(testutil.ContextBackgroundWithNoOpLogger(), vm) Expect(err).ShouldNot(HaveOccurred()) }) diff --git a/images/virtualization-artifact/pkg/controller/vm/vm_controller.go b/images/virtualization-artifact/pkg/controller/vm/vm_controller.go index 348983b209..a98748f9df 100644 --- a/images/virtualization-artifact/pkg/controller/vm/vm_controller.go +++ b/images/virtualization-artifact/pkg/controller/vm/vm_controller.go @@ -108,7 +108,7 @@ func SetupController( if err = builder.WebhookManagedBy(mgr). For(&v1alpha2.VirtualMachine{}). - WithValidator(NewValidator(client, blockDeviceService, featuregates.Default(), log)). + WithValidator(NewValidator(client, blockDeviceService, attachmentService, featuregates.Default(), log)). WithDefaulter(NewDefaulter(client, vmClassService, log)). Complete(); err != nil { return err diff --git a/images/virtualization-artifact/pkg/controller/vm/vm_webhook.go b/images/virtualization-artifact/pkg/controller/vm/vm_webhook.go index e9e44a867e..5ec2bc9283 100644 --- a/images/virtualization-artifact/pkg/controller/vm/vm_webhook.go +++ b/images/virtualization-artifact/pkg/controller/vm/vm_webhook.go @@ -42,14 +42,14 @@ type Validator struct { log *log.Logger } -func NewValidator(client client.Client, service *service.BlockDeviceService, featureGate featuregate.FeatureGate, log *log.Logger) *Validator { +func NewValidator(client client.Client, blockDeviceService *service.BlockDeviceService, attachmentService *service.AttachmentService, featureGate featuregate.FeatureGate, log *log.Logger) *Validator { return &Validator{ validators: []VirtualMachineValidator{ validators.NewMetaValidator(client), validators.NewIPAMValidator(client), validators.NewBlockDeviceSpecRefsValidator(), validators.NewSizingPolicyValidator(client), - validators.NewBlockDeviceLimiterValidator(service, log), + validators.NewBlockDeviceLimiterValidator(blockDeviceService, log), validators.NewAffinityValidator(), validators.NewTopologySpreadConstraintValidator(), validators.NewCPUCountValidator(), @@ -57,7 +57,7 @@ func NewValidator(client client.Client, service *service.BlockDeviceService, fea validators.NewFirstDiskValidator(client), validators.NewUSBDevicesValidator(client, featureGate), validators.NewVMBDAConflictValidator(client), - validators.NewPVNodeAffinityValidator(client), + validators.NewPVNodeAffinityValidator(attachmentService), }, log: log.With("webhook", "validation"), } From d821f2d19c4cf92e202740e758359a2284db20c3 Mon Sep 17 00:00:00 2001 From: Daniil Loktev Date: Tue, 24 Mar 2026 11:20:16 +0300 Subject: [PATCH 3/6] fix linter errors Signed-off-by: Daniil Loktev --- .../pv_node_affinity_validator_test.go | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/validators/pv_node_affinity_validator_test.go b/images/virtualization-artifact/pkg/controller/vm/internal/validators/pv_node_affinity_validator_test.go index daa123590f..fb77cb5b57 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/validators/pv_node_affinity_validator_test.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/validators/pv_node_affinity_validator_test.go @@ -37,11 +37,11 @@ var _ = Describe("PVNodeAffinityValidator", func() { node2 = "node-2" ) - makeNode := func(name string) *corev1.Node { + makeNode := func() *corev1.Node { return &corev1.Node{ ObjectMeta: metav1.ObjectMeta{ - Name: name, - Labels: map[string]string{"topology.kubernetes.io/node": name}, + Name: node1, + Labels: map[string]string{"topology.kubernetes.io/node": node1}, }, } } @@ -78,7 +78,7 @@ var _ = Describe("PVNodeAffinityValidator", func() { } } - makeVM := func(name string, nodeName string, refs ...v1alpha2.BlockDeviceSpecRef) *v1alpha2.VirtualMachine { + makeVM := func(name, nodeName string, refs ...v1alpha2.BlockDeviceSpecRef) *v1alpha2.VirtualMachine { return &v1alpha2.VirtualMachine{ ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: ns}, Spec: v1alpha2.VirtualMachineSpec{BlockDeviceRefs: refs}, @@ -86,9 +86,9 @@ var _ = Describe("PVNodeAffinityValidator", func() { } } - makeKVVMI := func(name, nodeName string) *virtv1.VirtualMachineInstance { + makeKVVMI := func(nodeName string) *virtv1.VirtualMachineInstance { return &virtv1.VirtualMachineInstance{ - ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: ns}, + ObjectMeta: metav1.ObjectMeta{Name: "vm", Namespace: ns}, Status: virtv1.VirtualMachineInstanceStatus{NodeName: nodeName}, } } @@ -114,7 +114,7 @@ var _ = Describe("PVNodeAffinityValidator", func() { refs := []v1alpha2.BlockDeviceSpecRef{{Kind: v1alpha2.DiskDevice, Name: "disk1"}} oldVM := makeVM("vm", node1, refs...) newVM := makeVM("vm", node1, refs...) - v := makeValidator(oldVM, makeNode(node1)) + v := makeValidator(oldVM, makeNode()) _, err := v.ValidateUpdate(testutil.ContextBackgroundWithNoOpLogger(), oldVM, newVM) Expect(err).ShouldNot(HaveOccurred()) }) @@ -126,7 +126,7 @@ var _ = Describe("PVNodeAffinityValidator", func() { v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "net-disk"}, ) v := makeValidator( - oldVM, makeNode(node1), makeKVVMI("vm", node1), + oldVM, makeNode(), makeKVVMI(node1), makeVD("net-disk", "pvc-net"), makePVC("pvc-net", "pv-net"), makePV("pv-net"), @@ -142,7 +142,7 @@ var _ = Describe("PVNodeAffinityValidator", func() { v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "local-disk"}, ) v := makeValidator( - oldVM, makeNode(node1), makeKVVMI("vm", node1), + oldVM, makeNode(), makeKVVMI(node1), makeVD("local-disk", "pvc-local"), makePVC("pvc-local", "pv-local"), makePV("pv-local", node1), @@ -158,7 +158,7 @@ var _ = Describe("PVNodeAffinityValidator", func() { v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "local-disk"}, ) v := makeValidator( - oldVM, makeNode(node1), makeKVVMI("vm", node1), + oldVM, makeNode(), makeKVVMI(node1), makeVD("local-disk", "pvc-local"), makePVC("pvc-local", "pv-local"), makePV("pv-local", node2), @@ -179,7 +179,7 @@ var _ = Describe("PVNodeAffinityValidator", func() { Spec: corev1.PersistentVolumeClaimSpec{}, } v := makeValidator( - oldVM, makeNode(node1), makeKVVMI("vm", node1), + oldVM, makeNode(), makeKVVMI(node1), makeVD("new-disk", "pvc-pending"), pvcPending, ) @@ -194,7 +194,7 @@ var _ = Describe("PVNodeAffinityValidator", func() { v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "bad-disk"}, ) v := makeValidator( - oldVM, makeNode(node1), makeKVVMI("vm", node1), + oldVM, makeNode(), makeKVVMI(node1), makeVD("good-disk", "pvc-good"), makePVC("pvc-good", "pv-good"), makePV("pv-good", node1), From 9fb758bd66da77df501c78b06ecbea848e40574b Mon Sep 17 00:00:00 2001 From: Daniil Loktev Date: Tue, 24 Mar 2026 11:45:00 +0300 Subject: [PATCH 4/6] fix linter errors Signed-off-by: Daniil Loktev --- .../pv_node_affinity_validator_test.go | 48 +++++++++---------- 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/validators/pv_node_affinity_validator_test.go b/images/virtualization-artifact/pkg/controller/vm/internal/validators/pv_node_affinity_validator_test.go index fb77cb5b57..085628db60 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/validators/pv_node_affinity_validator_test.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/validators/pv_node_affinity_validator_test.go @@ -78,18 +78,18 @@ var _ = Describe("PVNodeAffinityValidator", func() { } } - makeVM := func(name, nodeName string, refs ...v1alpha2.BlockDeviceSpecRef) *v1alpha2.VirtualMachine { + makeVM := func(nodeName string, refs ...v1alpha2.BlockDeviceSpecRef) *v1alpha2.VirtualMachine { return &v1alpha2.VirtualMachine{ - ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: ns}, + ObjectMeta: metav1.ObjectMeta{Name: "vm", Namespace: ns}, Spec: v1alpha2.VirtualMachineSpec{BlockDeviceRefs: refs}, Status: v1alpha2.VirtualMachineStatus{Node: nodeName}, } } - makeKVVMI := func(nodeName string) *virtv1.VirtualMachineInstance { + makeKVVMI := func() *virtv1.VirtualMachineInstance { return &virtv1.VirtualMachineInstance{ ObjectMeta: metav1.ObjectMeta{Name: "vm", Namespace: ns}, - Status: virtv1.VirtualMachineInstanceStatus{NodeName: nodeName}, + Status: virtv1.VirtualMachineInstanceStatus{NodeName: node1}, } } @@ -100,8 +100,8 @@ var _ = Describe("PVNodeAffinityValidator", func() { } It("should allow when VM is not running (no node)", func() { - oldVM := makeVM("vm", "", v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "disk1"}) - newVM := makeVM("vm", "", + oldVM := makeVM("", v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "disk1"}) + newVM := makeVM("", v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "disk1"}, v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "disk2"}, ) @@ -112,21 +112,21 @@ var _ = Describe("PVNodeAffinityValidator", func() { It("should allow when blockDeviceRefs unchanged", func() { refs := []v1alpha2.BlockDeviceSpecRef{{Kind: v1alpha2.DiskDevice, Name: "disk1"}} - oldVM := makeVM("vm", node1, refs...) - newVM := makeVM("vm", node1, refs...) + oldVM := makeVM(node1, refs...) + newVM := makeVM(node1, refs...) v := makeValidator(oldVM, makeNode()) _, err := v.ValidateUpdate(testutil.ContextBackgroundWithNoOpLogger(), oldVM, newVM) Expect(err).ShouldNot(HaveOccurred()) }) It("should allow adding a network disk (PV without nodeAffinity)", func() { - oldVM := makeVM("vm", node1, v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "disk1"}) - newVM := makeVM("vm", node1, + oldVM := makeVM(node1, v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "disk1"}) + newVM := makeVM(node1, v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "disk1"}, v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "net-disk"}, ) v := makeValidator( - oldVM, makeNode(), makeKVVMI(node1), + oldVM, makeNode(), makeKVVMI(), makeVD("net-disk", "pvc-net"), makePVC("pvc-net", "pv-net"), makePV("pv-net"), @@ -136,13 +136,13 @@ var _ = Describe("PVNodeAffinityValidator", func() { }) It("should allow adding a local disk available on VM node", func() { - oldVM := makeVM("vm", node1, v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "disk1"}) - newVM := makeVM("vm", node1, + oldVM := makeVM(node1, v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "disk1"}) + newVM := makeVM(node1, v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "disk1"}, v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "local-disk"}, ) v := makeValidator( - oldVM, makeNode(), makeKVVMI(node1), + oldVM, makeNode(), makeKVVMI(), makeVD("local-disk", "pvc-local"), makePVC("pvc-local", "pv-local"), makePV("pv-local", node1), @@ -152,13 +152,13 @@ var _ = Describe("PVNodeAffinityValidator", func() { }) It("should reject adding a local disk NOT available on VM node", func() { - oldVM := makeVM("vm", node1, v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "disk1"}) - newVM := makeVM("vm", node1, + oldVM := makeVM(node1, v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "disk1"}) + newVM := makeVM(node1, v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "disk1"}, v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "local-disk"}, ) v := makeValidator( - oldVM, makeNode(), makeKVVMI(node1), + oldVM, makeNode(), makeKVVMI(), makeVD("local-disk", "pvc-local"), makePVC("pvc-local", "pv-local"), makePV("pv-local", node2), @@ -170,8 +170,8 @@ var _ = Describe("PVNodeAffinityValidator", func() { }) It("should allow adding a disk with pending PVC (WFFC, no PV yet)", func() { - oldVM := makeVM("vm", node1) - newVM := makeVM("vm", node1, + oldVM := makeVM(node1) + newVM := makeVM(node1, v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "new-disk"}, ) pvcPending := &corev1.PersistentVolumeClaim{ @@ -179,7 +179,7 @@ var _ = Describe("PVNodeAffinityValidator", func() { Spec: corev1.PersistentVolumeClaimSpec{}, } v := makeValidator( - oldVM, makeNode(), makeKVVMI(node1), + oldVM, makeNode(), makeKVVMI(), makeVD("new-disk", "pvc-pending"), pvcPending, ) @@ -188,13 +188,13 @@ var _ = Describe("PVNodeAffinityValidator", func() { }) It("should reject when any of multiple new disks is incompatible", func() { - oldVM := makeVM("vm", node1) - newVM := makeVM("vm", node1, + oldVM := makeVM(node1) + newVM := makeVM(node1, v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "good-disk"}, v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "bad-disk"}, ) v := makeValidator( - oldVM, makeNode(), makeKVVMI(node1), + oldVM, makeNode(), makeKVVMI(), makeVD("good-disk", "pvc-good"), makePVC("pvc-good", "pv-good"), makePV("pv-good", node1), @@ -208,7 +208,7 @@ var _ = Describe("PVNodeAffinityValidator", func() { }) It("should allow on create (VM not running yet)", func() { - vm := makeVM("vm", "", v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "disk1"}) + vm := makeVM("", v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "disk1"}) v := makeValidator(vm) _, err := v.ValidateCreate(testutil.ContextBackgroundWithNoOpLogger(), vm) Expect(err).ShouldNot(HaveOccurred()) From 5546e58e28cd1457474944b960408c86a5059854 Mon Sep 17 00:00:00 2001 From: Daniil Loktev Date: Wed, 25 Mar 2026 15:36:14 +0300 Subject: [PATCH 5/6] wip Signed-off-by: Daniil Loktev --- .../pkg/common/nodeaffinity/nodeaffinity.go | 65 +++- .../validators/pv_node_affinity_validator.go | 121 ++++++- .../pv_node_affinity_validator_test.go | 300 +++++++++++------- .../pkg/controller/vm/vm_webhook.go | 2 +- .../validators/pv_node_affinity_validator.go | 81 ++++- .../pkg/controller/vmbda/vmbda_controller.go | 2 +- .../pkg/controller/vmbda/vmbda_webhook.go | 5 +- 7 files changed, 439 insertions(+), 137 deletions(-) diff --git a/images/virtualization-artifact/pkg/common/nodeaffinity/nodeaffinity.go b/images/virtualization-artifact/pkg/common/nodeaffinity/nodeaffinity.go index 23d4d23372..7a123e65ed 100644 --- a/images/virtualization-artifact/pkg/common/nodeaffinity/nodeaffinity.go +++ b/images/virtualization-artifact/pkg/common/nodeaffinity/nodeaffinity.go @@ -16,7 +16,13 @@ limitations under the License. package nodeaffinity -import corev1 "k8s.io/api/core/v1" +import ( + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/labels" + corev1helpers "k8s.io/component-helpers/scheduling/corev1" + + "github.com/deckhouse/virtualization/api/core/v1alpha2" +) func IntersectTerms(perPVTerms [][]corev1.NodeSelectorTerm) []corev1.NodeSelectorTerm { if len(perPVTerms) == 0 { @@ -29,6 +35,63 @@ func IntersectTerms(perPVTerms [][]corev1.NodeSelectorTerm) []corev1.NodeSelecto return result } +func MatchesVMPlacement(node *corev1.Node, vm *v1alpha2.VirtualMachine, vmClass *v1alpha2.VirtualMachineClass) bool { + return matchesNodeSelector(node, vm.Spec.NodeSelector) && + matchesVMAffinity(node, vm.Spec.Affinity) && + matchesVMClassNodeSelector(node, vmClass) && + toleratesNodeTaints(node, vm.Spec.Tolerations) +} + +func matchesNodeSelector(node *corev1.Node, nodeSelector map[string]string) bool { + if len(nodeSelector) == 0 { + return true + } + return labels.SelectorFromSet(nodeSelector).Matches(labels.Set(node.Labels)) +} + +func matchesVMAffinity(node *corev1.Node, affinity *v1alpha2.VMAffinity) bool { + if affinity == nil || affinity.NodeAffinity == nil || + affinity.NodeAffinity.RequiredDuringSchedulingIgnoredDuringExecution == nil { + return true + } + match, err := corev1helpers.MatchNodeSelectorTerms(node, affinity.NodeAffinity.RequiredDuringSchedulingIgnoredDuringExecution) + if err != nil { + return true + } + return match +} + +func matchesVMClassNodeSelector(node *corev1.Node, vmClass *v1alpha2.VirtualMachineClass) bool { + nodeSelector := vmClass.Spec.NodeSelector + if len(nodeSelector.MatchLabels) > 0 { + if !labels.SelectorFromSet(nodeSelector.MatchLabels).Matches(labels.Set(node.Labels)) { + return false + } + } + if len(nodeSelector.MatchExpressions) > 0 { + match, err := corev1helpers.MatchNodeSelectorTerms(node, &corev1.NodeSelector{ + NodeSelectorTerms: []corev1.NodeSelectorTerm{{ + MatchExpressions: nodeSelector.MatchExpressions, + }}, + }) + if err != nil { + return true + } + return match + } + return true +} + +func toleratesNodeTaints(node *corev1.Node, tolerations []corev1.Toleration) bool { + _, untolerated := corev1helpers.FindMatchingUntoleratedTaint( + node.Spec.Taints, tolerations, + func(t *corev1.Taint) bool { + return t.Effect == corev1.TaintEffectNoSchedule || t.Effect == corev1.TaintEffectNoExecute + }, + ) + return !untolerated +} + func CrossProductTerms(a, b []corev1.NodeSelectorTerm) []corev1.NodeSelectorTerm { var result []corev1.NodeSelectorTerm for _, termA := range a { diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/validators/pv_node_affinity_validator.go b/images/virtualization-artifact/pkg/controller/vm/internal/validators/pv_node_affinity_validator.go index 1391634c9a..93fbfbe9f7 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/validators/pv_node_affinity_validator.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/validators/pv_node_affinity_validator.go @@ -20,23 +20,30 @@ import ( "context" "fmt" "reflect" + "strings" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/types" + k8snodeaffinity "k8s.io/component-helpers/scheduling/corev1/nodeaffinity" + "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + "github.com/deckhouse/virtualization-controller/pkg/common/nodeaffinity" "github.com/deckhouse/virtualization-controller/pkg/controller/service" "github.com/deckhouse/virtualization/api/core/v1alpha2" ) type PVNodeAffinityValidator struct { + client client.Client attacher *service.AttachmentService } -func NewPVNodeAffinityValidator(attacher *service.AttachmentService) *PVNodeAffinityValidator { - return &PVNodeAffinityValidator{attacher: attacher} +func NewPVNodeAffinityValidator(client client.Client, attacher *service.AttachmentService) *PVNodeAffinityValidator { + return &PVNodeAffinityValidator{client: client, attacher: attacher} } -func (v *PVNodeAffinityValidator) ValidateCreate(_ context.Context, _ *v1alpha2.VirtualMachine) (admission.Warnings, error) { - return nil, nil +func (v *PVNodeAffinityValidator) ValidateCreate(ctx context.Context, vm *v1alpha2.VirtualMachine) (admission.Warnings, error) { + return v.validateUnscheduledVM(ctx, vm, vm.Spec.BlockDeviceRefs, "create") } func (v *PVNodeAffinityValidator) ValidateUpdate(ctx context.Context, oldVM, newVM *v1alpha2.VirtualMachine) (admission.Warnings, error) { @@ -44,10 +51,14 @@ func (v *PVNodeAffinityValidator) ValidateUpdate(ctx context.Context, oldVM, new return nil, nil } - if newVM.Status.Node == "" { - return nil, nil + if newVM.Status.Node != "" { + return v.validateScheduledVM(ctx, oldVM, newVM) } + return v.validateUnscheduledVM(ctx, newVM, newVM.Spec.BlockDeviceRefs, "update") +} + +func (v *PVNodeAffinityValidator) validateScheduledVM(ctx context.Context, oldVM, newVM *v1alpha2.VirtualMachine) (admission.Warnings, error) { kvvmi, err := v.attacher.GetKVVMI(ctx, newVM) if err != nil { return nil, fmt.Errorf("failed to get KVVMI for VM %q: %w", newVM.Name, err) @@ -58,13 +69,12 @@ func (v *PVNodeAffinityValidator) ValidateUpdate(ctx context.Context, oldVM, new oldRefs := make(map[string]struct{}, len(oldVM.Spec.BlockDeviceRefs)) for _, ref := range oldVM.Spec.BlockDeviceRefs { - key := string(ref.Kind) + "/" + ref.Name - oldRefs[key] = struct{}{} + oldRefs[string(ref.Kind)+"/"+ref.Name] = struct{}{} } + var incompatibleDisks []string for _, ref := range newVM.Spec.BlockDeviceRefs { - key := string(ref.Kind) + "/" + ref.Name - if _, existed := oldRefs[key]; existed { + if _, existed := oldRefs[string(ref.Kind)+"/"+ref.Name]; existed { continue } @@ -89,16 +99,99 @@ func (v *PVNodeAffinityValidator) ValidateUpdate(ctx context.Context, oldVM, new return nil, fmt.Errorf("failed to check PV availability: %w", err) } if !available { - return nil, fmt.Errorf( - "unable to attach disk %q to VM %q: the disk is not available on node %q where the VM is running", - ref.Name, newVM.Name, newVM.Status.Node, - ) + incompatibleDisks = append(incompatibleDisks, ref.Name) } } + if len(incompatibleDisks) > 0 { + return nil, fmt.Errorf( + `unable to attach disks to VM %q: disks ["%s"] are not available on node %q where the VM is running`, + newVM.Name, strings.Join(incompatibleDisks, `", "`), newVM.Status.Node, + ) + } + return nil, nil } +func (v *PVNodeAffinityValidator) validateUnscheduledVM(ctx context.Context, vm *v1alpha2.VirtualMachine, refs []v1alpha2.BlockDeviceSpecRef, action string) (admission.Warnings, error) { + pvSelectors, err := v.collectPVNodeSelectors(ctx, refs, vm.Namespace) + if err != nil { + return nil, err + } + if len(pvSelectors) == 0 { + return nil, nil + } + + var vmClass v1alpha2.VirtualMachineClass + if err := v.client.Get(ctx, types.NamespacedName{Name: vm.Spec.VirtualMachineClassName}, &vmClass); err != nil { + return nil, nil + } + + var nodeList corev1.NodeList + if err := v.client.List(ctx, &nodeList); err != nil { + return nil, fmt.Errorf("failed to list nodes: %w", err) + } + + for i := range nodeList.Items { + node := &nodeList.Items[i] + if !nodeaffinity.MatchesVMPlacement(node, vm, &vmClass) { + continue + } + matchesAllPVs := true + for _, pvSel := range pvSelectors { + if !pvSel.Match(node) { + matchesAllPVs = false + break + } + } + if matchesAllPVs { + return nil, nil + } + } + + return nil, fmt.Errorf( + `unable to %s VM %q due to a topology conflict. Ensure that all disks are accessible on the nodes in accordance with the VM node placement rules (node selector, affinity, tolerations)`, + action, vm.Name, + ) +} + +func (v *PVNodeAffinityValidator) collectPVNodeSelectors(ctx context.Context, refs []v1alpha2.BlockDeviceSpecRef, namespace string) ([]*k8snodeaffinity.NodeSelector, error) { + var selectors []*k8snodeaffinity.NodeSelector + for _, ref := range refs { + ad, err := v.resolveAttachmentDisk(ctx, ref, namespace) + if err != nil { + return nil, err + } + if ad == nil || ad.PVCName == "" { + continue + } + + pvc, err := v.attacher.GetPersistentVolumeClaim(ctx, ad) + if err != nil { + return nil, fmt.Errorf("failed to get PVC %q: %w", ad.PVCName, err) + } + if pvc == nil || pvc.Spec.VolumeName == "" { + continue + } + + var pv corev1.PersistentVolume + if err := v.client.Get(ctx, types.NamespacedName{Name: pvc.Spec.VolumeName}, &pv); err != nil { + continue + } + + if pv.Spec.NodeAffinity == nil || pv.Spec.NodeAffinity.Required == nil { + continue + } + + ns, err := k8snodeaffinity.NewNodeSelector(pv.Spec.NodeAffinity.Required) + if err != nil { + continue + } + selectors = append(selectors, ns) + } + return selectors, nil +} + func (v *PVNodeAffinityValidator) resolveAttachmentDisk(ctx context.Context, ref v1alpha2.BlockDeviceSpecRef, namespace string) (*service.AttachmentDisk, error) { switch ref.Kind { case v1alpha2.DiskDevice: diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/validators/pv_node_affinity_validator_test.go b/images/virtualization-artifact/pkg/controller/vm/internal/validators/pv_node_affinity_validator_test.go index 085628db60..c2f58ff43a 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/validators/pv_node_affinity_validator_test.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/validators/pv_node_affinity_validator_test.go @@ -37,12 +37,13 @@ var _ = Describe("PVNodeAffinityValidator", func() { node2 = "node-2" ) - makeNode := func() *corev1.Node { + makeNode := func(name string, taint ...corev1.Taint) *corev1.Node { return &corev1.Node{ ObjectMeta: metav1.ObjectMeta{ - Name: node1, - Labels: map[string]string{"topology.kubernetes.io/node": node1}, + Name: name, + Labels: map[string]string{"topology.kubernetes.io/node": name}, }, + Spec: corev1.NodeSpec{Taints: taint}, } } @@ -81,8 +82,17 @@ var _ = Describe("PVNodeAffinityValidator", func() { makeVM := func(nodeName string, refs ...v1alpha2.BlockDeviceSpecRef) *v1alpha2.VirtualMachine { return &v1alpha2.VirtualMachine{ ObjectMeta: metav1.ObjectMeta{Name: "vm", Namespace: ns}, - Spec: v1alpha2.VirtualMachineSpec{BlockDeviceRefs: refs}, - Status: v1alpha2.VirtualMachineStatus{Node: nodeName}, + Spec: v1alpha2.VirtualMachineSpec{ + BlockDeviceRefs: refs, + VirtualMachineClassName: "generic", + }, + Status: v1alpha2.VirtualMachineStatus{Node: nodeName}, + } + } + + makeVMClass := func() *v1alpha2.VirtualMachineClass { + return &v1alpha2.VirtualMachineClass{ + ObjectMeta: metav1.ObjectMeta{Name: "generic"}, } } @@ -96,121 +106,189 @@ var _ = Describe("PVNodeAffinityValidator", func() { makeValidator := func(objs ...client.Object) *validators.PVNodeAffinityValidator { fakeClient := setupEnvironment(objs...) attacher := service.NewAttachmentService(fakeClient, nil, "") - return validators.NewPVNodeAffinityValidator(attacher) + return validators.NewPVNodeAffinityValidator(fakeClient, attacher) } - It("should allow when VM is not running (no node)", func() { - oldVM := makeVM("", v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "disk1"}) - newVM := makeVM("", - v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "disk1"}, - v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "disk2"}, - ) - v := makeValidator(oldVM) - _, err := v.ValidateUpdate(testutil.ContextBackgroundWithNoOpLogger(), oldVM, newVM) - Expect(err).ShouldNot(HaveOccurred()) - }) + Context("scheduled VM", func() { + It("should allow when blockDeviceRefs unchanged", func() { + refs := []v1alpha2.BlockDeviceSpecRef{{Kind: v1alpha2.DiskDevice, Name: "disk1"}} + oldVM := makeVM(node1, refs...) + newVM := makeVM(node1, refs...) + v := makeValidator(oldVM, makeNode(node1)) + _, err := v.ValidateUpdate(testutil.ContextBackgroundWithNoOpLogger(), oldVM, newVM) + Expect(err).ShouldNot(HaveOccurred()) + }) - It("should allow when blockDeviceRefs unchanged", func() { - refs := []v1alpha2.BlockDeviceSpecRef{{Kind: v1alpha2.DiskDevice, Name: "disk1"}} - oldVM := makeVM(node1, refs...) - newVM := makeVM(node1, refs...) - v := makeValidator(oldVM, makeNode()) - _, err := v.ValidateUpdate(testutil.ContextBackgroundWithNoOpLogger(), oldVM, newVM) - Expect(err).ShouldNot(HaveOccurred()) - }) + It("should allow adding a network disk", func() { + oldVM := makeVM(node1, v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "disk1"}) + newVM := makeVM(node1, + v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "disk1"}, + v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "net-disk"}, + ) + v := makeValidator( + oldVM, makeNode(node1), makeKVVMI(), + makeVD("net-disk", "pvc-net"), + makePVC("pvc-net", "pv-net"), + makePV("pv-net"), + ) + _, err := v.ValidateUpdate(testutil.ContextBackgroundWithNoOpLogger(), oldVM, newVM) + Expect(err).ShouldNot(HaveOccurred()) + }) - It("should allow adding a network disk (PV without nodeAffinity)", func() { - oldVM := makeVM(node1, v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "disk1"}) - newVM := makeVM(node1, - v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "disk1"}, - v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "net-disk"}, - ) - v := makeValidator( - oldVM, makeNode(), makeKVVMI(), - makeVD("net-disk", "pvc-net"), - makePVC("pvc-net", "pv-net"), - makePV("pv-net"), - ) - _, err := v.ValidateUpdate(testutil.ContextBackgroundWithNoOpLogger(), oldVM, newVM) - Expect(err).ShouldNot(HaveOccurred()) - }) + It("should allow adding a local disk available on VM node", func() { + oldVM := makeVM(node1, v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "disk1"}) + newVM := makeVM(node1, + v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "disk1"}, + v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "local-disk"}, + ) + v := makeValidator( + oldVM, makeNode(node1), makeKVVMI(), + makeVD("local-disk", "pvc-local"), + makePVC("pvc-local", "pv-local"), + makePV("pv-local", node1), + ) + _, err := v.ValidateUpdate(testutil.ContextBackgroundWithNoOpLogger(), oldVM, newVM) + Expect(err).ShouldNot(HaveOccurred()) + }) - It("should allow adding a local disk available on VM node", func() { - oldVM := makeVM(node1, v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "disk1"}) - newVM := makeVM(node1, - v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "disk1"}, - v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "local-disk"}, - ) - v := makeValidator( - oldVM, makeNode(), makeKVVMI(), - makeVD("local-disk", "pvc-local"), - makePVC("pvc-local", "pv-local"), - makePV("pv-local", node1), - ) - _, err := v.ValidateUpdate(testutil.ContextBackgroundWithNoOpLogger(), oldVM, newVM) - Expect(err).ShouldNot(HaveOccurred()) - }) + It("should reject adding a local disk NOT available on VM node", func() { + oldVM := makeVM(node1, v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "disk1"}) + newVM := makeVM(node1, + v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "disk1"}, + v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "local-disk"}, + ) + v := makeValidator( + oldVM, makeNode(node1), makeKVVMI(), + makeVD("local-disk", "pvc-local"), + makePVC("pvc-local", "pv-local"), + makePV("pv-local", node2), + ) + _, err := v.ValidateUpdate(testutil.ContextBackgroundWithNoOpLogger(), oldVM, newVM) + Expect(err).Should(HaveOccurred()) + Expect(err.Error()).Should(ContainSubstring("unable to attach disks")) + Expect(err.Error()).Should(ContainSubstring("local-disk")) + }) - It("should reject adding a local disk NOT available on VM node", func() { - oldVM := makeVM(node1, v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "disk1"}) - newVM := makeVM(node1, - v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "disk1"}, - v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "local-disk"}, - ) - v := makeValidator( - oldVM, makeNode(), makeKVVMI(), - makeVD("local-disk", "pvc-local"), - makePVC("pvc-local", "pv-local"), - makePV("pv-local", node2), - ) - _, err := v.ValidateUpdate(testutil.ContextBackgroundWithNoOpLogger(), oldVM, newVM) - Expect(err).Should(HaveOccurred()) - Expect(err.Error()).Should(ContainSubstring("unable to attach disk")) - Expect(err.Error()).Should(ContainSubstring("local-disk")) - }) + It("should list all incompatible disks in error message", func() { + oldVM := makeVM(node1) + newVM := makeVM(node1, + v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "bad-1"}, + v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "bad-2"}, + ) + v := makeValidator( + oldVM, makeNode(node1), makeKVVMI(), + makeVD("bad-1", "pvc-1"), makePVC("pvc-1", "pv-1"), makePV("pv-1", node2), + makeVD("bad-2", "pvc-2"), makePVC("pvc-2", "pv-2"), makePV("pv-2", node2), + ) + _, err := v.ValidateUpdate(testutil.ContextBackgroundWithNoOpLogger(), oldVM, newVM) + Expect(err).Should(HaveOccurred()) + Expect(err.Error()).Should(ContainSubstring("bad-1")) + Expect(err.Error()).Should(ContainSubstring("bad-2")) + }) - It("should allow adding a disk with pending PVC (WFFC, no PV yet)", func() { - oldVM := makeVM(node1) - newVM := makeVM(node1, - v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "new-disk"}, - ) - pvcPending := &corev1.PersistentVolumeClaim{ - ObjectMeta: metav1.ObjectMeta{Name: "pvc-pending", Namespace: ns}, - Spec: corev1.PersistentVolumeClaimSpec{}, - } - v := makeValidator( - oldVM, makeNode(), makeKVVMI(), - makeVD("new-disk", "pvc-pending"), - pvcPending, - ) - _, err := v.ValidateUpdate(testutil.ContextBackgroundWithNoOpLogger(), oldVM, newVM) - Expect(err).ShouldNot(HaveOccurred()) + It("should allow adding a disk with pending PVC", func() { + oldVM := makeVM(node1) + newVM := makeVM(node1, + v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "new-disk"}, + ) + v := makeValidator( + oldVM, makeNode(node1), makeKVVMI(), + makeVD("new-disk", "pvc-pending"), + &corev1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{Name: "pvc-pending", Namespace: ns}, + }, + ) + _, err := v.ValidateUpdate(testutil.ContextBackgroundWithNoOpLogger(), oldVM, newVM) + Expect(err).ShouldNot(HaveOccurred()) + }) }) - It("should reject when any of multiple new disks is incompatible", func() { - oldVM := makeVM(node1) - newVM := makeVM(node1, - v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "good-disk"}, - v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "bad-disk"}, - ) - v := makeValidator( - oldVM, makeNode(), makeKVVMI(), - makeVD("good-disk", "pvc-good"), - makePVC("pvc-good", "pv-good"), - makePV("pv-good", node1), - makeVD("bad-disk", "pvc-bad"), - makePVC("pvc-bad", "pv-bad"), - makePV("pv-bad", node2), - ) - _, err := v.ValidateUpdate(testutil.ContextBackgroundWithNoOpLogger(), oldVM, newVM) - Expect(err).Should(HaveOccurred()) - Expect(err.Error()).Should(ContainSubstring("bad-disk")) - }) + Context("unscheduled VM", func() { + It("should allow create when topology is compatible", func() { + vm := makeVM("", + v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "local-disk"}, + ) + v := makeValidator( + vm, makeNode(node1), makeVMClass(), + makeVD("local-disk", "pvc-local"), + makePVC("pvc-local", "pv-local"), + makePV("pv-local", node1), + ) + _, err := v.ValidateCreate(testutil.ContextBackgroundWithNoOpLogger(), vm) + Expect(err).ShouldNot(HaveOccurred()) + }) + + It("should reject create when no node satisfies all constraints", func() { + vm := makeVM("", + v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "disk-a"}, + v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "disk-b"}, + ) + v := makeValidator( + vm, makeNode(node1), makeNode(node2), makeVMClass(), + makeVD("disk-a", "pvc-a"), makePVC("pvc-a", "pv-a"), makePV("pv-a", node1), + makeVD("disk-b", "pvc-b"), makePVC("pvc-b", "pv-b"), makePV("pv-b", node2), + ) + _, err := v.ValidateCreate(testutil.ContextBackgroundWithNoOpLogger(), vm) + Expect(err).Should(HaveOccurred()) + Expect(err.Error()).Should(ContainSubstring("unable to create")) + Expect(err.Error()).Should(ContainSubstring("topology conflict")) + }) + + It("should reject update when no node satisfies all constraints", func() { + oldVM := makeVM("") + newVM := makeVM("", + v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "disk-a"}, + v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "disk-b"}, + ) + v := makeValidator( + newVM, makeNode(node1), makeNode(node2), makeVMClass(), + makeVD("disk-a", "pvc-a"), makePVC("pvc-a", "pv-a"), makePV("pv-a", node1), + makeVD("disk-b", "pvc-b"), makePVC("pvc-b", "pv-b"), makePV("pv-b", node2), + ) + _, err := v.ValidateUpdate(testutil.ContextBackgroundWithNoOpLogger(), oldVM, newVM) + Expect(err).Should(HaveOccurred()) + Expect(err.Error()).Should(ContainSubstring("unable to update")) + }) - It("should allow on create (VM not running yet)", func() { - vm := makeVM("", v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "disk1"}) - v := makeValidator(vm) - _, err := v.ValidateCreate(testutil.ContextBackgroundWithNoOpLogger(), vm) - Expect(err).ShouldNot(HaveOccurred()) + It("should allow when disks have no PV nodeAffinity (network storage)", func() { + vm := makeVM("", + v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "net-disk"}, + ) + v := makeValidator( + vm, makeNode(node1), makeVMClass(), + makeVD("net-disk", "pvc-net"), + makePVC("pvc-net", "pv-net"), + makePV("pv-net"), + ) + _, err := v.ValidateCreate(testutil.ContextBackgroundWithNoOpLogger(), vm) + Expect(err).ShouldNot(HaveOccurred()) + }) + + It("should reject when PV node conflicts with VMClass nodeSelector", func() { + vm := makeVM("", + v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "local-disk"}, + ) + vmClass := &v1alpha2.VirtualMachineClass{ + ObjectMeta: metav1.ObjectMeta{Name: "generic"}, + Spec: v1alpha2.VirtualMachineClassSpec{ + NodeSelector: v1alpha2.NodeSelector{ + MatchExpressions: []corev1.NodeSelectorRequirement{{ + Key: "topology.kubernetes.io/node", + Operator: corev1.NodeSelectorOpIn, + Values: []string{node2}, + }}, + }, + }, + } + v := makeValidator( + vm, makeNode(node1), makeNode(node2), vmClass, + makeVD("local-disk", "pvc-local"), + makePVC("pvc-local", "pv-local"), + makePV("pv-local", node1), // disk on node1, class requires node2 + ) + _, err := v.ValidateCreate(testutil.ContextBackgroundWithNoOpLogger(), vm) + Expect(err).Should(HaveOccurred()) + Expect(err.Error()).Should(ContainSubstring("topology conflict")) + }) }) }) diff --git a/images/virtualization-artifact/pkg/controller/vm/vm_webhook.go b/images/virtualization-artifact/pkg/controller/vm/vm_webhook.go index 5ec2bc9283..6f52795685 100644 --- a/images/virtualization-artifact/pkg/controller/vm/vm_webhook.go +++ b/images/virtualization-artifact/pkg/controller/vm/vm_webhook.go @@ -57,7 +57,7 @@ func NewValidator(client client.Client, blockDeviceService *service.BlockDeviceS validators.NewFirstDiskValidator(client), validators.NewUSBDevicesValidator(client, featureGate), validators.NewVMBDAConflictValidator(client), - validators.NewPVNodeAffinityValidator(attachmentService), + validators.NewPVNodeAffinityValidator(client, attachmentService), }, log: log.With("webhook", "validation"), } diff --git a/images/virtualization-artifact/pkg/controller/vmbda/internal/validators/pv_node_affinity_validator.go b/images/virtualization-artifact/pkg/controller/vmbda/internal/validators/pv_node_affinity_validator.go index 773e430423..81e298ae1e 100644 --- a/images/virtualization-artifact/pkg/controller/vmbda/internal/validators/pv_node_affinity_validator.go +++ b/images/virtualization-artifact/pkg/controller/vmbda/internal/validators/pv_node_affinity_validator.go @@ -20,18 +20,24 @@ import ( "context" "fmt" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/types" + k8snodeaffinity "k8s.io/component-helpers/scheduling/corev1/nodeaffinity" + "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + "github.com/deckhouse/virtualization-controller/pkg/common/nodeaffinity" "github.com/deckhouse/virtualization-controller/pkg/controller/service" "github.com/deckhouse/virtualization/api/core/v1alpha2" ) type PVNodeAffinityValidator struct { + client client.Client attacher *service.AttachmentService } -func NewPVNodeAffinityValidator(attacher *service.AttachmentService) *PVNodeAffinityValidator { - return &PVNodeAffinityValidator{attacher: attacher} +func NewPVNodeAffinityValidator(client client.Client, attacher *service.AttachmentService) *PVNodeAffinityValidator { + return &PVNodeAffinityValidator{client: client, attacher: attacher} } func (v *PVNodeAffinityValidator) ValidateCreate(ctx context.Context, vmbda *v1alpha2.VirtualMachineBlockDeviceAttachment) (admission.Warnings, error) { @@ -39,10 +45,22 @@ func (v *PVNodeAffinityValidator) ValidateCreate(ctx context.Context, vmbda *v1a if err != nil { return nil, fmt.Errorf("failed to get VirtualMachine %q: %w", vmbda.Spec.VirtualMachineName, err) } - if vm == nil || vm.Status.Node == "" { + if vm == nil { return nil, nil } + if vm.Status.Node != "" { + return v.validateScheduledVM(ctx, vm, vmbda) + } + + return v.validateUnscheduledVM(ctx, vm, vmbda) +} + +func (v *PVNodeAffinityValidator) ValidateUpdate(_ context.Context, _, _ *v1alpha2.VirtualMachineBlockDeviceAttachment) (admission.Warnings, error) { + return nil, nil +} + +func (v *PVNodeAffinityValidator) validateScheduledVM(ctx context.Context, vm *v1alpha2.VirtualMachine, vmbda *v1alpha2.VirtualMachineBlockDeviceAttachment) (admission.Warnings, error) { kvvmi, err := v.attacher.GetKVVMI(ctx, vm) if err != nil { return nil, fmt.Errorf("failed to get KVVMI for VM %q: %w", vm.Name, err) @@ -74,16 +92,65 @@ func (v *PVNodeAffinityValidator) ValidateCreate(ctx context.Context, vmbda *v1a if !available { return nil, fmt.Errorf( - "unable to attach disk %q to VM %q: the disk is not available on node %q where the VM is running", - vmbda.Spec.BlockDeviceRef.Name, vmbda.Spec.VirtualMachineName, vm.Status.Node, + `unable to attach disk to VM %q: the disk %q is not available on node %q where the VM is running`, + vmbda.Spec.VirtualMachineName, vmbda.Spec.BlockDeviceRef.Name, vm.Status.Node, ) } return nil, nil } -func (v *PVNodeAffinityValidator) ValidateUpdate(_ context.Context, _, _ *v1alpha2.VirtualMachineBlockDeviceAttachment) (admission.Warnings, error) { - return nil, nil +func (v *PVNodeAffinityValidator) validateUnscheduledVM(ctx context.Context, vm *v1alpha2.VirtualMachine, vmbda *v1alpha2.VirtualMachineBlockDeviceAttachment) (admission.Warnings, error) { + ad, err := v.resolveAttachmentDisk(ctx, vmbda) + if err != nil { + return nil, err + } + if ad == nil || ad.PVCName == "" { + return nil, nil + } + + pvc, err := v.attacher.GetPersistentVolumeClaim(ctx, ad) + if err != nil { + return nil, fmt.Errorf("failed to get PVC %q: %w", ad.PVCName, err) + } + if pvc == nil || pvc.Spec.VolumeName == "" { + return nil, nil + } + + var pv corev1.PersistentVolume + if err := v.client.Get(ctx, types.NamespacedName{Name: pvc.Spec.VolumeName}, &pv); err != nil { + return nil, nil + } + if pv.Spec.NodeAffinity == nil || pv.Spec.NodeAffinity.Required == nil { + return nil, nil + } + + pvSel, err := k8snodeaffinity.NewNodeSelector(pv.Spec.NodeAffinity.Required) + if err != nil { + return nil, nil + } + + var vmClass v1alpha2.VirtualMachineClass + if err := v.client.Get(ctx, types.NamespacedName{Name: vm.Spec.VirtualMachineClassName}, &vmClass); err != nil { + return nil, nil + } + + var nodeList corev1.NodeList + if err := v.client.List(ctx, &nodeList); err != nil { + return nil, fmt.Errorf("failed to list nodes: %w", err) + } + + for i := range nodeList.Items { + node := &nodeList.Items[i] + if nodeaffinity.MatchesVMPlacement(node, vm, &vmClass) && pvSel.Match(node) { + return nil, nil + } + } + + return nil, fmt.Errorf( + `unable to attach disk to VM %q due to a topology conflict. Ensure that disk %q is accessible on the nodes in accordance with the VM node placement rules (node selector, affinity, tolerations)`, + vmbda.Spec.VirtualMachineName, vmbda.Spec.BlockDeviceRef.Name, + ) } func (v *PVNodeAffinityValidator) resolveAttachmentDisk(ctx context.Context, vmbda *v1alpha2.VirtualMachineBlockDeviceAttachment) (*service.AttachmentDisk, error) { diff --git a/images/virtualization-artifact/pkg/controller/vmbda/vmbda_controller.go b/images/virtualization-artifact/pkg/controller/vmbda/vmbda_controller.go index 99cff3e675..3c0280a644 100644 --- a/images/virtualization-artifact/pkg/controller/vmbda/vmbda_controller.go +++ b/images/virtualization-artifact/pkg/controller/vmbda/vmbda_controller.go @@ -74,7 +74,7 @@ func NewController( if err = builder.WebhookManagedBy(mgr). For(&v1alpha2.VirtualMachineBlockDeviceAttachment{}). - WithValidator(NewValidator(attacher, blockDeviceService, lg)). + WithValidator(NewValidator(mgr.GetClient(), attacher, blockDeviceService, lg)). Complete(); err != nil { return nil, err } diff --git a/images/virtualization-artifact/pkg/controller/vmbda/vmbda_webhook.go b/images/virtualization-artifact/pkg/controller/vmbda/vmbda_webhook.go index 1c3c976c63..d38986fe87 100644 --- a/images/virtualization-artifact/pkg/controller/vmbda/vmbda_webhook.go +++ b/images/virtualization-artifact/pkg/controller/vmbda/vmbda_webhook.go @@ -21,6 +21,7 @@ import ( "fmt" "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" "github.com/deckhouse/deckhouse/pkg/log" @@ -39,14 +40,14 @@ type Validator struct { log *log.Logger } -func NewValidator(attachmentService *service.AttachmentService, service *service.BlockDeviceService, log *log.Logger) *Validator { +func NewValidator(c client.Client, attachmentService *service.AttachmentService, service *service.BlockDeviceService, log *log.Logger) *Validator { return &Validator{ log: log.With("webhook", "validation"), validators: []VirtualMachineBlockDeviceAttachmentValidator{ validators.NewSpecMutateValidator(), validators.NewAttachmentConflictValidator(attachmentService, log), validators.NewVMConnectLimiterValidator(service, log), - validators.NewPVNodeAffinityValidator(attachmentService), + validators.NewPVNodeAffinityValidator(c, attachmentService), }, } } From 39ceb629047a9979f321f03e974eb5442f6a01cb Mon Sep 17 00:00:00 2001 From: Daniil Loktev Date: Wed, 25 Mar 2026 15:48:58 +0300 Subject: [PATCH 6/6] wip Signed-off-by: Daniil Loktev --- .../vm/internal/validators/pv_node_affinity_validator_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/validators/pv_node_affinity_validator_test.go b/images/virtualization-artifact/pkg/controller/vm/internal/validators/pv_node_affinity_validator_test.go index c2f58ff43a..6ffbc2d2e1 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/validators/pv_node_affinity_validator_test.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/validators/pv_node_affinity_validator_test.go @@ -83,8 +83,8 @@ var _ = Describe("PVNodeAffinityValidator", func() { return &v1alpha2.VirtualMachine{ ObjectMeta: metav1.ObjectMeta{Name: "vm", Namespace: ns}, Spec: v1alpha2.VirtualMachineSpec{ - BlockDeviceRefs: refs, - VirtualMachineClassName: "generic", + BlockDeviceRefs: refs, + VirtualMachineClassName: "generic", }, Status: v1alpha2.VirtualMachineStatus{Node: nodeName}, }