From 05c3a522b6692d2e5424f5db25d3f63d1040cee6 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 b2008dd3ea..d4ca5757fb 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" . "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 + } } }