Skip to content

[kubernetes] Helm hooks for cleanup - #1606

Merged
Timofei Larkin (lllamnyp) merged 1 commit into
mainfrom
feat/k8s-cleanup-hooks
Nov 7, 2025
Merged

[kubernetes] Helm hooks for cleanup#1606
Timofei Larkin (lllamnyp) merged 1 commit into
mainfrom
feat/k8s-cleanup-hooks

Conversation

@lllamnyp

@lllamnyp Timofei Larkin (lllamnyp) commented Nov 6, 2025

Copy link
Copy Markdown
Member

What this PR does

When deleting a Kubernetes, some resources may linger post deletion because of a race to remove HelmReleases deployed inside the tenant cluster and the removal of the cluster and its controlplane itself. This patch modifies the existing pre-delete hook to remove those helmreleases instead of simply suspending them. Similarly, datavolumes may also remain. These are now delete with a post-delete hook.

Release note

[kubernetes] Use Helm hooks to clean up HelmReleases deployed in tenant
clusters and DataVolumes backing the tenant clusters' PVCs when deleting
a tenant Kubernetes.

Summary by CodeRabbit

  • Chores
    • Added an automated post-delete cleanup job to remove persistent data volumes scoped to the release namespace when a release is deleted.
    • Updated Helm release teardown to actively delete lingering release resources (rather than only suspending them) for cleaner uninstall behavior.
    • Broadened lifecycle hooks to run on successful completions and expanded teardown permissions to list and delete related release artifacts, including gateway CRDs.

@dosubot dosubot Bot added the size/L This PR changes 100-499 lines, ignoring generated files label Nov 6, 2025
@coderabbitai

coderabbitai Bot commented Nov 6, 2025

Copy link
Copy Markdown
Contributor

Note

Other AI code review bot(s) detected

CodeRabbit 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.

Walkthrough

Adds a new Helm post-delete manifest that removes CDI DataVolumes by label and updates an existing HelmReleases teardown Job: the Job command was condensed to a one-line kubectl patch, Role verbs/resourceNames were expanded, and ServiceAccount hook-delete-policy now includes hook-succeeded.

Changes

Cohort / File(s) Summary
CSI DataVolume Cleanup
packages/apps/kubernetes/templates/csi/delete.yaml
New file: Helm post-delete Hook resources (Job, ServiceAccount, Role, RoleBinding) to run kubectl deleting DataVolume resources in the release namespace filtered by cluster-name=<release>. Includes RBAC, tolerations, restartPolicy Never, and --ignore-not-found.
HelmReleases Teardown
packages/apps/kubernetes/templates/helmreleases/delete.yaml
Modified: Replaced multi-line shell patch sequence with a single-line kubectl patch invocation (preserving suspend logic). ServiceAccount helm.sh/hook-delete-policy now includes hook-succeeded. Role verbs expanded to include list and delete; resourceNames extended to include {{ .Release.Name }}-gateway-api-crds. Minor YAML formatting changes.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant Helm as Helm (Release uninstall)
    participant SA1 as ServiceAccount (datavolume-cleanup)
    participant RBAC1 as Role/RoleBinding (csi)
    participant Job1 as Job: datavolume-cleanup
    participant K8s as Kubernetes API
    participant DV as DataVolume

    Helm->>SA1: create ServiceAccount & hook annotations (post-delete)
    Helm->>RBAC1: apply Role & RoleBinding
    Helm->>Job1: create Job (post-delete hook)
    Job1->>K8s: kubectl delete datavolumes --namespace <release> --selector=cluster-name=<release> --ignore-not-found
    K8s->>DV: remove matching DataVolumes
    K8s-->>Job1: return status
Loading
sequenceDiagram
    autonumber
    participant Helm as Helm (Release uninstall)
    participant SA2 as ServiceAccount (helmreleases-teardown)
    participant RBAC2 as Role/RoleBinding (helmreleases)
    participant Job2 as Job: helmreleases-teardown
    participant K8s as Kubernetes API
    participant HR as HelmRelease

    Helm->>SA2: create ServiceAccount & hook annotations (includes hook-succeeded)
    Helm->>RBAC2: apply Role & RoleBinding (updated verbs/resourceNames)
    Helm->>Job2: create teardown Job (hook)
    Job2->>K8s: kubectl patch helmreleases ... --type merge (single-line command)
    K8s->>HR: apply suspend/patch to targeted HelmReleases
    K8s-->>Job2: return status
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Pay attention to RBAC scope and least-privilege for the new Role (added delete) and the modified Role for HelmReleases.
  • Verify correctness of kubectl arguments, label selector, namespace interpolation, and --ignore-not-found.
  • Confirm hook annotations and hook-weight ordering for both ServiceAccounts and Jobs.
  • Validate the one-line patch preserves conditional/suspend behavior from the previous multi-line script.

Possibly related PRs

Suggested reviewers

  • kvaps
  • nbykov0

Poem

🐰 a tidy-hop for the merge:
I scrub the volumes, patch with care,
Hooks align and RBAC's fair,
One-line commands, neat and spry,
I hop away beneath the sky,
Carrots, code, and cleanup flair 🥕

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title '[kubernetes] Helm hooks for cleanup' accurately reflects the main changes: modifying and adding Helm hooks for cleanup operations when deleting a Kubernetes cluster.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat/k8s-cleanup-hooks

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@gemini-code-assist

Copy link
Copy Markdown
Contributor

Summary of Changes

Hello Timofei Larkin (@lllamnyp), 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 enhances the Kubernetes cluster deletion process by implementing more comprehensive cleanup mechanisms. It addresses issues where certain resources, specifically HelmReleases and DataVolumes, could persist after a tenant cluster was deleted. By modifying an existing pre-delete hook and introducing a new post-delete hook, the changes ensure a cleaner and more complete removal of all associated resources, improving the overall reliability of cluster lifecycle management.

Highlights

  • HelmRelease Cleanup: The existing pre-delete Helm hook for HelmReleases has been updated to directly delete these resources instead of merely suspending them, ensuring they are fully removed during cluster teardown.
  • DataVolume Cleanup: A new post-delete Helm hook has been introduced to automatically clean up DataVolumes associated with a Kubernetes tenant cluster, preventing them from lingering after cluster deletion.
  • Hook Delete Policy: The helm.sh/hook-delete-policy for the ServiceAccount used in the HelmRelease cleanup hook was updated to include hook-succeeded, ensuring proper cleanup of the ServiceAccount itself.
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 by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

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 (@gemini-code-assist) Responds in comments when explicitly tagged, both in pull request 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 (@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

  1. 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.

@dosubot dosubot Bot added the kind/feature Categorizes issue or PR as related to a new feature label Nov 6, 2025

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces Helm hooks to clean up lingering resources like HelmReleases and DataVolumes upon cluster deletion, which is a good improvement for resource lifecycle management. However, I've identified a few critical issues that will prevent these hooks from working as intended. The pre-delete hook for HelmReleases will fail because it lacks the necessary delete permission in its RBAC Role and doesn't handle cases where optional addons are not installed. Additionally, the new post-delete hook for DataVolumes references a non-existent Docker image tag. These issues will block the Helm deletion process. I've provided specific suggestions to fix these problems. Please also ensure you update the Role for the flux-teardown service account to include the delete verb for helmreleases, as this change is missing from the PR but is essential for the pre-delete hook to function correctly.

Comment thread packages/apps/kubernetes/templates/csi/delete.yaml
Comment on lines +27 to +43
- kubectl
--namespace={{ .Release.Namespace }}
delete
helmrelease
{{ .Release.Name }}-cilium
{{ .Release.Name }}-gateway-api-crds
{{ .Release.Name }}-csi
{{ .Release.Name }}-cert-manager
{{ .Release.Name }}-cert-manager-crds
{{ .Release.Name }}-vertical-pod-autoscaler
{{ .Release.Name }}-vertical-pod-autoscaler-crds
{{ .Release.Name }}-ingress-nginx
{{ .Release.Name }}-fluxcd-operator
{{ .Release.Name }}-fluxcd
{{ .Release.Name }}-gpu-operator
{{ .Release.Name }}-velero
{{ .Release.Name }}-coredns

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

The kubectl delete command is missing the --ignore-not-found=true flag. Since many of the HelmReleases are for optional addons, this command will fail if any of them don't exist. A failing pre-delete hook will block the entire Helm deletion process.

Additionally, for better readability, consider using a literal block scalar (|) for the multi-line command, as was done in the previous version of this file.

            - |
              kubectl delete helmrelease \
                --namespace={{ .Release.Namespace }} \
                --ignore-not-found=true \
                {{ .Release.Name }}-cilium \
                {{ .Release.Name }}-gateway-api-crds \
                {{ .Release.Name }}-csi \
                {{ .Release.Name }}-cert-manager \
                {{ .Release.Name }}-cert-manager-crds \
                {{ .Release.Name }}-vertical-pod-autoscaler \
                {{ .Release.Name }}-vertical-pod-autoscaler-crds \
                {{ .Release.Name }}-ingress-nginx \
                {{ .Release.Name }}-fluxcd-operator \
                {{ .Release.Name }}-fluxcd \
                {{ .Release.Name }}-gpu-operator \
                {{ .Release.Name }}-velero \
                {{ .Release.Name }}-coredns

Comment on lines +27 to +29
- kubectl -n {{ .Release.Namespace }} delete datavolumes
-l "cluster.x-k8s.io/cluster-name={{ .Release.Name }}"
--ignore-not-found=true

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The YAML syntax for this multi-line command is valid but hard to read and maintain. Using a YAML literal block scalar (|) would make the command much clearer and less error-prone.

            - |
              kubectl -n {{ .Release.Namespace }} delete datavolumes \
                -l "cluster.x-k8s.io/cluster-name={{ .Release.Name }}" \
                --ignore-not-found=true

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
packages/apps/kubernetes/templates/csi/delete.yaml (1)

1-76: Minor: Remove trailing blank lines.

YAMLlint reports excessive blank lines at the end of the file (line 76). Clean up the trailing whitespace to maintain file hygiene.

     namespace: {{ .Release.Namespace }}
-
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4278692 and ac6dc2f.

📒 Files selected for processing (2)
  • packages/apps/kubernetes/templates/csi/delete.yaml (1 hunks)
  • packages/apps/kubernetes/templates/helmreleases/delete.yaml (1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
packages/apps/kubernetes/templates/csi/delete.yaml

[warning] 76-76: too many blank lines (1 > 0)

(empty-lines)


[error] 9-9: syntax error: expected , but found ''

(syntax)

⏰ 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 (3)
packages/apps/kubernetes/templates/helmreleases/delete.yaml (1)

51-51: ServiceAccount hook-delete-policy alignment approved.

Adding hook-succeeded to the ServiceAccount's hook-delete-policy aligns it with the Role and RoleBinding cleanup behavior and ensures consistent cleanup across all hook resources after successful execution.

packages/apps/kubernetes/templates/csi/delete.yaml (2)

27-29: Verify kubectl command syntax and label selector.

The kubectl command uses a label selector to target DataVolumes by cluster name. Ensure that the label cluster.x-k8s.io/cluster-name is consistently applied to all DataVolumes that should be cleaned up on tenant deletion. The use of --ignore-not-found=true is appropriate for handling cases where DataVolumes may have already been removed.

Verify that DataVolumes are labeled with cluster.x-k8s.io/cluster-name={{ .Release.Name }} during their lifecycle so that the label-based selection in this cleanup job will match the intended resources.


50-58: RBAC permissions are appropriately scoped.

The Role grants get, list, and delete verbs on datavolumes in the cdi.kubevirt.io API group, which is necessary and sufficient for the post-delete cleanup Job to function. The permissions are narrow and targeted to the specific resource type.

Comment thread packages/apps/kubernetes/templates/helmreleases/delete.yaml Outdated
@kvaps Andrei Kvapil (kvaps) added the backport Should change be backported on previous release label Nov 6, 2025
@lllamnyp
Timofei Larkin (lllamnyp) force-pushed the feat/k8s-cleanup-hooks branch 2 times, most recently from 03d01b5 to b081c9c Compare November 6, 2025 16:17

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (1)
packages/apps/kubernetes/templates/csi/delete.yaml (1)

23-23: Critical: Docker image does not exist on docker.io — use quay.io instead.

The clastix/kubectl images are published to quay.io, not docker.io. The current image reference will cause an ImagePullBackOff error. Update to quay.io/clastix/kubectl:v1.32 and consider pinning to a digest for better reproducibility.

-          image: docker.io/clastix/kubectl:v1.32
+          image: quay.io/clastix/kubectl:v1.32
🧹 Nitpick comments (2)
packages/apps/kubernetes/templates/csi/delete.yaml (1)

27-29: Improve command readability with a literal block scalar.

The multi-line command is fragmented across three lines making it harder to understand and maintain. Use a YAML literal block scalar (|) for clarity.

           command:
             - /bin/sh
             - -c
-            - kubectl -n {{ .Release.Namespace }} delete datavolumes
-              -l "cluster.x-k8s.io/cluster-name={{ .Release.Name }}"
-              --ignore-not-found=true
+            - |
+              kubectl -n {{ .Release.Namespace }} delete datavolumes \
+                -l "cluster.x-k8s.io/cluster-name={{ .Release.Name }}" \
+                --ignore-not-found=true
packages/apps/kubernetes/templates/helmreleases/delete.yaml (1)

27-45: Consider converting to literal block scalar for better readability.

The kubectl patch command is now spread across 19 lines, with each release name on its own line. While this format is valid and explicit, a literal block scalar (|) with line continuations would be more concise and easier to scan, similar to the first file's refactored style.

           command:
             - /bin/sh
             - -c
-            - kubectl
-              --namespace={{ .Release.Namespace }}
-              patch
-              helmrelease
-              {{ .Release.Name }}-cilium
-              {{ .Release.Name }}-gateway-api-crds
-              {{ .Release.Name }}-csi
-              {{ .Release.Name }}-cert-manager
-              {{ .Release.Name }}-cert-manager-crds
-              {{ .Release.Name }}-vertical-pod-autoscaler
-              {{ .Release.Name }}-vertical-pod-autoscaler-crds
-              {{ .Release.Name }}-ingress-nginx
-              {{ .Release.Name }}-fluxcd-operator
-              {{ .Release.Name }}-fluxcd
-              {{ .Release.Name }}-gpu-operator
-              {{ .Release.Name }}-velero
-              {{ .Release.Name }}-coredns
-              -p '{"spec": {"suspend": true}}'
-              --type=merge --field-manager=flux-client-side-apply || true
+            - |
+              kubectl --namespace={{ .Release.Namespace }} patch helmrelease \
+                {{ .Release.Name }}-cilium \
+                {{ .Release.Name }}-gateway-api-crds \
+                {{ .Release.Name }}-csi \
+                {{ .Release.Name }}-cert-manager \
+                {{ .Release.Name }}-cert-manager-crds \
+                {{ .Release.Name }}-vertical-pod-autoscaler \
+                {{ .Release.Name }}-vertical-pod-autoscaler-crds \
+                {{ .Release.Name }}-ingress-nginx \
+                {{ .Release.Name }}-fluxcd-operator \
+                {{ .Release.Name }}-fluxcd \
+                {{ .Release.Name }}-gpu-operator \
+                {{ .Release.Name }}-velero \
+                {{ .Release.Name }}-coredns \
+                -p '{"spec": {"suspend": true}}' \
+                --type=merge --field-manager=flux-client-side-apply || true
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 03d01b5 and b081c9c.

📒 Files selected for processing (2)
  • packages/apps/kubernetes/templates/csi/delete.yaml (1 hunks)
  • packages/apps/kubernetes/templates/helmreleases/delete.yaml (2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-03T15:25:49.150Z
Learnt from: lllamnyp
Repo: cozystack/cozystack PR: 1579
File: packages/system/cozystack-api/templates/hook.yaml:32-32
Timestamp: 2025-11-03T15:25:49.150Z
Learning: The Docker image tag alpine/k8s:1.33.4 is valid and exists (published Aug 17, 2025).

Applied to files:

  • packages/apps/kubernetes/templates/csi/delete.yaml
📚 Learning: 2025-10-14T04:52:22.093Z
Learnt from: lllamnyp
Repo: cozystack/cozystack PR: 1515
File: packages/system/lineage-controller-webhook/templates/daemonset.yaml:29-34
Timestamp: 2025-10-14T04:52:22.093Z
Learning: In Kubernetes YAML manifests and Helm templates, list items can be formatted with the leading `- ` aligned at the same level as the parent key (e.g., `args:\n- item1\n- item2`) because Kubernetes uses the YAML v2 library which considers the leading `- ` to be part of the indentation. This is the standard style for Kubernetes resources.

Applied to files:

  • packages/apps/kubernetes/templates/csi/delete.yaml
🪛 YAMLlint (1.37.1)
packages/apps/kubernetes/templates/csi/delete.yaml

[warning] 76-76: too many blank lines (1 > 0)

(empty-lines)


[error] 9-9: syntax error: expected , but found ''

(syntax)

🔇 Additional comments (2)
packages/apps/kubernetes/templates/helmreleases/delete.yaml (2)

53-53: Expand hook-delete-policy to include hook-succeeded.

The ServiceAccount's hook-delete-policy is properly extended to before-hook-creation,hook-failed,hook-succeeded, aligning with the new post-delete hook workflows that also use hook-succeeded.


77-77: RBAC fix: gateway-api-crds now included in resourceNames.

The addition of {{ .Release.Name }}-gateway-api-crds to the Role's resourceNames resolves the prior RBAC mismatch flagged in earlier reviews, ensuring the pre-delete cleanup Job has proper authorization to patch all HelmReleases.

"helm.sh/hook": post-delete
"helm.sh/hook-weight": "10"
"helm.sh/hook-delete-policy": hook-succeeded,before-hook-creation,hook-failed
name: {{ .Release.Name }}-datavolume-cleanup

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

YAML syntax error: Quote the hook-delete-policy value.

YAMLlint reports a syntax error on this line. The comma-separated value should be quoted to avoid ambiguity in YAML parsing. For consistency, also quote the key.

-    "helm.sh/hook-delete-policy": hook-succeeded,before-hook-creation,hook-failed
+    "helm.sh/hook-delete-policy": "hook-succeeded,before-hook-creation,hook-failed"

Committable suggestion skipped: line range outside the PR's diff.

🧰 Tools
🪛 YAMLlint (1.37.1)

[error] 9-9: syntax error: expected , but found ''

(syntax)

🤖 Prompt for AI Agents
In packages/apps/kubernetes/templates/csi/delete.yaml around line 9, the
hook-delete-policy key/value is not quoted which YAMLlint flags as a syntax
error; update the template to quote the key and the comma-separated value (e.g.
"helm.sh/hook-delete-policy": "hook1,hook2") so the YAML parser treats the value
as a single string and avoids ambiguity. Ensure you preserve surrounding
indentation and Helm templating syntax when adding the quotes.

- kind: ServiceAccount
name: {{ .Release.Name }}-datavolume-cleanup
namespace: {{ .Release.Namespace }}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Remove trailing blank line.

YAMLlint reports too many blank lines at end of file. Remove the extra blank line to comply with linting rules.

🧰 Tools
🪛 YAMLlint (1.37.1)

[warning] 76-76: too many blank lines (1 > 0)

(empty-lines)

🤖 Prompt for AI Agents
In packages/apps/kubernetes/templates/csi/delete.yaml around line 76, there is
an extra trailing blank line at the end of the file; remove the redundant blank
line(s) so the file ends immediately after the final YAML content (ensure only a
single newline at EOF and no empty lines following the last content) to satisfy
YAMLlint.

## What this PR does

When deleting a Kubernetes, some resources may linger post deletion
because of a race to remove HelmReleases deployed inside the tenant
cluster and the removal of the cluster and its controlplane itself. This
patch modifies the existing pre-delete hook to remove those helmreleases
instead of simply suspending them. Similarly, datavolumes may also
remain. These are now delete with a post-delete hook.

### Release note

```release-note
[kubernetes] Use Helm hooks to clean up HelmReleases deployed in tenant
clusters and DataVolumes backing the tenant clusters' PVCs when deleting
a tenant Kubernetes.
```

Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (1)
packages/apps/kubernetes/templates/csi/delete.yaml (1)

8-8: YAML syntax error: Quote the hook-delete-policy value.

YAMLlint reports a syntax error because the comma-separated value is unquoted. The value must be quoted to ensure the entire string is parsed as a single scalar, not as separate YAML tokens.

-    "helm.sh/hook-delete-policy": hook-succeeded,before-hook-creation,hook-failed
+    "helm.sh/hook-delete-policy": "hook-succeeded,before-hook-creation,hook-failed"

Also applies to: 8-8

🧹 Nitpick comments (1)
packages/apps/kubernetes/templates/csi/delete.yaml (1)

27-29: Improve readability of multi-line command using a literal block scalar.

While the current folded scalar syntax is valid, using a YAML literal block scalar (|) would make the command more readable and maintainable, consistent with best practices for shell commands in Kubernetes manifests.

          command:
            - /bin/sh
            - -c
            - |
              kubectl -n {{ .Release.Namespace }} delete datavolumes \
                -l "cluster.x-k8s.io/cluster-name={{ .Release.Name }}" \
                --ignore-not-found=true
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b081c9c and 63db8ca.

📒 Files selected for processing (2)
  • packages/apps/kubernetes/templates/csi/delete.yaml (1 hunks)
  • packages/apps/kubernetes/templates/helmreleases/delete.yaml (2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-03T15:25:49.150Z
Learnt from: lllamnyp
Repo: cozystack/cozystack PR: 1579
File: packages/system/cozystack-api/templates/hook.yaml:32-32
Timestamp: 2025-11-03T15:25:49.150Z
Learning: The Docker image tag alpine/k8s:1.33.4 is valid and exists (published Aug 17, 2025).

Applied to files:

  • packages/apps/kubernetes/templates/csi/delete.yaml
📚 Learning: 2025-10-14T04:52:22.093Z
Learnt from: lllamnyp
Repo: cozystack/cozystack PR: 1515
File: packages/system/lineage-controller-webhook/templates/daemonset.yaml:29-34
Timestamp: 2025-10-14T04:52:22.093Z
Learning: In Kubernetes YAML manifests and Helm templates, list items can be formatted with the leading `- ` aligned at the same level as the parent key (e.g., `args:\n- item1\n- item2`) because Kubernetes uses the YAML v2 library which considers the leading `- ` to be part of the indentation. This is the standard style for Kubernetes resources.

Applied to files:

  • packages/apps/kubernetes/templates/csi/delete.yaml
🪛 YAMLlint (1.37.1)
packages/apps/kubernetes/templates/csi/delete.yaml

[warning] 76-76: too many blank lines (1 > 0)

(empty-lines)


[error] 9-9: syntax error: expected , but found ''

(syntax)

⏰ 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 (4)
packages/apps/kubernetes/templates/helmreleases/delete.yaml (2)

54-54: Hook deletion policy now properly includes hook-succeeded.

The ServiceAccount annotation has been updated to include hook-succeeded in the hook-delete-policy, which allows the hook resource to be cleaned up even when the Job succeeds. This is the correct behavior for temporary pre-delete cleanup tasks.

Also applies to: 54-54


27-46: The original review comment is incorrect and should be disregarded.

The git diff shows that this PR made only formatting (YAML |>-) and policy changes (hook-succeeded addition, gateway-api-crds inclusion in resourceNames). The kubectl patch command with spec.suspend=true was already present before this PR—it was not modified in this changeset.

The reviewer's claim that the PR objective is to "remove HelmReleases instead of suspending them" does not match the actual scope of this PR. The suspend behavior is pre-existing code unrelated to the changes being made here. If changing suspend to delete is desired, that would be a separate issue tracked independently, not a concern for this PR.

Likely an incorrect or invalid review comment.

packages/apps/kubernetes/templates/csi/delete.yaml (2)

23-23: Verify Docker image tag exists and consider pinning to a digest.

The image docker.io/clastix/kubectl:v1.32 was flagged in previous reviews as potentially non-existent on Docker Hub. A missing or invalid image tag will cause the Job to fail with ImagePullBackOff. Additionally, for better security and reproducibility, pin the image to a specific digest (@sha256:...) rather than relying on a tag.

Please verify that this image tag exists and is the intended version. If unavailable, consider using an alternative such as:

image: docker.io/clastix/kubectl:v1.29

Or use an official image with digest pinning if available in your registry.

Also applies to: 23-23


50-58: RBAC rules are appropriate for the DataVolume cleanup task.

The Role correctly grants get, list, and delete permissions on datavolumes in the cdi.kubevirt.io API group, which aligns with the Job's requirements. The resource scope is properly limited to the namespace via RoleBinding.

Comment on lines +30 to +31


Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Remove excess blank lines at end of manifest.

YAMLlint reports too many blank lines (2 instead of ≤1). Remove the extra blank line before the next manifest separator.

              --ignore-not-found=true
-

-
 ---
🤖 Prompt for AI Agents
In packages/apps/kubernetes/templates/csi/delete.yaml around lines 30 to 31,
there are two blank lines at the end of the manifest causing YAMLlint to
complain; remove the extra blank line so there is at most one blank line before
the next manifest separator (---) or end of file, ensuring only a single newline
remains.

name: {{ .Release.Name }}-datavolume-cleanup
annotations:
helm.sh/hook: post-delete
helm.sh/hook-delete-policy: before-hook-creation,hook-failed,hook-succeeded

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

YAML syntax error: Quote the hook-delete-policy value (also on ServiceAccount annotation).

The ServiceAccount annotation has the same issue as line 8—the comma-separated hook-delete-policy value must be quoted for proper YAML parsing.

-    helm.sh/hook-delete-policy: before-hook-creation,hook-failed,hook-succeeded
+    helm.sh/hook-delete-policy: "before-hook-creation,hook-failed,hook-succeeded"

Also applies to: 39-39

🤖 Prompt for AI Agents
packages/apps/kubernetes/templates/csi/delete.yaml around lines 39 (and also
line 8): the helm hook annotations use a comma-separated value which is not
quoted, causing YAML parsing errors; update the annotations so the
helm.sh/hook-delete-policy value (and the ServiceAccount annotation at line 8)
are wrapped in quotes (e.g. "before-hook-creation,hook-failed,hook-succeeded")
to ensure valid YAML.

@kvaps Andrei Kvapil (kvaps) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dosubot dosubot Bot added the lgtm This PR has been approved by a maintainer label Nov 7, 2025
@lllamnyp
Timofei Larkin (lllamnyp) merged commit 7009c8d into main Nov 7, 2025
22 checks passed
@lllamnyp
Timofei Larkin (lllamnyp) deleted the feat/k8s-cleanup-hooks branch November 7, 2025 09:50
@github-actions

github-actions Bot commented Nov 7, 2025

Copy link
Copy Markdown

Successfully created backport PR for release-0.37:

Andrei Kvapil (kvaps) added a commit that referenced this pull request Nov 7, 2025
myasnikovdaniil added a commit that referenced this pull request Apr 30, 2026
## What this PR does

The `generate-changelog` job in `.github/workflows/tags.yaml` previously
checked out `main` and ran the AI agent there. The agent followed
`docs/agents/changelog.md`, which instructed it to compute the release
range with `git log <previous_version>..HEAD`. That works for minor
releases (cut from `main`), but it breaks for any patch release cut from
a `release-X.Y` branch — `HEAD-on-main` is a strict superset of the tag
and contains commits that were merged to `main` both before and after
the tag.

The v1.3.1 changelog generated by this workflow (#2480) demonstrated the
failure mode: 8 PRs that were merged to `main` but never shipped in
v1.3.1, 6 backport PRs that landed on `release-1.3` *after* v1.3.1 was
tagged, both originals and their backports as separate entries, a
hallucinated 2024 PR (#435), and the `cozystack-ci` bot in the
contributors list. The corrected v1.3.1 changelog is in #2480.

This PR fixes the root cause and tightens the agent guardrails:

* **`tags.yaml`** — check out the release tag commit (`ref: ${{
steps.tag.outputs.tag }}`) instead of `main`, so `HEAD == release
commit` and `git log v<prev>..HEAD` corresponds to what the tag actually
contains. The "Create changelog branch" step still creates the PR branch
from `origin/main`, so PRs continue to merge cleanly.
* **`tags.yaml`** — the AI prompt now states explicitly that `HEAD` is
the release commit and that the upper bound of the range is the new tag,
never `main`.
* **`docs/agents/changelog.md`** — every example replaces `..HEAD` with
`..v<new_version>`, so the instruction is unambiguous regardless of
which branch is checked out.
* **`docs/agents/changelog.md`** — hard rule: backport PRs MUST be
combined with the original into a single entry (`#1606, backport
#1609`), never listed as a second entry. Documented edge case: if the
original isn't in the range, drop the entry entirely (it shipped in a
previous release).
* **`docs/agents/changelog.md`** — forbid using the brief description
verbatim as the detailed description (the failure was `* **fix(foo):
X**: fix(foo): X (...)`), and forbid inventing entries for PRs outside
the release range.
* **`docs/agents/changelog.md`** — filter bot/CI accounts (`app/*`,
`*[bot]`, `cozystack-ci`, `github-actions`, `dependabot`, `renovate`)
out of the human Contributors list. Bot attribution on individual
entries is still valid.

The v1.3.1 changelog correction itself lives in #2480 (separate scope).

### Release note

```release-note
fix(ci): tags.yaml now generates the release changelog from the tag commit instead of `main`, fixing patch-release changelogs (e.g. v1.3.1) that previously included unrelated commits from `main`. The AI prompt and `docs/agents/changelog.md` were tightened to forbid duplicate backport entries, title-as-description, fabricated PRs, and bot accounts in the Contributors list.
```

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **Chores**
* Changelog generation now checks out the release tag commit and
verifies whether the release changelog already exists on main before
generating.
* Updated prompts and tooling to treat the release tag as the upper
bound and avoid using HEAD when computing diffs.
* Enforced combining original and backport PRs into single changelog
entries with explicit “backport #…”.

* **Documentation**
* Strengthened authoring rules (distinct brief/detail, forbid invented
entries, PRs must fall within tag-to-tag ranges).
* Updated examples and guidance to use tag-to-tag comparisons and
improved contributor extraction to use bold @user attribution while
excluding automation accounts and the current release file.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport Should change be backported on previous release kind/feature Categorizes issue or PR as related to a new feature lgtm This PR has been approved by a maintainer size/L This PR changes 100-499 lines, ignoring generated files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants