Skip to content

Commit 121243a

Browse files
ecordellecordell
committed
fix(contraction): requeue any csv that may need it after operatorgroup
expansion/contraction
1 parent cc7265f commit 121243a

File tree

6 files changed

+284
-33
lines changed

6 files changed

+284
-33
lines changed

‎pkg/controller/operators/olm/operator.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1243,7 +1243,7 @@ func (a *Operator) isReplacing(in *v1alpha1.ClusterServiceVersion) *v1alpha1.Clu
12431243
}
12441244
previous, err := a.lister.OperatorsV1alpha1().ClusterServiceVersionLister().ClusterServiceVersions(in.GetNamespace()).Get(in.Spec.Replaces)
12451245
if err != nil {
1246-
a.Log.Debugf("unable to get previous csv: %s", err.Error())
1246+
a.Log.WithField("replacing", in.Spec.Replaces).WithError(err).Debugf("unable to get previous csv")
12471247
return nil
12481248
}
12491249
return previous

‎pkg/controller/operators/olm/operatorgroup.go

Lines changed: 21 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -72,8 +72,7 @@ func (a *Operator) syncOperatorGroups(obj interface{}) error {
7272
return err
7373
}
7474
logger.Debug("namespace change detected and operatorgroup status updated")
75-
// CSV requeue is handled by the succeeding sync
76-
75+
// CSV requeue is handled by the succeeding sync in `annotateCSVs`
7776
return nil
7877
}
7978

@@ -119,9 +118,14 @@ func (a *Operator) syncOperatorGroups(obj interface{}) error {
119118

120119
func (a *Operator) annotateCSVs(group *v1alpha2.OperatorGroup, targetNamespaces []string, logger *logrus.Entry) error {
121120
updateErrs := []error{}
121+
targetNamespaceSet := resolver.NewNamespaceSet(targetNamespaces)
122+
122123
for _, csv := range a.csvSet(group.GetNamespace(), v1alpha1.CSVPhaseAny) {
123124
logger := logger.WithField("csv", csv.GetName())
124125

126+
originalNamespacesAnnotation, _ := a.copyOperatorGroupAnnotations(&csv.ObjectMeta)[v1alpha2.OperatorGroupTargetsAnnotationKey]
127+
originalNamespaceSet := resolver.NewNamespaceSetFromString(originalNamespacesAnnotation)
128+
125129
if a.operatorGroupAnnotationsDiffer(&csv.ObjectMeta, group) {
126130
a.setOperatorGroupAnnotations(&csv.ObjectMeta, group, true)
127131
// CRDs don't support strategic merge patching, but in the future if they do this should be updated to patch
@@ -131,12 +135,21 @@ func (a *Operator) annotateCSVs(group *v1alpha2.OperatorGroup, targetNamespaces
131135
continue
132136
}
133137
}
134-
logger.WithField("targets", targetNamespaces).Debug("requeueing copied csvs in target namespaces")
135-
for _, ns := range targetNamespaces {
136-
_, err := a.lister.OperatorsV1alpha1().ClusterServiceVersionLister().ClusterServiceVersions(ns).Get(csv.GetName())
137-
if k8serrors.IsNotFound(err) {
138-
if err := a.csvQueueSet.Requeue(csv.GetName(), csv.GetNamespace()); err != nil {
139-
logger.WithError(err).Warn("could not requeue provider")
138+
139+
// requeue csvs in original namespaces or in new target namespaces (to capture removed/added namespaces)
140+
requeueNamespaces := originalNamespaceSet.Union(targetNamespaceSet)
141+
if !requeueNamespaces.IsAllNamespaces() {
142+
for ns := range requeueNamespaces {
143+
if err := a.csvQueueSet.Requeue(csv.GetName(), ns); err != nil {
144+
logger.WithError(err).Warn("could not requeue csv")
145+
}
146+
}
147+
}
148+
// have to requeue in all namespaces, previous or new targets were AllNamespaces
149+
if namespaces, err := a.lister.CoreV1().NamespaceLister().List(labels.Everything()); err != nil {
150+
for _, ns := range namespaces {
151+
if err := a.csvQueueSet.Requeue(csv.GetName(), ns.GetName()); err != nil {
152+
logger.WithError(err).Warn("could not requeue csv")
140153
}
141154
}
142155
}

‎pkg/controller/registry/resolver/groups.go

Lines changed: 28 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -34,13 +34,13 @@ func (n NamespaceSet) Peek() string {
3434
func (n NamespaceSet) Intersection(set NamespaceSet) NamespaceSet {
3535
intersection := make(NamespaceSet)
3636
// Handle special NamespaceAll cases
37-
if len(n) == 1 && n.Peek() == "" {
37+
if n.IsAllNamespaces() {
3838
for namespace := range set {
3939
intersection[namespace] = struct{}{}
4040
}
4141
return intersection
4242
}
43-
if len(set) == 1 && set.Peek() == "" {
43+
if set.IsAllNamespaces() {
4444
for namespace := range n {
4545
intersection[namespace] = struct{}{}
4646
}
@@ -56,14 +56,39 @@ func (n NamespaceSet) Intersection(set NamespaceSet) NamespaceSet {
5656
return intersection
5757
}
5858

59+
func (n NamespaceSet) Union(set NamespaceSet) NamespaceSet {
60+
// Handle special NamespaceAll cases
61+
if n.IsAllNamespaces() {
62+
return n
63+
}
64+
if set.IsAllNamespaces() {
65+
return set
66+
}
67+
union := make(NamespaceSet)
68+
for namespace := range n {
69+
union[namespace] = struct{}{}
70+
}
71+
for namespace := range set {
72+
union[namespace] = struct{}{}
73+
}
74+
return union
75+
}
76+
5977
func (n NamespaceSet) Contains(namespace string) bool {
60-
if len(n) == 1 && n.Peek() == "" {
78+
if n.IsAllNamespaces() {
6179
return true
6280
}
6381
_, ok := n[namespace]
6482
return ok
6583
}
6684

85+
func (n NamespaceSet) IsAllNamespaces() bool {
86+
if len(n) == 1 && n.Peek() == "" {
87+
return true
88+
}
89+
return false
90+
}
91+
6792
type OperatorGroupSurface interface {
6893
Identifier() string
6994
Namespace() string

‎pkg/controller/registry/resolver/groups_test.go

Lines changed: 223 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -336,6 +336,229 @@ func TestNamespaceSetIntersection(t *testing.T) {
336336
}
337337
}
338338

339+
func TestNamespaceSetUnion(t *testing.T) {
340+
type input struct {
341+
left NamespaceSet
342+
right NamespaceSet
343+
}
344+
tests := []struct {
345+
name string
346+
in input
347+
want NamespaceSet
348+
}{
349+
{
350+
name: "EmptySets",
351+
in: input{
352+
left: make(NamespaceSet),
353+
right: make(NamespaceSet),
354+
},
355+
want: make(NamespaceSet),
356+
},
357+
{
358+
name: "EmptyLeft/MultipleRight",
359+
in: input{
360+
left: make(NamespaceSet),
361+
right: NamespaceSet{
362+
"ns": {},
363+
"ns-1": {},
364+
"ns-2": {},
365+
},
366+
},
367+
want: NamespaceSet{
368+
"ns": {},
369+
"ns-1": {},
370+
"ns-2": {},
371+
},
372+
},
373+
{
374+
name: "MultipleLeft/EmptyRight",
375+
in: input{
376+
left: NamespaceSet{
377+
"ns": {},
378+
"ns-1": {},
379+
"ns-2": {},
380+
},
381+
right: make(NamespaceSet),
382+
},
383+
want: NamespaceSet{
384+
"ns": {},
385+
"ns-1": {},
386+
"ns-2": {},
387+
},
388+
},
389+
{
390+
name: "OneLeft/SameRight",
391+
in: input{
392+
left: NamespaceSet{
393+
"ns": {},
394+
},
395+
right: NamespaceSet{
396+
"ns": {},
397+
},
398+
},
399+
want: NamespaceSet{
400+
"ns": {},
401+
},
402+
},
403+
{
404+
name: "MultipleLeft/MultipleRight/Differ",
405+
in: input{
406+
left: NamespaceSet{
407+
"ns": {},
408+
"ns-1": {},
409+
"ns-2": {},
410+
},
411+
right: NamespaceSet{
412+
"ns": {},
413+
"ns-1": {},
414+
"ns-3": {},
415+
},
416+
},
417+
want: NamespaceSet{
418+
"ns": {},
419+
"ns-1": {},
420+
"ns-2": {},
421+
"ns-3": {},
422+
},
423+
},
424+
{
425+
name: "MultipleLeft/MultipleRight/AllSame",
426+
in: input{
427+
left: NamespaceSet{
428+
"ns": {},
429+
"ns-1": {},
430+
"ns-2": {},
431+
},
432+
right: NamespaceSet{
433+
"ns": {},
434+
"ns-1": {},
435+
"ns-2": {},
436+
},
437+
},
438+
want: NamespaceSet{
439+
"ns": {},
440+
"ns-1": {},
441+
"ns-2": {},
442+
},
443+
},
444+
{
445+
name: "AllLeft/MultipleRight",
446+
in: input{
447+
left: NamespaceSet{
448+
"": {},
449+
},
450+
right: NamespaceSet{
451+
"ns": {},
452+
"ns-1": {},
453+
"ns-2": {},
454+
},
455+
},
456+
want: NamespaceSet{
457+
"": {},
458+
},
459+
},
460+
{
461+
name: "MultipleLeft/AllRight",
462+
in: input{
463+
left: NamespaceSet{
464+
"ns": {},
465+
"ns-1": {},
466+
"ns-2": {},
467+
},
468+
right: NamespaceSet{
469+
"": {},
470+
},
471+
},
472+
want: NamespaceSet{
473+
"": {},
474+
},
475+
},
476+
{
477+
name: "AllLeft/EmptyRight",
478+
in: input{
479+
left: NamespaceSet{
480+
"": {},
481+
},
482+
right: make(NamespaceSet),
483+
},
484+
want: NamespaceSet{
485+
"": {},
486+
},
487+
},
488+
{
489+
name: "EmptyLeft/AllRight",
490+
in: input{
491+
left: make(NamespaceSet),
492+
right: NamespaceSet{
493+
"": {},
494+
},
495+
},
496+
want: NamespaceSet{
497+
"": {},
498+
},
499+
},
500+
{
501+
name: "AllLeft/AllRight",
502+
in: input{
503+
left: NamespaceSet{
504+
"": {},
505+
},
506+
right: NamespaceSet{
507+
"": {},
508+
},
509+
},
510+
want: NamespaceSet{
511+
"": {},
512+
},
513+
},
514+
}
515+
516+
for _, tt := range tests {
517+
t.Run(tt.name, func(t *testing.T) {
518+
require.EqualValues(t, tt.want, tt.in.left.Union(tt.in.right))
519+
})
520+
}
521+
}
522+
523+
func TestNamespaceSetIsAllNamespaces(t *testing.T) {
524+
type input struct {
525+
set NamespaceSet
526+
}
527+
tests := []struct {
528+
name string
529+
in input
530+
want bool
531+
}{
532+
{
533+
name: "All/Yes",
534+
in: input{
535+
set: NewNamespaceSet([]string{metav1.NamespaceAll}),
536+
},
537+
want: true,
538+
},
539+
{
540+
name: "One/NotAll",
541+
in: input{
542+
set: NewNamespaceSet([]string{"a"}),
543+
},
544+
want: false,
545+
},
546+
{
547+
name: "Many/NotAll",
548+
in: input{
549+
set: NewNamespaceSet([]string{"a", "b", "c"}),
550+
},
551+
want: false,
552+
},
553+
}
554+
555+
for _, tt := range tests {
556+
t.Run(tt.name, func(t *testing.T) {
557+
require.Equal(t, tt.want, tt.in.set.IsAllNamespaces())
558+
})
559+
}
560+
}
561+
339562
func TestNamespaceSetContains(t *testing.T) {
340563
type input struct {
341564
set NamespaceSet

0 commit comments

Comments
 (0)