Repository navigation
fix(image): use synchronous containerd exports - #7643
Conversation
@coderabbitai full review |
✅ Action performedFull review finished. |
Validation at The matching K3s containerd source starts the transfer output copy separately from its request: archive exporter, transfer proxy, ctr file lifetime. This supports the timing hypothesis; the original failing archive was not retained and is not claimed as reproduced. Actual native K3s and Vanilla export/import/restart trials remain required before merge, along with the current-head normal review and CI gates. |
✅MegaLinter analysis: Success✅ Linters with no issuesactionlint, bash-exec, git_diff, hadolint, jscpd, jsonlint, lychee, markdown-table-formatter, markdownlint, prettier, prettier, shellcheck, shfmt, stylelint, syft, trivy-sbom, trufflehog, v8r, v8r, yamllint See detailed reports in MegaLinter artifacts
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
⛔ Files ignored due to path filters (1)
📒 Files selected for processing (9)
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 0 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (8)
🧰 Additional context used📚 Code guidelines (1)📓 Path-based instructions (1)Source excerpt: Follow standard Go conventions in this repository: Source excerpt: Follow standard Go conventions in this repository: **Formatting:** Format code with `gofmt` and keep imports organized with `goimports` (or run `golangci-lin...📄 CodeRabbit inference engine (CONTRIBUTING.md) Files:
🪛 Checkov (3.3.19)images/ksail-analysis-runner/Dockerfile[low] 1-39: Ensure that HEALTHCHECK instructions have been added to container images (CKV_DOCKER_2) 🔇 Additional comments (9)
📝 WalkthroughWalkthroughThe module and analysis-runner image now use Go 1.26.9, and the indirect Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to This change selects synchronous containerd exports when the node supports them and updates the Go toolchain. No concrete merge-blocking risk was found in the supplied context. Native cluster export trials remain to be run, as the PR notes. Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (3 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 13.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 7 files. (2 skipped: 2 unsupported.) Full details: Linked Issues checkExplanation The implementation addresses synchronous exports and related fallback behavior, but the stated acceptance criteria still include outstanding native K3s and Vanilla export/import trials or a bounded archive receipt if those trials fail.
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 |
…nchronous-image-export-7642
devantler
left a comment
devantler
left a comment
There was a problem hiding this comment.
🤖 Generated by the Agentic Engineer
Self-review (fallback — CodeRabbit, Codex and Cursor Bugbot unavailable)
Reviewed commit: 96d6c81
- CodeRabbit: the freshly read authenticated repository included-review refusal, updated 2026-10-09T01:35:46Z, states a 36-minute retry window that remains active. No usage-based billing is authorized. The new head's direct review surfaces contain no current provider verdict; another request is not sent before that reset.
- Codex: freshly read authenticated account usage-limit notice has no stated reset; the completed fresh lane-health receipt reports a newer 2026-10-09T01:18:08Z refusal and no serving recovery. No credits are authorized.
- Cursor Bugbot: freshly read authenticated public user/team usage-limit notice has no stated reset; the completed fresh lane-health receipt reports a newer 2026-10-08T22:47:52Z refusal and no serving recovery. No limit increase is authorized.
I reviewed the final five-file export diff against landed main 4dfe77d. Capability discovery occurs once per operation and preserves the selected mode through bulk export, repair and individual fallbacks. A per-operation copy avoids shared mode mutation. Exact flag tokens preserve older containerd compatibility, and discovery failures or missing final help flags stop export. No error, unsupported flag or asynchronous transfer fallback is silently accepted.
This signed integration absorbs the merged security dependency, leaving the full source tree byte-identical to parent 652b452, which received a finding-free CodeRabbit full review and independent static review. That identical tree passes the full root Go suite, image race tests, affected lint and CLI build; the public K3s regression was RED before the repair and GREEN afterward. Compatibility, incomplete-output and repair/fallback negative controls remain active. Signature verification and source comparison pass, and current main is an ancestor.
Complete direct comments, reviews, check-runs and threads at the new head contain no current provider verdict or reported unresolved finding; there are zero unresolved threads. The original failing archive was not retained, so its individual cause remains a timing hypothesis. Native K3s and Vanilla export/import/restart trials and current-head CI remain required before merge. No check or review setting changes.
Verdict: no P0/P1 findings
Code Coverage OverviewLanguages: Go Go / code-coverage/goThe overall line coverage in commit d7898d1 in the Show a line coverage summary of the most impacted files.
Updated |
Blocker: #7131 | upstream | last-verified 2026-10-09: Open; current head 96d6c81 fails managed Go Code Quality in run 37871381265/job 113630034853 with a runner shutdown at 02:07:31 UTC. Its log also records dependency acquisition/import defects; the shutdown trigger remains unknown. Owned Go analysis and local validation pass; native cluster trials remain pending. Complete managed extraction and root ARC acceptance remain required. |
No longer parked on #7131. With the default branch merged in (head |
devantler
left a comment
devantler
left a comment
There was a problem hiding this comment.
🤖 Generated by the Agentic Engineer
Self-review (fallback — CodeRabbit, Codex and Cursor Bugbot unavailable)
Reviewed commit: d7898d1ee1166e7e1980c8ad471d51ff46f6feb3
- CodeRabbit: the lane-health read at 2026-10-10T14:21Z reports it rate limited until 14:51:51Z (newest refusal: monorepo#4103, updated 14:01:54Z). No paid overage is requested.
- Codex: the same read reports the account usage limit in force since 2026-10-09T18:11:46Z, with no review served since. Recovery needs account action.
- Cursor Bugbot: the same read reports no serving evidence; its last known state is the user/team usage-limit notice.
This round is incremental on the round at 96d6c813; the only commit since is a merge of the default branch, and the change against it is still five files in pkg/svc/image (+187 −4). I re-read the whole change.
- The premise holds against the released containerd sources (read from the Go module proxy): in v2.1.4
ctr images exportdeclares--localas an ordinary flag that is off by default and takes the transfer path when it is not set; in v1.7.27 the flag exists and is on by default, so passing it changes nothing; v1.6.38 has no such flag. That is exactly the three-row table the new test encodes, so the flag is only ever passed where it exists. - Probe safety: the mode is chosen from
export --helpby whole-word match, so--localizationcannot be mistaken for it (tested). Empty or cut-off help is refused by requiring the trailing help entry, instead of silently choosing the asynchronous mode (tested), and a failed probe stops the export with the underlying error (tested). - Scope of the selection: the choice lives on a per-operation copy of the exporter, so the bulk export, per-image fallback and repair retries all use the same mode and concurrent exports cannot change each other's. The command builder inserts the flag before the archive path, which is where
ctrexpects options. - Cost: one extra
ctrcall per export on the node. It can turn a previously working export into a failure only ifctrcannot print its own help, in which case the export command itself would not run either. - Evidence at this head: all 28 Docker system-test jobs (Vanilla, K3s, Talos, VCluster, KWOK), code coverage and
CI - Required Checksaresuccess. Review threads: 0 of 0. The PR merges cleanly.
One thing this does not prove, as the PR body already says: the original unreadable K3s archive was not kept, so whether this was its cause stays unconfirmed. That is a statement about the incident, not a defect in the change.
Verdict: no P0/P1 findings
1 participant
Why
An image export can report success before its archive has finished writing, leaving the next import with an incomplete file. A K3s trial exposed an unreadable archive; its original contents were not retained, so that individual failure still needs confirmation.
What
Use the runtime's synchronous export path and preserve compatibility with older supported runtimes. Repairs and retries use the same selection, and incomplete capability information stops the operation instead of guessing.
Fixes #7642.