payload-snapshot: extract RHCOS rpmdb.sqlite into snapshot - #579
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe PR updates ci plugin version metadata and adds optional RHCOS RPMDB extraction to payload snapshots. It threads collected RPMDB data into ChangesCI plugin and payload snapshot updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 9 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 markdownlint-cli2 (0.22.1)plugins/ci/skills/payload-snapshot/SKILL.mdmarkdownlint-cli2 wrapper config was not available before execution Comment |
| def _fetch_image_references(self) -> Optional[dict]: | ||
| """Read /release-manifests/image-references from the release image.""" | ||
| output = _run_podman([ | ||
| "podman", "run", "--rm", "--entrypoint", "cat", |
There was a problem hiding this comment.
I don't think this will work in prow without changes
There was a problem hiding this comment.
Can we install podman in the conatiner and configure the payload jobs to enable nested podman?
There was a problem hiding this comment.
There should be a gate in front of this that should degrade gracefully when podman is not available or does not work.
Enabling podman seems possible but not entirely trivial. https://docs.ci.openshift.org/how-tos/nested-podman/ has some docs but it needs the image to be built in a special way. The intent is the workloads would use a TP-maintained image as a base but our image is also fairly invovled, so it felt better to mimic the setup in our build: #581 tries that.
The final step would be a change in how the ci-op job is configured.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py`:
- Around line 988-1001: `_read_existing()` is rebuilding `rhcos_rpms` entries
with only `tag` and `name`, so the resumed `collect()` path does not match the
documented summary schema and drops `pullspec`. Update the resume logic in
`payload_snapshot.py` so it can reconstruct the full entry metadata for each
rpmdb, using a persisted sidecar or equivalent metadata source written during
extraction, and ensure the returned dicts include the same fields as the
fresh-extraction path (`tag`, `name`, `pullspec`, and `rpmdb`). Keep the
existing `collect()`/`.complete` flow intact while making `_read_existing()`
produce schema-complete entries.
- Around line 895-913: The payload snapshot completion marker is being written
unconditionally in the RPMDB extraction flow, which can cache a failed run as
complete. In the method that loops over rhcos_images and builds summaries, only
call _write_text(self._marker, "") when _extract_rpmdb succeeds for the expected
images, and skip marking complete if any extraction fails or if summaries is
empty. Use the existing collect/_read_existing and extraction loop in
payload_snapshot.py to ensure a transient failure does not permanently disable
future RPMDB collection.
In `@plugins/ci/skills/payload-snapshot/SKILL.md`:
- Around line 147-151: The rpm example in the skill docs is using the SQLite
file path for rpm’s --dbpath, which should be the database directory instead.
Update the Query RHCOS RPM packages example so the rpm command points to the
payload/<version>/<stream>/<tag>/rpmdb directory, and keep
payload/<version>/<stream>/<tag>/rpmdb/rhel-coreos.sqlite only in the sqlite3
example; use the nearby “Query RHCOS RPM packages” section as the location cue.
🪄 Autofix (Beta)
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: Enterprise
Run ID: b5ad5796-e50d-45eb-bee1-d3bce1d0a882
📒 Files selected for processing (5)
.claude-plugin/marketplace.jsondocs/index.htmlplugins/ci/.claude-plugin/plugin.jsonplugins/ci/skills/payload-snapshot/SKILL.mdplugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py
1b756c2 to
12c64c3
Compare
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 `@plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py`:
- Around line 993-996: The metadata loading path in payload_snapshot.py returns
any valid JSON from metadata_path without validating its shape, which can later
break summary generation when entry['tag'] is assumed. Update the metadata
read/return logic in the file-open block to verify the parsed value is the
expected list of snapshot entries and that each entry contains a tag field
before returning it; if the JSON is not in the expected format, treat it as
invalid and fall back to regenerating metadata.
🪄 Autofix (Beta)
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: Enterprise
Run ID: 0a27310f-4595-4eed-b19c-30b4926cb813
📒 Files selected for processing (4)
.claude-plugin/marketplace.jsondocs/index.htmlplugins/ci/.claude-plugin/plugin.jsonplugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py
✅ Files skipped from review due to trivial changes (2)
- plugins/ci/.claude-plugin/plugin.json
- docs/index.html
🚧 Files skipped from review as they are similar to previous changes (1)
- .claude-plugin/marketplace.json
42b8559 to
16ba905
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py`:
- Around line 1963-1966: The RPMDB lookup in payload_snapshot should be treated
as optional, so update the flow around _get_pullspec_map() to catch
HTTPError/URLError and skip RPMDB instead of letting the exception escape to
main(). Keep the existing no-pullspecs fast path in place, and make sure the
failure path logs a skip message and returns an empty result so summary
generation can continue after the core snapshot work.
- Around line 2518-2520: The podman warning in the RPMDB extraction path is too
specific because `_check_podman()` also fails for runtime, storage, and
namespace issues, not just missing binaries. Update the warning text in the
`collect_rpmdb` / `_check_podman()` branch to describe a generic podman
unavailability or failure condition instead of saying it was “not found” or
telling users to “Install podman” unconditionally. Keep the message accurate for
nested-container CI by referring to podman execution/setup failure rather than
absence.
- Around line 1393-1395: The documentation string in payload_snapshot.py for
`rhcos_rpms[]` mixes up `rpm --dbpath` usage with the `rpmdb.sqlite` file path;
update the help text near the `payload_snapshot` payload schema so it keeps the
`sqlite3` example for the file, and either states that `rpm --dbpath` expects
the containing database directory or explains the directory/rename step before
using `rpm`.
🪄 Autofix (Beta)
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: Enterprise
Run ID: f6df0631-e55f-4915-8277-3271e7a3964a
📒 Files selected for processing (5)
.claude-plugin/marketplace.jsondocs/index.htmlplugins/ci/.claude-plugin/plugin.jsonplugins/ci/skills/payload-snapshot/SKILL.mdplugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py
✅ Files skipped from review due to trivial changes (3)
- .claude-plugin/marketplace.json
- plugins/ci/.claude-plugin/plugin.json
- docs/index.html
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/ci/skills/payload-snapshot/SKILL.md
16ba905 to
d1df4a3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py (1)
914-917: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winOnly mark RPMDB extraction complete after all expected variants succeed.
With the current
if summaries:guard, a payload withrhel-coreosextracted butrhel-coreos-10failed gets a.completemarker and will never retry the missing variant. Track whetherlen(summaries) == len(rhcos_images)before writing the marker.Proposed fix
- if summaries: + if summaries: metadata_path = os.path.join(self.rpmdb_dir, "metadata.json") _write_text(metadata_path, json.dumps(summaries, indent=2)) + if len(summaries) == len(rhcos_images): _write_text(self._marker, "")🤖 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 `@plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py` around lines 914 - 917, The RPMDB extraction completion check in payload_snapshot.py is too permissive because the .complete marker is written whenever any summaries exist. Update the logic around the summaries/metadata write path in the RPMDB extraction flow so the marker is only written when all expected variants have succeeded, using len(summaries) == len(rhcos_images) as the gate before calling _write_text on self._marker. Keep writing metadata.json when partial results exist, but do not finalize extraction until every expected image variant is present.
🤖 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 `@plugins/ci/skills/payload-snapshot/SKILL.md`:
- Around line 184-185: The `rhcos_rpms[]` schema description is misleading
because it says the field contains full package lists, but `summary.json` only
stores RPMDB metadata and the relative `rpmdb` path. Update the wording in
`SKILL.md` to match the actual structure used by `payloads[]` and
`rhcos_rpms[]`, keeping the description aligned with the symbols `payloads[]`,
`rhcos_changes[]`, and `rhcos_rpms[]` so readers understand that `rhcos_rpms[]`
is metadata only and not a package list.
---
Duplicate comments:
In `@plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py`:
- Around line 914-917: The RPMDB extraction completion check in
payload_snapshot.py is too permissive because the .complete marker is written
whenever any summaries exist. Update the logic around the summaries/metadata
write path in the RPMDB extraction flow so the marker is only written when all
expected variants have succeeded, using len(summaries) == len(rhcos_images) as
the gate before calling _write_text on self._marker. Keep writing metadata.json
when partial results exist, but do not finalize extraction until every expected
image variant is present.
🪄 Autofix (Beta)
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: Enterprise
Run ID: a00c19f4-8487-4345-a56e-77ac413d5049
📒 Files selected for processing (5)
.claude-plugin/marketplace.jsondocs/index.htmlplugins/ci/.claude-plugin/plugin.jsonplugins/ci/skills/payload-snapshot/SKILL.mdplugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py
✅ Files skipped from review due to trivial changes (3)
- .claude-plugin/marketplace.json
- plugins/ci/.claude-plugin/plugin.json
- docs/index.html
d1df4a3 to
726c251
Compare
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py (1)
914-917: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDon’t mark RPMDB collection complete after only a partial variant extraction.
Line 914 writes
.completewhenever at least one RHCOS image succeeded. Ifrhel-coreossucceeds butrhel-coreos-10fails transiently, the next run skips extraction and permanently omits the missing variant from the snapshot.Proposed guard
- if summaries: + if len(summaries) == len(rhcos_images): metadata_path = os.path.join(self.rpmdb_dir, "metadata.json") _write_text(metadata_path, json.dumps(summaries, indent=2)) _write_text(self._marker, "") + elif summaries: + _log(" RPMDB extraction incomplete; will retry on next run") return summaries🤖 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 `@plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py` around lines 914 - 917, The RPMDB collection flow in payload_snapshot.py is marking the snapshot complete too early in the summaries-writing block, even when only a subset of variants succeeded. Update the logic around the summaries/metadata_path/_write_text(self._marker, "") path so .complete is written only after all expected RHCOS variants have been successfully extracted, and make the completeness check use the full variant set tracked by this snapshot step rather than “any summaries present.”
🤖 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 `@plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py`:
- Around line 2211-2230: The podman availability checks in the helper functions
around _run_podman and the podman info probe only handle FileNotFoundError and
timeout cases, so OS-level exec failures like PermissionError can still abort
the snapshot. Update the exception handling in those subprocess.run() wrappers
to also catch OSError variants and return the same unavailable/None fallback,
keeping optional RPMDB collection disabled rather than failing the snapshot
flow.
---
Duplicate comments:
In `@plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py`:
- Around line 914-917: The RPMDB collection flow in payload_snapshot.py is
marking the snapshot complete too early in the summaries-writing block, even
when only a subset of variants succeeded. Update the logic around the
summaries/metadata_path/_write_text(self._marker, "") path so .complete is
written only after all expected RHCOS variants have been successfully extracted,
and make the completeness check use the full variant set tracked by this
snapshot step rather than “any summaries present.”
🪄 Autofix (Beta)
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: Enterprise
Run ID: 677c8fc7-b788-4480-a416-41bf59e7b038
📒 Files selected for processing (5)
.claude-plugin/marketplace.jsondocs/index.htmlplugins/ci/.claude-plugin/plugin.jsonplugins/ci/skills/payload-snapshot/SKILL.mdplugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py
✅ Files skipped from review due to trivial changes (4)
- .claude-plugin/marketplace.json
- plugins/ci/.claude-plugin/plugin.json
- docs/index.html
- plugins/ci/skills/payload-snapshot/SKILL.md
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
726c251 to
c9bb246
Compare
|
/test payload-agent |
|
I don't think the presubmit actually exercises the code from this PR, I think it still isntalls the plugin from the standard main branhc marketplace... |
|
openshift/release#81229 should do the trick |
|
/test payload-agent |
|
The most recent rejected nightly had no failed jobs 🤦🏻 The presubmit didn't consider this rare case... |
|
🤦 |
|
I'm trying a little hack, https://prow.ci.openshift.org/view/gs/test-platform-results/pr-logs/pull/openshift-eng_ai-helpers/579/pull-ci-openshift-eng-ai-helpers-main-payload-agent/2071962622172860416 should test 5.0.0-0.nightly-2026-06-29-083331 through |
|
Looks good, graceful degradation seems to work at least |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: petr-muller, stbenjam 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 |
|
/hold cancel |
Extract the raw rpmdb.sqlite from each RHCOS variant image (rhel-coreos, rhel-coreos-10) for every payload in the chain, so the snapshot contains queryable RPM databases without needing live API calls.
Uses podman to read image-references from the release image, then
podman create+podman cpto extract/usr/lib/sysimage/rpm/rpmdb.sqlite. Gracefully degrades when podman is unavailable or non-functional (e.g. nested containers in OpenShift) — usespodman infoto verify the runtime actually works before attempting extraction.New files in the snapshot:
<tag>/rpmdb/rhel-coreos.sqlite,<tag>/rpmdb/rhel-coreos-10.sqlite. New--no-rpmdbflag to skip extraction.🤖 Generated with Claude Code
Summary by CodeRabbit
--no-rpmdb). If the required container runtime isn’t available, RPMDB extraction is skipped and the rest still runs.rpmdb/files andrhcos_rpms[]metadata.0.0.55.