fix(k3s): cap parallel image pulls - #2123
Conversation
|
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
You do not have enough credits to review this pull request. Please purchase more credits to continue. |
📝 WalkthroughWalkthroughThe k3s activation script now deploys a KYBER kubelet drop-in, conditionally copies both configuration files, and restarts k3s only when either file changes. Home-manager provides the drop-in, and the activation spec verifies its wiring. ChangesK3s kubelet configuration
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a custom kubelet configuration drop-in (10-kyber.conf) for k3s to optimize cold starts by configuring parallel image pulls. It updates the Nix configuration to deploy this file, adjusts the activate.sh script to copy it to the target directory, and adds a test case to verify the script's behavior. A critical issue was identified in the activation script where running diff without $SUDO_CMD on root-owned files will fail with permission errors, leading to redundant file copies and unnecessary k3s service restarts on every activation. Using $SUDO_CMD diff is recommended to resolve this.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if ! diff -q "$K3S_CONFIG_SOURCE" /etc/rancher/k3s/config.yaml >/dev/null 2>&1; then | ||
| $SUDO_CMD cp "$K3S_CONFIG_SOURCE" /etc/rancher/k3s/config.yaml | ||
| K3S_CONFIG_CHANGED=1 | ||
| fi | ||
|
|
||
| if ! diff -q "$KUBELET_CONFIG_SOURCE" "$KUBELET_CONFIG_TARGET" >/dev/null 2>&1; then | ||
| $SUDO_CMD cp "$KUBELET_CONFIG_SOURCE" "$KUBELET_CONFIG_TARGET" | ||
| K3S_CONFIG_CHANGED=1 | ||
| fi |
There was a problem hiding this comment.
The activation script runs as a non-root user during Home Manager activation. Because the target files and directories (especially under /var/lib/rancher/k3s/) are typically owned by root and restricted (e.g., permissions 700 or 750), running diff without $SUDO_CMD will fail with a 'Permission denied' error (exit status 2).
Since the condition uses ! diff, any non-zero exit status (including permission errors) evaluates to true. This causes the script to copy the files and set K3S_CONFIG_CHANGED=1 on every single activation, leading to unnecessary and disruptive k3s service restarts.
Using $SUDO_CMD diff ensures that diff has the necessary privileges to read the target files and correctly determine if they have changed.
| if ! diff -q "$K3S_CONFIG_SOURCE" /etc/rancher/k3s/config.yaml >/dev/null 2>&1; then | |
| $SUDO_CMD cp "$K3S_CONFIG_SOURCE" /etc/rancher/k3s/config.yaml | |
| K3S_CONFIG_CHANGED=1 | |
| fi | |
| if ! diff -q "$KUBELET_CONFIG_SOURCE" "$KUBELET_CONFIG_TARGET" >/dev/null 2>&1; then | |
| $SUDO_CMD cp "$KUBELET_CONFIG_SOURCE" "$KUBELET_CONFIG_TARGET" | |
| K3S_CONFIG_CHANGED=1 | |
| fi | |
| if ! $SUDO_CMD diff -q "$K3S_CONFIG_SOURCE" /etc/rancher/k3s/config.yaml >/dev/null 2>&1; then | |
| $SUDO_CMD cp "$K3S_CONFIG_SOURCE" /etc/rancher/k3s/config.yaml | |
| K3S_CONFIG_CHANGED=1 | |
| fi | |
| if ! $SUDO_CMD diff -q "$KUBELET_CONFIG_SOURCE" "$KUBELET_CONFIG_TARGET" >/dev/null 2>&1; then | |
| $SUDO_CMD cp "$KUBELET_CONFIG_SOURCE" "$KUBELET_CONFIG_TARGET" | |
| K3S_CONFIG_CHANGED=1 | |
| fi |
There was a problem hiding this comment.
Addressed in 17524cc: both installed-file comparisons now run through the detected sudo command, and ShellCheck plus the focused 16-example ShellSpec suite pass.
![indent-zero[bot]](https://avatars.githubusercontent.com/in/1426746?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@config/k3s/activate.sh`:
- Around line 58-61: Update both file-copy operations in config/k3s/activate.sh:
the copy guarded by the K3S_CONFIG_SOURCE comparison at lines 58-61 and the
KUBELET_CONFIG_SOURCE copy at lines 63-66 must use the force flag with $SUDO_CMD
cp, preserving their existing destinations and change-tracking behavior.
- Line 37: Separate the K3S_CONFIG_SOURCE and KUBELET_CONFIG_SOURCE existence
checks in the activation script so missing Kyber-specific kubelet configuration
does not prevent k3s setup on non-Kyber nodes. Keep k3s validation and
configuration unconditional, and wrap the downstream KUBELET_CONFIG_SOURCE
diff/copy logic in a file-existence conditional.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e599a977-e352-4532-8405-42d09ca6d754
📒 Files selected for processing (4)
config/k3s/activate.shconfig/k3s/default.nixconfig/k3s/kubelet.confspec/activate_k3s_spec.sh
| KUBELET_CONFIG_SOURCE="$HOME/.config/k3s/kubelet.conf.d/10-kyber.conf" | ||
| KUBELET_CONFIG_TARGET="/var/lib/rancher/k3s/agent/etc/kubelet.conf.d/10-kyber.conf" | ||
|
|
||
| if [ ! -f "$K3S_CONFIG_SOURCE" ] || [ ! -f "$KUBELET_CONFIG_SOURCE" ]; then |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Separate the existence checks to prevent breaking non-Kyber nodes.
Because KUBELET_CONFIG_SOURCE is deployed conditionally for Kyber nodes (lib.mkIf isKyber in default.nix), combining its check with K3S_CONFIG_SOURCE means this script could inadvertently skip or abort k3s activation entirely on non-Kyber nodes where the drop-in file is naturally absent.
Please consider separating the validation so the script can still configure k3s even if the Kyber-specific drop-in is not present.
🛠️ Proposed fix
-if [ ! -f "$K3S_CONFIG_SOURCE" ] || [ ! -f "$KUBELET_CONFIG_SOURCE" ]; then
+if [ ! -f "$K3S_CONFIG_SOURCE" ]; then(You will also need to wrap the downstream diff/cp logic for KUBELET_CONFIG_SOURCE in a conditional if [ -f "$KUBELET_CONFIG_SOURCE" ]; then ... fi block.)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@config/k3s/activate.sh` at line 37, Separate the K3S_CONFIG_SOURCE and
KUBELET_CONFIG_SOURCE existence checks in the activation script so missing
Kyber-specific kubelet configuration does not prevent k3s setup on non-Kyber
nodes. Keep k3s validation and configuration unconditional, and wrap the
downstream KUBELET_CONFIG_SOURCE diff/copy logic in a file-existence
conditional.
| if ! diff -q "$K3S_CONFIG_SOURCE" /etc/rancher/k3s/config.yaml >/dev/null 2>&1; then | ||
| $SUDO_CMD cp "$K3S_CONFIG_SOURCE" /etc/rancher/k3s/config.yaml | ||
| K3S_CONFIG_CHANGED=1 | ||
| fi |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Use non-interactive flags for file operations.
As per coding guidelines, always use non-interactive flags with file operations (cp -f) to avoid hanging on confirmation prompts during automated runs.
config/k3s/activate.sh#L58-L61: Update to use$SUDO_CMD cp -f "$K3S_CONFIG_SOURCE" /etc/rancher/k3s/config.yaml.config/k3s/activate.sh#L63-L66: Update to use$SUDO_CMD cp -f "$KUBELET_CONFIG_SOURCE" "$KUBELET_CONFIG_TARGET".
📍 Affects 1 file
config/k3s/activate.sh#L58-L61(this comment)config/k3s/activate.sh#L63-L66
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@config/k3s/activate.sh` around lines 58 - 61, Update both file-copy
operations in config/k3s/activate.sh: the copy guarded by the K3S_CONFIG_SOURCE
comparison at lines 58-61 and the KUBELET_CONFIG_SOURCE copy at lines 63-66 must
use the force flag with $SUDO_CMD cp, preserving their existing destinations and
change-tracking behavior.
Source: Coding guidelines
Mesa DescriptionTL;DRCaps K3s parallel image pulls at four concurrent downloads via a new Kubelet configuration drop-in to prevent node disk saturation and CRI failures during cold starts. What changed?
Root cause addressedK3s 1.35 generated Validation
Description generated by Mesa. Update settings |
1 participant
Summary
Root cause addressed
K3s 1.35 generated serializeImagePulls: false without maxParallelImagePulls. During cold runtime recovery, more than 40 image downloads ran concurrently, saturated the single disk, and caused CRI deadlines, name reservations, and pull-QPS failures. Kubernetes documents maxParallelImagePulls specifically to bound network and disk consumption while retaining parallel pulls.
Validation
Summary by cubic
Caps
k3simage pull concurrency at 4 using a kubelet config drop-in to prevent disk saturation and CRI timeouts during cold recovery. The activation script installs the drop-in and restartsk3sonce when config changes.k3sonce.Written for commit 17524cc. Summary will update on new commits.