Skip to content

Commit a783430

Browse files
YCShen1010YCShen1010
authored
Fix operand deletion inconsistency in Keycloak upgrade case (#1151)
* fix operands deletion inconsistency in upgrade case Signed-off-by: YuChen <[email protected]> * add test cases for the operand deletion Signed-off-by: YuChen <[email protected]> * using variable for the case operators with diff channel shared same configname Signed-off-by: YuChen <[email protected]> --------- Signed-off-by: YuChen <[email protected]>
1 parent 98b5c9b commit a783430

File tree

2 files changed

+120
-3
lines changed

2 files changed

+120
-3
lines changed

‎controllers/operandrequest/reconcile_operator.go

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -795,11 +795,12 @@ func checkSubAnnotationsForUninstall(reqName, reqNs, opName, installMode string,
795795
// 1. operator is not uninstalled AND intallMode is no-op.
796796
// 2. operator is uninstalled AND at least one other <prefix>/operatorNamespace annotation exists.
797797
// 3. remaining <prefix>/request annotation's values contain the same operator name
798-
// 4. remaining <prefix>/config annotation's values contain the same configName as this operator
798+
// 4. currentConfigName is found AND remaining <prefix>/config annotation's values contain the same configName as this operator
799+
// 5. currentConfigName is empty (not found in annotations)
799800
if (!uninstallOperator && installMode == operatorv1alpha1.InstallModeNoop) ||
800801
(uninstallOperator && len(opreqNsSlice) != 0) ||
801802
util.Contains(operatorNameSlice, opName) ||
802-
(currentConfigName != "" && util.Contains(configNameSlice, currentConfigName)) {
803+
(currentConfigName != "" && util.Contains(configNameSlice, currentConfigName)) || currentConfigName == "" {
803804
uninstallOperand = false
804805
}
805806

‎controllers/operandrequest/reconcile_operator_test.go

Lines changed: 117 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -93,19 +93,23 @@ func TestCheckSubAnnotationsForUninstall(t *testing.T) {
9393
reqNsOld := "ibm-common-services"
9494
opNameV3 := "ibm-iam"
9595
opChannelV3 := "v3"
96+
opConfigV3 := "ibm-iam"
9697

9798
reqNameB := "other-request"
9899
reqNsNew := "other-namespace"
99100
opNameV4 := "ibm-im"
100101
opChannelV4 := "v4.0"
102+
opConfigV4 := "ibm-im"
101103

102104
reqNameC := "common-service-postgresql"
103105
opNameC := "common-service-postgresql"
104106
opChannelC := "stable-v1.22"
107+
opConfigC := "common-service-postgresql"
105108

106109
reqNameD := "edb-keycloak"
107110
opNameD := "edb-keycloak"
108111
opChannelD := "stable"
112+
opConfigD := "edb-keycloak"
109113

110114
// The annotation key has prefix: <requestNamespace>.<requestName>.<operatorName>/*
111115

@@ -115,7 +119,9 @@ func TestCheckSubAnnotationsForUninstall(t *testing.T) {
115119
// When one of following conditions are met, the operand will NOT be uninstalled:
116120
// 1. operator is not uninstalled AND intallMode is no-op.
117121
// 2. operator is uninstalled AND at least one other <prefix>/operatorNamespace annotation exists.
118-
// 2. remaining <prefix>/request annotation's values contain the same operator name
122+
// 3. remaining <prefix>/request annotation's values contain the same operator name
123+
// 4. currentConfigName is found AND remaining <prefix>/config annotation's values contain the same configName as this operator
124+
// 5. currentConfigName is empty (not found in annotations)
119125

120126
// Test case 1: uninstallOperator is true, uninstallOperand is true for uninstalling operator with v3 no-op installMode
121127
// The operator and operand should be uninstalled because no remaining annotation is left.
@@ -125,6 +131,7 @@ func TestCheckSubAnnotationsForUninstall(t *testing.T) {
125131
Annotations: map[string]string{
126132
reqNsOld + "." + reqNameA + "." + opNameV3 + "/request": opChannelV3,
127133
reqNsOld + "." + reqNameA + "." + opNameV3 + "/operatorNamespace": reqNsOld,
134+
reqNsOld + "." + reqNameA + "." + opNameV3 + "/config": opConfigV3,
128135
},
129136
Labels: map[string]string{
130137
constant.OpreqLabel: "true",
@@ -139,6 +146,7 @@ func TestCheckSubAnnotationsForUninstall(t *testing.T) {
139146

140147
assert.NotContains(t, sub.Annotations, reqNsOld+"."+reqNameA+"."+opNameV3+"/request")
141148
assert.NotContains(t, sub.Annotations, reqNsOld+"."+reqNameA+"."+opNameV3+"/operatorNamespace")
149+
assert.NotContains(t, sub.Annotations, reqNsOld+"."+reqNameA+"."+opNameV3+"/config")
142150

143151
// Test case 2: uninstallOperator is true, uninstallOperand is true for uninstalling operator with general installMode
144152
// The operator and operand should be uninstalled because no remaining annotation is left.
@@ -148,6 +156,7 @@ func TestCheckSubAnnotationsForUninstall(t *testing.T) {
148156
Annotations: map[string]string{
149157
reqNsOld + "." + reqNameA + "." + opNameV4 + "/request": opChannelV4,
150158
reqNsOld + "." + reqNameA + "." + opNameV4 + "/operatorNamespace": reqNsOld,
159+
reqNsOld + "." + reqNameA + "." + opNameV4 + "/config": opConfigV4,
151160
},
152161
Labels: map[string]string{
153162
constant.OpreqLabel: "true",
@@ -162,6 +171,7 @@ func TestCheckSubAnnotationsForUninstall(t *testing.T) {
162171

163172
assert.NotContains(t, sub.Annotations, reqNsOld+"."+reqNameA+"."+opNameV4+"/request")
164173
assert.NotContains(t, sub.Annotations, reqNsOld+"."+reqNameA+"."+opNameV4+"/operatorNamespace")
174+
assert.NotContains(t, sub.Annotations, reqNsOld+"."+reqNameA+"."+opNameV4+"/config")
165175

166176
// Test case 3: uninstallOperator is true, uninstallOperand is false for all namespace migration
167177
// where operator is uninstalled and will be reinstalled in new namespace and operand is not for old operator with v3 no-op installMode
@@ -227,8 +237,10 @@ func TestCheckSubAnnotationsForUninstall(t *testing.T) {
227237
Annotations: map[string]string{
228238
reqNsOld + "." + reqNameC + "." + opNameC + "/request": opChannelC,
229239
reqNsOld + "." + reqNameC + "." + opNameC + "/operatorNamespace": reqNsOld,
240+
reqNsOld + "." + reqNameC + "." + opNameC + "/config": opConfigC,
230241
reqNsOld + "." + reqNameD + "." + opNameD + "/request": opChannelD,
231242
reqNsOld + "." + reqNameD + "." + opNameD + "/operatorNamespace": reqNsOld,
243+
reqNsOld + "." + reqNameD + "." + opNameD + "/config": opConfigD,
232244
},
233245
Labels: map[string]string{
234246
constant.OpreqLabel: "true",
@@ -243,8 +255,10 @@ func TestCheckSubAnnotationsForUninstall(t *testing.T) {
243255

244256
assert.NotContains(t, sub.Annotations, reqNsOld+"."+reqNameC+"."+opNameC+"/request")
245257
assert.NotContains(t, sub.Annotations, reqNsOld+"."+reqNameC+"."+opNameC+"/operatorNamespace")
258+
assert.NotContains(t, sub.Annotations, reqNsOld+"."+reqNameC+"."+opNameC+"/config")
246259
assert.Contains(t, sub.Annotations, reqNsOld+"."+reqNameD+"."+opNameD+"/request")
247260
assert.Contains(t, sub.Annotations, reqNsOld+"."+reqNameD+"."+opNameD+"/operatorNamespace")
261+
assert.Contains(t, sub.Annotations, reqNsOld+"."+reqNameD+"."+opNameD+"/config")
248262

249263
// Test case 6: uninstallOperator is false, uninstallOperand is false for removing one no-op operand only for upgrade scenario
250264
// The operator should not be uninstalled because the remaining request is requesting operator is in the same namespace as the Subscription.
@@ -284,8 +298,10 @@ func TestCheckSubAnnotationsForUninstall(t *testing.T) {
284298
Annotations: map[string]string{
285299
reqNsNew + "." + reqNameA + "." + opNameV4 + "/request": opChannelV4,
286300
reqNsNew + "." + reqNameA + "." + opNameV4 + "/operatorNamespace": reqNsNew,
301+
reqNsNew + "." + reqNameA + "." + opNameV4 + "/config": opConfigV4,
287302
reqNsNew + "." + reqNameB + "." + opNameV4 + "/request": opChannelV4,
288303
reqNsNew + "." + reqNameB + "." + opNameV4 + "/operatorNamespace": reqNsNew,
304+
reqNsNew + "." + reqNameB + "." + opNameV4 + "/config": opConfigV4,
289305
},
290306
Labels: map[string]string{
291307
constant.OpreqLabel: "true",
@@ -300,6 +316,7 @@ func TestCheckSubAnnotationsForUninstall(t *testing.T) {
300316

301317
assert.NotContains(t, sub.Annotations, reqNsNew+"."+reqNameA+"."+opNameV4+"/request")
302318
assert.NotContains(t, sub.Annotations, reqNsNew+"."+reqNameA+"."+opNameV4+"/operatorNamespace")
319+
assert.NotContains(t, sub.Annotations, reqNsNew+"."+reqNameA+"."+opNameV4+"/config")
303320
assert.Contains(t, sub.Annotations, reqNsNew+"."+reqNameB+"."+opNameV4+"/request")
304321
assert.Contains(t, sub.Annotations, reqNsNew+"."+reqNameB+"."+opNameV4+"/operatorNamespace")
305322

@@ -312,6 +329,7 @@ func TestCheckSubAnnotationsForUninstall(t *testing.T) {
312329
Annotations: map[string]string{
313330
reqNsOld + "." + reqNameA + "." + opNameV4 + "/request": opChannelV4,
314331
reqNsOld + "." + reqNameA + "." + opNameV4 + "/operatorNamespace": reqNsOld,
332+
reqNsOld + "." + reqNameA + "." + opNameV4 + "/config": opConfigV4,
315333
},
316334
Labels: map[string]string{
317335
constant.OpreqLabel: "false",
@@ -326,4 +344,102 @@ func TestCheckSubAnnotationsForUninstall(t *testing.T) {
326344

327345
assert.NotContains(t, sub.Annotations, reqNsOld+"."+reqNameA+"."+opNameV4+"/request")
328346
assert.NotContains(t, sub.Annotations, reqNsOld+"."+reqNameA+"."+opNameV4+"/operatorNamespace")
347+
assert.NotContains(t, sub.Annotations, reqNsOld+"."+reqNameA+"."+opNameV4+"/config")
348+
349+
// Test case 9: Config name sharing test - operands should be preserved when config names match
350+
// When uninstalling one operator but another operator shares the same config name,
351+
// the operands should NOT be uninstalled since they're shared
352+
sub = &olmv1alpha1.Subscription{
353+
ObjectMeta: metav1.ObjectMeta{
354+
Namespace: reqNsOld,
355+
Annotations: map[string]string{
356+
reqNsOld + "." + reqNameA + "." + opNameV3 + "/request": opChannelV3,
357+
reqNsOld + "." + reqNameA + "." + opNameV3 + "/operatorNamespace": reqNsOld,
358+
reqNsOld + "." + reqNameA + "." + opNameV3 + "/config": opConfigV3,
359+
reqNsOld + "." + reqNameB + "." + opNameV4 + "/request": opChannelV4,
360+
reqNsOld + "." + reqNameB + "." + opNameV4 + "/operatorNamespace": reqNsOld,
361+
reqNsOld + "." + reqNameB + "." + opNameV4 + "/config": opConfigV3,
362+
},
363+
Labels: map[string]string{
364+
constant.OpreqLabel: "true",
365+
},
366+
},
367+
}
368+
369+
uninstallOperator, uninstallOperand = checkSubAnnotationsForUninstall(reqNameA, reqNsOld, opNameV3, operatorv1alpha1.InstallModeNamespace, sub)
370+
371+
assert.False(t, uninstallOperator)
372+
assert.False(t, uninstallOperand)
373+
374+
assert.NotContains(t, sub.Annotations, reqNsOld+"."+reqNameA+"."+opNameV3+"/request")
375+
assert.NotContains(t, sub.Annotations, reqNsOld+"."+reqNameA+"."+opNameV3+"/operatorNamespace")
376+
assert.NotContains(t, sub.Annotations, reqNsOld+"."+reqNameA+"."+opNameV3+"/config")
377+
assert.Contains(t, sub.Annotations, reqNsOld+"."+reqNameB+"."+opNameV4+"/request")
378+
assert.Contains(t, sub.Annotations, reqNsOld+"."+reqNameB+"."+opNameV4+"/operatorNamespace")
379+
assert.Contains(t, sub.Annotations, reqNsOld+"."+reqNameB+"."+opNameV4+"/config")
380+
381+
// Test case 10: Different config names - operands SHOULD be uninstalled
382+
// When uninstalling one operator and other operators have different config names,
383+
// the operands SHOULD be uninstalled
384+
sub = &olmv1alpha1.Subscription{
385+
ObjectMeta: metav1.ObjectMeta{
386+
Namespace: reqNsOld,
387+
Annotations: map[string]string{
388+
reqNsOld + "." + reqNameA + "." + opNameV3 + "/request": opChannelV3,
389+
reqNsOld + "." + reqNameA + "." + opNameV3 + "/operatorNamespace": reqNsOld,
390+
reqNsOld + "." + reqNameA + "." + opNameV3 + "/config": opConfigV3,
391+
reqNsOld + "." + reqNameB + "." + opNameV4 + "/request": opChannelV4,
392+
reqNsOld + "." + reqNameB + "." + opNameV4 + "/operatorNamespace": reqNsOld,
393+
reqNsOld + "." + reqNameB + "." + opNameV4 + "/config": opConfigV4,
394+
},
395+
Labels: map[string]string{
396+
constant.OpreqLabel: "true",
397+
},
398+
},
399+
}
400+
401+
uninstallOperator, uninstallOperand = checkSubAnnotationsForUninstall(reqNameA, reqNsOld, opNameV3, operatorv1alpha1.InstallModeNamespace, sub)
402+
403+
assert.False(t, uninstallOperator)
404+
assert.True(t, uninstallOperand)
405+
406+
assert.NotContains(t, sub.Annotations, reqNsOld+"."+reqNameA+"."+opNameV3+"/request")
407+
assert.NotContains(t, sub.Annotations, reqNsOld+"."+reqNameA+"."+opNameV3+"/operatorNamespace")
408+
assert.NotContains(t, sub.Annotations, reqNsOld+"."+reqNameA+"."+opNameV3+"/config")
409+
assert.Contains(t, sub.Annotations, reqNsOld+"."+reqNameB+"."+opNameV4+"/request")
410+
assert.Contains(t, sub.Annotations, reqNsOld+"."+reqNameB+"."+opNameV4+"/operatorNamespace")
411+
assert.Contains(t, sub.Annotations, reqNsOld+"."+reqNameB+"."+opNameV4+"/config")
412+
413+
// Test case 11: Same operator with different channels shared the same config name - operands should be preserved
414+
// When uninstalling one operator but another operator shares the same config name,
415+
// the operands should NOT be uninstalled since they're shared
416+
sub = &olmv1alpha1.Subscription{
417+
ObjectMeta: metav1.ObjectMeta{
418+
Namespace: reqNsOld,
419+
Annotations: map[string]string{
420+
reqNsOld + "." + reqNameA + "." + opNameV3 + "/request": opChannelV3,
421+
reqNsOld + "." + reqNameA + "." + opNameV3 + "/operatorNamespace": reqNsOld,
422+
reqNsOld + "." + reqNameA + "." + opNameV3 + "/config": opConfigV3,
423+
reqNsOld + "." + reqNameB + "." + opNameV4 + "/request": opChannelV4,
424+
reqNsOld + "." + reqNameB + "." + opNameV4 + "/operatorNamespace": reqNsOld,
425+
reqNsOld + "." + reqNameB + "." + opNameV4 + "/config": opConfigV3,
426+
},
427+
Labels: map[string]string{
428+
constant.OpreqLabel: "true",
429+
},
430+
},
431+
}
432+
433+
uninstallOperator, uninstallOperand = checkSubAnnotationsForUninstall(reqNameA, reqNsOld, opNameV3, operatorv1alpha1.InstallModeNamespace, sub)
434+
435+
assert.False(t, uninstallOperator)
436+
assert.False(t, uninstallOperand)
437+
438+
assert.NotContains(t, sub.Annotations, reqNsOld+"."+reqNameA+"."+opNameV3+"/request")
439+
assert.NotContains(t, sub.Annotations, reqNsOld+"."+reqNameA+"."+opNameV3+"/operatorNamespace")
440+
assert.NotContains(t, sub.Annotations, reqNsOld+"."+reqNameA+"."+opNameV3+"/config")
441+
assert.Contains(t, sub.Annotations, reqNsOld+"."+reqNameB+"."+opNameV4+"/request")
442+
assert.Contains(t, sub.Annotations, reqNsOld+"."+reqNameB+"."+opNameV4+"/operatorNamespace")
443+
assert.Contains(t, sub.Annotations, reqNsOld+"."+reqNameB+"."+opNameV4+"/config")
444+
329445
}

0 commit comments

Comments
 (0)