Order an operator CR's status.Component.Refs array (#2880)

Problem: The operator CR's status includes a list of componenets owned
by the operator. The list of components are ordered by GVK, but the
order of objects with the same GVK may change. If an operator owns many
components, there is a high likelyhood that OLM will continuously
update the status of the operator CR because the order of its
components have changed,

Solution: Order an operators component references so OLM does not
attempt to update the status of the operator CR.

Signed-off-by: Alexander Greene <[email protected]>

Upstream-repository: operator-lifecycle-manager
Upstream-commit: 793a7cc20d18f4907fc17ce2a0cebbca9a0f00be

Author: awgreene

staging/operator-lifecycle-manager/pkg/controller/operators/decorators/operator.go

@@ -2,6 +2,7 @@ package decorators
 
 import (
 	"fmt"
+	"sort"
 	"strings"
 
 	"github.com/itchyny/gojq"
@@ -331,6 +332,23 @@ func (o *Operator) AddComponents(components ...runtime.Object) error {
 
 	o.Status.Components.Refs = append(o.Status.Components.Refs, refs...)
 
+	// Sort the component refs to so subsequent reconciles of the object do not change
+	// the status and result in an update to the object.
+	sort.SliceStable(o.Status.Components.Refs, func(i, j int) bool {
+		if o.Status.Components.Refs[i].Kind != o.Status.Components.Refs[j].Kind {
+			return o.Status.Components.Refs[i].Kind < o.Status.Components.Refs[j].Kind
+		}
+
+		if o.Status.Components.Refs[i].APIVersion != o.Status.Components.Refs[j].APIVersion {
+			return o.Status.Components.Refs[i].APIVersion < o.Status.Components.Refs[j].APIVersion
+		}
+
+		if o.Status.Components.Refs[i].Namespace != o.Status.Components.Refs[j].Namespace {
+			return o.Status.Components.Refs[i].Namespace < o.Status.Components.Refs[j].Namespace
+		}
+		return o.Status.Components.Refs[i].Name < o.Status.Components.Refs[j].Name
+	})
+
 	return nil
 }
 

staging/operator-lifecycle-manager/pkg/controller/operators/decorators/operator_test.go

@@ -174,6 +174,22 @@ func TestAddComponents(t *testing.T) {
 						},
 					}
 					operator.Status.Components.Refs = []operatorsv1.RichReference{
+						{
+							ObjectReference: &corev1.ObjectReference{
+								APIVersion: operatorsv1alpha1.SchemeGroupVersion.String(),
+								Kind:       operatorsv1alpha1.ClusterServiceVersionKind,
+								Namespace:  "atlantic",
+								Name:       "puffin",
+							},
+							Conditions: []operatorsv1.Condition{
+								{
+									Type:    operatorsv1.ConditionType(operatorsv1alpha1.CSVPhaseSucceeded),
+									Status:  corev1.ConditionTrue,
+									Reason:  string(operatorsv1alpha1.CSVReasonInstallSuccessful),
+									Message: "this puffin is happy",
+								},
+							},
+						},
 						{
 							ObjectReference: &corev1.ObjectReference{
 								APIVersion: "v1",
@@ -195,22 +211,6 @@ func TestAddComponents(t *testing.T) {
 								},
 							},
 						},
-						{
-							ObjectReference: &corev1.ObjectReference{
-								APIVersion: operatorsv1alpha1.SchemeGroupVersion.String(),
-								Kind:       operatorsv1alpha1.ClusterServiceVersionKind,
-								Namespace:  "atlantic",
-								Name:       "puffin",
-							},
-							Conditions: []operatorsv1.Condition{
-								{
-									Type:    operatorsv1.ConditionType(operatorsv1alpha1.CSVPhaseSucceeded),
-									Status:  corev1.ConditionTrue,
-									Reason:  string(operatorsv1alpha1.CSVReasonInstallSuccessful),
-									Message: "this puffin is happy",
-								},
-							},
-						},
 					}
 
 					return operator

staging/operator-lifecycle-manager/pkg/controller/operators/operator_controller.go

@@ -9,6 +9,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
+	"k8s.io/apimachinery/pkg/api/equality"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
 	"k8s.io/apimachinery/pkg/labels"
@@ -151,9 +152,11 @@ func (r *OperatorReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c
 			return ctrl.Result{Requeue: true}, nil
 		}
 	} else {
-		if err := r.Status().Update(ctx, operator.Operator); err != nil {
-			log.Error(err, "Could not update Operator status")
-			return ctrl.Result{Requeue: true}, nil
+		if !equality.Semantic.DeepEqual(in.Status, operator.Operator.Status) {
+			if err := r.Status().Update(ctx, operator.Operator); err != nil {
+				log.Error(err, "Could not update Operator status")
+				return ctrl.Result{Requeue: true}, nil
+			}
 		}
 	}
 

staging/operator-lifecycle-manager/pkg/controller/operators/operator_controller_test.go

@@ -2,6 +2,7 @@ package operators
 
 import (
 	"context"
+	"fmt"
 
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
@@ -200,6 +201,44 @@ var _ = Describe("Operator Controller", func() {
 				})
 			})
 
+			Context("when multiple types of a gvk are labeled", func() {
+				BeforeEach(func() {
+					newObjs := make([]runtime.Object, 9)
+
+					// Create objects in reverse order to ensure they are eventually ordered alphabetically in the status of the operator.
+					for i := 8; i >= 0; i-- {
+						newObjs[8-i] = testobj.WithLabels(map[string]string{expectedKey: ""}, testobj.WithNamespacedName(
+							&types.NamespacedName{Namespace: namespace, Name: fmt.Sprintf("sa-%d", i)}, &corev1.ServiceAccount{},
+						))
+					}
+
+					for _, obj := range newObjs {
+						Expect(k8sClient.Create(ctx, obj.(client.Object))).To(Succeed())
+					}
+
+					objs = append(objs, newObjs...)
+					expectedRefs = append(expectedRefs, toRefs(scheme, newObjs...)...)
+				})
+
+				It("should list each of the component references in alphabetical order by namespace and name", func() {
+					Eventually(func() ([]operatorsv1.RichReference, error) {
+						err := k8sClient.Get(ctx, name, operator)
+						return operator.Status.Components.Refs, err
+					}, timeout, interval).Should(ConsistOf(expectedRefs))
+
+					serviceAccountCount := 0
+					for _, ref := range operator.Status.Components.Refs {
+						if ref.Kind != rbacv1.ServiceAccountKind {
+							continue
+						}
+
+						Expect(ref.Name).Should(Equal(fmt.Sprintf("sa-%d", serviceAccountCount)))
+						serviceAccountCount++
+					}
+					Expect(serviceAccountCount).To(Equal(9))
+				})
+			})
+
 			Context("when component labels are removed", func() {
 				BeforeEach(func() {
 					for _, obj := range testobj.StripLabel(expectedKey, objs...) {