build-sync: Mirror RHCOS images to QCI#2991
Open
joepvd wants to merge 1 commit into
Open
Conversation
Contributor
|
[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 |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR adds QCI mirroring functionality to ChangesRHCOS-to-QCI Mirroring Integration
🎯 4 (Complex) | ⏱️ ~45 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
1 task
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
pyartcd/tests/pipelines/test_build_sync.py (2)
144-147: 💤 Low valueTest assertions may be fragile due to
uses_konflux_imagestream_overridedependency.The assertions check for
'art-latest:rhel-coreos'butis_base_namedepends onuses_konflux_imagestream_override(self.version). If this function returnsFalsefor version4.17, the actual imagestream name would be4.17-konflux-art-latest, causing these assertions to fail.Consider mocking
uses_konflux_imagestream_overrideor using a more flexible assertion pattern.🤖 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 `@pyartcd/tests/pipelines/test_build_sync.py` around lines 144 - 147, The test assertions are brittle because the imagestream name in the command depends on uses_konflux_imagestream_override(self.version); update the test to either mock uses_konflux_imagestream_override to return a known value for the tested version or relax the assertions to accept both possible imagestream forms (e.g., check for suffix ':rhel-coreos' and that the command contains either 'art-latest:rhel-coreos' or '4.17-konflux-art-latest:rhel-coreos'); locate the call site around mock_cmd_gather and the helper is_base_name/uses_konflux_imagestream_override to implement the mocking or flexible assertion so the test no longer fails depending on the override result.
275-282: 💤 Low valueUse constant instead of hardcoded string for registry check.
Line 282 uses hardcoded
'quay.io/openshift'while line 275 already importsREGISTRY_QUAY_CI. Consider importing and usingREGISTRY_QUAY_OPENSHIFTfor consistency with the implementation.# Should include QCI registry from artcommonlib.constants import REGISTRY_QUAY_CI + from artcommonlib.constants import REGISTRY_QUAY_OPENSHIFT assert REGISTRY_QUAY_CI in call_kwargs['registries'] # Should include QCI credentials assert len(call_kwargs['credentials']) == 1 cred = call_kwargs['credentials'][0] - assert cred.registry == 'quay.io/openshift' + assert cred.registry == REGISTRY_QUAY_OPENSHIFT🤖 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 `@pyartcd/tests/pipelines/test_build_sync.py` around lines 275 - 282, Replace the hardcoded registry string in the test with the proper constant: instead of asserting cred.registry == 'quay.io/openshift', import and use REGISTRY_QUAY_OPENSHIFT (similar to the existing REGISTRY_QUAY_CI import) and assert cred.registry == REGISTRY_QUAY_OPENSHIFT so the test uses the canonical constant; update the import list to include REGISTRY_QUAY_OPENSHIFT and adjust the assertion that inspects call_kwargs['credentials'] / cred.pyartcd/pyartcd/pipelines/build_sync.py (1)
498-500: 💤 Low valueQCI mirroring failures are silently swallowed.
When mirroring fails (e.g., network issues, auth problems), the exception is logged and a Slack notification is sent, but the build continues without re-raising. This means QCI mirroring failures won't fail the overall build-sync job.
If this is intentional (to avoid breaking builds for a non-critical mirror), consider documenting this decision in the code comment. If QCI mirrors are expected to be reliable, consider re-raising after notification so operators are alerted via build failure.
🤖 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 `@pyartcd/pyartcd/pipelines/build_sync.py` around lines 498 - 500, The except block currently logs the error and notifies Slack but swallows the exception (self.logger.error(...) and await self.slack_client.say(...)) which lets build-sync continue on QCI mirror failures; update the handler in build_sync.py (the method containing these lines) to either re-raise the exception after sending the Slack notification (add "raise" after await self.slack_client.say(...)) so the job fails on mirror errors, or if swallowing is intentional add a clear code comment explaining why this is tolerated and what alerts/operators should expect; reference the existing symbols self.logger.error and await self.slack_client.say when making the change.
🤖 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 `@pyartcd/pyartcd/pipelines/build_sync.py`:
- Around line 498-500: The except block currently logs the error and notifies
Slack but swallows the exception (self.logger.error(...) and await
self.slack_client.say(...)) which lets build-sync continue on QCI mirror
failures; update the handler in build_sync.py (the method containing these
lines) to either re-raise the exception after sending the Slack notification
(add "raise" after await self.slack_client.say(...)) so the job fails on mirror
errors, or if swallowing is intentional add a clear code comment explaining why
this is tolerated and what alerts/operators should expect; reference the
existing symbols self.logger.error and await self.slack_client.say when making
the change.
In `@pyartcd/tests/pipelines/test_build_sync.py`:
- Around line 144-147: The test assertions are brittle because the imagestream
name in the command depends on uses_konflux_imagestream_override(self.version);
update the test to either mock uses_konflux_imagestream_override to return a
known value for the tested version or relax the assertions to accept both
possible imagestream forms (e.g., check for suffix ':rhel-coreos' and that the
command contains either 'art-latest:rhel-coreos' or
'4.17-konflux-art-latest:rhel-coreos'); locate the call site around
mock_cmd_gather and the helper is_base_name/uses_konflux_imagestream_override to
implement the mocking or flexible assertion so the test no longer fails
depending on the override result.
- Around line 275-282: Replace the hardcoded registry string in the test with
the proper constant: instead of asserting cred.registry == 'quay.io/openshift',
import and use REGISTRY_QUAY_OPENSHIFT (similar to the existing REGISTRY_QUAY_CI
import) and assert cred.registry == REGISTRY_QUAY_OPENSHIFT so the test uses the
canonical constant; update the import list to include REGISTRY_QUAY_OPENSHIFT
and adjust the assertion that inspects call_kwargs['credentials'] / cred.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: ac4a2d90-545e-4611-9983-5817d0362fff
📒 Files selected for processing (2)
pyartcd/pyartcd/pipelines/build_sync.pypyartcd/tests/pipelines/test_build_sync.py
joepvd
force-pushed
the
mirror-rhcos-to-qci
branch
from
0500142 to
331df40
Compare
Contributor
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 `@pyartcd/pyartcd/pipelines/build_sync.py`:
- Around line 521-527: The private path is querying the wrong namespace: when
private is True, change the namespace used for the ART istag lookup (the
namespace variable next to is_name) to the ART private imagestream namespace
used by _backup_all_imagestreams (the ocp*-priv namespace) instead of
'ocp-private' so _tag_into_ci_imagestream() pulls from the ART private source;
update the namespace assignment for the private branch to derive/use the same
ART private namespace value that _backup_all_imagestreams uses.
🪄 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 1b24a9d3-54bc-41ed-b4c0-07f4f816fb82
📒 Files selected for processing (2)
pyartcd/pyartcd/pipelines/build_sync.pypyartcd/tests/pipelines/test_build_sync.py
Contributor
Author
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
joepvd
force-pushed
the
mirror-rhcos-to-qci
branch
2 times, most recently
from
558912f to
ad38007
Compare
Contributor
Author
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@pyartcd/pyartcd/pipelines/build_sync.py`:
- Around line 558-564: The current loop builds a dest pullspec using
REGISTRY_QUAY_CI and runs `oc tag` which only creates an ImageStreamTag
in-cluster and does not create tags in Quay; update the loop that iterates over
qci_tag (and uses variables dest and tag_pullspec) to use the same registry
mirror/push flow used elsewhere in this module (i.e., the oc image mirror /
mirroring logic that actually pushes to external registries) instead of `oc tag
--reference`; specifically, replace the `oc tag` invocation with the
image-mirroring call/path that mirrors tag_pullspec to quay pullspec
(constructed from REGISTRY_QUAY_CI and qci_tag) so the QCI tags land in quay.io
rather than just on the ImageStream.
- Around line 529-533: The oc jsonpath expression is over-braced causing empty
tag_pullspec: change the cmd string built into cmd (used with
exectools.cmd_gather_async) to use a single jsonpath brace expression like
-o=jsonpath={.tag.from.name}. Remove passing retries into
exectools.cmd_assert_async (it forwards kwargs to create_subprocess_exec);
either implement retry logic around cmd_assert_async calls or use a helper that
retries cmd_gather_async instead. Finally, stop using a registry-style pullspec
for oc tag dest: build dest as an ImageStreamTag (namespace/imagestream:tag),
e.g., set dest to f"openshift/ci:{tag}" or f"{dest_namespace}/{dest_is}:{tag}"
before invoking oc tag so oc receives an imagestream:tag target.
In `@pyartcd/pyartcd/pipelines/rebuild_golang_rpms.py`:
- Around line 170-177: The current truthiness checks on self.rpms cause an
explicit empty --rpms list to be treated like "no filter" and widen to all RPMs;
change those boolean checks to explicitly test for None so an empty list remains
a valid explicit filter (e.g., replace "if self.rpms:" with "if self.rpms is not
None" in the filtering logic and the later checks that rely on self.rpms), and
ensure the existing exclusion/filtering code (using get_excluded_rpms and
assigning self.rpms = filtered) preserves an empty list instead of converting it
to a falsy None-like behavior.
- Around line 139-154: The code currently creates a Jira client unconditionally
via self.runtime.new_jira_client() before checking self.runtime.dry_run, which
causes Jira auth to run on dry-run paths; change the flow so the Jira client is
only created when actually needed — for example, move the call to
self.runtime.new_jira_client() inside the non-dry-run branch (after the
self.runtime.dry_run check) or create it lazily just before calling
jira_client.add_comment/close_with_resolution for each bug; update the block
around excluded_bugs, the self.runtime.dry_run check, and the uses of
jira_client.add_comment/close_with_resolution so no Jira client is instantiated
during dry runs.
In `@pyartcd/tests/test_rebuild_golang_rpms.py`:
- Around line 1-7: Remove the unused import EXCLUDED_RPMS from the test module
import list so Ruff F401 is resolved; update the import statement that currently
reads "from pyartcd.pipelines.rebuild_golang_rpms import EXCLUDED_RPMS,
RebuildGolangRPMsPipeline" to only import RebuildGolangRPMsPipeline (leave the
class name RebuildGolangRPMsPipeline unchanged so tests still reference it).
🪄 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 5f800289-e57c-4394-9278-be79bcf2422b
📒 Files selected for processing (7)
.worktrees/autoclose-golang-bugs.worktrees/providespyartcd/pyartcd/jira_client.pypyartcd/pyartcd/pipelines/build_sync.pypyartcd/pyartcd/pipelines/rebuild_golang_rpms.pypyartcd/tests/pipelines/test_build_sync.pypyartcd/tests/test_rebuild_golang_rpms.py
✅ Files skipped from review due to trivial changes (1)
- .worktrees/provides
joepvd
force-pushed
the
mirror-rhcos-to-qci
branch
from
ad38007 to
fd750e8
Compare
Contributor
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 `@pyartcd/tests/pipelines/test_build_sync.py`:
- Around line 288-290: The test currently asserts that the created credential
uses REGISTRY_QUAY_OPENSHIFT; update the expectation to REGISTRY_QUAY_CI when
the implementation bug is fixed: locate the assertion using
call_kwargs['credentials'], the local variable cred, and the constant
REGISTRY_QUAY_OPENSHIFT in the test_build_sync.py test and replace it with
REGISTRY_QUAY_CI so the test validates the corrected QCI credential registry.
🪄 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 5a5aab35-5675-4974-a992-e698961f3490
📒 Files selected for processing (7)
.worktrees/autoclose-golang-bugs.worktrees/providespyartcd/pyartcd/jira_client.pypyartcd/pyartcd/pipelines/build_sync.pypyartcd/pyartcd/pipelines/rebuild_golang_rpms.pypyartcd/tests/pipelines/test_build_sync.pypyartcd/tests/test_rebuild_golang_rpms.py
✅ Files skipped from review due to trivial changes (1)
- .worktrees/provides
🚧 Files skipped from review as they are similar to previous changes (3)
- pyartcd/pyartcd/jira_client.py
- pyartcd/tests/test_rebuild_golang_rpms.py
- .worktrees/autoclose-golang-bugs
joepvd
force-pushed
the
mirror-rhcos-to-qci
branch
2 times, most recently
from
3307a56 to
1253bbf
Compare
Contributor
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 `@pyartcd/pyartcd/pipelines/build_sync.py`:
- Around line 487-499: The asyncio.gather call currently cancels all pending
mirror tasks when any _mirror_single_rhcos_to_qci(tag, ...) raises; change the
gather call to not fail-fast by using asyncio.gather(...,
return_exceptions=True) or wrap each invocation in a small coroutine that
catches and logs its own exceptions so one tag failure doesn't abort the rest —
target the block that builds tasks from tags_to_transfer and the asyncio.gather
invocation, ensure per-tag failures are logged (including exception details) and
that the overall flow still logs final success/failure summary for self.version.
🪄 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 51454158-ebe0-4100-bda4-7cc24562bafb
📒 Files selected for processing (1)
pyartcd/pyartcd/pipelines/build_sync.py
Comment on lines
+487
to
+499
| tasks = [] | ||
| for tag in tags_to_transfer: | ||
| # Mirror to public QCI | ||
| tasks.append(self._mirror_single_rhcos_to_qci(tag, prune_timestamp, private=False)) | ||
| # Mirror to private QCI (for openshift-priv CI) | ||
| tasks.append(self._mirror_single_rhcos_to_qci(tag, prune_timestamp, private=True)) | ||
|
|
||
| await asyncio.gather(*tasks) | ||
| self.logger.info('Successfully mirrored RHCOS images to QCI') | ||
|
|
||
| except Exception as e: | ||
| self.logger.error('Failed to mirror RHCOS images to QCI: %s', e) | ||
| await self.slack_client.say(f'Unable to mirror CoreOS images to QCI for {self.version}: {e}') |
Contributor
There was a problem hiding this comment.
Don’t let one failed tag cancel the rest of the mirror batch.
Line 494 fails fast: if one _mirror_single_rhcos_to_qci() raises, asyncio.gather() cancels the remaining QCI updates. That makes this “best effort” step stop after the first failure instead of attempting the rest of the tags.
♻️ Suggested change
- await asyncio.gather(*tasks)
- self.logger.info('Successfully mirrored RHCOS images to QCI')
+ results = await asyncio.gather(*tasks, return_exceptions=True)
+ failures = [result for result in results if isinstance(result, Exception)]
+ if failures:
+ raise ChildProcessError(f'{len(failures)} QCI mirror task(s) failed')
+ self.logger.info('Successfully mirrored RHCOS images to QCI')🧰 Tools
🪛 Ruff (0.15.15)
[warning] 497-497: Do not catch blind exception: Exception
(BLE001)
🤖 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 `@pyartcd/pyartcd/pipelines/build_sync.py` around lines 487 - 499, The
asyncio.gather call currently cancels all pending mirror tasks when any
_mirror_single_rhcos_to_qci(tag, ...) raises; change the gather call to not
fail-fast by using asyncio.gather(..., return_exceptions=True) or wrap each
invocation in a small coroutine that catches and logs its own exceptions so one
tag failure doesn't abort the rest — target the block that builds tasks from
tags_to_transfer and the asyncio.gather invocation, ensure per-tag failures are
logged (including exception details) and that the overall flow still logs final
success/failure summary for self.version.
b7b444d to
40fe337
Compare
Tag RHCOS container images (rhel-coreos, machine-os-content, etc.) to quay.io/openshift/ci during build-sync for external CI consumption. Uses `oc tag` with reference mode: - --reference-policy='source' - --import-mode='PreserveOriginal' - --reference QCI tag format: - Prunable: quay.io/openshift/ci:<timestamp>_prune_art__ocp_<version>_<tag> - Floating: quay.io/openshift/ci:art__ocp_<version>_<tag> Requires QCI_USER/QCI_PASSWORD env vars (already in Jenkinsfile).
joepvd
force-pushed
the
mirror-rhcos-to-qci
branch
from
40fe337 to
4503eb8
Compare
Contributor
|
@joepvd: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Tag RHCOS container images to QCI (
quay.io/openshift/ci) during build-sync for external CI consumption.Uses
oc tagwith reference mode (as suggested by reviewer) to create image references without copying bytes:--reference-policy='source'- Reference points to source location--import-mode='PreserveOriginal'- Preserve original manifest format--reference- Create reference without importing imageQCI Pullspec Format
Images are tagged with:
quay.io/openshift/ci:<timestamp>_prune_art__ocp_<version>_<tag>quay.io/openshift/ci:art__ocp_<version>_<tag>Examples for OCP 4.17
Public images:
Private images (for openshift-priv CI):
Implementation Details
_mirror_rhcos_to_qci()orchestration method with guards for version ≥4.12, stream assembly, and QCI credentials_mirror_single_rhcos_to_qci()to fetch ART imagestream pullspecs and tag to QCI_tag_into_ci_imagestream)QCI_USER/QCI_PASSWORDenvironment variables (requires Jenkinsfile update in aos-cd-jobs)Test Plan