Repository navigation
fix(platform): restore the isp-hosted dependency closure - #3391
Conversation
The isp-hosted preset shipped bundles.system.enabled: false with paas and naas enabled. templates/bundles/system.yaml is gated end to end on that flag, but it carries two separable things: a variant-selected data-plane head (kubeovn/cilium/linstor on isp-full, a noop networking Package on isp-hosted) and a variant-agnostic control-plane body — cozystack-engine, the sole source of the application-definition-crd component, plus cert-manager, prometheus-operator-crds, postgres-operator and objectstorage-controller. "Hosted" means the host already provides CNI, storage and virtualisation, so only the data-plane head should be dropped. Disabling the whole bundle also deleted the control-plane body, leaving an app catalog whose entire dependency closure was absent: all 24 emitted Packages sat in DependenciesNotReady, the *-rd HelmReleases failed with `no matches for kind "ApplicationDefinition" in version "cozystack.io/v1alpha1"`, and the cozystack-platform HelmRelease never went Ready. That the variant field was still set to isp-hosted — and that system.yaml has an isp-hosted branch which could never render — shows the disable was the mistake, not the intent; iaas.yaml already refuses to render on this variant, encoding the same "system bundle minus iaas" contract. The preset has never worked since it was introduced in 2d022e3. Enable the system bundle for the preset. isp-hosted now emits 53 Packages with every dependsOn satisfied, and installs 59 components against the 60 of the pre-refactor bundles/paas-hosted.yaml, with no LINSTOR, KubeVirt, CAPI or Kamaji leakage. Also move cozystack.bucket-application from the iaas bundle into the system bundle. Its own dependsOn closure is networking, objectstorage-controller and cozystack-engine, none of which are iaas, and cozystack.backupstrategy-controller — a default package of the system bundle — hard-depends on it. Any system-on/iaas-off configuration therefore wedged backupstrategy-controller; both isp-full presets masked this by enabling iaas. Guard the class of bug rather than only these two instances. The existing paas/naas/iaas guards check bundles.system.variant, which keeps its value when the bundle is disabled, so they passed on exactly this configuration. Make all three require bundles.system.enabled too and point the operator at the supported topology, turning a silent multi-hour DependenciesNotReady hang into a render-time failure. Covered by two helm-unittest suites: one pinning the dependency closure the catalog needs — including a case that loads values-isp-hosted.yaml unmodified, so a regression in the preset itself is caught — and one pinning the guards. The fix changes and the guards were mutation-tested by reverting each in turn and confirming the suite fails. This restores the dependency closure; it is not by itself sufficient for a fully converged hosted install. Review surfaced further hosted-unaware defaults that the dead bundle had been masking — notably apps/tenant rendering cilium.io/v2 policies with no .Capabilities guard, and extra/monitoring defaulting logsStorages to the LINSTOR-only "replicated" StorageClass. Those are tracked separately. bundles.disabledPackages can also still remove a dependency while its dependents stay emitted; the guards do not cover that path. Refs #3376 Signed-off-by: Myasnikov Daniil <[email protected]>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe hosted preset now enables the system bundle, moves ChangesHosted bundle dependency closure
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
github-actions
Bot
added
size/L
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 dependency closure issue in the isp-hosted platform variant where the system bundle was incorrectly disabled. By restoring the system bundle and moving shared components like the bucket-application to the system bundle, the platform now correctly satisfies dependency requirements for PaaS and NaaS catalogs. Additionally, new configuration guards and unit tests were added to prevent similar misconfigurations and ensure the platform fails early with descriptive errors if required bundles are missing. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request ensures that the system bundle's control-plane body is always enabled when other bundles (IaaS, PaaS, NaaS) are active, preventing dependency resolution failures in hosted topologies. It introduces template validation guards, moves bucket-application to the system bundle, updates the isp-hosted preset, and adds comprehensive unit tests. The review feedback focuses on improving the accuracy of comments and ensuring consistent use of fully-qualified package names in error messages.
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.
| presets masked that by enabling iaas. Same reasoning as the velero comment | ||
| below: a hard dependency of a default package must itself be emitted |
There was a problem hiding this comment.
The comment refers to the velero comment as being "below", but the velero comment actually resides above this block (lines 221-225). Let's update "below" to "above" to keep the documentation accurate.
presets masked that by enabling iaas. Same reasoning as the velero comment
above: a hard dependency of a default package must itself be emitted| depends on its control-plane body — rendering iaas without it leaves all 12 | ||
| Packages in DependenciesNotReady with nothing naming the cause. */ -}} | ||
| {{- if and .Values.bundles.iaas.enabled (not .Values.bundles.system.enabled) }} | ||
| {{- fail "bundles.iaas.enabled requires bundles.system.enabled: the iaas bundle depends on packages emitted by the system bundle (cozystack.networking, cozystack-engine, cert-manager, prometheus-operator-crds and cozystack-basics). Enable the system bundle with bundles.system.variant: isp-full or isp-full-generic." }} |
There was a problem hiding this comment.
For consistency and easier searchability, let's use the exact package names (with the cozystack. prefix) in the error message, matching how they are defined in the templates.
{{- fail "bundles.iaas.enabled requires bundles.system.enabled: the iaas bundle depends on packages emitted by the system bundle (cozystack.networking, cozystack.cozystack-engine, cozystack.cert-manager, cozystack.prometheus-operator-crds and cozystack.cozystack-basics). Enable the system bundle with bundles.system.variant: isp-full or isp-full-generic." }}| imply the system bundle is actually emitted, and every package below depends | ||
| on its control-plane body. */ -}} | ||
| {{- if and .Values.bundles.naas.enabled (not .Values.bundles.system.enabled) }} | ||
| {{- fail "bundles.naas.enabled requires bundles.system.enabled: the naas catalog depends on packages emitted by the system bundle (cozystack-engine, which ships the ApplicationDefinition CRD, plus cozystack.networking). To run a hosted platform where the host provides CNI, storage and virtualisation, keep bundles.system.enabled: true and set bundles.system.variant: isp-hosted (which selects a noop networking Package) with bundles.iaas.enabled: false — do not disable the system bundle." }} |
There was a problem hiding this comment.
For consistency and accuracy, let's use the exact package name cozystack.cozystack-engine instead of cozystack-engine in the error message.
{{- fail "bundles.naas.enabled requires bundles.system.enabled: the naas catalog depends on packages emitted by the system bundle (cozystack.cozystack-engine, which ships the ApplicationDefinition CRD, plus cozystack.networking). To run a hosted platform where the host provides CNI, storage and virtualisation, keep bundles.system.enabled: true and set bundles.system.variant: isp-hosted (which selects a noop networking Package) with bundles.iaas.enabled: false — do not disable the system bundle." }}| DependenciesNotReady forever and the cozystack-platform HelmRelease never goes | ||
| Ready, with no error pointing at the cause. Fail the render instead. */ -}} | ||
| {{- if and .Values.bundles.paas.enabled (not .Values.bundles.system.enabled) }} | ||
| {{- fail "bundles.paas.enabled requires bundles.system.enabled: the paas catalog depends on packages emitted by the system bundle (cozystack-engine, which ships the ApplicationDefinition CRD, plus cozystack.networking, cert-manager, prometheus-operator-crds, postgres-operator and objectstorage-controller). To run a hosted platform where the host provides CNI, storage and virtualisation, keep bundles.system.enabled: true and set bundles.system.variant: isp-hosted (which selects a noop networking Package) with bundles.iaas.enabled: false — do not disable the system bundle." }} |
There was a problem hiding this comment.
For consistency and easier searchability, let's use the exact package names (with the cozystack. prefix) in the error message.
{{- fail "bundles.paas.enabled requires bundles.system.enabled: the paas catalog depends on packages emitted by the system bundle (cozystack.cozystack-engine, which ships the ApplicationDefinition CRD, plus cozystack.networking, cozystack.cert-manager, cozystack.prometheus-operator-crds, cozystack.postgres-operator and cozystack.objectstorage-controller). To run a hosted platform where the host provides CNI, storage and virtualisation, keep bundles.system.enabled: true and set bundles.system.variant: isp-hosted (which selects a noop networking Package) with bundles.iaas.enabled: false — do not disable the system bundle." }}Address review feedback on the guards added alongside the isp-hosted dependency-closure fix. Spell every package the guards name in its fully-qualified form (cozystack.cozystack-engine rather than cozystack-engine, and likewise for cert-manager, prometheus-operator-crds, postgres-operator, objectstorage-controller and cozystack-basics). The messages exist to be acted on, and an operator who greps the emitted Package CRs for the name they were given should find it. Also correct the direction of the cross-reference in the bucket-application comment: the velero comment it points at sits above the block, not below. No behavioural change — the guards fire on the same conditions and the tests match on the unchanged leading clause. Signed-off-by: Myasnikov Daniil <[email protected]>
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
LGTM — restores the isp-hosted dependency closure at its in-repo root cause, and the tests pin both the emitted closure and the new render-time guards.
Business context: The isp-hosted preset shipped with the system bundle disabled while its paas/naas app catalogs stayed enabled, so every emitted Package hung in DependenciesNotReady and the platform HelmRelease never became Ready; this PR re-enables the bundle for that variant, relocates bucket-application into it, and adds render-time guards against the same misconfiguration.
I verified every load-bearing claim against source rather than the description:
- Root cause is in this repo and the fix targets it:
templates/bundles/system.yamlis gated end to end onbundles.system.enabled, and the variant-agnostic control-plane body (cozystack-engine— the sole source of the ApplicationDefinition CRD — pluscert-manager,prometheus-operator-crds,postgres-operator,objectstorage-controller) sits below the variant branches, so disabling the bundle deleted the whole closure the catalog renders against. The preset has shipped this way since the bundle refactor, so it never converged. - The
bucket-applicationmove (iaas → system) is correct and single-source:backupstrategy-controller, a default package of the system bundle, hard-depends on it, while its owndependsOnclosure (networking+objectstorage-controller+cozystack-engine) is entirely system-bundle with nothing iaas/KubeVirt/CAPI. It is now emitted from exactly one place, soisp-fullstill emits it once and is behavior-preserving. - No new wedge is introduced on hosted: none of the packages emitted in the variant-agnostic body depend on a data-plane-only package (
linstor,kubeovn-*,multus,metallb,reloader,snapshot-controller,securitygroup-controllerare all confined to the isp-full-only common-packages helper), andcozystack-scheduler'sdefaultvariant carries nolinstor-schedulerdependency. - The new guards close a real hole — the pre-existing checks read only
bundles.system.variant, which keeps its value when the bundle is off — and fire on no supported preset:default,isp-fullandisp-full-genericall setsystem.enabled: truewherever a catalog bundle is on. They convert a silent multi-hourDependenciesNotReadyhang into a render-time failure that names the remedy. - Tests are substantive and non-vacuous: both suites pass and the full platform suite is green (23 suites / 97 tests). They assert each guard fires, assert the remedy text, keep a positive control that the supported config still renders, load the real
values-isp-hosted.yaml(which catches the exact regression that shipped), and use the load-bearingany: trueon the negatedcontainsDocumentfor the data-plane exclusion.
Non-blocking follow-ups
- Non-blocking (PR description, not code): the "two isp-full variants are byte-identical before and after" claim is functionally correct (identical Package set and dependency edges), but a raw
helm templatestream does differ because thebucket-applicationdocument moves from the iaas block to the system block. Consider softening to "the emitted package set is unchanged". - Non-blocking (scope acknowledgement, correct as-is): the guards intentionally cover the bundle flags only, not
bundles.disabledPackages, and nothing in CI exercisesisp-hosted— both are already tracked as separate follow-ups, so these unit suites are the only automated backstop for this path. Appropriate for this layer; noting for visibility.
2 participants
Problem
Fixes #3376. The
isp-hostedpreset shippedbundles.system.enabled: falsewhilepaasandnaaswere enabled, so it enabled an app catalog whose dependency closure it simultaneously disabled. Every emitted Package sat inDependenciesNotReady, the*-rdHelmReleases failed withno matches for kind "ApplicationDefinition" in version "cozystack.io/v1alpha1", and thecozystack-platformHelmRelease never went Ready.Rendering the preset at
origin/mainshows the scale: 24 emitted Packages, 24 of them blocked, by 6 missing dependencies — two more than the issue lists (cert-managerandprometheus-operator-crdsare also absent).isp-fullrenders 75 Packages with a fully satisfied closure, which is a clean control.Root cause
templates/bundles/system.yamlis gated end to end onbundles.system.enabled, but it carries two separable things: a variant-selected data-plane head (kubeovn/cilium/linstor onisp-full, a noop networking Package onisp-hosted) and a variant-agnostic control-plane body —cozystack-engine, the sole source of theapplication-definition-crdcomponent, pluscert-manager,prometheus-operator-crds,postgres-operatorandobjectstorage-controller."Hosted" means the host already provides CNI, storage and virtualisation, so only the data-plane head should be dropped. Disabling the whole bundle also deleted the control-plane body that every variant needs.
Three things show the disable was the mistake rather than the intent: the preset still sets
variant: "isp-hosted";system.yamlcontains anisp-hostedbranch (noop networking plus default scheduler) that could never render; andiaas.yamlalready refuses to render on this variant, encoding the same "system bundle minus iaas" contract. The preset has never worked — it was born this way in2d022e38e(January 2026), the refactor that replacedbundles/paas-hosted.yaml, whose contents confirm hosted was always meant to ship the engine,cert-managerandprometheus-operator-crds.Changes
values-isp-hosted.yaml— enable the system bundle. Hosted now emits 53 Packages with everydependsOnsatisfied, installing 59 components against the 60 of the pre-refactorpaas-hostedbundle, with no LINSTOR, KubeVirt, CAPI or Kamaji leakage.cozystack.bucket-applicationfrom the iaas bundle into the system bundle. Its owndependsOnclosure isnetworking+objectstorage-controller+cozystack-engine, none of them iaas, whilecozystack.backupstrategy-controller— a default package of the system bundle — hard-depends on it. Any system-on/iaas-off configuration therefore wedgedbackupstrategy-controller; bothisp-fullpresets masked this by enabling iaas. This is a second, independent bug not reported in the issue.paas,naasandiaasnow also requirebundles.system.enabled. The pre-existing checks looked only atbundles.system.variant, which keeps its value when the bundle is disabled, so they passed on exactly this configuration.iaashad the identical hole (12 Packages, 6 unsatisfied deps). This turns a silent multi-hourDependenciesNotReadyhang into a render-time failure that names the remedy.Verification
default0 Packages,isp-hosted53,isp-full75,isp-full-generic75. The twoisp-fullvariants are byte-identical before and after, so the bucket move is a no-op for them.packages/core/platform; repo-widehack/helm-unit-tests.shis green.paas/naasdisabled) still renders cleanly; the new guards do not fire on it.cozystack-apiis a Deployment with only a preferred control-plane affinity plus blanket tolerations, andcozystack-scheduleruses itsdefaultvariant (plain Deployment, no LINSTOR extender) — both schedule fine on a cluster with no control-plane nodes.What this does not fix
This restores the dependency closure. It is not by itself sufficient for a fully converged hosted install. Review surfaced further hosted-unaware defaults that the dead bundle had been masking, filed separately:
apps/tenantrenders 14cilium.io/v2policies with no.Capabilitiesguard, sotenant-rootfails on a non-Cilium cluster. This is the blocker for actual hosted convergence, and it needs a deliberate decision rather than a reflexive capability guard, because those policies implement tenant network isolation.extra/monitoringdefaultslogsStoragesto the LINSTOR-onlyreplicatedStorageClass.bundles.disabledPackagescan still break the closure with no render-time error; the guards here cover the bundle flags only.isp-hosted, which is why all of the above accumulated undetected.Upgrade note
Existing
*-applicationPackage CRs carryhelm.sh/resource-policy: keep. An operator who applied the #3376 workaround but did not also delete the retained Package CRs (as that issue instructs) will find them install once this change makes their dependencies Ready, since the reconciler re-enqueues dependents on readiness. Deleting the unwanted Packages before upgrading avoids that.Summary by CodeRabbit
isp-hostedvariant.