MGMT-23874: fix plugin IDMS/ITMS overwrite during mirror deploy#312
MGMT-23874: fix plugin IDMS/ITMS overwrite during mirror deploy#312
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughPlaybooks and the Quay operator mirror play were changed to generate plugin-specific IDMS/ITMS manifest filenames and resource names at runtime, copy and rewrite landing-zone outputs into those plugin-named manifests, and apply the computed manifests to the cluster using new manifest-path facts passed to Changes
Sequence Diagram(s)sequenceDiagram
participant Ansible as Ansible Controller
participant FS as Landing-Zone Filesystem
participant QuayPlay as quay_disconnected_mirrors.yaml
participant OC as oc (Cluster)
Ansible->>Ansible: include mirror_plugin (quay_plugin_mirror=true)
Ansible->>FS: stat `idms-oc-mirror.yaml` / `itms-oc-mirror.yaml`
FS-->>Ansible: file exists
Ansible->>FS: copy to plugin-specific files (`*-plugin-{{ plugin_name }}.yaml`)
Ansible->>FS: rewrite `name:` fields to end with `-{{ plugin_name }}`
Ansible->>FS: delete generic landing-zone oc-mirror files
Ansible->>QuayPlay: include quay_disconnected_mirrors.yaml (quay_plugin_mirror=true)
QuayPlay->>QuayPlay: compute `quay_mirror_suffix`, set manifest path facts
QuayPlay->>FS: copy/prepare `quay_idms_internal_manifest` / `quay_itms_internal_manifest`
QuayPlay->>OC: oc apply -f {{ quay_idms_apply_manifest }} and {{ quay_itms_apply_manifest }}
OC-->>Ansible: apply results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
|
Tarball created: |
|
can we consider an approach that every plugin uses a custom catalog? that would remove the concern of overwriting completely. made the same suggestion in #288 (comment) as for naming, i've created #306 which may help, if we adopt the above suggestion. |
Yeah, I've actually considered exactly that:) |
started illustrating what that might look like in #316. |
danielerez
force-pushed
the
mgmt-23874-plugin-mirror-merge
branch
from
11bb8d9 to
238f16d
Compare
|
Tarball created: |
|
Thanks, as suggested, changed this PR to generate a per-plugin IDMS/ITMS. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
playbooks/tasks/mirror_plugin.yaml (1)
88-106: Block structure and comments are clear.The block properly sets up the directory path and documents the naming strategy. The comment at lines 103-106 acknowledges the multi-resource concern ("If oc-mirror emits multiple idms-* / itms-* in one file, each becomes the same name") but the current implementation doesn't actually handle this case—it just warns about it.
If you need to handle multi-document manifests gracefully in the future, consider splitting the YAML into individual documents before renaming, or using a tool like
yqto iterate over documents and assign sequential suffixes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playbooks/tasks/mirror_plugin.yaml` around lines 88 - 106, The current task sets lz_cluster_resources_dir and stats lz_idms_src_stat / lz_itms_src_stat but does not handle multi-document manifests (multiple idms-* / itms-* in one file) as noted in the comment; update the task to detect multi-document YAML in idms-oc-mirror.yaml and itms-oc-mirror.yaml and split or iterate over documents before applying renames so each document gets a unique name/suffix (e.g., use yq to split into separate files or iterate documents and append sequential suffixes), ensuring the subsequent steps that reference lz_idms_src_stat, lz_itms_src_stat, and the files handle the generated per-document filenames.
🤖 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_mirrors.yaml`:
- Around line 68-85: The two replace tasks "Rename ImageTagMirrorSet resources
in ITMS manifest (plugin flat names)" and "(default suffix)" risk colliding when
a manifest contains multiple itms-* entries because their regexp
'^(\s*name:\s*)itms-([^\s]+)\s*$' replaces all matches to the same target;
narrow the match so only the intended resource is renamed by including the
plugin identifier or original exact name in the regexp (e.g. match itms-{{
plugin_name }}-internal or a specific suffix), or iterate over a list of
expected resource names and perform replacements per-item; update the replace
tasks to reference the more-specific regexp and/or loop variable so only the
intended itms-<plugin> resource in quay_itms_internal_manifest is changed
(adjust the replace string accordingly and keep the existing when conditions).
- Around line 37-54: The plugin-mode replace currently maps every "name:
idms-<something>" to the same flat name (ansible task named "Rename
ImageDigestMirrorSet resources in IDMS manifest (plugin flat names)"), causing
duplicate metadata.name when multiple idms-* entries exist; update that replace
so it preserves uniqueness by incorporating either the original captured suffix
or the configured quay_mirror_suffix (use the existing regexp capture group for
the original suffix or append -{{ quay_mirror_suffix }}), i.e. change the
replace of the ansible.builtin.replace that targets "{{
quay_idms_internal_manifest }}" when quay_plugin_mirror is true to include the
captured part (or quay_mirror_suffix) rather than always using "idms-plugin-{{
plugin_name }}-internal", ensuring each replaced name remains unique.
In `@playbooks/tasks/mirror_plugin.yaml`:
- Around line 107-125: The replace task that matches regexp
'^(\s*name:\s*)idms-([^\s]+)\s*$' (task "Rename ImageDigestMirrorSet names in
plugin LZ IDMS manifest" targeting idms-oc-mirror-plugin-{{ plugin_name }}.yaml)
currently replaces all matches with the same flat name and causes collisions;
change the replacement to preserve the original suffix by including the second
capture group (e.g. append "-\2" or otherwise incorporate \2 into the
replacement) so each idms entry becomes unique (for example idms-plugin-{{
plugin_name }}-<original-suffix>) instead of collapsing to a single name.
- Around line 127-145: The ITMS handling duplicates the earlier IDMS logic but
the PR only handles ITMS—apply the same sequence of tasks for IDMS: add a copy
task (like "Copy LZ ITMS to plugin-specific manifest") to copy
idms-oc-mirror.yaml to idms-oc-mirror-plugin-{{ plugin_name }}.yaml when
lz_idms_src_stat.stat.exists, add a replace task (like "Rename ImageTagMirrorSet
names in plugin LZ ITMS manifest") that updates the name from idms-... to
idms-plugin-{{ plugin_name }} using a similar regexp/replace, and add a cleanup
file task (like "Remove generic LZ ITMS after plugin-specific copy") to remove
the original idms-oc-mirror.yaml; use the same conditional variable checks
(lz_idms_src_stat.stat.exists) and mirror the task names to locate where to add
these changes.
---
Nitpick comments:
In `@playbooks/tasks/mirror_plugin.yaml`:
- Around line 88-106: The current task sets lz_cluster_resources_dir and stats
lz_idms_src_stat / lz_itms_src_stat but does not handle multi-document manifests
(multiple idms-* / itms-* in one file) as noted in the comment; update the task
to detect multi-document YAML in idms-oc-mirror.yaml and itms-oc-mirror.yaml and
split or iterate over documents before applying renames so each document gets a
unique name/suffix (e.g., use yq to split into separate files or iterate
documents and append sequential suffixes), ensuring the subsequent steps that
reference lz_idms_src_stat, lz_itms_src_stat, and the files handle the generated
per-document filenames.
🪄 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: 4d76e8df-642f-460d-80fd-0620e84abc52
📒 Files selected for processing (3)
operators/quay-operator/quay_disconnected_mirrors.yamlplaybooks/tasks/deploy_plugin.yamlplaybooks/tasks/mirror_plugin.yaml
Sure, added a comment there. But it looks complimentary to this change:) Still need this mirror set handling. |
i agree. i'm assuming that this PR may slightly change in implementation as a result of #318? |
Each plugin mirror run used oc-mirror output whose ImageDigestMirrorSet and ImageTagMirrorSet objects reused generic names, so applying Quay disconnected mirrors could overwrite or collide with other plugins or the core flow. Landing-zone manifests are copied and renamed to stable per-plugin names (idms-plugin-<plugin>, itms-plugin-<plugin>). Quay Enterprise follow-on mirrors use matching internal names (idms-plugin-<plugin>-internal, itms-plugin-<plugin>-internal). Example of resulting resources: $ oc get idms NAME idms-plugin-lso idms-plugin-lso-internal idms-plugin-mtv idms-plugin-mtv-internal
danielerez
force-pushed
the
mgmt-23874-plugin-mirror-merge
branch
from
238f16d to
f0a2c39
Compare
|
Tarball created: |
Actually not really, there's no special handling for core plugins in this change. Tested it locally with #318 and seems to work fine. |
maorfr
left a comment
maorfr
left a comment
There was a problem hiding this comment.
LGTM
i have much more to learn
3 participants
Each plugin mirror run used oc-mirror output whose ImageDigestMirrorSet and ImageTagMirrorSet objects reused generic names, so applying Quay disconnected mirrors could overwrite or collide with other plugins or the core flow.
Landing-zone manifests are copied and renamed to stable per-plugin names (idms-plugin-[name], itms-plugin-[name]).
Quay Enterprise follow-on mirrors use matching internal names (idms-plugin-[name]-internal, itms-plugin-[name]-internal).
Example of resulting resources:
$ oc get idms
NAME
idms-operator-0-plugin-lso
idms-operator-0-plugin-lso-internal
idms-operator-0-plugin-mtv
idms-operator-0-plugin-mtv-internal
Summary by CodeRabbit
New Features
Chores