fix(doozer,pyartcd): handle manifest unknown errors in sync-ci-images - #3161
Conversation
The sync-ci-images Jenkins job has been broken for 10+ days across all OCP versions (4.12-5.0). The failure occurs in mirror_image() when the GC-prevention mirror step tries to use a digest obtained from get_image_digest() with `oc image mirror ... @sha256:...`. For multi-arch images (manifest lists), the digest returned by `oc image info` text output is not directly resolvable, causing "manifest unknown" errors that crash the job via cmd_assert(). Two fixes applied: 1. Make the GC-prevention mirror step graceful: replace cmd_assert() with cmd_gather() and handle "manifest unknown" / "not found" errors with a warning instead of a hard failure. The image was already successfully mirrored to the floating QCI tag in the prior step, so the GC-prevention tag is a safety net, not a hard requirement. 2. Improve get_image_digest() to try JSON output first (`-o json`), which returns a digest that works reliably for single-arch images with oc image mirror. Falls back to text-output parsing when JSON fails (which happens for manifest lists), preserving existing behavior. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The _open_reconciliation_prs() method calls doozer's `images:streams prs open` without --ignore-missing-images. When builder images like rhel-9-golang-1.25-openshift-4.22 don't yet exist in the CI registry, doozer's check_if_upstream_image_exists() raises an exception that crashes the pipeline with RuntimeError: PR opening failed with rc=1. The mirror step earlier in the pipeline already gracefully skips missing images. Adding --ignore-missing-images to the PR reconciliation step makes it consistent, preventing the pipeline from crashing when new builder images are defined but not yet available. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
[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 |
WalkthroughImage digest lookup now prefers JSON with text fallback, CI imagestream mirroring tolerates missing manifests while preserving failures for other errors, and reconciliation PR creation passes ChangesCI image synchronization
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
@redhat-chai-bot: 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
doozer/doozerlib/cli/images_streams.py (2)
248-263: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMaintain consistency in
ChildProcessErrorarguments.Other instances in this file that raise
ChildProcessErrorpass the full 3-tuple(rc, stdout, stderr)fromcmd_gatheras the second argument (e.g., line 222, line 316). Here,_is used forstdoutand only a 2-tuple(rc_gc, stderr_gc)is passed. Capturingstdoutand passing the full 3-tuple keeps exception handling consistent.♻️ Proposed refactor
- rc_gc, _, stderr_gc = exectools.cmd_gather(gc_mirror_cmd, retries=3, realtime=True) + rc_gc, stdout_gc, stderr_gc = exectools.cmd_gather(gc_mirror_cmd, retries=3, realtime=True) if rc_gc != 0: if 'manifest unknown' in stderr_gc or 'not found' in stderr_gc: runtime.logger.warning( f'Could not create GC-prevention tag for {upstream_entry_name}, ' f'skipping: {stderr_gc.strip()}' ) print( f'For {upstream_entry_name}, ' f'GC-prevention mirror failed (manifest unknown) - continuing' ) else: raise ChildProcessError( f'Failed to create GC-prevention tag for {upstream_entry_name}: {stderr_gc}', - (rc_gc, stderr_gc), + (rc_gc, stdout_gc, stderr_gc), )🤖 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/cli/images_streams.py` around lines 248 - 263, Update the gc_mirror_cmd handling to capture stdout instead of discarding it, and pass the complete (rc_gc, stdout_gc, stderr_gc) tuple as the second argument to ChildProcessError. Keep the existing manifest/not-found warning path unchanged.
86-98: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix unused variables and improve JSON dictionary validation.
Static analysis correctly points out that
stderris unpacked but never used on lines 86 and 98. You can replace it with_to resolve the warning.Additionally,
.get()does not raise aKeyError, making the exception handler ineffective for that type. To make the parsing more robust against unexpected valid JSON structures (like arrays or strings), you can verify that the parsedinfois a dictionary before calling.get().♻️ Proposed refactor
- rc, stdout, stderr = exectools.cmd_gather(json_cmd) + rc, stdout, _ = exectools.cmd_gather(json_cmd) if rc == 0: try: info = json.loads(stdout) - digest = info.get('digest', '') - if digest: - return digest - except (json.JSONDecodeError, KeyError): + if isinstance(info, dict): + digest = info.get('digest', '') + if digest: + return digest + except json.JSONDecodeError: pass # Fall through to text parsing # Fallback: text output (works for manifest lists where -o json fails) text_cmd = f'oc image info {pullspec}{registry_flag}' - rc, stdout, stderr = exectools.cmd_gather(text_cmd) + rc, stdout, _ = exectools.cmd_gather(text_cmd)🤖 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/cli/images_streams.py` around lines 86 - 98, Update both exectools.cmd_gather assignments in the JSON and text fallback paths to unpack the unused stderr value as _. In the JSON parsing block, validate that the parsed info is a dictionary before calling get('digest'), and remove KeyError from the exception handler while preserving the existing fallback behavior for invalid or unexpected JSON.Source: Linters/SAST tools
🤖 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 `@doozer/tests/cli/test_images_streams.py`:
- Around line 483-491: Update test_get_image_digest_with_registry_config to
configure cmd_gather with side_effect responses that make the JSON attempt fail
and the text fallback succeed. Assert cmd_gather was called twice and verify
--registry-config=/path/to/auth.json is present in each call’s command
arguments.
---
Nitpick comments:
In `@doozer/doozerlib/cli/images_streams.py`:
- Around line 248-263: Update the gc_mirror_cmd handling to capture stdout
instead of discarding it, and pass the complete (rc_gc, stdout_gc, stderr_gc)
tuple as the second argument to ChildProcessError. Keep the existing
manifest/not-found warning path unchanged.
- Around line 86-98: Update both exectools.cmd_gather assignments in the JSON
and text fallback paths to unpack the unused stderr value as _. In the JSON
parsing block, validate that the parsed info is a dictionary before calling
get('digest'), and remove KeyError from the exception handler while preserving
the existing fallback behavior for invalid or unexpected JSON.
🪄 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: Repository: openshift-eng/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0e422a78-78d4-4e8f-a5a1-ab225b6bf2e4
📒 Files selected for processing (3)
doozer/doozerlib/cli/images_streams.pydoozer/tests/cli/test_images_streams.pypyartcd/pyartcd/pipelines/sync_ci_images.py
| def test_get_image_digest_with_registry_config(mocker): | ||
| """Test get_image_digest passes registry config to both attempts.""" | ||
| mock_gather = mocker.patch('doozerlib.cli.images_streams.exectools.cmd_gather') | ||
| mock_gather.return_value = (0, '{"digest": "sha256:abc"}', '') | ||
|
|
||
| images_streams.get_image_digest('quay.io/openshift/ci:test', registry_config='/path/to/auth.json') | ||
|
|
||
| cmd = mock_gather.call_args[0][0] | ||
| assert '--registry-config=/path/to/auth.json' in cmd |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test does not verify the fallback attempt as claimed.
The docstring states that this test verifies the registry config is passed to "both attempts". However, because mock_gather.return_value is configured to simulate a successful JSON response, get_image_digest returns immediately without ever executing the text-fallback attempt.
To accurately test both attempts, configure side_effect to simulate a JSON failure followed by a text success, and then verify the arguments for both calls.
💚 Proposed fix to test both attempts
def test_get_image_digest_with_registry_config(mocker):
"""Test get_image_digest passes registry config to both attempts."""
mock_gather = mocker.patch('doozerlib.cli.images_streams.exectools.cmd_gather')
- mock_gather.return_value = (0, '{"digest": "sha256:abc"}', '')
+ mock_gather.side_effect = [
+ (1, '', 'error: json failed'),
+ (0, 'Digest: sha256:abc\n', '')
+ ]
images_streams.get_image_digest('quay.io/openshift/ci:test', registry_config='/path/to/auth.json')
- cmd = mock_gather.call_args[0][0]
- assert '--registry-config=/path/to/auth.json' in cmd
+ assert mock_gather.call_count == 2
+ assert '--registry-config=/path/to/auth.json' in mock_gather.call_args_list[0][0][0]
+ assert '--registry-config=/path/to/auth.json' in mock_gather.call_args_list[1][0][0]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_get_image_digest_with_registry_config(mocker): | |
| """Test get_image_digest passes registry config to both attempts.""" | |
| mock_gather = mocker.patch('doozerlib.cli.images_streams.exectools.cmd_gather') | |
| mock_gather.return_value = (0, '{"digest": "sha256:abc"}', '') | |
| images_streams.get_image_digest('quay.io/openshift/ci:test', registry_config='/path/to/auth.json') | |
| cmd = mock_gather.call_args[0][0] | |
| assert '--registry-config=/path/to/auth.json' in cmd | |
| def test_get_image_digest_with_registry_config(mocker): | |
| """Test get_image_digest passes registry config to both attempts.""" | |
| mock_gather = mocker.patch('doozerlib.cli.images_streams.exectools.cmd_gather') | |
| mock_gather.side_effect = [ | |
| (1, '', 'error: json failed'), | |
| (0, 'Digest: sha256:abc\n', '') | |
| ] | |
| images_streams.get_image_digest('quay.io/openshift/ci:test', registry_config='/path/to/auth.json') | |
| assert mock_gather.call_count == 2 | |
| assert '--registry-config=/path/to/auth.json' in mock_gather.call_args_list[0][0][0] | |
| assert '--registry-config=/path/to/auth.json' in mock_gather.call_args_list[1][0][0] |
🤖 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/cli/test_images_streams.py` around lines 483 - 491, Update
test_get_image_digest_with_registry_config to configure cmd_gather with
side_effect responses that make the JSON attempt fail and the text fallback
succeed. Assert cmd_gather was called twice and verify
--registry-config=/path/to/auth.json is present in each call’s command
arguments.
1 participant
Summary
Fixes sync-ci-images pipeline failures caused by missing image manifests. The job has been broken for 10+ days across all OCP versions (4.12–5.0), with every build (#515–#614) failing on
manifest unknownerrors.Root Cause
PR #3025 introduced a GC-prevention mirroring step that uses
cmd_assert, which crashes the entire pipeline when a manifest doesn't resolve. PR #3036 added graceful handling for the initial mirror step but not the GC-prevention step. Additionally, the PR reconciliation step (_open_reconciliation_prs) crashes on missing builder images that the mirror step already skips.Changes
Fix 1: doozer — GC-prevention graceful handling (
doozer/doozerlib/cli/images_streams.py)cmd_asserttocmd_gatherin the GC-prevention mirror step, withmanifest unknowndetectionget_image_digest()now tries-o jsonfirst, falls back to text parsing for manifest listsget_image_digestbehaviorFix 2: pyartcd — PR reconciliation missing image handling (
pyartcd/pyartcd/pipelines/sync_ci_images.py)--ignore-missing-imagestoimages:streams prs opencommandTesting
Fixes follow-up to #3025 and #3036.
@locriandev requested in Slack thread
Summary by CodeRabbit
Bug Fixes
Tests