🐛 Fix duplicate ClusterExtensionRevisions during Helm-to-Boxcutter migr… #2329
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…ation
This commit fixes OCPBUGS-62943 where two ClusterExtensionRevisions were
being created during Helm-to-Boxcutter migration instead of just one.
Root causes:
1. Manifest ordering inconsistency: CRDs from Helm release manifest and
bundle manifest appeared in different orders, causing PhaseSort to
produce different phase structures even though they contained the same
objects.
2. CollisionProtection mismatch: The migrated revision had
collisionProtection=None (needed to adopt Helm-managed resources) while
the bundle-generated revision had collisionProtection=Prevent (the
default value).
Solution:
1. Added deterministic sorting in PhaseSort (phase.go):
- Sort objects within each phase by Group, Version, Kind, Namespace, Name
- Ensures consistent phase structure regardless of input order
- Critical for comparing revisions from different sources
2. Added CollisionProtection preservation (boxcutter.go):
- New preserveCollisionProtection() function copies CollisionProtection
values from current revision to desired revision for matching objects
- New objectKey() helper generates unique keys based on GVK+namespace+name
- Called before patching to ensure CollisionProtection values match
With these changes, only a single ClusterExtensionRevision is created
during Helm-to-Boxcutter migration, as expected.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Todd Short <tshort@redhat.com>
tmshort
force-pushed
the
fix-extra-cer
branch
from
5897825 to
8b067ab
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2329 +/- ##
=======================================
Coverage 74.30% 74.31%
=======================================
Files 91 91
Lines 7083 7128 +45
=======================================
+ Hits 5263 5297 +34
- Misses 1405 1411 +6
- Partials 415 420 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
I think this is intentional. We need |
I think the original behavior makes sense. We shouldn't keep |
| // to ensure consistent ordering regardless of input order. This is critical for | ||
| // Helm-to-Boxcutter migration where the same resources may come from different sources | ||
| // (Helm release manifest vs bundle manifest) and need to produce identical phases. | ||
| func compareClusterExtensionRevision(a, b ocv1.ClusterExtensionRevisionObject) int { |
There was a problem hiding this comment.
| func compareClusterExtensionRevision(a, b ocv1.ClusterExtensionRevisionObject) int { | |
| func compareClusterExtensionRevisionObject(a, b ocv1.ClusterExtensionRevisionObject) int { |
There was a problem hiding this comment.
or even just compareRevisionObject ?
So, removing either of the changes will cause the bug (two CERs created) to assert itself. If we are OK with this, then we don't necessarily need this fix, although sorting the objects during the phase is probably still a good idea. @dtfranz and I found the same solution to the problem:
If we're good with having two initial CERs upon transition from the helm applier to the boxcutter applier, then I can reduce this to be just the sorting changes, and remove the CollisionProtection change. I would then close the bug as intentional. |
3 participants
…ation
This commit fixes OCPBUGS-62943 where two ClusterExtensionRevisions were being created during Helm-to-Boxcutter migration instead of just one.
Root causes:
Manifest ordering inconsistency: CRDs from Helm release manifest and bundle manifest appeared in different orders, causing PhaseSort to produce different phase structures even though they contained the same objects.
CollisionProtection mismatch: The migrated revision had collisionProtection=None (needed to adopt Helm-managed resources) while the bundle-generated revision had collisionProtection=Prevent (the default value).
Solution:
Added deterministic sorting in PhaseSort (phase.go):
Added CollisionProtection preservation (boxcutter.go):
With these changes, only a single ClusterExtensionRevision is created during Helm-to-Boxcutter migration, as expected.
🤖 Generated with Claude Code
Description
Reviewer Checklist