feat: Add HPA for Autoscaling Webhook and Workers#286
feat: Add HPA for Autoscaling Webhook and Workers#286dmfrahm wants to merge 5 commits into8gears:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughBumps n8n Helm chart to 2.1.0, adds conditional HorizontalPodAutoscaler templates for worker and webhook (with optional CPU/memory metrics), updates autoscaling/resource guidance and formatting in values.yaml, and adds a "Commit Messages" section to CONTRIBUTING.md. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer (Helm)
participant Helm as Helm renderer
participant K8s as Kubernetes API
participant Metrics as Metrics API
participant HPA as K8s HPA Controller
participant Deploy as Deployment
Dev->>Helm: helm install/upgrade (chart v2.1.0)
Helm->>K8s: Apply manifests (Deployment, optional HPA for worker/webhook)
K8s->>HPA: Register HPA resources (if enabled)
HPA->>Metrics: Query CPU/Memory utilization (if configured)
Metrics-->>HPA: Return metrics
HPA->>Deploy: Adjust replicas within min/max based on metrics
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![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
🤖 Fix all issues with AI agents
In @charts/n8n/templates/hpa.webhook.yaml:
- Around line 1-14: The HPA template (hpa.webhook.yaml) can render empty
minReplicas/maxReplicas if .Values.webhook.autoscaling.minReplicas or
.Values.webhook.autoscaling.maxReplicas are undefined; update the template to
use Helm's default filter when rendering those fields (e.g., default to 1 for
minReplicas and 100 for maxReplicas) so that spec.minReplicas and
spec.maxReplicas always have valid numeric values, referencing the existing
variables .Values.webhook.autoscaling.minReplicas and
.Values.webhook.autoscaling.maxReplicas (or alternatively fall back to the main
autoscaling defaults) to avoid Kubernetes validation errors.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
charts/n8n/Chart.yamlcharts/n8n/templates/hpa.webhook.yamlcharts/n8n/templates/hpa.worker.yaml
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: tsch157
Repo: 8gears/n8n-helm-chart PR: 239
File: charts/n8n/Chart.yaml:3-4
Timestamp: 2025-09-02T08:05:39.372Z
Learning: n8n 1.110.0 is a pre-release version and should not be used in production deployments. Always verify if a version is stable before recommending upgrades in the n8n Helm chart.
📚 Learning: 2025-09-02T08:05:39.372Z
Learnt from: tsch157
Repo: 8gears/n8n-helm-chart PR: 239
File: charts/n8n/Chart.yaml:3-4
Timestamp: 2025-09-02T08:05:39.372Z
Learning: n8n 1.110.0 is a pre-release version and should not be used in production deployments. Always verify if a version is stable before recommending upgrades in the n8n Helm chart.
Applied to files:
charts/n8n/Chart.yaml
🪛 YAMLlint (1.37.1)
charts/n8n/templates/hpa.webhook.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/n8n/templates/hpa.worker.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
🔇 Additional comments (4)
charts/n8n/templates/hpa.webhook.yaml (1)
15-33: Metrics configuration looks correct.The conditional logic properly handles optional CPU and memory utilization targets. The
orcheck on line 15 ensures themetricsblock is only rendered when at least one target is specified, avoiding an empty metrics array.charts/n8n/Chart.yaml (2)
3-3: Version bump follows semantic versioning.Minor version increment from 2.0.1 to 2.1.0 is appropriate for adding new features (HPA support) that are backward-compatible.
36-38: Changelog entry is accurate.The
artifacthub.io/changesannotation correctly documents the new HPA feature withkind: added.charts/n8n/templates/hpa.worker.yaml (1)
1-34: Template structure is consistent with webhook HPA.The worker HPA template mirrors the webhook HPA, which is good for consistency. Default values for
minReplicas(1) andmaxReplicas(100) are already defined invalues.yamland correctly referenced by the template.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
charts/n8n/templates/hpa.worker.yaml (1)
1-1: Same nil-pointer concern as webhook template.Apply the same guard against undefined parent keys here.
♻️ Safer conditional pattern
-{{- if .Values.worker.autoscaling.enabled }} +{{- if and .Values.worker .Values.worker.autoscaling .Values.worker.autoscaling.enabled }}
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
charts/n8n/templates/hpa.webhook.yamlcharts/n8n/templates/hpa.worker.yaml
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: tsch157
Repo: 8gears/n8n-helm-chart PR: 239
File: charts/n8n/Chart.yaml:3-4
Timestamp: 2025-09-02T08:05:39.372Z
Learning: n8n 1.110.0 is a pre-release version and should not be used in production deployments. Always verify if a version is stable before recommending upgrades in the n8n Helm chart.
🪛 YAMLlint (1.37.1)
charts/n8n/templates/hpa.webhook.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/n8n/templates/hpa.worker.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
🔇 Additional comments (4)
charts/n8n/templates/hpa.webhook.yaml (2)
2-34: LGTM! HPA template structure is correct.The template correctly uses the
autoscaling/v2API and properly conditionalizes the metrics section. The structure follows Kubernetes HPA best practices with optional CPU and memory utilization targets.Note: The YAMLlint error in static analysis is a false positive—
{{-is valid Helm template syntax for whitespace control.
1-1: No action needed. Thewebhook.autoscalingstructure is already defined in values.yaml with sensible defaults (enabled: false,minReplicas: 1,maxReplicas: 100, etc.). Helm Go templates safely handle undefined nested keys by evaluating them to false rather than throwing errors, so the template pattern used here is safe and correct. This same pattern is consistently used across all HPA templates in the chart (main, worker, webhook).charts/n8n/templates/hpa.worker.yaml (2)
14-14: Inconsistent default maxReplicas between webhook and worker.Worker defaults to
maxReplicas: 100while webhook defaults tomaxReplicas: 10. If intentional (workers may need more scaling headroom), this is fine. Otherwise, consider aligning the defaults for consistency.Is this difference intentional? Workers processing background jobs may indeed need more replicas than webhooks handling HTTP traffic, but a 10x difference in defaults could surprise users.
2-34: LGTM! Template structure mirrors webhook HPA correctly.The worker HPA template follows the same correct pattern as the webhook template, using
autoscaling/v2with conditional metrics.
|
@Vad1mo Got the commit message updated, should pass that check now |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@charts/n8n/templates/hpa.worker.yaml`:
- Around line 15-33: Add a clear warning in the Helm values docs/NOTES.txt that
when using HPA metrics (controlled by
.Values.worker.autoscaling.targetCPUUtilizationPercentage or
.Values.worker.autoscaling.targetMemoryUtilizationPercentage) the worker pods
must have resource requests set; instruct users to set
worker.resources.requests.cpu and/or worker.resources.requests.memory (or update
worker.resources) because leaving worker.resources: {} prevents HPA from
calculating CPU/memory utilization correctly.
♻️ Duplicate comments (1)
charts/n8n/templates/hpa.webhook.yaml (1)
1-14: Previous nil-safety concern has been addressed.The template now includes proper defaults using
| default 1and| default 100forminReplicasandmaxReplicasrespectively, preventing invalid manifests when these values are not explicitly configured.
Bump n8n version to 2.1.0 and update changelog Add HorizontalPodAutoscaler for webhook service Rename hpa.workers.yaml to hpa.webhook.yaml Add HorizontalPodAutoscaler for worker deployment Update charts/n8n/templates/hpa.webhook.yaml from coderabbit's suggestion Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Add default values for Worker HPA min and max replicas Set default values for minReplicas and maxReplicas in HPA configuration. Increase maxReplicas for webhook deployment to 100 Matches the root values.yaml Signed-off-by: dmfrahm <martik87@gmail.com>
Added guidelines for commit messages following conventional commits style. Signed-off-by: dmfrahm <martik87@gmail.com>
Signed-off-by: dmfrahm <martik87@gmail.com>
Signed-off-by: dmfrahm <martik87@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@charts/n8n/values.yaml`:
- Around line 453-456: Remove the duplicated word "target" from the autoscaling
note comments so they read "...If you use targetCPUUtilizationPercentage or
targetMemoryUtilizationPercentage, you **MUST** set the requests..." (update
both occurrences that mention targetCPUUtilizationPercentage and
targetMemoryUtilizationPercentage and the example referencing
resources.requests.cpu).
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@charts/n8n/values.yaml`:
- Around line 40-44: The YAML for ingress.hosts is misindented: the list item
(the mapping containing host/paths/pathType) must be indented beneath the hosts
key; update the block under ingress.hosts so the "- host: workflow.example.com"
mapping is nested under hosts (and its child keys path and pathType indented
accordingly) to produce a valid YAML structure for the ingress.hosts list.
- Around line 79-80: The values file currently leaves main.extraEnv empty which
renders as null; change the default for main.extraEnv to an empty map so
templates expecting a map get {} instead of null. Update the values.yaml entry
for extraEnv (the main.extraEnv key) to explicitly be an empty map ({}),
ensuring it matches the worker/webhook defaults and prevents template rendering
issues where a map is expected.
|
@Vad1mo @mbaitelman @alexandre-k Please review and let me know if any further updates are needed |
|
@Vad1mo @mbaitelman @alexandre-k I've successfully run the linting actions in my own PR, still waiting on someone to approve their run here and get this approved/merged in! |
|
What steps need to happen for this to be reviewed? It's been over a month with zero feedback, and that is incredibly frustrating. |
…s.yaml Signed-off-by: dmfrahm <martik87@gmail.com>
1 participant
What this PR does / why we need it
This PR introduces support for Horizontal Pod Autoscalers (HPA) for both the
webhookandworkercomponents in the Helm chart.Fixes issue #179
Special notes for your reviewer
Based on PR #186 by @simonefrancia, just implements the fixes requested in there, just trying to get this over the finish line.
Per that PR (text copied from @simonefrancia's PR):
🆕 Changes Introduced
Added a new template:
charts/n8n/templates/hpa.webhook.yamlDefines an HPA resource for the webhook deployment, conditionally created based on .Values.webhook.autoscaling.enabled.
Added a new template:
charts/n8n/templates/hpa.worker.yamlDefines an HPA resource for the worker deployment, conditionally created based on .Values.worker.autoscaling.enabled.
⚙️ Configuration
Each HPA supports:
These values are configurable via:
🧪 Notes
Checklist
Please place an 'x' in all applicable fields and remove unrelated items.
Version and Documentation
Chart.yamlfollowing semantic versioningartifacthub.io/changessection updated inChart.yaml(see ArtifactHub annotation reference)Summary by CodeRabbit
New Features
Chores
Documentation