MGMT-22946: Run mirror in two different phases#216
Conversation
github-actions
Bot
added
deployment
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe pull request implements a split approach for OpenShift mirroring, separating core installation image mirroring from post-installation operator mirroring. New playbooks handle asynchronous operator mirroring, MachineConfigPool state management, and mirror resource configuration. Existing mirror tasks are refactored to support selective execution via tags. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
playbooks/tasks/mirror_cache.yaml (1)
32-47:⚠️ Potential issue | 🟡 MinorPotential issue:
oc-mirrorpermissions may not be set when runningmirror-cachein isolation.The
oc-mirrorpermission change was moved tomirror_registry.yaml, which only runs with themirror-registrytag. If someone runs only--tags mirror-cachewithout first runningmirror-registry, theoc-mirrorbinary may not have execute permissions.Consider either:
- Adding the permission task to this file as well (idempotent, safe to duplicate)
- Documenting that
mirror-registrymust run beforemirror-cache🤖 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 32 - 47, The Start oc-mirror process task may fail if the oc-mirror binary lacks execute permissions when mirror-cache is run standalone; add an idempotent permission task before the shell task that ensures the binary at {{ workingDir }}/bin/oc-mirror is executable (e.g., an ansible.builtin.file or ansible.builtin.command to chmod +x) so the task named "Start oc-mirror process" always has the proper permissions even when mirror-registry is not run, or alternatively add a clear comment/documentation requiring the mirror-registry tag to be executed first.
🧹 Nitpick comments (3)
playbooks/wait_operator_mirror.yaml (1)
28-31: Missing tag application on include_tasks.The task include is missing an
apply: tags:block, which is inconsistent with similar includes in other playbooks (e.g.,05-operators.yaml). This may affect tag-based execution filtering.♻️ Suggested fix to add tags
- name: Wait for the async task mirror_registry_post when: disconnected | default(true) | bool ansible.builtin.include_tasks: file: tasks/wait_operator_mirror.yaml + apply: + tags: always + tags: always🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playbooks/wait_operator_mirror.yaml` around lines 28 - 31, The include_tasks call for the "Wait for the async task mirror_registry_post" task is missing the apply: tags: block; add an apply: tags: line to the ansible.builtin.include_tasks invocation (the include_tasks that references file: tasks/wait_operator_mirror.yaml) to match the tagging pattern used elsewhere (e.g., 05-operators.yaml) so the include honors tag-based execution filtering.playbooks/tasks/mirror_registry_post.yaml (1)
37-40: Job ID file write could fail silently if async task registration fails.If the shell command fails to start (e.g., binary not found),
r_oc_mirror_post.ansible_job_idmay be undefined, causing this task to fail. Consider adding a check.🛡️ Suggested defensive check
- name: Save async job ID for later reference + when: r_oc_mirror_post.ansible_job_id is defined ansible.builtin.copy: content: "{{ r_oc_mirror_post.ansible_job_id }}" dest: "{{ workingDir }}/logs/operator-mirror.async_job_id" + +- name: Fail if async task did not start + when: r_oc_mirror_post.ansible_job_id is not defined + ansible.builtin.fail: + msg: "Failed to start async mirror task. Check that oc-mirror binary exists and has execute permissions."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playbooks/tasks/mirror_registry_post.yaml` around lines 37 - 40, The ansible.builtin.copy task writing "{{ r_oc_mirror_post.ansible_job_id }}" can fail if the async registration never happened; update the task that writes operator-mirror.async_job_id to first check that r_oc_mirror_post and r_oc_mirror_post.ansible_job_id exist (e.g., guard with a when: condition checking r_oc_mirror_post is defined and r_oc_mirror_post.ansible_job_id is defined and not empty) and optionally add a fallback/conditional path (or a debug/error task) when the job id is missing so the play does not fail silently; change the task that references r_oc_mirror_post and the dest "{{ workingDir }}/logs/operator-mirror.async_job_id" accordingly.playbooks/tasks/configure_mirror_post.yaml (1)
49-51: Potential null reference if MCP status fields are not yet populated.The
untilconditions access nested status fields that may not exist immediately after unpausing. Consider adding defensive checks.🛡️ Suggested defensive conditions
until: - - mcp_status.resources[0].status.updatedMachineCount == mcp_status.resources[0].status.machineCount - - mcp_status.resources[0].status.degradedMachineCount == 0 + - mcp_status.resources | length > 0 + - mcp_status.resources[0].status is defined + - mcp_status.resources[0].status.updatedMachineCount | default(0) == mcp_status.resources[0].status.machineCount | default(-1) + - (mcp_status.resources[0].status.degradedMachineCount | default(0)) == 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playbooks/tasks/configure_mirror_post.yaml` around lines 49 - 51, The until block currently dereferences mcp_status.resources[0].status.updatedMachineCount and other nested fields directly; change the conditions to defensively check that mcp_status.resources exists and has at least one element and that resources[0].status is defined, and use default() (or compare against default zeros) for updatedMachineCount, machineCount and degradedMachineCount so the until expression won't fail when fields are missing; update the until expression that references mcp_status.resources[0].status.updatedMachineCount, mcp_status.resources[0].status.machineCount and mcp_status.resources[0].status.degradedMachineCount to include these existence checks and default fallbacks.
🤖 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/02-mirror.yaml`:
- Around line 61-68: The task name "Include tasks for core OpenShift
installation mirroring" is duplicated; change one of the task names to a unique,
descriptive string (for example "Include tasks for initial OpenShift
installation mirroring" or "Include tasks for additional OpenShift mirroring")
so Ansible output/logs are unambiguous—locate the include_tasks entries that
reference tasks/mirror_registry.yaml and the duplicate task with the exact name
"Include tasks for core OpenShift installation mirroring" and update one of
those name fields only.
In `@playbooks/tasks/configure_mirror_post.yaml`:
- Around line 20-23: The loop contains a hardcoded catalog filename
`cs-redhat-operator-index-v4-20.yaml` which breaks for other OpenShift releases;
update the loop in configure_mirror_post.yaml to generate the catalog filename
dynamically from the playbook's OpenShift version variable (e.g., use the
existing openshift_version/ocp_version), building the string
"cs-redhat-operator-index-v4-<major-minor>.yaml" at runtime (extract major.minor
from the version variable if needed) and replace the static entry in the loop so
the correct oc-mirror-produced file is selected for each OpenShift version.
- Around line 41-53: The task "Wait for MachineConfigPools to finish updating
(In-Place)" only queries the single named resource 'master'
(kubernetes.core.k8s_info name: master) but is named/plural and will miss other
pools; change the task to fetch all MachineConfigPools (remove the name
parameter so the module returns mcp_status.resources array for all MCPs) and
update the until checks to iterate over each item (ensure for every resource in
mcp_status.resources that status.updatedMachineCount == status.machineCount and
status.degradedMachineCount == 0) or, if the intent is to only wait on master,
rename the task to explicitly reference 'master' and add a comment documenting
the scope.
In `@playbooks/tasks/wait_operator_mirror.yaml`:
- Around line 69-74: The cleanup task's when condition references
async_job_id_file_stat.stat.exists which can be undefined if the stat task was
skipped; update the when clause for the "Clean up async job ID file on success"
task to first check that async_job_id_file_stat is defined and then that
async_job_id_file_stat.stat.exists (e.g. add "async_job_id_file_stat is defined"
and optionally "async_job_id_file_stat.stat.exists is defined" before checking
its truthiness), keeping the existing checks for async_job_id and
operator_mirror_result.rc == 0 so the task only runs when the variable exists
and indicates a file is present.
- Around line 16-25: The task that sets async_job_id from file ("Set async job
ID from file") can assign an empty string which bypasses the subsequent fail
task; update the when condition and/or set_fact so empty values are rejected:
ensure the file content is decoded and trimmed and checked for non-zero length
(e.g., replace the current when conditions on async_job_id_file_content and
async_job_id with a check like (async_job_id_file_content.content | b64decode |
trim) is defined and (async_job_id_file_content.content | b64decode | trim) |
length > 0) so async_job_id is only set when non-empty, and update the "Get
async job ID from user input or fail" task to also fail when async_job_id is an
empty string (use a length > 0 check against async_job_id) before calling
async_status.
In `@templates/imagesetconfiguration-core.yaml.j2`:
- Around line 8-10: The template assumes v.version has three dot-separated
segments and directly uses v.version.split('.')[2], which can raise IndexError
for strings like "4.20"; update the Jinja logic around the openshift_versions
loop to validate/safely parse v.version: compute parts = v.version.split('.')
and set channel_name using the first two parts only if parts|length >= 2
(fallback to v.version or join with a default like '0' for missing segments),
and set patch to int(parts[2]) only when parts|length >= 3 (fallback to 0 or
None otherwise); change references to channel_name and patch accordingly so the
template never assumes a third segment.
- Around line 29-31: Remove the two Quay builder images currently listed under
blockedImages (registry.redhat.io/quay/quay-builder-qemu-rhcos-rhel8 and
registry.redhat.io/quay/quay-builder-rhel8) and instead add them under
mirror.additionalImages with the exact tags your Quay deployment requires (e.g.,
registry.redhat.io/quay/quay-builder-qemu-rhcos-rhel8:<tag> and
registry.redhat.io/quay/quay-builder-rhel8:<tag>) so they will be mirrored
rather than blocked.
In `@templates/imagesetconfiguration-post.yaml.j2`:
- Around line 11-18: The template is placing operator.csvNames (CSV names) into
packages[].name, which must be package names (operator.name); change the logic
in imagesetconfiguration-post.yaml.j2 (and the other two affected templates) to
stop appending additional_operator_names into packages[].name: keep
packages[].name as operator.name only, and if csv-level selection is required
iterate operator.csvNames into packages[].bundles[].name instead (use
operator.csvNames -> packages[].bundles[].name), otherwise remove the csvNames
branch entirely; update the variables referenced (additional_operator_names,
operator.csvNames, operator.name, packages[].name, packages[].bundles[].name)
accordingly.
---
Outside diff comments:
In `@playbooks/tasks/mirror_cache.yaml`:
- Around line 32-47: The Start oc-mirror process task may fail if the oc-mirror
binary lacks execute permissions when mirror-cache is run standalone; add an
idempotent permission task before the shell task that ensures the binary at {{
workingDir }}/bin/oc-mirror is executable (e.g., an ansible.builtin.file or
ansible.builtin.command to chmod +x) so the task named "Start oc-mirror process"
always has the proper permissions even when mirror-registry is not run, or
alternatively add a clear comment/documentation requiring the mirror-registry
tag to be executed first.
---
Nitpick comments:
In `@playbooks/tasks/configure_mirror_post.yaml`:
- Around line 49-51: The until block currently dereferences
mcp_status.resources[0].status.updatedMachineCount and other nested fields
directly; change the conditions to defensively check that mcp_status.resources
exists and has at least one element and that resources[0].status is defined, and
use default() (or compare against default zeros) for updatedMachineCount,
machineCount and degradedMachineCount so the until expression won't fail when
fields are missing; update the until expression that references
mcp_status.resources[0].status.updatedMachineCount,
mcp_status.resources[0].status.machineCount and
mcp_status.resources[0].status.degradedMachineCount to include these existence
checks and default fallbacks.
In `@playbooks/tasks/mirror_registry_post.yaml`:
- Around line 37-40: The ansible.builtin.copy task writing "{{
r_oc_mirror_post.ansible_job_id }}" can fail if the async registration never
happened; update the task that writes operator-mirror.async_job_id to first
check that r_oc_mirror_post and r_oc_mirror_post.ansible_job_id exist (e.g.,
guard with a when: condition checking r_oc_mirror_post is defined and
r_oc_mirror_post.ansible_job_id is defined and not empty) and optionally add a
fallback/conditional path (or a debug/error task) when the job id is missing so
the play does not fail silently; change the task that references
r_oc_mirror_post and the dest "{{ workingDir
}}/logs/operator-mirror.async_job_id" accordingly.
In `@playbooks/wait_operator_mirror.yaml`:
- Around line 28-31: The include_tasks call for the "Wait for the async task
mirror_registry_post" task is missing the apply: tags: block; add an apply:
tags: line to the ansible.builtin.include_tasks invocation (the include_tasks
that references file: tasks/wait_operator_mirror.yaml) to match the tagging
pattern used elsewhere (e.g., 05-operators.yaml) so the include honors tag-based
execution filtering.
🪄 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: ab5110d8-c204-4100-b6c0-695a660d7165
📒 Files selected for processing (12)
playbooks/02-mirror.yamlplaybooks/05-operators.yamlplaybooks/tasks/configure_mirror_post.yamlplaybooks/tasks/configure_ocp_abi.yamlplaybooks/tasks/mirror_cache.yamlplaybooks/tasks/mirror_registry.yamlplaybooks/tasks/mirror_registry_core.yamlplaybooks/tasks/mirror_registry_post.yamlplaybooks/tasks/wait_operator_mirror.yamlplaybooks/wait_operator_mirror.yamltemplates/imagesetconfiguration-core.yaml.j2templates/imagesetconfiguration-post.yaml.j2
💤 Files with no reviewable changes (1)
- playbooks/tasks/configure_ocp_abi.yaml
| # Mirror registry setup | ||
| # This must run first as it sets up the registry and basic infrastructure | ||
| - name: Include tasks for core OpenShift installation mirroring | ||
| ansible.builtin.include_tasks: | ||
| file: tasks/mirror_registry.yaml | ||
| apply: | ||
| tags: mirror-registry | ||
| tags: mirror-registry |
There was a problem hiding this comment.
Duplicate task name with line 71.
Both line 63 and line 71 have the same task name "Include tasks for core OpenShift installation mirroring". This will cause confusion in Ansible output and logs.
✏️ Suggested fix
# Mirror registry setup
# This must run first as it sets up the registry and basic infrastructure
- - name: Include tasks for core OpenShift installation mirroring
+ - name: Include tasks for mirror registry setup
ansible.builtin.include_tasks:
file: tasks/mirror_registry.yaml📝 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.
| # Mirror registry setup | |
| # This must run first as it sets up the registry and basic infrastructure | |
| - name: Include tasks for core OpenShift installation mirroring | |
| ansible.builtin.include_tasks: | |
| file: tasks/mirror_registry.yaml | |
| apply: | |
| tags: mirror-registry | |
| tags: mirror-registry | |
| # Mirror registry setup | |
| # This must run first as it sets up the registry and basic infrastructure | |
| - name: Include tasks for mirror registry setup | |
| ansible.builtin.include_tasks: | |
| file: tasks/mirror_registry.yaml | |
| apply: | |
| tags: mirror-registry | |
| tags: mirror-registry |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@playbooks/02-mirror.yaml` around lines 61 - 68, The task name "Include tasks
for core OpenShift installation mirroring" is duplicated; change one of the task
names to a unique, descriptive string (for example "Include tasks for initial
OpenShift installation mirroring" or "Include tasks for additional OpenShift
mirroring") so Ansible output/logs are unambiguous—locate the include_tasks
entries that reference tasks/mirror_registry.yaml and the duplicate task with
the exact name "Include tasks for core OpenShift installation mirroring" and
update one of those name fields only.
| loop: | ||
| - idms-oc-mirror.yaml | ||
| - itms-oc-mirror.yaml | ||
| - cs-redhat-operator-index-v4-20.yaml |
There was a problem hiding this comment.
Hardcoded OpenShift version in catalog source filename.
The filename cs-redhat-operator-index-v4-20.yaml is hardcoded, which will fail for other OpenShift versions. The oc-mirror tool generates catalog source files with version-specific names.
Consider dynamically determining the filename based on the OpenShift version being deployed.
🐛 Suggested fix using dynamic filename
+- name: Find catalog source manifest
+ ansible.builtin.find:
+ paths: "{{ workingDir }}/config/oc-mirror-workspace-post/working-dir/cluster-resources"
+ patterns: "cs-redhat-operator-index-*.yaml"
+ register: cs_files
+
+- name: Set catalog source filename
+ ansible.builtin.set_fact:
+ cs_filename: "{{ cs_files.files[0].path | basename }}"
+ when: cs_files.files | length > 0
+
- name: Apply mirroring and catalog resources
environment:
KUBECONFIG: "{{ workingDir }}/ocp-cluster/auth/kubeconfig"
kubernetes.core.k8s:
state: present
src: "{{ workingDir }}/config/oc-mirror-workspace-post/working-dir/cluster-resources/{{ item }}"
loop:
- idms-oc-mirror.yaml
- itms-oc-mirror.yaml
- - cs-redhat-operator-index-v4-20.yaml
+ - "{{ cs_filename }}"📝 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.
| loop: | |
| - idms-oc-mirror.yaml | |
| - itms-oc-mirror.yaml | |
| - cs-redhat-operator-index-v4-20.yaml | |
| - name: Find catalog source manifest | |
| ansible.builtin.find: | |
| paths: "{{ workingDir }}/config/oc-mirror-workspace-post/working-dir/cluster-resources" | |
| patterns: "cs-redhat-operator-index-*.yaml" | |
| register: cs_files | |
| - name: Set catalog source filename | |
| ansible.builtin.set_fact: | |
| cs_filename: "{{ cs_files.files[0].path | basename }}" | |
| when: cs_files.files | length > 0 | |
| - name: Apply mirroring and catalog resources | |
| environment: | |
| KUBECONFIG: "{{ workingDir }}/ocp-cluster/auth/kubeconfig" | |
| kubernetes.core.k8s: | |
| state: present | |
| src: "{{ workingDir }}/config/oc-mirror-workspace-post/working-dir/cluster-resources/{{ item }}" | |
| loop: | |
| - idms-oc-mirror.yaml | |
| - itms-oc-mirror.yaml | |
| - "{{ cs_filename }}" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@playbooks/tasks/configure_mirror_post.yaml` around lines 20 - 23, The loop
contains a hardcoded catalog filename `cs-redhat-operator-index-v4-20.yaml`
which breaks for other OpenShift releases; update the loop in
configure_mirror_post.yaml to generate the catalog filename dynamically from the
playbook's OpenShift version variable (e.g., use the existing
openshift_version/ocp_version), building the string
"cs-redhat-operator-index-v4-<major-minor>.yaml" at runtime (extract major.minor
from the version variable if needed) and replace the static entry in the loop so
the correct oc-mirror-produced file is selected for each OpenShift version.
| - name: Wait for MachineConfigPools to finish updating (In-Place) | ||
| environment: | ||
| KUBECONFIG: "{{ workingDir }}/ocp-cluster/auth/kubeconfig" | ||
| kubernetes.core.k8s_info: | ||
| api_version: machineconfiguration.openshift.io/v1 | ||
| kind: MachineConfigPool | ||
| name: master | ||
| register: mcp_status | ||
| until: | ||
| - mcp_status.resources[0].status.updatedMachineCount == mcp_status.resources[0].status.machineCount | ||
| - mcp_status.resources[0].status.degradedMachineCount == 0 | ||
| retries: 60 | ||
| delay: 30 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if worker MachineConfigPool handling exists elsewhere
rg -n "MachineConfigPool" --type yaml -C 3 playbooks/Repository: rh-ecosystem-edge/enclave
Length of output: 2768
🏁 Script executed:
cat -n playbooks/tasks/configure_mirror_post.yamlRepository: rh-ecosystem-edge/enclave
Length of output: 2099
🏁 Script executed:
rg -n "worker" --type yaml playbooks/Repository: rh-ecosystem-edge/enclave
Length of output: 51
🏁 Script executed:
rg -n "MachineConfigPool" --type yaml playbooks/ -B 2 -A 10Repository: rh-ecosystem-edge/enclave
Length of output: 3815
Task name references plural "MachineConfigPools" but only waits for the master pool.
The wait task only checks the master MachineConfigPool, despite its plural name. The preceding pause and unpause operations also only target master. If worker nodes exist and require configuration changes, the playbook won't wait for them to reconcile, potentially completing prematurely. This behavior should be explicitly documented or the logic updated to include all pools that were modified.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@playbooks/tasks/configure_mirror_post.yaml` around lines 41 - 53, The task
"Wait for MachineConfigPools to finish updating (In-Place)" only queries the
single named resource 'master' (kubernetes.core.k8s_info name: master) but is
named/plural and will miss other pools; change the task to fetch all
MachineConfigPools (remove the name parameter so the module returns
mcp_status.resources array for all MCPs) and update the until checks to iterate
over each item (ensure for every resource in mcp_status.resources that
status.updatedMachineCount == status.machineCount and
status.degradedMachineCount == 0) or, if the intent is to only wait on master,
rename the task to explicitly reference 'master' and add a comment documenting
the scope.
| - name: Set async job ID from file | ||
| when: | ||
| - async_job_id is not defined | ||
| - async_job_id_file_stat.stat.exists | ||
| - async_job_id_file_content.content is defined | ||
| ansible.builtin.set_fact: | ||
| async_job_id: "{{ async_job_id_file_content.content | b64decode | trim }}" | ||
|
|
||
| - name: Get async job ID from user input or fail | ||
| when: async_job_id is not defined |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "wait_operator_mirror.yaml" --exec wc -l {} +Repository: rh-ecosystem-edge/enclave
Length of output: 265
🏁 Script executed:
cat -n playbooks/tasks/wait_operator_mirror.yaml | sed -n '1,30p'Repository: rh-ecosystem-edge/enclave
Length of output: 1296
🏁 Script executed:
git log --oneline -1 playbooks/tasks/wait_operator_mirror.yaml 2>/dev/null || echo "File found"Repository: rh-ecosystem-edge/enclave
Length of output: 123
🏁 Script executed:
rg "async_status" playbooks/tasks/ -A 5 -B 2Repository: rh-ecosystem-edge/enclave
Length of output: 698
🏁 Script executed:
# Check if the suggested filter syntax is valid Ansible
# Look for similar patterns in the codebase to validate the approach
rg "length.*>.*0" playbooks/ -t yaml -A 2 -B 2 | head -30Repository: rh-ecosystem-edge/enclave
Length of output: 2994
🏁 Script executed:
# Check for other instances where async_job_id might be set or checked
rg "async_job_id" playbooks/ -t yaml -B 2 -A 2Repository: rh-ecosystem-edge/enclave
Length of output: 4927
🏁 Script executed:
# Check how 02-mirror.yaml creates the async_job_id file to understand expected content
fd -t f "02-mirror.yaml" playbooks/ --exec grep -n "async_job_id" {} + -B 3 -A 3 2>/dev/null | head -40Repository: rh-ecosystem-edge/enclave
Length of output: 51
🏁 Script executed:
# Check if async_job_id can be passed as a user input variable and its expected format
rg "e async_job_id" playbooks/ -t yaml -B 2 -A 2 | head -20Repository: rh-ecosystem-edge/enclave
Length of output: 461
Reject empty async job IDs before polling.
An empty file currently sets async_job_id to '' (defined), bypasses the fail task, and then fails later in async_status with a less actionable error. The same issue can occur if a user provides an empty value via -e async_job_id=.
Suggested fix
- name: Set async job ID from file
when:
- async_job_id is not defined
- async_job_id_file_stat.stat.exists
- async_job_id_file_content.content is defined
+ - (async_job_id_file_content.content | b64decode | trim | length) > 0
ansible.builtin.set_fact:
async_job_id: "{{ async_job_id_file_content.content | b64decode | trim }}"
- name: Get async job ID from user input or fail
- when: async_job_id is not defined
+ when: (async_job_id | default('') | trim | length) == 0
ansible.builtin.fail:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@playbooks/tasks/wait_operator_mirror.yaml` around lines 16 - 25, The task
that sets async_job_id from file ("Set async job ID from file") can assign an
empty string which bypasses the subsequent fail task; update the when condition
and/or set_fact so empty values are rejected: ensure the file content is decoded
and trimmed and checked for non-zero length (e.g., replace the current when
conditions on async_job_id_file_content and async_job_id with a check like
(async_job_id_file_content.content | b64decode | trim) is defined and
(async_job_id_file_content.content | b64decode | trim) | length > 0) so
async_job_id is only set when non-empty, and update the "Get async job ID from
user input or fail" task to also fail when async_job_id is an empty string (use
a length > 0 check against async_job_id) before calling async_status.
| - name: Clean up async job ID file on success | ||
| when: | ||
| - async_job_id is defined | ||
| - operator_mirror_result.rc == 0 | ||
| - async_job_id_file_stat.stat.exists is defined | ||
| - async_job_id_file_stat.stat.exists |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "wait_operator_mirror.yaml" -type fRepository: rh-ecosystem-edge/enclave
Length of output: 150
🏁 Script executed:
cat -n ./playbooks/tasks/wait_operator_mirror.yamlRepository: rh-ecosystem-edge/enclave
Length of output: 3252
🏁 Script executed:
cat -n ./playbooks/wait_operator_mirror.yamlRepository: rh-ecosystem-edge/enclave
Length of output: 1330
Guard cleanup when condition against undefined async_job_id_file_stat.
When async_job_id is provided directly (e.g., via -e async_job_id=<ID>), the stat task is skipped, leaving async_job_id_file_stat unregistered. The cleanup task then references async_job_id_file_stat.stat.exists in its condition, which breaks evaluation on an undefined variable.
Suggested fix
- name: Clean up async job ID file on success
when:
- async_job_id is defined
- operator_mirror_result.rc == 0
- - async_job_id_file_stat.stat.exists is defined
- - async_job_id_file_stat.stat.exists
+ - async_job_id_file_stat is defined
+ - async_job_id_file_stat.stat is defined
+ - async_job_id_file_stat.stat.exists | default(false)
ansible.builtin.file:
path: "{{ workingDir }}/logs/operator-mirror.async_job_id"
state: absent📝 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.
| - name: Clean up async job ID file on success | |
| when: | |
| - async_job_id is defined | |
| - operator_mirror_result.rc == 0 | |
| - async_job_id_file_stat.stat.exists is defined | |
| - async_job_id_file_stat.stat.exists | |
| - name: Clean up async job ID file on success | |
| when: | |
| - async_job_id is defined | |
| - operator_mirror_result.rc == 0 | |
| - async_job_id_file_stat is defined | |
| - async_job_id_file_stat.stat is defined | |
| - async_job_id_file_stat.stat.exists | default(false) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@playbooks/tasks/wait_operator_mirror.yaml` around lines 69 - 74, The cleanup
task's when condition references async_job_id_file_stat.stat.exists which can be
undefined if the stat task was skipped; update the when clause for the "Clean up
async job ID file on success" task to first check that async_job_id_file_stat is
defined and then that async_job_id_file_stat.stat.exists (e.g. add
"async_job_id_file_stat is defined" and optionally
"async_job_id_file_stat.stat.exists is defined" before checking its truthiness),
keeping the existing checks for async_job_id and operator_mirror_result.rc == 0
so the task only runs when the variable exists and indicates a file is present.
| {% for v in openshift_versions %} | ||
| {% set channel_name = 'stable-' + v.version.split('.')[:2] | join('.') %} | ||
| {% set patch = v.version.split('.')[2] | int %} |
There was a problem hiding this comment.
Version parsing assumes three-segment format without validation.
Line 10 extracts v.version.split('.')[2] which will fail with an IndexError if a version string like "4.20" (only two segments) is provided. The schema in schemas/platforms.yaml does not enforce the three-segment format.
Consider adding defensive handling or documenting the required version format:
🛡️ Option 1: Add defensive Jinja logic
{% for v in openshift_versions %}
-{% set channel_name = 'stable-' + v.version.split('.')[:2] | join('.') %}
-{% set patch = v.version.split('.')[2] | int %}
+{% set version_parts = v.version.split('.') %}
+{% set channel_name = 'stable-' + version_parts[:2] | join('.') %}
+{% set patch = (version_parts[2] | default('0')) | int %}🛡️ Option 2: Add schema validation in schemas/platforms.yaml
openshift_versions:
type: array
minItems: 1
items:
type: object
properties:
version:
type: string
pattern: "^[0-9]+\\.[0-9]+\\.[0-9]+$"
description: OpenShift version in semver format (e.g., 4.20.8)📝 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.
| {% for v in openshift_versions %} | |
| {% set channel_name = 'stable-' + v.version.split('.')[:2] | join('.') %} | |
| {% set patch = v.version.split('.')[2] | int %} | |
| {% for v in openshift_versions %} | |
| {% set version_parts = v.version.split('.') %} | |
| {% set channel_name = 'stable-' + version_parts[:2] | join('.') %} | |
| {% set patch = (version_parts[2] | default('0')) | int %} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@templates/imagesetconfiguration-core.yaml.j2` around lines 8 - 10, The
template assumes v.version has three dot-separated segments and directly uses
v.version.split('.')[2], which can raise IndexError for strings like "4.20";
update the Jinja logic around the openshift_versions loop to validate/safely
parse v.version: compute parts = v.version.split('.') and set channel_name using
the first two parts only if parts|length >= 2 (fallback to v.version or join
with a default like '0' for missing segments), and set patch to int(parts[2])
only when parts|length >= 3 (fallback to 0 or None otherwise); change references
to channel_name and patch accordingly so the template never assumes a third
segment.
| blockedImages: | ||
| - name: registry.redhat.io/quay/quay-builder-qemu-rhcos-rhel8 | ||
| - name: registry.redhat.io/quay/quay-builder-rhel8 No newline at end of file |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
oc-mirror blockedImages quay-builder-qemu-rhcos-rhel8
💡 Result:
oc-mirror blockedImages quay-builder-qemu-rhcos-rhel8 means your ImageSetConfiguration is telling oc-mirror to exclude the Quay builder VM image (registry.redhat.io/quay/quay-builder-qemu-rhcos-rhel8...) via mirror.blockedImages (“full tag, digest, or pattern of images to block from mirroring”). [1]
If you are deploying Red Hat Quay builders, this image is commonly required because Quay’s builder config references it as BUILDER_VM_CONTAINER_IMAGE (example uses registry.redhat.io/quay/quay-builder-qemu-rhcos-rhel8:...). [2]
Fix
-
Remove it from
mirror.blockedImagesin yourimageset-config.yaml. [1] -
Ensure it is included to mirror, typically via
mirror.additionalImages, e.g.
mirror:
additionalImages:
- name: registry.redhat.io/quay/quay-builder-qemu-rhcos-rhel8:<tag>(Use the exact tag/digest your Quay deployment is configured to use.) [1][2]
- If mirroring succeeds but the cluster still won’t pull it, check cluster image policy: when
allowedRegistriesis set, everything is blocked unless explicitly listed (includingregistry.redhat.io). Add the needed registry/repository there. [3]
Sources
[1] OpenShift docs – mirror.blockedImages in ImageSetConfiguration parameters (oc-mirror)
[2] Red Hat Quay docs – builders configuration showing BUILDER_VM_CONTAINER_IMAGE set to registry.redhat.io/quay/quay-builder-qemu-rhcos-rhel8:...
[3] OpenShift docs – allowedRegistries behavior in image.config.openshift.io/cluster
Remove these images from blockedImages — they are required for Quay builder deployments.
The blockedImages section prevents mirroring of registry.redhat.io/quay/quay-builder-qemu-rhcos-rhel8 and registry.redhat.io/quay/quay-builder-rhel8, but these images are essential for Quay builders and should not be excluded. To fix this:
- Remove these entries from
blockedImages. - Add them to
mirror.additionalImageswith the correct tags to ensure they are mirrored:Use the exact tags your Quay deployment is configured to use.mirror: additionalImages: - name: registry.redhat.io/quay/quay-builder-qemu-rhcos-rhel8:<tag> - name: registry.redhat.io/quay/quay-builder-rhel8:<tag>
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@templates/imagesetconfiguration-core.yaml.j2` around lines 29 - 31, Remove
the two Quay builder images currently listed under blockedImages
(registry.redhat.io/quay/quay-builder-qemu-rhcos-rhel8 and
registry.redhat.io/quay/quay-builder-rhel8) and instead add them under
mirror.additionalImages with the exact tags your Quay deployment requires (e.g.,
registry.redhat.io/quay/quay-builder-qemu-rhcos-rhel8:<tag> and
registry.redhat.io/quay/quay-builder-rhel8:<tag>) so they will be mirrored
rather than blocked.
| {% set additional_operator_names = operator.csvNames if (operator.csvMirror | default(false) | bool) else [] %} | ||
| {% for operator_name in [operator.name] + additional_operator_names %} | ||
| - name: {{ operator_name }} | ||
| defaultChannel: {{ operator.channel }} | ||
| channels: | ||
| - name: {{ operator.channel }} | ||
| minVersion: {{ operator.init_version }} | ||
| maxVersion: {{ operator.version }} |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For oc-mirror ImageSetConfiguration apiVersion mirror.openshift.io/v2alpha1, where should CSV names be specified (if supported)? Is using mirror.operators[].packages[].name with CSV names valid?
💡 Result:
For apiVersion: mirror.openshift.io/v2alpha1, CSV names are not specified in mirror.operators[].packages[].name.
mirror.operators[].packages[].nameis the Operator package name (for example,elasticsearch-operator), not a CSV name. Using a CSV there is not valid. [1]- If you want to pin/select specific Operator versions/CSVs, use one of these (depending on what you’re trying to match):
- Specific bundles (bundle names are typically the CSV names) via
mirror.operators[].packages[].bundles[].name. [1] - Version range selection via
minVersion/maxVersion(at package level and/or per-channel). [1] - If the bundle you need doesn’t have semver metadata, use
minBundle(bundle name) to anchor selection. [2]
- Specific bundles (bundle names are typically the CSV names) via
Example (CSV/bundle pinning):
mirror:
operators:
- catalog: registry.redhat.io/redhat/redhat-operator-index:v4.14
packages:
- name: aws-load-balancer-operator
bundles:
- name: aws-load-balancer-operator.v1.1.0[1]
Sources:
[1] Red Hat OpenShift docs (oc-mirror v2) – ImageSetConfiguration params + bundle filtering scenarios (includes packages.name meaning and bundles.name usage) (docs.redhat.com)
[2] oc-mirror v2 API (Go types) – minBundle semantics in IncludeBundle (pkg.go.dev)
Citations:
- 1: https://docs.redhat.com/en/documentation/openshift_container_platform/4.16/html/disconnected_installation_mirroring/about-installing-oc-mirror-v2
- 2: https://pkg.go.dev/github.com/openshift/oc-mirror/v2%40v2.0.0-20231019101302-443e6bfc9726/pkg/api/v1alpha2
🏁 Script executed:
# First, let's examine the template file to understand the context
cat -n templates/imagesetconfiguration-post.yaml.j2 | head -30Repository: rh-ecosystem-edge/enclave
Length of output: 1525
🏁 Script executed:
# Check the schemas/operators.yaml to see how csvNames is defined
cat -n schemas/operators.yaml | head -60Repository: rh-ecosystem-edge/enclave
Length of output: 2059
🏁 Script executed:
# Search for csvNames usage in the codebase
rg "csvNames" -n --context 3Repository: rh-ecosystem-edge/enclave
Length of output: 10583
csvNames emitted in packages[].name field generates invalid ImageSetConfiguration.
Per oc-mirror schema for mirror.openshift.io/v2alpha1, packages[].name must contain operator package names. The template places csvNames (ClusterServiceVersion names) into this field, creating malformed configurations.
For example, in defaults/operators.yaml: openshift-update-service (package name) vs. update-service-operator (CSV name from csvNames) are distinct. Placing the CSV name in packages[].name is not valid per oc-mirror specification.
If bundle-level selection is required, use packages[].bundles[].name instead. Otherwise, use only operator.name in packages. This affects three templates: imagesetconfiguration.yaml.j2, plugin-imageset.yaml.j2, and imagesetconfiguration-post.yaml.j2.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@templates/imagesetconfiguration-post.yaml.j2` around lines 11 - 18, The
template is placing operator.csvNames (CSV names) into packages[].name, which
must be package names (operator.name); change the logic in
imagesetconfiguration-post.yaml.j2 (and the other two affected templates) to
stop appending additional_operator_names into packages[].name: keep
packages[].name as operator.name only, and if csv-level selection is required
iterate operator.csvNames into packages[].bundles[].name instead (use
operator.csvNames -> packages[].bundles[].name), otherwise remove the csvNames
branch entirely; update the variables referenced (additional_operator_names,
operator.csvNames, operator.name, packages[].name, packages[].bundles[].name)
accordingly.
Signed-off-by: Javi Polo <jpolo@redhat.com>
|
related to openshift/oc-mirror#1394 |
2 participants
Summary by CodeRabbit
New Features
Refactor