From ebdff9390e72888ddaca32ea905cc2125327e604 Mon Sep 17 00:00:00 2001 From: Pavel Tishkov Date: Sat, 27 Jun 2026 18:22:48 +0300 Subject: [PATCH 1/8] feat(api): decouple KubeVirt disk/volume names from resource names Introduce shortenDiskName in kvbuilder: names within the per-device budget that are valid DNS-1123 labels pass through byte-identically (existing volumes are never renamed), while longer or non-label names are deterministically shortened to a readable prefix plus an FNV-1a 64-bit hash of the full name, keeping the derived KubeVirt volume/disk name a valid label within 63 chars. This is the groundwork for allowing resource names up to the Kubernetes limit: behaviour is unchanged for every name the current validation already accepts. Add a Ginkgo suite bootstrap to the kvbuilder package (it had none, so the existing specs did not run) and golden tests for the new derivation. Signed-off-by: Pavel Tishkov --- .../controller/kvbuilder/kvvm_names_test.go | 102 ++++++++++++++++++ .../pkg/controller/kvbuilder/kvvm_utils.go | 80 ++++++++++++-- .../pkg/controller/kvbuilder/suite_test.go | 29 +++++ 3 files changed, 205 insertions(+), 6 deletions(-) create mode 100644 images/virtualization-artifact/pkg/controller/kvbuilder/kvvm_names_test.go create mode 100644 images/virtualization-artifact/pkg/controller/kvbuilder/suite_test.go diff --git a/images/virtualization-artifact/pkg/controller/kvbuilder/kvvm_names_test.go b/images/virtualization-artifact/pkg/controller/kvbuilder/kvvm_names_test.go new file mode 100644 index 0000000000..1d1dd8139c --- /dev/null +++ b/images/virtualization-artifact/pkg/controller/kvbuilder/kvvm_names_test.go @@ -0,0 +1,102 @@ +/* +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 kvbuilder + +import ( + "strings" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + kvalidation "k8s.io/apimachinery/pkg/util/validation" + + "github.com/deckhouse/virtualization/api/core/v1alpha2" +) + +var _ = Describe("Derived KubeVirt disk/volume names", func() { + DescribeTable("passthrough: short, valid label names are returned unchanged", + func(name string) { + // Every name allowed by the previous validation must map to the + // byte-identical derived name, so existing volumes are never renamed. + Expect(GenerateVDDiskName(name)).To(Equal("vd-" + name)) + }, + Entry("typical", "web-data"), + Entry("exactly at budget", strings.Repeat("a", vdNameBudget)), + ) + + It("hashes names over the budget into a valid label within 63 chars", func() { + name := strings.Repeat("a", vdNameBudget+1) + got := GenerateVDDiskName(name) + + Expect(got).To(HavePrefix("vd-")) + Expect(len(got)).To(BeNumerically("<=", 63)) + Expect(kvalidation.IsDNS1123Label(got)).To(BeEmpty()) + Expect(got).NotTo(Equal("vd-" + name)) + Expect(got).To(MatchRegexp(`-[0-9a-f]{16}$`)) // FNV-1a 64-bit suffix + }) + + It("is deterministic for the same input", func() { + name := strings.Repeat("b", 120) + Expect(GenerateVDDiskName(name)).To(Equal(GenerateVDDiskName(name))) + }) + + It("allows dots by sanitizing and hashing instead of passthrough", func() { + got := GenerateVDDiskName("disk.2024.backup") + Expect(got).NotTo(ContainSubstring(".")) + Expect(kvalidation.IsDNS1123Label(got)).To(BeEmpty()) + Expect(got).To(HavePrefix("vd-disk-2024-backup-")) + }) + + It("does not collide when sanitization makes readable parts equal", func() { + // The dotted name is hashed; its dashed twin is a valid label (passthrough). + Expect(GenerateVDDiskName("disk.2024.backup")). + NotTo(Equal(GenerateVDDiskName("disk-2024-backup"))) + }) + + It("does not collide when long names share a truncated prefix", func() { + base := strings.Repeat("a", vdNameBudget) + Expect(GenerateVDDiskName(base + "-one")). + NotTo(Equal(GenerateVDDiskName(base + "-two"))) + }) + + DescribeTable("containerDisk container name stays within 63 for long image names", + func(gen func(string) string) { + vol := gen(strings.Repeat("a", 200)) + // KubeVirt wraps a containerDisk volume name as "volume-init". + container := "volume" + vol + "-init" + Expect(kvalidation.IsDNS1123Label(vol)).To(BeEmpty()) + Expect(len(container)).To(BeNumerically("<=", 63), container) + }, + Entry("VirtualImage", GenerateVIDiskName), + Entry("ClusterVirtualImage", GenerateCVIDiskName), + ) + + DescribeTable("GenerateDiskName routes to the kind-specific generator", + func(kind v1alpha2.BlockDeviceKind, gen func(string) string) { + name := strings.Repeat("z", 80) + Expect(GenerateDiskName(kind, name)).To(Equal(gen(name))) + }, + Entry("disk", v1alpha2.DiskDevice, GenerateVDDiskName), + Entry("image", v1alpha2.ImageDevice, GenerateVIDiskName), + Entry("cluster image", v1alpha2.ClusterImageDevice, GenerateCVIDiskName), + ) + + It("round-trips legacy short names through GetOriginalDiskName", func() { + name, kind := GetOriginalDiskName(GenerateVDDiskName("data-disk")) + Expect(name).To(Equal("data-disk")) + Expect(kind).To(Equal(v1alpha2.DiskDevice)) + }) +}) diff --git a/images/virtualization-artifact/pkg/controller/kvbuilder/kvvm_utils.go b/images/virtualization-artifact/pkg/controller/kvbuilder/kvvm_utils.go index 7c29d677fe..b4e7c475e9 100644 --- a/images/virtualization-artifact/pkg/controller/kvbuilder/kvvm_utils.go +++ b/images/virtualization-artifact/pkg/controller/kvbuilder/kvvm_utils.go @@ -20,11 +20,13 @@ import ( "crypto/md5" "encoding/hex" "fmt" + "hash/fnv" "slices" "strings" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" + kvalidation "k8s.io/apimachinery/pkg/util/validation" "k8s.io/utils/ptr" virtv1 "kubevirt.io/api/core/v1" @@ -42,30 +44,96 @@ const ( CVIDiskPrefix = "cvi-" ) +// Budgets for the user-controlled part of a derived KubeVirt volume/disk name. +// +// A KubeVirt volume/disk name must be a valid DNS-1123 label (<=63), because it +// can become a container name. For containerDisks (VirtualImage/ClusterVirtualImage) +// KubeVirt additionally wraps the volume name as "volume-init", so the +// effective budget is tighter. These values keep the final container name within 63: +// +// VD : "vd-"+X , X <= 60 +// VI : "volume"+"vi-"+X+"-init" <=63 , X <= 49 +// CVI: "volume"+"cvi-"+X+"-init" <=63 , X <= 48 +const ( + vdNameBudget = 60 + viNameBudget = 49 + cviNameBudget = 48 +) + +// diskNameHashLen is the length in hex chars of the FNV-1a 64-bit suffix appended +// when the user name does not fit the budget (or is not a valid DNS-1123 label). +const diskNameHashLen = 16 + func GenerateVDDiskName(name string) string { - return VDDiskPrefix + name + return VDDiskPrefix + shortenDiskName(name, vdNameBudget) } func GenerateVIDiskName(name string) string { - return VIDiskPrefix + name + return VIDiskPrefix + shortenDiskName(name, viNameBudget) } func GenerateCVIDiskName(name string) string { - return CVIDiskPrefix + name + return CVIDiskPrefix + shortenDiskName(name, cviNameBudget) } func GenerateDiskName(kind v1alpha2.BlockDeviceKind, name string) string { switch kind { case v1alpha2.DiskDevice: - return VDDiskPrefix + name + return GenerateVDDiskName(name) case v1alpha2.ImageDevice: - return VIDiskPrefix + name + return GenerateVIDiskName(name) case v1alpha2.ClusterImageDevice: - return CVIDiskPrefix + name + return GenerateCVIDiskName(name) } return "" } +// shortenDiskName maps a user resource name onto the user-controlled part of a +// KubeVirt volume/disk name within the given budget. +// +// If the name already fits the budget and is a valid DNS-1123 label, it is +// returned unchanged (passthrough) — this keeps derived names byte-identical for +// every name allowed by the previous validation, so existing KubeVirt volumes are +// never renamed. Otherwise the name is deterministically shortened to a readable +// prefix plus an FNV-1a 64-bit hash of the full name, keeping the result a valid +// label within the budget. The hash is taken from the full name so that distinct +// names never share a derived name due to truncation or sanitization. +func shortenDiskName(name string, budget int) string { + if len(name) <= budget && len(kvalidation.IsDNS1123Label(name)) == 0 { + return name + } + + h := fnv.New64a() + _, _ = h.Write([]byte(name)) + suffix := hex.EncodeToString(h.Sum(nil)) // 16 hex chars for a 64-bit hash + + readable := strings.Trim(sanitizeLabel(name), "-") + if maxReadable := budget - 1 - len(suffix); len(readable) > maxReadable { + readable = strings.TrimRight(readable[:maxReadable], "-") + } + if readable == "" { + return suffix + } + return readable + "-" + suffix +} + +// sanitizeLabel replaces every character that is not allowed in a DNS-1123 label +// with '-'. Resource names are already lowercase DNS subdomains, so in practice +// this only rewrites '.'. +func sanitizeLabel(s string) string { + var b strings.Builder + b.Grow(len(s)) + for _, r := range s { + switch { + case r >= 'a' && r <= 'z', r >= '0' && r <= '9', r == '-': + b.WriteRune(r) + default: + b.WriteByte('-') + } + } + return b.String() +} + func GetOriginalDiskName(prefixedName string) (string, v1alpha2.BlockDeviceKind) { switch { case strings.HasPrefix(prefixedName, VDDiskPrefix): diff --git a/images/virtualization-artifact/pkg/controller/kvbuilder/suite_test.go b/images/virtualization-artifact/pkg/controller/kvbuilder/suite_test.go new file mode 100644 index 0000000000..b51540d37d --- /dev/null +++ b/images/virtualization-artifact/pkg/controller/kvbuilder/suite_test.go @@ -0,0 +1,29 @@ +/* +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 kvbuilder + +import ( + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +func TestKVBuilder(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "KVBuilder Suite") +} From 816ef8d6aaba2733d70d19dac96773227c8d37fb Mon Sep 17 00:00:00 2001 From: Pavel Tishkov Date: Sat, 27 Jun 2026 18:34:36 +0300 Subject: [PATCH 2/8] refactor(api): resolve KubeVirt volume names without relying on prefix-strip Derived volume/disk names will be shortened/hashed for long resource names, so recovering the user resource from a volume name by stripping the vd-/vi-/cvi- prefix no longer works. Introduce kvbuilder.VolumeNameResolver, seeded with the resources a VM references (spec + VMBDA), which matches by forward-generating each candidate's volume name and falls back to prefix-strip for legacy names. - vm block-device status and hotplug handler now resolve via the seeded resolver; - VMBDA and VirtualDiskSnapshot KVVMI watchers forward-map candidates instead of reversing the volume name (the vdsnapshot watcher dropped a manual vd- strip); - extract GenerateVMBDADiskName and reuse it in VMBDA deletion. Behaviour is unchanged for existing short names. Signed-off-by: Pavel Tishkov --- .../controller/kvbuilder/kvvm_names_test.go | 30 +++++++++++ .../pkg/controller/kvbuilder/kvvm_utils.go | 50 +++++++++++++++++++ .../internal/watcher/kvvmi_watcher.go | 16 +++--- .../vm/internal/block_device_status.go | 17 ++++++- .../controller/vm/internal/hotplug_handler.go | 16 ++++-- .../pkg/controller/vmbda/internal/deletion.go | 10 +--- .../vmbda/internal/watcher/kvvmi_watcher.go | 16 +++--- 7 files changed, 126 insertions(+), 29 deletions(-) diff --git a/images/virtualization-artifact/pkg/controller/kvbuilder/kvvm_names_test.go b/images/virtualization-artifact/pkg/controller/kvbuilder/kvvm_names_test.go index 1d1dd8139c..5fa4141d4d 100644 --- a/images/virtualization-artifact/pkg/controller/kvbuilder/kvvm_names_test.go +++ b/images/virtualization-artifact/pkg/controller/kvbuilder/kvvm_names_test.go @@ -100,3 +100,33 @@ var _ = Describe("Derived KubeVirt disk/volume names", func() { Expect(kind).To(Equal(v1alpha2.DiskDevice)) }) }) + +var _ = Describe("VolumeNameResolver", func() { + It("reverses derived names via candidates, including hashed ones", func() { + longName := strings.Repeat("a", vdNameBudget+10) // forced into the hash branch + r := NewVolumeNameResolver() + r.Add(v1alpha2.DiskDevice, longName) + r.Add(v1alpha2.ImageDevice, "ubuntu") + + name, kind := r.Resolve(GenerateVDDiskName(longName)) + Expect(name).To(Equal(longName)) + Expect(kind).To(Equal(v1alpha2.DiskDevice)) + + name, kind = r.Resolve(GenerateVIDiskName("ubuntu")) + Expect(name).To(Equal("ubuntu")) + Expect(kind).To(Equal(v1alpha2.ImageDevice)) + }) + + It("falls back to prefix-strip for legacy names not among candidates", func() { + r := NewVolumeNameResolver() + name, kind := r.Resolve("vd-legacy-disk") + Expect(name).To(Equal("legacy-disk")) + Expect(kind).To(Equal(v1alpha2.DiskDevice)) + }) + + It("returns empty kind for non block-device volumes", func() { + r := NewVolumeNameResolver() + _, kind := r.Resolve("cloudinit") + Expect(kind).To(BeEmpty()) + }) +}) diff --git a/images/virtualization-artifact/pkg/controller/kvbuilder/kvvm_utils.go b/images/virtualization-artifact/pkg/controller/kvbuilder/kvvm_utils.go index b4e7c475e9..4ad26bdf54 100644 --- a/images/virtualization-artifact/pkg/controller/kvbuilder/kvvm_utils.go +++ b/images/virtualization-artifact/pkg/controller/kvbuilder/kvvm_utils.go @@ -88,6 +88,56 @@ func GenerateDiskName(kind v1alpha2.BlockDeviceKind, name string) string { return "" } +// GenerateVMBDADiskName returns the derived KubeVirt volume/disk name for a VMBDA +// block device reference. +func GenerateVMBDADiskName(ref v1alpha2.VMBDAObjectRef) string { + switch ref.Kind { + case v1alpha2.VMBDAObjectRefKindVirtualDisk: + return GenerateVDDiskName(ref.Name) + case v1alpha2.VMBDAObjectRefKindVirtualImage: + return GenerateVIDiskName(ref.Name) + case v1alpha2.VMBDAObjectRefKindClusterVirtualImage: + return GenerateCVIDiskName(ref.Name) + } + return "" +} + +type nameKind struct { + name string + kind v1alpha2.BlockDeviceKind +} + +// VolumeNameResolver reverses a derived KubeVirt volume/disk name back to the user +// resource it was generated from. Shortened/hashed names are not reversible by +// prefix-strip, so callers seed the resolver with the resources they already know +// (VM spec refs, VMBDA refs); the resolver matches by forward-generating each +// candidate's volume name. Names without a matching candidate fall back to +// prefix-strip, which stays correct for legacy names that were never shortened. +type VolumeNameResolver struct { + byVolumeName map[string]nameKind +} + +func NewVolumeNameResolver() *VolumeNameResolver { + return &VolumeNameResolver{byVolumeName: make(map[string]nameKind)} +} + +// Add registers a candidate user resource by its kind and name. +func (r *VolumeNameResolver) Add(kind v1alpha2.BlockDeviceKind, name string) { + if volumeName := GenerateDiskName(kind, name); volumeName != "" { + r.byVolumeName[volumeName] = nameKind{name: name, kind: kind} + } +} + +// Resolve returns the user resource (name, kind) for a derived volume name, or +// ("", "") for volumes that are not vd/vi/cvi block devices. It matches the +// seeded candidates first and falls back to prefix-strip for legacy short names. +func (r *VolumeNameResolver) Resolve(volumeName string) (string, v1alpha2.BlockDeviceKind) { + if nk, ok := r.byVolumeName[volumeName]; ok { + return nk.name, nk.kind + } + return GetOriginalDiskName(volumeName) +} + // shortenDiskName maps a user resource name onto the user-controlled part of a // KubeVirt volume/disk name within the given budget. // diff --git a/images/virtualization-artifact/pkg/controller/vdsnapshot/internal/watcher/kvvmi_watcher.go b/images/virtualization-artifact/pkg/controller/vdsnapshot/internal/watcher/kvvmi_watcher.go index 70330e48d2..8df2ade36b 100644 --- a/images/virtualization-artifact/pkg/controller/vdsnapshot/internal/watcher/kvvmi_watcher.go +++ b/images/virtualization-artifact/pkg/controller/vdsnapshot/internal/watcher/kvvmi_watcher.go @@ -20,7 +20,6 @@ import ( "context" "fmt" "log/slog" - "strings" "k8s.io/apimachinery/pkg/types" virtv1 "kubevirt.io/api/core/v1" @@ -34,6 +33,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/source" "github.com/deckhouse/virtualization-controller/pkg/common/annotations" + "github.com/deckhouse/virtualization-controller/pkg/controller/kvbuilder" "github.com/deckhouse/virtualization/api/core/v1alpha2" ) @@ -62,18 +62,15 @@ func (w KVVMIWatcher) Watch(mgr manager.Manager, ctr controller.Controller) erro } func (w KVVMIWatcher) enqueueRequests(ctx context.Context, kvvmi *virtv1.VirtualMachineInstance) (requests []reconcile.Request) { - volumeByName := make(map[string]struct{}) + volumeNames := make(map[string]struct{}) for _, v := range kvvmi.Status.VolumeStatus { if v.PersistentVolumeClaimInfo == nil { continue } - - if originalName, ok := strings.CutPrefix(v.Name, "vd-"); ok { - volumeByName[originalName] = struct{}{} - } + volumeNames[v.Name] = struct{}{} } - if len(volumeByName) == 0 { + if len(volumeNames) == 0 { return requests } @@ -87,7 +84,10 @@ func (w KVVMIWatcher) enqueueRequests(ctx context.Context, kvvmi *virtv1.Virtual } for _, vdSnapshot := range vdSnapshots.Items { - _, ok := volumeByName[vdSnapshot.Spec.VirtualDiskName] + // Match by the derived KubeVirt volume name instead of reversing it: + // the derivation is deterministic, so forward-mapping the VirtualDisk + // name handles shortened/hashed names that prefix-strip cannot reverse. + _, ok := volumeNames[kvbuilder.GenerateVDDiskName(vdSnapshot.Spec.VirtualDiskName)] if !ok { continue } diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/block_device_status.go b/images/virtualization-artifact/pkg/controller/vm/internal/block_device_status.go index 92410e28ed..bbb45030c5 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/block_device_status.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/block_device_status.go @@ -89,11 +89,26 @@ func (h *BlockDeviceHandler) getBlockDeviceStatusRefs(ctx context.Context, s sta } } + // Resolve derived KubeVirt volume names back to user resources. Shortened/ + // hashed names are not reversible by prefix-strip, so seed the resolver with + // the resources this VM references (spec + VMBDA-attached). + resolver := kvbuilder.NewVolumeNameResolver() + for _, bd := range specRefs { + resolver.Add(bd.Kind, bd.Name) + } + vmbdas, err := s.VirtualMachineBlockDeviceAttachments(ctx) + if err != nil { + return nil, err + } + for ref := range vmbdas { + resolver.Add(v1alpha2.BlockDeviceKind(ref.Kind), ref.Name) + } + attachedBlockDeviceRefs := make(map[nameKindKey]struct{}) // 2. The kvvm already exists: populate block device refs with the kvvm volumes. for _, volume := range kvvm.Spec.Template.Spec.Volumes { - bdName, kind := kvbuilder.GetOriginalDiskName(volume.Name) + bdName, kind := resolver.Resolve(volume.Name) if kind == "" { // Reflect only vi, vd, or cvi block devices in status. // This is neither of them, so skip. diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/hotplug_handler.go b/images/virtualization-artifact/pkg/controller/vm/internal/hotplug_handler.go index 8480646ff4..bbc15854ea 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/hotplug_handler.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/hotplug_handler.go @@ -89,7 +89,17 @@ func (h *HotplugHandler) Handle(ctx context.Context, s state.VirtualMachineState vmbdaDevices[nameKindKey{kind: v1alpha2.BlockDeviceKind(ref.Kind), name: ref.Name}] = struct{}{} } - kvvmDevices, pending := parseKVVMVolumes(kvvm) + // Seed the resolver with the resources this VM references so derived (possibly + // shortened/hashed) KubeVirt volume names map back to the right user resource. + resolver := kvbuilder.NewVolumeNameResolver() + for _, bd := range current.Spec.BlockDeviceRefs { + resolver.Add(bd.Kind, bd.Name) + } + for ref := range vmbdaMap { + resolver.Add(v1alpha2.BlockDeviceKind(ref.Kind), ref.Name) + } + + kvvmDevices, pending := parseKVVMVolumes(kvvm, resolver) var errs []error @@ -152,13 +162,13 @@ type kvvmVolume struct { hotpluggable bool } -func parseKVVMVolumes(kvvm *virtv1.VirtualMachine) (map[nameKindKey]kvvmVolume, map[string]struct{}) { +func parseKVVMVolumes(kvvm *virtv1.VirtualMachine, resolver *kvbuilder.VolumeNameResolver) (map[nameKindKey]kvvmVolume, map[string]struct{}) { devices := make(map[nameKindKey]kvvmVolume) pending := make(map[string]struct{}) if kvvm.Spec.Template != nil { for _, vol := range kvvm.Spec.Template.Spec.Volumes { - name, kind := kvbuilder.GetOriginalDiskName(vol.Name) + name, kind := resolver.Resolve(vol.Name) if kind == "" { continue } diff --git a/images/virtualization-artifact/pkg/controller/vmbda/internal/deletion.go b/images/virtualization-artifact/pkg/controller/vmbda/internal/deletion.go index de2f80940f..30c5fb1f50 100644 --- a/images/virtualization-artifact/pkg/controller/vmbda/internal/deletion.go +++ b/images/virtualization-artifact/pkg/controller/vmbda/internal/deletion.go @@ -90,15 +90,7 @@ func (h *DeletionHandler) detach(ctx context.Context, kvvm *virtv1.VirtualMachin return reconcile.Result{}, errors.New("intvirtvm not found to unplug") } - var blockDeviceName string - switch vmbda.Spec.BlockDeviceRef.Kind { - case v1alpha2.VMBDAObjectRefKindVirtualDisk: - blockDeviceName = kvbuilder.GenerateVDDiskName(vmbda.Spec.BlockDeviceRef.Name) - case v1alpha2.VMBDAObjectRefKindVirtualImage: - blockDeviceName = kvbuilder.GenerateVIDiskName(vmbda.Spec.BlockDeviceRef.Name) - case v1alpha2.VMBDAObjectRefKindClusterVirtualImage: - blockDeviceName = kvbuilder.GenerateCVIDiskName(vmbda.Spec.BlockDeviceRef.Name) - } + blockDeviceName := kvbuilder.GenerateVMBDADiskName(vmbda.Spec.BlockDeviceRef) log := logger.FromContext(ctx).With(logger.SlogHandler(deletionHandlerName)) log.Info("Unplug block device", slog.String("blockDeviceName", blockDeviceName), slog.String("vm", kvvm.Name)) diff --git a/images/virtualization-artifact/pkg/controller/vmbda/internal/watcher/kvvmi_watcher.go b/images/virtualization-artifact/pkg/controller/vmbda/internal/watcher/kvvmi_watcher.go index 3b735580bc..e886b66058 100644 --- a/images/virtualization-artifact/pkg/controller/vmbda/internal/watcher/kvvmi_watcher.go +++ b/images/virtualization-artifact/pkg/controller/vmbda/internal/watcher/kvvmi_watcher.go @@ -103,7 +103,10 @@ func (eh KVVMIEventHandler) enqueueRequests(ctx context.Context, ns string, vsTo } for _, vmbda := range vmbdas.Items { - _, ok := vsToReconcile[vmbda.Spec.BlockDeviceRef.Name] + // Match by the derived KubeVirt volume name: the derivation is + // deterministic, so forward-mapping the VMBDA reference handles + // shortened/hashed names that prefix-strip cannot reverse. + _, ok := vsToReconcile[kvbuilder.GenerateVMBDADiskName(vmbda.Spec.BlockDeviceRef)] if ok { q.Add(reconcile.Request{NamespacedName: types.NamespacedName{ Namespace: vmbda.Namespace, @@ -123,13 +126,10 @@ func (eh KVVMIEventHandler) getHotPluggedVolumeStatuses(obj client.Object) map[s for _, vs := range kvvmi.Status.VolumeStatus { if vs.HotplugVolume != nil { - name, kind := kvbuilder.GetOriginalDiskName(vs.Name) - if kind == "" { - slog.Default().Warn("VolumeStatus is not a Disk", "vsName", vs.Name, "name", kvvmi.Name, "ns", kvvmi.Namespace) - continue - } - - volumeStatuses[name] = vs + // Key by the KubeVirt volume name; matching happens by forward-mapping + // VMBDA references in enqueueRequests (reverse is not derivable for + // shortened/hashed names). + volumeStatuses[vs.Name] = vs } } From b19e1fe949dbe3a730fae0d4f94ab11b08f931b5 Mon Sep 17 00:00:00 2001 From: Pavel Tishkov Date: Sat, 27 Jun 2026 18:37:48 +0300 Subject: [PATCH 3/8] feat(api): fail loudly on derived disk-name collisions The internal disk name is derived from the resource name; SetDisk replaces an existing disk/volume by that name, so if two distinct block devices ever derived the same internal name one would silently overwrite the other (dropping a disk / mounting the wrong PVC). The derivation uses a 64-bit hash so a collision is astronomically unlikely, but detect it at build time and fail with a clear, user-facing message instead of corrupting the VM. The error names the two conflicting resources in full and asks to recreate one with a different name (resource names are immutable, so renaming is not an option). Signed-off-by: Pavel Tishkov --- .../controller/kvbuilder/kvvm_names_test.go | 33 ++++++++++++++++ .../pkg/controller/kvbuilder/kvvm_utils.go | 39 +++++++++++++++++++ 2 files changed, 72 insertions(+) diff --git a/images/virtualization-artifact/pkg/controller/kvbuilder/kvvm_names_test.go b/images/virtualization-artifact/pkg/controller/kvbuilder/kvvm_names_test.go index 5fa4141d4d..ea03ecec07 100644 --- a/images/virtualization-artifact/pkg/controller/kvbuilder/kvvm_names_test.go +++ b/images/virtualization-artifact/pkg/controller/kvbuilder/kvvm_names_test.go @@ -130,3 +130,36 @@ var _ = Describe("VolumeNameResolver", func() { Expect(kind).To(BeEmpty()) }) }) + +var _ = Describe("detectDiskNameCollisions", func() { + It("passes for distinct block devices (incl. long and dotted names)", func() { + vm := &v1alpha2.VirtualMachine{ + Spec: v1alpha2.VirtualMachineSpec{ + BlockDeviceRefs: []v1alpha2.BlockDeviceSpecRef{ + {Kind: v1alpha2.DiskDevice, Name: "data"}, + {Kind: v1alpha2.DiskDevice, Name: strings.Repeat("a", 80)}, + {Kind: v1alpha2.ImageDevice, Name: "ubuntu"}, + {Kind: v1alpha2.DiskDevice, Name: "disk.with.dots"}, + }, + }, + } + vmbda := map[v1alpha2.VMBDAObjectRef][]*v1alpha2.VirtualMachineBlockDeviceAttachment{ + {Kind: v1alpha2.VMBDAObjectRefKindVirtualDisk, Name: "extra"}: nil, + } + Expect(detectDiskNameCollisions(vm, vmbda)).To(Succeed()) + }) + + It("does not flag the same resource present in both spec and VMBDA", func() { + vm := &v1alpha2.VirtualMachine{ + Spec: v1alpha2.VirtualMachineSpec{ + BlockDeviceRefs: []v1alpha2.BlockDeviceSpecRef{ + {Kind: v1alpha2.DiskDevice, Name: "shared"}, + }, + }, + } + vmbda := map[v1alpha2.VMBDAObjectRef][]*v1alpha2.VirtualMachineBlockDeviceAttachment{ + {Kind: v1alpha2.VMBDAObjectRefKindVirtualDisk, Name: "shared"}: nil, + } + Expect(detectDiskNameCollisions(vm, vmbda)).To(Succeed()) + }) +}) diff --git a/images/virtualization-artifact/pkg/controller/kvbuilder/kvvm_utils.go b/images/virtualization-artifact/pkg/controller/kvbuilder/kvvm_utils.go index 4ad26bdf54..f0e421d326 100644 --- a/images/virtualization-artifact/pkg/controller/kvbuilder/kvvm_utils.go +++ b/images/virtualization-artifact/pkg/controller/kvbuilder/kvvm_utils.go @@ -284,6 +284,14 @@ func applyBlockDeviceRefs( cviByName map[string]*v1alpha2.ClusterVirtualImage, vmbdaByBlockDeviceRef map[v1alpha2.VMBDAObjectRef][]*v1alpha2.VirtualMachineBlockDeviceAttachment, ) error { + // Backstop against a derived-name collision. The derivation is collision- + // resistant (64-bit hash), so this is astronomically unlikely, but SetDisk + // replaces an existing disk/volume by name, so a silent collision would drop + // one disk and mount another's PVC. Fail loudly instead of corrupting the VM. + if err := detectDiskNameCollisions(vm, vmbdaByBlockDeviceRef); err != nil { + return err + } + isParavirtualizationEnabled := vm.Spec.IsParavirtualizationEnabled() hasExplicitBootOrder := false @@ -345,6 +353,37 @@ func applyBlockDeviceRefs( return nil } +// detectDiskNameCollisions returns an error if two distinct block devices of this +// VM (spec refs or VMBDA-attached) derive the same KubeVirt volume/disk name. +func detectDiskNameCollisions(vm *v1alpha2.VirtualMachine, vmbdaByBlockDeviceRef map[v1alpha2.VMBDAObjectRef][]*v1alpha2.VirtualMachineBlockDeviceAttachment) error { + owners := make(map[string]nameKind) + check := func(kind v1alpha2.BlockDeviceKind, name string) error { + diskName := GenerateDiskName(kind, name) + if diskName == "" { + return nil + } + cur := nameKind{name: name, kind: kind} + if prev, ok := owners[diskName]; ok && prev != cur { + return fmt.Errorf("%s %q and %s %q resolve to the same internal disk name and cannot be attached to the same virtual machine; recreate one of them with a different name", + prev.kind, prev.name, kind, name) + } + owners[diskName] = cur + return nil + } + + for _, bd := range vm.Spec.BlockDeviceRefs { + if err := check(bd.Kind, bd.Name); err != nil { + return err + } + } + for ref := range vmbdaByBlockDeviceRef { + if err := check(v1alpha2.BlockDeviceKind(ref.Kind), ref.Name); err != nil { + return err + } + } + return nil +} + func cleanupRemovedStaticDisks(kvvm *KVVM, specDiskNames, hotpluggableVolumes, vmbdaDiskNames map[string]struct{}, isVmRunning bool) { // Disks attached via VMBDA should not be removed from KVVM spec even when VM is stopped. // If VMBDA exists, the disk is associated with this VM - VMBDA controller will From 2b66b6f932101ec249b276eea7adc43aaf6f5064 Mon Sep 17 00:00:00 2001 From: Pavel Tishkov Date: Sat, 27 Jun 2026 18:46:29 +0300 Subject: [PATCH 4/8] feat(api): allow VirtualDisk/Image names up to the Kubernetes name limit The tight VirtualDisk/VirtualImage/ClusterVirtualImage name limits (60/49/48) existed only because the name was used verbatim as the KubeVirt volume/disk name (a DNS-1123 label). That name is now derived and shortened independently, so the user-facing name may use the full Kubernetes DNS subdomain length (253). - raise the create limits to 253 (VirtualMachine stays 63: its name still flows into KubeVirt pod names and label values, to be addressed separately); - drop the per-reference length check in the VM block-device validator: it only duplicated limits the referenced resource is already bound by. Signed-off-by: Pavel Tishkov --- .../pkg/common/validate/validate.go | 24 +++++++++++-------- .../validators/block_device_refs_validator.go | 21 +++------------- .../block_device_refs_validator_test.go | 18 -------------- 3 files changed, 17 insertions(+), 46 deletions(-) diff --git a/images/virtualization-artifact/pkg/common/validate/validate.go b/images/virtualization-artifact/pkg/common/validate/validate.go index bd008874e0..f56b501324 100644 --- a/images/virtualization-artifact/pkg/common/validate/validate.go +++ b/images/virtualization-artifact/pkg/common/validate/validate.go @@ -16,21 +16,25 @@ limitations under the License. package validate +import kvalidation "k8s.io/apimachinery/pkg/util/validation" + +// The underlying KubeVirt volume/disk name (and the container it may become) must +// be a valid DNS-1123 label (<=63). That name is now derived independently and +// shortened to fit (see kvbuilder.GenerateDiskName), so the user-facing name is no +// longer constrained by it and may use the full Kubernetes DNS subdomain length. + // MaxDiskNameLen determines the max len of vd. -// Disk and volume name in kubevirt can be a valid container name (len 63) since disk name can become a container name which will fail to schedule if invalid. -// We add prefix "vd-" for the vd name, so max len reduced to 60. -const MaxDiskNameLen = 60 +const MaxDiskNameLen = kvalidation.DNS1123SubdomainMaxLength // MaxVirtualImageNameLen determines the max len of vi. -// Disk and volume name in kubevirt can be a valid container name (len 63) since disk name can become a container name which will fail to schedule if invalid. -// We and kubevirt add prefixes "vi-", "volume" and suffix "-init", so max len reduced to 49. -const MaxVirtualImageNameLen = 49 +const MaxVirtualImageNameLen = kvalidation.DNS1123SubdomainMaxLength // MaxClusterVirtualImageNameLen determines the max len of cvi. -// Disk and volume name in kubevirt can be a valid container name (len 63) since disk name can become a container name which will fail to schedule if invalid. -// We and kubevirt add prefixes "cvi-", "volume" and suffix "-init", so max len reduced to 48. -const MaxClusterVirtualImageNameLen = 48 +const MaxClusterVirtualImageNameLen = kvalidation.DNS1123SubdomainMaxLength // MaxVirtualMachineNameLen determines the max len of vm. -// The limitation is reportedly associated with the PodDisruptionBudget resource, which has a label containing the virtual machine's name, and the label's value cannot exceed 63 characters. +// Unlike disks/images, a VirtualMachine name is not decoupled yet: it still flows +// into KubeVirt pod names (e.g. the launcher pod "d8v-vm--...") and label +// values that cap at 63. Raising it requires changes in the KubeVirt fork, so it +// stays limited for now. const MaxVirtualMachineNameLen = 63 diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/validators/block_device_refs_validator.go b/images/virtualization-artifact/pkg/controller/vm/internal/validators/block_device_refs_validator.go index 2b489e44f6..affe733e93 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/validators/block_device_refs_validator.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/validators/block_device_refs_validator.go @@ -22,7 +22,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/webhook/admission" - "github.com/deckhouse/virtualization-controller/pkg/common/validate" "github.com/deckhouse/virtualization/api/core/v1alpha2" ) @@ -46,23 +45,9 @@ func (v *BlockDeviceSpecRefsValidator) validate(vm *v1alpha2.VirtualMachine) err return err } - for _, bdRef := range vm.Spec.BlockDeviceRefs { - var maxLen int - switch bdRef.Kind { - case v1alpha2.DiskDevice: - maxLen = validate.MaxDiskNameLen - case v1alpha2.ImageDevice: - maxLen = validate.MaxVirtualImageNameLen - case v1alpha2.ClusterImageDevice: - maxLen = validate.MaxClusterVirtualImageNameLen - default: - continue - } - - if len(bdRef.Name) > maxLen { - return fmt.Errorf("%s name %q is too long: it must be no more than %d characters", bdRef.Kind, bdRef.Name, maxLen) - } - } + // The referenced resource's name is validated by its own webhook and bounded + // by Kubernetes; a reference longer than that simply cannot match any existing + // resource (the VM stays Pending), so no length check is needed here. return nil } diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/validators/block_device_refs_validator_test.go b/images/virtualization-artifact/pkg/controller/vm/internal/validators/block_device_refs_validator_test.go index 901b14ee38..795529ceea 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/validators/block_device_refs_validator_test.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/validators/block_device_refs_validator_test.go @@ -18,7 +18,6 @@ package validators_test import ( "context" - "fmt" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -59,23 +58,6 @@ var _ = Describe("BlockDeviceSpecRefsValidator", func() { }), ) - DescribeTable("ValidateCreate with invalid name length", func(kind v1alpha2.BlockDeviceKind, name string, maxLen int) { - vm := &v1alpha2.VirtualMachine{ - Spec: v1alpha2.VirtualMachineSpec{ - BlockDeviceRefs: []v1alpha2.BlockDeviceSpecRef{ - {Kind: kind, Name: name}, - }, - }, - } - _, err := validator.ValidateCreate(context.Background(), vm) - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring(fmt.Sprintf("too long: it must be no more than %d characters", maxLen))) - }, - Entry("VirtualDisk too long", v1alpha2.DiskDevice, string(make([]byte, 61)), 60), - Entry("VirtualImage too long", v1alpha2.ImageDevice, string(make([]byte, 50)), 49), - Entry("ClusterVirtualImage too long", v1alpha2.ClusterImageDevice, string(make([]byte, 49)), 48), - ) - DescribeTable("ValidateCreate with duplicates", func(refs []v1alpha2.BlockDeviceSpecRef) { vm := &v1alpha2.VirtualMachine{ Spec: v1alpha2.VirtualMachineSpec{ From f22ad84c2bffad2ff3aa17fbc0f3f32b2fa7da1f Mon Sep 17 00:00:00 2001 From: Pavel Tishkov Date: Sat, 27 Jun 2026 19:00:03 +0300 Subject: [PATCH 5/8] docs(api): drop obsolete VirtualDisk/Image name-length notes The 60/49/48-character limits no longer apply: these names may now use the full Kubernetes name length. Update the type doc comments and regenerate CRDs (and the Russian docs). Signed-off-by: Pavel Tishkov --- api/core/v1alpha2/cluster_virtual_image.go | 2 +- api/core/v1alpha2/virtual_disk.go | 2 +- api/core/v1alpha2/virtual_image.go | 2 +- crds/clustervirtualimages.yaml | 2 +- crds/doc-ru-clustervirtualimages.yaml | 2 +- crds/doc-ru-virtualdisks.yaml | 2 +- crds/doc-ru-virtualimages.yaml | 2 +- crds/virtualdisks.yaml | 2 +- crds/virtualimages.yaml | 2 +- 9 files changed, 9 insertions(+), 9 deletions(-) diff --git a/api/core/v1alpha2/cluster_virtual_image.go b/api/core/v1alpha2/cluster_virtual_image.go index dda945c29e..b4dcf9b605 100644 --- a/api/core/v1alpha2/cluster_virtual_image.go +++ b/api/core/v1alpha2/cluster_virtual_image.go @@ -32,7 +32,7 @@ const ( // // With this resource in the cluster, a container image is created and stored in a dedicated Deckhouse Virtualization Container Registry (DVCR). // -// **Note:** The `metadata.name` field must comply with [Kubernetes object naming conventions](https://kubernetes.io/docs/concepts/overview/working-with-objects/names/) and must not exceed 48 characters. +// **Note:** The `metadata.name` field must comply with [Kubernetes object naming conventions](https://kubernetes.io/docs/concepts/overview/working-with-objects/names/). // +kubebuilder:object:root=true // +kubebuilder:metadata:labels={heritage=deckhouse,module=virtualization,backup.deckhouse.io/cluster-config=true} // +kubebuilder:resource:categories={virtualization-cluster},scope=Cluster,shortName={cvi},singular=clustervirtualimage diff --git a/api/core/v1alpha2/virtual_disk.go b/api/core/v1alpha2/virtual_disk.go index 71a4c6022b..51166f2415 100644 --- a/api/core/v1alpha2/virtual_disk.go +++ b/api/core/v1alpha2/virtual_disk.go @@ -31,7 +31,7 @@ const ( // // Once a VirtualDisk is created, the following fields in `.spec.persistentVolumeClaim` can be changed: `size` and `storageClassName`. All other fields are immutable. // -// **Note:** The `metadata.name` field must comply with [Kubernetes object naming conventions](https://kubernetes.io/docs/concepts/overview/working-with-objects/names/) and must not exceed 60 characters. +// **Note:** The `metadata.name` field must comply with [Kubernetes object naming conventions](https://kubernetes.io/docs/concepts/overview/working-with-objects/names/). // +kubebuilder:object:root=true // +kubebuilder:metadata:labels={heritage=deckhouse,module=virtualization} // +kubebuilder:resource:categories={virtualization},scope=Namespaced,shortName={vd},singular=virtualdisk diff --git a/api/core/v1alpha2/virtual_image.go b/api/core/v1alpha2/virtual_image.go index ecb335d406..019fa4a93f 100644 --- a/api/core/v1alpha2/virtual_image.go +++ b/api/core/v1alpha2/virtual_image.go @@ -32,7 +32,7 @@ const ( // // With this resource in the cluster, a container image is created and stored in a dedicated Deckhouse Virtualization Container Registry (DVCR) or PVC, with the data filled in from the source. // -// **Note:** The `metadata.name` field must comply with [Kubernetes object naming conventions](https://kubernetes.io/docs/concepts/overview/working-with-objects/names/) and must not exceed 49 characters. +// **Note:** The `metadata.name` field must comply with [Kubernetes object naming conventions](https://kubernetes.io/docs/concepts/overview/working-with-objects/names/). // +genclient // +kubebuilder:object:root=true // +kubebuilder:metadata:labels={heritage=deckhouse,module=virtualization} diff --git a/crds/clustervirtualimages.yaml b/crds/clustervirtualimages.yaml index 6b3b6ce8bd..edc0e826cd 100644 --- a/crds/clustervirtualimages.yaml +++ b/crds/clustervirtualimages.yaml @@ -57,7 +57,7 @@ spec: With this resource in the cluster, a container image is created and stored in a dedicated Deckhouse Virtualization Container Registry (DVCR). - **Note:** The `metadata.name` field must comply with [Kubernetes object naming conventions](https://kubernetes.io/docs/concepts/overview/working-with-objects/names/) and must not exceed 48 characters. + **Note:** The `metadata.name` field must comply with [Kubernetes object naming conventions](https://kubernetes.io/docs/concepts/overview/working-with-objects/names/). properties: apiVersion: description: |- diff --git a/crds/doc-ru-clustervirtualimages.yaml b/crds/doc-ru-clustervirtualimages.yaml index dbaa763c2f..98950e0166 100644 --- a/crds/doc-ru-clustervirtualimages.yaml +++ b/crds/doc-ru-clustervirtualimages.yaml @@ -10,7 +10,7 @@ spec: После появления в кластере этого ресурса создаётся образ контейнера, который хранится в специальном реестре контейнеров Deckhouse Virtualization Container Registry (DVCR). - **Важно:** Поле `metadata.name` должно соответствовать [правилам именования объектов Kubernetes](https://kubernetes.io/docs/concepts/overview/working-with-objects/names/) и не должно превышать 48 символов. + **Важно:** Поле `metadata.name` должно соответствовать [правилам именования объектов Kubernetes](https://kubernetes.io/docs/concepts/overview/working-with-objects/names/). properties: spec: properties: diff --git a/crds/doc-ru-virtualdisks.yaml b/crds/doc-ru-virtualdisks.yaml index c24e41c64d..7adc0a292f 100644 --- a/crds/doc-ru-virtualdisks.yaml +++ b/crds/doc-ru-virtualdisks.yaml @@ -8,7 +8,7 @@ spec: После создания VirtualDisk в `.spec.persistentVolumeClaim` можно изменить поля `size` и `storageClassName`. Все остальные поля неизменяемы. - **Важно:** Поле `metadata.name` должно соответствовать [правилам именования объектов Kubernetes](https://kubernetes.io/docs/concepts/overview/working-with-objects/names/) и не должно превышать 60 символов. + **Важно:** Поле `metadata.name` должно соответствовать [правилам именования объектов Kubernetes](https://kubernetes.io/docs/concepts/overview/working-with-objects/names/). properties: spec: properties: diff --git a/crds/doc-ru-virtualimages.yaml b/crds/doc-ru-virtualimages.yaml index 56457c9f17..8c4d5f4141 100644 --- a/crds/doc-ru-virtualimages.yaml +++ b/crds/doc-ru-virtualimages.yaml @@ -10,7 +10,7 @@ spec: После появления в кластере этого ресурса создаётся образ контейнера, который хранится в специальном реестре контейнеров Deckhouse Virtualization Container Registry (DVCR). - **Важно:** Поле `metadata.name` должно соответствовать [правилам именования объектов Kubernetes](https://kubernetes.io/docs/concepts/overview/working-with-objects/names/) и не должно превышать 49 символов. + **Важно:** Поле `metadata.name` должно соответствовать [правилам именования объектов Kubernetes](https://kubernetes.io/docs/concepts/overview/working-with-objects/names/). properties: spec: properties: diff --git a/crds/virtualdisks.yaml b/crds/virtualdisks.yaml index 01eb3e42ed..4ab4131f54 100644 --- a/crds/virtualdisks.yaml +++ b/crds/virtualdisks.yaml @@ -59,7 +59,7 @@ spec: Once a VirtualDisk is created, the following fields in `.spec.persistentVolumeClaim` can be changed: `size` and `storageClassName`. All other fields are immutable. - **Note:** The `metadata.name` field must comply with [Kubernetes object naming conventions](https://kubernetes.io/docs/concepts/overview/working-with-objects/names/) and must not exceed 60 characters. + **Note:** The `metadata.name` field must comply with [Kubernetes object naming conventions](https://kubernetes.io/docs/concepts/overview/working-with-objects/names/). properties: apiVersion: description: |- diff --git a/crds/virtualimages.yaml b/crds/virtualimages.yaml index 85ea37cae8..bab31ec915 100644 --- a/crds/virtualimages.yaml +++ b/crds/virtualimages.yaml @@ -60,7 +60,7 @@ spec: With this resource in the cluster, a container image is created and stored in a dedicated Deckhouse Virtualization Container Registry (DVCR) or PVC, with the data filled in from the source. - **Note:** The `metadata.name` field must comply with [Kubernetes object naming conventions](https://kubernetes.io/docs/concepts/overview/working-with-objects/names/) and must not exceed 49 characters. + **Note:** The `metadata.name` field must comply with [Kubernetes object naming conventions](https://kubernetes.io/docs/concepts/overview/working-with-objects/names/). properties: apiVersion: description: |- From 4b94c155213d7b15f58d8b44240bd923e0a224e1 Mon Sep 17 00:00:00 2001 From: Pavel Tishkov Date: Sat, 27 Jun 2026 19:09:42 +0300 Subject: [PATCH 6/8] test(e2e): exercise long disk names in existing lifecycle suites Reuse the existing builder-based e2e tests to validate end-to-end that VirtualDisk names up to the Kubernetes name length work: disk hotplug attach/detach, snapshotting a running VM's disk, and live-migration via eviction now use disk names longer than the former 60-character limit. VirtualMachine names stay short, as that limit is unchanged. Signed-off-by: Pavel Tishkov --- test/e2e/blockdevice/vd_snapshots.go | 5 ++++- test/e2e/vm/disk_attachment.go | 11 +++++++++-- test/e2e/vm/evacuation.go | 9 +++++++-- 3 files changed, 20 insertions(+), 5 deletions(-) diff --git a/test/e2e/blockdevice/vd_snapshots.go b/test/e2e/blockdevice/vd_snapshots.go index ad54fa99ac..d86b7f95b5 100644 --- a/test/e2e/blockdevice/vd_snapshots.go +++ b/test/e2e/blockdevice/vd_snapshots.go @@ -19,6 +19,7 @@ package blockdevice import ( "context" "fmt" + "strings" "sync" "time" @@ -63,7 +64,9 @@ var _ = Describe("VirtualDiskSnapshots", Label(precheck.PrecheckImmediateStorage DeferCleanup(f.After) By("Environment preparation") - vd := object.NewVDFromCVI("vd", f.Namespace().Name, object.PrecreatedCVIUbuntu) + // Long disk name (>60 chars, the former limit) to exercise snapshotting a + // disk whose name uses the full Kubernetes name length. + vd := object.NewVDFromCVI("vd-"+strings.Repeat("a", 80), f.Namespace().Name, object.PrecreatedCVIUbuntu) vm := object.NewMinimalVM("vm-", f.Namespace().Name, vmbuilder.WithName("vm"), vmbuilder.WithBlockDeviceRefs(v1alpha2.BlockDeviceSpecRef{ diff --git a/test/e2e/vm/disk_attachment.go b/test/e2e/vm/disk_attachment.go index 8293c0fc73..d243bfb5b0 100644 --- a/test/e2e/vm/disk_attachment.go +++ b/test/e2e/vm/disk_attachment.go @@ -18,6 +18,7 @@ package vm import ( "context" + "strings" "time" . "github.com/onsi/ginkgo/v2" @@ -58,14 +59,20 @@ var _ = Describe("DiskAttachment", Label(precheck.NoPrecheck), func() { It("attaches and detaches a virtual disk to a running VM", func() { By("Create test resources", func() { + // Use names longer than the former 60-character VirtualDisk limit to + // exercise end-to-end that disk names up to the Kubernetes name length + // work through VM start and disk hotplug; the underlying KubeVirt volume + // name is derived and shortened internally. + longName := func(base string) string { return base + "-" + strings.Repeat("a", 80) } + // Create VD from CVI for VM root disk - vdRoot = object.NewVDFromCVI("vd-root", f.Namespace().Name, object.PrecreatedCVIAlpineBIOS, + vdRoot = object.NewVDFromCVI(longName("vd-root"), f.Namespace().Name, object.PrecreatedCVIAlpineBIOS, vdbuilder.WithSize(ptr.To(resource.MustParse("512Mi"))), ) // Create blank VD without consumer (for attachment test) vdBlank = vdbuilder.New( - vdbuilder.WithName("vd-blank"), + vdbuilder.WithName(longName("vd-blank")), vdbuilder.WithNamespace(f.Namespace().Name), vdbuilder.WithPersistentVolumeClaim(nil, ptr.To(resource.MustParse("100Mi"))), ) diff --git a/test/e2e/vm/evacuation.go b/test/e2e/vm/evacuation.go index 73049fbefd..2eb1f1e690 100644 --- a/test/e2e/vm/evacuation.go +++ b/test/e2e/vm/evacuation.go @@ -18,6 +18,7 @@ package vm import ( "context" + "strings" "time" . "github.com/onsi/ginkgo/v2" @@ -113,15 +114,19 @@ func newEvacuationVM(name, namespace, cviName string, bootloader v1alpha2.Bootlo *v1alpha2.VirtualDisk, *v1alpha2.VirtualDisk, ) { + // Long disk names (>60 chars, the former limit) to exercise live-migrating a VM + // whose disks use the full Kubernetes name length. The VM name itself stays + // short, as VirtualMachine names remain limited. + longSuffix := "-" + strings.Repeat("a", 80) vdRoot := object.NewVDFromCVI( - name+"-root", + name+"-root"+longSuffix, namespace, cviName, vdbuilder.WithSize(ptr.To(resource.MustParse("350Mi"))), ) vdBlank := object.NewBlankVD( - name+"-blank", + name+"-blank"+longSuffix, namespace, nil, ptr.To(resource.MustParse("100Mi")), From 5e355048acc6168b472619fb74660a9ed1ef0410 Mon Sep 17 00:00:00 2001 From: Pavel Tishkov Date: Sat, 27 Jun 2026 20:21:21 +0300 Subject: [PATCH 7/8] fix(api): remove unused diskNameHashLen constant The hash suffix length is taken from the generated value directly, so the constant was unused and tripped the linter. Signed-off-by: Pavel Tishkov --- .../pkg/controller/kvbuilder/kvvm_utils.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/images/virtualization-artifact/pkg/controller/kvbuilder/kvvm_utils.go b/images/virtualization-artifact/pkg/controller/kvbuilder/kvvm_utils.go index f0e421d326..1a8d899961 100644 --- a/images/virtualization-artifact/pkg/controller/kvbuilder/kvvm_utils.go +++ b/images/virtualization-artifact/pkg/controller/kvbuilder/kvvm_utils.go @@ -60,10 +60,6 @@ const ( cviNameBudget = 48 ) -// diskNameHashLen is the length in hex chars of the FNV-1a 64-bit suffix appended -// when the user name does not fit the budget (or is not a valid DNS-1123 label). -const diskNameHashLen = 16 - func GenerateVDDiskName(name string) string { return VDDiskPrefix + shortenDiskName(name, vdNameBudget) } From 7e1b29c32c8955479bc1fb1c9e77a6ec4f458403 Mon Sep 17 00:00:00 2001 From: Pavel Tishkov Date: Sat, 27 Jun 2026 21:18:02 +0300 Subject: [PATCH 8/8] refactor(api): drop redundant VD/VI/CVI length checks; unit-test DVP-specific name rules The >253 length check for VirtualDisk/VirtualImage/ClusterVirtualImage only duplicated the Kubernetes metadata.name limit (the apiserver rejects >253 first), so it is removed along with the now-unused constants; restore falls back to the 253 default for these kinds. VirtualMachine keeps its DVP-specific 63 limit. Add unit tests for the rules DVP actually owns (Kubernetes does not enforce them): VirtualMachine name length (63 ok / 64 rejected) and the dot ban for VirtualDisk. Signed-off-by: Pavel Tishkov --- .../pkg/common/validate/validate.go | 27 +++------- .../pkg/controller/cvi/cvi_webhook.go | 9 ---- .../service/restorer/common/common.go | 10 ++-- .../vd/internal/validator/name_validator.go | 11 +--- .../internal/validator/name_validator_test.go | 53 +++++++++++++++++++ .../pkg/controller/vi/vi_webhook.go | 9 ---- .../validators/meta_validator_test.go | 50 +++++++++++++++++ 7 files changed, 116 insertions(+), 53 deletions(-) create mode 100644 images/virtualization-artifact/pkg/controller/vd/internal/validator/name_validator_test.go create mode 100644 images/virtualization-artifact/pkg/controller/vm/internal/validators/meta_validator_test.go diff --git a/images/virtualization-artifact/pkg/common/validate/validate.go b/images/virtualization-artifact/pkg/common/validate/validate.go index f56b501324..fdcb268a29 100644 --- a/images/virtualization-artifact/pkg/common/validate/validate.go +++ b/images/virtualization-artifact/pkg/common/validate/validate.go @@ -16,25 +16,14 @@ limitations under the License. package validate -import kvalidation "k8s.io/apimachinery/pkg/util/validation" - -// The underlying KubeVirt volume/disk name (and the container it may become) must -// be a valid DNS-1123 label (<=63). That name is now derived independently and -// shortened to fit (see kvbuilder.GenerateDiskName), so the user-facing name is no -// longer constrained by it and may use the full Kubernetes DNS subdomain length. - -// MaxDiskNameLen determines the max len of vd. -const MaxDiskNameLen = kvalidation.DNS1123SubdomainMaxLength - -// MaxVirtualImageNameLen determines the max len of vi. -const MaxVirtualImageNameLen = kvalidation.DNS1123SubdomainMaxLength - -// MaxClusterVirtualImageNameLen determines the max len of cvi. -const MaxClusterVirtualImageNameLen = kvalidation.DNS1123SubdomainMaxLength +// VirtualDisk/VirtualImage/ClusterVirtualImage names are not length-limited by DVP: +// the derived KubeVirt volume/disk name is shortened independently (see +// kvbuilder.GenerateDiskName), and the overall name is already bounded by Kubernetes +// (DNS subdomain, <=253). Only VirtualMachine keeps a DVP limit. // MaxVirtualMachineNameLen determines the max len of vm. -// Unlike disks/images, a VirtualMachine name is not decoupled yet: it still flows -// into KubeVirt pod names (e.g. the launcher pod "d8v-vm--...") and label -// values that cap at 63. Raising it requires changes in the KubeVirt fork, so it -// stays limited for now. +// Unlike disks/images, a VirtualMachine name is not decoupled: it flows into KubeVirt +// pod names (e.g. the launcher pod "d8v-vm--...") and label values that cap at +// 63 (Kubernetes does not enforce this, so DVP must). Raising it requires changes in +// the KubeVirt fork. const MaxVirtualMachineNameLen = 63 diff --git a/images/virtualization-artifact/pkg/controller/cvi/cvi_webhook.go b/images/virtualization-artifact/pkg/controller/cvi/cvi_webhook.go index f85200f208..3a2d7ff152 100644 --- a/images/virtualization-artifact/pkg/controller/cvi/cvi_webhook.go +++ b/images/virtualization-artifact/pkg/controller/cvi/cvi_webhook.go @@ -26,7 +26,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/webhook/admission" "github.com/deckhouse/deckhouse/pkg/log" - "github.com/deckhouse/virtualization-controller/pkg/common/validate" "github.com/deckhouse/virtualization-controller/pkg/controller/conditions" "github.com/deckhouse/virtualization/api/core/v1alpha2" "github.com/deckhouse/virtualization/api/core/v1alpha2/cvicondition" @@ -52,10 +51,6 @@ func (v *Validator) ValidateCreate(_ context.Context, obj runtime.Object) (admis return nil, fmt.Errorf("the ClusterVirtualImage name %q is invalid: '.' is forbidden, allowed name symbols are [0-9a-zA-Z-]", cvi.Name) } - if len(cvi.Name) > validate.MaxClusterVirtualImageNameLen { - return nil, fmt.Errorf("the ClusterVirtualImage name %q is too long: it must be no more than %d characters", cvi.Name, validate.MaxClusterVirtualImageNameLen) - } - return nil, nil } @@ -87,10 +82,6 @@ func (v *Validator) ValidateUpdate(_ context.Context, oldObj, newObj runtime.Obj warnings = append(warnings, fmt.Sprintf("the ClusterVirtualImage name %q is invalid as it contains now forbidden symbol '.', allowed symbols for name are [0-9a-zA-Z-]. Create another image with valid name to avoid problems with future updates.", newCVI.Name)) } - if len(newCVI.Name) > validate.MaxClusterVirtualImageNameLen { - warnings = append(warnings, fmt.Sprintf("the ClusterVirtualImage name %q is too long: it must be no more than %d characters", newCVI.Name, validate.MaxClusterVirtualImageNameLen)) - } - return warnings, nil } diff --git a/images/virtualization-artifact/pkg/controller/service/restorer/common/common.go b/images/virtualization-artifact/pkg/controller/service/restorer/common/common.go index efdb87b973..db5e486c21 100644 --- a/images/virtualization-artifact/pkg/controller/service/restorer/common/common.go +++ b/images/virtualization-artifact/pkg/controller/service/restorer/common/common.go @@ -73,16 +73,12 @@ func OverrideName(kind, name string, rules []v1alpha2.NameReplacement) string { // ValidateResourceNameLength checks if the given resource name exceeds // the maximum allowed length for the specified Kubernetes resource kind. -// By default, the maximum length is set to MaxKubernetesResourceNameLength, -// but for VirtualMachine and VirtualDisk resources, it uses -// MaxVirtualMachineNameLen and MaxDiskNameLen respectively. +// By default, the maximum length is MaxKubernetesResourceNameLength (253); +// VirtualMachine uses the tighter MaxVirtualMachineNameLen. func ValidateResourceNameLength(resourceName, kind string) error { maxLength := MaxKubernetesResourceNameLength - switch kind { - case v1alpha2.VirtualMachineKind: + if kind == v1alpha2.VirtualMachineKind { maxLength = validate.MaxVirtualMachineNameLen - case v1alpha2.VirtualDiskKind: - maxLength = validate.MaxDiskNameLen } if len(resourceName) > maxLength { return fmt.Errorf("name %q too long (%d > %d): %w", diff --git a/images/virtualization-artifact/pkg/controller/vd/internal/validator/name_validator.go b/images/virtualization-artifact/pkg/controller/vd/internal/validator/name_validator.go index 53f8ea41a0..41b0a04c98 100644 --- a/images/virtualization-artifact/pkg/controller/vd/internal/validator/name_validator.go +++ b/images/virtualization-artifact/pkg/controller/vd/internal/validator/name_validator.go @@ -23,7 +23,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/webhook/admission" - "github.com/deckhouse/virtualization-controller/pkg/common/validate" "github.com/deckhouse/virtualization/api/core/v1alpha2" ) @@ -38,10 +37,8 @@ func (v *NameValidator) ValidateCreate(_ context.Context, vd *v1alpha2.VirtualDi return nil, fmt.Errorf("the VirtualDisk name %q is invalid: '.' is forbidden, allowed name symbols are [0-9a-zA-Z-]", vd.Name) } - if len(vd.Name) > validate.MaxDiskNameLen { - return nil, fmt.Errorf("the VirtualDisk name %q is too long: it must be no more than %d characters", vd.Name, validate.MaxDiskNameLen) - } - + // The overall name length is bounded by Kubernetes (DNS subdomain, <=253); no + // extra DVP limit is needed since the derived KubeVirt name is shortened. return nil, nil } @@ -52,9 +49,5 @@ func (v *NameValidator) ValidateUpdate(_ context.Context, _, newVD *v1alpha2.Vir warnings = append(warnings, fmt.Sprintf("the VirtualDisk name %q is invalid as it contains now forbidden symbol '.', allowed symbols for name are [0-9a-zA-Z-]. Create another disk with valid name to avoid problems with future updates.", newVD.Name)) } - if len(newVD.Name) > validate.MaxDiskNameLen { - warnings = append(warnings, fmt.Sprintf("the VirtualDisk name %q is too long: it must be no more than %d characters", newVD.Name, validate.MaxDiskNameLen)) - } - return warnings, nil } diff --git a/images/virtualization-artifact/pkg/controller/vd/internal/validator/name_validator_test.go b/images/virtualization-artifact/pkg/controller/vd/internal/validator/name_validator_test.go new file mode 100644 index 0000000000..b98e483137 --- /dev/null +++ b/images/virtualization-artifact/pkg/controller/vd/internal/validator/name_validator_test.go @@ -0,0 +1,53 @@ +/* +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 validator + +import ( + "context" + "strings" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "github.com/deckhouse/virtualization/api/core/v1alpha2" +) + +var _ = Describe("VirtualDisk NameValidator", func() { + v := NewNameValidator() + + newVD := func(name string) *v1alpha2.VirtualDisk { + return &v1alpha2.VirtualDisk{ObjectMeta: metav1.ObjectMeta{Name: name}} + } + + It("rejects a name containing a dot on create (DVP rule, not enforced by Kubernetes)", func() { + _, err := v.ValidateCreate(context.Background(), newVD("my.disk")) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("'.' is forbidden")) + }) + + It("accepts a long dot-free name on create (length is bounded by Kubernetes, not by DVP)", func() { + _, err := v.ValidateCreate(context.Background(), newVD("vd-"+strings.Repeat("a", 200))) + Expect(err).NotTo(HaveOccurred()) + }) + + It("warns about a dot on update", func() { + warnings, err := v.ValidateUpdate(context.Background(), newVD("my.disk"), newVD("my.disk")) + Expect(err).NotTo(HaveOccurred()) + Expect(warnings).NotTo(BeEmpty()) + }) +}) diff --git a/images/virtualization-artifact/pkg/controller/vi/vi_webhook.go b/images/virtualization-artifact/pkg/controller/vi/vi_webhook.go index bc0514d692..4de0f0e644 100644 --- a/images/virtualization-artifact/pkg/controller/vi/vi_webhook.go +++ b/images/virtualization-artifact/pkg/controller/vi/vi_webhook.go @@ -30,7 +30,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/webhook/admission" "github.com/deckhouse/deckhouse/pkg/log" - "github.com/deckhouse/virtualization-controller/pkg/common/validate" "github.com/deckhouse/virtualization-controller/pkg/controller/conditions" intsvc "github.com/deckhouse/virtualization-controller/pkg/controller/vi/internal/service" "github.com/deckhouse/virtualization/api/core/v1alpha2" @@ -61,10 +60,6 @@ func (v *Validator) ValidateCreate(ctx context.Context, obj runtime.Object) (adm return nil, fmt.Errorf("the VirtualImage name %q is invalid: '.' is forbidden, allowed name symbols are [0-9a-zA-Z-]", vi.Name) } - if len(vi.Name) > validate.MaxVirtualImageNameLen { - return nil, fmt.Errorf("the VirtualImage name %q is too long: it must be no more than %d characters", vi.Name, validate.MaxVirtualImageNameLen) - } - if vi.Spec.Storage == v1alpha2.StorageKubernetes { warnings := admission.Warnings{ fmt.Sprintf("Using the `%s` storage type is deprecated. It is recommended to use `%s` instead.", @@ -191,10 +186,6 @@ func (v *Validator) ValidateUpdate(ctx context.Context, oldObj, newObj runtime.O warnings = append(warnings, fmt.Sprintf(" the VirtualImage name %q is invalid as it contains now forbidden symbol '.', allowed symbols for name are [0-9a-zA-Z-]. Create another image with valid name to avoid problems with future updates.", newVI.Name)) } - if len(newVI.Name) > validate.MaxVirtualImageNameLen { - warnings = append(warnings, fmt.Sprintf("the VirtualImage name %q is too long: it must be no more than %d characters", newVI.Name, validate.MaxVirtualImageNameLen)) - } - return warnings, nil } diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/validators/meta_validator_test.go b/images/virtualization-artifact/pkg/controller/vm/internal/validators/meta_validator_test.go new file mode 100644 index 0000000000..219bb4b876 --- /dev/null +++ b/images/virtualization-artifact/pkg/controller/vm/internal/validators/meta_validator_test.go @@ -0,0 +1,50 @@ +/* +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 ( + "context" + "strings" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "github.com/deckhouse/virtualization-controller/pkg/controller/vm/internal/validators" + "github.com/deckhouse/virtualization/api/core/v1alpha2" +) + +// VirtualMachine keeps a DVP-specific name limit of 63 (its name flows into pod +// names and label values); Kubernetes allows up to 253, so DVP must enforce it. +var _ = Describe("VirtualMachine MetaValidator name length", func() { + v := validators.NewMetaValidator(nil) + + newVM := func(name string) *v1alpha2.VirtualMachine { + return &v1alpha2.VirtualMachine{ObjectMeta: metav1.ObjectMeta{Name: name}} + } + + It("accepts a 63-character name", func() { + _, err := v.ValidateCreate(context.Background(), newVM(strings.Repeat("a", 63))) + Expect(err).NotTo(HaveOccurred()) + }) + + It("rejects a 64-character name", func() { + _, err := v.ValidateCreate(context.Background(), newVM(strings.Repeat("a", 64))) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("no more than 63 characters")) + }) +})