Repository navigation
Follow seiload's log so a rotation can't hide its summary - #616
Conversation
|
I'll fix CI failures and address comments from users with write access that start with 'Devin'.
|
PR SummaryLow Risk Overview
Reviewed by Cursor Bugbot for commit b2eb362. Bugbot is set up for automated code reviews on this repo. Configure here. |
There was a problem hiding this comment.
The change follows the seiload pod's log from the moment the Job is created and keeps the summary lines. A kubelet log rotation can then no longer hide the summary from the 200-line tail read, and the fail-closed check stays in place because the stream's lines are merged with the existing tail. Nothing blocks: the two suggestions below cover a 30-second worst-case delay before the pod is inspected and a single-shot stream that does not reconnect. Both fall back to today's behaviour rather than passing a run unverified. Reviewed against merge commit 92fb0e5. No Go toolchain was available, so this is unbuilt. Kept codex's finding as the first suggestion, narrowed to when the delay actually happens. Every scout finding was checked against the code.
Non-blocking
2 findings on the changed lines, as inline comments.
seidroid review · decision approve · session b7d44acd9d6c4759b98fbfd85aa73e26 · turn resp_claude_64e5265628aa338d2bc46f9b19162d2b · item 8bfb7cb746355b8b8aaddba2f68d5480
Findings: 0 blocking | 2 non-blocking | 2 posted inline
| // Read seiload's pod before the follower catch-up wait: Karpenter can | ||
| // reclaim the finished pod's node, taking its log with it. | ||
| assertSeiloadRun(ctx, t, cs, job, s) | ||
| assertSeiloadRun(ctx, t, cs, job, s, summary.lines()) |
There was a problem hiding this comment.
suggestion — (Raised by codex, confirmed.) summary.lines() is evaluated before assertSeiloadRun reads the pod. When the stream has not ended, it blocks for up to summaryStreamDrain (30s) first. That happens when the stream hangs, or when followPodLog never connected and is still retrying until the context is cancelled. Karpenter can reclaim the finished pod during that wait, which is the risk the comment above this call guards against. When kubelet ends the stream shortly after the container exits, there is no delay, so the problem is limited to those edge cases. The fix is to look up the pod and read the tail before draining the follower, for example by passing the follower into assertSeiloadRun and calling lines() after jobPod/podLog.
| if statusReadLine.MatchString(line) || revertedLine.MatchString(line) { | ||
| f.matched = append(f.matched, strings.TrimRight(line, "\n")) | ||
| } | ||
| if err != nil { |
There was a problem hiding this comment.
suggestion — Any read error ends the follower for good. If the followed connection drops mid-run, for example through an apiserver restart or a connection reset during a 30-minute saturation run, the summary is never captured and the check falls back to the tail read, which brings back the original flake. The check still fails closed. Reopening the stream after an error that happens while ctx is still live would close this gap. Re-matched lines are harmless because summaryCounts takes the first match.
The 10-18 nightly failed
TestNightlyBenchmark/saturationwith "seiload summary has no revert count" even though seiload printedOf the 243822 the chain executed, 0 revertedand exited cleanly.assertSeiloadRunreads the summary with a 200-lineTailLinesread after the Job completes, and kubelet serves that read only from the container's current log file. A saturation pod writes tens of MB of log, so when kubelet rotates just after the summary, the tail returns only the few lines logged after it (here about 14 exemplar-label warnings andShutdown complete), and the run cannot be verified.runSeiloadnow follows the seiload pod's log withFollow: truefrom the time the Job is created until it finishes. It keeps the lines matchingstatusReadLineandrevertedLine, and when kubelet recreates the log file on rotation the followed stream reopens it.assertSeiloadRunparses those lines together with the existing tail, so the check stays fail-closed: if neither source contains a summary, it fails the same way it does today.runJobis split intocreateJobandwaitJobso the follower can start between them, and the release suites keep callingrunJobunchanged.