From d02b6bed7bb2b4035fa8a2b699c7b5e5a83eadfc Mon Sep 17 00:00:00 2001 From: Melvin Hillsman Date: Mon, 25 May 2026 21:16:39 -0500 Subject: [PATCH 1/4] fix(vm): replace resource.MustParse with ParseQuantity to prevent panics MustParse panics on invalid input, which crashes the process when users provide malformed memory or disk-size values. Use ParseQuantity and propagate errors to callers instead. Resolves #97 Signed-off-by: Melvin Hillsman --- internal/vm/vm.go | 21 ++++++++++---- internal/vm/vm_test.go | 65 +++++++++++++++++++++++++++++++++--------- 2 files changed, 66 insertions(+), 20 deletions(-) diff --git a/internal/vm/vm.go b/internal/vm/vm.go index 18b06e4..695d96b 100644 --- a/internal/vm/vm.go +++ b/internal/vm/vm.go @@ -39,7 +39,7 @@ type VMSpecOpts struct { // BuildVMSpec constructs a KubeVirt VirtualMachine from the given options. // It configures a containerDisk for the OS image, cloudInitNoCloud for userdata, // masquerade networking, and virtio disk bus. -func BuildVMSpec(opts VMSpecOpts) *kubevirtv1.VirtualMachine { +func BuildVMSpec(opts VMSpecOpts) (*kubevirtv1.VirtualMachine, error) { runStrategy := kubevirtv1.RunStrategyAlways disks := []kubevirtv1.Disk{ @@ -98,6 +98,11 @@ func BuildVMSpec(opts VMSpecOpts) *kubevirtv1.VirtualMachine { } volumes = append(volumes, opts.ExtraVolumes...) + memQty, err := resource.ParseQuantity(opts.Memory) + if err != nil { + return nil, fmt.Errorf("parsing memory %q: %w", opts.Memory, err) + } + return &kubevirtv1.VirtualMachine{ TypeMeta: metav1.TypeMeta{ APIVersion: kubevirtv1.SchemeGroupVersion.String(), @@ -122,7 +127,7 @@ func BuildVMSpec(opts VMSpecOpts) *kubevirtv1.VirtualMachine { }, Resources: kubevirtv1.ResourceRequirements{ Requests: corev1.ResourceList{ - corev1.ResourceMemory: resource.MustParse(opts.Memory), + corev1.ResourceMemory: memQty, }, }, Devices: kubevirtv1.Devices{ @@ -150,12 +155,16 @@ func BuildVMSpec(opts VMSpecOpts) *kubevirtv1.VirtualMachine { }, DataVolumeTemplates: opts.DataVolumeTemplates, }, - } + }, nil } // BuildDataVolumeTemplate constructs a DataVolumeTemplateSpec for a blank disk // with the given name and size. -func BuildDataVolumeTemplate(name, size string) kubevirtv1.DataVolumeTemplateSpec { +func BuildDataVolumeTemplate(name, size string) (kubevirtv1.DataVolumeTemplateSpec, error) { + sizeQty, err := resource.ParseQuantity(size) + if err != nil { + return kubevirtv1.DataVolumeTemplateSpec{}, fmt.Errorf("parsing disk size %q: %w", size, err) + } return kubevirtv1.DataVolumeTemplateSpec{ ObjectMeta: metav1.ObjectMeta{ Name: name, @@ -167,12 +176,12 @@ func BuildDataVolumeTemplate(name, size string) kubevirtv1.DataVolumeTemplateSpe Storage: &cdiv1beta1.StorageSpec{ Resources: corev1.VolumeResourceRequirements{ Requests: corev1.ResourceList{ - corev1.ResourceStorage: resource.MustParse(size), + corev1.ResourceStorage: sizeQty, }, }, }, }, - } + }, nil } // CreateVM creates a VirtualMachine. AlreadyExists errors are treated as diff --git a/internal/vm/vm_test.go b/internal/vm/vm_test.go index 3aaf674..6d9aa02 100644 --- a/internal/vm/vm_test.go +++ b/internal/vm/vm_test.go @@ -42,7 +42,9 @@ var _ = Describe("BuildVMSpec", func() { "app.kubernetes.io/name": "cpu", }, } - result = vm.BuildVMSpec(opts) + var err error + result, err = vm.BuildVMSpec(opts) + Expect(err).NotTo(HaveOccurred()) }) It("should set correct API version and kind", func() { @@ -118,7 +120,9 @@ var _ = Describe("BuildVMSpec", func() { It("should use UserDataSecretRef when CloudInitSecretName is set", func() { opts.CloudInitSecretName = "my-vm-cloudinit" - result = vm.BuildVMSpec(opts) + var err error + result, err = vm.BuildVMSpec(opts) + Expect(err).NotTo(HaveOccurred()) volumes := result.Spec.Template.Spec.Volumes var cloudInit *kubevirtv1.Volume @@ -137,7 +141,9 @@ var _ = Describe("BuildVMSpec", func() { It("should use inline UserData when CloudInitSecretName is empty", func() { opts.CloudInitSecretName = "" - result = vm.BuildVMSpec(opts) + var err error + result, err = vm.BuildVMSpec(opts) + Expect(err).NotTo(HaveOccurred()) volumes := result.Spec.Template.Spec.Volumes var cloudInit *kubevirtv1.Volume @@ -174,7 +180,9 @@ var _ = Describe("BuildVMSpec", func() { }, }, } - result = vm.BuildVMSpec(opts) + var err error + result, err = vm.BuildVMSpec(opts) + Expect(err).NotTo(HaveOccurred()) disks := result.Spec.Template.Spec.Domain.Devices.Disks Expect(disks).To(HaveLen(3)) // containerdisk + cloudinitdisk + datadisk @@ -184,9 +192,11 @@ var _ = Describe("BuildVMSpec", func() { }) It("should include data volume templates when provided", func() { - dvt := vm.BuildDataVolumeTemplate("test-data", "10Gi") + dvt, err := vm.BuildDataVolumeTemplate("test-data", "10Gi") + Expect(err).NotTo(HaveOccurred()) opts.DataVolumeTemplates = []kubevirtv1.DataVolumeTemplateSpec{dvt} - result = vm.BuildVMSpec(opts) + result, err = vm.BuildVMSpec(opts) + Expect(err).NotTo(HaveOccurred()) Expect(result.Spec.DataVolumeTemplates).To(HaveLen(1)) Expect(result.Spec.DataVolumeTemplates[0].Name).To(Equal("test-data")) @@ -195,23 +205,32 @@ var _ = Describe("BuildVMSpec", func() { var _ = Describe("BuildDataVolumeTemplate", func() { It("should set name", func() { - dvt := vm.BuildDataVolumeTemplate("data-disk", "20Gi") + dvt, err := vm.BuildDataVolumeTemplate("data-disk", "20Gi") + Expect(err).NotTo(HaveOccurred()) Expect(dvt.Name).To(Equal("data-disk")) }) It("should set blank source", func() { - dvt := vm.BuildDataVolumeTemplate("data-disk", "20Gi") + dvt, err := vm.BuildDataVolumeTemplate("data-disk", "20Gi") + Expect(err).NotTo(HaveOccurred()) Expect(dvt.Spec.Source).NotTo(BeNil()) Expect(dvt.Spec.Source.Blank).NotTo(BeNil()) }) It("should set storage size", func() { - dvt := vm.BuildDataVolumeTemplate("data-disk", "20Gi") + dvt, err := vm.BuildDataVolumeTemplate("data-disk", "20Gi") + Expect(err).NotTo(HaveOccurred()) Expect(dvt.Spec.Storage).NotTo(BeNil()) storageReq := dvt.Spec.Storage.Resources.Requests[corev1.ResourceStorage] expected := resource.MustParse("20Gi") Expect(storageReq.Equal(expected)).To(BeTrue()) }) + + It("should return error for invalid disk size", func() { + _, err := vm.BuildDataVolumeTemplate("data-disk", "not-a-size") + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("parsing disk size")) + }) }) var _ = Describe("CreateVM", func() { @@ -227,7 +246,7 @@ var _ = Describe("CreateVM", func() { }) newTestVM := func(name string) *kubevirtv1.VirtualMachine { - return vm.BuildVMSpec(vm.VMSpecOpts{ + v, err := vm.BuildVMSpec(vm.VMSpecOpts{ Name: name, Namespace: "default", ContainerDiskImage: "test-image", @@ -236,8 +255,23 @@ var _ = Describe("CreateVM", func() { Memory: "1Gi", Labels: map[string]string{"test": "true"}, }) + Expect(err).NotTo(HaveOccurred()) + return v } + It("should return error for invalid memory", func() { + _, err := vm.BuildVMSpec(vm.VMSpecOpts{ + Name: "bad-mem", + Namespace: "default", + ContainerDiskImage: "test-image", + CloudInitUserdata: "#cloud-config\n", + CPUCores: 1, + Memory: "not-a-quantity", + }) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("parsing memory")) + }) + It("should create VM successfully", func() { c := fake.NewClientBuilder().WithScheme(scheme).Build() testVM := newTestVM("test-vm") @@ -331,7 +365,7 @@ var _ = Describe("DeleteVM", func() { }) It("should delete VM successfully", func() { - testVM := vm.BuildVMSpec(vm.VMSpecOpts{ + testVM, err := vm.BuildVMSpec(vm.VMSpecOpts{ Name: "delete-me", Namespace: "default", ContainerDiskImage: "test-image", @@ -339,9 +373,10 @@ var _ = Describe("DeleteVM", func() { CPUCores: 1, Memory: "1Gi", }) + Expect(err).NotTo(HaveOccurred()) c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(testVM).Build() - err := vm.DeleteVM(ctx, c, "delete-me", "default") + err = vm.DeleteVM(ctx, c, "delete-me", "default") Expect(err).NotTo(HaveOccurred()) got := &kubevirtv1.VirtualMachine{} @@ -368,7 +403,7 @@ var _ = Describe("ListVMs", func() { }) It("should list VMs by labels", func() { - vm1 := vm.BuildVMSpec(vm.VMSpecOpts{ + vm1, err := vm.BuildVMSpec(vm.VMSpecOpts{ Name: "vm-1", Namespace: "default", ContainerDiskImage: "test-image", @@ -377,7 +412,8 @@ var _ = Describe("ListVMs", func() { Memory: "1Gi", Labels: map[string]string{"app.kubernetes.io/managed-by": "virtwork"}, }) - vm2 := vm.BuildVMSpec(vm.VMSpecOpts{ + Expect(err).NotTo(HaveOccurred()) + vm2, err := vm.BuildVMSpec(vm.VMSpecOpts{ Name: "vm-2", Namespace: "default", ContainerDiskImage: "test-image", @@ -386,6 +422,7 @@ var _ = Describe("ListVMs", func() { Memory: "1Gi", Labels: map[string]string{"app.kubernetes.io/managed-by": "other"}, }) + Expect(err).NotTo(HaveOccurred()) c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(vm1, vm2).Build() vms, err := vm.ListVMs(ctx, c, "default", map[string]string{ From 30a3a4b96cf743c71921d77345dab808d12e2954 Mon Sep 17 00:00:00 2001 From: Melvin Hillsman Date: Mon, 25 May 2026 21:25:54 -0500 Subject: [PATCH 2/4] fix(callers): propagate error returns from BuildVMSpec and DataVolumeTemplates Update all callers of BuildVMSpec and DataVolumeTemplates to handle the (value, error) return pattern introduced in dfd2617. This includes production code (cmd/virtwork/main.go, workload implementations), unit tests, and integration tests. Signed-off-by: Melvin Hillsman --- cmd/virtwork/main.go | 28 ++++++++++++++++---- cmd/virtwork/main_test.go | 18 ++++++++----- internal/cleanup/cleanup_integration_test.go | 12 ++++++--- internal/cleanup/cleanup_test.go | 14 +++++++--- internal/vm/vm_integration_test.go | 21 ++++++++++----- internal/wait/wait_integration_test.go | 7 +++-- internal/workloads/chaos_disk.go | 8 +++--- internal/workloads/chaos_disk_test.go | 3 ++- internal/workloads/database.go | 8 +++--- internal/workloads/database_test.go | 3 ++- internal/workloads/disk.go | 8 +++--- internal/workloads/disk_test.go | 3 ++- internal/workloads/registry_test.go | 3 ++- internal/workloads/workload.go | 6 ++--- 14 files changed, 99 insertions(+), 43 deletions(-) diff --git a/cmd/virtwork/main.go b/cmd/virtwork/main.go index 407da5b..ac14074 100644 --- a/cmd/virtwork/main.go +++ b/cmd/virtwork/main.go @@ -286,13 +286,17 @@ func runE(cmd *cobra.Command, args []string) error { res := w.VMResources() // Record workload in audit + dvTemplatesForAudit, err := w.DataVolumeTemplates() + if err != nil { + return fmt.Errorf("building data volume templates for %q: %w", name, err) + } wlID, _ := auditor.RecordWorkload(ctx, execID, audit.WorkloadRecord{ WorkloadType: name, Enabled: true, VMCount: vmCount, CPUCores: res.CPUCores, Memory: res.Memory, - HasDataDisk: len(w.DataVolumeTemplates()) > 0, + HasDataDisk: len(dvTemplatesForAudit) > 0, DataDiskSize: cfg.DataDiskSize, RequiresService: w.RequiresService(), }) @@ -308,8 +312,12 @@ func runE(cmd *cobra.Command, args []string) error { vmName := fmt.Sprintf("virtwork-%s-%d", name, i) // Namespace DataVolume names to avoid collisions across VMs + dvts, err := w.DataVolumeTemplates() + if err != nil { + return fmt.Errorf("building data volume templates for %q vm %s: %w", name, vmName, err) + } dvTemplates, extraVols := namespaceDataVolumes( - w.DataVolumeTemplates(), + dvts, w.ExtraVolumes(), vmName, ) @@ -359,8 +367,12 @@ func runE(cmd *cobra.Command, args []string) error { } // Namespace DataVolume names to avoid collisions across VMs + dvts, err := w.DataVolumeTemplates() + if err != nil { + return fmt.Errorf("building data volume templates for %q role %q vm %s: %w", name, role, vmName, err) + } dvTemplates, extraVols := namespaceDataVolumes( - w.DataVolumeTemplates(), + dvts, w.ExtraVolumes(), vmName, ) @@ -496,7 +508,10 @@ func runE(cmd *cobra.Command, args []string) error { for _, plan := range plans { p := plan // capture loop variable g.Go(func() error { - vmObj := vm.BuildVMSpec(*p.vmSpec) + vmObj, err := vm.BuildVMSpec(*p.vmSpec) + if err != nil { + return fmt.Errorf("building VM spec for %q: %w", p.vmName, err) + } if err := vm.CreateVM(gctx, c, vmObj); err != nil { _ = auditor.RecordEvent(ctx, execID, audit.EventRecord{ EventType: "vm_failed", @@ -746,7 +761,10 @@ func printDryRun(logger *slog.Logger, plans []vmPlan) error { logger.Info("dry run mode", slog.Int("total_vms", len(plans))) for _, p := range plans { - vmObj := vm.BuildVMSpec(*p.vmSpec) + vmObj, err := vm.BuildVMSpec(*p.vmSpec) + if err != nil { + return fmt.Errorf("building VM spec for %q: %w", p.vmName, err) + } data, err := sigyaml.Marshal(vmObj) if err != nil { return fmt.Errorf("marshaling VM spec for %q: %w", p.vmName, err) diff --git a/cmd/virtwork/main_test.go b/cmd/virtwork/main_test.go index b22fe91..2869bf1 100644 --- a/cmd/virtwork/main_test.go +++ b/cmd/virtwork/main_test.go @@ -364,7 +364,7 @@ var _ = Describe("Run orchestration", func() { Expect(err).NotTo(HaveOccurred()) res := w.VMResources() - vmSpec := vm.BuildVMSpec(vm.VMSpecOpts{ + vmSpec, err := vm.BuildVMSpec(vm.VMSpecOpts{ Name: "virtwork-cpu-0", Namespace: constants.DefaultNamespace, ContainerDiskImage: constants.DefaultContainerDiskImage, @@ -377,6 +377,7 @@ var _ = Describe("Run orchestration", func() { constants.LabelComponent: "cpu", }, }) + Expect(err).NotTo(HaveOccurred()) Expect(vmSpec).NotTo(BeNil()) Expect(vmSpec.Name).To(Equal("virtwork-cpu-0")) Expect(vmSpec.Namespace).To(Equal(constants.DefaultNamespace)) @@ -401,7 +402,7 @@ var _ = Describe("Run orchestration", func() { Expect(err).NotTo(HaveOccurred()) res := w.VMResources() - vmSpec := vm.BuildVMSpec(vm.VMSpecOpts{ + vmSpec, err := vm.BuildVMSpec(vm.VMSpecOpts{ Name: "virtwork-cpu-0", Namespace: constants.DefaultNamespace, ContainerDiskImage: constants.DefaultContainerDiskImage, @@ -414,6 +415,7 @@ var _ = Describe("Run orchestration", func() { constants.LabelComponent: "cpu", }, }) + Expect(err).NotTo(HaveOccurred()) // Verify spec can be marshaled (simulating dry-run output) var buf bytes.Buffer @@ -464,7 +466,7 @@ var _ = Describe("Run orchestration", func() { Expect(err).NotTo(HaveOccurred()) res := w.VMResources() - vmSpec := vm.BuildVMSpec(vm.VMSpecOpts{ + vmSpec, err := vm.BuildVMSpec(vm.VMSpecOpts{ Name: "virtwork-cpu-0", Namespace: constants.DefaultNamespace, ContainerDiskImage: constants.DefaultContainerDiskImage, @@ -477,6 +479,7 @@ var _ = Describe("Run orchestration", func() { constants.LabelComponent: "cpu", }, }) + Expect(err).NotTo(HaveOccurred()) err = vm.CreateVM(ctx, c, vmSpec) Expect(err).NotTo(HaveOccurred()) @@ -718,7 +721,7 @@ var _ = Describe("CLI end-to-end scenarios", func() { Expect(err).NotTo(HaveOccurred()) res := w.VMResources() - vmSpec := vm.BuildVMSpec(vm.VMSpecOpts{ + vmSpec, err := vm.BuildVMSpec(vm.VMSpecOpts{ Name: "virtwork-cpu-0", Namespace: constants.DefaultNamespace, ContainerDiskImage: constants.DefaultContainerDiskImage, @@ -731,6 +734,7 @@ var _ = Describe("CLI end-to-end scenarios", func() { constants.LabelComponent: "cpu", }, }) + Expect(err).NotTo(HaveOccurred()) var buf bytes.Buffer fmt.Fprintf(&buf, "--- Dry Run ---\n") @@ -836,7 +840,8 @@ var _ = Describe("DataVolume namespacing for multi-VM deployments", func() { ) Expect(err).NotTo(HaveOccurred()) - dvTemplates := w.DataVolumeTemplates() + dvTemplates, err := w.DataVolumeTemplates() + Expect(err).NotTo(HaveOccurred()) Expect(dvTemplates).To(HaveLen(1)) Expect(dvTemplates[0].Name).To(Equal("virtwork-disk-data")) @@ -863,7 +868,8 @@ var _ = Describe("DataVolume namespacing for multi-VM deployments", func() { ) Expect(err).NotTo(HaveOccurred()) - dvTemplates := w.DataVolumeTemplates() + dvTemplates, err := w.DataVolumeTemplates() + Expect(err).NotTo(HaveOccurred()) Expect(dvTemplates).To(HaveLen(1)) Expect(dvTemplates[0].Name).To(Equal("virtwork-database-data")) diff --git a/internal/cleanup/cleanup_integration_test.go b/internal/cleanup/cleanup_integration_test.go index 8a14a68..89b4b65 100644 --- a/internal/cleanup/cleanup_integration_test.go +++ b/internal/cleanup/cleanup_integration_test.go @@ -43,7 +43,9 @@ var _ = Describe("CleanupAll [integration]", func() { It("should delete VMs by managed-by label", func() { opts := testutil.DefaultVMOpts("cleanup-vm-0", namespace) - Expect(vm.CreateVM(ctx, c, vm.BuildVMSpec(opts))).To(Succeed()) + vmObj, err := vm.BuildVMSpec(opts) + Expect(err).NotTo(HaveOccurred()) + Expect(vm.CreateVM(ctx, c, vmObj)).To(Succeed()) result, err := cleanup.CleanupAll(ctx, c, &config.Config{Namespace: namespace}, false, "") Expect(err).NotTo(HaveOccurred()) @@ -127,7 +129,9 @@ var _ = Describe("CleanupAll [integration]", func() { It("should report accurate counts for mixed resources", func() { // Create a VM, a service, and a secret opts := testutil.DefaultVMOpts("cleanup-mix-vm", namespace) - Expect(vm.CreateVM(ctx, c, vm.BuildVMSpec(opts))).To(Succeed()) + vmObj, err := vm.BuildVMSpec(opts) + Expect(err).NotTo(HaveOccurred()) + Expect(vm.CreateVM(ctx, c, vmObj)).To(Succeed()) svc := &corev1.Service{ ObjectMeta: metav1.ObjectMeta{ @@ -174,7 +178,9 @@ var _ = Describe("CleanupAll [integration]", func() { It("should be idempotent when run twice", func() { opts := testutil.DefaultVMOpts("cleanup-idem-vm", namespace) - Expect(vm.CreateVM(ctx, c, vm.BuildVMSpec(opts))).To(Succeed()) + vmObj, err := vm.BuildVMSpec(opts) + Expect(err).NotTo(HaveOccurred()) + Expect(vm.CreateVM(ctx, c, vmObj)).To(Succeed()) result1, err := cleanup.CleanupAll(ctx, c, &config.Config{Namespace: namespace}, false, "") Expect(err).NotTo(HaveOccurred()) diff --git a/internal/cleanup/cleanup_test.go b/internal/cleanup/cleanup_test.go index 245465e..51cbb9b 100644 --- a/internal/cleanup/cleanup_test.go +++ b/internal/cleanup/cleanup_test.go @@ -46,7 +46,7 @@ var _ = Describe("PreviewCleanup", func() { for _, el := range extraLabels { maps.Copy(l, el) } - return vm.BuildVMSpec(vm.VMSpecOpts{ + v, err := vm.BuildVMSpec(vm.VMSpecOpts{ Name: name, Namespace: namespace, ContainerDiskImage: "test-image", @@ -55,6 +55,8 @@ var _ = Describe("PreviewCleanup", func() { Memory: "1Gi", Labels: l, }) + Expect(err).NotTo(HaveOccurred()) + return v } newManagedSecret := func(name string, extraLabels ...map[string]string) *corev1.Secret { @@ -163,7 +165,7 @@ var _ = Describe("PreviewCleanup", func() { It("should not count unmanaged resources", func() { managedVM := newManagedVM("managed-vm") - unmanagedVM := vm.BuildVMSpec(vm.VMSpecOpts{ + unmanagedVM, err := vm.BuildVMSpec(vm.VMSpecOpts{ Name: "unmanaged-vm", Namespace: namespace, ContainerDiskImage: "test-image", @@ -174,6 +176,7 @@ var _ = Describe("PreviewCleanup", func() { constants.LabelManagedBy: "other-tool", }, }) + Expect(err).NotTo(HaveOccurred()) c := fake.NewClientBuilder(). WithScheme(scheme). WithObjects(managedVM, unmanagedVM). @@ -226,7 +229,7 @@ var _ = Describe("CleanupAll", func() { }) newManagedVM := func(name string) *kubevirtv1.VirtualMachine { - return vm.BuildVMSpec(vm.VMSpecOpts{ + v, err := vm.BuildVMSpec(vm.VMSpecOpts{ Name: name, Namespace: namespace, ContainerDiskImage: "test-image", @@ -235,6 +238,8 @@ var _ = Describe("CleanupAll", func() { Memory: "1Gi", Labels: labels, }) + Expect(err).NotTo(HaveOccurred()) + return v } newManagedSecret := func(name string) *corev1.Secret { @@ -476,7 +481,7 @@ var _ = Describe("CleanupAll", func() { It("should use correct managed-by=virtwork label selector", func() { // Create a managed VM and an unmanaged VM managedVM := newManagedVM("managed-vm") - unmanagedVM := vm.BuildVMSpec(vm.VMSpecOpts{ + unmanagedVM, err := vm.BuildVMSpec(vm.VMSpecOpts{ Name: "unmanaged-vm", Namespace: namespace, ContainerDiskImage: "test-image", @@ -487,6 +492,7 @@ var _ = Describe("CleanupAll", func() { constants.LabelManagedBy: "other-tool", }, }) + Expect(err).NotTo(HaveOccurred()) // Create a managed service and an unmanaged service managedSvc := newManagedService("managed-svc") unmanagedSvc := &corev1.Service{ diff --git a/internal/vm/vm_integration_test.go b/internal/vm/vm_integration_test.go index 8de0ebc..ffbfa62 100644 --- a/internal/vm/vm_integration_test.go +++ b/internal/vm/vm_integration_test.go @@ -38,9 +38,10 @@ var _ = Describe("CreateVM [integration]", func() { It("should create a VirtualMachine on the cluster", func() { opts := testutil.DefaultVMOpts("integ-vm-0", namespace) - vmObj := vm.BuildVMSpec(opts) + vmObj, err := vm.BuildVMSpec(opts) + Expect(err).NotTo(HaveOccurred()) - err := vm.CreateVM(ctx, c, vmObj) + err = vm.CreateVM(ctx, c, vmObj) Expect(err).NotTo(HaveOccurred()) // Verify the VM exists @@ -52,14 +53,17 @@ var _ = Describe("CreateVM [integration]", func() { It("should be idempotent on repeated calls", func() { opts := testutil.DefaultVMOpts("integ-vm-idem", namespace) + vmObj, err := vm.BuildVMSpec(opts) + Expect(err).NotTo(HaveOccurred()) - Expect(vm.CreateVM(ctx, c, vm.BuildVMSpec(opts))).To(Succeed()) - Expect(vm.CreateVM(ctx, c, vm.BuildVMSpec(opts))).To(Succeed()) + Expect(vm.CreateVM(ctx, c, vmObj)).To(Succeed()) + Expect(vm.CreateVM(ctx, c, vmObj)).To(Succeed()) }) It("should set the correct labels on the created VM", func() { opts := testutil.DefaultVMOpts("integ-vm-labels", namespace) - vmObj := vm.BuildVMSpec(opts) + vmObj, err := vm.BuildVMSpec(opts) + Expect(err).NotTo(HaveOccurred()) Expect(vm.CreateVM(ctx, c, vmObj)).To(Succeed()) @@ -90,7 +94,8 @@ var _ = Describe("DeleteVM [integration]", func() { It("should delete an existing VM", func() { opts := testutil.DefaultVMOpts("integ-vm-del", namespace) - vmObj := vm.BuildVMSpec(opts) + vmObj, err := vm.BuildVMSpec(opts) + Expect(err).NotTo(HaveOccurred()) Expect(vm.CreateVM(ctx, c, vmObj)).To(Succeed()) err := vm.DeleteVM(ctx, c, "integ-vm-del", namespace) @@ -131,7 +136,9 @@ var _ = Describe("ListVMs [integration]", func() { It("should list VMs by label selector", func() { for i := 0; i < 3; i++ { opts := testutil.DefaultVMOpts("integ-vm-list-"+string(rune('0'+i)), namespace) - Expect(vm.CreateVM(ctx, c, vm.BuildVMSpec(opts))).To(Succeed()) + vmObj, err := vm.BuildVMSpec(opts) + Expect(err).NotTo(HaveOccurred()) + Expect(vm.CreateVM(ctx, c, vmObj)).To(Succeed()) } vms, err := vm.ListVMs(ctx, c, namespace, testutil.ManagedLabels()) diff --git a/internal/wait/wait_integration_test.go b/internal/wait/wait_integration_test.go index ada0df9..d909ff7 100644 --- a/internal/wait/wait_integration_test.go +++ b/internal/wait/wait_integration_test.go @@ -40,7 +40,8 @@ var _ = Describe("WaitForVMReady [integration]", Label("slow"), func() { It("should return nil once a VM reaches Running phase", func() { opts := testutil.DefaultVMOpts("wait-vm-0", namespace) - vmObj := vm.BuildVMSpec(opts) + vmObj, err := vm.BuildVMSpec(opts) + Expect(err).NotTo(HaveOccurred()) Expect(vm.CreateVM(ctx, c, vmObj)).To(Succeed()) err := wait.WaitForVMReady(ctx, c, slog.Default(), "wait-vm-0", namespace, 5*time.Minute, 5*time.Second) @@ -74,7 +75,9 @@ var _ = Describe("WaitForAllVMsReady [integration]", Label("slow"), func() { It("should wait for multiple VMs concurrently", func() { for _, name := range []string{"wait-multi-0", "wait-multi-1"} { opts := testutil.DefaultVMOpts(name, namespace) - Expect(vm.CreateVM(ctx, c, vm.BuildVMSpec(opts))).To(Succeed()) + vmObj, err := vm.BuildVMSpec(opts) + Expect(err).NotTo(HaveOccurred()) + Expect(vm.CreateVM(ctx, c, vmObj)).To(Succeed()) } results := wait.WaitForAllVMsReady(ctx, c, slog.Default(), diff --git a/internal/workloads/chaos_disk.go b/internal/workloads/chaos_disk.go index a24635f..fc117a7 100644 --- a/internal/workloads/chaos_disk.go +++ b/internal/workloads/chaos_disk.go @@ -157,10 +157,12 @@ func (w *ChaosDiskWorkload) CloudInitUserdata() (string, error) { } // DataVolumeTemplates returns a DataVolumeTemplateSpec for the data disk. -func (w *ChaosDiskWorkload) DataVolumeTemplates() []kubevirtv1.DataVolumeTemplateSpec { - return []kubevirtv1.DataVolumeTemplateSpec{ - vm.BuildDataVolumeTemplate("virtwork-chaos-disk-data", w.DataDiskSize), +func (w *ChaosDiskWorkload) DataVolumeTemplates() ([]kubevirtv1.DataVolumeTemplateSpec, error) { + dvt, err := vm.BuildDataVolumeTemplate("virtwork-chaos-disk-data", w.DataDiskSize) + if err != nil { + return nil, err } + return []kubevirtv1.DataVolumeTemplateSpec{dvt}, nil } // ExtraDisks returns the data disk definition. diff --git a/internal/workloads/chaos_disk_test.go b/internal/workloads/chaos_disk_test.go index 93200e6..6be9bdb 100644 --- a/internal/workloads/chaos_disk_test.go +++ b/internal/workloads/chaos_disk_test.go @@ -110,7 +110,8 @@ var _ = Describe("ChaosDiskWorkload", func() { }) It("should have data volume template", func() { - dvts := w.DataVolumeTemplates() + dvts, err := w.DataVolumeTemplates() + Expect(err).NotTo(HaveOccurred()) Expect(dvts).To(HaveLen(1)) Expect(dvts[0].Name).To(Equal("virtwork-chaos-disk-data")) }) diff --git a/internal/workloads/database.go b/internal/workloads/database.go index a7df51d..42508b4 100644 --- a/internal/workloads/database.go +++ b/internal/workloads/database.go @@ -142,10 +142,12 @@ func (w *DatabaseWorkload) CloudInitUserdata() (string, error) { } // DataVolumeTemplates returns a DataVolumeTemplateSpec for the PostgreSQL data disk. -func (w *DatabaseWorkload) DataVolumeTemplates() []kubevirtv1.DataVolumeTemplateSpec { - return []kubevirtv1.DataVolumeTemplateSpec{ - vm.BuildDataVolumeTemplate("virtwork-database-data", w.DataDiskSize), +func (w *DatabaseWorkload) DataVolumeTemplates() ([]kubevirtv1.DataVolumeTemplateSpec, error) { + dvt, err := vm.BuildDataVolumeTemplate("virtwork-database-data", w.DataDiskSize) + if err != nil { + return nil, err } + return []kubevirtv1.DataVolumeTemplateSpec{dvt}, nil } // ExtraDisks returns the data disk definition for PostgreSQL storage. diff --git a/internal/workloads/database_test.go b/internal/workloads/database_test.go index eab4485..e5eda1e 100644 --- a/internal/workloads/database_test.go +++ b/internal/workloads/database_test.go @@ -138,7 +138,8 @@ var _ = Describe("DatabaseWorkload", func() { }) It("should have data volume template", func() { - dvts := w.DataVolumeTemplates() + dvts, err := w.DataVolumeTemplates() + Expect(err).NotTo(HaveOccurred()) Expect(dvts).To(HaveLen(1)) Expect(dvts[0].Name).To(Equal("virtwork-database-data")) }) diff --git a/internal/workloads/disk.go b/internal/workloads/disk.go index c5eabf3..e37f457 100644 --- a/internal/workloads/disk.go +++ b/internal/workloads/disk.go @@ -121,10 +121,12 @@ func (w *DiskWorkload) CloudInitUserdata() (string, error) { } // DataVolumeTemplates returns a DataVolumeTemplateSpec for the data disk. -func (w *DiskWorkload) DataVolumeTemplates() []kubevirtv1.DataVolumeTemplateSpec { - return []kubevirtv1.DataVolumeTemplateSpec{ - vm.BuildDataVolumeTemplate("virtwork-disk-data", w.DataDiskSize), +func (w *DiskWorkload) DataVolumeTemplates() ([]kubevirtv1.DataVolumeTemplateSpec, error) { + dvt, err := vm.BuildDataVolumeTemplate("virtwork-disk-data", w.DataDiskSize) + if err != nil { + return nil, err } + return []kubevirtv1.DataVolumeTemplateSpec{dvt}, nil } // ExtraDisks returns the data disk definition. diff --git a/internal/workloads/disk_test.go b/internal/workloads/disk_test.go index 22098b4..891970b 100644 --- a/internal/workloads/disk_test.go +++ b/internal/workloads/disk_test.go @@ -58,7 +58,8 @@ var _ = Describe("DiskWorkload", func() { }) It("should have data volume template", func() { - dvts := w.DataVolumeTemplates() + dvts, err := w.DataVolumeTemplates() + Expect(err).NotTo(HaveOccurred()) Expect(dvts).To(HaveLen(1)) Expect(dvts[0].Name).To(Equal("virtwork-disk-data")) }) diff --git a/internal/workloads/registry_test.go b/internal/workloads/registry_test.go index 90f099c..1f817bb 100644 --- a/internal/workloads/registry_test.go +++ b/internal/workloads/registry_test.go @@ -209,7 +209,8 @@ var _ = Describe("Registry", func() { }, workloads.WithDataDiskSize("20Gi")) Expect(err).NotTo(HaveOccurred()) - dvts := w.DataVolumeTemplates() + dvts, err := w.DataVolumeTemplates() + Expect(err).NotTo(HaveOccurred()) Expect(dvts).NotTo(BeEmpty()) }) }) diff --git a/internal/workloads/workload.go b/internal/workloads/workload.go index fa36117..bbbe043 100644 --- a/internal/workloads/workload.go +++ b/internal/workloads/workload.go @@ -41,7 +41,7 @@ type Workload interface { // DataVolumeTemplates returns CDI DataVolumeTemplateSpecs for persistent storage. // Returns nil if no data volumes needed. - DataVolumeTemplates() []kubevirtv1.DataVolumeTemplateSpec + DataVolumeTemplates() ([]kubevirtv1.DataVolumeTemplateSpec, error) // RequiresService returns true if this workload needs a K8s Service. RequiresService() bool @@ -96,8 +96,8 @@ func (b *BaseWorkload) ExtraDisks() []kubevirtv1.Disk { } // DataVolumeTemplates returns nil — no data volumes by default. -func (b *BaseWorkload) DataVolumeTemplates() []kubevirtv1.DataVolumeTemplateSpec { - return nil +func (b *BaseWorkload) DataVolumeTemplates() ([]kubevirtv1.DataVolumeTemplateSpec, error) { + return nil, nil } // RequiresService returns false — no K8s Service by default. From 222d28f1ea749f91b54098d209394ca03764b0e8 Mon Sep 17 00:00:00 2001 From: Melvin Hillsman Date: Tue, 26 May 2026 08:18:35 -0500 Subject: [PATCH 3/4] fix(vm): use human-friendly examples in quantity parse errors Replace terse "parsing memory" / "parsing disk size" prefixes with messages that show valid examples (e.g. 512Mi, 2Gi), so users don't have to decipher the Kubernetes quantity regex. Signed-off-by: Melvin Hillsman --- internal/vm/vm.go | 4 ++-- internal/vm/vm_test.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/vm/vm.go b/internal/vm/vm.go index 695d96b..1d773d1 100644 --- a/internal/vm/vm.go +++ b/internal/vm/vm.go @@ -100,7 +100,7 @@ func BuildVMSpec(opts VMSpecOpts) (*kubevirtv1.VirtualMachine, error) { memQty, err := resource.ParseQuantity(opts.Memory) if err != nil { - return nil, fmt.Errorf("parsing memory %q: %w", opts.Memory, err) + return nil, fmt.Errorf("invalid memory %q (expected a quantity like 512Mi, 2Gi, or 4G): %w", opts.Memory, err) } return &kubevirtv1.VirtualMachine{ @@ -163,7 +163,7 @@ func BuildVMSpec(opts VMSpecOpts) (*kubevirtv1.VirtualMachine, error) { func BuildDataVolumeTemplate(name, size string) (kubevirtv1.DataVolumeTemplateSpec, error) { sizeQty, err := resource.ParseQuantity(size) if err != nil { - return kubevirtv1.DataVolumeTemplateSpec{}, fmt.Errorf("parsing disk size %q: %w", size, err) + return kubevirtv1.DataVolumeTemplateSpec{}, fmt.Errorf("invalid disk size %q (expected a quantity like 10Gi, 500Mi, or 1Ti): %w", size, err) } return kubevirtv1.DataVolumeTemplateSpec{ ObjectMeta: metav1.ObjectMeta{ diff --git a/internal/vm/vm_test.go b/internal/vm/vm_test.go index 6d9aa02..d6d694b 100644 --- a/internal/vm/vm_test.go +++ b/internal/vm/vm_test.go @@ -229,7 +229,7 @@ var _ = Describe("BuildDataVolumeTemplate", func() { It("should return error for invalid disk size", func() { _, err := vm.BuildDataVolumeTemplate("data-disk", "not-a-size") Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("parsing disk size")) + Expect(err.Error()).To(ContainSubstring("invalid disk size")) }) }) @@ -269,7 +269,7 @@ var _ = Describe("CreateVM", func() { Memory: "not-a-quantity", }) Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("parsing memory")) + Expect(err.Error()).To(ContainSubstring("invalid memory")) }) It("should create VM successfully", func() { From 6e1c777561e8b746c3dc57a18a5315e6bdac363b Mon Sep 17 00:00:00 2001 From: Melvin Hillsman Date: Thu, 28 May 2026 00:08:33 -0500 Subject: [PATCH 4/4] chore: address linting issues Signed-off-by: Melvin Hillsman --- cmd/virtwork/main.go | 4 +++- cmd/virtwork/main_test.go | 2 ++ internal/vm/vm.go | 4 +++- internal/vm/vm_integration_test.go | 2 +- internal/wait/wait_integration_test.go | 2 +- 5 files changed, 10 insertions(+), 4 deletions(-) diff --git a/cmd/virtwork/main.go b/cmd/virtwork/main.go index ac14074..3bdcb48 100644 --- a/cmd/virtwork/main.go +++ b/cmd/virtwork/main.go @@ -369,7 +369,9 @@ func runE(cmd *cobra.Command, args []string) error { // Namespace DataVolume names to avoid collisions across VMs dvts, err := w.DataVolumeTemplates() if err != nil { - return fmt.Errorf("building data volume templates for %q role %q vm %s: %w", name, role, vmName, err) + return fmt.Errorf( + "building data volume templates for %q role %q vm %s: %w", name, role, vmName, err, + ) } dvTemplates, extraVols := namespaceDataVolumes( dvts, diff --git a/cmd/virtwork/main_test.go b/cmd/virtwork/main_test.go index 2869bf1..6216e1f 100644 --- a/cmd/virtwork/main_test.go +++ b/cmd/virtwork/main_test.go @@ -823,6 +823,7 @@ var _ = Describe("CLI end-to-end scenarios", func() { }) var _ = Describe("DataVolume namespacing for multi-VM deployments", func() { + // nolint: dupl Context("when deploying multiple VMs of disk workload", func() { It("should return base DataVolume template name", func() { registry := workloads.DefaultRegistry() @@ -851,6 +852,7 @@ var _ = Describe("DataVolume namespacing for multi-VM deployments", func() { }) }) + // nolint:dupl Context("when deploying multiple VMs of database workload", func() { It("should return base DataVolume template name", func() { registry := workloads.DefaultRegistry() diff --git a/internal/vm/vm.go b/internal/vm/vm.go index 1d773d1..9275961 100644 --- a/internal/vm/vm.go +++ b/internal/vm/vm.go @@ -163,7 +163,9 @@ func BuildVMSpec(opts VMSpecOpts) (*kubevirtv1.VirtualMachine, error) { func BuildDataVolumeTemplate(name, size string) (kubevirtv1.DataVolumeTemplateSpec, error) { sizeQty, err := resource.ParseQuantity(size) if err != nil { - return kubevirtv1.DataVolumeTemplateSpec{}, fmt.Errorf("invalid disk size %q (expected a quantity like 10Gi, 500Mi, or 1Ti): %w", size, err) + return kubevirtv1.DataVolumeTemplateSpec{}, fmt.Errorf( + "invalid disk size %q (expected a quantity like 10Gi, 500Mi, or 1Ti): %w", size, err, + ) } return kubevirtv1.DataVolumeTemplateSpec{ ObjectMeta: metav1.ObjectMeta{ diff --git a/internal/vm/vm_integration_test.go b/internal/vm/vm_integration_test.go index ffbfa62..c707a92 100644 --- a/internal/vm/vm_integration_test.go +++ b/internal/vm/vm_integration_test.go @@ -98,7 +98,7 @@ var _ = Describe("DeleteVM [integration]", func() { Expect(err).NotTo(HaveOccurred()) Expect(vm.CreateVM(ctx, c, vmObj)).To(Succeed()) - err := vm.DeleteVM(ctx, c, "integ-vm-del", namespace) + err = vm.DeleteVM(ctx, c, "integ-vm-del", namespace) Expect(err).NotTo(HaveOccurred()) // KubeVirt finalizers keep the VM in Terminating state briefly. diff --git a/internal/wait/wait_integration_test.go b/internal/wait/wait_integration_test.go index d909ff7..54f2a3b 100644 --- a/internal/wait/wait_integration_test.go +++ b/internal/wait/wait_integration_test.go @@ -44,7 +44,7 @@ var _ = Describe("WaitForVMReady [integration]", Label("slow"), func() { Expect(err).NotTo(HaveOccurred()) Expect(vm.CreateVM(ctx, c, vmObj)).To(Succeed()) - err := wait.WaitForVMReady(ctx, c, slog.Default(), "wait-vm-0", namespace, 5*time.Minute, 5*time.Second) + err = wait.WaitForVMReady(ctx, c, slog.Default(), "wait-vm-0", namespace, 5*time.Minute, 5*time.Second) Expect(err).NotTo(HaveOccurred()) })