Skip to content

Commit f67451e

Browse files
njhalenjhale
committed
refactor(openshift): don't rely on reflection as version predicate
Signed-off-by: Nick Hale <[email protected]>
1 parent 484e0c7 commit f67451e

File tree

3 files changed

+164
-5
lines changed

3 files changed

+164
-5
lines changed

‎pkg/controller/operators/openshift/clusteroperator_controller.go

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -128,7 +128,7 @@ func (r *ClusterOperatorReconciler) Reconcile(ctx context.Context, req reconcile
128128

129129
func (r *ClusterOperatorReconciler) setVersions(_ context.Context, co *ClusterOperator) error {
130130
// If we've successfully synced, we know our operator is working properly, so we can update the version
131-
if r.syncTracker.SuccessfulSyncs() > 0 && !reflect.DeepEqual(co.Status.Versions, r.TargetVersions) {
131+
if r.syncTracker.SuccessfulSyncs() > 0 && !versionsMatch(co.Status.Versions, r.TargetVersions) {
132132
co.Status.Versions = r.TargetVersions
133133
}
134134

@@ -141,7 +141,7 @@ func (r *ClusterOperatorReconciler) setProgressing(_ context.Context, co *Cluste
141141
LastTransitionTime: r.Now(),
142142
}
143143

144-
if r.syncTracker.SuccessfulSyncs() > 0 && reflect.DeepEqual(co.Status.Versions, r.TargetVersions) {
144+
if r.syncTracker.SuccessfulSyncs() > 0 && versionsMatch(co.Status.Versions, r.TargetVersions) {
145145
desired.Status = configv1.ConditionFalse
146146
desired.Message = fmt.Sprintf("Deployed %s", olmversion.OLMVersion)
147147
} else {
@@ -165,7 +165,7 @@ func (r *ClusterOperatorReconciler) setAvailable(_ context.Context, co *ClusterO
165165
LastTransitionTime: r.Now(),
166166
}
167167

168-
if r.syncTracker.SuccessfulSyncs() > 0 && reflect.DeepEqual(co.Status.Versions, r.TargetVersions) {
168+
if r.syncTracker.SuccessfulSyncs() > 0 && versionsMatch(co.Status.Versions, r.TargetVersions) {
169169
desired.Status = configv1.ConditionTrue
170170
} else {
171171
desired.Status = configv1.ConditionFalse
@@ -187,7 +187,7 @@ func (r *ClusterOperatorReconciler) setDegraded(_ context.Context, co *ClusterOp
187187
LastTransitionTime: r.Now(),
188188
}
189189

190-
if r.syncTracker.SuccessfulSyncs() > 0 && reflect.DeepEqual(co.Status.Versions, r.TargetVersions) {
190+
if r.syncTracker.SuccessfulSyncs() > 0 && versionsMatch(co.Status.Versions, r.TargetVersions) {
191191
desired.Status = configv1.ConditionFalse
192192
} else {
193193
desired.Status = configv1.ConditionTrue
@@ -218,7 +218,7 @@ func (r *ClusterOperatorReconciler) setUpgradeable(ctx context.Context, co *Clus
218218
// Set upgradeable=false if (either/or):
219219
// 1. OLM currently upgrading (takes priorty in the status message)
220220
// 2. Operators currently installed that are incompatible with the next OCP minor version
221-
if r.syncTracker.SuccessfulSyncs() < 1 || !reflect.DeepEqual(co.Status.Versions, r.TargetVersions) {
221+
if r.syncTracker.SuccessfulSyncs() < 1 || !versionsMatch(co.Status.Versions, r.TargetVersions) {
222222
// OLM is still upgrading
223223
desired.Status = configv1.ConditionFalse
224224
desired.Message = "Waiting for updates to take effect"

‎pkg/controller/operators/openshift/helpers.go

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,33 @@ func conditionsEqual(a, b *configv1.ClusterOperatorStatusCondition) bool {
4848
return a.Type == b.Type && a.Status == b.Status && a.Message == b.Message && a.Reason == b.Reason
4949
}
5050

51+
func versionsMatch(a []configv1.OperandVersion, b []configv1.OperandVersion) bool {
52+
if len(a) != len(b) {
53+
return false
54+
}
55+
56+
counts := map[configv1.OperandVersion]int{}
57+
for _, av := range a {
58+
counts[av] += 1
59+
}
60+
61+
for _, bv := range b {
62+
remaining, ok := counts[bv]
63+
if !ok {
64+
return false
65+
}
66+
67+
if remaining == 1 {
68+
delete(counts, bv)
69+
continue
70+
}
71+
72+
counts[bv] -= 1
73+
}
74+
75+
return len(counts) < 1
76+
}
77+
5178
type skews []skew
5279

5380
func (s skews) String() string {

‎pkg/controller/operators/openshift/helpers_test.go

Lines changed: 132 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,139 @@ func TestConditionsEqual(t *testing.T) {
7777
require.Equal(t, tt.expect, conditionsEqual(tt.args.a, tt.args.b))
7878
})
7979
}
80+
}
8081

82+
func TestVersionsMatch(t *testing.T) {
83+
type in struct {
84+
a, b []configv1.OperandVersion
85+
}
86+
for _, tt := range []struct {
87+
description string
88+
in in
89+
expect bool
90+
}{
91+
{
92+
description: "Different/Nil",
93+
in: in{
94+
a: []configv1.OperandVersion{
95+
{Name: "weyland", Version: "1.0.0"},
96+
},
97+
b: nil,
98+
},
99+
expect: false,
100+
},
101+
{
102+
description: "Different/Names",
103+
in: in{
104+
a: []configv1.OperandVersion{
105+
{Name: "weyland", Version: "1.0.0"},
106+
},
107+
b: []configv1.OperandVersion{
108+
{Name: "yutani", Version: "1.0.0"},
109+
},
110+
},
111+
expect: false,
112+
},
113+
{
114+
description: "Different/Versions",
115+
in: in{
116+
a: []configv1.OperandVersion{
117+
{Name: "weyland", Version: "1.0.0"},
118+
},
119+
b: []configv1.OperandVersion{
120+
{Name: "weyland", Version: "2.0.0"},
121+
},
122+
},
123+
expect: false,
124+
},
125+
{
126+
description: "Different/Lengths",
127+
in: in{
128+
a: []configv1.OperandVersion{
129+
{Name: "weyland", Version: "1.0.0"},
130+
},
131+
b: []configv1.OperandVersion{
132+
{Name: "weyland", Version: "1.0.0"},
133+
{Name: "yutani", Version: "1.0.0"},
134+
},
135+
},
136+
expect: false,
137+
},
138+
{
139+
description: "Different/Elements",
140+
in: in{
141+
a: []configv1.OperandVersion{
142+
{Name: "weyland", Version: "1.0.0"},
143+
{Name: "weyland", Version: "1.0.0"},
144+
{Name: "weyland", Version: "1.0.0"},
145+
{Name: "yutani", Version: "1.0.0"},
146+
},
147+
b: []configv1.OperandVersion{
148+
{Name: "weyland", Version: "1.0.0"},
149+
{Name: "weyland", Version: "1.0.0"},
150+
{Name: "yutani", Version: "1.0.0"},
151+
{Name: "yutani", Version: "1.0.0"},
152+
},
153+
},
154+
expect: false,
155+
},
156+
{
157+
description: "Same/Nil",
158+
in: in{
159+
a: nil,
160+
b: nil,
161+
},
162+
expect: true,
163+
},
164+
{
165+
description: "Same/Empty",
166+
in: in{
167+
a: []configv1.OperandVersion{},
168+
b: []configv1.OperandVersion{},
169+
},
170+
expect: true,
171+
},
172+
{
173+
description: "Same/Empty/Nil",
174+
in: in{
175+
a: []configv1.OperandVersion{},
176+
b: nil,
177+
},
178+
expect: true,
179+
},
180+
{
181+
description: "Same",
182+
in: in{
183+
a: []configv1.OperandVersion{
184+
{Name: "weyland", Version: "1.0.0"},
185+
{Name: "yutani", Version: "1.0.0"},
186+
},
187+
b: []configv1.OperandVersion{
188+
{Name: "weyland", Version: "1.0.0"},
189+
{Name: "yutani", Version: "1.0.0"},
190+
},
191+
},
192+
expect: true,
193+
},
194+
{
195+
description: "Same/Unordered",
196+
in: in{
197+
a: []configv1.OperandVersion{
198+
{Name: "weyland", Version: "1.0.0"},
199+
{Name: "yutani", Version: "1.0.0"},
200+
},
201+
b: []configv1.OperandVersion{
202+
{Name: "yutani", Version: "1.0.0"},
203+
{Name: "weyland", Version: "1.0.0"},
204+
},
205+
},
206+
expect: true,
207+
},
208+
} {
209+
t.Run(tt.description, func(t *testing.T) {
210+
require.Equal(t, tt.expect, versionsMatch(tt.in.a, tt.in.b))
211+
})
212+
}
81213
}
82214

83215
func TestIncompatibleOperators(t *testing.T) {

0 commit comments

Comments
 (0)