Fix cluster-autoscaler #561
Conversation
dosubot
bot
added
the
size:S
WalkthroughThe pull request introduces updates to the Kubernetes cluster autoscaler, involving changes to the image tag, Dockerfile, and deployment configuration. The modifications include upgrading the Go programming language version, updating the cluster autoscaler version, and adding a patch to modify scaling behavior. The deployment manifest receives new command-line arguments to fine-tune the autoscaler's node management and resource utilization strategies. Changes
Sequence DiagramsequenceDiagram
participant CA as Cluster Autoscaler
participant Nodes as Kubernetes Nodes
participant Pods as Kubernetes Pods
CA->>Nodes: Scan cluster
CA->>Pods: Analyze resource utilization
CA->>Nodes: Apply scaling decisions
Note over CA: Enforce min/max node group sizes
Note over CA: Ignore DaemonSets and Mirror Pods
CA->>Nodes: Delete unregistered nodes if needed
Poem
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 using PR comments)
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: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/apps/kubernetes/images/cluster-autoscaler.tag(1 hunks)packages/apps/kubernetes/images/cluster-autoscaler/Dockerfile(1 hunks)packages/apps/kubernetes/images/cluster-autoscaler/fix-downscale.diff(1 hunks)packages/apps/kubernetes/templates/cluster-autoscaler/deployment.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/apps/kubernetes/images/cluster-autoscaler.tag
🔇 Additional comments (1)
packages/apps/kubernetes/images/cluster-autoscaler/Dockerfile (1)
7-7: Verify cluster-autoscaler versionLet's verify if this is a valid release tag.
✅ Verification successful
Version tag verification successful
The tag
cluster-autoscaler-1.32.0exists in the upstream kubernetes/autoscaler repository.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the specified tag exists in the upstream repository git ls-remote --tags https://github.com/kubernetes/autoscaler.git cluster-autoscaler-1.32.0Length of output: 170
| func (r unstructuredScalableResource) SetSize(nreplicas int) error { | ||
| switch { | ||
| - case nreplicas > r.maxSize: | ||
| - return fmt.Errorf("size increase too large - desired:%d max:%d", nreplicas, r.maxSize) | ||
| case nreplicas < r.minSize: | ||
| return fmt.Errorf("size decrease too large - desired:%d min:%d", nreplicas, r.minSize) | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Custom patch removes critical scaling safeguard present in upstream
The upstream cluster-autoscaler implementation enforces both minimum and maximum size limits. Removing the maximum size check is a custom modification that deviates from upstream behavior and could lead to uncontrolled scaling.
- Found in upstream
cluster-autoscaler/cloudprovider/clusterapi/clusterapi_unstructured.go:case nreplicas > r.maxSize: return fmt.Errorf("size increase too large - desired:%d max:%d", nreplicas, r.maxSize)
Strong justification is needed for removing this critical safeguard. Consider keeping the upstream behavior unless there's a compelling reason otherwise.
🔗 Analysis chain
Critical change in scaling behavior - needs verification
This patch removes the maximum size limit check while keeping the minimum size limit. This is a significant change that could lead to uncontrolled scaling beyond the configured maximum size.
Questions:
- What's the rationale behind removing the maximum size check?
- Is this a custom patch or backported from upstream?
Let's verify if this is a custom patch or exists in upstream:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if this change exists in upstream cluster-autoscaler
git clone https://github.com/kubernetes/autoscaler.git /tmp/autoscaler
cd /tmp/autoscaler
git log -p --all -- cluster-autoscaler/cloudprovider/clusterapi/clusterapi_unstructured.go | grep -B 5 "case nreplicas > r.maxSize"
Length of output: 2683
Script:
#!/bin/bash
# Search for SetSize method implementations and scaling limit checks
cd /tmp/autoscaler
rg -A 5 "func.*SetSize.*nreplicas.*int.*error" cluster-autoscaler/
rg "case nreplicas [<>] .*Size:" cluster-autoscaler/
Length of output: 1085
| @@ -1,12 +1,14 @@ | |||
| # Source: https://raw.githubusercontent.com/kubernetes/autoscaler/refs/heads/master/cluster-autoscaler/Dockerfile.amd64 | |||
| ARG builder_image=docker.io/library/golang:1.22.5 | |||
| ARG builder_image=docker.io/library/golang:1.23.4 | |||
There was a problem hiding this comment.
Invalid Go version specified
The Go version 1.23.4 is incorrect as Go 1.22 is currently the latest major version.
Please update to a valid Go version:
-ARG builder_image=docker.io/library/golang:1.23.4
+ARG builder_image=docker.io/library/golang:1.22.0📝 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.
| ARG builder_image=docker.io/library/golang:1.23.4 | |
| ARG builder_image=docker.io/library/golang:1.22.0 |
| - --enforce-node-group-min-size=true | ||
| - --ignore-daemonsets-utilization=true | ||
| - --ignore-mirror-pods-utilization=true | ||
| - --scale-down-unneeded-time=30s | ||
| - --scan-interval=25s | ||
| - --force-delete-unregistered-nodes=true |
There was a problem hiding this comment.
💡 Codebase verification
The current configuration uses significantly more aggressive settings than recommended defaults:
scale-down-unneeded-time=30s(default: 10m)scan-interval=25s(default: 10s)
These aggressive timings combined with force-delete-unregistered-nodes=true (undocumented feature) could lead to:
- Frequent scaling operations causing thrashing
- Premature node deletion
- Data loss if nodes are forcefully deleted during temporary network issues
Recommendations:
- --scale-down-unneeded-time=10m # Use default
- --scan-interval=10s # Use default🔗 Analysis chain
Review aggressive autoscaler settings
- The timing settings appear very aggressive:
scale-down-unneeded-time=30s(default is 10 minutes)scan-interval=25s(default is 10 seconds)
force-delete-unregistered-nodes=truecould be dangerous in case of network issues
Let's verify these settings against the official documentation:
Recommendations:
- Consider using less aggressive timing settings to prevent thrashing:
- - --scale-down-unneeded-time=30s
- - --scan-interval=25s
+ - --scale-down-unneeded-time=10m
+ - --scan-interval=10s- Document the rationale for using
force-delete-unregistered-nodes=trueas it could lead to data loss in case of temporary network issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Clone autoscaler repo and check documentation for these parameters
git clone https://github.com/kubernetes/autoscaler.git /tmp/autoscaler
cd /tmp/autoscaler
git checkout cluster-autoscaler-1.32.0
# Search for parameter documentation
for param in "scale-down-unneeded-time" "scan-interval" "force-delete-unregistered-nodes"; do
echo "=== $param ==="
rg -A 2 "$param" cluster-autoscaler/FAQ.md cluster-autoscaler/README.md
done
Length of output: 5534
Script:
#!/bin/bash
# Search for force-delete-unregistered-nodes in source code and docs
rg -A 3 "force.*delete.*unregistered.*nodes" .
# Also search for any related documentation about unregistered nodes
rg -A 3 "unregistered.*nodes?" .
Length of output: 1221
1 participant
Summary by CodeRabbit
New Features
Improvements
Technical Updates