[kubernetes] Add dependency for snapshot CRD and migration to latest version #1275
Conversation
…version Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
WalkthroughThis change increments the Kubernetes app chart version from 0.26.0 to 0.26.1, updates the version mapping to reflect the new release, adds a dependency on the volumesnapshot CRD HelmRelease in the CSI HelmRelease template, and introduces a migration script to update custom resources to appVersion 0.26.1. Changes
Sequence Diagram(s)sequenceDiagram
participant MigrationScript
participant KubernetesAPI
participant ConfigMap
MigrationScript->>KubernetesAPI: List all kuberneteses.apps.cozystack.io (all namespaces)
loop For each resource
MigrationScript->>KubernetesAPI: Patch appVersion to "0.26.1"
end
MigrationScript->>ConfigMap: Create/Update cozystack-version ConfigMap (version=18)
sequenceDiagram
participant HelmReleaseCSI
participant HelmReleaseVolumeSnapshotCRD
HelmReleaseCSI->>HelmReleaseVolumeSnapshotCRD: dependsOn (wait for CRD HelmRelease readiness)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Summary of Changes
Hello @kvaps, 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 updates the Kubernetes application chart to version 0.26.1. The primary goal is to establish a critical dependency on the volume snapshot Custom Resource Definition (CRD) for the CSI components, ensuring proper deployment order. Additionally, it includes a new migration script to facilitate the upgrade of existing Kubernetes application instances to this updated version.
Highlights
- Kubernetes Chart Version Update: The Kubernetes application chart version has been incremented from
0.26.0to0.26.1inChart.yaml. - Volume Snapshot CRD Dependency: A new
dependsOnentry has been added to the CSI HelmRelease intemplates/helmreleases/csi.yaml. This ensures that thevolumesnapshot-crd-for-tenant-k8sresource is available before the CSI components are deployed, establishing a crucial dependency. - New Migration Script: A new migration script,
scripts/migrations/17, has been introduced. This script is designed to automatically update existingkuberneteses.apps.cozystack.ioresources by patching theirappVersionto0.26.1and incrementing thecozystack-versionconfigmap to18.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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-code-assist comments or fill out our survey to provide feedback.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request updates the kubernetes app, introduces a dependency on the volume snapshot CRD, and adds a migration script. The chart and version map changes are correct. However, the new migration script has a critical error handling vulnerability that could lead to silent failures, and a potential issue in the kubectl patch command that needs verification to ensure the migration is effective.
| @@ -0,0 +1,10 @@ | |||
| #!/bin/sh | |||
There was a problem hiding this comment.
The script lacks robust error handling, which can lead to silent failures and leave the system in an inconsistent state. I strongly recommend adding set -eo pipefail at the beginning of the script to ensure that the script exits immediately if any command fails and that the pipeline's exit code reflects the failure of any command in the pipeline, not just the last one.
#!/bin/sh
set -eo pipefail
|
|
||
| # Upgrade kubernetes.apps to new chart version | ||
| kubectl get kuberneteses.apps.cozystack.io -A --no-headers --output=custom-columns='NAMESPACE:.metadata.namespace,NAME:.metadata.name' | while read NAMESPACE NAME; do | ||
| kubectl patch kuberneteses.apps.cozystack.io -n "$NAMESPACE" "$NAME" --type merge -p '{"appVersion":"0.26.1"}' |
There was a problem hiding this comment.
The patch '-p '{\"appVersion\":\"0.26.1\"}' targets a top-level appVersion field. Please verify the structure of the kuberneteses.apps.cozystack.io CRD. If the appVersion field is under spec, the patch will not work as intended and could either fail or be ignored, leaving existing resources un-migrated. The patch should be updated to target the correct path, which is likely spec.appVersion.
kubectl patch kuberneteses.apps.cozystack.io -n "$NAMESPACE" "$NAME" --type merge -p '{"spec":{"appVersion":"0.26.1"}}'
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
scripts/migrations/17 (1)
9-10: Race condition: ConfigMap may already exist at version 18
kubectl create … | kubectl applyis idempotent, but if multiple installers run concurrently there is still a chance of conflict. Consider usingkubectl patchorkubectl label --overwritewith a retry loop to make this bullet-proof.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/apps/kubernetes/Chart.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/csi.yaml(1 hunks)packages/apps/versions_map(1 hunks)scripts/migrations/17(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: lllamnyp
PR: cozystack/cozystack#1130
File: hack/e2e-apps/kubernetes.bats:101-101
Timestamp: 2025-07-11T06:11:25.438Z
Learning: In cozystack, the plural form for the Kubernetes custom resource is `kuberneteses.apps.cozystack.io`, not `kubernetes.apps.cozystack.io`. This is defined in the API schema even though it's not grammatically perfect.
scripts/migrations/17 (1)
Learnt from: lllamnyp
PR: #1130
File: hack/e2e-apps/kubernetes.bats:101-101
Timestamp: 2025-07-11T06:11:25.438Z
Learning: In cozystack, the plural form for the Kubernetes custom resource is kuberneteses.apps.cozystack.io, not kubernetes.apps.cozystack.io. This is defined in the API schema even though it's not grammatically perfect.
packages/apps/versions_map (1)
Learnt from: kvaps
PR: #1206
File: packages/apps/versions_map:30-31
Timestamp: 2025-07-16T13:17:07.580Z
Learning: In packages/apps/versions_map, using HEAD for certain versions (like ferretdb 1.0.0) is intentional and part of the core logic, not an oversight that needs to be fixed for reproducibility.
🧬 Code Graph Analysis (2)
scripts/migrations/17 (1)
scripts/installer.sh (1)
run_migrations(8-20)
packages/apps/kubernetes/templates/helmreleases/csi.yaml (1)
internal/controller/tenant_helm_reconciler.go (2)
annotateTenantRootNs(132-158)Reconcile(27-106)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (1)
packages/apps/kubernetes/Chart.yaml (1)
19-19: Chart version bump looks correct but remember to update accompanying docs/release-notes
The Semantic-Version bump is fine; however, without a matching entry in CHANGELOG / release-notes, downstream operators will not know what changed between 0.26.0 and 0.26.1.
| - name: {{ .Release.Name }}-volumesnapshot-crd-for-tenant-k8s | ||
| namespace: {{ .Release.Namespace }} | ||
| {{- if lookup "helm.toolkit.fluxcd.io/v2" "HelmRelease" .Release.Namespace .Release.Name }} |
There was a problem hiding this comment.
Unconditional dependency can dead-lock the HelmRelease
dependsOn will block reconciliation until the referenced HelmRelease exists and is ready. Because the new VolumeSnapshot CRD release is not created by this template and has no safeguard (unlike the next item wrapped in lookup), Flux will wait forever on a missing object.
Add the same guard or move CRD creation into this chart.
- - name: {{ .Release.Name }}-volumesnapshot-crd-for-tenant-k8s
- namespace: {{ .Release.Namespace }}
+ {{- if lookup "helm.toolkit.fluxcd.io/v2" "HelmRelease" .Release.Namespace (printf "%s-volumesnapshot-crd-for-tenant-k8s" .Release.Name) }}
+ - name: {{ .Release.Name }}-volumesnapshot-crd-for-tenant-k8s
+ namespace: {{ .Release.Namespace }}
+ {{- end }}🤖 Prompt for AI Agents
In packages/apps/kubernetes/templates/helmreleases/csi.yaml around lines 38 to
40, the dependsOn reference to the VolumeSnapshot CRD HelmRelease is
unconditional, causing potential deadlocks if the referenced HelmRelease does
not exist. To fix this, add a conditional guard using the lookup function to
check for the existence of the HelmRelease before depending on it, similar to
the pattern used for the next item, or alternatively move the CRD creation into
this chart to ensure it is managed and available.
| kubernetes 0.26.0 9584e5f5 | ||
| kubernetes 0.26.1 HEAD |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Confirm the pinned commit hash exists and is immutable
9584e5f5 must be an existing commit in the repo; otherwise consumers relying on reproducible builds will break.
If this command fails, fix the hash before merging.
🏁 Script executed:
#!/bin/bash
git cat-file -e 9584e5f5^{commit}Length of output: 86
Ensure the pinned commit hash is valid
The commit 9584e5f5 referenced in packages/apps/versions_map (lines 64–65) does not exist in this repository, which will break reproducible builds. Please update it to a valid, immutable commit:
• File: packages/apps/versions_map
Lines: 64–65
Current snippet:
kubernetes 0.26.0 9584e5f5
kubernetes 0.26.1 HEAD
Please replace 9584e5f5 with an existing commit hash (e.g., from the kubernetes submodule or upstream tag) and verify with:
git cat-file -e <new-hash>^{commit}🤖 Prompt for AI Agents
In packages/apps/versions_map at lines 64 to 65, the commit hash '9584e5f5' for
kubernetes 0.26.0 is invalid and will break reproducible builds. Replace this
hash with a valid, immutable commit hash from the kubernetes repository that
corresponds to version 0.26.0. Verify the new hash exists by running 'git
cat-file -e <new-hash>^{commit}' before updating the file.
| #!/bin/sh | ||
| # Migration 17 --> 18 | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add strict error handling to avoid half-applied migrations
Use set -euo pipefail so the script stops on the first failure instead of silently continuing.
-#!/bin/sh
+#!/bin/sh
+set -euo pipefail📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #!/bin/sh | |
| # Migration 17 --> 18 | |
| #!/bin/sh | |
| set -euo pipefail | |
| # Migration 17 --> 18 | |
🤖 Prompt for AI Agents
In scripts/migrations/17 at the beginning of the file (lines 1 to 3), add the
line `set -euo pipefail` right after the shebang (`#!/bin/sh`). This will enable
strict error handling, causing the script to exit immediately if any command
fails, if any variable is unset, or if any part of a pipeline fails, preventing
partial or half-applied migrations.
| kubectl get kuberneteses.apps.cozystack.io -A --no-headers --output=custom-columns='NAMESPACE:.metadata.namespace,NAME:.metadata.name' | while read NAMESPACE NAME; do | ||
| kubectl patch kuberneteses.apps.cozystack.io -n "$NAMESPACE" "$NAME" --type merge -p '{"appVersion":"0.26.1"}' | ||
| done |
There was a problem hiding this comment.
Patch path likely wrong – field is inside .spec
Most Cozystack CRDs store the application version under spec.appVersion, not root level. If that’s the case, this patch is a no-op.
- kubectl patch kuberneteses.apps.cozystack.io -n "$NAMESPACE" "$NAME" --type merge -p '{"appVersion":"0.26.1"}'
+ kubectl patch kuberneteses.apps.cozystack.io -n "$NAMESPACE" "$NAME" --type merge -p '{"spec":{"appVersion":"0.26.1"}}'Please verify the CRD schema before running the migration.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| kubectl get kuberneteses.apps.cozystack.io -A --no-headers --output=custom-columns='NAMESPACE:.metadata.namespace,NAME:.metadata.name' | while read NAMESPACE NAME; do | |
| kubectl patch kuberneteses.apps.cozystack.io -n "$NAMESPACE" "$NAME" --type merge -p '{"appVersion":"0.26.1"}' | |
| done | |
| kubectl get kuberneteses.apps.cozystack.io -A --no-headers --output=custom-columns='NAMESPACE:.metadata.namespace,NAME:.metadata.name' | while read NAMESPACE NAME; do | |
| kubectl patch kuberneteses.apps.cozystack.io -n "$NAMESPACE" "$NAME" --type merge -p '{"spec":{"appVersion":"0.26.1"}}' | |
| done |
🤖 Prompt for AI Agents
In scripts/migrations/17 around lines 5 to 7, the patch command incorrectly
targets the appVersion field at the root level, but it should be inside the spec
object. Update the patch JSON to set "spec.appVersion" instead of "appVersion"
by changing the patch to '{"spec":{"appVersion":"0.26.1"}}' to correctly update
the field according to the CRD schema.
|
Successfully created backport PR for |
Signed-off-by: Andrei Kvapil kvapss@gmail.com
What this PR does
Release note
Summary by CodeRabbit