Skip to content

Follow seiload's log so a rotation can't hide its summary - #616

Merged
masih merged 1 commit into
mainfrom
masih/1792349001-seiload-summary-stream
Oct 18, 2026
Merged

masih merged 1 commit into
mainfrom
masih/1792349001-seiload-summary-stream

Conversation

@masih

@masih masih commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

The 10-18 nightly failed TestNightlyBenchmark/saturation with "seiload summary has no revert count" even though seiload printed Of the 243822 the chain executed, 0 reverted and exited cleanly. assertSeiloadRun reads the summary with a 200-line TailLines read 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 and Shutdown complete), and the run cannot be verified.

runSeiload now follows the seiload pod's log with Follow: true from the time the Job is created until it finishes. It keeps the lines matching statusReadLine and revertedLine, and when kubelet recreates the log file on rotation the followed stream reopens it. assertSeiloadRun parses 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. runJob is split into createJob and waitJob so the follower can start between them, and the release suites keep calling runJob unchanged.

@devin-ai-integration

Copy link
Copy Markdown
Contributor

I'll fix CI failures and address comments from users with write access that start with 'Devin'.

  • Disable automatic comment, CI, and merge conflict monitoring

@masih
masih marked this pull request as ready for review October 18, 2026 19:01
@cursor

cursor Bot commented Oct 18, 2026 •

Copy link
Copy Markdown

PR Summary

Low Risk
Integration-test harness only; no production or runtime behavior changes.

Overview
Fixes false "seiload summary has no revert count" failures when kubelet log rotation moves the end-of-run summary out of the post-job 200-line tail read.

runSeiload now starts a Follow: true log stream right after job creation (via split createJob / waitJob), captures lines matching the existing summary regexes, and passes them into assertSeiloadRun, which merges followed lines with the tail so verification stays fail-closed. followPodLog retries until the pod exists; summaryStreamDrain bounds shutdown of the follower goroutine. Release suites still call runJob unchanged.

Reviewed by Cursor Bugbot for commit b2eb362. Bugbot is set up for automated code reviews on this repo. Configure here.

@seidroid seidroid Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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())

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@masih
masih merged commit 0750582 into main Oct 18, 2026
18 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant