OCPBUGS-85065: Fix repeated CPO Deployment churn due to OPENSHIFT_IMG_OVERRIDES reordering#8656
OCPBUGS-85065: Fix repeated CPO Deployment churn due to OPENSHIFT_IMG_OVERRIDES reordering#8656dhgautam99 wants to merge 1 commit into
Conversation
The prior fix for OCPBUGS-57626 sorted ICSP items by name to ensure deterministic ordering of OPENSHIFT_IMG_OVERRIDES. However, when overlapping mirror definitions exist across multiple IDMS and/or ICSP objects for the same source registry, the mirrors within each source were not sorted. This caused the final OPENSHIFT_IMG_OVERRIDES string to vary between reconciliation cycles, triggering repeated CPO Deployment rollouts. Sort mirrors alphabetically per source in ConvertOpenShiftImageRegistryOverridesToCommandLineFlag() to guarantee a deterministic output regardless of the order mirrors are collected from IDMS/ICSP objects.
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
jira/severity-moderate
|
Skipping CI for Draft Pull Request. |
openshift-ci-robot
added
jira/valid-reference
|
@dhgautam99: This pull request references Jira Issue OCPBUGS-85065, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
📝 WalkthroughWalkthroughThis pull request adds deterministic sorting of registry mirror replacements in the Suggested reviewers
🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (10 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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dhgautam99 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 |
openshift-ci
Bot
added
area/control-plane-operator
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
support/util/util.go (1)
253-255: ⚡ Quick winAvoid mutating caller-owned mirror slices in place.
sort.Strings(registryReplacements)mutates the slice stored inregistryOverrides. Copy before sorting to keep this converter side-effect free.Proposed fix
for _, registrySource := range sortedRegistrySources { - registryReplacements := registryOverrides[registrySource] + registryReplacements := append([]string(nil), registryOverrides[registrySource]...) sort.Strings(registryReplacements) for _, registryReplacement := range registryReplacements { commandLineFlagArray = append(commandLineFlagArray, fmt.Sprintf("%s=%s", registrySource, registryReplacement)) } }🤖 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 `@support/util/util.go` around lines 253 - 255, The code is mutating caller-owned slices by calling sort.Strings on registryReplacements (which is taken from registryOverrides[registrySource]); to fix, make a fresh copy of the slice before sorting and iterating so you don't modify registryOverrides. Specifically, read registryReplacements := registryOverrides[registrySource] into a new slice (e.g., create a copy via append to an empty slice), call sort.Strings on that copied slice, and use the copied slice in the for _, registryReplacement range loop so registryOverrides remains unchanged.
🤖 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.
Nitpick comments:
In `@support/util/util.go`:
- Around line 253-255: The code is mutating caller-owned slices by calling
sort.Strings on registryReplacements (which is taken from
registryOverrides[registrySource]); to fix, make a fresh copy of the slice
before sorting and iterating so you don't modify registryOverrides.
Specifically, read registryReplacements := registryOverrides[registrySource]
into a new slice (e.g., create a copy via append to an empty slice), call
sort.Strings on that copied slice, and use the copied slice in the for _,
registryReplacement range loop so registryOverrides remains unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: ce0fcb00-9b93-447e-a802-85c65faa35ff
📒 Files selected for processing (2)
support/util/util.gosupport/util/util_test.go
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@dhgautam99: This pull request references Jira Issue OCPBUGS-85065, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8656 +/- ##
==========================================
+ Coverage 40.69% 41.27% +0.58%
==========================================
Files 755 755
Lines 93373 93447 +74
==========================================
+ Hits 37994 38567 +573
+ Misses 52646 52148 -498
+ Partials 2733 2732 -1
... and 35 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
3 participants
What this PR does / why we need it:
Sorts mirrors within each registry source alphabetically when building
the
OPENSHIFT_IMG_OVERRIDESenvironment variable string. Without this,overlapping mirror definitions across multiple IDMS/ICSP objects for the
same source produce non-deterministic mirror ordering, causing the CPO
Deployment to roll out repeatedly even though the content is identical.
The prior fix (OCPBUGS-57626) sorted ICSP items by name but did not sort
the mirrors within each source. This is a variant where the same source
appears in multiple IDMS/ICSP objects, and the mirror values for that
source get appended in non-deterministic order.
Which issue(s) this PR fixes:
Fixes OCPBUGS-85065
Special notes for your reviewer:
The fix is a single
sort.Strings()call inConvertOpenShiftImageRegistryOverridesToCommandLineFlag(). This makesthe output deterministic regardless of how mirrors are collected upstream
from IDMS/ICSP objects.
Checklist:
Summary by CodeRabbit
Bug Fixes
Tests