From d45eb9bfec58d66663c19ee24de1c1872b886657 Mon Sep 17 00:00:00 2001 From: Yan Bondarenko <202671653+yankawai@users.noreply.github.com> Date: Wed, 23 Sep 2026 01:17:44 +0300 Subject: [PATCH] feat(kubevirt): expose migration configuration through platform values Backport of #4254 to release-1.6. Cluster-wide migration settings could only be applied by patching the KubeVirt CR: the kubevirt chart did not render `migrations`, and the platform bundle did not forward it. The optional `kubevirt.migrations` platform value now passes through the generated Package to `spec.configuration.migrations`, and a hand patch of that Package is undone on the next platform render, so this hop is the only supported setter. Four differences from main, all from release-1.6 predating the disabledFeatureGates work: - platform values gains a `kubevirt` block holding `migrations` alone, and the iaas bundle gains only the migrations hop; - the wiring test takes the four migrations cases and the case for a null `kubevirt` block, without the assertions that belong to disabledFeatureGates cases absent here; - update_idempotency_test.sh gains `run_update_logged`, which arrived on main alongside that work and the new cases need; - the kubevirt Makefile header counts six sed patches rather than five. Verified on this branch: 9 kubevirt chart tests, 31 platform wiring tests (102 across the platform suite), and update_idempotency_test PASS under GNU make and sed. Removing the new awk guard from the Makefile turns that test red naming the migrations block, so the ported guard is not decorative. On this line (.Values.kubevirt).migrations is the only reader of .Values.kubevirt, and rewriting it as .Values.kubevirt.migrations turns the null-block case red with a nil pointer. The automated backport in #4322 stopped with conflict markers in four files as its only commit, which is why this exists. Assisted-by: LLM Signed-off-by: Yan Bondarenko <202671653+yankawai@users.noreply.github.com> --- .../core/platform/templates/bundles/iaas.yaml | 11 ++ .../bundles_gpu_kubevirt_wiring_test.yaml | 112 ++++++++++++++++++ packages/core/platform/values.yaml | 8 ++ packages/system/kubevirt/Makefile | 11 +- .../kubevirt/templates/kubevirt-cr.yaml | 4 + .../kubevirt/tests/kubevirt_cr_test.yaml | 42 +++++++ .../kubevirt/tests/update_idempotency_test.sh | 92 +++++++++++++- packages/system/kubevirt/values.yaml | 7 ++ 8 files changed, 285 insertions(+), 2 deletions(-) diff --git a/packages/core/platform/templates/bundles/iaas.yaml b/packages/core/platform/templates/bundles/iaas.yaml index 25215c321..1f8f11b8b 100644 --- a/packages/core/platform/templates/bundles/iaas.yaml +++ b/packages/core/platform/templates/bundles/iaas.yaml @@ -6,6 +6,17 @@ {{- if .Values.resources.cpuAllocationRatio -}} {{- $_ := set $kubevirtValues "cpuAllocationRatio" (.Values.resources.cpuAllocationRatio | int) -}} {{- end -}} +{{- /* + KubeVirt CR settings that no other platform knob implies, forwarded + verbatim. This include is the only writer of the cozystack.kubevirt Package, + and patching that Package by hand is reverted the next time the platform + re-renders it, so without this hop the chart value has no supported setter. + The parenthesized walk is nil-safe for the reason the gpu block below spells + out. +*/ -}} +{{- with (.Values.kubevirt).migrations -}} +{{- $_ := set $kubevirtValues "migrations" . -}} +{{- end -}} {{- /* When cozystack.gpu-operator is enabled the platform mirrors the variant into the KubeVirt CR: the HostDevices feature gate plus a starter diff --git a/packages/core/platform/tests/bundles_gpu_kubevirt_wiring_test.yaml b/packages/core/platform/tests/bundles_gpu_kubevirt_wiring_test.yaml index 9f6077ff9..7e06f6bf2 100644 --- a/packages/core/platform/tests/bundles_gpu_kubevirt_wiring_test.yaml +++ b/packages/core/platform/tests/bundles_gpu_kubevirt_wiring_test.yaml @@ -386,6 +386,118 @@ tests: path: spec.components.kubevirt.values.cpuAllocationRatio value: 7 + - it: forwards kubevirt.migrations into the kubevirt component values + set: + bundles: + system: + enabled: true + variant: isp-full + iaas: + enabled: true + enabledPackages: [] + kubevirt: + migrations: + bandwidthPerMigration: 625M + parallelMigrationsPerCluster: 2 + parallelOutboundMigrationsPerNode: 1 + documentSelector: + path: metadata.name + value: cozystack.kubevirt + asserts: + - equal: + path: spec.components.kubevirt.values + value: + cpuAllocationRatio: 10 + migrations: + bandwidthPerMigration: 625M + parallelMigrationsPerCluster: 2 + parallelOutboundMigrationsPerNode: 1 + + - it: omits empty kubevirt.migrations + set: + bundles: + system: + enabled: true + variant: isp-full + iaas: + enabled: true + enabledPackages: [] + kubevirt: + migrations: {} + documentSelector: + path: metadata.name + value: cozystack.kubevirt + asserts: + - equal: + path: spec.components.kubevirt.values + value: + cpuAllocationRatio: 10 + + - it: omits null kubevirt.migrations + set: + bundles: + system: + enabled: true + variant: isp-full + iaas: + enabled: true + enabledPackages: [] + kubevirt: + migrations: null + documentSelector: + path: metadata.name + value: cozystack.kubevirt + asserts: + - equal: + path: spec.components.kubevirt.values + value: + cpuAllocationRatio: 10 + + - it: renders the kubevirt Package without a nil-pointer error when the whole kubevirt block is set to null + set: + bundles: + system: + enabled: true + variant: isp-full + iaas: + enabled: true + enabledPackages: [] + kubevirt: null + documentSelector: + path: metadata.name + value: cozystack.kubevirt + asserts: + - equal: + path: kind + value: Package + - notExists: + path: spec.components.kubevirt.values.migrations + + - it: preserves false and zero kubevirt.migrations values + set: + bundles: + system: + enabled: true + variant: isp-full + iaas: + enabled: true + enabledPackages: [] + kubevirt: + migrations: + allowAutoConverge: false + completionTimeoutPerGiB: 0 + documentSelector: + path: metadata.name + value: cozystack.kubevirt + asserts: + - equal: + path: spec.components.kubevirt.values + value: + cpuAllocationRatio: 10 + migrations: + allowAutoConverge: false + completionTimeoutPerGiB: 0 + - it: appends user-supplied pciHostDevices in vgpu mode so post-kubevirt#16890 SR-IOV vGPU VFs can be advertised alongside the platform mediated defaults set: bundles: diff --git a/packages/core/platform/values.yaml b/packages/core/platform/values.yaml index d5d27e391..d10ca3633 100644 --- a/packages/core/platform/values.yaml +++ b/packages/core/platform/values.yaml @@ -511,6 +511,14 @@ resources: cpuAllocationRatio: 10 memoryAllocationRatio: 1 ephemeralStorageAllocationRatio: 40 +# KubeVirt CR settings that no other platform knob implies. Forwarded into the +# emitted cozystack.kubevirt Package CR as components.kubevirt.values, next to +# the entries the iaas bundle derives from resources.cpuAllocationRatio and +# from the gpu block below. +kubevirt: + # Passed to spec.configuration.migrations on the KubeVirt CR. + # Empty keeps KubeVirt's defaults. bandwidthPerMigration is bytes/second. + migrations: {} # GPU host-device wiring for VirtualMachine workloads. Auto-injected into the # KubeVirt CR's permittedHostDevices when bundles.enabledPackages contains # cozystack.gpu-operator. Without it the admission webhook rejects any VM diff --git a/packages/system/kubevirt/Makefile b/packages/system/kubevirt/Makefile index 516da1838..efb2cf541 100644 --- a/packages/system/kubevirt/Makefile +++ b/packages/system/kubevirt/Makefile @@ -27,7 +27,7 @@ CR_FILE ?= templates/kubevirt-cr.yaml # 1. Operator inspects the new upstream release notes for spec changes # that affect the cozystack-managed fields listed above. # 2. Operator hand-merges the new fields into templates/kubevirt-cr.yaml. -# 3. Operator runs `make update` to re-apply the five cozystack +# 3. Operator runs `make update` to re-apply the six cozystack # parameterization sed patches in case they were lost during the # hand-merge. # @@ -46,6 +46,7 @@ update: @# sanity-check tail below. grep -q '{{- with .Values.permittedHostDevices }}' $(CR_FILE) || sed -i '/^ evictionStrategy:/i\ {{- with .Values.permittedHostDevices }}\n permittedHostDevices:\n {{- toYaml . | nindent 6 }}\n {{- end }}' $(CR_FILE) grep -q '{{- with .Values.mediatedDevicesConfiguration }}' $(CR_FILE) || sed -i '/^ evictionStrategy:/i\ {{- with .Values.mediatedDevicesConfiguration }}\n mediatedDevicesConfiguration:\n {{- toYaml . | nindent 6 }}\n {{- end }}' $(CR_FILE) + grep -q '{{- with .Values.migrations }}' $(CR_FILE) || sed -i '/^ evictionStrategy:/i\ {{- with .Values.migrations }}\n migrations:\n {{- toYaml . | nindent 6 }}\n {{- end }}' $(CR_FILE) @# Sanity-check: every cozystack patch MUST end up in the file. If the @# hand-merge step moved or renamed an anchor (developerConfiguration:, @# - VMExport, evictionStrategy:) the sed silently no-ops and the chart @@ -56,3 +57,11 @@ update: @grep -q '{{- range .Values.extraFeatureGates }}' $(CR_FILE) || { echo "ERROR: directive '{{- range .Values.extraFeatureGates }}' not inserted — restore the '- VMExport' anchor in $(CR_FILE) and re-run" >&2; exit 1; } @grep -q '{{- with .Values.permittedHostDevices }}' $(CR_FILE) || { echo "ERROR: directive '{{- with .Values.permittedHostDevices }}' not inserted — restore the 'evictionStrategy:' anchor in $(CR_FILE) and re-run" >&2; exit 1; } @grep -q '{{- with .Values.mediatedDevicesConfiguration }}' $(CR_FILE) || { echo "ERROR: directive '{{- with .Values.mediatedDevicesConfiguration }}' not inserted — restore the 'evictionStrategy:' anchor in $(CR_FILE) and re-run" >&2; exit 1; } + @grep -q '{{- with .Values.migrations }}' $(CR_FILE) || { echo "ERROR: directive '{{- with .Values.migrations }}' not inserted — restore the 'evictionStrategy:' anchor in $(CR_FILE) and re-run" >&2; exit 1; } + @# A retained directive skips insertion even if its body or end was lost. + @# Check the whole block in order; only END decides the exit status. + @awk '\ + $$0 == " migrations:" { keys++ } \ + $$0 == " {{- with .Values.migrations }}" { blocks++; step=1; next } \ + step { expected = step == 1 ? " migrations:" : step == 2 ? " {{- toYaml . | nindent 6 }}" : " {{- end }}"; if ($$0 != expected) bad=1; step=(step+1)%4 } \ + END { exit !(blocks == 1 && keys == 1 && step == 0 && !bad) }' $(CR_FILE) || { echo "ERROR: migrations block is partial or duplicated — restore the complete block or delete all its remaining lines from $(CR_FILE) and re-run" >&2; exit 1; } diff --git a/packages/system/kubevirt/templates/kubevirt-cr.yaml b/packages/system/kubevirt/templates/kubevirt-cr.yaml index 10754aad3..6b7c0b06d 100644 --- a/packages/system/kubevirt/templates/kubevirt-cr.yaml +++ b/packages/system/kubevirt/templates/kubevirt-cr.yaml @@ -32,6 +32,10 @@ spec: mediatedDevicesConfiguration: {{- toYaml . | nindent 6 }} {{- end }} + {{- with .Values.migrations }} + migrations: + {{- toYaml . | nindent 6 }} + {{- end }} evictionStrategy: LiveMigrate virtualMachineOptions: disableSerialConsoleLog: {} diff --git a/packages/system/kubevirt/tests/kubevirt_cr_test.yaml b/packages/system/kubevirt/tests/kubevirt_cr_test.yaml index eda3218ca..556524e67 100644 --- a/packages/system/kubevirt/tests/kubevirt_cr_test.yaml +++ b/packages/system/kubevirt/tests/kubevirt_cr_test.yaml @@ -23,6 +23,48 @@ tests: path: spec.configuration.permittedHostDevices - notExists: path: spec.configuration.mediatedDevicesConfiguration + - notExists: + path: spec.configuration.migrations + + - it: omits migrations when explicitly empty + set: + migrations: {} + asserts: + - notExists: + path: spec.configuration.migrations + + - it: omits migrations when explicitly null + set: + migrations: null + asserts: + - notExists: + path: spec.configuration.migrations + + - it: renders migration limits without changing their values + set: + migrations: + bandwidthPerMigration: 625M + parallelMigrationsPerCluster: 2 + parallelOutboundMigrationsPerNode: 1 + asserts: + - equal: + path: spec.configuration.migrations + value: + bandwidthPerMigration: 625M + parallelMigrationsPerCluster: 2 + parallelOutboundMigrationsPerNode: 1 + + - it: preserves false and zero migration values + set: + migrations: + allowAutoConverge: false + completionTimeoutPerGiB: 0 + asserts: + - equal: + path: spec.configuration.migrations + value: + allowAutoConverge: false + completionTimeoutPerGiB: 0 - it: emits cpuAllocationRatio under developerConfiguration when set set: diff --git a/packages/system/kubevirt/tests/update_idempotency_test.sh b/packages/system/kubevirt/tests/update_idempotency_test.sh index 8bf373d40..a698d8016 100755 --- a/packages/system/kubevirt/tests/update_idempotency_test.sh +++ b/packages/system/kubevirt/tests/update_idempotency_test.sh @@ -50,11 +50,17 @@ run_update() { make -C "$pkg" update CR_FILE="$1" >/dev/null 2>&1 } +# Same call, but keeps stdout+stderr so a case can assert on the diagnostic. +run_update_logged() { + make -C "$pkg" update CR_FILE="$1" >"$2" 2>&1 +} + tmpdir="$(mktemp -d)" trap 'rm -rf "$tmpdir"' EXIT PHD='{{- with .Values.permittedHostDevices }}' MDC='{{- with .Values.mediatedDevicesConfiguration }}' +MIG='{{- with .Values.migrations }}' # --- case 1: a hand-merge dropped ONLY the mediatedDevicesConfiguration block. # `make update` must reinsert it without duplicating permittedHostDevices. @@ -96,8 +102,92 @@ for guard in \ '{{- if .Values.cpuAllocationRatio }}' \ '{{- range .Values.extraFeatureGates }}' \ "$PHD" \ - "$MDC"; do + "$MDC" \ + "$MIG"; do assert_count 1 "$guard" "$missing_mdev" done +# A hand-merge may drop migrations while preserving both device blocks. +# Restoring it must reproduce the committed template, including the YAML key +# and indentation, rather than merely reintroduce the directive. +missing_migrations="$tmpdir/missing-migrations.yaml" +sed '/{{- with .Values.migrations }}/,/{{- end }}/d' "$src" >"$missing_migrations" +assert_absent "$MIG" "$missing_migrations" +assert_present "$PHD" "$missing_migrations" +assert_present "$MDC" "$missing_migrations" +if ! run_update "$missing_migrations"; then + fail "make update exited non-zero on a template missing only migrations" +fi +assert_present "$MIG" "$missing_migrations" +if ! diff -u "$src" "$missing_migrations" >/dev/null; then + fail "make update rebuilt migrations differently from the committed template" +fi +run_update "$missing_migrations" +assert_count 1 "$MIG" "$missing_migrations" +if ! diff -u "$src" "$missing_migrations" >/dev/null; then + fail "repeated make update changed the restored migrations template" +fi + +# sed succeeds even when its insertion anchor is absent; the target's sanity +# check must reject a hand-merged CR that cannot restore migration settings. +anchorless_migrations="$tmpdir/anchorless-migrations.yaml" +sed -e '/{{- with .Values.migrations }}/,/{{- end }}/d' \ + -e '/^ evictionStrategy:/d' "$src" >"$anchorless_migrations" +anchorless_migrations_log="$tmpdir/anchorless-migrations.log" +if run_update_logged "$anchorless_migrations" "$anchorless_migrations_log"; then + fail "make update exited zero without the migrations block or its evictionStrategy anchor" +fi +if ! grep -qF "directive '$MIG' not inserted" "$anchorless_migrations_log"; then + fail "make update did not name the missing migrations directive: $(cat "$anchorless_migrations_log")" +fi + +# A retained opening directive must not hide a missing body or terminator. +# The opening-less case also leaves an orphan key: inserting a second complete +# block must not make that damaged template pass validation. +for missing in key payload end body opening; do + partial_migrations="$tmpdir/migrations-missing-$missing.yaml" + awk -v missing="$missing" -v opening="$MIG" ' + index($0, opening) { inside=1; if (missing != "opening") print; next } + inside && /{{- end }}/ { inside=0; if (missing != "end") print; next } + inside && /^ migrations:$/ && (missing == "key" || missing == "body") { next } + inside && /{{- toYaml \. \| nindent 6 }}/ && (missing == "payload" || missing == "body") { next } + { print } + ' "$src" >"$partial_migrations" + partial_migrations_log="$tmpdir/migrations-missing-$missing.log" + if run_update_logged "$partial_migrations" "$partial_migrations_log"; then + fail "make update exited zero on a migrations block missing $missing" + fi + if ! grep -qF 'migrations block is partial or duplicated' "$partial_migrations_log"; then + fail "make update did not diagnose migrations missing $missing: $(cat "$partial_migrations_log")" + fi +done + +# Presence checks can also accept reordered lines, a duplicate complete block, +# or a complete body with EOF in place of its closing directive. +for malformed in reordered duplicate eof; do + malformed_migrations="$tmpdir/migrations-$malformed.yaml" + awk -v malformed="$malformed" -v opening="$MIG" ' + index($0, opening) { inside=1; block=$0 ORS; print; next } + inside { + block=block $0 ORS + if (/{{- end }}/) { + if (malformed == "eof") exit + inside=0; print + if (malformed == "duplicate") printf "%s", block + next + } + if (malformed == "reordered" && /^ migrations:$/) { print " {{- toYaml . | nindent 6 }}"; next } + if (malformed == "reordered" && /{{- toYaml \. \| nindent 6 }}/) { print " migrations:"; next } + } + { print } + ' "$src" >"$malformed_migrations" + malformed_migrations_log="$tmpdir/migrations-$malformed.log" + if run_update_logged "$malformed_migrations" "$malformed_migrations_log"; then + fail "make update exited zero on a $malformed migrations block" + fi + if ! grep -qF 'migrations block is partial or duplicated' "$malformed_migrations_log"; then + fail "make update did not diagnose $malformed migrations: $(cat "$malformed_migrations_log")" + fi +done + echo "PASS update_idempotency_test" diff --git a/packages/system/kubevirt/values.yaml b/packages/system/kubevirt/values.yaml index 5a8bb493b..57d1beefc 100644 --- a/packages/system/kubevirt/values.yaml +++ b/packages/system/kubevirt/values.yaml @@ -46,3 +46,10 @@ permittedHostDevices: {} # mediatedDeviceTypes: # - nvidia-740 mediatedDevicesConfiguration: {} + +# Passed to spec.configuration.migrations on the KubeVirt CR. +# Empty leaves KubeVirt's migration defaults unchanged. +# bandwidthPerMigration is a quantity in bytes per second. +# Set through spec.components.platform.values.kubevirt.migrations +# on the cozystack.cozystack-platform Package. +migrations: {}