feat(kubernetes): boot tenant worker disks from a shared golden Talos image (CDI clone) - #3294
feat(kubernetes): boot tenant worker disks from a shared golden Talos image (CDI clone)#3294myasnikovdaniil wants to merge 17 commits into
Conversation
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 optimizes the provisioning of tenant Kubernetes worker VMs by shifting from a per-worker HTTP image import pattern to a shared golden image cloning strategy. By leveraging CDI CSI-cloning, the system reduces network overhead and improves the reliability of node-join operations. The changes are designed to be non-disruptive, ensuring that existing clusters continue to function with their current configuration while new node groups automatically benefit from the improved cloning mechanism. 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 introduces the kubernetes-worker-image package to pre-populate golden Talos worker OS images as DataVolumes in the cozy-public namespace. This enables tenant Kubernetes worker VMs to boot via CDI storage-layer clones instead of downloading raw images over HTTP, resolving node-join flakes and eliminating external dependencies during worker provisioning. The kubernetes app is updated to utilize these clones while maintaining backward compatibility for existing node groups. Feedback on the changes identifies that the fallback storage size in dv.yaml is set to 4Gi, which is smaller than the ~4.15 GiB Talos raw image and would cause failures. It is recommended to increase this fallback to 6Gi and quote the rendered value.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| storage: | ||
| resources: | ||
| requests: | ||
| storage: {{ .storage | default $.Values.storage | default "4Gi" }} |
There was a problem hiding this comment.
The fallback default storage size is set to 4Gi here. However, as noted in the PR description, the Talos raw image is ~4.15 GiB virtual, meaning a 4Gi PVC will fail with DataVolume too small. We should update this fallback default to 6Gi to match the default in values.yaml and prevent failures if $.Values.storage is ever omitted or cleared. Additionally, quoting the value ensures it is always rendered as a valid Kubernetes quantity string.
storage: {{ .storage | default $.Values.storage | default "6Gi" | quote }}There was a problem hiding this comment.
Done — the fallback default is now 6Gi and quoted: {{ .storage | default $.Values.storage | default "6Gi" | quote }}. That matches the 6Gi in values.yaml and the documented ~4.15 GiB minimum of the Talos v1.13 openstack raw, so a cleared $.Values.storage can no longer render a too-small PVC. Fixed in d8ce521.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds a packaged golden Talos worker image catalog, supports per-node-group clone or HTTP image sources, updates installer and rendering tests, and removes the prior Talos image-cache e2e integration. ChangesGolden Talos worker image provisioning
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant KubernetesTemplate
participant CozyPublic
participant CDI
participant WorkerVM
KubernetesTemplate->>CozyPublic: check golden Talos DataVolume
alt Builtin source selected
KubernetesTemplate->>CDI: configure PVC clone source
CDI->>WorkerVM: provision cloned boot disk
else Factory source selected
KubernetesTemplate->>CDI: configure HTTP import source
CDI->>WorkerVM: provision imported boot disk
end
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
NOT LGTM — the golden-image package is wired opt-in (off by default), so neither the production import reduction nor the e2e node-join flake fix this PR is premised on actually happens; the image-cache workaround is removed in the same change, leaving e2e more exposed than before.
Business context: Replace per-worker HTTP import of the Talos OS image from the public Image Factory with a per-worker CDI clone of a shared golden image in cozy-public, to remove the per-worker Factory dependency and the kubernetes-* e2e node-join flake (#3231).
Blockers
B1: The golden package is opt-in (default OFF), so the feature is inert by default and the description/release-note are inverted
File: packages/core/platform/templates/bundles/iaas.yaml:123
Issue: kubernetes-worker-image is registered via cozystack.platform.package.optional.default. That helper emits the Package only when the name is in bundles.enabledPackages (_helpers.tpl: if and (has $name $enabled) (not (has $name $disabled))). It is opt-in / disabled-by-default, not opt-out.
Evidence: the optional helper body gates on has $name $enabled. The sibling vm-default-images uses the same helper and its own changelog (v1.3.4 / v1.4.1) documents it as "disabled by default … enable via bundles.enabledPackages". No default enabledPackages in packages/core/platform/values.yaml lists either package. So on a stock install the golden DataVolume is never created and every worker takes the HTTP-import fallback.
Impact: the production benefit ("one import per image instead of one per worker") does not occur out of the box. The release note ("New node groups adopt the clone automatically") is false by default — with no golden, nothing clones. The description's "Opt-out via bundles.disabledPackages" is inverted.
Fix: decide the intent. If the golden should be active by default, use cozystack.platform.package.default (always-on). If opt-in is intended (a 6Gi replicated volume on every install, including VM-only / app-only clusters, is a real cost — the reason vm-default-images is opt-in), correct the description and release note to "opt-in via enabledPackages" and address B2.
B2: e2e never enables the golden, and the image-cache it removes was the only #3231 mitigation there
File: hack/e2e-install-cozystack.bats:127
Issue: the e2e installer sets bundles.enabledPackages: [cozystack.external-dns-application] only — kubernetes-worker-image is absent. Combined with B1, the golden is not deployed in e2e, so kubernetes-latest / kubernetes-previous workers hit $useClone=false and HTTP-import from the default https://factory.talos.dev. This PR also removes the in-sandbox talos-image-cache mirror that buffered that public endpoint.
Evidence: run-kubernetes.sh no longer injects talos.imageFactoryURL; no chainsaw fixture sets it; the new diagnostics comment itself says "a cluster that fell back to the HTTP import instead shows an importer-* pod looping on a factory error". The green E2E run is not proof — #3231 is by definition an intermittent factory stall, so one pass is consistent with "the factory happened to be reachable".
Impact: the PR's headline justification (fix the e2e flake) is not delivered, and e2e reliability likely regresses versus main (buffer removed, nothing replacing it).
Fix: add cozystack.kubernetes-worker-image to the e2e enabledPackages (the default golden schematicID/version already match packages/apps/kubernetes talos ce4c98… / v1.13.6, so the clone would be selected), and confirm a worker-spinning suite actually takes the clone path before removing the cache.
B3: New clone-selection branching ships with zero unit coverage in a chart that already mocks lookup
File: packages/apps/kubernetes/templates/cluster.yaml:110-127
Issue: the live-KMT source-type pin (http stays http, pvc stays pvc) and the new-group clone-adoption are the safety-critical "no worker roll" guarantee, yet no unit test exercises any of the new branches — the existing tests only cover the lookup-nil HTTP path.
Evidence: packages/apps/kubernetes/tests/nodegroups_default_test.yaml already uses kubernetesProvider.objects to mock lookup for MachineDeployment and KubevirtMachineTemplate — the exact kinds this logic reads. The pin path (highest blast radius: breaking byte-identity rolls every worker in every tenant cluster on the next platform bump) is covered by neither unit tests nor e2e (e2e is fresh-install only, so every group is new).
Fix: add helm-unittest cases with mocked lookups: (a) live MD + KMT with http disk-system renders http and keeps usePopulator: "false"; (b) live MD + KMT with pvc renders the clone; (c) no live MD plus a golden DataVolume present renders the clone.
Non-blocking follow-ups
packages/system/kubernetes-worker-image/templates/dv.yaml:32— the| default "4Gi"tail contradicts the documented ~4.15Gi minimum; if$.Values.storageis ever cleared it renders a too-small PVC (DataVolume too small). Make the final fallback6Giand quote it (already flagged by the Gemini bot).packages/apps/kubernetes/templates/cluster.yaml:127—$useCloneis latched on golden existence, not readiness. A new group rendered during the golden's import window clones an unpopulated source with no HTTP fallback; the worker DataVolume blocks until the golden reachesSucceeded, or indefinitely if the golden import fails. Consider gating ondig "status" "phase" "" $goldenDV | eq "Succeeded".- Cross-StorageClass clone: if a tenant's worker
storageClassdiffers from the golden's (replicated), CDI cannot CSI-clone and silently falls back to a host-assisted network copy — documented invalues.yamlbut unguarded. Fine to leave, worth a note.
3f3dd13 to
f3589e4
Compare
|
Aleksei Sviridkin (@lexfrei) Thanks — the design was reworked so the image source is an explicit per-node-group choice rather than an implicit auto-clone, which addresses the blockers:
|
ba1ab25 to
80c2c2c
Compare
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
NOT LGTM
feat(kubernetes): boot tenant worker disks from a shared golden Talos image (CDI clone). Two MAJOR issues around catalog-entry lifecycle and version-drift guarding block it. Verified by execution: helm-unittest across three charts, go build/go vet, and a render-diff of the default path against main (byte-identical apart from random tokens).
Findings
[MAJOR] packages/system/kubernetes-worker-image/templates/dv.yaml:6-32 + packages/apps/kubernetes/templates/cluster.yaml:142-145 — removing a catalog images[] entry silently blocks ALL future reconciliation of any existing tenant pinned to it. The golden DataVolume lacks helm.sh/resource-policy: keep, so removing (rather than appending) an entry prunes the DataVolume + its CDI PVC. Then cluster.yaml:142 does a lookup on that golden and cluster.yaml:144 fail()s the entire chart render when nil — aborting the whole Kubernetes HelmRelease, not just the disk block. An unrelated tenant then gets stuck on its next Helm action (scale / addon / RBAC edit). This is strictly worse than the vm-disk precedent, which fails at resource level only. Add resource-policy: keep to the golden DV and/or make the missing-golden path degrade instead of failing the whole render.
[MAJOR] packages/apps/kubernetes/values.yaml:307-308 vs packages/system/kubernetes-worker-image/values.yaml:44-46 — no automated guard ties the tenant chart's default Talos (version, schematicID) to the catalog's default images[] entry. They are two independently-edited files; a future one-sided bump silently breaks every fresh install using image.builtin: {}, and no existing test catches the drift. Add a test/lint asserting the two defaults stay in sync.
Claim mismatches / caveats
- [PARTIAL] Stale unit-test counts in the PR body; actual is 193/193 kubernetes, 88/88 platform, all green.
- [UNVERIFIABLE] live-cluster claim — outside the hermetic toolset.
Reconciliation with prior feedback (lexfrei CHANGES_REQUESTED)
- These findings are on different grounds than lexfrei's B1 — independent convergence, not overlap.
- lexfrei's B1 "image-cache workaround removed leaving e2e more exposed" is addressed: the removed
talos-image-cache.sh/e2e-talos-image-cache.yaml/talos-image-cache_test.batsare replaced by the golden clone (hack/e2e-chainsaw/_lib/run-kubernetes.sh:334-339setsimage.builtin: {}on worker groups and waits for the golden import at 263-269, explicitly as the #3231 mitigation). - lexfrei's "opt-in / off by default, so the production import-reduction premise doesn't happen" is not resolved for the production default path (by design): the default/omitted path renders identically to
mainand does not use the golden clone. The feature was deliberately reframed as an opt-in catalog, so production imports are only reduced for tenants who explicitly opt in.
|
Aleksei Sviridkin (@lexfrei) follow-ups 2 and 3 are now closed in B1 — opt-in inverted. Correct, and the description and release note were wrong. Both now state opt-in via B2 — e2e never enables the golden. Fixed: B3 — untested lookup pin. Removed rather than tested: the disk-system source now derives purely from Follow-up 1 — 4Gi fallback. Fixed: 6Gi, quoted. Follow-up 2 — readiness vs existence. Fixed in Follow-up 3 — cross-StorageClass. Fixed in One point of yours I'd rather keep on the record than mark closed: a single green E2E is weak evidence against an intermittent factory stall, and that cuts both ways — it doesn't prove the clone path fixed #3231 either, only that the path works end to end. The structural claim is narrower than the original description implied: the golden still does exactly one factory fetch, same as the cache it replaces, so the delta isn't "fewer factory dependencies" but that the N-way fanout moved off the pod network onto the storage layer. Unit tests: |
A helm fail() aborts the entire chart render, so the golden-image guards took down the whole Kubernetes HelmRelease of every tenant pinned to that golden — every resource, not just the worker disk — blocking unrelated scale and addon operations. Two changes narrow when that can happen. Only phase=Failed aborts the render now. A golden mid-import is transient: CDI holds the worker DataVolume until the source populates, which is the correct behaviour and recovers unattended, so it no longer fails the render. Failed never recovers without operator action, so pinning workers to it is worth the abort. Falling back to the HTTP import is still not an option — it would make the disk source cluster-state-dependent and flip the content-hashed KubevirtMachineTemplate name once the golden went Ready, rolling every worker in the group. Goldens now carry helm.sh/resource-policy: keep, so removing an images[] entry orphans the DataVolume instead of pruning it. Pruning a golden that tenants still clone was the main way to trip the missing-golden fail() by accident; removal now stops managing the golden and leaves cleanup to the operator. Reported by @IvanHunters in review of #3294. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
`image.builtin: {}` resolves its golden from the kubernetes app's own
talos defaults and then requires the kubernetes-worker-image catalog to
hold a matching (schematicID, version) — the tenant render fails outright
when it does not. Those defaults live in independently edited files, so a
one-sided Talos bump silently breaks every fresh install using
image.builtin, with nothing catching it until a ~95-minute e2e run does.
The StorageClass is the same trap from the other direction: CDI cannot
CSI-clone across classes, so the render also rejects a group whose
storageClass differs from its golden's, and changing one file's default
alone brings that guard down on the default path.
Both assertions are mutation-proven: each fails on its own axis when the
corresponding default is changed alone, and passes once it is restored.
Reported by @IvanHunters in review of #3294.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
|
IvanHunters both MAJORs are fixed — thanks, the first one caught something I had actively made worse. MAJOR 1 — fixed in Goldens now carry The render also no longer aborts on "golden not ready", only on I did keep the MAJOR 2 — fixed in I widened it past what you flagged. The StorageClass has the identical shape — two independently-edited defaults feeding a render-time hard-fail — and the cross-StorageClass guard added for lexfrei's follow-up 3 is exactly what turned that into a brick, so covering One precision on "no existing test catches the drift": a catalog-side bump is caught today, by the pinned golden name in On the test counts — you were right, and my earlier correction was incomplete. 193/193 was accurate for the commit you reviewed. The body now reads 198/198 for On the live-cluster claim — agreed it is outside a hermetic toolset and worth treating as unverified. For what it is worth on CI: the last completed e2e run failed at install on a |
`image.builtin: {}` resolves its golden from the kubernetes app's own
talos defaults and then requires the kubernetes-worker-image catalog to
hold a matching (schematicID, version) — the tenant render fails outright
when it does not. Those defaults live in independently edited files, so a
one-sided Talos bump silently breaks every fresh install using
image.builtin, with nothing catching it until a ~95-minute e2e run does.
The StorageClass is the same trap from the other direction: CDI cannot
CSI-clone across classes, so the render also rejects a group whose
storageClass differs from its golden's, and changing one file's default
alone brings that guard down on the default path.
Both assertions are mutation-proven: each fails on its own axis when the
corresponding default is changed alone, and passes once it is restored.
Reported by @IvanHunters in review of #3294.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
043a27d to
5061fb7
Compare
|
Heads-up on a textual and API overlap, agreed with Aleksei Sviridkin (@lexfrei) on #3571 so it is not discovered at rebase time. #3571 adds a flat Your Two concrete things to expect:
|
A helm fail() aborts the entire chart render, so the golden-image guards took down the whole Kubernetes HelmRelease of every tenant pinned to that golden — every resource, not just the worker disk — blocking unrelated scale and addon operations. Two changes narrow when that can happen. Only phase=Failed aborts the render now. A golden mid-import is transient: CDI holds the worker DataVolume until the source populates, which is the correct behaviour and recovers unattended, so it no longer fails the render. Failed never recovers without operator action, so pinning workers to it is worth the abort. Falling back to the HTTP import is still not an option — it would make the disk source cluster-state-dependent and flip the content-hashed KubevirtMachineTemplate name once the golden went Ready, rolling every worker in the group. Goldens now carry helm.sh/resource-policy: keep, so removing an images[] entry orphans the DataVolume instead of pruning it. Pruning a golden that tenants still clone was the main way to trip the missing-golden fail() by accident; removal now stops managing the golden and leaves cleanup to the operator. Reported by @IvanHunters in review of #3294. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
`image.builtin: {}` resolves its golden from the kubernetes app's own
talos defaults and then requires the kubernetes-worker-image catalog to
hold a matching (schematicID, version) — the tenant render fails outright
when it does not. Those defaults live in independently edited files, so a
one-sided Talos bump silently breaks every fresh install using
image.builtin, with nothing catching it until a ~95-minute e2e run does.
The StorageClass is the same trap from the other direction: CDI cannot
CSI-clone across classes, so the render also rejects a group whose
storageClass differs from its golden's, and changing one file's default
alone brings that guard down on the default path.
Both assertions are mutation-proven: each fails on its own axis when the
corresponding default is changed alone, and passes once it is restored.
Reported by @IvanHunters in review of #3294.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
f0f40f2 to
c028775
Compare
A helm fail() aborts the entire chart render, so the golden-image guards took down the whole Kubernetes HelmRelease of every tenant pinned to that golden — every resource, not just the worker disk — blocking unrelated scale and addon operations. Two changes narrow when that can happen. Only phase=Failed aborts the render now. A golden mid-import is transient: CDI holds the worker DataVolume until the source populates, which is the correct behaviour and recovers unattended, so it no longer fails the render. Failed never recovers without operator action, so pinning workers to it is worth the abort. Falling back to the HTTP import is still not an option — it would make the disk source cluster-state-dependent and flip the content-hashed KubevirtMachineTemplate name once the golden went Ready, rolling every worker in the group. Goldens now carry helm.sh/resource-policy: keep, so removing an images[] entry orphans the DataVolume instead of pruning it. Pruning a golden that tenants still clone was the main way to trip the missing-golden fail() by accident; removal now stops managing the golden and leaves cleanup to the operator. Reported by @IvanHunters in review of #3294. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
`image.builtin: {}` resolves its golden from the kubernetes app's own
talos defaults and then requires the kubernetes-worker-image catalog to
hold a matching (schematicID, version) — the tenant render fails outright
when it does not. Those defaults live in independently edited files, so a
one-sided Talos bump silently breaks every fresh install using
image.builtin, with nothing catching it until a ~95-minute e2e run does.
The StorageClass is the same trap from the other direction: CDI cannot
CSI-clone across classes, so the render also rejects a group whose
storageClass differs from its golden's, and changing one file's default
alone brings that guard down on the default path.
Both assertions are mutation-proven: each fails on its own axis when the
corresponding default is changed alone, and passes once it is restored.
Reported by @IvanHunters in review of #3294.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
… caching Seed the Talos worker OS image once into a DataVolume in cozy-public (packages/system/kubernetes-worker-image), installed via the iaas bundle, so tenant Kubernetes worker VMs can boot by a CDI storage-layer clone instead of each worker importing the raw image over HTTP from the Talos Image Factory. Mirrors the vm-default-images golden-image convention; the existing cdi-clone-dv RoleBinding in cozy-public already authorises cross-namespace clones. Keyed by (schematicID, version) via the deterministic name talos-worker-<schematicID>-<version> that the kubernetes app reconstructs. storage defaults to 6Gi: the Talos v1.13.x openstack raw is ~4.15 GiB virtual, and a live-cluster import into a 4Gi PVC fails with "DataVolume too small". Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Point each new worker's disk-system DataVolume at a CDI storage-layer clone of the golden Talos image in cozy-public instead of a per-worker HTTP import, removing the per-worker Image Factory dependency and the kubernetes-* e2e node-join flake where N workers race to reach the image cache ClusterIP across the sandbox CNI (#3231). The clone path omits the usePopulator=false annotation so CDI's populator does a CSI clone (live-verified: temp PVC CSI-cloned + resized to the worker diskSize, RWX, no host-assisted upload pod), not a pod-network copy. Existing node groups are never migrated: the KubevirtMachineTemplate is content-hashed, so both the source and that annotation are pinned to the live KMT's values when a MachineDeployment for the group already exists (render and hash unchanged, no worker roll); only a brand-new group with a matching golden clones, else HTTP. lookup() is empty under helm template, so renders and unit tests take the HTTP path (verified: KMT byte-identical old-vs-new). Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
…-image clone The in-sandbox talos-image-cache mirror (hack/e2e-talos-image-cache.yaml) and its probe/egress/injection machinery existed only to buffer the flaky public Talos Image Factory for per-worker DataVolume imports (#3231). Now that tenant workers boot by cloning the shared golden image in cozy-public, nothing consumes the mirror: new clusters clone (never touching the cache ClusterIP) and the golden imports once directly from the factory. Remove the manifest, the resolve/probe/diagnose helper, its bats unit test, the install-time deploy step, and the tenant-CR imageFactoryURL injection; the node-join failure diagnostics now dump the golden source DataVolume instead of re-probing the cache. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Reframe packages/system/kubernetes-worker-image as an explicit catalog of golden Talos worker images (each images[] entry is one (schematicID, version) flavor) and document that it is opt-in: disabled by default and enabled via bundles.enabledPackages. A tenant node group clones a golden only when it sets image.builtin; groups without it import over HTTP. Also raise the dv.yaml storage fallback from 4Gi to 6Gi and quote it: the Talos raw image is ~4.15 GiB and the values.yaml default is 6Gi, so a cleared $.Values.storage would otherwise render a too-small PVC. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Introduce a WorkerImage union on NodeGroup so each node group can choose its
worker OS image source explicitly:
- image.factory{version,schematicID,imageFactoryURL} - import over HTTP from
an Image Factory / mirror (the default when image is omitted);
- image.builtin{schematicID,version} - CDI-clone a golden from the opt-in
worker image catalog in cozy-public.
All fields default to the cluster-wide talos.* values. This commit adds the API
plus the regenerated schema, CRD, Go types, and deepcopy. The WorkerImage doc
states the real rollout contract: a group with image omitted renders identically
to the pre-feature chart (adoption rolls no worker), while editing an existing
group's image re-images that group. The factory imageFactoryURL doc notes it
redirects only the boot disk, not the cluster-wide installer repository.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Consume the NodeGroup.image selector in the worker DataVolume template:
- image.builtin -> CDI clone of cozy-public/talos-worker-<schematicID>-<version>;
- image.factory or omitted -> HTTP import from the (per-group or cluster)
Image Factory.
The disk-system source is derived purely from .group.image, replacing the
previous implicit auto-adoption. A group clones only when it sets image.builtin;
builtin/factory presence is detected with hasKey so an empty map selects the
cluster talos.* defaults. A builtin group fails the render loudly if the golden
is absent, or if the worker diskSize is smaller than the golden's storage (a CDI
clone target must be at least its source). With image omitted the render is
byte-identical to the pre-feature chart (verified: the default md0
KubevirtMachineTemplate content hash is unchanged), so adopting this feature
rolls no existing worker.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
…mage The talos-reconcile Job builds each node group's install.image from the cluster-wide talos.schematicID/version. With per-group image sources a group can boot a different flavor/version, so an in-guest upgrade would reinstall the cluster default and silently flip that group back. Resolve schematicID/version per group from .group.image (same hasKey selection as cluster.yaml) and use them for install.image, so the installer matches the OS the group actually booted. For image-omitted groups the resolved values equal talos.*, so the rendered Job spec — and its content-hash name — is unchanged (verified: the pinned Job-name hash test still passes), keeping the immutable Job churn-free across upgrades. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
The image-cache removal assumed workers clone by default, but the clone is opt-in per node group. Enable the cozystack.kubernetes-worker-image catalog in the e2e install and set nodeGroups.md0.image.builtin on the worker-spinning kubernetes CR, so the suites take the CDI clone path instead of a per-worker HTTP import — delivering the #3231 mitigation the cache used to provide. The golden imports once from the factory and every worker clones it, so a separate cache mirror is redundant. Wait for the golden DataVolume to reach Succeeded before applying the tenant CR (capturing the DV list once so a transient get cannot skip the wait): it is the single remaining factory dependency, so a stall surfaces as a clear failure rather than an opaque node-join timeout, and the render (which fails when the golden is absent) is guaranteed a created source. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Add helm-unittest coverage for the image.builtin / image.factory branches that
carry the no-worker-roll and clone-selection guarantees:
- image omitted / empty image.factory -> HTTP import from the cluster talos.*;
- image.factory overrides -> HTTP import from the given mirror/schematic/version;
- image.builtin (empty and explicit) -> CDI clone of the golden, usePopulator
annotation dropped; hasKey detection so an empty builtin map still clones;
- image.builtin with an absent golden -> render fails;
- image.builtin with diskSize below the golden storage -> render fails;
- image.builtin + image.factory together -> render fails;
- talos-reconcile install.image tracks each group's resolved schematicID/version
(builtin, factory-override, and omitted);
- an image-omitted group keeps a stable, main-identical KubevirtMachineTemplate
content hash (regression guard for the no-roll invariant).
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
The kubernetes suites boot workers by CDI-cloning the golden Talos image in cozy-public and resizing the clone up to the worker diskSize; that clone+grow needs transient pool headroom (the golden itself, a temp clone PVC, and the grown target) on top of the full suite's other PVCs. At 200G the per-node LINSTOR/ZFS data pool ran at its limit, so the resize-grow failed with "storage pool does not have enough free space", stalled worker boot, and the kubernetes suites burned their node-ready timeouts until the run blew past the 180m e2e budget. Bump the per-node data.img to 300G (sparse, so it costs host disk only as the pool fills). Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
A node group with image.builtin selected the clone as soon as the golden DataVolume existed in cozy-public, not once it was populated. Cloning a still-importing golden leaves the worker DataVolume blocked with no signal, and a golden whose import failed blocks it forever, with no HTTP fallback to recover through. Require phase=Succeeded and fail the render otherwise. Failing is deliberate over falling back to the HTTP import: a fallback would make disk-system's source depend on cluster state, so the source — and with it the content-hashed KubevirtMachineTemplate name — would flip once the golden went Ready and roll every worker in the group. The app HelmRelease retries on failure every 17s with no remediation (Strategy.Name=RetryOnFailure), so a cluster created during the import window self-heals rather than sticking. The e2e kubernetes suites already wait for the golden to reach Succeeded before applying the Kubernetes CR, so this changes nothing there. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
CDI takes the storage-layer CSI-clone path only when source and target share a StorageClass. Across classes it silently falls back to a host-assisted copy over the pod network — precisely the per-worker network transfer image.builtin exists to eliminate (#3231) — so a node group whose storageClass diverged from the golden's got the old failure mode back with no signal that anything had changed. Fail the render when both StorageClasses are known and differ. When either side is empty the cluster default applies, which is not resolvable at render time, so the guard skips rather than guessing a mismatch. The defaults on both sides are "replicated", so the common path is unaffected. This rules out the guaranteed-host-assisted case, not every one: a matching StorageClass still leaves the clone strategy to the CDI StorageProfile, which the worker DataVolume inherits. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
A helm fail() aborts the entire chart render, so the golden-image guards took down the whole Kubernetes HelmRelease of every tenant pinned to that golden — every resource, not just the worker disk — blocking unrelated scale and addon operations. Two changes narrow when that can happen. Only phase=Failed aborts the render now. A golden mid-import is transient: CDI holds the worker DataVolume until the source populates, which is the correct behaviour and recovers unattended, so it no longer fails the render. Failed never recovers without operator action, so pinning workers to it is worth the abort. Falling back to the HTTP import is still not an option — it would make the disk source cluster-state-dependent and flip the content-hashed KubevirtMachineTemplate name once the golden went Ready, rolling every worker in the group. Goldens now carry helm.sh/resource-policy: keep, so removing an images[] entry orphans the DataVolume instead of pruning it. Pruning a golden that tenants still clone was the main way to trip the missing-golden fail() by accident; removal now stops managing the golden and leaves cleanup to the operator. Reported by @IvanHunters in review of #3294. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
`image.builtin: {}` resolves its golden from the kubernetes app's own
talos defaults and then requires the kubernetes-worker-image catalog to
hold a matching (schematicID, version) — the tenant render fails outright
when it does not. Those defaults live in independently edited files, so a
one-sided Talos bump silently breaks every fresh install using
image.builtin, with nothing catching it until a ~95-minute e2e run does.
The StorageClass is the same trap from the other direction: CDI cannot
CSI-clone across classes, so the render also rejects a group whose
storageClass differs from its golden's, and changing one file's default
alone brings that guard down on the default path.
Both assertions are mutation-proven: each fails on its own axis when the
corresponding default is changed alone, and passes once it is restored.
Reported by @IvanHunters in review of #3294.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Four value-schema descriptions still documented the pre-golden-image behaviour, where the worker OS image source was cluster-wide and always an HTTP import. nodeGroups.<name>.diskSize claimed the disk carries "the Talos OS image (factory.talos.dev raw artifact streamed in by CDI)", which is only the image.factory path; on image.builtin the disk is a storage-layer CDI clone of a shared golden and never fetches over HTTP. It also said nothing about the hard floor templates/cluster.yaml:177-185 now enforces at render time, whose only documentation lived on the catalog side (packages/system/kubernetes-worker-image/values.yaml). An operator who hits that failure now finds the constraint in the field's own docs. talos.version and talos.imageFactoryURL read as the single source for every worker. Both are cluster-wide defaults a node group overrides through nodeGroups.<name>.image, and imageFactoryURL does not apply to a builtin group at all, since $reqFactoryURL only feeds the HTTP source branch. talos.version additionally carries the note that the Talos<->Kubernetes support matrix is evaluated against the cluster-wide value only. talos.installerRepository documented the installer as <installerRepository>/<schematicID>:<version> against the cluster-wide pair. talos/talos-reconcile-job.yaml:549-563 resolves the schematic and version from the group's own image and threads them into both install.image (:420) and the TalosConfigTemplate talosVersion (:340), so the installer follows the OS the group actually booted while the prefix stays cluster-wide. Text only, no template or behaviour change. The generated consumers (values.schema.json, README.md parameter table, api types.go doc comments, kubernetes-rd openAPISchema) are regenerated from values.yaml by cozyvalues-gen and hack/update-crd.sh; both were verified byte-reproducible against the committed tree before the edit, and the resulting diff carries the four description strings with no key reordering. The README table rows widen because the generator recomputes column padding from the longest cell. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
…elper etcd-probe.sh cited hack/e2e-chainsaw/_lib/talos-image-cache.sh as a second, unextracted copy of the same lost-attach-stream recovery, and gave that second copy as the reason the helper had not been given a non-etcd-specific name. Commit 2731a3f deletes that file, so the comment pointed at a path that no longer exists and asserted a state the same branch falsifies. Keep what still holds — the race itself, the fact that the inline copy is what surfaced this call site, and the hack/etcd-probe_test.bats coverage — and say plainly that this is now the only copy, with the rename deferred to whenever a second caller appears. Comment only; the function is untouched and hack/etcd-probe_test.bats still passes. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Deleting hack/e2e-chainsaw/_lib/talos-image-cache.sh left four bats suites that main grew in the meantime asserting against it. They did not conflict during the rebase because they never touched the deleted files by name -- they enumerate collectors, stub helpers and pin the diagnostics spend order, so they only break when run. Drop the six node-join @test blocks whose whole subject is the cache gate and the bounded cache dump, and the cache arm of every enumeration that outlived it: the guarded-collector table, the spend-order phrase table (and the matching line in both chainsaw suites' documented order), the missing-timeout warning in run-kubernetes.sh and the marker list that pins what a spent phase budget declines. The end-of-block assertion now names the ghcr-mirror section, which is what runs last once the cache re-probe is gone. Collapse run-kubernetes-talos-spec_test.bats to the one fragment talos_spec_block still composes. Its four-combination matrix existed because two independent mirrors contributed to spec.talos; only registryMirrors does now. The emptiness case and the single-talos-key invariant are kept -- they are what the heredoc splice rides on -- and a new case pins that no imageFactoryURL is emitted for a disk the workers no longer import. Rewrite the ghcr-mirror early-return guard around the hazard rather than around the neighbour that used to embody it: what must hold is that the @test applying the mirror manifest reaches its apply unconditionally, which no longer depends on a second mirror @test existing. Also repoint the prose in ghcr-mirror.sh, e2e-ghcr-mirror.yaml and run-kubernetes.sh that named the deleted manifest and helper by path. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
c028775 to
5952c7d
Compare
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
NOT LGTM.
The customer-facing chart change is solid: with image omitted the KubevirtMachineTemplate renders byte-for-byte as on main (verified at base 7194edf6 vs HEAD, both test-k8s-md0-767d86), the guards fail loudly, and adopting the feature rolls no existing worker. Two things block merge. The e2e change silently severs the ghcr.io worker-image mirror from the tenant CR it applies, walking straight back into the node-join flake this PR exists to remove; and the Talos-to-Kubernetes support-matrix guard does not see the new per-group version fields, so a schema-valid config can boot an unsupported combination without the render failing.
Findings
[MAJOR] hack/e2e-chainsaw/_lib/run-kubernetes.sh:4077, the ghcr.io worker-image mirror is computed but never applied to the tenant CR.
This PR keeps talos_block=$(talos_spec_block) but deletes the ${talos_block} line from the tenant Kubernetes heredoc (the diff shows the removed -${talos_block} line; the heredoc now runs from spec: at 4106 straight to addons: at 4107). talos_spec_block is the only thing that emits spec.talos.registryMirrors, so the applied CR carries no mirror. Every worker's kubelet image pull then leaves the sandbox to public ghcr.io instead of the in-cluster ghcr-mirror.kube-system.svc that the suite warms and installs a Cilium policy for. That is the exact egress the mirror exists to remove, and it is the failure shape of the node-join flake (#3513/#3548 class) this PR targets via #3231. Nothing fails loudly: falling back to public ghcr.io is a legal state, so the diagnostics only report it after a node-join timeout. The helper, its unit tests (hack/run-kubernetes-talos-spec_test.bats) and the surrounding comments were all kept, and one comment still claims "${talos_block} sits on a line of its own in the heredoc", which no longer matches the code. The test does not catch the break because it exercises talos_spec_block as an isolated string builder (spec_doc "$(talos_spec_block)"), never the splice into the real CR. Fix: restore the ${talos_block} line under spec:, or, if the mirror is meant to go, delete the helper, the assignment, the comments and the test together.
[MAJOR] packages/apps/kubernetes/templates/cluster.yaml:21, the Talos-to-Kubernetes support-matrix guard ignores per-group image versions.
The guard keys only on the cluster-wide .Values.talos.version ($talosMinor := regexFind "^v[0-9]+\.[0-9]+" .Values.talos.version, then a fail if .Values.version is outside that Talos minor's window). This PR adds nodeGroups.<name>.image.builtin.version and image.factory.version, and both cluster.yaml and talos-reconcile-job.yaml resolve the boot disk and the in-guest installer from that per-group version. So a tenant can keep talos.version: v1.13 and version: v1.35 (passes the guard) and set image.factory.version: v1.9.0 on one group; that group boots a Talos minor whose kubelet is outside its support window, which is exactly the "silently broken Talos+kubelet combination that HelmRelease cannot detect" the guard's own comment promises to prevent. The result is an asymmetry: a known-bad combination reached through the cluster-wide field is rejected at render, but the same combination reached through a per-group override is not. Enforcement is cheap because the per-group resolved version already exists in both templates, so the same matrix lookup can run against each resolved group version. The new installer test itself uses v1.15.0, a minor the matrix does not list, with no pushback. What would change my mind: if maintainers consider a per-group version override an advanced, operator-owns-it action equivalent to the documented talos.version-override escape hatch, this drops to MINOR; but the render guard is the safety net, and it is silently absent on the new path.
[MINOR] packages/system/kubernetes-worker-image/values.yaml:28, golden deletion is a footgun with no discoverability mechanism.
The catalog documents "remove it by hand once no node group clones it" (values.yaml:28-31, dv.yaml:10-16) and correctly uses helm.sh/resource-policy: keep, but a worker references its golden only inside a VM dataVolumeTemplates source, so there is no label or query that tells the operator which tenants still pin talos-worker-<schematicID>-<version>. An operator who drops an images[] entry and later hand-deletes the golden believing it unused fails the whole Kubernetes HelmRelease render of every tenant still on that builtin, on their next values/chart change, blocking unrelated addon and scaling changes for those tenants. The guard is also render-time only: an autoscaler scale-up months after the last render clones against live state with no render to re-fire the guard, so a new worker whose golden was deleted just sits Pending. Suggest labelling cloned worker DataVolumes or documenting a kubectl get kuberneteses -A -o ... query so "no node group clones it" is actually checkable.
[MINOR] packages/system/kubernetes-worker-image/templates/dv.yaml:39, catalog value changes collide with DataVolume spec immutability.
The golden DataVolume name is keyed only on (schematicID, version) (dv.yaml:29), while storage, storageClass and the source url (via imageFactoryURL) are mutable values rendered into the spec. values.yaml:39 explicitly invites bumping storage ("bump per image if a future Talos version's raw grows"), but a DataVolume's spec is not mutable in place: patching storage: 6Gi to 8Gi (or repointing the URL) on an existing golden makes the kubernetes-worker-image HelmRelease upgrade emit a rejected patch, and the only recovery is hand-deleting the golden, which trips the deletion footgun above for any pinned tenant. I could not reproduce the webhook rejection hermetically, so there is mild uncertainty on whether Helm's three-way merge always emits the offending patch, but the collision exists by construction. Consider keying the golden name on the full flavor (including a storage/URL discriminator) or documenting that a size/URL change requires a new (schematicID, version) entry rather than an in-place edit.
[MINOR] packages/apps/kubernetes/templates/cluster.yaml:210, tenant-controlled image strings are interpolated unquoted with no schema pattern.
name: {{ $goldenName }} (210) and url: {{ $httpURL }} (213) embed image.builtin.schematicID, version and image.factory.imageFactoryURL, and values.schema.json types all three as a bare string with no pattern (only the quantity fields carry a regex). A value with a newline or YAML metacharacter injects keys into the group's KubevirtMachineTemplate. nindent confines the blast radius to the tenant's own KMT, so this is not a cross-tenant escalation, and the same shape pre-exists for the cluster-wide talos.* fields, but this PR widens the surface. A ^[a-f0-9]{64}$ pattern on schematicID and a ^v[0-9]+\.[0-9]+\.[0-9]+$ pattern on version in the schema close it cheaply and turn a malformed value into a legible dashboard rejection instead of an opaque admission failure.
Claim mismatches
[PARTIAL] "The ghcr.io pull-through mirror stays untouched and still covers kubelet image pull." The mirror helper and its Cilium plumbing are intact, but the line that splices its output into the e2e tenant CR was removed (Finding 1), so in the kubernetes-latest/kubernetes-previous suites the mirror no longer reaches any worker.
Caveats
- No-roll invariant verified by execution, not by trusting the body: the merge-base
cluster.yamland HEAD both render an image-omittedmd0KubevirtMachineTemplate astest-k8s-md0-767d86, so the pinned hash inworker_image_source_test.yamlis the genuine pre-PR value and existing workers do not roll on upgrade. - Mutable-identity of
nodeGroups.<name>.imagecleared: it feeds only the KMT content-hash (cluster.yaml:928-934); the MachineDeployment/MachineHealthCheck/WorkloadMonitor names and the MD selector are<release>-<groupName>, independent ofimage, and old KMTs are preserved until no MachineSet references them. Editingimagere-rolls the group with no prune of the MD and no data loss, consistent with the existingstorageClass"deliberately not immutable" NOTE. Aself == oldSelfguard here would be wrong. - Render-guard-overmatch cleared: the
fails (cluster.yaml:145,161,175,183) key on the golden's own existence /phase == Failed/ storageClass / storage size, live entirely insideif $useClone, and only terminalFailedaborts render (mid-import proceeds). There is no migration or hook writing a "handled-and-safe" signature the guard could misread (the #3315 shape is absent), and the abort is scoped per-tenant to groups that opted intoimage.builtin, not fleet-wide. - Golden-lifecycle sharp edge (a missing/
Failedgolden fails the whole tenant HelmRelease render, not just the disk) is a documented, opt-in tradeoff mitigated byresource-policy: keepand an actionable remedy string. Noted alongside Finding 3 so an operator is not surprised. - Cross-namespace CDI clone RBAC is satisfied in-tree:
packages/system/kubevirt-cdi/templates/cdi-cr.yaml:26-43bindscdi-clone-dvforsystem:serviceaccountsincozy-public, andvm-diskalready clones fromcozy-public, so the clone-source authorization has precedent. Live verification of the helm-controllerlookupread permission oncozy-publicDataVolumes is still out of scope for a static review. - The 5 bootstrap
cozy_invariantsare refuted: the threevalues-prose-driftentries were fixed by commita80c460f(diskSize,talos.{version,imageFactoryURL,installerRepository}and the newWorkerImage*typedefs now describe per-group scope, the clone mechanism and thediskSize >= goldenfloor); the twotemplate-comment-bloatblocks are{{- /* ... */}}Go-template comments that are stripped at render and never ship into a manifest, so that check misfired.
Recommended follow-ups
cozystack-pr-teston a dev cluster for the parts a static review cannot reach: a liveimage.builtinclone fromcozy-publicinto a tenant namespace (cross-namespace CDI clone RBAC plus thelookupread permission), a golden-goes-Failedcase to confirm the render-fail surfaces as a legible HelmRelease condition, and a catalogstoragebump to confirm or refute Finding 4.- Consider comparing
diskSizeagainst the golden PVC'sstatus.capacityrather thanspec.storage.resources.requests.storage: a provisioner that rounds the 6Gi request up can let a workerdiskSizethat passes the render guard still stall the clone at runtime (low confidence, not verified hermetically).
Findings not anchored to changed lines
These reference code outside this PR's diff (unchanged files, or lines outside a hunk), so GitHub cannot render them inline.
[MAJOR] packages/apps/kubernetes/templates/cluster.yaml:21 Talos-to-Kubernetes support-matrix guard ignores per-group image versions
The guard keys only on cluster-wide .Values.talos.version, but this PR adds nodeGroups.<name>.image.builtin.version / image.factory.version, and both cluster.yaml and talos-reconcile-job.yaml resolve the boot disk and installer per group. A tenant can keep talos.version: v1.13 + version: v1.35 (passes the guard) and set image.factory.version: v1.9.0 on one group, booting a Talos minor whose kubelet is outside its support window with no render failure, which is exactly the silently-broken combo the guard exists to catch. Known-bad via the cluster-wide field is rejected; the same via a per-group override is not. Enforcement is cheap since the per-group resolved version already exists in both templates. What would change my mind: if a per-group version override is considered an operator-owns-it action equivalent to the documented talos.version escape hatch, this is MINOR.
| # `talos: { registryMirrors: {...} }`, or nothing when the default applies. The | ||
| # worker OS disk is not sourced here at all — it is cloned from the golden below. | ||
| local talos_block | ||
| talos_block=$(talos_spec_block) |
There was a problem hiding this comment.
[MAJOR] ghcr.io worker-image mirror is computed but never applied to the tenant CR
talos_block=$(talos_spec_block) is kept but this PR deleted the ${talos_block} line from the tenant Kubernetes heredoc below (the diff shows -${talos_block}; spec: now runs straight into addons:). talos_spec_block is the only emitter of spec.talos.registryMirrors, so the applied CR carries no mirror and every worker's kubelet image pull leaves the sandbox to public ghcr.io instead of the warmed ghcr-mirror.kube-system.svc. That is the node-join flake egress (#3513/#3548) this PR targets via #3231, and it fails silently. hack/run-kubernetes-talos-spec_test.bats does not catch it because it tests the helper as an isolated string builder, never the CR splice. Fix: restore the ${talos_block} line under spec:, or delete the helper, assignment, comments and test together if the mirror is intentionally gone.
| ## golden's — so keep the catalog in sync with the (schematicID, version) pairs and | ||
| ## StorageClasses that tenant node groups reference. A golden that is merely | ||
| ## mid-import is not an error: CDI holds the worker disk until the source populates. | ||
| ## Goldens carry `helm.sh/resource-policy: keep`, so removing an `images[]` entry |
There was a problem hiding this comment.
[MINOR] golden deletion is a footgun with no discoverability mechanism
The catalog documents "remove it by hand once no node group clones it" and uses resource-policy: keep, but a worker references its golden only inside a VM dataVolumeTemplates source, so nothing tells the operator which tenants still pin talos-worker-<schematicID>-<version>. Hand-deleting a still-pinned golden fails the whole Kubernetes HelmRelease render of every pinned tenant on their next values/chart change. The guard is render-time only, so an autoscaler scale-up long after the last render clones against live state unguarded and a worker whose golden was deleted just sits Pending. Suggest labelling cloned worker DataVolumes or documenting a kubectl get kuberneteses -A -o ... query.
| storage: | ||
| resources: | ||
| requests: | ||
| storage: {{ .storage | default $.Values.storage | default "6Gi" | quote }} |
There was a problem hiding this comment.
[MINOR] catalog value changes collide with DataVolume spec immutability
The golden DataVolume name is keyed only on (schematicID, version), while storage, storageClass and the source url are mutable values in the spec. values.yaml:39 explicitly invites bumping storage, but a DataVolume spec is not mutable in place: patching storage (or the URL) on an existing golden makes the HelmRelease upgrade emit a rejected patch, and recovery is hand-deleting the golden, which trips the deletion footgun for pinned tenants. Uncertainty: I could not reproduce the webhook rejection hermetically, so whether Helm's three-way merge always emits the patch is unconfirmed, but the collision exists by construction. Consider keying the name on a storage/URL discriminator or documenting that a size/URL change needs a new entry.
| {{- if $useClone }} | ||
| pvc: | ||
| namespace: cozy-public | ||
| name: {{ $goldenName }} |
There was a problem hiding this comment.
[MINOR] tenant-controlled image strings interpolated unquoted with no schema pattern
name: {{ $goldenName }} and url: {{ $httpURL }} (213) embed schematicID, version and imageFactoryURL, none of which carry a pattern in values.schema.json (only the quantity fields do). A newline or YAML metacharacter injects keys into the group's KubevirtMachineTemplate. nindent confines it to the tenant's own KMT (not a cross-tenant escalation) and the shape pre-exists for cluster-wide talos.*, but this PR widens it. Add ^[a-f0-9]{64}$ on schematicID and ^v[0-9]+\.[0-9]+\.[0-9]+$ on version in the schema.
What this PR does
Tenant Kubernetes worker VMs boot their Talos OS disk by a per-worker CDI HTTP import from the Image Factory. Every worker of every cluster independently streams the ~4 GiB raw image, so each node-join depends on Image Factory reachability, which is the root of the flaky
kubernetes-latest/kubernetes-previouse2e failures where N importers race the in-sandbox image-cache across the kube-ovn/Cilium datapath and time out (#3231).This PR makes each worker node group's OS image source an explicit choice, modeled on
vm-disk'ssourceunion and backed by a golden-image catalog likevm-default-images:packages/system/kubernetes-worker-image, an opt-in catalog (disabled by default; enable viabundles.enabledPackages) that seeds Talos worker OS images once asDataVolumes incozy-public, one per(schematicID, version)flavor, namedtalos-worker-<schematicID>-<version>.nodeGroups.<name>.image, a per-node-group union on the tenantKubernetesAPI:image.builtin{schematicID,version}→ boot by CDI storage-layer CSI clone of a catalog golden (no per-worker Factory dependency, no pod-network image transfer);image.factory{version,schematicID,imageFactoryURL}→ import over HTTP from a factory / mirror;talos.*defaults.Default is unchanged. With
imageomitted a group imports over HTTP exactly as before. The renderedKubevirtMachineTemplateis byte-identical to the pre-feature chart (verified: the md0 KMT content hash is identical onmainand this branch), so adopting this PR rolls no existing worker. Cloning is opt-in per group viaimage.builtin. Editing an existing group'simagere-images that group (like changing any image reference). Abuiltingroup whose golden is absent, or whosediskSizeis smaller than the golden's storage, fails the render loudly rather than silently importing.In-guest upgrades are per-group. The
talos-reconcileJob resolves each group'sinstall.imagefrom its ownimagesource, so an upgrade never flips a non-default group's flavor back to the cluster default.Removes the e2e
talos-image-cacheworkaround, 1518 lines counting the helper, its manifest, its own suite and the assertions other suites had grown against it. Kubernetes suites clone the golden instead. Golden imports once from factory and every worker clones it, so it is the same single-import buffer cache provided, separate mirror is redundant. The ghcr.io pull-through mirror stays untouched and still covers kubelet image pull, that is different egress path.talos_spec_blockjust stops emittingimageFactoryURLfor a disk no worker imports anymore.Verification
kubernetes242/242 (incl. newworker_image_source/worker_image_installersuites, covering the golden-failure and same-StorageClass guards), worker-image catalog 3/3,platform133/133, and 69 of 70hack/*.bats. The one failure (ghcr-mirror_test.bats) reproduces the same way on unmodifiedmainin that environment, so it is not from this PR. Plushack/check-worker-image-catalog-defaults.batstying the app's default Talos(schematicID, version)and StorageClass to the catalog's golden (both assertions mutation-proven), andmake generateclean (no drift).mainrenders the image-omitted group'sKubevirtMachineTemplateastest-k8s-md0-767d86, same as this branch, so adopting this PR rolls no existing worker. Checked againstmainitself rather than against the value the regression test pins, and the test pins that hash so a later edit to the image-selection block has to rotate it consciously.openstackraw is ~4.15 GiB, a 4Gi PVC failsDataVolume too small); the worker clone takes the CSI storage-layer path (temp PVC CSI-cloned + resized, no host-assisted upload pod) yielding an RWX disk (live-migration preserved).Rebase note
Rebased onto
mainafter around 360 commits. The e2e area moved under this PR: diagnostics block becamecozy_report_node_join_failure, the ghcr.io pull-through mirror landed, worker CPU sizing changed several times. All of that is kept and the golden-clone changes are re-applied on top.Four bats suites that landed on main in the meantime assert against the deleted image-cache helper without naming it by path. They enumerate collectors and stub helpers, so they would have failed at CI time instead of conflicting. Those assertions are retired in a separate commit.
Notes
cozystack.kubernetes-worker-image, then select goldens per node group withimage.builtin.builtingroup requires the catalog to hold a matching(schematicID, version), else the render fails with a clear message. Falling back to the HTTP import is deliberately not an option: it would make the disk source cluster-state-dependent, flipping the content-hashedKubevirtMachineTemplatename once the golden appeared and rolling the whole group.phase: Failed) fails the render. A golden that is merely mid-import does not: CDI holds the workerDataVolumeuntil the source populates, which recovers unattended. Afail()aborts the entireKubernetesHelmRelease, every resource and not just the disk, for every tenant pinned to that golden, so it is reserved for the state that never recovers on its own.helm.sh/resource-policy: keep, so removing animages[]entry stops managing that golden rather than pruning it. Pruning a golden that tenants still clone would trip the missing-goldenfail()above and strand unrelated tenant operations; cleanup is left to the operator.image.factory.imageFactoryURLredirects only the boot disk; the in-guest upgrade installer uses the cluster-widetalos.installerRepository, so an air-gapped group must also point that at a reachable registry.storageClassdiffers from its golden's rather than degrading quietly. The guard skips when either side is empty (cluster default, not resolvable at render); it rules out the guaranteed-host-assisted case, not every one, since a matching StorageClass still leaves the strategy to the CDI StorageProfile.Release note
Summary by CodeRabbit