Merged
Conversation
cwrau
requested review from
marvinWolff,
tasches and
teutonet-bot
as code owners
|
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. WalkthroughAdds a Helm template define Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Helm as Helm (upgrade)
participant HookTpl as Template: recreateOnSizeChangeHook
participant K8s as Kubernetes API
participant Job as Pre-upgrade Job (kubectl)
Helm->>HookTpl: Render chart templates
HookTpl->>K8s: lookup existing StatefulSet(s)
HookTpl->>HookTpl: Compare rendered vs live volumeClaimTemplates (count & storage)
alt Recreation needed
HookTpl-->>Helm: Emit ServiceAccount/Role/RoleBinding + pre-upgrade Job (hook annotations & weight)
Helm->>K8s: Apply RBAC & Job resources
Job->>K8s: Run kubectl to patch PVC sizes and delete StatefulSet --cascade=orphan
K8s-->>Job: Confirm patches/deletions
Job-->>Helm: Hook succeeded
else No recreation
HookTpl-->>Helm: No hook resources emitted
end
Helm->>K8s: Continue applying remaining resources
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (4)📓 Common learnings📚 Learning: 2025-07-24T09:41:28.072ZApplied to files:
📚 Learning: 2025-07-24T09:41:28.072ZApplied to files:
📚 Learning: 2025-07-24T09:49:40.961ZApplied to files:
🪛 YAMLlint (1.37.1)charts/common/templates/_statefulset_recreate_hook.yaml[error] 1-1: syntax error: expected the node content, but found '-' (syntax) 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 |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a Helm template function that automatically creates a pre-upgrade hook to delete StatefulSets when their volume claim templates change, enabling safe recreation of StatefulSets with modified storage configurations.
- Adds kubectl image configuration to global values for use in the hook job
- Creates a template function that detects StatefulSet volume claim template changes and generates a deletion job
- Includes RBAC resources to allow the hook job to delete StatefulSets
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| charts/common/values.yaml | Adds global kubectl image configuration for the StatefulSet recreation hook |
| charts/common/templates/_statefulset_recreate_hook.yaml | Implements the StatefulSet recreation hook template with job, RBAC, and detection logic |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
charts/common/templates/_statefulset_recreate_hook.yaml(1 hunks)charts/common/values.yaml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
charts/common/templates/_statefulset_recreate_hook.yaml
[error] 1-1: syntax error: expected the node content, 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). (2)
- GitHub Check: Update release-please config file for a possibly new chart
- GitHub Check: check licenses
cwrau
force-pushed
the
feat/common/add-statefulset-recreation-job
branch
2 times, most recently
from
e973687 to
a8ba2d9
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
charts/common/templates/_statefulset_recreate_hook.yaml (2)
62-62: Make kubectl image tag truly default to cluster version (don’t override configured tag).Use default on the tag field to avoid accidentally replacing a user-specified/empty tag. Also aligns with prior feedback.
- image: {{ include "common.images.image" (dict "imageRoot" (mustMerge (dict "tag" $.Capabilities.KubeVersion.Version) $.Values.global.kubectl.image) "global" $.Values.global) -}} + image: {{ include "common.images.image" (dict "imageRoot" (mustMerge $.Values.global.kubectl.image (dict "tag" (default $.Capabilities.KubeVersion.Version $.Values.global.kubectl.image.tag))) "global" $.Values.global) -}}
12-12: Default namespace for lookup and emitted resources (prevent empty namespace).Without defaulting, manifests render with an empty namespace and lookup targets "", causing failures.
Apply:
- {{- $currentStatefulset := lookup $newStatefulSet.apiVersion $newStatefulSet.kind $newStatefulSet.metadata.namespace $newStatefulSet.metadata.name -}} + {{- $resourceNamespace := default $.context.Release.Namespace $newStatefulSet.metadata.namespace -}} + {{- $currentStatefulset := lookup $newStatefulSet.apiVersion $newStatefulSet.kind $resourceNamespace $newStatefulSet.metadata.name -}} @@ - namespace: {{ $newStatefulSet.metadata.namespace }} + namespace: {{ $resourceNamespace }} @@ - - --namespace={{ $newStatefulSet.metadata.namespace }} + - --namespace={{ $resourceNamespace }} @@ - namespace: {{ $newStatefulSet.metadata.namespace }} + namespace: {{ $resourceNamespace }} @@ - namespace: {{ $newStatefulSet.metadata.namespace }} + namespace: {{ $resourceNamespace }} @@ - namespace: {{ $newStatefulSet.metadata.namespace }} + namespace: {{ $resourceNamespace }} @@ - namespace: {{ $newStatefulSet.metadata.namespace }} + namespace: {{ $resourceNamespace }}Also applies to: 42-42, 76-76, 87-87, 100-100, 134-134, 145-145
🧹 Nitpick comments (1)
charts/common/templates/_statefulset_recreate_hook.yaml (1)
2-2: Remove no-op merge.Assigning mustMerge to $_ has no effect. Safe to drop.
- {{- $_ := mustMerge . (pick .context "Values" "Capabilities") -}}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
charts/common/templates/_statefulset_recreate_hook.yaml(1 hunks)charts/common/values.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- charts/common/values.yaml
🧰 Additional context used
🪛 YAMLlint (1.37.1)
charts/common/templates/_statefulset_recreate_hook.yaml
[error] 1-1: syntax error: expected the node content, 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). (2)
- GitHub Check: Update release-please config file for a possibly new chart
- GitHub Check: lint helm chart (common)
tasches
previously requested changes
Oct 16, 2025
Collaborator
tasches
left a comment
tasches
left a comment
There was a problem hiding this comment.
check out the suggestions and if they are not applicable dismiss them
marvinWolff
reviewed
Nov 20, 2025
cwrau
force-pushed
the
feat/common/add-statefulset-recreation-job
branch
from
a8ba2d9 to
8461581
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
charts/common/templates/_statefulset_recreate_hook.yaml (2)
62-62: Use consistent image helper for all kubectl containers.Line 62 (delete) correctly uses
common.images.image, but line 72 (patch) usesloki.baseImagefrom the wrong chart. This inconsistency causes incorrect image resolution and breaks PVC patching. Use the same helper and values for both:- image: {{ include "loki.baseImage" (dict "service" (dict "registry" "docker.io" "repository" "rancher/kubectl" "tag" ($.Capabilities.KubeVersion.Version | default "v1.33.0")) "global" $.Values.global.image) }} + image: {{ include "common.images.image" (dict "imageRoot" (mustMerge $.Values.global.kubectl.image (dict "tag" (default $.Capabilities.KubeVersion.Version $.Values.global.kubectl.image.tag))) "global" $.Values.global) -}}Also applies to: 72-72
119-129: Fix RBAC rule: apiGroup and conditional resourceNames.Two issues prevent valid RBAC:
- Line 119: PersistentVolumeClaims are core resources (apiGroup
""), not"v1".- Lines 122–127: When
$templatesis empty,resourceNamesrenders as an empty list, producing invalid YAML. Guard the entire rule or omit it.- apiGroups: - apps resources: - statefulsets resourceNames: - {{ $newStatefulSet.metadata.name }} verbs: - delete - - apiGroups: - - v1 + {{- if gt (len $templates) 0 }} + - apiGroups: + - "" resources: - persistentvolumeclaims resourceNames: - {{- range $index := until (int $newStatefulSet.spec.replicas) }} + {{- range $index := until (int (default 1 $newStatefulSet.spec.replicas)) }} {{- range $template := $templates | keys }} - {{ printf "%s-%s-%d" $template $newStatefulSet.metadata.name $index }} {{- end }} {{- end }} verbs: - patch + {{- end }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
charts/common/templates/_statefulset_recreate_hook.yaml(1 hunks)charts/common/values.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: cwrau
Repo: teutonet/teutonet-helm-charts PR: 1604
File: charts/base-cluster/templates/monitoring/metrics-server/metrics-server.yaml:20-21
Timestamp: 2025-07-24T09:41:28.072Z
Learning: The official kubernetes-sigs/metrics-server Helm chart pins the image tag to a specific version in its values.yaml by default, ensuring reproducible deployments without requiring explicit tag specification in consuming charts.
📚 Learning: 2025-07-24T09:41:28.072Z
Learnt from: cwrau
Repo: teutonet/teutonet-helm-charts PR: 1604
File: charts/base-cluster/templates/monitoring/metrics-server/metrics-server.yaml:20-21
Timestamp: 2025-07-24T09:41:28.072Z
Learning: The kubernetes-sigs/metrics-server Helm chart uses `v{{ .Chart.AppVersion }}` as the default image tag when `image.tag` is empty, which provides pinned versioning through the chart's AppVersion rather than using floating tags like "latest".
Applied to files:
charts/common/templates/_statefulset_recreate_hook.yamlcharts/common/values.yaml
📚 Learning: 2025-07-24T09:41:28.072Z
Learnt from: cwrau
Repo: teutonet/teutonet-helm-charts PR: 1604
File: charts/base-cluster/templates/monitoring/metrics-server/metrics-server.yaml:20-21
Timestamp: 2025-07-24T09:41:28.072Z
Learning: The official kubernetes-sigs/metrics-server Helm chart pins the image tag to a specific version in its values.yaml by default, ensuring reproducible deployments without requiring explicit tag specification in consuming charts.
Applied to files:
charts/common/templates/_statefulset_recreate_hook.yamlcharts/common/values.yaml
📚 Learning: 2025-07-24T09:49:40.961Z
Learnt from: cwrau
Repo: teutonet/teutonet-helm-charts PR: 1602
File: charts/base-cluster/templates/monitoring/kube-prometheus-stack/oauth-proxy.yaml:38-40
Timestamp: 2025-07-24T09:49:40.961Z
Learning: Official Helm charts like oauth2-proxy manage image versioning automatically through their Chart.yaml appVersion field, making manual tag pinning in consumer values unnecessary and potentially harmful. The chart version itself provides reproducibility by ensuring the correct image tag is used.
Applied to files:
charts/common/templates/_statefulset_recreate_hook.yaml
🪛 YAMLlint (1.37.1)
charts/common/templates/_statefulset_recreate_hook.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
🔇 Additional comments (1)
charts/common/values.yaml (1)
2-6: LGTM—Image pinning with digest ensures reproducibility.Pinning kubectl image to a specific SHA digest follows best practices for reproducible deployments. The placement under
global.kubectl.imageprovides a single source of truth for consuming templates.
cwrau
force-pushed
the
feat/common/add-statefulset-recreation-job
branch
from
8461581 to
43b45f1
Compare
including this automatically adds a `pre-upgrade` hook that deletes the referenced statefulset.
cwrau
force-pushed
the
feat/common/add-statefulset-recreation-job
branch
from
43b45f1 to
6471e40
Compare
marvinWolff
approved these changes
Dec 1, 2025
github-merge-queue bot
pushed a commit
that referenced
this pull request
Dec 9, 2025
🤖 I have created a release *beep* *boop* --- ## [1.7.0](common-v1.6.0...common-v1.7.0) (2025-12-09) ### Features * **common:** add statefulset recreate job ([#1723](#1723)) ([6f40d20](6f40d20)) * **common:** centralise helmRepositories templating ([#1844](#1844)) ([31b1629](31b1629)) ### Miscellaneous Chores * **common/dependencies:** update helm release common to v2.31.4 ([#1532](#1532)) ([7df033f](7df033f)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
including this automatically adds a
pre-upgradehook that deletes thereferenced statefulset.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.