feat: implement terminationGracePeriodSeconds for all pods #216
Conversation
WalkthroughHelm chart templates and values for n8n were updated to add a configurable Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
♻️ Duplicate comments (2)
charts/n8n/templates/deployment.worker.yaml (1)
133-135: Same0-value bug as in main deployment – see comment indeployment.yaml.charts/n8n/templates/deployment.webhook.yaml (1)
137-139: Same0-value bug as in main deployment – see comment indeployment.yaml.
🧹 Nitpick comments (1)
charts/n8n/values.yaml (1)
269-271: Avoid hard-wiring the default and clarify nullable behaviour
- By declaring a literal
30here you always emitterminationGracePeriodSecondsin the rendered manifest, even when the user does not intend to change the Kubernetes default (which is already 30 s).- A user cannot set the grace period to
0(immediate kill) or unset it completely vianullbecause the default from this file will override their attempt in avalues-override.yamlunless they also remember to delete the key.Consider:
-terminationGracePeriodSeconds: 30 +# Leave unset by default. Helm templates already fallback to the K8s +# default (30 s) if the key is missing. +# terminationGracePeriodSeconds:This keeps backward compatibility and makes “set/unset” semantics intuitive.
Also applies to: 457-459, 645-647
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
charts/n8n/templates/deployment.webhook.yaml(1 hunks)charts/n8n/templates/deployment.worker.yaml(1 hunks)charts/n8n/templates/deployment.yaml(1 hunks)charts/n8n/values.yaml(3 hunks)
|
Great addition! Would you consider removing the default value for the grace period? Currently, users cannot completely unset the grace period by setting it to |
Hey @RoseSecurity! Thanks for the suggestion. If we don't pass |
|
Hi @Vad1mo! Could you please have a look and approve it? 🙏🏻 |
|
Update readme with new value |
Thanks for the reminder. I updated it. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
README.md (3)
550-552: Document intent clearly for worker pods and avoid implying a hard defaultMirror the guidance used in the main section so operators know they can leave this unset to inherit Kubernetes’ default and only set it when needed for long-running jobs.
Apply this diff:
- # Pod termination grace period in seconds - terminationGracePeriodSeconds: 30 + # Pod termination grace period in seconds. Leave unset to use Kubernetes default (30s). + # terminationGracePeriodSeconds: 120 # example for long-running workersContext note:
- If the templates use a simple
if .Values.worker.terminationGracePeriodSeconds, an explicit0would be treated as “false” and the field would be omitted. Prefer a presence check that still renders0if set (see snippet below).Outside-this-range reference snippet (template idea):
{{- $v := .Values.worker.terminationGracePeriodSeconds -}} {{- if (hasKey .Values.worker "terminationGracePeriodSeconds") and ne $v nil -}} terminationGracePeriodSeconds: {{ $v }} {{- end -}}
738-740: Keep webhook docs consistent and add a short operational hintStay consistent with main/worker docs and add a note that webhook handlers might need longer grace periods during rolling updates to drain in-flight requests.
Apply this diff:
- # Pod termination grace period in seconds - terminationGracePeriodSeconds: 30 + # Pod termination grace period in seconds. Leave unset to use Kubernetes default (30s). + # terminationGracePeriodSeconds: 90 # example: allow draining in-flight webhooks on rolloutOptional doc improvement (outside this range):
- Add a short “Graceful shutdown tips” subsection near the Values section that mentions:
- Consider aligning terminationGracePeriodSeconds with average/max workflow duration.
- Use RollingUpdate with maxUnavailable: 0 for zero-downtime rollouts when increasing grace periods.
- Combine with a preStop hook if you need custom drain logic.
Happy to draft the “Graceful shutdown tips” blurb if you want it included.
362-364: Prefer leaving terminationGracePeriodSeconds unset to inherit the Kubernetes defaultThe Helm chart templates already use
{{- if hasKey .Values.<component> "terminationGracePeriodSeconds" }} terminationGracePeriodSeconds: {{ .Values.<component>.terminationGracePeriodSeconds }} {{- end }}so an explicit
0will be honored (immediate kill) and omitting the key falls back to Kubernetes’ 30 s default.Action items:
• README.md (lines 362–364) – replace the hard-coded default with a commented-out example and note the fallback:
- # Pod termination grace period in seconds - terminationGracePeriodSeconds: 30 + # Pod termination grace period in seconds. Leave unset to use Kubernetes default (30 s). + # terminationGracePeriodSeconds: 60 # uncomment to override• charts/n8n/values.yaml – the default is currently set to
30in three places (main, webhook, worker). Comment these out so users can “unset” and fall back to 30 s:- # Pod termination grace period in seconds - terminationGracePeriodSeconds: 30 + # Pod termination grace period in seconds. Leave unset to use Kubernetes default (30 s). + # terminationGracePeriodSeconds: 30 # uncomment to overrideApply this change in each section around lines ~270 (main), ~458 (webhook), and ~646 (worker).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
README.md(3 hunks)
|
Hi @Vad1mo! Could you please help me to get it merged? 🙏🏻 |
1 similar comment
|
Hi @Vad1mo! Could you please help me to get it merged? 🙏🏻 |
3 participants
Summary
This PR adds support for configuring
terminationGracePeriodSecondson all pod deployments in the n8n Helm chart (main, worker, and webhook). This enhancement allows users to customize the graceful termination period for n8n pods, ensuring workflows can complete properly before pod shutdowns.Motivation
Currently, the n8n Helm chart uses Kubernetes' default
terminationGracePeriodSeconds(30 seconds), which may not be sufficient for long-running (more than 30 seconds) n8n workflows that need time to complete gracefully.Benefits
Summary by CodeRabbit