From f244097f564f6e61579bd1b7a0680f98623090f1 Mon Sep 17 00:00:00 2001 From: Alexander Greene Date: Tue, 1 Nov 2022 12:50:26 -0400 Subject: [PATCH] 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 Upstream-repository: operator-lifecycle-manager Upstream-commit: 793a7cc20d18f4907fc17ce2a0cebbca9a0f00be --- .../operators/decorators/operator.go | 18 +++++++++ .../operators/decorators/operator_test.go | 32 +++++++-------- .../operators/operator_controller.go | 9 +++-- .../operators/operator_controller_test.go | 39 +++++++++++++++++++ .../operators/decorators/operator.go | 18 +++++++++ .../operators/operator_controller.go | 9 +++-- 6 files changed, 103 insertions(+), 22 deletions(-) diff --git a/staging/operator-lifecycle-manager/pkg/controller/operators/decorators/operator.go b/staging/operator-lifecycle-manager/pkg/controller/operators/decorators/operator.go index f85d9ef790..56b9c92b67 100644 --- a/staging/operator-lifecycle-manager/pkg/controller/operators/decorators/operator.go +++ b/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 } diff --git a/staging/operator-lifecycle-manager/pkg/controller/operators/decorators/operator_test.go b/staging/operator-lifecycle-manager/pkg/controller/operators/decorators/operator_test.go index b438680ce1..eb99b7e9a0 100644 --- a/staging/operator-lifecycle-manager/pkg/controller/operators/decorators/operator_test.go +++ b/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 diff --git a/staging/operator-lifecycle-manager/pkg/controller/operators/operator_controller.go b/staging/operator-lifecycle-manager/pkg/controller/operators/operator_controller.go index a8e6cab804..e1b6accabf 100644 --- a/staging/operator-lifecycle-manager/pkg/controller/operators/operator_controller.go +++ b/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" @@ -152,9 +153,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 + } } } diff --git a/staging/operator-lifecycle-manager/pkg/controller/operators/operator_controller_test.go b/staging/operator-lifecycle-manager/pkg/controller/operators/operator_controller_test.go index 37caad0cbe..8f417378c1 100644 --- a/staging/operator-lifecycle-manager/pkg/controller/operators/operator_controller_test.go +++ b/staging/operator-lifecycle-manager/pkg/controller/operators/operator_controller_test.go @@ -2,6 +2,7 @@ package operators import ( "context" + "fmt" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -204,6 +205,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() { diff --git a/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/decorators/operator.go b/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/decorators/operator.go index f85d9ef790..56b9c92b67 100644 --- a/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/decorators/operator.go +++ b/vendor/github.com/operator-framework/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 } diff --git a/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/operator_controller.go b/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/operator_controller.go index a8e6cab804..e1b6accabf 100644 --- a/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/operator_controller.go +++ b/vendor/github.com/operator-framework/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" @@ -152,9 +153,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 + } } }