ART-21305: run rpm-lockfile-prototype in podman container#3031
Conversation
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Walkthrough
ChangesRPM lockfile containerization
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant RpmResolver
participant podman
participant rpm-lockfile-prototype
RpmResolver->>podman: Run configured image with mounted paths
podman->>rpm-lockfile-prototype: Execute lockfile resolution
rpm-lockfile-prototype-->>RpmResolver: Write lockfile and return status
RpmResolver->>podman: Retry after clearing corrupt RPMDB cache
Possibly related PRs
Suggested labels: Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
| # Work directory: rpms.in.yaml input and rpms.lock.yaml output | ||
| cmd.extend(["-v", f"{tmpdir}:/work:Z"]) | ||
|
|
||
| # DNF repodata cache | ||
| cmd.extend(["-v", f"{self._cache_path}:/cache:Z"]) | ||
| cmd.extend(["-e", "RPM_LOCKFILE_PROTOTYPE_DNF_CACHE=/cache"]) | ||
|
|
||
| # RPMDB cache: host path → container /root/.cache/... | ||
| RPMDB_CACHE_PATH.mkdir(parents=True, exist_ok=True) | ||
| cmd.extend(["-v", f"{RPMDB_CACHE_PATH}:{CONTAINER_RPMDB_CACHE_PATH}:Z"]) | ||
|
|
||
| # Mount host entitlement certs for accessing protected repos. | ||
| # Use :ro without :Z — SELinux won't allow relabeling system dirs. | ||
| for host_path in ("/etc/pki/entitlement", "/etc/rhsm/ca", "/etc/pki/rpm-gpg"): | ||
| if Path(host_path).is_dir(): | ||
| cmd.extend(["-v", f"{host_path}:{host_path}:ro"]) |
There was a problem hiding this comment.
| # Work directory: rpms.in.yaml input and rpms.lock.yaml output | |
| cmd.extend(["-v", f"{tmpdir}:/work:Z"]) | |
| # DNF repodata cache | |
| cmd.extend(["-v", f"{self._cache_path}:/cache:Z"]) | |
| cmd.extend(["-e", "RPM_LOCKFILE_PROTOTYPE_DNF_CACHE=/cache"]) | |
| # RPMDB cache: host path → container /root/.cache/... | |
| RPMDB_CACHE_PATH.mkdir(parents=True, exist_ok=True) | |
| cmd.extend(["-v", f"{RPMDB_CACHE_PATH}:{CONTAINER_RPMDB_CACHE_PATH}:Z"]) | |
| # Mount host entitlement certs for accessing protected repos. | |
| # Use :ro without :Z — SELinux won't allow relabeling system dirs. | |
| for host_path in ("/etc/pki/entitlement", "/etc/rhsm/ca", "/etc/pki/rpm-gpg"): | |
| if Path(host_path).is_dir(): | |
| cmd.extend(["-v", f"{host_path}:{host_path}:ro"]) | |
| # Work directory: rpms.in.yaml input and rpms.lock.yaml output | |
| cmd.extend(["-v", f"{tmpdir}:/work:z"]) | |
| # DNF repodata cache | |
| cmd.extend(["-v", f"{self._cache_path}:/cache:z"]) | |
| cmd.extend(["-e", "RPM_LOCKFILE_PROTOTYPE_DNF_CACHE=/cache"]) | |
| # RPMDB cache: host path → container /root/.cache/... | |
| RPMDB_CACHE_PATH.mkdir(parents=True, exist_ok=True) | |
| cmd.extend(["-v", f"{RPMDB_CACHE_PATH}:{CONTAINER_RPMDB_CACHE_PATH}:z"]) | |
| # Mount host entitlement certs for accessing protected repos. | |
| # Use :ro without :z — SELinux won't allow relabeling system dirs. | |
| for host_path in ("/etc/pki/entitlement", "/etc/rhsm/ca", "/etc/pki/rpm-gpg"): | |
| if Path(host_path).is_dir(): | |
| cmd.extend(["-v", f"{host_path}:{host_path}:ro"]) |
I'm a bit rusty from my pre-ART days when I was doing similar with a container, but thought I needed z instead of Z for reasons (exclusive vs shared). We should evaluate what this looks like when running 2+ podman run simultaneously.
| rc, _, _ = await cmd_gather_async(["podman", "image", "exists", self._image], check=False) | ||
| if rc == 0: | ||
| return | ||
| self.logger.info("Building rpm-lockfile-prototype container image: %s", self._image) | ||
| rc, _, stderr = await cmd_gather_async( | ||
| ["podman", "build", "-t", self._image, "-f", str(RPM_LOCKFILE_CONTAINERFILE), "."], | ||
| check=False, | ||
| ) |
There was a problem hiding this comment.
Thoughts on guarding this for risk of parallel executions due to potential timing of 2+ determining it doesn't exist and executing image builds?
Not sure what the real-world likelihood might be (low to non-existent?) nor what the impact might be in application (build it a couple times?) when it doesn't exist.
openshift-ci
Bot
added
the
needs-rebase
fgallott
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@fgallott: This pull request references ART-21305 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
fgallott
force-pushed
the
rpm-lockfile-prototype-containerized
branch
from
2ecc7f7 to
5758efb
Compare
openshift-ci
Bot
removed
the
needs-rebase
fgallott
force-pushed
the
rpm-lockfile-prototype-containerized
branch
from
5758efb to
9975723
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
doozer/tests/lockfile_prototype/test_resolver.py (1)
61-65: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
host_tmpdircan be unbound if no/workmount is found.If
cmdever lacks a-v ...:/work:...entry (e.g. a future refactor of_build_podman_cmd), the loop falls through without assigninghost_tmpdir, raising a confusingUnboundLocalErrorinstead of a clear assertion failure.♻️ Proposed fix
+ host_tmpdir = None for i, arg in enumerate(cmd): if arg == "-v" and ":/work:" in cmd[i + 1]: host_tmpdir = cmd[i + 1].split(":")[0] break + self.assertIsNotNone(host_tmpdir, "Expected a /work volume mount in podman command") host_outfile = os.path.join(host_tmpdir, DEFAULT_RPM_LOCKFILE_NAME)🤖 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 `@doozer/tests/lockfile_prototype/test_resolver.py` around lines 61 - 65, Initialize or validate host_tmpdir in the command-processing logic around _build_podman_cmd so the absence of a -v ...:/work:... mount produces a clear assertion failure before host_outfile is constructed. Preserve the existing extraction and break behavior when the work mount is present.doozer/doozerlib/lockfile_prototype/resolver.py (1)
97-107: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueAuth file mount isn't existence-checked, unlike the entitlement cert mounts.
Lines 99-101 guard entitlement/cert mounts with
Path(host_path).is_dir(), butauth_file(lines 104-107) is mounted unconditionally once the env var is set, with noPath(auth_file).is_file()check. A stale/misconfiguredQUAY_AUTH_FILE/REGISTRY_AUTH_FILEwill fail loudly atpodman runtime, but adding the same defensive check would make behavior consistent and the failure clearer.♻️ Proposed fix
auth_file = os.environ.get("QUAY_AUTH_FILE") or os.environ.get("REGISTRY_AUTH_FILE") - if auth_file: + if auth_file and Path(auth_file).is_file(): cmd.extend(["-v", f"{auth_file}:/auth/auth.json:ro,z"]) cmd.extend(["-e", "REGISTRY_AUTH_FILE=/auth/auth.json"])🤖 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 `@doozer/doozerlib/lockfile_prototype/resolver.py` around lines 97 - 107, Update the Registry auth handling around auth_file to mount the file only when Path(auth_file).is_file() is true, while preserving the existing environment-variable precedence and mount arguments. Do not set REGISTRY_AUTH_FILE or add the mount for missing or invalid paths.
🤖 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.
Nitpick comments:
In `@doozer/doozerlib/lockfile_prototype/resolver.py`:
- Around line 97-107: Update the Registry auth handling around auth_file to
mount the file only when Path(auth_file).is_file() is true, while preserving the
existing environment-variable precedence and mount arguments. Do not set
REGISTRY_AUTH_FILE or add the mount for missing or invalid paths.
In `@doozer/tests/lockfile_prototype/test_resolver.py`:
- Around line 61-65: Initialize or validate host_tmpdir in the
command-processing logic around _build_podman_cmd so the absence of a -v
...:/work:... mount produces a clear assertion failure before host_outfile is
constructed. Preserve the existing extraction and break behavior when the work
mount is present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift-eng/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8e480a51-ef3b-405d-b3ea-daf3e06d341e
📒 Files selected for processing (3)
doozer/doozerlib/lockfile_prototype/constants.pydoozer/doozerlib/lockfile_prototype/resolver.pydoozer/tests/lockfile_prototype/test_resolver.py
4 participants
Replace host system Python subprocess with podman container execution. The container image is built on demand from a bundled Containerfile (Fedora base, pinned to v0.22.0) if not already present locally, removing the dependency on host-installed python3-dnf and rpm-lockfile-prototype.
The container mounts host entitlement certs for CDN repo access and bakes in Red Hat IT Root CA certs for internal repo connectivity.
Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com
rh-pre-commit.version: 2.3.2
rh-pre-commit.check-secrets: ENABLED
Summary by CodeRabbit