(bugfix): reduce frequency of update requests for copied CSVs

.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:

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 \

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) {
@@ -804,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)
@@ -812,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,
@@ -826,11 +836,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 +857,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 +960,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
 	}

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"
 )
 
@@ -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",
+					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{
@@ -96,7 +109,8 @@ func TestCopyToNamespace(t *testing.T) {
 			StatusHash:    "hs",
 			Prototype: v1alpha1.ClusterServiceVersion{
 				ObjectMeta: metav1.ObjectMeta{
-					Name: "name",
+					Name:        "name",
+					Annotations: map[string]string{},
 				},
 				Spec: v1alpha1.ClusterServiceVersionSpec{
 					Replaces: "replacee",
@@ -112,8 +126,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",
 					},
 				},
 			},
@@ -149,7 +163,8 @@ func TestCopyToNamespace(t *testing.T) {
 			StatusHash:    "hs-1",
 			Prototype: v1alpha1.ClusterServiceVersion{
 				ObjectMeta: metav1.ObjectMeta{
-					Name: "name",
+					Name:        "name",
+					Annotations: map[string]string{},
 				},
 				Spec: v1alpha1.ClusterServiceVersionSpec{
 					Replaces: "replacee",
@@ -165,8 +180,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",
 					},
 				},
 			},
@@ -202,7 +217,8 @@ func TestCopyToNamespace(t *testing.T) {
 			StatusHash:    "hs-1",
 			Prototype: v1alpha1.ClusterServiceVersion{
 				ObjectMeta: metav1.ObjectMeta{
-					Name: "name",
+					Name:        "name",
+					Annotations: map[string]string{},
 				},
 				Spec: v1alpha1.ClusterServiceVersionSpec{
 					Replaces: "replacee",
@@ -218,8 +234,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",
 					},
 				},
 			},
@@ -269,7 +285,8 @@ func TestCopyToNamespace(t *testing.T) {
 			StatusHash:    "hs",
 			Prototype: v1alpha1.ClusterServiceVersion{
 				ObjectMeta: metav1.ObjectMeta{
-					Name: "name",
+					Name:        "name",
+					Annotations: map[string]string{},
 				},
 			},
 			ExistingCopy: &metav1.PartialObjectMetadata{
@@ -278,8 +295,8 @@ func TestCopyToNamespace(t *testing.T) {
 					Namespace: "to",
 					UID:       "uid",
 					Annotations: map[string]string{
-						"$copyhash-nonstatus": "hn",
-						"$copyhash-status":    "hs",
+						nonStatusCopyHashAnnotation: "hn",
+						statusCopyHashAnnotation:    "hs",
 					},
 				},
 			},
@@ -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())
 			}