fix: Add a missing worker node guard file cleanup - #65
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe PR centralizes the kubelet stop-guard path, reports skipped or failed guard operations, and strengthens stale-guard cleanup in master and worker remediation tests. ChangesKubelet stop-guard handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
|
This PR also raises a warning if the before/after cleanup fails instead of silently ignoring it. |
PR Summary by QodoFix SNR tests by reliably cleaning up kubelet stop guard on workers/masters
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Code Review by Qodo
1.
|
![qodo-2-for-medik8s[bot]](https://avatars.githubusercontent.com/in/4026794?s=60&v=4){kind=small}
| By("Removing any stale kubelet stop guard from a previous run") | ||
|
|
||
| _ = helpers.RemoveKubeletStopGuard(ctx, targetMasterName, snrparams.OcDebugTimeout) | ||
| Expect(helpers.RemoveKubeletStopGuard(ctx, targetMasterName, snrparams.OcDebugTimeout)).To(Succeed(), |
There was a problem hiding this comment.
Not introduced in this PR, but the cleanup improvements here make an existing helper contract more visible. In node_ops.go, the guard path /var/tmp/.medik8s-kubelet-stop-guard is duplicated as a string literal in both StopKubelet (shell one-liner) and RemoveKubeletStopGuard (rm -f argument). Extracting it to a shared const would prevent drift if the path ever changes. StopKubelet's doc comment should also mention the guard-file behavior and the caller's obligation to call RemoveKubeletStopGuard, since MDR tests also call this helper without cleanup.
Suggested follow-up in node_ops.go: add const kubeletStopGuardPath = "/var/tmp/.medik8s-kubelet-stop-guard" and update the StopKubelet doc comment to describe the guard file creation, skip-on-exist behavior, and the cleanup requirement.
|
|
||
| By("Removing kubelet stop guard on worker") | ||
|
|
||
| if guardErr := helpers.RemoveKubeletStopGuard(ctx, targetWorkerName, snrparams.OcDebugTimeout); guardErr != nil { |
There was a problem hiding this comment.
Follow-up for StopKubelet in node_ops.go: when the guard file exists and the stop is skipped, the function returns nil without logging, making the skip invisible in test output (logf is available but unused on this path). Also, if systemctl stop kubelet fails after the guard is created, the guard persists and makes later calls no-ops.
Fix: For observability, echo a sentinel on the guard-skip path ([ -f "$g" ] && echo GUARD_SKIP && exit 0) and log via logf in Go. For the stop-failure path: touch "$g" && systemctl stop kubelet || { rm -f "$g"; exit 1; }.
|
|
||
| By("Removing any stale kubelet stop guard on worker") | ||
|
|
||
| Expect(helpers.RemoveKubeletStopGuard(ctx, targetWorkerName, snrparams.OcDebugTimeout)).To(Succeed(), |
There was a problem hiding this comment.
The worker guard cleanup in the simultaneous test is inline in the test body. If the test aborts early (e.g., assertion failure after stopping kubelet on the worker but before the post-recovery cleanup at line 321 runs), JustAfterEach only cleans the master guard (targetMasterName), not the dynamically scoped worker. The stale worker guard would make the next run's StopKubelet on that worker a silent no-op.
The pre-test cleanup here (line 242) handles cross-run contamination, but intra-run abort leaves a gap. Consider registering a DeferCleanup immediately after selecting targetWorkerName so the worker guard is removed even on early abort.
Fix: After selecting targetWorkerName, add:
DeferCleanup(func() {
if err := helpers.RemoveKubeletStopGuard(ctx, targetWorkerName, snrparams.OcDebugTimeout); err != nil {
GinkgoWriter.Printf("WARNING: deferred worker guard cleanup failed on %s: %v\n", targetWorkerName, err)
}
})
43f26a0 to
2c46bc5
Compare
|
I think I addressed the issues you raised @ugreener. Please let me know if you still have concerns! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/snr-operator/tests/master_remediation.go`:
- Around line 219-226: Inside the DeferCleanup callback registering
RemoveKubeletStopGuard, add a By step with the description "Removing deferred
worker kubelet stop guard" before performing the cleanup, while preserving the
existing error logging behavior.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a99d4e52-4a45-41f6-8ebb-2eec00b662a3
📒 Files selected for processing (3)
tests/internal/helpers/node_ops.gotests/snr-operator/tests/master_remediation.gotests/snr-operator/tests/worker_remediation.go
2c46bc5 to
300dc00
Compare
After a hard reboot, the oc debug pod left behind by StopKubelet gets re-executed by kubelet (CRI-O cleans stale containers on boot, so kubelet sees no record of the pod having run and starts it fresh). This stops kubelet ~4 seconds after it starts, causing test flakes. Write a guard file on the host (/var/tmp/.medik8s-kubelet-stop-guard) before stopping kubelet. On re-execution after reboot, the guard file is found and the stop is skipped. The guard is removed in BeforeEach (handles aborted prior runs) and JustAfterEach (normal cleanup). Additional improvements from review feedback: - Extract guard path to a shared const to prevent drift - Log when the guard skip occurs for observability - Roll back the guard file if systemctl stop kubelet fails - Use DeferCleanup for the worker guard in the simultaneous test so it is removed even on early abort Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
300dc00 to
5f44eff
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jmontleon, ugreener The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/test 4.22-konflux-e2e-snr-aws |
2 participants
After a hard reboot, the oc debug pod left behind by StopKubelet gets re-executed by kubelet (CRI-O cleans stale containers on boot, so kubelet sees no record of the pod having run and starts it fresh). This stops kubelet ~4 seconds after it starts, causing test flakes.
Write a guard file on the host (/var/tmp/.medik8s-kubelet-stop-guard) before stopping kubelet. On re-execution after reboot, the guard file is found and the stop is skipped. The guard is removed in BeforeEach (handles aborted prior runs) and JustAfterEach (normal cleanup).
Why we need this PR
Changes made
Which issue(s) this PR fixes
Test plan
Summary by CodeRabbit