Add block/rescue error handling and log visibility in oc-mirror tasks#225
Open
Add block/rescue error handling and log visibility in oc-mirror tasks#225
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:
WalkthroughRefactors oc-mirror runs to create persistent timestamped log files, parameterize oc-mirror flags via new defaults, run oc-mirror inside Ansible Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Ansible
participant FS as "Filesystem ({{ workingDir }}/logs)"
participant ocMirror as "oc-mirror"
participant Task as "Ansible Task / Operator"
Ansible->>FS: ensure logs directory exists
Ansible->>Ansible: set_fact `_oc_mirror_*_log` (timestamped)
Ansible->>Task: debug computed log path
Ansible->>ocMirror: run oc-mirror with parameterized flags -> redirect to `_oc_mirror_*_log`
ocMirror-->>FS: write stdout/stderr to `_oc_mirror_*_log`
alt success
ocMirror-->>Ansible: exit 0
else failure
Ansible->>FS: tail -n 10 `_oc_mirror_*_log`
FS-->>Ansible: last 10 lines (if readable)
Ansible-->>Ansible: fail play with attempts + tailed log + full log path
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
playbooks/tasks/mirror_registry.yaml (1)
49-65:⚠️ Potential issue | 🔴 CriticalPreserve
oc-mirror's exit code through this pipeline.Line 61 makes
untilevaluategrep's return code instead ofoc-mirror's. Withoutpipefail, a successful run with no matching ERROR line returns grep exit code 1 (false failure/retry), while a failedoc-mirrorrun with matching ERROR output returns grep exit code 0 (false success). Useset -o pipefailto capture pipeline failures, save the rc, then rungrepseparately as a non-fatal follow-up:set -o pipefail log_file="{{ workingDir }}/logs/oc-mirror.progress.$(date +%s).log" {{ workingDir }}/bin/oc-mirror ... 2>&1 | tee "$log_file" rc=$? grep "ERROR" "$log_file" || true exit "$rc"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playbooks/tasks/mirror_registry.yaml` around lines 49 - 65, The current "Start oc-mirror process" task lets grep's exit code drive Ansible's until check (registered in r_oc_mirror) so oc-mirror failures get hidden; change the shell block to enable pipefail (set -o pipefail), write the oc-mirror output to a timestamped log_file variable, run oc-mirror piped to tee, capture its exit code into rc, run grep "ERROR" against the saved log as a non-fatal check (grep ... || true), and finally exit with the saved rc so r_oc_mirror reflects oc-mirror's real exit status; keep the same variables (workingDir, mirror_dry_run, ocMirrorLogLevel, pullSecretPath, quayHostname) and the task name and register target (r_oc_mirror).playbooks/tasks/mirror_cache.yaml (1)
37-52:⚠️ Potential issue | 🔴 CriticalThe pipeline exit code is determined by
grep, notoc-mirror, breaking the retry logic.Line 48 runs a pipeline without
set -o pipefail: successfuloc-mirroroutput (no "ERROR" lines) causesgrepto return 1, triggering false retries. Conversely,oc-mirrorfailures containing "ERROR" will causegrepto return 0, masking the error and stopping retries. Enablepipefailin the shell options or saveoc-mirroroutput to the log separately, then grep that file afterward with|| trueto preserve the actual exit code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playbooks/tasks/mirror_cache.yaml` around lines 37 - 52, The shell pipeline for the "Start oc-mirror process" task causes grep to control the exit code so retries (registered as __r_oc_mirror_cache) behave incorrectly; update the ansible.builtin.shell invocation running oc-mirror to enable pipefail (e.g. prefix with "set -o pipefail;") so the task returns oc-mirror's real exit code, or alternatively write oc-mirror output to the log file first and run grep on that file with "|| true" so grepping doesn't change the task exit status; ensure the task still registers to __r_oc_mirror_cache and the until condition remains based on that registration.operators/quay-operator/quay_disconnected.yaml (1)
30-50:⚠️ Potential issue | 🔴 CriticalFix pipeline exit code handling:
grep "ERROR"tail inverts the success condition.Line 45's pipeline makes the task succeed/fail based on
grepexit code instead ofoc-mirrorexit code. Withoutset -o pipefail, this task will:
- Fail (retry) on successful runs with no ERROR output
- Appear successful on failed runs that produce ERROR messages
Preserve the oc-mirror exit code using
set -o pipefailand capture log filtering separately:ansible.builtin.shell: | set -o pipefail {{ workingDir }}/bin/oc-mirror --v2 \ --log-level {{ ocMirrorLogLevel }} \ --authfile {{ workingDir }}/config/pull-secret.quay.json \ -c {{ workingDir }}/config/imagesetconfiguration.internal.yaml \ --workspace file://{{ workingDir }}/config/oc-mirror-workspace-quay \ docker://registry-quay-quay-enterprise.apps.{{ clusterName }}.{{ baseDomain }} \ --dest-tls-verify=false \ --src-tls-verify=false \ --parallel-images 10 \ --parallel-layers {{ 1 if quayBackend == 'LocalStorage' else 10 }} \ --retry-times 10 \ --retry-delay 0 \ --image-timeout 40m0s \ 2>&1 | tee {{ workingDir }}/logs/oc-mirror.progress.quay.$(date +%s).log; \ tee -a {{ workingDir }}/logs/oc-mirror.progress.quay.log | grep "ERROR" || true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/quay-operator/quay_disconnected.yaml` around lines 30 - 50, The task "Start oc-mirror process" currently pipes oc-mirror output into grep which makes the task's exit code depend on grep instead of the oc-mirror command (registered as r_oc_mirror_quay); update the ansible.builtin.shell block to enable pipefail (set -o pipefail) so the oc-mirror exit status is preserved, write logs with tee, and run the grep on the saved log (or tee -a) separately with a harmless "|| true" so that grep does not change the task success; ensure you keep the retries/delay/register/until using r_oc_mirror_quay unchanged.playbooks/tasks/deploy_plugin.yaml (1)
127-144:⚠️ Potential issue | 🔴 CriticalKeep this retry loop keyed to
oc-mirror, notgrep.Line 140 changes the task rc to the tail of the pipeline. That means successful runs with no
ERRORline will retry, while failed runs can pass ifgrepmatches. Apply the same Bash +pipefail+ preserved-rc pattern here, and treat thegrepstep as best-effort only.#!/bin/bash set -uo pipefail printf '%s\n' '--- grep returns 1 when no line matches ---' set +e printf 'INFO only\n' | grep 'ERROR' >/dev/null printf 'grep_rc=%s\n' "$?" # expected: 1 printf '%s\n' '' printf '%s\n' '--- failing producer can look successful when grep matches ---' bash -lc '{ printf "ERROR: boom\n"; exit 42; } | tee /dev/null | grep "ERROR" >/dev/null; printf "pipeline_rc=%s\n" "$?"' # expected: 0 without pipefail printf '%s\n' '' printf '%s\n' '--- affected task definition ---' rg -n -C2 'tee .*grep "ERROR"|until: .* is (success|succeeded)' playbooks/tasks/deploy_plugin.yaml🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playbooks/tasks/deploy_plugin.yaml` around lines 127 - 144, The oc-mirror Ansible task "Run oc-mirror for plugin" currently uses a pipeline ending in grep which mutates the task return code; change the shell invocation so the oc-mirror command's exit code is preserved (use set -o pipefail or capture the oc-mirror RC immediately into a variable), then run tee/grep as best-effort that does not overwrite the saved RC, and ensure the task registers to r_plugin_mirror and retries/until logic checks that saved oc-mirror exit status (not the grep exit). Reference the oc-mirror binary invocation ({{ workingDir }}/bin/oc-mirror), the registered variable r_plugin_mirror, and the task name "Run oc-mirror for plugin" when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@operators/quay-operator/quay_disconnected.yaml`:
- Around line 30-50: The task "Start oc-mirror process" currently pipes
oc-mirror output into grep which makes the task's exit code depend on grep
instead of the oc-mirror command (registered as r_oc_mirror_quay); update the
ansible.builtin.shell block to enable pipefail (set -o pipefail) so the
oc-mirror exit status is preserved, write logs with tee, and run the grep on the
saved log (or tee -a) separately with a harmless "|| true" so that grep does not
change the task success; ensure you keep the retries/delay/register/until using
r_oc_mirror_quay unchanged.
In `@playbooks/tasks/deploy_plugin.yaml`:
- Around line 127-144: The oc-mirror Ansible task "Run oc-mirror for plugin"
currently uses a pipeline ending in grep which mutates the task return code;
change the shell invocation so the oc-mirror command's exit code is preserved
(use set -o pipefail or capture the oc-mirror RC immediately into a variable),
then run tee/grep as best-effort that does not overwrite the saved RC, and
ensure the task registers to r_plugin_mirror and retries/until logic checks that
saved oc-mirror exit status (not the grep exit). Reference the oc-mirror binary
invocation ({{ workingDir }}/bin/oc-mirror), the registered variable
r_plugin_mirror, and the task name "Run oc-mirror for plugin" when making the
change.
In `@playbooks/tasks/mirror_cache.yaml`:
- Around line 37-52: The shell pipeline for the "Start oc-mirror process" task
causes grep to control the exit code so retries (registered as
__r_oc_mirror_cache) behave incorrectly; update the ansible.builtin.shell
invocation running oc-mirror to enable pipefail (e.g. prefix with "set -o
pipefail;") so the task returns oc-mirror's real exit code, or alternatively
write oc-mirror output to the log file first and run grep on that file with "||
true" so grepping doesn't change the task exit status; ensure the task still
registers to __r_oc_mirror_cache and the until condition remains based on that
registration.
In `@playbooks/tasks/mirror_registry.yaml`:
- Around line 49-65: The current "Start oc-mirror process" task lets grep's exit
code drive Ansible's until check (registered in r_oc_mirror) so oc-mirror
failures get hidden; change the shell block to enable pipefail (set -o
pipefail), write the oc-mirror output to a timestamped log_file variable, run
oc-mirror piped to tee, capture its exit code into rc, run grep "ERROR" against
the saved log as a non-fatal check (grep ... || true), and finally exit with the
saved rc so r_oc_mirror reflects oc-mirror's real exit status; keep the same
variables (workingDir, mirror_dry_run, ocMirrorLogLevel, pullSecretPath,
quayHostname) and the task name and register target (r_oc_mirror).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 93ea67e1-8612-4a97-9072-29aae1027f6c
📒 Files selected for processing (4)
operators/quay-operator/quay_disconnected.yamlplaybooks/tasks/deploy_plugin.yamlplaybooks/tasks/mirror_cache.yamlplaybooks/tasks/mirror_registry.yaml
github-actions
Bot
added
deployment
eurijon
force-pushed
the
oc-mirror-tee-grep-error
branch
from
7265ccb to
0fbb430
Compare
github-actions
Bot
removed
ci-cd
eurijon
force-pushed
the
oc-mirror-tee-grep-error
branch
from
0fbb430 to
0a9be30
Compare
eurijon
force-pushed
the
oc-mirror-tee-grep-error
branch
from
0a9be30 to
f321df8
Compare
eurijon
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
operators/quay-operator/quay_disconnected.yaml (1)
41-55: Retries overwrite the log; only the last attempt is preserved fortail -10.The shell uses
> {{ _oc_mirror_quay_log }} 2>&1(truncate). Because_oc_mirror_quay_logis computed once atset_facttime, every Ansible retry (retries: {{ ocMirrorAnsibleRetries }}) writes to the same file and overwrites the previous attempt's output. If an early attempt fails at a different/more informative stage than the final one, that context is gone by the timerescuereads the log.Consider appending instead, so the tail reflects the most recent failure while earlier attempts remain on disk for post-mortem. Same pattern in
mirror_registry.yaml,mirror_cache.yaml, and both blocks indeploy_plugin.yaml.♻️ Minimal change
- > {{ _oc_mirror_quay_log }} 2>&1 + >> {{ _oc_mirror_quay_log }} 2>&1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/quay-operator/quay_disconnected.yaml` around lines 41 - 55, The oc-mirror shell invocation currently truncates the log file (redirect ">" to {{ _oc_mirror_quay_log }}), which causes each Ansible retry to overwrite prior attempts; update the redirection in the oc-mirror command (the ansible.builtin.shell block running {{ workingDir }}/bin/oc-mirror and writing to {{ _oc_mirror_quay_log }}) to append instead (use ">> {{ _oc_mirror_quay_log }} 2>&1" or equivalent) so each retry is preserved; apply the same change to the analogous invocations in mirror_registry.yaml, mirror_cache.yaml, and both oc-mirror blocks in deploy_plugin.yaml.playbooks/tasks/deploy_plugin.yaml (2)
285-287: Disambiguate the second "Show oc-mirror log path" debug task name.There are now two tasks with the identical
name: Show oc-mirror log pathin this file (lines 135 and 285). When scanning play recaps or filtering logs/tags by task name, this makes it ambiguous which mirror stage is being reported. A small rename avoids the confusion.✏️ Suggested rename
-- name: Show oc-mirror log path +- name: Show oc-mirror log path (Quay Enterprise) ansible.builtin.debug: msg: "oc-mirror log file: {{ _oc_mirror_plugin_quay_log }}"Consider similarly naming the earlier one (line 135) e.g.
Show oc-mirror log path (plugin).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playbooks/tasks/deploy_plugin.yaml` around lines 285 - 287, Rename the duplicate Ansible task named "Show oc-mirror log path" to a distinct, descriptive name to disambiguate the two occurrences; locate the task that references the variable _oc_mirror_plugin_quay_log and change its name to something like "Show oc-mirror log path (plugin/quay)" (and optionally rename the earlier task at the other occurrence to "Show oc-mirror log path (main)" or similar) so play recaps and log filtering clearly identify which mirror stage is reported.
143-181: Consider extracting the duplicated oc-mirror block/rescue into a reusable task file.The two
block/rescueflows here (plugin→LZ and plugin→Quay Enterprise) are structurally identical and mirror the same pattern found inmirror_registry.yaml,mirror_cache.yaml, andquay_disconnected.yaml. Each duplication means a future change (e.g., switching>to>>, adding a pre-flightmkdir, adjusting thetailbehavior, or tweaking thefailmessage) needs to be made in 5 places and is easy to get out of sync.A single
playbooks/tasks/_run_oc_mirror.yamlthat takes the log fact name, command args, and a human label as parameters, then executes the block/rescue, would collapse all five call-sites to a handful of lines and remove the risk of drift.Not blocking for this PR — the current changes are a clear improvement — but worth filing as follow-up.
Also applies to: 275-337
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playbooks/tasks/deploy_plugin.yaml` around lines 143 - 181, Extract the duplicated oc-mirror block/rescue into a reusable task file (e.g., playbooks/tasks/_run_oc_mirror.yaml) that accepts parameters for the command args, log fact name (e.g., _oc_mirror_plugin_log), the registered result name (e.g., r_plugin_mirror), human label (used in messages like "oc-mirror for plugin {{ plugin_name }} failed"), and retry/delay vars; implement the same shell invocation, retries, register/until logic and the rescue steps (tail and fail) inside that task file using those variables; then replace the in-place block in deploy_plugin.yaml (the task named "Run oc-mirror for plugin {{ plugin_name }}" and the rescue task "oc-mirror failed") with an include_tasks/import_tasks call to _run_oc_mirror.yaml passing the specific vars (workingDir, plugin_name, pullSecretPath, _oc_mirror_plugin_log, r_plugin_mirror name if needed, ocMirror* retry/delay values, quayHostname, ocMirrorParallelImages, ocMirrorParallelLayers, ocMirrorRetryTimes, ocMirrorRetryDelay, ocMirrorLogLevel) so all five call-sites can reuse the same logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@operators/quay-operator/quay_disconnected.yaml`:
- Around line 41-55: The oc-mirror shell invocation currently truncates the log
file (redirect ">" to {{ _oc_mirror_quay_log }}), which causes each Ansible
retry to overwrite prior attempts; update the redirection in the oc-mirror
command (the ansible.builtin.shell block running {{ workingDir }}/bin/oc-mirror
and writing to {{ _oc_mirror_quay_log }}) to append instead (use ">> {{
_oc_mirror_quay_log }} 2>&1" or equivalent) so each retry is preserved; apply
the same change to the analogous invocations in mirror_registry.yaml,
mirror_cache.yaml, and both oc-mirror blocks in deploy_plugin.yaml.
In `@playbooks/tasks/deploy_plugin.yaml`:
- Around line 285-287: Rename the duplicate Ansible task named "Show oc-mirror
log path" to a distinct, descriptive name to disambiguate the two occurrences;
locate the task that references the variable _oc_mirror_plugin_quay_log and
change its name to something like "Show oc-mirror log path (plugin/quay)" (and
optionally rename the earlier task at the other occurrence to "Show oc-mirror
log path (main)" or similar) so play recaps and log filtering clearly identify
which mirror stage is reported.
- Around line 143-181: Extract the duplicated oc-mirror block/rescue into a
reusable task file (e.g., playbooks/tasks/_run_oc_mirror.yaml) that accepts
parameters for the command args, log fact name (e.g., _oc_mirror_plugin_log),
the registered result name (e.g., r_plugin_mirror), human label (used in
messages like "oc-mirror for plugin {{ plugin_name }} failed"), and retry/delay
vars; implement the same shell invocation, retries, register/until logic and the
rescue steps (tail and fail) inside that task file using those variables; then
replace the in-place block in deploy_plugin.yaml (the task named "Run oc-mirror
for plugin {{ plugin_name }}" and the rescue task "oc-mirror failed") with an
include_tasks/import_tasks call to _run_oc_mirror.yaml passing the specific vars
(workingDir, plugin_name, pullSecretPath, _oc_mirror_plugin_log, r_plugin_mirror
name if needed, ocMirror* retry/delay values, quayHostname,
ocMirrorParallelImages, ocMirrorParallelLayers, ocMirrorRetryTimes,
ocMirrorRetryDelay, ocMirrorLogLevel) so all five call-sites can reuse the same
logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 21b625e8-3219-4508-a019-46a0e53ed4a8
📒 Files selected for processing (6)
defaults/oc_mirror.yamloperators/quay-operator/quay_disconnected.yamlplaybooks/common/load-vars.yamlplaybooks/tasks/deploy_plugin.yamlplaybooks/tasks/mirror_cache.yamlplaybooks/tasks/mirror_registry.yaml
✅ Files skipped from review due to trivial changes (2)
- playbooks/common/load-vars.yaml
- defaults/oc_mirror.yaml
eurijon
force-pushed
the
oc-mirror-tee-grep-error
branch
from
f321df8 to
5e1ccca
Compare
Contributor
|
/hold |
eurijon
changed the title
eurijon
force-pushed
the
oc-mirror-tee-grep-error
branch
from
5e1ccca to
f58b3f0
Compare
|
Tarball created: |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@playbooks/tasks/deploy_plugin.yaml`:
- Around line 159-173: The oc-mirror invocation in the deploy_plugin.yaml task
currently always performs a real mirror; modify the task that runs "{{
workingDir }}/bin/oc-mirror" so it respects the mirror_dry_run flag the same way
mirror_registry.yaml does (see the handling around mirror_registry.yaml:67 and
79-80). Concretely, update the ansible.builtin.shell command that runs the
oc-mirror (the block producing _oc_mirror_plugin_log) to conditionally include
the dry-run behavior based on the mirror_dry_run variable (e.g., change the
command or its destination args when mirror_dry_run is true) so plugin mirroring
uses the same retry/dry-run semantics as the mirror_registry tasks. Ensure you
apply the same change to the other oc-mirror invocation referenced (lines
~313-329).
- Around line 178-188: The rescue log-tail tasks "Read oc-mirror log tail" and
the equivalent in the other rescue block should be made tolerant to missing
logs: add failed_when: false to the ansible.builtin.command task that runs tail
so the rescue block continues even if the file is missing, and when referencing
_oc_mirror_plugin_tail.stdout in the fail message use a default fallback like {{
_oc_mirror_plugin_tail.stdout | default('Unable to read oc-mirror log tail.') }}
so the failure message always contains a useful string; update the task that
registers _oc_mirror_plugin_tail and the oc-mirror failed fail task to use these
changes (refer to variable names _oc_mirror_plugin_tail, plugin_name,
r_plugin_mirror.attempts and task name "oc-mirror failed").
In `@playbooks/tasks/mirror_cache.yaml`:
- Around line 53-67: The cache mirroring task always runs a real oc-mirror;
change it to honor mirror_dry_run by branching the task (or templating the
command) so that when mirror_dry_run is true you run oc-mirror with the
--dry-run flag (or echo the equivalent command) and disable retries/delay, and
when false run the existing real mirror command; update the shell invocation in
playbooks/tasks/mirror_cache.yaml (the ansible.builtin.shell task that invokes
{{ workingDir }}/bin/oc-mirror with docker://{{ dc_cache_address }}) to
conditionally append --dry-run when mirror_dry_run is set and ensure retries:
and delay: are skipped or set to 0 for the dry-run path so the cache address is
not mutated.
- Around line 72-82: The rescue block's "Read oc-mirror log tail" task can fail
if the log is missing, which aborts the rescue; update the "Read oc-mirror log
tail" ansible task to not fail the play when tail errors by adding failed_when:
false (keeping changed_when: false), and then make the "oc-mirror failed" fail
task use a safe/default value when referencing _oc_mirror_cache_tail (e.g., use
the stdout or stderr or a default message including the log path) so the curated
error message and full log path are always reported even if tail could not read
the file.
In `@playbooks/tasks/mirror_registry.yaml`:
- Around line 85-95: Make the "Read oc-mirror log tail" task non-fatal by adding
failed_when: false to that task (so the tail command cannot abort the rescue
flow), and update the "oc-mirror failed" fail message to reference
_oc_mirror_tail.stdout with a default filter (e.g. _oc_mirror_tail.stdout |
default('')) so missing/unreadable log files won't cause
undefined-variable/template errors when rendering the failure message that
includes r_oc_mirror.attempts.
🪄 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: Pro
Run ID: ec21f4a5-7976-43e6-ad70-eee2dd76d88f
📒 Files selected for processing (6)
defaults/oc_mirror.yamloperators/quay-operator/quay_disconnected.yamlplaybooks/common/load-vars.yamlplaybooks/tasks/deploy_plugin.yamlplaybooks/tasks/mirror_cache.yamlplaybooks/tasks/mirror_registry.yaml
✅ Files skipped from review due to trivial changes (2)
- playbooks/common/load-vars.yaml
- defaults/oc_mirror.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- operators/quay-operator/quay_disconnected.yaml
eurijon
force-pushed
the
oc-mirror-tee-grep-error
branch
from
f58b3f0 to
5735d6d
Compare
|
Tarball created: |
eurijon
changed the title
eurijon
force-pushed
the
oc-mirror-tee-grep-error
branch
from
5735d6d to
cf1754d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
playbooks/tasks/deploy_plugin.yaml (1)
127-358: LGTM overall.Both oc-mirror invocations correctly:
- gate log-dir creation + log-path
set_facton identical conditions as the block,- redirect stdout/stderr to the computed log,
- drive retries/delay via the new
ocMirror*defaults,- use the resilient rescue pattern (
argvtail,failed_when: false,| default(...)).Optional nit: the two
Show oc-mirror log pathdebug tasks (Lines 144-146 and 300-302) share the same name. Disambiguating them (e.g.,Show oc-mirror plugin log path/Show oc-mirror plugin Quay log path) makes the terminal output easier to follow when both run.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playbooks/tasks/deploy_plugin.yaml` around lines 127 - 358, The two ansible.builtin.debug tasks both named "Show oc-mirror log path" should be renamed to unique, descriptive names to disambiguate output: change the debug task that prints "{{ _oc_mirror_plugin_log }}" to something like "Show oc-mirror plugin log path" and change the debug task that prints "{{ _oc_mirror_plugin_quay_log }}" to "Show oc-mirror plugin Quay log path"; locate the tasks by their debug module and the variables _oc_mirror_plugin_log and _oc_mirror_plugin_quay_log and update their name: fields only.bootstrap.sh (1)
251-253: Suggested rerun command bakes in this run's timestamped log path.
${log}at this point resolves to${workingDir}/logs/<DSTAMP>from this invocation. The printed hint therefore tells the user to rerun later with that specific (now‑old) log file asANSIBLE_LOG_PATH, which will append the new run's output to a stale timestamped file and defeat the per‑run rotation done at Line 131. Consider either omittingANSIBLE_LOG_PATH=${log}from the hint or using a generic placeholder.💡 Suggested fix
- echo -e "\\033[31m WARNING! \033[0m Discovery hosts has failed, please check config and rerun: ANSIBLE_LOG_PATH=${log} ansible-playbook -e @$global_vars -e @$certs_vars -e @$cloud_infra_vars playbooks/07-configure-discovery.yaml" + echo -e "\\033[31m WARNING! \033[0m Discovery hosts has failed, please check config and rerun: ansible-playbook -e @$global_vars -e @$certs_vars -e @$cloud_infra_vars playbooks/07-configure-discovery.yaml"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bootstrap.sh` around lines 251 - 253, The failure message currently embeds the concrete ANSIBLE_LOG_PATH=${log} (a timestamped path) which misleads rerun advice; update the echo in the error branch of the ansible-playbook invocation for playbooks/07-configure-discovery.yaml to either omit ANSIBLE_LOG_PATH=${log} or replace it with a generic placeholder (e.g. ANSIBLE_LOG_PATH=<log_path> or ANSIBLE_LOG_PATH=/path/to/log) so the suggested rerun command does not point to the stale timestamped ${log} variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sync.sh`:
- Around line 67-68: The current sequence sets ANSIBLE_LOG_PATH=${log} for
structured Ansible logs while still piping validations.sh stdout into tee -a
${log}, which mixes formats; to fix, decide which logging format you want and
make the commands consistent: either remove ANSIBLE_LOG_PATH and pipe the
ansible-playbook invocation through tee (e.g., run ansible-playbook ... 2>&1 |
tee -a ${log}) so playbook output and validations.sh use the same tee-captured
human-readable log, or keep ANSIBLE_LOG_PATH and stop tee-appending raw stdout
(remove tee from validations.sh piping) so everything is written only via
Ansible’s structured logger; update the lines referencing ANSIBLE_LOG_PATH,
ansible-playbook and the validations.sh pipe accordingly to implement the chosen
approach.
---
Nitpick comments:
In `@bootstrap.sh`:
- Around line 251-253: The failure message currently embeds the concrete
ANSIBLE_LOG_PATH=${log} (a timestamped path) which misleads rerun advice; update
the echo in the error branch of the ansible-playbook invocation for
playbooks/07-configure-discovery.yaml to either omit ANSIBLE_LOG_PATH=${log} or
replace it with a generic placeholder (e.g. ANSIBLE_LOG_PATH=<log_path> or
ANSIBLE_LOG_PATH=/path/to/log) so the suggested rerun command does not point to
the stale timestamped ${log} variable.
In `@playbooks/tasks/deploy_plugin.yaml`:
- Around line 127-358: The two ansible.builtin.debug tasks both named "Show
oc-mirror log path" should be renamed to unique, descriptive names to
disambiguate output: change the debug task that prints "{{ _oc_mirror_plugin_log
}}" to something like "Show oc-mirror plugin log path" and change the debug task
that prints "{{ _oc_mirror_plugin_quay_log }}" to "Show oc-mirror plugin Quay
log path"; locate the tasks by their debug module and the variables
_oc_mirror_plugin_log and _oc_mirror_plugin_quay_log and update their name:
fields only.
🪄 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: Pro
Run ID: 8d87fd3f-0958-42b0-812e-28068ceed9f2
📒 Files selected for processing (8)
bootstrap.shdefaults/oc_mirror.yamloperators/quay-operator/quay_disconnected.yamlplaybooks/common/load-vars.yamlplaybooks/tasks/deploy_plugin.yamlplaybooks/tasks/mirror_cache.yamlplaybooks/tasks/mirror_registry.yamlsync.sh
✅ Files skipped from review due to trivial changes (1)
- defaults/oc_mirror.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- playbooks/common/load-vars.yaml
eurijon
force-pushed
the
oc-mirror-tee-grep-error
branch
from
cf1754d to
c34d7d2
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
playbooks/tasks/mirror_registry.yaml (1)
64-82: Preserve all retry output in the log file.Line 78 uses
>, so every Ansible retry truncates the same_oc_mirror_log; afterocMirrorAnsibleRetriesattempts, the “Full log” only contains the final attempt. If the intent is persistent retry visibility, append instead.Suggested change
- > {{ _oc_mirror_log }} 2>&1 + >> {{ _oc_mirror_log }} 2>&1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playbooks/tasks/mirror_registry.yaml` around lines 64 - 82, The oc-mirror shell task (name: "Run oc-mirror to mirror registry") currently redirects output with '>' to _oc_mirror_log which truncates the log on each retry; change the redirection to append (use '>>' for stdout and '2>&1' so stderr is appended too) so every retry's output is preserved in {{ _oc_mirror_log }}; update the shell invocation that builds the oc-mirror command and keep register: r_oc_mirror/until logic unchanged.operators/quay-operator/quay_disconnected.yaml (1)
45-64: Avoid truncating earlier retry logs.Line 60 redirects with
>, so each Ansible retry overwrites the previous attempt’soc-mirroroutput. Use append redirection if the full log should explain all attempts.Suggested change
- > {{ _oc_mirror_quay_log }} 2>&1 + >> {{ _oc_mirror_quay_log }} 2>&1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/quay-operator/quay_disconnected.yaml` around lines 45 - 64, The oc-mirror Ansible task "Run oc-mirror to internal Quay" currently redirects output with a single ">" which overwrites {{ _oc_mirror_quay_log }} on each retry; change the shell redirection to append (use ">>" with "2>&1") so each retry's stdout/stderr is preserved in the same file. Update the command in the task that registers r_oc_mirror_quay to use append redirection for {{ _oc_mirror_quay_log }}.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@operators/quay-operator/quay_disconnected.yaml`:
- Around line 78-83: Update the rescue fail message to prefer the tail task's
stderr, then stdout, then a static fallback, and guard the attempts value with a
default; specifically, in the rescue handler that calls ansible.builtin.fail
(referencing r_oc_mirror_quay.attempts and _oc_mirror_quay_tail.stdout), change
the template to use _oc_mirror_quay_tail.stderr |
default(_oc_mirror_quay_tail.stdout | default('Unable to read oc-mirror log
tail.')) and render the attempts as {{ r_oc_mirror_quay.attempts |
default('unknown') }} so missing attempts don't break the message.
In `@playbooks/tasks/mirror_registry.yaml`:
- Around line 96-101: The fail message uses `_oc_mirror_tail.stdout` which can
be an empty string, so change the template in the fail task to treat empty/falsy
values as missing by using Jinja2's default filter with the boolean `true`;
first fall back to `_oc_mirror_tail.stderr` (to surface command errors) and then
to a hardcoded message like "Unable to read oc-mirror log tail." Update the
`msg` expression in the oc-mirror failed task (the block referencing
`r_oc_mirror.attempts` and `_oc_mirror_tail.stdout`) to apply `default(...,
true)` so empty stdout is replaced by stderr and then by the final fallback.
---
Nitpick comments:
In `@operators/quay-operator/quay_disconnected.yaml`:
- Around line 45-64: The oc-mirror Ansible task "Run oc-mirror to internal Quay"
currently redirects output with a single ">" which overwrites {{
_oc_mirror_quay_log }} on each retry; change the shell redirection to append
(use ">>" with "2>&1") so each retry's stdout/stderr is preserved in the same
file. Update the command in the task that registers r_oc_mirror_quay to use
append redirection for {{ _oc_mirror_quay_log }}.
In `@playbooks/tasks/mirror_registry.yaml`:
- Around line 64-82: The oc-mirror shell task (name: "Run oc-mirror to mirror
registry") currently redirects output with '>' to _oc_mirror_log which truncates
the log on each retry; change the redirection to append (use '>>' for stdout and
'2>&1' so stderr is appended too) so every retry's output is preserved in {{
_oc_mirror_log }}; update the shell invocation that builds the oc-mirror command
and keep register: r_oc_mirror/until logic unchanged.
🪄 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: Pro
Run ID: 60ced428-b32e-4a1b-bf24-2e0a93391be8
📒 Files selected for processing (8)
bootstrap.shdefaults/oc_mirror.yamloperators/quay-operator/quay_disconnected.yamlplaybooks/common/load-vars.yamlplaybooks/tasks/deploy_plugin.yamlplaybooks/tasks/mirror_cache.yamlplaybooks/tasks/mirror_registry.yamlsync.sh
✅ Files skipped from review due to trivial changes (1)
- defaults/oc_mirror.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- playbooks/common/load-vars.yaml
- playbooks/tasks/deploy_plugin.yaml
- sync.sh
- playbooks/tasks/mirror_cache.yaml
- Wrap all oc-mirror tasks in block/rescue to surface errors on failure - Replace tee by ANSIBLE_LOG_PATH in ansible-playbook calls to avoid buffer issues - Show last 10 lines of oc-mirror log when task fails - Redirect output to log file instead of piping through grep - Create retry/parallelism paremeters in defaults/oc_mirror.yaml
eurijon
force-pushed
the
oc-mirror-tee-grep-error
branch
from
c34d7d2 to
52186ca
Compare
|
Tarball created: |
Collaborator
Author
added a hold label. feel free to remove! |
Contributor
|
/hold cancel |
rporres
reviewed
Apr 21, 2026
| --- | ||
| # Retry and parallelism settings for oc-mirror tasks. | ||
| # These are internal defaults and are not meant to be overridden by the user. | ||
| ocMirrorParallelImages: 10 |
Contributor
There was a problem hiding this comment.
we should add schema for these new properties
| ocMirrorParallelImages: 10 | ||
| ocMirrorParallelLayers: 10 | ||
| ocMirrorRetryTimes: 10 | ||
| ocMirrorRetryDelay: 30s |
Contributor
There was a problem hiding this comment.
this value used to be zero. Why has it changed?
| --retry-delay {{ ocMirrorRetryDelay }} \ | ||
| docker://{{ dc_cache_address }} \ | ||
| > {{ _oc_mirror_cache_log }} 2>&1 | ||
| retries: "{{ ocMirrorAnsibleRetries }}" |
Contributor
There was a problem hiding this comment.
This used to be 5 now is 10. Is that intended?
| - r_plugin_mirror is success | ||
| tags: mirror | ||
|
|
||
| - name: Show oc-mirror log path |
Contributor
There was a problem hiding this comment.
This name is duplicated for this task. How about
Suggested change
| - name: Show oc-mirror log path | |
| - name: Show oc-mirror plugin Quay Enterprise log path |
| --dest-tls-verify=false \ | ||
| --src-tls-verify=false \ | ||
| --parallel-images {{ ocMirrorParallelImages }} \ | ||
| --parallel-layers {{ 1 if quayBackend == 'LocalStorage' else ocMirrorParallelLayers }} \ |
Contributor
There was a problem hiding this comment.
nit: since we're adding variables and avoiding hardcoded values all over the place, we may want to do it too here defining something like ocMirrorParallelLayersLocalStorage
Suggested change
| --parallel-layers {{ 1 if quayBackend == 'LocalStorage' else ocMirrorParallelLayers }} \ | |
| --parallel-layers {{ ocMirrorParallelLayersLocalStorage if quayBackend == 'LocalStorage' else ocMirrorParallelLayers }} \ |
but YMMV 😄
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
3 participants
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.
Output aspect of https://github.com/gori-project/GoRI/issues/869
oc-mirrortasks in block/rescue to surface errors on failureteebyANSIBLE_LOG_PATHin ansible-playbook calls to avoid buffer issues and delayed logsoc-mirrorlog when task failsdefaults/oc_mirror.yamlPlease note this change modifies the
bootstrap.shlog file format, asansible-playbookoutputs are now logged in Ansible format (with timestamps, module info, etc.) :MGMT-23996
Summary by CodeRabbit
Improvements
Chores