fix(k3s): preserve automatic crash restart - #2076
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. |
📝 WalkthroughWalkthroughThe k3s systemd unit no longer invokes Changesk3s service behavior
Estimated code review effort: 1 (Trivial) | ~5 minutes 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 |
There was a problem hiding this comment.
Code Review
This pull request removes the destructive k3s-killall.sh script from the ExecStopPost directive in the k3s.service configuration and adds corresponding tests to verify this change. The feedback suggests a more robust assertion in the test suite by checking for an exit status of 1 instead of any failure status, which prevents false positives if the service file is missing or unreadable.
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.
|
|
||
| It 'does not run the destructive killall helper after service exit' | ||
| When run grep 'ExecStopPost=.*k3s-killall' "$SERVICE" | ||
| The status should be failure |
There was a problem hiding this comment.
Asserting The status should be failure can lead to false positives. If the service file $SERVICE is missing, renamed, or unreadable, grep will fail with exit status 2 (or another non-zero error status), which ShellSpec treats as a failure, causing the test to pass.
By asserting The status should equal 1, you ensure that grep successfully scanned the file (proving the file exists and is readable) but did not find the pattern.
| The status should be failure | |
| The status should equal 1 |
| @@ -19,7 +19,6 @@ Restart=always | |||
| RestartSec=5s | |||
| ExecStartPre=-@coreutils@/bin/rm -f /run/k3s/containerd/containerd.sock | |||
| ExecStart=@k3s@/bin/k3s server | |||
There was a problem hiding this comment.
Operational behavior change on manual stop: Removing ExecStopPost=k3s-killall.sh is the right call for the auto-restart path (this is exactly what upstream k3s ships, and it lets KillMode=process + ExecStartPre=rm ... containerd.sock do their job so k3s can reattach to still-running containerd after a crash). One side effect worth mentioning in the PR description: a manual systemctl stop k3s will now leave containerd, pods, and CNI netns/mounts in place until an operator invokes k3s-killall.sh explicitly. Not a bug (matches upstream), just a UX change for anyone who was relying on systemctl stop k3s as a full teardown.
a24e7cf to
434981f
Compare
Summary
k3s-killall.shpost-stop hook from the Kyber k3s unitWhy
k3s-killall.shstops k3s services and resets container/runtime networking state. Running it automatically after every service exit defeatsRestart=always, which left Kyber k3s failed after the scheduler safety exit.After this change, a normal
systemctl stop k3smatches upstream behavior and does not perform a full container/network teardown. Operators can still runk3s-killall.shexplicitly when that full reset is intended.Verification
shellspec spec/k3s_service_activate_spec.shshellcheck spec/k3s_service_activate_spec.shmake nix-format-checkgit diff --check