From afe2d6159e8ac0df1cef65d96486e83bb7323175 Mon Sep 17 00:00:00 2001 From: everettraven Date: Tue, 8 Oct 2024 15:35:31 -0400 Subject: [PATCH 1/4] (bugfix): reduce frequency of update requests for CSVs by adding annotations to copied CSVs that are populated with hashes of the non-status fields and the status fields. This seems to be how this was intended to work, but was not actually working this way because the annotations never actually existed on the copied CSV. This resulted in a hot loop of update requests being made on all copied CSVs. Signed-off-by: everettraven --- Makefile | 4 ++-- pkg/controller/operators/olm/operatorgroup.go | 22 +++++++++++++++---- 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/Makefile b/Makefile index db91104e2d..1410d0891e 100644 --- a/Makefile +++ b/Makefile @@ -63,7 +63,7 @@ export CONFIGMAP_SERVER_IMAGE ?= quay.io/operator-framework/configmap-operator-r PKG := github.com/operator-framework/operator-lifecycle-manager IMAGE_REPO ?= quay.io/operator-framework/olm -IMAGE_TAG ?= "dev" +IMAGE_TAG ?= dev # Go build settings # @@ -211,7 +211,7 @@ kind-create: kind-clean #HELP Create a new kind cluster $KIND_CLUSTER_NAME (defa $(KIND) export kubeconfig --name $(KIND_CLUSTER_NAME) .PHONY: deploy -OLM_IMAGE := quay.io/operator-framework/olm:local +OLM_IMAGE := $(IMAGE_REPO):$(IMAGE_TAG) deploy: $(KIND) $(HELM) #HELP Deploy OLM to kind cluster $KIND_CLUSTER_NAME (default: kind-olmv0) using $OLM_IMAGE (default: quay.io/operator-framework/olm:local) $(KIND) load docker-image $(OLM_IMAGE) --name $(KIND_CLUSTER_NAME); \ $(HELM) upgrade --install olm deploy/chart \ diff --git a/pkg/controller/operators/olm/operatorgroup.go b/pkg/controller/operators/olm/operatorgroup.go index 18c8b19008..ce2f596479 100644 --- a/pkg/controller/operators/olm/operatorgroup.go +++ b/pkg/controller/operators/olm/operatorgroup.go @@ -361,7 +361,7 @@ func (a *Operator) pruneProvidedAPIs(group *operatorsv1.OperatorGroup, groupProv } // Prune providedAPIs annotation if the cluster has fewer providedAPIs (handles CSV deletion) - //if intersection := groupProvidedAPIs.Intersection(providedAPIsFromCSVs); len(intersection) < len(groupProvidedAPIs) { + // if intersection := groupProvidedAPIs.Intersection(providedAPIsFromCSVs); len(intersection) < len(groupProvidedAPIs) { if len(intersection) < len(groupProvidedAPIs) { difference := groupProvidedAPIs.Difference(intersection) logger := logger.WithFields(logrus.Fields{ @@ -791,6 +791,11 @@ func copyableCSVHash(original *v1alpha1.ClusterServiceVersion) (string, string, return newHash, originalHash, nil } +const ( + nonStatusCopyHashAnnotation = "olm.operatorframework.io/nonStatusCopyHash" + statusCopyHashAnnotation = "olm.operatorframework.io/statusCopyHash" +) + // If returned error is not nil, the returned ClusterServiceVersion // has only the Name, Namespace, and UID fields set. func (a *Operator) copyToNamespace(prototype *v1alpha1.ClusterServiceVersion, nsFrom, nsTo, nonstatus, status string) (*v1alpha1.ClusterServiceVersion, error) { @@ -826,11 +831,14 @@ func (a *Operator) copyToNamespace(prototype *v1alpha1.ClusterServiceVersion, ns prototype.Namespace = existing.Namespace prototype.ResourceVersion = existing.ResourceVersion prototype.UID = existing.UID - existingNonStatus := existing.Annotations["$copyhash-nonstatus"] - existingStatus := existing.Annotations["$copyhash-status"] + // Get the non-status and status hash of the existing copied CSV + existingNonStatus := existing.Annotations[nonStatusCopyHashAnnotation] + existingStatus := existing.Annotations[statusCopyHashAnnotation] var updated *v1alpha1.ClusterServiceVersion if existingNonStatus != nonstatus { + // include updates to the non-status hash annotation if there is a mismatch + prototype.Annotations[nonStatusCopyHashAnnotation] = nonstatus if updated, err = a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).Update(context.TODO(), prototype, metav1.UpdateOptions{}); err != nil { return nil, fmt.Errorf("failed to update: %w", err) } @@ -844,6 +852,13 @@ func (a *Operator) copyToNamespace(prototype *v1alpha1.ClusterServiceVersion, ns if _, err = a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).UpdateStatus(context.TODO(), updated, metav1.UpdateOptions{}); err != nil { return nil, fmt.Errorf("failed to update status: %w", err) } + // Update the status first if the existing copied CSV status hash doesn't match what we expect + // to prevent a scenario where the hash annotations match but the contents do not. + // We also need to update the CSV itself in this case to ensure we set the status hash annotation. + prototype.Annotations[statusCopyHashAnnotation] = status + if updated, err = a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).Update(context.TODO(), prototype, metav1.UpdateOptions{}); err != nil { + return nil, fmt.Errorf("failed to update: %w", err) + } } return &v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ @@ -940,7 +955,6 @@ func namespacesChanged(clusterNamespaces []string, statusNamespaces []string) bo func (a *Operator) getOperatorGroupTargets(op *operatorsv1.OperatorGroup) (map[string]struct{}, error) { selector, err := metav1.LabelSelectorAsSelector(op.Spec.Selector) - if err != nil { return nil, err } From 71448586e716d324609b4a1321e66d3c8ffa3e3e Mon Sep 17 00:00:00 2001 From: everettraven Date: Tue, 8 Oct 2024 15:43:40 -0400 Subject: [PATCH 2/4] update unit tests Signed-off-by: everettraven --- .../operators/olm/operatorgroup_test.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/pkg/controller/operators/olm/operatorgroup_test.go b/pkg/controller/operators/olm/operatorgroup_test.go index bb328c72cc..9af54e449f 100644 --- a/pkg/controller/operators/olm/operatorgroup_test.go +++ b/pkg/controller/operators/olm/operatorgroup_test.go @@ -112,8 +112,8 @@ func TestCopyToNamespace(t *testing.T) { UID: "uid", ResourceVersion: "42", Annotations: map[string]string{ - "$copyhash-nonstatus": "hn-2", - "$copyhash-status": "hs", + nonStatusCopyHashAnnotation: "hn-2", + statusCopyHashAnnotation: "hs", }, }, }, @@ -165,8 +165,8 @@ func TestCopyToNamespace(t *testing.T) { UID: "uid", ResourceVersion: "42", Annotations: map[string]string{ - "$copyhash-nonstatus": "hn", - "$copyhash-status": "hs-2", + nonStatusCopyHashAnnotation: "hn", + statusCopyHashAnnotation: "hs-2", }, }, }, @@ -218,8 +218,8 @@ func TestCopyToNamespace(t *testing.T) { UID: "uid", ResourceVersion: "42", Annotations: map[string]string{ - "$copyhash-nonstatus": "hn-2", - "$copyhash-status": "hs-2", + nonStatusCopyHashAnnotation: "hn-2", + statusCopyHashAnnotation: "hs-2", }, }, }, @@ -278,8 +278,8 @@ func TestCopyToNamespace(t *testing.T) { Namespace: "to", UID: "uid", Annotations: map[string]string{ - "$copyhash-nonstatus": "hn", - "$copyhash-status": "hs", + nonStatusCopyHashAnnotation: "hn", + statusCopyHashAnnotation: "hs", }, }, }, From 38901d70061e7fda556451bdf5a89a44e270e78d Mon Sep 17 00:00:00 2001 From: everettraven Date: Tue, 8 Oct 2024 17:08:32 -0400 Subject: [PATCH 3/4] updates to test so far Signed-off-by: everettraven --- .github/workflows/e2e-tests.yml | 2 +- pkg/controller/operators/olm/operatorgroup.go | 5 ++++ .../operators/olm/operatorgroup_test.go | 24 ++++++++++++++++++- 3 files changed, 29 insertions(+), 2 deletions(-) diff --git a/.github/workflows/e2e-tests.yml b/.github/workflows/e2e-tests.yml index aa4a4c4bcd..ae51aaeb71 100644 --- a/.github/workflows/e2e-tests.yml +++ b/.github/workflows/e2e-tests.yml @@ -25,7 +25,7 @@ jobs: - name: Build controller image run: make e2e-build - name: Save image - run: docker save quay.io/operator-framework/olm:local -o olm-image.tar + run: docker save quay.io/operator-framework/olm:dev -o olm-image.tar - name: Upload Docker image as artifact uses: actions/upload-artifact@v4 with: diff --git a/pkg/controller/operators/olm/operatorgroup.go b/pkg/controller/operators/olm/operatorgroup.go index ce2f596479..e304984201 100644 --- a/pkg/controller/operators/olm/operatorgroup.go +++ b/pkg/controller/operators/olm/operatorgroup.go @@ -809,6 +809,7 @@ func (a *Operator) copyToNamespace(prototype *v1alpha1.ClusterServiceVersion, ns existing, err := a.copiedCSVLister.Namespace(nsTo).Get(prototype.GetName()) if apierrors.IsNotFound(err) { + prototype.Annotations[nonStatusCopyHashAnnotation] = nonstatus created, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).Create(context.TODO(), prototype, metav1.CreateOptions{}) if err != nil { return nil, fmt.Errorf("failed to create new CSV: %w", err) @@ -817,6 +818,10 @@ func (a *Operator) copyToNamespace(prototype *v1alpha1.ClusterServiceVersion, ns if _, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).UpdateStatus(context.TODO(), created, metav1.UpdateOptions{}); err != nil { return nil, fmt.Errorf("failed to update status on new CSV: %w", err) } + prototype.Annotations[statusCopyHashAnnotation] = status + if _, err = a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).Update(context.TODO(), prototype, metav1.UpdateOptions{}); err != nil { + return nil, fmt.Errorf("failed to update annotations after updating status: %w", err) + } return &v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ Name: created.Name, diff --git a/pkg/controller/operators/olm/operatorgroup_test.go b/pkg/controller/operators/olm/operatorgroup_test.go index 9af54e449f..025f696299 100644 --- a/pkg/controller/operators/olm/operatorgroup_test.go +++ b/pkg/controller/operators/olm/operatorgroup_test.go @@ -44,9 +44,12 @@ func TestCopyToNamespace(t *testing.T) { Name: "copy created if does not exist", FromNamespace: "from", ToNamespace: "to", + Hash: "hn-1", + StatusHash: "hs", Prototype: v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ Name: "name", + Annotations: map[string]string{}, }, Spec: v1alpha1.ClusterServiceVersionSpec{ Replaces: "replacee", @@ -60,6 +63,9 @@ func TestCopyToNamespace(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "name", Namespace: "to", + Annotations: map[string]string{ + nonStatusCopyHashAnnotation: "hn-1", + }, }, Spec: v1alpha1.ClusterServiceVersionSpec{ Replaces: "replacee", @@ -80,6 +86,13 @@ func TestCopyToNamespace(t *testing.T) { Phase: "waxing gibbous", }, }), + ktesting.NewUpdateAction(gvr, "to", &v1alpha1.ClusterServiceVersion{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + statusCopyHashAnnotation: "hs", + }, + }, + }) }, ExpectedResult: &v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ @@ -97,6 +110,7 @@ func TestCopyToNamespace(t *testing.T) { Prototype: v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ Name: "name", + Annotations: map[string]string{}, }, Spec: v1alpha1.ClusterServiceVersionSpec{ Replaces: "replacee", @@ -150,6 +164,7 @@ func TestCopyToNamespace(t *testing.T) { Prototype: v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ Name: "name", + Annotations: map[string]string{}, }, Spec: v1alpha1.ClusterServiceVersionSpec{ Replaces: "replacee", @@ -203,6 +218,7 @@ func TestCopyToNamespace(t *testing.T) { Prototype: v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ Name: "name", + Annotations: map[string]string{}, }, Spec: v1alpha1.ClusterServiceVersionSpec{ Replaces: "replacee", @@ -270,6 +286,7 @@ func TestCopyToNamespace(t *testing.T) { Prototype: v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ Name: "name", + Annotations: map[string]string{}, }, }, ExistingCopy: &metav1.PartialObjectMetadata{ @@ -311,10 +328,15 @@ func TestCopyToNamespace(t *testing.T) { logger: logger, } - result, err := o.copyToNamespace(tc.Prototype.DeepCopy(), tc.FromNamespace, tc.ToNamespace, tc.Hash, tc.StatusHash) + proto := tc.Prototype.DeepCopy() + result, err := o.copyToNamespace(proto, tc.FromNamespace, tc.ToNamespace, tc.Hash, tc.StatusHash) if tc.ExpectedError == nil { require.NoError(t, err) + // if there is no error expected, ensure that the hash annotations are always present on the resulting CSV + annotations := proto.GetObjectMeta().GetAnnotations() + require.Equal(t, tc.Hash, annotations[nonStatusCopyHashAnnotation]) + require.Equal(t, tc.StatusHash, annotations[statusCopyHashAnnotation]) } else { require.EqualError(t, err, tc.ExpectedError.Error()) } From 01343d61720cfd4f1e771ade533d2ae39b55fd05 Mon Sep 17 00:00:00 2001 From: Brett Tofel Date: Wed, 9 Oct 2024 10:14:21 -0400 Subject: [PATCH 4/4] Fix typo in operatorgroup_test.go Signed-off-by: Brett Tofel --- .../operators/olm/operatorgroup_test.go | 44 +++++++++---------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/pkg/controller/operators/olm/operatorgroup_test.go b/pkg/controller/operators/olm/operatorgroup_test.go index 025f696299..d2519a74f4 100644 --- a/pkg/controller/operators/olm/operatorgroup_test.go +++ b/pkg/controller/operators/olm/operatorgroup_test.go @@ -8,14 +8,14 @@ import ( "github.com/sirupsen/logrus/hooks/test" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "k8s.io/client-go/metadata/metadatalister" - "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" + "k8s.io/client-go/metadata/metadatalister" ktesting "k8s.io/client-go/testing" "github.com/operator-framework/api/pkg/operators/v1alpha1" + "github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/clientset/versioned/fake" ) @@ -48,8 +48,8 @@ func TestCopyToNamespace(t *testing.T) { StatusHash: "hs", Prototype: v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ - Name: "name", - Annotations: map[string]string{}, + Name: "name", + Annotations: map[string]string{}, }, Spec: v1alpha1.ClusterServiceVersionSpec{ Replaces: "replacee", @@ -63,9 +63,9 @@ func TestCopyToNamespace(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "name", Namespace: "to", - Annotations: map[string]string{ - nonStatusCopyHashAnnotation: "hn-1", - }, + Annotations: map[string]string{ + nonStatusCopyHashAnnotation: "hn-1", + }, }, Spec: v1alpha1.ClusterServiceVersionSpec{ Replaces: "replacee", @@ -86,13 +86,13 @@ func TestCopyToNamespace(t *testing.T) { Phase: "waxing gibbous", }, }), - ktesting.NewUpdateAction(gvr, "to", &v1alpha1.ClusterServiceVersion{ - ObjectMeta: metav1.ObjectMeta{ - Annotations: map[string]string{ - statusCopyHashAnnotation: "hs", - }, - }, - }) + ktesting.NewUpdateAction(gvr, "to", &v1alpha1.ClusterServiceVersion{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + statusCopyHashAnnotation: "hs", + }, + }, + }), }, ExpectedResult: &v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ @@ -109,8 +109,8 @@ func TestCopyToNamespace(t *testing.T) { StatusHash: "hs", Prototype: v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ - Name: "name", - Annotations: map[string]string{}, + Name: "name", + Annotations: map[string]string{}, }, Spec: v1alpha1.ClusterServiceVersionSpec{ Replaces: "replacee", @@ -163,8 +163,8 @@ func TestCopyToNamespace(t *testing.T) { StatusHash: "hs-1", Prototype: v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ - Name: "name", - Annotations: map[string]string{}, + Name: "name", + Annotations: map[string]string{}, }, Spec: v1alpha1.ClusterServiceVersionSpec{ Replaces: "replacee", @@ -217,8 +217,8 @@ func TestCopyToNamespace(t *testing.T) { StatusHash: "hs-1", Prototype: v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ - Name: "name", - Annotations: map[string]string{}, + Name: "name", + Annotations: map[string]string{}, }, Spec: v1alpha1.ClusterServiceVersionSpec{ Replaces: "replacee", @@ -285,8 +285,8 @@ func TestCopyToNamespace(t *testing.T) { StatusHash: "hs", Prototype: v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ - Name: "name", - Annotations: map[string]string{}, + Name: "name", + Annotations: map[string]string{}, }, }, ExistingCopy: &metav1.PartialObjectMetadata{