Fix: Add missing components to distro-full bundle - #1620
Conversation
Summary of ChangesHello Apinant.u (@LoneExile), 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 addresses critical omissions in the Highlights
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 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 counter productive. 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request correctly identifies and fixes several critical missing components and dependency issues in the distro-full bundle. Adding cozystack-resource-definition-crd and cozystack-resource-definitions resolves the pod failures related to the missing CozystackResourceDefinition CRD. Making cert-manager-issuers non-optional and adding it as a dependency for snapshot-controller correctly addresses the cascading failures in snapshot-controller and other dependent services.
I have one critical suggestion regarding a potential dependency cycle with cozystack-controller. Additionally, based on the PR description, the lineage-controller-webhook also fails due to the missing CRD. You might need to add cozystack-resource-definition-crd to its dependsOn list as well to fully resolve the startup issues.
| releaseName: cozystack-resource-definition-crd | ||
| chart: cozystack-resource-definition-crd | ||
| namespace: cozy-system | ||
| dependsOn: [cilium,cozystack-controller] |
There was a problem hiding this comment.
Based on the pull request description, the cozystack-controller enters a CrashLoopBackOff state because the CozystackResourceDefinition CRD is missing. This indicates that cozystack-controller depends on cozystack-resource-definition-crd, not the other way around. By making cozystack-resource-definition-crd depend on cozystack-controller, you are introducing a circular dependency that will prevent either component from starting correctly.
To fix this, you should remove cozystack-controller from this dependsOn list. It is also highly recommended to update the cozystack-controller release definition (outside the scope of this diff) to explicitly depend on cozystack-resource-definition-crd to ensure the correct installation order.
dependsOn: [cilium]There was a problem hiding this comment.
right!
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdded two new releases to the distro-full bundle, removed Changes
Sequence DiagramsequenceDiagram
participant Bundle as Bundle controller
participant Cilium as cilium
participant Issuers as cert-manager-issuers
participant CRD as cozystack-resource-definition-crd
participant Definitions as cozystack-resource-definitions
participant Snapshot as snapshot-controller
rect rgb(240,255,240)
Bundle->>Cilium: deploy (dependency)
Bundle->>Issuers: deploy (now required)
Bundle->>CRD: deploy (new release)
CRD->>Definitions: ensure CRD present -> Definitions deploy
Cilium->>Snapshot: ready
Issuers->>Snapshot: ready
Snapshot->>Bundle: deploy (dependsOn: cilium, cert-manager-issuers)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
|
Hi Apinant.u (@LoneExile) , thanks for your contribution! We plan to deprecate the concept of bundles, so we'll need some extra time to discuss this patch among the maintainers. I'll try to give feedback by next week. |
|
Thank you for waiting here, this OR probably will be suppressed by #1641 Until that, let's merge it. Apinant.u (@LoneExile) could you please fix DCO: |
4c08bc6 to
e0cc093
Compare
Signed-off-by: Apinant U-suwantim <Hello@Apinant.dev>
Signed-off-by: Apinant U-suwantim <Hello@Apinant.dev>
e0cc093 to
0a93972
Compare
|
Andrei Kvapil (@kvaps) Done! I've rebased the commits with |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/core/platform/bundles/distro-full.yaml (1)
71-75: Verify whetherlineage-controller-webhookalso needscozystack-resource-definition-crddependency.The PR objectives mention that both
cozystack-controllerandlineage-controller-webhookenterCrashLoopBackOffdue to the missingCozystackResourceDefinitionCRD. However,lineage-controller-webhook(line 71–75) does not depend oncozystack-resource-definition-crd, only on[cozystack-controller,cilium,cert-manager].If
lineage-controller-webhookalso consumes the CRD, it should explicitly depend oncozystack-resource-definition-crdto ensure correct installation order.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/platform/bundles/distro-full.yaml(2 hunks)
🔇 Additional comments (3)
packages/core/platform/bundles/distro-full.yaml (3)
95-99: Approved:cert-manager-issuersandsnapshot-controllerdependency updates.The removal of
optional: truefromcert-manager-issuersand the addition ofcert-manager-issuerstosnapshot-controller's dependencies align well with the PR objective to provide the missingselfsigned-cluster-issuer. This ensures proper installation order and availability of the TLS secret thatsnapshot-controllerrequires.Also applies to: 214-218
60-69: Incomplete dependency fix:cozystack-controllershould depend oncozystack-resource-definition-crd.Per the PR objectives,
cozystack-controllercrashes withCrashLoopBackOffbecause theCozystackResourceDefinitionCRD is missing. While the PR adds the CRD release,cozystack-controller(line 60–69) does not explicitly depend oncozystack-resource-definition-crd. This meanscozystack-controllercould still be installed before the CRD, potentially reproducing the original crash.The past review comment (from
gemini-code-assist[bot]) already recommended this fix: "It is also highly recommended to update thecozystack-controllerrelease definition... to explicitly depend oncozystack-resource-definition-crd."Update line 64 to:
- dependsOn: [cilium] + dependsOn: [cilium,cozystack-resource-definition-crd]
77-87: Clarify chart naming: Arecozystack-resource-definition-crdandcozystack-resource-definitionsintentionally unprefixed?The two newly added releases deviate from the
cozy-prefix pattern used by all other charts in this bundle (e.g.,cozy-fluxcd-operator,cozy-cilium,cozy-cert-manager-issuers). Confirm whether this naming is intentional or if these should align with the established convention.
The `distro-full` bundle was missing critical components that exist in
paas-full, causing multiple pod failures during installation. This PR
adds the missing packages and fixes dependency issues.
When installing cozystack with bundle-name: "distro-full", several pods
failed to start:
1. CozystackResourceDefinition CRD missing
- cozystack-controller pod: CrashLoopBackOff
- lineage-controller-webhook pods: CrashLoopBackOff
- Error: no matches for kind "CozystackResourceDefinition" in version
"cozystack.io/v1alpha1"
https://github.com/cozystack/cozystack/blob/a861814c241e38360f03f89ef44eb1791e4800fd/packages/system/cozystack-resource-definition-crd/definition/cozystack.io_cozystackresourcedefinitions.yaml#L1-L14
2. selfsigned-cluster-issuer ClusterIssuer missing
- snapshot-validation-webhook pods: ContainerCreating (waiting for TLS
secret)
- snapshot-controller HelmRelease: Failed to install (timeout)
- Error: clusterissuer.cert-manager.io "selfsigned-cluster-issuer" not
found
https://github.com/cozystack/cozystack/blob/a861814c241e38360f03f89ef44eb1791e4800fd/packages/system/cert-manager-issuers/templates/cluster-issuers.yaml#L52-L57
https://github.com/cozystack/cozystack/blob/a861814c241e38360f03f89ef44eb1791e4800fd/packages/system/snapshot-controller/template/clusterissuer.yaml#L1-L8
3. Cascading failures
- linstor HelmRelease: Blocked (depends on snapshot-controller)
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **New Features**
* Added two new resource-definition components to the platform
distribution for enhanced configuration management.
* **Improvements**
* Made certificate issuer components required during deployment (no
longer optional).
* Adjusted snapshot controller startup order to wait for certificate
issuers, improving startup reliability.
<sub>✏️ Tip: You can customize this high-level summary in your review
settings.</sub>
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
The `distro-full` bundle was missing critical components that exist in
paas-full, causing multiple pod failures during installation. This PR
adds the missing packages and fixes dependency issues.
When installing cozystack with bundle-name: "distro-full", several pods
failed to start:
1. CozystackResourceDefinition CRD missing
- cozystack-controller pod: CrashLoopBackOff
- lineage-controller-webhook pods: CrashLoopBackOff
- Error: no matches for kind "CozystackResourceDefinition" in version
"cozystack.io/v1alpha1"
https://github.com/cozystack/cozystack/blob/a861814c241e38360f03f89ef44eb1791e4800fd/packages/system/cozystack-resource-definition-crd/definition/cozystack.io_cozystackresourcedefinitions.yaml#L1-L14
2. selfsigned-cluster-issuer ClusterIssuer missing
- snapshot-validation-webhook pods: ContainerCreating (waiting for TLS
secret)
- snapshot-controller HelmRelease: Failed to install (timeout)
- Error: clusterissuer.cert-manager.io "selfsigned-cluster-issuer" not
found
https://github.com/cozystack/cozystack/blob/a861814c241e38360f03f89ef44eb1791e4800fd/packages/system/cert-manager-issuers/templates/cluster-issuers.yaml#L52-L57
https://github.com/cozystack/cozystack/blob/a861814c241e38360f03f89ef44eb1791e4800fd/packages/system/snapshot-controller/template/clusterissuer.yaml#L1-L8
3. Cascading failures
- linstor HelmRelease: Blocked (depends on snapshot-controller)
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **New Features**
* Added two new resource-definition components to the platform
distribution for enhanced configuration management.
* **Improvements**
* Made certificate issuer components required during deployment (no
longer optional).
* Adjusted snapshot controller startup order to wait for certificate
issuers, improving startup reliability.
<sub>✏️ Tip: You can customize this high-level summary in your review
settings.</sub>
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
The `distro-full` bundle was missing critical components that exist in
paas-full, causing multiple pod failures during installation. This PR
adds the missing packages and fixes dependency issues.
When installing cozystack with bundle-name: "distro-full", several pods
failed to start:
1. CozystackResourceDefinition CRD missing
- cozystack-controller pod: CrashLoopBackOff
- lineage-controller-webhook pods: CrashLoopBackOff
- Error: no matches for kind "CozystackResourceDefinition" in version
"cozystack.io/v1alpha1"
https://github.com/cozystack/cozystack/blob/a861814c241e38360f03f89ef44eb1791e4800fd/packages/system/cozystack-resource-definition-crd/definition/cozystack.io_cozystackresourcedefinitions.yaml#L1-L14
2. selfsigned-cluster-issuer ClusterIssuer missing
- snapshot-validation-webhook pods: ContainerCreating (waiting for TLS
secret)
- snapshot-controller HelmRelease: Failed to install (timeout)
- Error: clusterissuer.cert-manager.io "selfsigned-cluster-issuer" not
found
https://github.com/cozystack/cozystack/blob/a861814c241e38360f03f89ef44eb1791e4800fd/packages/system/cert-manager-issuers/templates/cluster-issuers.yaml#L52-L57
https://github.com/cozystack/cozystack/blob/a861814c241e38360f03f89ef44eb1791e4800fd/packages/system/snapshot-controller/template/clusterissuer.yaml#L1-L8
3. Cascading failures
- linstor HelmRelease: Blocked (depends on snapshot-controller)
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **New Features**
* Added two new resource-definition components to the platform
distribution for enhanced configuration management.
* **Improvements**
* Made certificate issuer components required during deployment (no
longer optional).
* Adjusted snapshot controller startup order to wait for certificate
issuers, improving startup reliability.
<sub>✏️ Tip: You can customize this high-level summary in your review
settings.</sub>
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
The
distro-fullbundle was missing critical components that exist in paas-full, causing multiple pod failures during installation. This PR adds the missing packages and fixes dependency issues.When installing cozystack with bundle-name: "distro-full", several pods failed to start:
- cozystack-controller pod: CrashLoopBackOff
- lineage-controller-webhook pods: CrashLoopBackOff
- Error: no matches for kind "CozystackResourceDefinition" in version "cozystack.io/v1alpha1"
cozystack/packages/system/cozystack-resource-definition-crd/definition/cozystack.io_cozystackresourcedefinitions.yaml
Lines 1 to 14 in a861814
- snapshot-validation-webhook pods: ContainerCreating (waiting for TLS secret)
- snapshot-controller HelmRelease: Failed to install (timeout)
- Error: clusterissuer.cert-manager.io "selfsigned-cluster-issuer" not found
cozystack/packages/system/cert-manager-issuers/templates/cluster-issuers.yaml
Lines 52 to 57 in a861814
cozystack/packages/system/snapshot-controller/template/clusterissuer.yaml
Lines 1 to 8 in a861814
- linstor HelmRelease: Blocked (depends on snapshot-controller)
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.