fix(jenkins): bound journal and coredump collection to the current build - #15669
fix(jenkins): bound journal and coredump collection to the current build#15669fruch wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Bounds builder journal and coredump collection by Jenkins build start time for long-lived agents.
Changes:
- Filters journal entries and coredumps by build start time.
- Makes privileged journal collection non-interactive.
- Adds a 24-hour fallback window for standalone coredump collection.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
vars/collectTestCoredumps.groovy |
Passes the build timestamp to the collector. |
vars/collectBuilderLogs.groovy |
Bounds journal collection and handles sudo failure. |
utils/upload_sct_coredump.sh |
Archives only recently created coredumps. |
| if sudo -n journalctl --since "@${sinceEpoch}" --no-tail --no-pager -o short-precise > builder-\$SHORT_SCT_TEST_ID.log ; then | ||
| tar -zcvf builder-\$SHORT_SCT_TEST_ID.log.tar.gz builder-\$SHORT_SCT_TEST_ID.log | ||
| ./docker/env/hydra.sh upload --test-id \$SCT_TEST_ID builder-\$SHORT_SCT_TEST_ID.log.tar.gz | ||
| else | ||
| echo "WARNING: sudo -n journalctl failed on \$(hostname) - skipping builder journal upload" | ||
| fi |
There was a problem hiding this comment.
Fixed - you were right that the code did not do what the PR description promised. It now tries sudo -n journalctl first, falls back to an unprivileged journalctl with the same --since bound, and only skips the upload when both fail; the log line says which source produced the archive.
I kept the --since bound on the fallback rather than switching to --boot: the bound is the point of the change (weeks of a static agent's journal), and on a machine that has been up for a month --boot would bring most of it back. Verified all three paths with stub sudo/journalctl on PATH: sudo works, sudo fails then plain works, both fail -> skip.
📝 WalkthroughWalkthroughCoredump collection in upload_sct_coredump.sh now filters files by SCT_COREDUMPS_SINCE_EPOCH (default: prior 24 hours), archiving only matched files, and skips upload when none exist. collectTestCoredumps.groovy computes the build start epoch and passes it to the script. collectBuilderLogs.groovy restricts journalctl output to entries since build start, uses non-interactive sudo, and warns without failing the stage when journal reads are unreadable. Estimated code review effort: 2 (Simple) | ~12 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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 `@vars/collectBuilderLogs.groovy`:
- Around line 19-24: Update the journal collection fallback in
collectBuilderLogs so that when the sudo journalctl command fails, it retries
journalctl without sudo while restricting results to the current boot, and
continues with the existing archive and upload flow if that succeeds; only emit
the warning and skip upload when the fallback also fails.
In `@vars/collectTestCoredumps.groovy`:
- Line 7: Preserve fractional epoch precision in the since-time calculation used
by collectTestCoredumps.groovy at lines 7-7 and collectBuilderLogs.groovy at
lines 8-8. Update the shared collection-bound logic in each file so
SCT_COREDUMPS_SINCE_EPOCH and journalctl --since receive millisecond-precise
values rather than truncated whole seconds.
🪄 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: b7908ecc-ecc0-40f2-849b-107c21cc31fe
📒 Files selected for processing (3)
utils/upload_sct_coredump.shvars/collectBuilderLogs.groovyvars/collectTestCoredumps.groovy
| if sudo -n journalctl --since "@${sinceEpoch}" --no-tail --no-pager -o short-precise > builder-\$SHORT_SCT_TEST_ID.log ; then | ||
| tar -zcvf builder-\$SHORT_SCT_TEST_ID.log.tar.gz builder-\$SHORT_SCT_TEST_ID.log | ||
| ./docker/env/hydra.sh upload --test-id \$SCT_TEST_ID builder-\$SHORT_SCT_TEST_ID.log.tar.gz | ||
| else | ||
| echo "WARNING: sudo -n journalctl failed on \$(hostname) - skipping builder journal upload" | ||
| fi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Implement the current-boot fallback.
If sudo -n journalctl fails, this branch only skips collection. It does not collect the readable current boot. Run the non-sudo current-boot fallback before skipping the upload.
🤖 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 `@vars/collectBuilderLogs.groovy` around lines 19 - 24, Update the journal
collection fallback in collectBuilderLogs so that when the sudo journalctl
command fails, it retries journalctl without sudo while restricting results to
the current boot, and continues with the existing archive and upload flow if
that succeeds; only emit the warning and skip upload when the fallback also
fails.
There was a problem hiding this comment.
Implemented - see the reply on the sibling thread for detail: sudo -n first, unprivileged journalctl as the fallback, skip only when both fail, and the archive/upload flow is shared by both success paths. One deviation from the suggestion: the fallback keeps --since @<build start> rather than --boot, because bounding the window is the whole point of this change and --boot on a long-lived agent would restore most of what it removes.
| // Bound the coredump sweep to this build: on a long-lived agent the coredump directory holds | ||
| // every dump the host ever produced (other jobs' included), and unbounded the script tars and | ||
| // uploads all of it. Epoch seconds, consumed by upload_sct_coredump.sh via -newermt. | ||
| def sinceEpoch = (long) (currentBuild.startTimeInMillis / 1000) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- vars/collectTestCoredumps.groovy ---'
cat -n vars/collectTestCoredumps.groovy
printf '%s\n' '--- vars/collectBuilderLogs.groovy ---'
cat -n vars/collectBuilderLogs.groovy
printf '%s\n' '--- references ---'
rg -n -F 'SCT_COREDUMPS_SINCE_EPOCH' . || true
rg -n -F 'collectTestCoredumps' . || true
rg -n -F 'collectBuilderLogs' . || trueRepository: scylladb/scylla-cluster-tests
Length of output: 2931
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- utils/upload_sct_coredump.sh ---'
cat -n utils/upload_sct_coredump.sh
printf '%s\n' '--- vars/runCollectLogs.groovy ---'
cat -n vars/runCollectLogs.groovy
printf '%s\n' '--- timestamp consumers ---'
rg -n -C 4 'SINCE_EPOCH|journalctl|newermt|find .*core|coredump' utils varsRepository: scylladb/scylla-cluster-tests
Length of output: 13823
🏁 Script executed:
#!/bin/bash
set -e
python3 - <<'PY'
from decimal import Decimal
start_ms = 1_700_000_000_750
start_s = Decimal(start_ms) / Decimal(1000)
truncated_s = start_ms // 1000
samples = {
"prior_same_second": Decimal("1700000000.500"),
"build_start": start_s,
"after_start": Decimal("1700000000.900"),
}
print(f"build_start={start_s}")
print(f"truncated_lower_bound={truncated_s}")
for name, record_s in samples.items():
print(
f"{name}: record={record_s} "
f"included_with_truncated={record_s > truncated_s} "
f"included_with_fractional={record_s > start_s}"
)
PY
printf '%s\n' '--- GNU find fractional timestamp parsing ---'
find /dev/null -newermt '`@1700000000.750`' -print >/dev/null
echo "find_exit=$?"
printf '%s\n' '--- journalctl fractional timestamp parsing ---'
set +e
journalctl --since '`@1700000000.750`' --until '`@1700000000.751`' --no-pager --no-hostname -o cat >/dev/null 2>&1
status=$?
set -e
echo "journalctl_exit=$status"Repository: scylladb/scylla-cluster-tests
Length of output: 641
🌐 Web query:
journalctl --since @ UNIX timestamp fractional seconds timestamp grammar
💡 Result:
When using the --since or --until options in journalctl, you can specify a UNIX epoch timestamp by prefixing an integer or floating-point number with the "@" character [1]. The systemd time parsing logic, documented in systemd.time(7), supports fractional seconds for UNIX timestamps [1]. This allows you to achieve sub-second precision by including a decimal component after the integer portion of the timestamp (e.g., --since=@1625097600.500) [2][1]. In practical application, if you are generating this timestamp in a script, it is common to use tools like date to produce the required format: journalctl --since=@$(date +%s.%6N) In this example, +%s.%6N produces the UNIX epoch followed by a decimal point and 6 digits of microsecond precision, which journalctl will parse correctly [2].
Citations:
- 1: https://www.mankier.com/7/systemd.time
- 2: journalctl --since cannot parse ISO8601 systemd/systemd#5194
Sensitive Data Exposure (CWE-200): Exposure of Sensitive Information to an Unauthorized Actor
Reachability: External · Exploitability: Moderate
Preserve millisecond precision in both collection bounds.
Use a fractional epoch value for SCT_COREDUMPS_SINCE_EPOCH and journalctl --since; the current truncation includes prior-tenant records created before the build within the same second.
📍 Affects 2 files
vars/collectTestCoredumps.groovy#L7-L7(this comment)vars/collectBuilderLogs.groovy#L8-L8
🤖 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 `@vars/collectTestCoredumps.groovy` at line 7, Preserve fractional epoch
precision in the since-time calculation used by collectTestCoredumps.groovy at
lines 7-7 and collectBuilderLogs.groovy at lines 8-8. Update the shared
collection-bound logic in each file so SCT_COREDUMPS_SINCE_EPOCH and journalctl
--since receive millisecond-precise values rather than truncated whole seconds.
There was a problem hiding this comment.
Not changing this one, deliberately - the truncation is in the safe direction and the sub-second overlap is harmless.
currentBuild.startTimeInMillis / 1000 floors to whole seconds, so the window can start up to 999ms before the build did. That can only ever include slightly more than this build; rounding the other way would risk missing a coredump or journal entry written in the same second the build started, which is the failure that actually costs you a diagnosis. The bug being fixed here is weeks of unrelated history, not a millisecond boundary.
Fractional-epoch handling would also have to hold for both consumers (journalctl --since @… and SCT_COREDUMPS_SINCE_EPOCH in utils/upload_sct_coredump.sh), which is more surface than the benefit justifies. Happy to revisit if a real overlap ever shows up in a collected archive.
✅ Test Summary: PASSED✅ Precommit: PASSED
✅ Tests: PASSED
|
|
@claude review |
|
Claude finished @fruch's task in 1m 42s —— View job Code Review
The core idea is correct and the implementation is mostly solid. Two issues need attention before merge.
|
| Severity | File | Issue |
|---|---|---|
| Major | collectBuilderLogs.groovy:22 |
Fallback on sudo failure should attempt unprivileged journalctl --boot, not silently skip |
| Minor | collectBuilderLogs.groovy:8, collectTestCoredumps.groovy:7 |
Millisecond truncation in epoch calculation |
| Minor | upload_sct_coredump.sh:26,31 |
Two separate hydra.sh find calls introduce a race window |
The coredump bounding logic in upload_sct_coredump.sh is correct and well-structured. The journal fix needs the fallback path described in the PR description to actually be implemented.
Both collectors assume a builder that dies after its build, which stops
being true the moment a job runs on a static agent:
* collectBuilderLogs ran an unbounded `sudo journalctl`, so every build
uploaded the host's entire journal - weeks of it on a long-lived
machine - and a sudo that is not passwordless aborted the stage
outright.
* upload_sct_coredump.sh tarred every coredump the host had ever
produced, including other jobs' and other tenants'.
Both are now bounded by the build's own start time, and the journal read
degrades to the current boot instead of failing when sudo is unavailable.
Independent of any particular backend - this is groundwork for running
regular jobs on static builders.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
9264ba5 to
a465d24
Compare
|
@fruch new branch |
| sh """#!/bin/bash | ||
|
|
||
| ./utils/upload_sct_coredump.sh | ||
| SCT_COREDUMPS_SINCE_EPOCH=${sinceEpoch} ./utils/upload_sct_coredump.sh |
There was a problem hiding this comment.
SCT_* names are for the SCT config namespace. sct_config.py doesn't define coredumps_since_epoch config param, so it would reject this as Unsupported environment variables were used
| # Compress the coredumps into a tar.gz file | ||
| ./docker/env/hydra.sh $EXTRA_HYDRA_ARGS "bash -c \"sudo tar --zstd -cf $COREDUMP_TARBALL -C $COREDUMP_DIR .\"" | ||
| # Collect this build's coredumps, if any (find prints them; empty output means nothing new) | ||
| NEW_COREDUMPS=$(./docker/env/hydra.sh $EXTRA_HYDRA_ARGS "bash -c \"find $COREDUMP_DIR -maxdepth 1 -type f -newermt @$SINCE_EPOCH\"" | grep "^$COREDUMP_DIR/" || true) |
There was a problem hiding this comment.
|| true would drop the exit code of hydra.sh execution. Then on line 36 below the scipts returns/prints no coredumps ... nothing to upload, but the script states this as a fact and doesn't check exactly this is the result of hydra command execution and not some other error.
What
Split out of #15618 per review: both builder-side collectors assume a builder that dies after its
build, which stops being true the moment a job runs on a static agent. Groundwork for that,
independent of any backend.
collectBuilderLogsran an unboundedsudo journalctl, so every build uploaded the host'sentire journal — weeks of it on a long-lived machine — and a sudo that is not passwordless
aborted the stage outright. Now bounded by the build's start time, and it degrades to the
current boot instead of failing when sudo is unavailable.
upload_sct_coredump.shtarred every coredump the host had ever produced, including otherjobs' and other tenants'. Now bounded by build start time as well.
Why it was in a minicloud PR
Both bit a minicloud run on a lab machine, which is how they were found — but neither has anything
to do with minicloud, so per review they land on their own.
Tests
sh -nclean; each bounded command run standalone against a live journal, including theno-passwordless-sudo path. No behaviour change on ephemeral cloud builders, where the build start
time is a few minutes after boot and everything is in range anyway.
🤖 Generated with Claude Code