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.

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

✨ feat: worker and webhook autosacling #112

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

Conversation

mhkarimi1383
Copy link

@mhkarimi1383 mhkarimi1383 commented Aug 5, 2024

By adding webhookAutoscaling and workerAutoscaling

HPA will work for webhook and worker also fixes the problem where when autoscaling is enabled replicaCount will not work for webhook and worker

closes #80
closes #87

Summary by CodeRabbit

  • New Features

    • Introduced Horizontal Pod Autoscaler (HPA) configurations for both webhook and worker components, enabling automated scaling based on resource utilization.
    • Added granular autoscaling settings in the configuration file for improved resource management.
  • Bug Fixes

    • Adjusted conditions for autoscaling checks to ensure correct scaling behavior for webhooks and workers.

closes 8gears#80, closes 8gears#87

Signed-off-by: Muhammed Hussein Karimi <[email protected]>
@mhkarimi1383 mhkarimi1383 requested a review from a team as a code owner August 5, 2024 09:38
Copy link

coderabbitai bot commented Aug 5, 2024

Walkthrough

The changes introduce more granular autoscaling configurations for both webhook and worker deployments in the n8n Helm chart. By creating dedicated settings for webhook and worker autoscaling, the modifications allow better management of pod replicas based on specific metrics, enhancing the control over resource allocation and scaling behavior. Additionally, new Horizontal Pod Autoscaler templates improve scalability and responsiveness to resource utilization.

Changes

Files Change Summary
charts/n8n/templates/deployment.webhooks.yaml, charts/n8n/templates/deployment.worker.yaml Updated autoscaling conditions to check specific webhook and worker autoscaling flags instead of a general one.
charts/n8n/templates/hpa.webhook.yaml, charts/n8n/templates/hpa.worker.yaml Introduced new HPA configurations for webhooks and workers, dynamically adapting to Kubernetes versions.
charts/n8n/values.yaml Added new autoscaling settings sections for workers and webhooks, allowing for detailed configuration.

Assessment against linked issues

Objective Addressed Explanation
Autoscaling worker pods (Issue #80)
Setting autoscaling.enabled not affecting worker replicas (Issue #87)

🐰
In the fields where bunnies hop,
Autoscaling changes make us stop!
Webhook and worker, side by side,
With metrics to guide and help them glide.
More control for pods, oh what a delight,
Scaling up and down, all through the night! 🌙✨


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between d10683b and a5ca708.

Files selected for processing (5)
  • charts/n8n/templates/deployment.webhooks.yaml (1 hunks)
  • charts/n8n/templates/deployment.worker.yaml (1 hunks)
  • charts/n8n/templates/hpa.webhook.yaml (1 hunks)
  • charts/n8n/templates/hpa.worker.yaml (1 hunks)
  • charts/n8n/values.yaml (1 hunks)
Additional context used
yamllint
charts/n8n/templates/hpa.worker.yaml

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

(syntax)

charts/n8n/templates/hpa.webhook.yaml

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

(syntax)

Additional comments not posted (10)
charts/n8n/templates/hpa.worker.yaml (3)

8-18: LGTM! Metadata and spec sections are correct.

The metadata and spec sections are well-defined, with appropriate use of labels and replica settings.


20-36: LGTM! Metrics section for Kubernetes version >=1.25 is correct.

The metrics section is well-defined, with appropriate conditional logic for CPU and memory utilization.


37-49: LGTM! Metrics section for Kubernetes version <1.25 is correct.

The metrics section is well-defined, with appropriate conditional logic for CPU and memory utilization.

charts/n8n/templates/hpa.webhook.yaml (3)

8-18: LGTM! Metadata and spec sections are correct.

The metadata and spec sections are well-defined, with appropriate use of labels and replica settings.


20-36: LGTM! Metrics section for Kubernetes version >=1.25 is correct.

The metrics section is well-defined, with appropriate conditional logic for CPU and memory utilization.


37-49: LGTM! Metrics section for Kubernetes version <1.25 is correct.

The metrics section is well-defined, with appropriate conditional logic for CPU and memory utilization.

charts/n8n/templates/deployment.webhooks.yaml (1)

9-9: LGTM! Conditional statement for webhook autoscaling is correct.

The conditional statement is well-defined and ensures that the replicas are set correctly based on the webhook autoscaling setting.

charts/n8n/templates/deployment.worker.yaml (1)

9-11: LGTM! Verify consistency across the codebase.

The change to check workerAutoscaling.enabled instead of autoscaling.enabled aligns with the new autoscaling settings. Ensure that similar changes are consistently applied throughout the codebase.

Verification successful

Verification successful.

The change to check workerAutoscaling.enabled instead of autoscaling.enabled is consistent within the scope of its intended use. No further inconsistencies were found across the codebase.

  • workerAutoscaling settings are properly referenced in values.yaml and hpa.worker.yaml.
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify consistency of `workerAutoscaling.enabled` usage across the codebase.

# Test: Search for the usage of `workerAutoscaling.enabled`. Expect: Consistent usage across the codebase.
rg --type yaml 'workerAutoscaling.enabled'

Length of output: 136


Script:

#!/bin/bash
# Description: Verify broader usage of `workerAutoscaling` across the codebase.

# Test: Search for the usage of `workerAutoscaling`. Expect: Consistent usage across the codebase.
rg --type yaml 'workerAutoscaling'

Length of output: 1334

charts/n8n/values.yaml (2)

280-286: LGTM! The workerAutoscaling section is well-configured.

The default values for the workerAutoscaling parameters are reasonable, with autoscaling disabled by default. This allows users to enable and configure autoscaling as needed.


290-296: LGTM! The webhookAutoscaling section is well-configured.

The default values for the webhookAutoscaling parameters are reasonable, with autoscaling disabled by default. This allows users to enable and configure autoscaling as needed.

@@ -0,0 +1,51 @@
{{- if .Values.autoscaling.enabled }}
Copy link

@coderabbitai coderabbitai bot Aug 5, 2024

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

@@ -0,0 +1,51 @@
{{- if .Values.autoscaling.enabled }}
Copy link

@coderabbitai coderabbitai bot Aug 5, 2024

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

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 4

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between a5ca708 and 101e568.

Files selected for processing (2)
  • charts/n8n/templates/hpa.webhook.yaml (1 hunks)
  • charts/n8n/templates/hpa.worker.yaml (1 hunks)
Additional context used
yamllint
charts/n8n/templates/hpa.worker.yaml

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

(syntax)

charts/n8n/templates/hpa.webhook.yaml

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

(syntax)

Additional comments not posted (2)
charts/n8n/templates/hpa.worker.yaml (1)

52-53: LGTM!

The closing conditional blocks are correct and necessary for the autoscaling settings.

charts/n8n/templates/hpa.webhook.yaml (1)

52-53: LGTM!

The closing conditional blocks are correct and necessary for the autoscaling settings.

@@ -0,0 +1,53 @@
{{- if .Values.scaling.enabled }}
Copy link

@coderabbitai coderabbitai bot Aug 5, 2024

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.scaling.enabled }}
+ {{- if .Values.scaling.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.scaling.enabled }}
{{- if .Values.scaling.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, you are correct. The syntax {{- if .Values.scaling.enabled }} is consistently used across multiple files in the project, including hpa.worker.yaml, hpa.webhook.yaml, and deployment.worker.yaml. It appears that the static analysis tool might have flagged a false positive. Thank you for pointing this out!

Analysis chain
Scripts executed

The following scripts were executed for the analysis:

Script:

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

Length of output: 305

Comment on lines +13 to +51
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 }}
Copy link

@coderabbitai coderabbitai bot Aug 5, 2024

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.

@@ -0,0 +1,53 @@
{{- if .Values.scaling.enabled }}
Copy link

@coderabbitai coderabbitai bot Aug 5, 2024

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.scaling.enabled }}
+ {{- if .Values.scaling.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.scaling.enabled }}
{{- if .Values.scaling.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, you are correct. The original HPA files use the same syntax for the conditional blocks. Maintaining consistency across all HPA files is important. Thank you for pointing that out!

Analysis chain
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Find and display the contents of the original HPA files to check the syntax
fd -e yaml 'hpa.*.yaml' --exec cat {}

Length of output: 5302

Comment on lines +13 to +51
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 }}
Copy link

@coderabbitai coderabbitai bot Aug 5, 2024

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.

@mhkarimi1383
Copy link
Author

@Vad1mo
Hi
Can you review this?

@Vad1mo
Copy link
Member

Vad1mo commented Aug 6, 2024

Thank you for the contribution, I'll test it out next week, when I am back. Maybe some other community members can give this PR a try too.

@mhkarimi1383
Copy link
Author

@Vad1mo
Hi any updates?

@Vad1mo
Copy link
Member

Vad1mo commented Sep 10, 2024

@mhkarimi1383 how does this relate to #110?

@mhkarimi1383
Copy link
Author

@Vad1mo
I have not mentioned #110

@RafaelTCostella
Copy link

Any updates on this @Vad1mo?
I believe that this would solve an issue for me too

@Vad1mo
Copy link
Member

Vad1mo commented Sep 14, 2024

I would need to test it, did you guys use it? How does the values file look like?

@mhkarimi1383
Copy link
Author

I have done helm template command against changes
It was OK

See two parts are added for auto scaling worker and webhook deployments (webhookAutoscaling and workerAutoscaling) config looks like older autoScaling

@mhkarimi1383
Copy link
Author

@Vad1mo

@RoseSecurity
Copy link

Testing this PR in our development environment, and I'll provide feedback after testing

@RoseSecurity
Copy link

RoseSecurity commented Dec 16, 2024

Looks good to me!

❯ helm unittest .

### Chart [ n8n ] .


Charts:      1 passed, 1 total
Test Suites: 0 passed, 0 total
Tests:       0 passed, 0 total
Snapshot:    0 passed, 0 total
Time:        1.944917ms
NAME          REFERENCE                TARGETS          MINPODS   MAXPODS   REPLICAS   AGE
n8n-webhook   Deployment/n8n-webhook   60% / 80%        2         10        2          5m
n8n-worker    Deployment/n8n-worker    75% / 80%        1         10        3          5m

All of the values populate as intended as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

setting autoscaling.enabled to true disables the workers replicas autoscaling worker pods
4 participants