Make sync-ci-images pipeline steps best-effort#3030
Conversation
|
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:
WalkthroughSeparates critical vs best-effort doozer phases in SyncCIImagesPipeline with an async step wrapper, persists SHA conditionally, always cleans the clone dir, and returns 25 on any non-critical failures; images_streams now tracks missing upstream manifests and exits 25 when images were skipped. ChangesBest-effort sync CI images pipeline
images_streams PR opening
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ 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 |
![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 (1)
pyartcd/pyartcd/pipelines/sync_ci_images.py (1)
609-619: 💤 Low valueAdd type hint for
coroparameter.The
coroparameter lacks a type annotation, reducing IDE support and documentation clarity.+from typing import Coroutine, Any + - async def _run_step(self, name: str, coro) -> bool: + async def _run_step(self, name: str, coro: Coroutine[Any, Any, Any]) -> bool:🤖 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/sync_ci_images.py` around lines 609 - 619, The _run_step method's coro parameter is untyped; add a proper typing annotation (e.g., coro: Awaitable[Any] or coro: Coroutine[Any, Any, Any]) and import the corresponding symbol from typing at the top of the module so IDEs and linters know this is an awaitable; update the signature of _run_step(name: str, coro) -> bool to _run_step(self, name: str, coro: Awaitable[Any]) -> bool (or use Coroutine[Any, Any, Any]) and ensure the import (from typing import Awaitable, Any) is added.
🤖 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/sync_ci_images.py`:
- Around line 675-676: _prs-open's special return code (25) is ignored because
_run_step only treats exceptions as failures; call
_open_reconciliation_prs(doozer_opts) directly for the 'prs-open' step, capture
its return code, and if it returns 25 append 'prs-open' to failed_steps and skip
the Redis update path (i.e., preserve the "don't update Redis on partial
success" behavior) otherwise treat success normally; this change touches the
'prs-open' invocation (replace the _run_step wrapper), the
_open_reconciliation_prs function return handling, and the code that updates
Redis after steps to ensure Redis is only updated when no partial-success rc==25
occurred.
---
Nitpick comments:
In `@pyartcd/pyartcd/pipelines/sync_ci_images.py`:
- Around line 609-619: The _run_step method's coro parameter is untyped; add a
proper typing annotation (e.g., coro: Awaitable[Any] or coro: Coroutine[Any,
Any, Any]) and import the corresponding symbol from typing at the top of the
module so IDEs and linters know this is an awaitable; update the signature of
_run_step(name: str, coro) -> bool to _run_step(self, name: str, coro:
Awaitable[Any]) -> bool (or use Coroutine[Any, Any, Any]) and ensure the import
(from typing import Awaitable, Any) is added.
🪄 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: 1351eede-73f5-4e6a-a763-b29ef83c34b2
📒 Files selected for processing (1)
pyartcd/pyartcd/pipelines/sync_ci_images.py
|
The pipeline has a dependency chain where gen-buildconfigs generates and applies BuildConfigs to the CI cluster, then mirror copies builder and base images to CI registries, then start-builds triggers CI builds, wait pauses for build completion, check-upstream verifies imagestream consistency, and finally prs-open creates reconciliation PRs. The gen-buildconfigs step is critical because without BuildConfigs applied to the cluster, there is nothing for downstream steps to build, mirror, or verify. If this fails, downstream steps will either fail or operate on stale state. This step should raise an exception and abort the pipeline. The mirror step is also critical because builds need these images to execute. If mirroring fails, builds will fail with image pull errors. This step should also raise an exception and abort the pipeline. The start-builds step can be best-effort because builds might already exist from previous runs, and missing Konflux builds should not block the entire workflow. If this fails, the pipeline can still verify existing state and open remediation PRs. The step should log a warning and continue. The check-upstream step can be best-effort because this is validation, not a prerequisite for remediation. Even if verification fails, PRs can still be opened to fix drift. The step should log a warning and continue. The prs-open step can be best-effort because it already returns rc=25 for partial success. If this fails, the pipeline can still mark the run as successful. The step should log a warning and return rc=25. Modified _run_step() method: async def _run_step(self, name: str, coro, critical: bool = False) -> bool: Modified run() method: async def run(self) -> int: The key change from the current PR is adding a critical parameter to _run_step() to distinguish hard failures from soft failures. The gen-buildconfigs and mirror steps use critical=True because they are prerequisites for downstream operations, while start-builds, check-upstream, and prs-open continue to be best-effort. The updated docstring clarifies which steps are critical versus best-effort, and the logging distinguishes non-critical steps in the warning message. The PR goal of preventing transient or data issues from blocking the workflow is valid for missing Konflux builds in start-builds, missing CI images in check-upstream, and PR failures in prs-open. However, BuildConfig generation failures mean there is no sync to perform, and mirror failures mean builds cannot pull base images, so these should remain hard failures that abort the pipeline. |
openshift-ci
Bot
added
the
do-not-merge/hold
|
[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 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pyartcd/pyartcd/pipelines/sync_ci_images.py (2)
692-693:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale comment at line 586 contradicts new behavior.
Line 586 states
"Don't update Redis on partial success - retry on next run", but line 692-693 now explicitly updates Redis even whenfailed_stepsis non-empty. The comment at line 586 should be removed or updated to reflect the new intent.Suggested fix at line 586
if rc == 25: self._logger.warning(f"{self.version}: Some PRs skipped (rc=25)") - return 25 # Don't update Redis on partial success - retry on next run + return 25🤖 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/sync_ci_images.py` around lines 692 - 693, The comment "Don't update Redis on partial success - retry on next run" is stale because the code now calls await self._record_successful_run(current_sha) even when failed_steps is non-empty; remove or update that comment near the failed_steps handling to reflect the new intent (either delete the line or change wording to indicate that partial successes are recorded to prevent re-running the same SHA), and ensure references around the failed_steps variable and the _record_successful_run(current_sha) call remain consistent.
776-778:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win**CLI docstring inconsistent with
run()semantics.**The CLI docstring at lines 776-778 describes rc=25 as "some PRs were skipped", but therun()method's docstring (line 653) now defines 25 as "partial (non-critical steps failed)". Update the CLI docstring to match the broader semantics.Suggested fix
Return codes: - 25: Partial success (some PRs were skipped) + 25: Partial/unstable (non-critical steps failed)🤖 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/sync_ci_images.py` around lines 776 - 778, Update the CLI docstring in sync_ci_images.py so its exit-code descriptions match the run() method's semantics: replace the line "25: Partial success (some PRs were skipped)" with a description that matches run()'s docstring (e.g., "25: Partial (non-critical steps failed)" or similar wording). Locate the CLI/help docstring block that lists the numeric exit codes and align the text for code 25 with the run() method's explanation found in run(). Ensure the wording is consistent and clear across both docstrings.
🤖 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.
Outside diff comments:
In `@pyartcd/pyartcd/pipelines/sync_ci_images.py`:
- Around line 692-693: The comment "Don't update Redis on partial success -
retry on next run" is stale because the code now calls await
self._record_successful_run(current_sha) even when failed_steps is non-empty;
remove or update that comment near the failed_steps handling to reflect the new
intent (either delete the line or change wording to indicate that partial
successes are recorded to prevent re-running the same SHA), and ensure
references around the failed_steps variable and the
_record_successful_run(current_sha) call remain consistent.
- Around line 776-778: Update the CLI docstring in sync_ci_images.py so its
exit-code descriptions match the run() method's semantics: replace the line "25:
Partial success (some PRs were skipped)" with a description that matches run()'s
docstring (e.g., "25: Partial (non-critical steps failed)" or similar wording).
Locate the CLI/help docstring block that lists the numeric exit codes and align
the text for code 25 with the run() method's explanation found in run(). Ensure
the wording is consistent and clear across both docstrings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 6421bd4c-634d-4709-9452-18a8c34feb0a
📒 Files selected for processing (1)
pyartcd/pyartcd/pipelines/sync_ci_images.py
shruti-rh
force-pushed
the
sync-ci-images-resilient
branch
4 times, most recently
from
f9adc73 to
b187252
Compare
Each doozer step (gen-buildconfigs, mirror, start-builds, check-upstream, prs-open) now runs independently. If one fails (e.g. missing Konflux build, missing CI image), the pipeline continues with remaining steps and returns exit code 25 (partial) instead of crashing with exit code 50. This prevents transient or data issues in one step from blocking the entire sync workflow. Signed-off-by: Shruti Anekar <sanekar@redhat.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
shruti-rh
force-pushed
the
sync-ci-images-resilient
branch
from
b187252 to
a5f93ff
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/doozerlib/cli/images_streams.py`:
- Around line 1921-1923: The primary parent_upstream_image is never validated —
add a check using check_if_upstream_image_exists(parent_upstream_image) before
appending it or proceeding to open PRs; if the check fails, set images_skipped
(or mirror the existing failure path) and break/return or call exit(25)
consistent with the builder-parent handling so the final parent manifest absence
triggers the same skip/exit behavior as the other upstream_image checks.
- Around line 1882-1894: check_if_upstream_image_exists currently swallows all
exceptions from util.oc_image_info_for_arch(upstream_image) and treats them as
"missing", which hides real oc/auth/network errors; change the logic to call
util.oc_image_info_for_arch(upstream_image, strict=False) and if it returns None
treat the image as missing, but let real exceptions propagate (or handle/log
them separately) instead of mapping them to missing_upstream_images;
additionally, ensure the same existence check/gating is applied to
parent_upstream_image resolved from from_config (i.e., pass
parent_upstream_image through check_if_upstream_image_exists the same way you do
for desired_ci_build_root_image and builder-derived upstream images so a missing
parent manifest is caught and added to missing_upstream_images).
🪄 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: 8e99c0cd-c012-4d21-b949-34d5aad4aa9b
📒 Files selected for processing (2)
doozer/doozerlib/cli/images_streams.pypyartcd/pyartcd/pipelines/sync_ci_images.py
| util.oc_image_info_for_arch(upstream_image) | ||
| checked_upstream_images.add(upstream_image) | ||
| return True | ||
| except Exception: | ||
| yellow_print( | ||
| f'Unable to access upstream image {upstream_image} for {dgk}' | ||
| ' -- check whether buildconfigs are running successfully.' | ||
| ) | ||
| checked_upstream_images.add(upstream_image) | ||
| if ignore_missing_images: | ||
| return True | ||
| missing_upstream_images.add(upstream_image) | ||
| return False |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify API behavior and call pattern for manifest-missing handling
rg -n -C3 'def oc_image_info_for_arch|strict:\s*bool\s*=\s*True|manifest unknown' artcommon/artcommonlib/util.py
rg -n -C3 'check_if_upstream_image_exists|oc_image_info_for_arch\(' doozer/doozerlib/cli/images_streams.pyRepository: openshift-eng/art-tools
Length of output: 4687
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the exact helper and the nearby builder/parent resolution logic
sed -n '1820,1965p' doozer/doozerlib/cli/images_streams.py
# Locate any other uses of parent_upstream_image / oc_image_info_for_arch in this file
rg -n "parent_upstream_image|desired_parents|check_if_upstream_image_exists\(|oc_image_info_for_arch\(" doozer/doozerlib/cli/images_streams.pyRepository: openshift-eng/art-tools
Length of output: 8827
Don’t downgrade non-manifest oc failures to “missing image”, and gate parent_upstream_image too
check_if_upstream_image_exists()catches broadExceptionaroundutil.oc_image_info_for_arch(upstream_image), butoc_image_info_for_arch(..., strict=False)is specifically designed to returnNoneonly for “manifest unknown”; other failures (auth/network/oc errors) should raise instead of being treated as missing (line ~1885).- Existence gating is applied for builder-derived upstream images and
desired_ci_build_root_image, but not forparent_upstream_imageresolved fromfrom_config, so a missing parent manifest can slip through despite the skip-gating intent.
Proposed fix (for the broad exception handling)
- try:
- util.oc_image_info_for_arch(upstream_image)
- checked_upstream_images.add(upstream_image)
- return True
- except Exception:
- yellow_print(
- f'Unable to access upstream image {upstream_image} for {dgk}'
- ' -- check whether buildconfigs are running successfully.'
- )
- checked_upstream_images.add(upstream_image)
- if ignore_missing_images:
- return True
- missing_upstream_images.add(upstream_image)
- return False
+ info = util.oc_image_info_for_arch(upstream_image, strict=False)
+ checked_upstream_images.add(upstream_image)
+ if info is not None:
+ return True
+
+ yellow_print(
+ f'Unable to find upstream image manifest {upstream_image} for {dgk}'
+ ' -- check whether buildconfigs are running successfully.'
+ )
+ if ignore_missing_images:
+ return True
+ missing_upstream_images.add(upstream_image)
+ return False📝 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.
| util.oc_image_info_for_arch(upstream_image) | |
| checked_upstream_images.add(upstream_image) | |
| return True | |
| except Exception: | |
| yellow_print( | |
| f'Unable to access upstream image {upstream_image} for {dgk}' | |
| ' -- check whether buildconfigs are running successfully.' | |
| ) | |
| checked_upstream_images.add(upstream_image) | |
| if ignore_missing_images: | |
| return True | |
| missing_upstream_images.add(upstream_image) | |
| return False | |
| info = util.oc_image_info_for_arch(upstream_image, strict=False) | |
| checked_upstream_images.add(upstream_image) | |
| if info is not None: | |
| return True | |
| yellow_print( | |
| f'Unable to find upstream image manifest {upstream_image} for {dgk}' | |
| ' -- check whether buildconfigs are running successfully.' | |
| ) | |
| if ignore_missing_images: | |
| return True | |
| missing_upstream_images.add(upstream_image) | |
| return False |
🧰 Tools
🪛 Ruff (0.15.15)
[warning] 1885-1885: Do not catch blind exception: Exception
(BLE001)
[warning] 1887-1887: Function definition does not bind loop variable dgk
(B023)
🤖 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 1882 - 1894,
check_if_upstream_image_exists currently swallows all exceptions from
util.oc_image_info_for_arch(upstream_image) and treats them as "missing", which
hides real oc/auth/network errors; change the logic to call
util.oc_image_info_for_arch(upstream_image, strict=False) and if it returns None
treat the image as missing, but let real exceptions propagate (or handle/log
them separately) instead of mapping them to missing_upstream_images;
additionally, ensure the same existence check/gating is applied to
parent_upstream_image resolved from from_config (i.e., pass
parent_upstream_image through check_if_upstream_image_exists the same way you do
for desired_ci_build_root_image and builder-derived upstream images so a missing
parent manifest is caught and added to missing_upstream_images).
Source: Linters/SAST tools
rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED Signed-off-by: Shruti Anekar <sanekar@redhat.com>
shruti-rh
force-pushed
the
sync-ci-images-resilient
branch
from
24b30d4 to
6072ca8
Compare
|
@shruti-rh: The following test 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. |
2 participants
Each doozer step (gen-buildconfigs, mirror, start-builds, check-upstream, prs-open) now runs independently. If one fails (e.g. missing Konflux build, missing CI image), the pipeline continues with remaining steps and returns exit code 25 (partial) instead of crashing with exit code 50.
This prevents transient or data issues in one step from blocking the entire sync workflow.