From 2d315c3fd6babd91bef82f660931e9b2502275c0 Mon Sep 17 00:00:00 2001 From: Melvin Hillsman Date: Sat, 23 May 2026 14:52:35 -0500 Subject: [PATCH 1/4] fix(workloads): return errors from TPS file-size parser instead of silent fallback fileSizeBytes() and fileSizeMBCount() silently fell back to default values on parse failure, masking configuration errors. Extract a shared parseFileSize() that validates format (positive integer + K/M/G suffix) and return errors through buildServerUserdata/buildClientUserdata so invalid file-size params fail fast at the orchestration layer. Signed-off-by: Melvin Hillsman --- internal/workloads/tps.go | 76 +++++++++++++++++++++++----------- internal/workloads/tps_test.go | 44 ++++++++++++++++++++ 2 files changed, 96 insertions(+), 24 deletions(-) diff --git a/internal/workloads/tps.go b/internal/workloads/tps.go index 4485916..9e988dd 100644 --- a/internal/workloads/tps.go +++ b/internal/workloads/tps.go @@ -45,6 +45,7 @@ WantedBy=multi-user.target ` var ErrUnknownTPSRole = errors.New("unexpected tps role; expected 'server' or 'client'") +var ErrInvalidFileSize = errors.New("invalid file-size format: must be a positive integer followed by K, M, or G (e.g. \"10M\", \"1G\", \"512K\")") // TPSWorkload generates cloud-init userdata for a combined TPS benchmark. // It creates server/client VM pairs. The server runs netserver and python3 @@ -187,49 +188,71 @@ func (w *TPSWorkload) duration() string { return "60" } -func (w *TPSWorkload) fileSizeBytes() string { - raw := w.fileSize() +func parseFileSize(raw string) (num int, suffix string, err error) { raw = strings.TrimSpace(raw) - suffix := raw[len(raw)-1:] + if len(raw) < 2 { + return 0, "", fmt.Errorf("%w: %q", ErrInvalidFileSize, raw) + } + suffix = strings.ToUpper(raw[len(raw)-1:]) numStr := raw[:len(raw)-1] - num, err := strconv.Atoi(numStr) + + switch suffix { + case "K", "M", "G": + default: + return 0, "", fmt.Errorf("%w: %q (unrecognized suffix %q)", ErrInvalidFileSize, raw, raw[len(raw)-1:]) + } + + num, err = strconv.Atoi(numStr) if err != nil { - return "10485760" // 10M fallback + return 0, "", fmt.Errorf("%w: %q (%q is not a valid integer)", ErrInvalidFileSize, raw, numStr) } - switch strings.ToUpper(suffix) { + if num <= 0 { + return 0, "", fmt.Errorf("%w: %q (value must be positive)", ErrInvalidFileSize, raw) + } + + return num, suffix, nil +} + +func (w *TPSWorkload) fileSizeBytes() (string, error) { + num, suffix, err := parseFileSize(w.fileSize()) + if err != nil { + return "", err + } + switch suffix { case "G": - return strconv.Itoa(num * 1024 * 1024 * 1024) + return strconv.Itoa(num * 1024 * 1024 * 1024), nil case "M": - return strconv.Itoa(num * 1024 * 1024) + return strconv.Itoa(num * 1024 * 1024), nil case "K": - return strconv.Itoa(num * 1024) + return strconv.Itoa(num * 1024), nil default: - return raw + return "", fmt.Errorf("%w: %q", ErrInvalidFileSize, w.fileSize()) } } -func (w *TPSWorkload) fileSizeMBCount() string { - raw := w.fileSize() - raw = strings.TrimSpace(raw) - suffix := raw[len(raw)-1:] - numStr := raw[:len(raw)-1] - num, err := strconv.Atoi(numStr) +func (w *TPSWorkload) fileSizeMBCount() (string, error) { + num, suffix, err := parseFileSize(w.fileSize()) if err != nil { - return "10" + return "", err } - switch strings.ToUpper(suffix) { + switch suffix { case "G": - return strconv.Itoa(num * 1024) + return strconv.Itoa(num * 1024), nil case "M": - return numStr + return strconv.Itoa(num), nil case "K": - return "1" // minimum 1MB + return "1", nil default: - return raw + return "", fmt.Errorf("%w: %q", ErrInvalidFileSize, w.fileSize()) } } func (w *TPSWorkload) buildServerUserdata() (string, error) { + mbCount, err := w.fileSizeMBCount() + if err != nil { + return "", fmt.Errorf("tps server: %w", err) + } + serverScript := fmt.Sprintf(`#!/usr/bin/env bash set -e @@ -252,7 +275,7 @@ echo " HTTP: port 8080 (pid $HTTP_PID)" echo " testfile: /srv/virtwork/testfile (%s)" wait -`, w.fileSize(), w.fileSizeMBCount(), w.fileSize()) +`, w.fileSize(), mbCount, w.fileSize()) return w.BuildCloudConfig(CloudConfigOpts{ Packages: []string{"netperf", "python3"}, @@ -276,6 +299,11 @@ wait } func (w *TPSWorkload) buildClientUserdata(namespace string) (string, error) { + fileSizeBytes, err := w.fileSizeBytes() + if err != nil { + return "", fmt.Errorf("tps client: %w", err) + } + dnsName := w.serverDNSName(namespace) clientScript := fmt.Sprintf(`#!/usr/bin/env bash @@ -364,7 +392,7 @@ done echo "==========================================" echo " TPS testing complete" echo "==========================================" -`, dnsName, w.iterations(), w.duration(), w.fileSizeBytes(), w.fileSize()) +`, dnsName, w.iterations(), w.duration(), fileSizeBytes, w.fileSize()) return w.BuildCloudConfig(CloudConfigOpts{ Packages: []string{"netperf", "curl"}, diff --git a/internal/workloads/tps_test.go b/internal/workloads/tps_test.go index 8c62ac1..acc2d18 100644 --- a/internal/workloads/tps_test.go +++ b/internal/workloads/tps_test.go @@ -368,6 +368,50 @@ var _ = Describe("TPSWorkload", func() { }) }) + Context("invalid file-size params", func() { + DescribeTable("should return error for malformed file-size", + func(fileSize string) { + w2 := workloads.NewTPSWorkload(config.WorkloadConfig{ + Enabled: config.BoolPtr(true), + VMCount: 1, + CPUCores: 2, + Memory: "2Gi", + Params: map[string]string{"file-size": fileSize}, + }, "virtwork", "virtwork", "", nil) + + _, err := w2.UserdataForRole("server", "virtwork") + Expect(err).To(HaveOccurred()) + Expect(err).To(MatchError(workloads.ErrInvalidFileSize)) + + _, err = w2.UserdataForRole("client", "virtwork") + Expect(err).To(HaveOccurred()) + Expect(err).To(MatchError(workloads.ErrInvalidFileSize)) + }, + Entry("decimal number", "1.5G"), + Entry("multi-char suffix GB", "2GB"), + Entry("binary suffix MiB", "10MiB"), + Entry("suffix only, no number", "M"), + Entry("unknown suffix", "10X"), + Entry("no valid structure", "garbage"), + Entry("zero value", "0M"), + Entry("negative value", "-5M"), + ) + + It("should propagate file-size error through CloudInitUserdata", func() { + w2 := workloads.NewTPSWorkload(config.WorkloadConfig{ + Enabled: config.BoolPtr(true), + VMCount: 1, + CPUCores: 2, + Memory: "2Gi", + Params: map[string]string{"file-size": "garbage"}, + }, "virtwork", "virtwork", "", nil) + + _, err := w2.CloudInitUserdata() + Expect(err).To(HaveOccurred()) + Expect(err).To(MatchError(workloads.ErrInvalidFileSize)) + }) + }) + Context("configurable params", func() { It("should default file size to 10M", func() { result, err := w.UserdataForRole("server", "virtwork") From 3c8e5da0d6a216ba2582549c6ec30448d7f6e84b Mon Sep 17 00:00:00 2001 From: Melvin Hillsman Date: Sat, 23 May 2026 14:53:02 -0500 Subject: [PATCH 2/4] fix(golden-image): add netperf and python3 to container disk image The tps workload server requires netperf and python3 but they were missing from the golden-image pre-install list, causing cloud-init to install them on every boot. Add both packages and verify netserver and python3 binaries are present. Signed-off-by: Melvin Hillsman --- build/golden-image/Containerfile | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/build/golden-image/Containerfile b/build/golden-image/Containerfile index f0b7376..f112004 100644 --- a/build/golden-image/Containerfile +++ b/build/golden-image/Containerfile @@ -21,6 +21,8 @@ RUN dnf install -y \ postgresql-server \ iproute-tc \ iptables-nft \ + netperf \ + python3 \ && dnf clean all \ && rm -rf /var/cache/dnf @@ -30,7 +32,9 @@ RUN which stress-ng && \ which iperf3 && \ which pgbench && \ which tc && \ - which iptables-nft + which iptables-nft && \ + which netserver && \ + which python3 # Expose metadata for KubeVirt compatibility # The containerdisk format expects the disk image to be at /disk/ From ada8d07161c168f76bb4b1e8d4e46093dec440c7 Mon Sep 17 00:00:00 2001 From: Melvin Hillsman Date: Sat, 23 May 2026 14:53:37 -0500 Subject: [PATCH 3/4] test(e2e): add TPS workload deployment and cleanup tests Verify that the tps workload deploys both server and client VMs, creates the three-port service (netperf-ctrl, netperf-data, http-data), and cleans up all resources. Covers the multi-VM deployment path that was previously untested end-to-end. Signed-off-by: Melvin Hillsman --- tests/e2e/tps_test.go | 78 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 78 insertions(+) create mode 100644 tests/e2e/tps_test.go diff --git a/tests/e2e/tps_test.go b/tests/e2e/tps_test.go new file mode 100644 index 0000000..75e89d4 --- /dev/null +++ b/tests/e2e/tps_test.go @@ -0,0 +1,78 @@ +//go:build e2e + +// Copyright 2026 Red Hat +// SPDX-License-Identifier: Apache-2.0 + +package e2e_test + +import ( + "context" + "time" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + + "github.com/opdev/virtwork/internal/testutil" + "github.com/opdev/virtwork/internal/vm" +) + +var _ = Describe("TPS workload", Label("slow"), func() { + var namespace string + + BeforeEach(func() { + namespace = testutil.UniqueNamespace("e2e-tps") + }) + + AfterEach(func() { + _, _, _, _ = testutil.RunVirtwork("cleanup", + "--namespace", namespace, "--delete-namespace") + }) + + It("should deploy server and client VMs with a service", func() { + stdout, _, exitCode, err := testutil.RunVirtwork( + "run", "--workloads", "tps", "--vm-count", "1", + "--namespace", namespace, "--no-wait") + Expect(err).NotTo(HaveOccurred()) + Expect(exitCode).To(Equal(0)) + Expect(stdout).To(ContainSubstring("virtwork-tps-server-0")) + Expect(stdout).To(ContainSubstring("virtwork-tps-client-0")) + + c := testutil.MustConnect("") + ctx := context.Background() + + svc := &corev1.Service{} + err = c.Get(ctx, client.ObjectKey{ + Name: "virtwork-tps-server", Namespace: namespace, + }, svc) + Expect(err).NotTo(HaveOccurred()) + Expect(svc.Spec.Ports).To(HaveLen(3)) + + vms, err := vm.ListVMs(ctx, c, namespace, testutil.ManagedLabels()) + Expect(err).NotTo(HaveOccurred()) + Expect(vms).To(HaveLen(2)) + }) + + It("should clean up all TPS resources", func() { + _, _, exitCode, err := testutil.RunVirtwork( + "run", "--workloads", "tps", "--vm-count", "1", + "--namespace", namespace, "--no-wait") + Expect(err).NotTo(HaveOccurred()) + Expect(exitCode).To(Equal(0)) + + stdout, _, exitCode, err := testutil.RunVirtwork( + "cleanup", "--namespace", namespace) + Expect(err).NotTo(HaveOccurred()) + Expect(exitCode).To(Equal(0)) + Expect(stdout).To(ContainSubstring(`"vms_deleted":2`)) + + c := testutil.MustConnect("") + ctx := context.Background() + Eventually(func() int { + vms, err := vm.ListVMs(ctx, c, namespace, testutil.ManagedLabels()) + Expect(err).NotTo(HaveOccurred()) + return len(vms) + }, 60*time.Second, 2*time.Second).Should(Equal(0)) + }) +}) From 3d7cfc62cde26ec809aa621c0469c4e6ba16fd7c Mon Sep 17 00:00:00 2001 From: Melvin Hillsman Date: Sat, 23 May 2026 14:53:46 -0500 Subject: [PATCH 4/4] docs: document exact file-size format for tps workload params Clarify that file-size must be a positive integer followed by K, M, or G and that decimal values and binary suffixes are not supported. Signed-off-by: Melvin Hillsman --- docs/configuration.md | 2 +- internal/workloads/tps.go | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index d3c1f3e..33b3524 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -198,7 +198,7 @@ Each workload's `params` block accepts string-valued keys. The current parameter | Workload | Key | Default | Effect | |---|---|---|---| -| **tps** | `file-size` | `10M` | Size of the HTTP test file (suffix `K`/`M`/`G`) | +| **tps** | `file-size` | `10M` | Size of the HTTP test file — a positive integer followed by `K`, `M`, or `G` (e.g. `512K`, `10M`, `1G`). Decimal values and binary suffixes like `MiB` are not supported. | | **tps** | `iterations` | `30` | Number of test iterations | | **tps** | `duration` | `60` | Seconds per iteration | | **chaos-disk** | `mount` | `/mnt/data` | Mountpoint of the data disk to fill | diff --git a/internal/workloads/tps.go b/internal/workloads/tps.go index 9e988dd..de552a8 100644 --- a/internal/workloads/tps.go +++ b/internal/workloads/tps.go @@ -45,7 +45,10 @@ WantedBy=multi-user.target ` var ErrUnknownTPSRole = errors.New("unexpected tps role; expected 'server' or 'client'") -var ErrInvalidFileSize = errors.New("invalid file-size format: must be a positive integer followed by K, M, or G (e.g. \"10M\", \"1G\", \"512K\")") + +var ErrInvalidFileSize = errors.New( + "invalid file-size format: must be a positive integer followed by K, M, or G (e.g. '10M')", +) // TPSWorkload generates cloud-init userdata for a combined TPS benchmark. // It creates server/client VM pairs. The server runs netserver and python3