fix(kubevirt-instancetypes): drop persistent strip that produced null TPM - #2612
fix(kubevirt-instancetypes): drop persistent strip that produced null TPM#2612myasnikovdaniil wants to merge 2 commits into
Conversation
… TPM
`make update` stripped `persistent: true` from upstream's
`preferredTPM: { persistent: true }`, leaving the parent key with a null value.
KubeVirt v1.6's CRD tolerated it; v1.8 (the operator version we bumped to in
6af0bf7) enforces structural schema and rejects null:
VirtualMachineClusterPreference "windows.11" is invalid:
spec.devices.preferredTPM: Invalid value: "null": spec.devices.preferredTPM
in body must be of type object: "null"
Stop stripping the line and regenerate the templates from upstream
common-instancetypes. Persistent TPM state falls back to the cluster default
StorageClass with RWO access mode when `vmStateStorageClass` is unset on the
KubeVirt CR, per pkg/storage/backend-storage/backend-storage.go.
Drive-by from the regen:
- Removed (upstream EOL): centos.7*, centos.stream8*
- Added: debian, oraclelinux, legacy, linux*, fedora.s390x, rhel.{9,10}.s390x,
windows.{xp,7,7.virtio,2k3,2k8,2k8.virtio,2k12,2k12.virtio}, plus new
cx1.*1gi and d1 instancetype variants.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a critical issue introduced by the KubeVirt operator upgrade to v1.8.2, where the stricter CRD schema rejected null values in the 'preferredTPM' field. By removing an outdated post-processing step in the Makefile and refreshing the upstream templates, the operator now correctly handles TPM configurations. Furthermore, this update aligns the project with the latest upstream instancetype definitions, adding support for modern OS versions and new hardware variants while cleaning up deprecated configurations. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request updates KubeVirt instance types and preferences, introducing the D series (Dedicated vCPU), expanding M and CX series variants, and adding numerous OS preferences for various Linux distributions and Windows versions. Key changes include specific hugepage configurations, the addition of maxSockets to prevent CPU hotplugging issues, and persistence settings for TPM/EFI on modern Windows types. Feedback indicates that structural modifications to the generated template files (instancetypes.yaml and preferences.yaml) must be moved to patch files in the patches/ directory and applied via the Makefile to comply with Rule 21 of the Repository Style Guide and ensure changes are preserved during future regenerations.
| the IO threading from cores dedicated to the workload. | ||
| In addition, in this series, the NUMA topology of the used | ||
| cores is provided to the VM. | ||
|
|
||
| The amount of spec.cpu.maxSockets is set to the same as | ||
| guest cpus. This avoids KubeVirt's default behavior of | ||
| providing extra CPUs for hotplugging, which can exceed the | ||
| amount of available CPUs on a node and cause the VM to fail. | ||
|
|
||
| Requirements for CX series instance types: | ||
| - CPU manager has to be enabled. | ||
| - Huge pages have to be available on the nodes. |
There was a problem hiding this comment.
Structural changes to generated files should be applied via a patch file in packages/system/kubevirt-instancetypes/patches/ and applied by the Makefile to ensure they are not lost during regeneration. Please refer to the Repository Style Guide, Rule 21.
References
- Rule 21: For structural changes: suggest a patch file in
packages/<name>/patches/applied by the Makefile. (link) - Rule 21: For structural changes: suggest a patch file in
packages/<name>/patches/applied by the Makefile.
| annotations: | ||
| iconClass: icon-alpine | ||
| openshift.io/display-name: Alpine | ||
| openshift.io/documentation-url: https://github.com/kubevirt/common-instancetypes | ||
| openshift.io/documentation-url: | | ||
| "https://github.com/kubevirt/common-instancetypes" | ||
| openshift.io/provider-display-name: KubeVirt | ||
| openshift.io/support-url: https://github.com/kubevirt/common-instancetypes/issues | ||
| openshift.io/support-url: | | ||
| "https://github.com/kubevirt/common-instancetypes/issues" | ||
| tags: hidden,kubevirt,alpine |
There was a problem hiding this comment.
Structural changes to generated files should be applied via a patch file in packages/system/kubevirt-instancetypes/patches/ and applied by the Makefile to ensure they are not lost during regeneration. Please refer to the Repository Style Guide, Rule 21.
References
- Rule 21: For structural changes: suggest a patch file in
packages/<name>/patches/applied by the Makefile. (link) - Rule 21: For structural changes: suggest a patch file in
packages/<name>/patches/applied by the Makefile.
… refresh `make generate` in packages/apps/vm-instance/ rebuilds the instanceProfile enum from packages/system/kubevirt-instancetypes/templates/preferences.yaml. The pre-commit job picked up the drift after the upstream refresh in the previous commit. No source changes — only regenerated outputs. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
📝 WalkthroughWalkthroughThis PR standardizes KubeVirt VM instance profiles and configuration templates. Instance profile enum values are expanded to include new Linux distributions and Windows variants across multiple schema files. The build pipeline is simplified by removing persistent field cleanup from the Makefile. InstanceType configurations add explicit hugepage sizing and CPU socket constraints, while preference templates receive standardized metadata annotations, resource requirement labels, and device settings across all OS variants. ChangesVM Instance Profile and KubeVirt Configuration Standardization
🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/system/kubevirt-instancetypes/templates/preferences.yaml`:
- Around line 7-11: The YAML annotations openshift.io/documentation-url and
openshift.io/support-url are using literal block scalars (|) with quoted
strings, which causes the stored values to include the quotes and newline;
update every preference definition so these two annotation keys are plain scalar
strings (remove the | block indicator and surrounding quotes) so the values are
stored as normal URL strings without embedded quotes or trailing newlines (apply
this change for all occurrences of openshift.io/documentation-url and
openshift.io/support-url in the file).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8e6f56d1-7d91-4d3a-9564-7410aeebd9e3
📒 Files selected for processing (5)
packages/apps/vm-instance/values.schema.jsonpackages/system/kubevirt-instancetypes/Makefilepackages/system/kubevirt-instancetypes/templates/instancetypes.yamlpackages/system/kubevirt-instancetypes/templates/preferences.yamlpackages/system/vm-instance-rd/cozyrds/vm-instance.yaml
💤 Files with no reviewable changes (1)
- packages/system/kubevirt-instancetypes/Makefile
| openshift.io/documentation-url: | | ||
| "https://github.com/kubevirt/common-instancetypes" | ||
| openshift.io/provider-display-name: KubeVirt | ||
| openshift.io/support-url: https://github.com/kubevirt/common-instancetypes/issues | ||
| openshift.io/support-url: | | ||
| "https://github.com/kubevirt/common-instancetypes/issues" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
import pathlib
import subprocess
import sys
try:
import yaml
except ImportError:
subprocess.check_call([sys.executable, "-m", "pip", "install", "--quiet", "pyyaml"])
import yaml
path = pathlib.Path("packages/system/kubevirt-instancetypes/templates/preferences.yaml")
for doc in yaml.safe_load_all(path.read_text()):
if not doc:
continue
name = doc.get("metadata", {}).get("name")
ann = doc.get("metadata", {}).get("annotations", {})
bad = {}
for key in ("openshift.io/documentation-url", "openshift.io/support-url"):
val = ann.get(key)
if isinstance(val, str) and (val.startswith('"') or val.endswith("\n")):
bad[key] = repr(val)
if bad:
print(name, bad)
PYRepository: cozystack/cozystack
Length of output: 11161
Fix YAML annotation serialization across all preference definitions.
The openshift.io/documentation-url and openshift.io/support-url annotations use YAML literal block scalars (|) that preserve the surrounding quotes and trailing newline as literal content. This affects all 43 preference objects in the file, causing the annotation values to be stored as strings like "https://github.com/kubevirt/common-instancetypes"\n instead of valid URLs. Use plain scalar values without the | syntax and quotes.
Example fix (apply to all affected annotations)
- openshift.io/documentation-url: |
- "https://github.com/kubevirt/common-instancetypes"
+ openshift.io/documentation-url: https://github.com/kubevirt/common-instancetypes📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| openshift.io/documentation-url: | | |
| "https://github.com/kubevirt/common-instancetypes" | |
| openshift.io/provider-display-name: KubeVirt | |
| openshift.io/support-url: https://github.com/kubevirt/common-instancetypes/issues | |
| openshift.io/support-url: | | |
| "https://github.com/kubevirt/common-instancetypes/issues" | |
| openshift.io/documentation-url: https://github.com/kubevirt/common-instancetypes | |
| openshift.io/provider-display-name: KubeVirt | |
| openshift.io/support-url: | | |
| "https://github.com/kubevirt/common-instancetypes/issues" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/system/kubevirt-instancetypes/templates/preferences.yaml` around
lines 7 - 11, The YAML annotations openshift.io/documentation-url and
openshift.io/support-url are using literal block scalars (|) with quoted
strings, which causes the stored values to include the quotes and newline;
update every preference definition so these two annotation keys are plain scalar
strings (remove the | block indicator and surrounding quotes) so the values are
stored as normal URL strings without embedded quotes or trailing newlines (apply
this change for all occurrences of openshift.io/documentation-url and
openshift.io/support-url in the file).
|
Folded into #2602 ("feat(flux): upgrade to v2.8.0 + chart fixes for strict SSA & kstatus"). The kubevirt-instancetypes null-TPM fix from this PR lives there as commit Closing in favor of #2602. |
…(folds #2612) (#2602) ## What this PR does Upgrades Flux v2.7.3 → v2.8.0 across both the vendored tenant chart and the embedded management-cluster manifests, and folds in the chart fixes that become hard errors under the new strict helm-controller v1.5. Flux v2.8's helm-controller v1.5.0 ships: - **Server-Side Apply with `--force-conflicts`** — strict CRD schema enforcement; misplaced fields (silently dropped on v2.7) now fail the apply. - **kstatus-based health checking** by default — parent HR waits for every applied resource (including child HRs) to be Ready before flipping its own Ready, surfacing latent ordering deadlocks. - **Helm v4 embedded** — `upgrade.force: true` is deprecated and now conflicts with SSA. - **`HelmRelease.spec.healthCheckExprs`** — prerequisite for proper readiness gating (used in PR #2601 split). Folds in PR #2612 (kubevirt-instancetypes null TPM fix) since the same Flux upgrade triggers it. ## Commits **Flux upgrade itself:** - `feat(fluxcd)`: bump `flux-operator` / `flux-instance` vendored charts to v0.48.0; web UI opt-in. - `feat(flux)`: regenerate embedded management-cluster manifests via `make update` in `packages/core/flux-aio` (timoni bundle build). **Chart fixes for strict SSA — fields the chart sent that v2.7 silently dropped, v2.8 rejects:** - `fix(kubevirt-instancetypes)`: drop persistent strip that produced null `preferredTPM` (folds #2612). - `fix(foundationdb)`: relocate `faultDomain`, `imageType`, `labels`, `minimumUptimeSecondsForBounce` from inside `automationOptions` to direct children of `spec`. - `fix(kafka)`: place `enableServiceLinks` under `template.pod`, not a phantom `template.spec`. - `fix(vm-instance)`: emit `disk: {}` (not `disk:`/null) when no bus is set. - `fix(platform)`: drop deprecated `upgrade.force: true` from HelmReleases; fix `kafka` WorkloadMonitor `replicas` paths. **Ordering / deadlock fixes under v2.8 kstatus:** - `fix(vpa)`: break circular wait between parent install and nested `vpa-for-vpa` HR. - `fix(kubernetes)`: drop lookup-guarded parent-HR `dependsOn` on tenant addon child HRs (parent waits on child via kstatus, child waited on parent — deadlock). **E2E waits for v2.8 kstatus timing:** - `test(e2e)`: bump app HR-Ready waits to 5m (was 20s–100s under v2.7's faster dispatch). - `test(e2e)`: wait for parent HR Ready before downstream asserts in `run-kubernetes.sh` and `vminstance.bats`. ## Scope discipline This PR is part of the split of #2619 (the consolidated CI fixes branch) into review-friendly pieces. Companion PRs: - **PR #2601** (seaweedfs split) — folded into this PR (commits `29c6afc8`, `0e8b46d7`, `7157158c`, `dccdeb52`, `f880b324`): the seaweedfs-system → seaweedfs-db + seaweedfs-system split, its adoption migration 43 (targetVersion 44), and the configurable db resources all land here, because the strict-SSA `upgrade.force` removal and the kstatus parent-HR timeout bump only make sense together with the split. #2601 is superseded. - **PR #2558** (drop 3× retry on `Run E2E` + `Install Cozystack`) — independent, lands separately. - Several smaller standalone fixes lifted out of #2619 (startup probes, cert-manager `dependsOn`, prepull machinery, CSI HR timeout, NFS/OIDC test improvements) — opened as separate PRs. ## Verification - `helm template` renders cleanly for both `fluxcd` and `fluxcd-operator` packages with `web.enabled=false` (default) and `web.enabled=true`. - Embedded `cmd/cozystack-operator` binary contains the v1.5.0 / v1.8.0 / v2.1.0 controller image strings. - No references to the v0.39-removed `--disable-wait-interruption` flag anywhere in `packages/` or `internal/`. ### Release note ```release-note Flux upgraded to v2.8.0 (helm-controller v1.5 — Helm v4 Server-Side Apply with --force-conflicts, kstatus health checking). When upgrading existing clusters: - Kubernetes 1.33+ is now required for the platform (management) cluster, and for any tenant cluster that enables the optional (default-off) Flux addon — that addon ships the bumped Flux too. - HelmReleases no longer set `upgrade.force: true`. Helm v4 SSA resolves field-ownership conflicts automatically (`--force-conflicts`), but that is not the old client-side replace: immutable-field changes (e.g. StatefulSet volumeClaimTemplates/serviceName) no longer self-heal and require manual recreation — delete the object (e.g. `kubectl delete sts <name> --cascade=orphan`) and let Flux recreate it. - KubeVirt: persistent TPM/EFI is re-enabled for the Windows 11/2k22/2k25 preferences (KubeVirt 1.8 VMPersistentState); each affected VM provisions an extra RWO backend-storage PVC from the default StorageClass. - KubeVirt: the EOL centos.7*/centos.stream8* preferences are retained as deprecated, hidden aliases (`tags: hidden`, `instancetype.kubevirt.io/deprecated: "true"`) — existing VMInstances on these profiles keep rendering and need no action on upgrade, but the profiles are no longer offered for new VMs; repoint to centos.stream9/10 when convenient. The gn1.* GPU instancetypes are likewise retained. - FoundationDB: imageType now reaches the operator (silently dropped pre-SSA); it is pinned to `split` to match the value existing clusters effectively ran, so upgrades stay non-disruptive. Set `imageType: unified` to migrate deliberately. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Flux Status web UI: optional web server, config secret, service port, HTTPRoute/Ingress, network policy, and RBAC roles. * New SeaweedFS DB Helm chart and optional managed DB release. * **Improvements** * CRD/schema enhancements: new provider kinds, validations, variant option, and external checksum refs. * Raised Kubernetes prerequisite to 1.30+; extended e2e timeouts for reliability. * **Chores** * Bumped Flux Operator and Flux versions; documentation links updated to fluxoperator.dev. * **Bug Fixes** * Removed aggressive HelmRelease force-upgrade/install flags. <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/cozystack/cozystack/pull/2602?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
What this PR does
KubeVirt operator was bumped from v1.6.3 to v1.8.2 in
6af0bf71. The v1.8 operator ships a stricterVirtualMachineClusterPreferenceCRD schema that no longer tolerates null values for object-typed fields. Ourpackages/system/kubevirt-instancetypes/Makefilehadsed -i '/persistent: true/d'in itsupdatetarget, which stripped the only field from upstream'spreferredTPM: { persistent: true }and left the parent key dangling as null. That produced YAML the new CRD rejects, breaking Helm reconciliation ofcozy-kubevirt/kubevirt-instancetypes:Changes:
sedfrom the Makefile.templates/preferences.yamlandtemplates/instancetypes.yamlfrom upstreamkubevirt/common-instancetypesHEAD.vm-instancevalues.schema.jsonand the embedded schema invm-instance-rd/cozyrds/vm-instance.yamlto match the new preference list.Runtime implication:
preferredTPM: { persistent: true }now reaches the cluster. WhenvmStateStorageClassis unset on the KubeVirt CR (the default in cozystack), KubeVirt falls back to the cluster's default StorageClass with RWO access mode for TPM state, perpkg/storage/backend-storage/backend-storage.goin v1.8.2. Cozystack shipsreplicatedas the default StorageClass, so this works out of the box.Drive-by content changes from the upstream refresh
centos.7,centos.7.desktop,centos.stream8,centos.stream8.desktop,centos.stream8.dpdkpreferences. See "Upgrade impact" below.debian,oraclelinux,legacy,linux,linux.efi,linux.virtiotransitional,fedora.s390x,rhel.9.s390x,rhel.10.s390x,windows.xp,windows.7,windows.7.virtio,windows.2k3,windows.2k8,windows.2k8.virtio,windows.2k12,windows.2k12.virtio.cx1.*1gi(1 GiB-per-vCPU) variants, newd1series.Upgrade impact: VMs referencing removed preferences
The five removed CentOS-7 / CentOS-Stream-8 preferences are deleted on upgrade. Existing VMs that reference them fall into three buckets:
Running VMs — unaffected. KubeVirt resolves the preference once at VMI creation; the DomainSpec is already materialized. No live or live-migration impact. Becomes a latent problem on next stop/start or unscheduled node failure.
Stopped VMs (or VMs restarted after upgrade) — fail to start. The VM controller emits:
Helm-managed VMs (
vm-instance/virtual-machinecharts) —HelmReleaseenters a failed reconcile on next chart upgrade because the regeneratedvalues.schema.jsonenum rejects the old name. The existingVirtualMachineresource is not modified by the failed reconcile.Discovery — find affected resources before upgrade
Self-serve remediation for tenants
Tenants can fix their own VMs in their tenant namespace without platform-team involvement:
centos.stream9), save. The form is driven by the regeneratedvalues.schema.json, so only valid choices are offered.kubectl -n <tenant-ns> edit hr <release>and updatespec.values.instanceProfile.kubectl -n <tenant-ns> patch vm <name> --type=json -p='[{"op":"replace","path":"/spec/preference/name","value":"centos.stream9"}]'.Suggested replacements (the hardware defaults are nearly identical; guest OS remains whatever it is):
centos.7,centos.stream8centos.stream9centos.7.desktop,centos.stream8.desktopcentos.stream9.desktopcentos.stream8.dpdkcentos.stream9.dpdkRelease note