Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

✨ feat: worker and webhook autosacling #112

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

✨ feat: worker and webhook autosacling #112

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a5ca708
✨ feat: worker and webhook autosacling
mhkarimi1383 Aug 5, 2024
101e568
✏️ typo: also check if scaling is enabled in worker and webhook hpa
mhkarimi1383 Aug 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion charts/n8n/templates/deployment.webhooks.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ metadata:
labels:
{{- include "n8n.labels" . | nindent 4 }}
spec:
{{- if not .Values.autoscaling.enabled }}
{{- if not .Values.webhookAutoscaling.enabled }}
replicas: {{ .Values.scaling.webhook.count }}
{{- end }}
strategy:
Expand Down
2 changes: 1 addition & 1 deletion charts/n8n/templates/deployment.worker.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ metadata:
labels:
{{- include "n8n.labels" . | nindent 4 }}
spec:
{{- if not .Values.autoscaling.enabled }}
{{- if not .Values.workerAutoscaling.enabled }}
replicas: {{ .Values.scaling.worker.count }}
{{- end }}
strategy:
Expand Down
51 changes: 51 additions & 0 deletions charts/n8n/templates/hpa.webhook.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
{{- if .Values.autoscaling.enabled }}
Copy link

@coderabbitai coderabbitai bot Aug 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Fix the syntax error in the conditional block.

The static analysis tool flagged a syntax error on line 1. The correct syntax should be:

- {{- if .Values.autoscaling.enabled }}
+ {{- if .Values.autoscaling.enabled -}}
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.

Suggested change
{{- if .Values.autoscaling.enabled }}
{{- if .Values.autoscaling.enabled -}}
Tools
yamllint

[error] 1-1: syntax error: expected the node content, but found '-'

(syntax)

Copy link
Author

Choose a reason for hiding this comment

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

Original HPA made by the same syntax

Copy link

Choose a reason for hiding this comment

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

@mhkarimi1383, I checked the original HPA file (charts/n8n/templates/hpa.yaml), and it indeed uses the same syntax ({{- if .Values.autoscaling.enabled }}). It seems that the static analysis tool might have flagged a false positive in this context. Based on this, the existing syntax should be acceptable.

Analysis chain
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for original HPA files in the repository to verify their syntax.
fd -e yaml -e yml -x grep -Hn '{{- if .Values.autoscaling.enabled }}' {}

Length of output: 148

{{- if semverCompare ">=1.25-0" .Capabilities.KubeVersion.GitVersion -}}
apiVersion: autoscaling/v2
{{- else -}}
apiVersion: autoscaling/v2beta1
{{- end }}
kind: HorizontalPodAutoscaler
metadata:
name: {{ include "n8n.fullname" . }}-webhook
labels:
{{- include "n8n.labels" . | nindent 4 }}
spec:
scaleTargetRef:
apiVersion: apps/v1
kind: Deployment
name: {{ include "n8n.fullname" . }}-webhook
minReplicas: {{ .Values.webhookAutoscaling.minReplicas }}
maxReplicas: {{ .Values.webhookAutoscaling.maxReplicas }}
metrics:
{{- if semverCompare ">=1.25-0" .Capabilities.KubeVersion.GitVersion -}}
{{- if .Values.webhookAutoscaling.targetCPUUtilizationPercentage }}
- type: Resource
resource:
name: cpu
target:
type: Utilization
averageUtilization: {{ .Values.webhookAutoscaling.targetCPUUtilizationPercentage }}
{{- end }}
{{- if .Values.webhookAutoscaling.targetMemoryUtilizationPercentage }}
- type: Resource
resource:
name: memory
target:
type: Utilization
averageUtilization: {{ .Values.webhookAutoscaling.targetMemoryUtilizationPercentage }}
{{- end }}
{{- else -}}
{{- if .Values.webhookAutoscaling.targetCPUUtilizationPercentage }}
- type: Resource
resource:
name: cpu
targetAverageUtilization: {{ .Values.webhookAutoscaling.targetCPUUtilizationPercentage }}
{{- end }}
{{- if .Values.webhookAutoscaling.targetMemoryUtilizationPercentage }}
- type: Resource
resource:
name: memory
targetAverageUtilization: {{ .Values.webhookAutoscaling.targetMemoryUtilizationPercentage }}
{{- end }}
{{- end }}
Comment on lines +13 to +51
Copy link

@coderabbitai coderabbitai bot Aug 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Improve readability of the metrics section.

The metrics section uses nested conditional blocks, which can be simplified for better readability. Consider refactoring to reduce nesting.

- {{- if semverCompare ">=1.25-0" .Capabilities.KubeVersion.GitVersion -}}
- {{- if .Values.webhookAutoscaling.targetCPUUtilizationPercentage }}
- - type: Resource
-   resource:
-     name: cpu
-     target:
-       type: Utilization
-       averageUtilization: {{ .Values.webhookAutoscaling.targetCPUUtilizationPercentage }}
- {{- end }}
- {{- if .Values.webhookAutoscaling.targetMemoryUtilizationPercentage }}
- - type: Resource
-   resource:
-     name: memory
-     target:
-       type: Utilization
-       averageUtilization: {{ .Values.webhookAutoscaling.targetMemoryUtilizationPercentage }}
- {{- end }}
- {{- else -}}
- {{- if .Values.webhookAutoscaling.targetCPUUtilizationPercentage }}
- - type: Resource
-   resource:
-     name: cpu
-     targetAverageUtilization: {{ .Values.webhookAutoscaling.targetCPUUtilizationPercentage }}
- {{- end }}
- {{- if .Values.webhookAutoscaling.targetMemoryUtilizationPercentage }}
- - type: Resource
-   resource:
-     name: memory
-     targetAverageUtilization: {{ .Values.webhookAutoscaling.targetMemoryUtilizationPercentage }}
- {{- end }}
- {{- end }}
+ {{- if semverCompare ">=1.25-0" .Capabilities.KubeVersion.GitVersion -}}
+ {{- $cpuTarget := "Utilization" }}
+ {{- $memoryTarget := "Utilization" }}
+ {{- else -}}
+ {{- $cpuTarget := "targetAverageUtilization" }}
+ {{- $memoryTarget := "targetAverageUtilization" }}
+ {{- end }}
+ {{- if .Values.webhookAutoscaling.targetCPUUtilizationPercentage }}
+ - type: Resource
+   resource:
+     name: cpu
+     target:
+       type: {{ $cpuTarget }}
+       averageUtilization: {{ .Values.webhookAutoscaling.targetCPUUtilizationPercentage }}
+ {{- end }}
+ {{- if .Values.webhookAutoscaling.targetMemoryUtilizationPercentage }}
+ - type: Resource
+   resource:
+     name: memory
+     target:
+       type: {{ $memoryTarget }}
+       averageUtilization: {{ .Values.webhookAutoscaling.targetMemoryUtilizationPercentage }}
+ {{- end }}
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.

Suggested change
spec:
scaleTargetRef:
apiVersion: apps/v1
kind: Deployment
name: {{ include "n8n.fullname" . }}-webhook
minReplicas: {{ .Values.webhookAutoscaling.minReplicas }}
maxReplicas: {{ .Values.webhookAutoscaling.maxReplicas }}
metrics:
{{- if semverCompare ">=1.25-0" .Capabilities.KubeVersion.GitVersion -}}
{{- if .Values.webhookAutoscaling.targetCPUUtilizationPercentage }}
- type: Resource
resource:
name: cpu
target:
type: Utilization
averageUtilization: {{ .Values.webhookAutoscaling.targetCPUUtilizationPercentage }}
{{- end }}
{{- if .Values.webhookAutoscaling.targetMemoryUtilizationPercentage }}
- type: Resource
resource:
name: memory
target:
type: Utilization
averageUtilization: {{ .Values.webhookAutoscaling.targetMemoryUtilizationPercentage }}
{{- end }}
{{- else -}}
{{- if .Values.webhookAutoscaling.targetCPUUtilizationPercentage }}
- type: Resource
resource:
name: cpu
targetAverageUtilization: {{ .Values.webhookAutoscaling.targetCPUUtilizationPercentage }}
{{- end }}
{{- if .Values.webhookAutoscaling.targetMemoryUtilizationPercentage }}
- type: Resource
resource:
name: memory
targetAverageUtilization: {{ .Values.webhookAutoscaling.targetMemoryUtilizationPercentage }}
{{- end }}
{{- end }}
spec:
scaleTargetRef:
apiVersion: apps/v1
kind: Deployment
name: {{ include "n8n.fullname" . }}-webhook
minReplicas: {{ .Values.webhookAutoscaling.minReplicas }}
maxReplicas: {{ .Values.webhookAutoscaling.maxReplicas }}
metrics:
{{- if semverCompare ">=1.25-0" .Capabilities.KubeVersion.GitVersion -}}
{{- $cpuTarget := "Utilization" }}
{{- $memoryTarget := "Utilization" }}
{{- else -}}
{{- $cpuTarget := "targetAverageUtilization" }}
{{- $memoryTarget := "targetAverageUtilization" }}
{{- end }}
{{- if .Values.webhookAutoscaling.targetCPUUtilizationPercentage }}
- type: Resource
resource:
name: cpu
target:
type: {{ $cpuTarget }}
averageUtilization: {{ .Values.webhookAutoscaling.targetCPUUtilizationPercentage }}
{{- end }}
{{- if .Values.webhookAutoscaling.targetMemoryUtilizationPercentage }}
- type: Resource
resource:
name: memory
target:
type: {{ $memoryTarget }}
averageUtilization: {{ .Values.webhookAutoscaling.targetMemoryUtilizationPercentage }}
{{- end }}

Copy link
Author

Choose a reason for hiding this comment

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

Original HPA made by the same syntax

Copy link

Choose a reason for hiding this comment

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

@mhkarimi1383, understood. Maintaining consistency with the existing HPA syntax is important. We'll keep the current structure as it aligns with the rest of the codebase.

{{- end }}
51 changes: 51 additions & 0 deletions charts/n8n/templates/hpa.worker.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
{{- if .Values.autoscaling.enabled }}
Copy link

@coderabbitai coderabbitai bot Aug 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Fix the syntax error in the conditional block.

The static analysis tool flagged a syntax error on line 1. The correct syntax should be:

- {{- if .Values.autoscaling.enabled }}
+ {{- if .Values.autoscaling.enabled -}}
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.

Suggested change
{{- if .Values.autoscaling.enabled }}
{{- if .Values.autoscaling.enabled -}}
Tools
yamllint

[error] 1-1: syntax error: expected the node content, but found '-'

(syntax)

Copy link
Author

Choose a reason for hiding this comment

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

Original HPA made by the same syntax

Copy link

Choose a reason for hiding this comment

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

@mhkarimi1383 The search results confirm that the original HPA file uses the same syntax {{- if .Values.autoscaling.enabled }}, so the current syntax is consistent with the existing codebase and does not need to be changed. Thank you for pointing that out!

Analysis chain
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for the original HPA files and inspect their syntax
rg '{{- if .Values.autoscaling.enabled }}' -A 5

Length of output: 162

Script:

#!/bin/bash
# Search for the original HPA files and inspect their syntax with escaped curly braces
rg '\{\{- if \.Values\.autoscaling\.enabled \}\}' -A 5

Length of output: 430

{{- if semverCompare ">=1.25-0" .Capabilities.KubeVersion.GitVersion -}}
apiVersion: autoscaling/v2
{{- else -}}
apiVersion: autoscaling/v2beta1
{{- end }}
kind: HorizontalPodAutoscaler
metadata:
name: {{ include "n8n.fullname" . }}-worker
labels:
{{- include "n8n.labels" . | nindent 4 }}
spec:
scaleTargetRef:
apiVersion: apps/v1
kind: Deployment
name: {{ include "n8n.fullname" . }}-worker
minReplicas: {{ .Values.workerAutoscaling.minReplicas }}
maxReplicas: {{ .Values.workerAutoscaling.maxReplicas }}
metrics:
{{- if semverCompare ">=1.25-0" .Capabilities.KubeVersion.GitVersion -}}
{{- if .Values.workerAutoscaling.targetCPUUtilizationPercentage }}
- type: Resource
resource:
name: cpu
target:
type: Utilization
averageUtilization: {{ .Values.workerAutoscaling.targetCPUUtilizationPercentage }}
{{- end }}
{{- if .Values.workerAutoscaling.targetMemoryUtilizationPercentage }}
- type: Resource
resource:
name: memory
target:
type: Utilization
averageUtilization: {{ .Values.workerAutoscaling.targetMemoryUtilizationPercentage }}
{{- end }}
{{- else -}}
{{- if .Values.workerAutoscaling.targetCPUUtilizationPercentage }}
- type: Resource
resource:
name: cpu
targetAverageUtilization: {{ .Values.workerAutoscaling.targetCPUUtilizationPercentage }}
{{- end }}
{{- if .Values.workerAutoscaling.targetMemoryUtilizationPercentage }}
- type: Resource
resource:
name: memory
targetAverageUtilization: {{ .Values.workerAutoscaling.targetMemoryUtilizationPercentage }}
{{- end }}
{{- end }}
Comment on lines +13 to +51
Copy link

@coderabbitai coderabbitai bot Aug 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Improve readability of the metrics section.

The metrics section uses nested conditional blocks, which can be simplified for better readability. Consider refactoring to reduce nesting.

- {{- if semverCompare ">=1.25-0" .Capabilities.KubeVersion.GitVersion -}}
- {{- if .Values.workerAutoscaling.targetCPUUtilizationPercentage }}
- - type: Resource
-   resource:
-     name: cpu
-     target:
-       type: Utilization
-       averageUtilization: {{ .Values.workerAutoscaling.targetCPUUtilizationPercentage }}
- {{- end }}
- {{- if .Values.workerAutoscaling.targetMemoryUtilizationPercentage }}
- - type: Resource
-   resource:
-     name: memory
-     target:
-       type: Utilization
-       averageUtilization: {{ .Values.workerAutoscaling.targetMemoryUtilizationPercentage }}
- {{- end }}
- {{- else -}}
- {{- if .Values.workerAutoscaling.targetCPUUtilizationPercentage }}
- - type: Resource
-   resource:
-     name: cpu
-     targetAverageUtilization: {{ .Values.workerAutoscaling.targetCPUUtilizationPercentage }}
- {{- end }}
- {{- if .Values.workerAutoscaling.targetMemoryUtilizationPercentage }}
- - type: Resource
-   resource:
-     name: memory
-     targetAverageUtilization: {{ .Values.workerAutoscaling.targetMemoryUtilizationPercentage }}
- {{- end }}
- {{- end }}
+ {{- if semverCompare ">=1.25-0" .Capabilities.KubeVersion.GitVersion -}}
+ {{- $cpuTarget := "Utilization" }}
+ {{- $memoryTarget := "Utilization" }}
+ {{- else -}}
+ {{- $cpuTarget := "targetAverageUtilization" }}
+ {{- $memoryTarget := "targetAverageUtilization" }}
+ {{- end }}
+ {{- if .Values.workerAutoscaling.targetCPUUtilizationPercentage }}
+ - type: Resource
+   resource:
+     name: cpu
+     target:
+       type: {{ $cpuTarget }}
+       averageUtilization: {{ .Values.workerAutoscaling.targetCPUUtilizationPercentage }}
+ {{- end }}
+ {{- if .Values.workerAutoscaling.targetMemoryUtilizationPercentage }}
+ - type: Resource
+   resource:
+     name: memory
+     target:
+       type: {{ $memoryTarget }}
+       averageUtilization: {{ .Values.workerAutoscaling.targetMemoryUtilizationPercentage }}
+ {{- end }}
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.

Suggested change
spec:
scaleTargetRef:
apiVersion: apps/v1
kind: Deployment
name: {{ include "n8n.fullname" . }}-worker
minReplicas: {{ .Values.workerAutoscaling.minReplicas }}
maxReplicas: {{ .Values.workerAutoscaling.maxReplicas }}
metrics:
{{- if semverCompare ">=1.25-0" .Capabilities.KubeVersion.GitVersion -}}
{{- if .Values.workerAutoscaling.targetCPUUtilizationPercentage }}
- type: Resource
resource:
name: cpu
target:
type: Utilization
averageUtilization: {{ .Values.workerAutoscaling.targetCPUUtilizationPercentage }}
{{- end }}
{{- if .Values.workerAutoscaling.targetMemoryUtilizationPercentage }}
- type: Resource
resource:
name: memory
target:
type: Utilization
averageUtilization: {{ .Values.workerAutoscaling.targetMemoryUtilizationPercentage }}
{{- end }}
{{- else -}}
{{- if .Values.workerAutoscaling.targetCPUUtilizationPercentage }}
- type: Resource
resource:
name: cpu
targetAverageUtilization: {{ .Values.workerAutoscaling.targetCPUUtilizationPercentage }}
{{- end }}
{{- if .Values.workerAutoscaling.targetMemoryUtilizationPercentage }}
- type: Resource
resource:
name: memory
targetAverageUtilization: {{ .Values.workerAutoscaling.targetMemoryUtilizationPercentage }}
{{- end }}
{{- end }}
spec:
scaleTargetRef:
apiVersion: apps/v1
kind: Deployment
name: {{ include "n8n.fullname" . }}-worker
minReplicas: {{ .Values.workerAutoscaling.minReplicas }}
maxReplicas: {{ .Values.workerAutoscaling.maxReplicas }}
metrics:
{{- if semverCompare ">=1.25-0" .Capabilities.KubeVersion.GitVersion -}}
{{- $cpuTarget := "Utilization" }}
{{- $memoryTarget := "Utilization" }}
{{- else -}}
{{- $cpuTarget := "targetAverageUtilization" }}
{{- $memoryTarget := "targetAverageUtilization" }}
{{- end }}
{{- if .Values.workerAutoscaling.targetCPUUtilizationPercentage }}
- type: Resource
resource:
name: cpu
target:
type: {{ $cpuTarget }}
averageUtilization: {{ .Values.workerAutoscaling.targetCPUUtilizationPercentage }}
{{- end }}
{{- if .Values.workerAutoscaling.targetMemoryUtilizationPercentage }}
- type: Resource
resource:
name: memory
target:
type: {{ $memoryTarget }}
averageUtilization: {{ .Values.workerAutoscaling.targetMemoryUtilizationPercentage }}
{{- end }}

Copy link
Author

Choose a reason for hiding this comment

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

Original HPA made by the same syntax

Copy link

Choose a reason for hiding this comment

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

@mhkarimi1383, understood. We'll retain the current syntax to ensure consistency with the original HPA.

{{- end }}
15 changes: 15 additions & 0 deletions charts/n8n/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,9 +277,24 @@ ingress:
workerResources:
{}

workerAutoscaling:
enabled: false
minReplicas: 1
maxReplicas: 100
targetCPUUtilizationPercentage: 80
# targetMemoryUtilizationPercentage: 80

webhookResources:
{}

webhookAutoscaling:
enabled: false
minReplicas: 1
maxReplicas: 100
targetCPUUtilizationPercentage: 80
# targetMemoryUtilizationPercentage: 80


resources:
{}
# We usually recommend not to specify default resources and to leave this as a conscious
Expand Down