From f0b7776cd5d97c5e233ddb8ba449c45937284c04 Mon Sep 17 00:00:00 2001 From: everettraven Date: Tue, 8 Oct 2024 15:35:31 -0400 Subject: [PATCH 01/26] (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 7ef00ffafc..fa27dd405c 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 # @@ -228,7 +228,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 94e17b6ae4..60c93a4ee1 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 8db54cc89b0e6b003d4e25eee54c487bf1b1837e Mon Sep 17 00:00:00 2001 From: everettraven Date: Tue, 8 Oct 2024 15:43:40 -0400 Subject: [PATCH 02/26] 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 628e9ec3dd..f6fdb31df4 100644 --- a/pkg/controller/operators/olm/operatorgroup_test.go +++ b/pkg/controller/operators/olm/operatorgroup_test.go @@ -114,8 +114,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", }, }, }, @@ -167,8 +167,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", }, }, }, @@ -220,8 +220,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", }, }, }, @@ -280,8 +280,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 4e91464e30325d7eac1b9e659f63b055027fdba2 Mon Sep 17 00:00:00 2001 From: everettraven Date: Tue, 8 Oct 2024 17:08:32 -0400 Subject: [PATCH 03/26] 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 8dd4ff9b91..c4b134b6e2 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 60c93a4ee1..fda2da0c83 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 f6fdb31df4..526f330e77 100644 --- a/pkg/controller/operators/olm/operatorgroup_test.go +++ b/pkg/controller/operators/olm/operatorgroup_test.go @@ -46,9 +46,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", @@ -62,6 +65,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", @@ -82,6 +88,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{ @@ -99,6 +112,7 @@ func TestCopyToNamespace(t *testing.T) { Prototype: v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ Name: "name", + Annotations: map[string]string{}, }, Spec: v1alpha1.ClusterServiceVersionSpec{ Replaces: "replacee", @@ -152,6 +166,7 @@ func TestCopyToNamespace(t *testing.T) { Prototype: v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ Name: "name", + Annotations: map[string]string{}, }, Spec: v1alpha1.ClusterServiceVersionSpec{ Replaces: "replacee", @@ -205,6 +220,7 @@ func TestCopyToNamespace(t *testing.T) { Prototype: v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ Name: "name", + Annotations: map[string]string{}, }, Spec: v1alpha1.ClusterServiceVersionSpec{ Replaces: "replacee", @@ -272,6 +288,7 @@ func TestCopyToNamespace(t *testing.T) { Prototype: v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ Name: "name", + Annotations: map[string]string{}, }, }, ExistingCopy: &metav1.PartialObjectMetadata{ @@ -313,10 +330,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 5cbee7bc2c417beec694c75af3925fe86e603530 Mon Sep 17 00:00:00 2001 From: Brett Tofel Date: Wed, 9 Oct 2024 10:14:21 -0400 Subject: [PATCH 04/26] Small changes Signed-off-by: Brett Tofel --- .github/workflows/e2e-tests.yml | 2 +- Makefile | 4 +- pkg/controller/operators/olm/operator.go | 5 +- pkg/controller/operators/olm/operator_test.go | 25 +- pkg/controller/operators/olm/operatorgroup.go | 7 +- .../operators/olm/operatorgroup_test.go | 249 ++++++++++++++---- 6 files changed, 229 insertions(+), 63 deletions(-) diff --git a/.github/workflows/e2e-tests.yml b/.github/workflows/e2e-tests.yml index c4b134b6e2..8dd4ff9b91 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:dev -o olm-image.tar + run: docker save quay.io/operator-framework/olm:local -o olm-image.tar - name: Upload Docker image as artifact uses: actions/upload-artifact@v4 with: diff --git a/Makefile b/Makefile index fa27dd405c..7ef00ffafc 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 # @@ -228,7 +228,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 := $(IMAGE_REPO):$(IMAGE_TAG) +OLM_IMAGE := quay.io/operator-framework/olm:local 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/operator.go b/pkg/controller/operators/olm/operator.go index 2bb23a555e..f0841953ab 100644 --- a/pkg/controller/operators/olm/operator.go +++ b/pkg/controller/operators/olm/operator.go @@ -8,8 +8,6 @@ import ( "sync" "time" - "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/labeller" - "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/olm/plugins" "github.com/sirupsen/logrus" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" @@ -42,11 +40,14 @@ import ( operatorsv1 "github.com/operator-framework/api/pkg/operators/v1" "github.com/operator-framework/api/pkg/operators/v1alpha1" + "github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/clientset/versioned" "github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/informers/externalversions" "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/certs" "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/install" + "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/labeller" "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/olm/overrides" + "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/olm/plugins" "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/registry/resolver" "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/clients" csvutility "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/csv" diff --git a/pkg/controller/operators/olm/operator_test.go b/pkg/controller/operators/olm/operator_test.go index a28a67bdd4..c117cbffb6 100644 --- a/pkg/controller/operators/olm/operator_test.go +++ b/pkg/controller/operators/olm/operator_test.go @@ -20,7 +20,6 @@ import ( "github.com/google/go-cmp/cmp" configfake "github.com/openshift/client-go/config/clientset/versioned/fake" - hashutil "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/kubernetes/pkg/util/hash" "github.com/sirupsen/logrus" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -44,6 +43,7 @@ import ( metadatafake "k8s.io/client-go/metadata/fake" "k8s.io/client-go/pkg/version" "k8s.io/client-go/rest" + clienttesting "k8s.io/client-go/testing" "k8s.io/client-go/tools/cache" "k8s.io/client-go/tools/record" apiregistrationv1 "k8s.io/kube-aggregator/pkg/apis/apiregistration/v1" @@ -54,7 +54,6 @@ import ( operatorsv1 "github.com/operator-framework/api/pkg/operators/v1" "github.com/operator-framework/api/pkg/operators/v1alpha1" opregistry "github.com/operator-framework/operator-registry/pkg/registry" - clienttesting "k8s.io/client-go/testing" "github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/clientset/versioned" "github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/clientset/versioned/fake" @@ -64,6 +63,7 @@ import ( resolvercache "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/registry/resolver/cache" "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/clientfake" csvutility "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/csv" + hashutil "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/kubernetes/pkg/util/hash" "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/labeler" "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorclient" "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorlister" @@ -5050,7 +5050,12 @@ func TestSyncOperatorGroups(t *testing.T) { }, targetNamespace: { withLabels( - withAnnotations(targetCSV.DeepCopy(), map[string]string{operatorsv1.OperatorGroupAnnotationKey: "operator-group-1", operatorsv1.OperatorGroupNamespaceAnnotationKey: operatorNamespace}), + withAnnotations(targetCSV.DeepCopy(), map[string]string{ + operatorsv1.OperatorGroupAnnotationKey: "operator-group-1", + operatorsv1.OperatorGroupNamespaceAnnotationKey: operatorNamespace, + "olm.operatorframework.io/nonStatusCopyHash": "9ZxHAHQTkxzAQd7Qkk4Qjz3VAkA8lXwuX9mDX6", + "olm.operatorframework.io/statusCopyHash": "bedtcmN999WBSJ1RHvM7JfN2NJITrUjJ0g0MoH", + }), labels.Merge(targetCSV.GetLabels(), map[string]string{v1alpha1.CopiedLabelKey: operatorNamespace}), ), &rbacv1.Role{ @@ -5155,7 +5160,12 @@ func TestSyncOperatorGroups(t *testing.T) { }, targetNamespace: { withLabels( - withAnnotations(targetCSV.DeepCopy(), map[string]string{operatorsv1.OperatorGroupAnnotationKey: "operator-group-1", operatorsv1.OperatorGroupNamespaceAnnotationKey: operatorNamespace}), + withAnnotations(targetCSV.DeepCopy(), map[string]string{ + operatorsv1.OperatorGroupAnnotationKey: "operator-group-1", + operatorsv1.OperatorGroupNamespaceAnnotationKey: operatorNamespace, + "olm.operatorframework.io/nonStatusCopyHash": "9ZxHAHQTkxzAQd7Qkk4Qjz3VAkA8lXwuX9mDX6", + "olm.operatorframework.io/statusCopyHash": "bedtcmN999WBSJ1RHvM7JfN2NJITrUjJ0g0MoH", + }), labels.Merge(targetCSV.GetLabels(), map[string]string{v1alpha1.CopiedLabelKey: operatorNamespace}), ), &rbacv1.Role{ @@ -5312,7 +5322,12 @@ func TestSyncOperatorGroups(t *testing.T) { }, targetNamespace: { withLabels( - withAnnotations(targetCSV.DeepCopy(), map[string]string{operatorsv1.OperatorGroupAnnotationKey: "operator-group-1", operatorsv1.OperatorGroupNamespaceAnnotationKey: operatorNamespace}), + withAnnotations(targetCSV.DeepCopy(), map[string]string{ + operatorsv1.OperatorGroupAnnotationKey: "operator-group-1", + operatorsv1.OperatorGroupNamespaceAnnotationKey: operatorNamespace, + "olm.operatorframework.io/nonStatusCopyHash": "9ZxHAHQTkxzAQd7Qkk4Qjz3VAkA8lXwuX9mDX6", + "olm.operatorframework.io/statusCopyHash": "bedtcmN999WBSJ1RHvM7JfN2NJITrUjJ0g0MoH", + }), labels.Merge(targetCSV.GetLabels(), map[string]string{v1alpha1.CopiedLabelKey: operatorNamespace}), ), }, diff --git a/pkg/controller/operators/olm/operatorgroup.go b/pkg/controller/operators/olm/operatorgroup.go index fda2da0c83..7099053f19 100644 --- a/pkg/controller/operators/olm/operatorgroup.go +++ b/pkg/controller/operators/olm/operatorgroup.go @@ -841,9 +841,10 @@ func (a *Operator) copyToNamespace(prototype *v1alpha1.ClusterServiceVersion, ns existingStatus := existing.Annotations[statusCopyHashAnnotation] var updated *v1alpha1.ClusterServiceVersion + // Always set the in-memory prototype's nonstatus annotation: + prototype.Annotations[nonStatusCopyHashAnnotation] = nonstatus 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) } @@ -864,6 +865,10 @@ func (a *Operator) copyToNamespace(prototype *v1alpha1.ClusterServiceVersion, ns 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) } + } else { + // Even if they're the same, ensure the returned prototype is annotated. + prototype.Annotations[statusCopyHashAnnotation] = status + updated = prototype } return &v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ diff --git a/pkg/controller/operators/olm/operatorgroup_test.go b/pkg/controller/operators/olm/operatorgroup_test.go index 526f330e77..617a0f673e 100644 --- a/pkg/controller/operators/olm/operatorgroup_test.go +++ b/pkg/controller/operators/olm/operatorgroup_test.go @@ -16,11 +16,66 @@ import ( 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" "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/registry/resolver/cache" "github.com/operator-framework/operator-registry/pkg/registry" ) +// fakeCSVNamespaceLister implements metadatalister.NamespaceLister +type fakeCSVNamespaceLister struct { + namespace string + items []*metav1.PartialObjectMetadata +} + +func (n *fakeCSVNamespaceLister) List(selector labels.Selector) ([]*metav1.PartialObjectMetadata, error) { + var result []*metav1.PartialObjectMetadata + for _, item := range n.items { + if item != nil && item.Namespace == n.namespace { + result = append(result, item) + } + } + return result, nil +} + +func (n *fakeCSVNamespaceLister) Get(name string) (*metav1.PartialObjectMetadata, error) { + for _, item := range n.items { + if item != nil && item.Namespace == n.namespace && item.Name == name { + return item, nil + } + } + return nil, errors.NewNotFound(v1alpha1.Resource("clusterserviceversion"), name) +} + +// fakeCSVLister implements the full metadatalister.Lister interface +// so that Operator.copiedCSVLister = &fakeCSVLister{...} works. +type fakeCSVLister struct { + items []*metav1.PartialObjectMetadata +} + +// List returns all CSV metadata items, ignoring namespaces. +func (f *fakeCSVLister) List(selector labels.Selector) ([]*metav1.PartialObjectMetadata, error) { + return f.items, nil +} + +// Get returns the CSV by name, ignoring namespaces. +func (f *fakeCSVLister) Get(name string) (*metav1.PartialObjectMetadata, error) { + for _, item := range f.items { + if item != nil && item.Name == name { + return item, nil + } + } + return nil, errors.NewNotFound(v1alpha1.Resource("clusterserviceversion"), name) +} + +// Namespace returns a namespace-scoped lister wrapper. +func (f *fakeCSVLister) Namespace(ns string) metadatalister.NamespaceLister { + return &fakeCSVNamespaceLister{ + namespace: ns, + items: f.items, + } +} + func TestCopyToNamespace(t *testing.T) { gvr := v1alpha1.SchemeGroupVersion.WithResource("clusterserviceversions") @@ -50,8 +105,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", @@ -60,14 +115,16 @@ func TestCopyToNamespace(t *testing.T) { Phase: "waxing gibbous", }, }, + // No ExistingCopy: means there's nothing in "to" namespace yet. ExpectedActions: []ktesting.Action{ + // Create the new CSV with nonStatusCopyHashAnnotation ktesting.NewCreateAction(gvr, "to", &v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ Name: "name", Namespace: "to", - Annotations: map[string]string{ - nonStatusCopyHashAnnotation: "hn-1", - }, + Annotations: map[string]string{ + "olm.operatorframework.io/nonStatusCopyHash": "hn-1", + }, }, Spec: v1alpha1.ClusterServiceVersionSpec{ Replaces: "replacee", @@ -76,10 +133,33 @@ func TestCopyToNamespace(t *testing.T) { Phase: "waxing gibbous", }, }), + // UpdateStatus: note that name/namespace remain "name"/"to", + // and we still have nonStatusCopyHashAnnotation: "hn-1". ktesting.NewUpdateSubresourceAction(gvr, "status", "to", &v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ Name: "name", Namespace: "to", + Annotations: map[string]string{ + "olm.operatorframework.io/nonStatusCopyHash": "hn-1", + }, + }, + Spec: v1alpha1.ClusterServiceVersionSpec{ + Replaces: "replacee", + }, + Status: v1alpha1.ClusterServiceVersionStatus{ + Phase: "waxing gibbous", + }, + }), + // Normal Update for the statusCopyHashAnnotation = "hs" + // We still keep the "hn-1" annotation as well, plus Name/Namespace as is. + ktesting.NewUpdateAction(gvr, "to", &v1alpha1.ClusterServiceVersion{ + ObjectMeta: metav1.ObjectMeta{ + Name: "name", + Namespace: "to", + Annotations: map[string]string{ + "olm.operatorframework.io/nonStatusCopyHash": "hn-1", + "olm.operatorframework.io/statusCopyHash": "hs", + }, }, Spec: v1alpha1.ClusterServiceVersionSpec{ Replaces: "replacee", @@ -88,13 +168,6 @@ 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{ @@ -111,8 +184,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", @@ -128,25 +201,23 @@ func TestCopyToNamespace(t *testing.T) { UID: "uid", ResourceVersion: "42", Annotations: map[string]string{ - nonStatusCopyHashAnnotation: "hn-2", - statusCopyHashAnnotation: "hs", + nonStatusCopyHashAnnotation: "hn-2", // differs => triggers normal Update + statusCopyHashAnnotation: "hs", // same => no status update }, }, }, - ExpectedResult: &v1alpha1.ClusterServiceVersion{ - ObjectMeta: metav1.ObjectMeta{ - Name: "name", - Namespace: "to", - UID: "uid", - }, - }, ExpectedActions: []ktesting.Action{ + // Non-status differs => 1 normal Update ktesting.NewUpdateAction(gvr, "to", &v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ Name: "name", Namespace: "to", UID: "uid", ResourceVersion: "42", + // We'll set the new nonStatusCopyHashAnnotation = "hn-1" + Annotations: map[string]string{ + nonStatusCopyHashAnnotation: "hn-1", + }, }, Spec: v1alpha1.ClusterServiceVersionSpec{ Replaces: "replacee", @@ -156,6 +227,13 @@ func TestCopyToNamespace(t *testing.T) { }, }), }, + ExpectedResult: &v1alpha1.ClusterServiceVersion{ + ObjectMeta: metav1.ObjectMeta{ + Name: "name", + Namespace: "to", + UID: "uid", + }, + }, }, { Name: "copy status updated if status hash differs", @@ -165,8 +243,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", @@ -182,25 +260,43 @@ func TestCopyToNamespace(t *testing.T) { UID: "uid", ResourceVersion: "42", Annotations: map[string]string{ + // non-status matches => no normal update nonStatusCopyHashAnnotation: "hn", - statusCopyHashAnnotation: "hs-2", + // status differs => subresource + normal update + statusCopyHashAnnotation: "hs-2", }, }, }, - ExpectedResult: &v1alpha1.ClusterServiceVersion{ - ObjectMeta: metav1.ObjectMeta{ - Name: "name", - Namespace: "to", - UID: "uid", - }, - }, ExpectedActions: []ktesting.Action{ + // UpdateStatus (we set the new status, and the in-memory CSV includes the matching nonStatus) ktesting.NewUpdateSubresourceAction(gvr, "status", "to", &v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ Name: "name", Namespace: "to", UID: "uid", ResourceVersion: "42", + Annotations: map[string]string{ + nonStatusCopyHashAnnotation: "hn", + }, + }, + Spec: v1alpha1.ClusterServiceVersionSpec{ + Replaces: "replacee", + }, + Status: v1alpha1.ClusterServiceVersionStatus{ + Phase: "waxing gibbous", + }, + }), + // Normal Update to set statusCopyHashAnnotation = "hs-1" + ktesting.NewUpdateAction(gvr, "to", &v1alpha1.ClusterServiceVersion{ + ObjectMeta: metav1.ObjectMeta{ + Name: "name", + Namespace: "to", + UID: "uid", + ResourceVersion: "42", + Annotations: map[string]string{ + nonStatusCopyHashAnnotation: "hn", + statusCopyHashAnnotation: "hs-1", + }, }, Spec: v1alpha1.ClusterServiceVersionSpec{ Replaces: "replacee", @@ -210,6 +306,13 @@ func TestCopyToNamespace(t *testing.T) { }, }), }, + ExpectedResult: &v1alpha1.ClusterServiceVersion{ + ObjectMeta: metav1.ObjectMeta{ + Name: "name", + Namespace: "to", + UID: "uid", + }, + }, }, { Name: "copy and copy status updated if both hashes differ", @@ -219,8 +322,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", @@ -236,25 +339,23 @@ func TestCopyToNamespace(t *testing.T) { UID: "uid", ResourceVersion: "42", Annotations: map[string]string{ + // Both nonStatus and status mismatch nonStatusCopyHashAnnotation: "hn-2", statusCopyHashAnnotation: "hs-2", }, }, }, - ExpectedResult: &v1alpha1.ClusterServiceVersion{ - ObjectMeta: metav1.ObjectMeta{ - Name: "name", - Namespace: "to", - UID: "uid", - }, - }, ExpectedActions: []ktesting.Action{ + // Normal update for the non-status mismatch ktesting.NewUpdateAction(gvr, "to", &v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ Name: "name", Namespace: "to", UID: "uid", ResourceVersion: "42", + Annotations: map[string]string{ + nonStatusCopyHashAnnotation: "hn-1", + }, }, Spec: v1alpha1.ClusterServiceVersionSpec{ Replaces: "replacee", @@ -263,12 +364,35 @@ func TestCopyToNamespace(t *testing.T) { Phase: "waxing gibbous", }, }), + // UpdateStatus because status hash mismatch ktesting.NewUpdateSubresourceAction(gvr, "status", "to", &v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ Name: "name", Namespace: "to", UID: "uid", ResourceVersion: "42", + Annotations: map[string]string{ + nonStatusCopyHashAnnotation: "hn-1", + }, + }, + Spec: v1alpha1.ClusterServiceVersionSpec{ + Replaces: "replacee", + }, + Status: v1alpha1.ClusterServiceVersionStatus{ + Phase: "waxing gibbous", + }, + }), + // Normal update for the new statusCopyHashAnnotation + ktesting.NewUpdateAction(gvr, "to", &v1alpha1.ClusterServiceVersion{ + ObjectMeta: metav1.ObjectMeta{ + Name: "name", + Namespace: "to", + UID: "uid", + ResourceVersion: "42", + Annotations: map[string]string{ + nonStatusCopyHashAnnotation: "hn-1", + statusCopyHashAnnotation: "hs-1", + }, }, Spec: v1alpha1.ClusterServiceVersionSpec{ Replaces: "replacee", @@ -278,6 +402,13 @@ func TestCopyToNamespace(t *testing.T) { }, }), }, + ExpectedResult: &v1alpha1.ClusterServiceVersion{ + ObjectMeta: metav1.ObjectMeta{ + Name: "name", + Namespace: "to", + UID: "uid", + }, + }, }, { Name: "no action taken if neither hash differs", @@ -287,8 +418,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{ @@ -309,18 +440,28 @@ func TestCopyToNamespace(t *testing.T) { UID: "uid", }, }, + ExpectedActions: nil, // no update calls if neither hash differs }, } { t.Run(tc.Name, func(t *testing.T) { + // Create a new fake clientset populated with the "existing copy" if any client := fake.NewSimpleClientset() var lister metadatalister.Lister + + // If we have an existing CSV in that target namespace, add it to the slice + items := []*metav1.PartialObjectMetadata{} if tc.ExistingCopy != nil { - client = fake.NewSimpleClientset(&v1alpha1.ClusterServiceVersion{ + existingObj := &v1alpha1.ClusterServiceVersion{ ObjectMeta: tc.ExistingCopy.ObjectMeta, - }) - lister = FakeClusterServiceVersionLister{tc.ExistingCopy} - } else { - lister = FakeClusterServiceVersionLister{{}} + // ... if you want to set Spec/Status in the client, you can + } + client = fake.NewSimpleClientset(existingObj) + items = []*metav1.PartialObjectMetadata{tc.ExistingCopy} + } + + // Create the full Lister + lister = &fakeCSVLister{ + items: items, } logger, _ := test.NewNullLogger() @@ -335,13 +476,17 @@ func TestCopyToNamespace(t *testing.T) { 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 + + // Ensure the in-memory 'proto' has the correct final annotations annotations := proto.GetObjectMeta().GetAnnotations() - require.Equal(t, tc.Hash, annotations[nonStatusCopyHashAnnotation]) - require.Equal(t, tc.StatusHash, annotations[statusCopyHashAnnotation]) + require.Equal(t, tc.Hash, annotations[nonStatusCopyHashAnnotation], + "proto should have the non-status hash annotation set") + require.Equal(t, tc.StatusHash, annotations[statusCopyHashAnnotation], + "proto should have the status hash annotation set") } else { require.EqualError(t, err, tc.ExpectedError.Error()) } + if diff := cmp.Diff(tc.ExpectedResult, result); diff != "" { t.Errorf("incorrect result: %v", diff) } From a2690ec6edfced405c13b2e0788e27e7b0d097b4 Mon Sep 17 00:00:00 2001 From: Brett Tofel Date: Tue, 29 Apr 2025 09:07:27 -0400 Subject: [PATCH 05/26] Add metadata drift guard to copyToNamespace MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since we switched to a PartialObjectMetadata cache to save memory, we lost visibility into copied CSV spec and status fields, and the reintroduced nonStatusCopyHash/statusCopyHash annotations only partially solved the problem. Manual edits to a copied CSV could still go undetected, causing drift without reconciliation. This commit adds two new annotations: olm.operatorframework.io/observedGeneration and olm.operatorframework.io/observedResourceVersion. It implements a mechanism to guard agains metadata drift at the top of the existing-copy path in copyToNamespace. If a stored observedGeneration or observedResourceVersion no longer matches the live object, the operator now: • Updates the spec and hash annotations • Updates the status subresource • Records the new generation and resourceVersion in the guard annotations Because the guard only fires when its annotations are already present, all existing unit tests pass unchanged. We preserve the memory benefits of the metadata‐only informer, avoid extra GETs, and eliminate unnecessary API churn. Future work may explore a WithTransform informer to regain full object visibility with minimal memory impact. Signed-off-by: Brett Tofel --- pkg/controller/operators/olm/operatorgroup.go | 47 +++++++++++++++++-- 1 file changed, 42 insertions(+), 5 deletions(-) diff --git a/pkg/controller/operators/olm/operatorgroup.go b/pkg/controller/operators/olm/operatorgroup.go index 7099053f19..529ea81aee 100644 --- a/pkg/controller/operators/olm/operatorgroup.go +++ b/pkg/controller/operators/olm/operatorgroup.go @@ -792,8 +792,11 @@ func copyableCSVHash(original *v1alpha1.ClusterServiceVersion) (string, string, } const ( - nonStatusCopyHashAnnotation = "olm.operatorframework.io/nonStatusCopyHash" - statusCopyHashAnnotation = "olm.operatorframework.io/statusCopyHash" + nonStatusCopyHashAnnotation = "olm.operatorframework.io/nonStatusCopyHash" + statusCopyHashAnnotation = "olm.operatorframework.io/statusCopyHash" + // annotations for metadata drift guard + observedGenerationAnnotation = "olm.operatorframework.io/observedGeneration" + observedResourceVersionAnnotation = "olm.operatorframework.io/observedResourceVersion" ) // If returned error is not nil, the returned ClusterServiceVersion @@ -829,9 +832,43 @@ func (a *Operator) copyToNamespace(prototype *v1alpha1.ClusterServiceVersion, ns UID: created.UID, }, }, nil - } else if err != nil { - return nil, err - } + } else if err != nil { + return nil, err + } + // metadata drift guard: detect manual modifications to spec or status + if og, orv := existing.Annotations[observedGenerationAnnotation], existing.Annotations[observedResourceVersionAnnotation]; (og != "" && og != fmt.Sprint(existing.GetGeneration())) || (orv != "" && orv != existing.ResourceVersion) { + // full resync for metadata drift + // prepare prototype for update + prototype.Namespace = existing.Namespace + prototype.ResourceVersion = existing.ResourceVersion + prototype.UID = existing.UID + // sync hash annotations + prototype.Annotations[nonStatusCopyHashAnnotation] = nonstatus + prototype.Annotations[statusCopyHashAnnotation] = status + // update spec and annotations + updated, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).Update(context.TODO(), prototype, metav1.UpdateOptions{}) + if err != nil { + return nil, fmt.Errorf("failed to resync spec for metadata drift guard: %w", err) + } + // update status subresource + updated.Status = prototype.Status + if _, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).UpdateStatus(context.TODO(), updated, metav1.UpdateOptions{}); err != nil { + return nil, fmt.Errorf("failed to resync status for metadata drift guard: %w", err) + } + // record observed generation and resourceVersion + updated.Annotations[observedGenerationAnnotation] = fmt.Sprint(updated.GetGeneration()) + updated.Annotations[observedResourceVersionAnnotation] = updated.ResourceVersion + if _, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).Update(context.TODO(), updated, metav1.UpdateOptions{}); err != nil { + return nil, fmt.Errorf("failed to update metadata guard annotations: %w", err) + } + return &v1alpha1.ClusterServiceVersion{ + ObjectMeta: metav1.ObjectMeta{ + Name: updated.Name, + Namespace: updated.Namespace, + UID: updated.UID, + }, + }, nil + } prototype.Namespace = existing.Namespace prototype.ResourceVersion = existing.ResourceVersion From 0b973b9923adcaac70b1012df74f35ba7a44d7d4 Mon Sep 17 00:00:00 2001 From: Brett Tofel Date: Tue, 29 Apr 2025 11:36:29 -0400 Subject: [PATCH 06/26] Tests for metadata guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Verifies that exactly three updates (spec, status, guard) are issued when the observedGeneration doesn’t match. Signed-off-by: Brett Tofel --- pkg/controller/operators/olm/operatorgroup.go | 84 +++++++++---------- .../operators/olm/operatorgroup_test.go | 70 ++++++++++++++++ 2 files changed, 112 insertions(+), 42 deletions(-) diff --git a/pkg/controller/operators/olm/operatorgroup.go b/pkg/controller/operators/olm/operatorgroup.go index 529ea81aee..eac0c48256 100644 --- a/pkg/controller/operators/olm/operatorgroup.go +++ b/pkg/controller/operators/olm/operatorgroup.go @@ -792,11 +792,11 @@ func copyableCSVHash(original *v1alpha1.ClusterServiceVersion) (string, string, } const ( - nonStatusCopyHashAnnotation = "olm.operatorframework.io/nonStatusCopyHash" - statusCopyHashAnnotation = "olm.operatorframework.io/statusCopyHash" - // annotations for metadata drift guard - observedGenerationAnnotation = "olm.operatorframework.io/observedGeneration" - observedResourceVersionAnnotation = "olm.operatorframework.io/observedResourceVersion" + nonStatusCopyHashAnnotation = "olm.operatorframework.io/nonStatusCopyHash" + statusCopyHashAnnotation = "olm.operatorframework.io/statusCopyHash" + // annotations for metadata drift guard + observedGenerationAnnotation = "olm.operatorframework.io/observedGeneration" + observedResourceVersionAnnotation = "olm.operatorframework.io/observedResourceVersion" ) // If returned error is not nil, the returned ClusterServiceVersion @@ -832,43 +832,43 @@ func (a *Operator) copyToNamespace(prototype *v1alpha1.ClusterServiceVersion, ns UID: created.UID, }, }, nil - } else if err != nil { - return nil, err - } - // metadata drift guard: detect manual modifications to spec or status - if og, orv := existing.Annotations[observedGenerationAnnotation], existing.Annotations[observedResourceVersionAnnotation]; (og != "" && og != fmt.Sprint(existing.GetGeneration())) || (orv != "" && orv != existing.ResourceVersion) { - // full resync for metadata drift - // prepare prototype for update - prototype.Namespace = existing.Namespace - prototype.ResourceVersion = existing.ResourceVersion - prototype.UID = existing.UID - // sync hash annotations - prototype.Annotations[nonStatusCopyHashAnnotation] = nonstatus - prototype.Annotations[statusCopyHashAnnotation] = status - // update spec and annotations - updated, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).Update(context.TODO(), prototype, metav1.UpdateOptions{}) - if err != nil { - return nil, fmt.Errorf("failed to resync spec for metadata drift guard: %w", err) - } - // update status subresource - updated.Status = prototype.Status - if _, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).UpdateStatus(context.TODO(), updated, metav1.UpdateOptions{}); err != nil { - return nil, fmt.Errorf("failed to resync status for metadata drift guard: %w", err) - } - // record observed generation and resourceVersion - updated.Annotations[observedGenerationAnnotation] = fmt.Sprint(updated.GetGeneration()) - updated.Annotations[observedResourceVersionAnnotation] = updated.ResourceVersion - if _, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).Update(context.TODO(), updated, metav1.UpdateOptions{}); err != nil { - return nil, fmt.Errorf("failed to update metadata guard annotations: %w", err) - } - return &v1alpha1.ClusterServiceVersion{ - ObjectMeta: metav1.ObjectMeta{ - Name: updated.Name, - Namespace: updated.Namespace, - UID: updated.UID, - }, - }, nil - } + } else if err != nil { + return nil, err + } + // metadata drift guard: detect manual modifications to spec or status + if og, orv := existing.Annotations[observedGenerationAnnotation], existing.Annotations[observedResourceVersionAnnotation]; (og != "" && og != fmt.Sprint(existing.GetGeneration())) || (orv != "" && orv != existing.ResourceVersion) { + // full resync for metadata drift + // prepare prototype for update + prototype.Namespace = existing.Namespace + prototype.ResourceVersion = existing.ResourceVersion + prototype.UID = existing.UID + // sync hash annotations + prototype.Annotations[nonStatusCopyHashAnnotation] = nonstatus + prototype.Annotations[statusCopyHashAnnotation] = status + // update spec and annotations + updated, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).Update(context.TODO(), prototype, metav1.UpdateOptions{}) + if err != nil { + return nil, fmt.Errorf("failed to resync spec for metadata drift guard: %w", err) + } + // update status subresource + updated.Status = prototype.Status + if _, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).UpdateStatus(context.TODO(), updated, metav1.UpdateOptions{}); err != nil { + return nil, fmt.Errorf("failed to resync status for metadata drift guard: %w", err) + } + // record observed generation and resourceVersion + updated.Annotations[observedGenerationAnnotation] = fmt.Sprint(updated.GetGeneration()) + updated.Annotations[observedResourceVersionAnnotation] = updated.ResourceVersion + if _, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).Update(context.TODO(), updated, metav1.UpdateOptions{}); err != nil { + return nil, fmt.Errorf("failed to update metadata guard annotations: %w", err) + } + return &v1alpha1.ClusterServiceVersion{ + ObjectMeta: metav1.ObjectMeta{ + Name: updated.Name, + Namespace: updated.Namespace, + UID: updated.UID, + }, + }, nil + } prototype.Namespace = existing.Namespace prototype.ResourceVersion = existing.ResourceVersion diff --git a/pkg/controller/operators/olm/operatorgroup_test.go b/pkg/controller/operators/olm/operatorgroup_test.go index 617a0f673e..100510bf6c 100644 --- a/pkg/controller/operators/olm/operatorgroup_test.go +++ b/pkg/controller/operators/olm/operatorgroup_test.go @@ -38,6 +38,76 @@ func (n *fakeCSVNamespaceLister) List(selector labels.Selector) ([]*metav1.Parti return result, nil } +// Test full resync via metadata drift guard when observedGeneration mismatches +func TestCopyToNamespace_MetadataDriftGuard(t *testing.T) { + // Prepare prototype CSV and compute hashes + prototype := v1alpha1.ClusterServiceVersion{ + ObjectMeta: metav1.ObjectMeta{ + Name: "name", + Annotations: map[string]string{}, + }, + Spec: v1alpha1.ClusterServiceVersionSpec{Replaces: "replacee"}, + Status: v1alpha1.ClusterServiceVersionStatus{Phase: "waxing gibbous"}, + } + nonstatus, status, err := copyableCSVHash(&prototype) + require.NoError(t, err) + + // Existing partial copy with observedGeneration mismatched + existingCopy := &metav1.PartialObjectMetadata{ + ObjectMeta: metav1.ObjectMeta{ + Name: "name", + Namespace: "to", + UID: "uid", + ResourceVersion: "42", + Generation: 2, + Annotations: map[string]string{ + nonStatusCopyHashAnnotation: nonstatus, + statusCopyHashAnnotation: status, + observedGenerationAnnotation: "1", + observedResourceVersionAnnotation: "42", + }, + }, + } + // Full CSV for fake client + full := &v1alpha1.ClusterServiceVersion{ + ObjectMeta: metav1.ObjectMeta{ + Name: existingCopy.Name, + Namespace: existingCopy.Namespace, + UID: existingCopy.UID, + ResourceVersion: existingCopy.ResourceVersion, + Generation: existingCopy.Generation, + Annotations: existingCopy.Annotations, + }, + Spec: prototype.Spec, + Status: prototype.Status, + } + + client := fake.NewSimpleClientset(full) + lister := &fakeCSVLister{items: []*metav1.PartialObjectMetadata{existingCopy}} + logger, _ := test.NewNullLogger() + o := &Operator{copiedCSVLister: lister, client: client, logger: logger} + + protoCopy := prototype.DeepCopy() + result, err := o.copyToNamespace(protoCopy, "from", "to", nonstatus, status) + require.NoError(t, err) + require.Equal(t, "name", result.GetName()) + require.Equal(t, "to", result.GetNamespace()) + require.Equal(t, "uid", string(result.GetUID())) + + actions := client.Actions() + // Expect: update(spec), updateStatus, update(guard annotations) + require.Len(t, actions, 3) + // First action: spec update + ua0 := actions[0].(ktesting.UpdateAction) + require.Equal(t, "", ua0.GetSubresource()) + // Second action: status subresource + ua1 := actions[1].(ktesting.UpdateAction) + require.Equal(t, "status", ua1.GetSubresource()) + // Third action: metadata guard update + ua2 := actions[2].(ktesting.UpdateAction) + require.Equal(t, "", ua2.GetSubresource()) +} + func (n *fakeCSVNamespaceLister) Get(name string) (*metav1.PartialObjectMetadata, error) { for _, item := range n.items { if item != nil && item.Namespace == n.namespace && item.Name == name { From f0a1d7eee11c05c6db082007f3dff9bc1f4a8f26 Mon Sep 17 00:00:00 2001 From: Brett Tofel Date: Thu, 8 May 2025 13:28:46 -0400 Subject: [PATCH 07/26] Persist observed annotations on all status updates Signed-off-by: Brett Tofel --- pkg/controller/operators/olm/operatorgroup.go | 47 ++++++++++++------- 1 file changed, 31 insertions(+), 16 deletions(-) diff --git a/pkg/controller/operators/olm/operatorgroup.go b/pkg/controller/operators/olm/operatorgroup.go index eac0c48256..3172552a6b 100644 --- a/pkg/controller/operators/olm/operatorgroup.go +++ b/pkg/controller/operators/olm/operatorgroup.go @@ -821,17 +821,25 @@ 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, - Namespace: created.Namespace, - UID: created.UID, - }, - }, nil + prototype.Annotations[statusCopyHashAnnotation] = status + // persist status-hash annotation + updatedCreated, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).Update(context.TODO(), prototype, metav1.UpdateOptions{}) + if err != nil { + return nil, fmt.Errorf("failed to update annotations after updating status: %w", err) + } + // record observed generation and resourceVersion for metadata-drift guard + updatedCreated.Annotations[observedGenerationAnnotation] = fmt.Sprint(updatedCreated.GetGeneration()) + updatedCreated.Annotations[observedResourceVersionAnnotation] = updatedCreated.ResourceVersion + if _, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).Update(context.TODO(), updatedCreated, metav1.UpdateOptions{}); err != nil { + return nil, fmt.Errorf("failed to update metadata guard annotations after creation: %w", err) + } + return &v1alpha1.ClusterServiceVersion{ + ObjectMeta: metav1.ObjectMeta{ + Name: updatedCreated.Name, + Namespace: updatedCreated.Namespace, + UID: updatedCreated.UID, + }, + }, nil } else if err != nil { return nil, err } @@ -897,11 +905,18 @@ func (a *Operator) copyToNamespace(prototype *v1alpha1.ClusterServiceVersion, ns } // 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) - } + // persist status-hash annotation + prototype.Annotations[statusCopyHashAnnotation] = status + updated, err = a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).Update(context.TODO(), prototype, metav1.UpdateOptions{}) + if err != nil { + return nil, fmt.Errorf("failed to update: %w", err) + } + // record observed generation and resourceVersion for metadata-drift guard + updated.Annotations[observedGenerationAnnotation] = fmt.Sprint(updated.GetGeneration()) + updated.Annotations[observedResourceVersionAnnotation] = updated.ResourceVersion + if updated, err = a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).Update(context.TODO(), updated, metav1.UpdateOptions{}); err != nil { + return nil, fmt.Errorf("failed to update metadata guard annotations after status update: %w", err) + } } else { // Even if they're the same, ensure the returned prototype is annotated. prototype.Annotations[statusCopyHashAnnotation] = status From edfa12d0aea04921b0d7c50b9be2302542ce7ed9 Mon Sep 17 00:00:00 2001 From: Brett Tofel Date: Mon, 19 May 2025 16:14:18 -0400 Subject: [PATCH 08/26] GCI the file Signed-off-by: Brett Tofel --- pkg/controller/operators/olm/operatorgroup.go | 62 +++++++++---------- 1 file changed, 31 insertions(+), 31 deletions(-) diff --git a/pkg/controller/operators/olm/operatorgroup.go b/pkg/controller/operators/olm/operatorgroup.go index 3172552a6b..fe867f95d8 100644 --- a/pkg/controller/operators/olm/operatorgroup.go +++ b/pkg/controller/operators/olm/operatorgroup.go @@ -821,25 +821,25 @@ 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 - // persist status-hash annotation - updatedCreated, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).Update(context.TODO(), prototype, metav1.UpdateOptions{}) - if err != nil { - return nil, fmt.Errorf("failed to update annotations after updating status: %w", err) - } - // record observed generation and resourceVersion for metadata-drift guard - updatedCreated.Annotations[observedGenerationAnnotation] = fmt.Sprint(updatedCreated.GetGeneration()) - updatedCreated.Annotations[observedResourceVersionAnnotation] = updatedCreated.ResourceVersion - if _, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).Update(context.TODO(), updatedCreated, metav1.UpdateOptions{}); err != nil { - return nil, fmt.Errorf("failed to update metadata guard annotations after creation: %w", err) - } - return &v1alpha1.ClusterServiceVersion{ - ObjectMeta: metav1.ObjectMeta{ - Name: updatedCreated.Name, - Namespace: updatedCreated.Namespace, - UID: updatedCreated.UID, - }, - }, nil + prototype.Annotations[statusCopyHashAnnotation] = status + // persist status-hash annotation + updatedCreated, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).Update(context.TODO(), prototype, metav1.UpdateOptions{}) + if err != nil { + return nil, fmt.Errorf("failed to update annotations after updating status: %w", err) + } + // record observed generation and resourceVersion for metadata-drift guard + updatedCreated.Annotations[observedGenerationAnnotation] = fmt.Sprint(updatedCreated.GetGeneration()) + updatedCreated.Annotations[observedResourceVersionAnnotation] = updatedCreated.ResourceVersion + if _, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).Update(context.TODO(), updatedCreated, metav1.UpdateOptions{}); err != nil { + return nil, fmt.Errorf("failed to update metadata guard annotations after creation: %w", err) + } + return &v1alpha1.ClusterServiceVersion{ + ObjectMeta: metav1.ObjectMeta{ + Name: updatedCreated.Name, + Namespace: updatedCreated.Namespace, + UID: updatedCreated.UID, + }, + }, nil } else if err != nil { return nil, err } @@ -905,18 +905,18 @@ func (a *Operator) copyToNamespace(prototype *v1alpha1.ClusterServiceVersion, ns } // 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. - // persist status-hash annotation - prototype.Annotations[statusCopyHashAnnotation] = status - updated, err = a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).Update(context.TODO(), prototype, metav1.UpdateOptions{}) - if err != nil { - return nil, fmt.Errorf("failed to update: %w", err) - } - // record observed generation and resourceVersion for metadata-drift guard - updated.Annotations[observedGenerationAnnotation] = fmt.Sprint(updated.GetGeneration()) - updated.Annotations[observedResourceVersionAnnotation] = updated.ResourceVersion - if updated, err = a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).Update(context.TODO(), updated, metav1.UpdateOptions{}); err != nil { - return nil, fmt.Errorf("failed to update metadata guard annotations after status update: %w", err) - } + // persist status-hash annotation + prototype.Annotations[statusCopyHashAnnotation] = status + updated, err = a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).Update(context.TODO(), prototype, metav1.UpdateOptions{}) + if err != nil { + return nil, fmt.Errorf("failed to update: %w", err) + } + // record observed generation and resourceVersion for metadata-drift guard + updated.Annotations[observedGenerationAnnotation] = fmt.Sprint(updated.GetGeneration()) + updated.Annotations[observedResourceVersionAnnotation] = updated.ResourceVersion + if updated, err = a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).Update(context.TODO(), updated, metav1.UpdateOptions{}); err != nil { + return nil, fmt.Errorf("failed to update metadata guard annotations after status update: %w", err) + } } else { // Even if they're the same, ensure the returned prototype is annotated. prototype.Annotations[statusCopyHashAnnotation] = status From e3130bb89d63efaf778a7c237130e2ff37ce7c99 Mon Sep 17 00:00:00 2001 From: Todd Short Date: Tue, 10 Jun 2025 12:00:08 -0400 Subject: [PATCH 09/26] Use TransformFunc Unit tests not updated Signed-off-by: Todd Short --- pkg/controller/operators/catalog/operator.go | 67 ++++++----- .../internal/pruning/listerwatcher.go | 33 +----- pkg/controller/operators/olm/operator.go | 70 +++++++++--- pkg/controller/operators/olm/operatorgroup.go | 106 +++--------------- .../operators/olm/operatorgroup_test.go | 8 +- .../operators/olm/plugins/operator_plugin.go | 3 +- 6 files changed, 122 insertions(+), 165 deletions(-) diff --git a/pkg/controller/operators/catalog/operator.go b/pkg/controller/operators/catalog/operator.go index 6893c87d3e..2d54e77aac 100644 --- a/pkg/controller/operators/catalog/operator.go +++ b/pkg/controller/operators/catalog/operator.go @@ -230,38 +230,53 @@ func NewOperator(ctx context.Context, kubeconfigPath string, clock utilclock.Clo // Fields are pruned from local copies of the objects managed // by this informer in order to reduce cached size. - prunedCSVInformer := cache.NewSharedIndexInformer( - pruning.NewListerWatcher(op.client, metav1.NamespaceAll, + prunedCSVInformer := cache.NewSharedIndexInformerWithOptions( + pruning.NewListerWatcher( + op.client, + metav1.NamespaceAll, func(options *metav1.ListOptions) { options.LabelSelector = fmt.Sprintf("!%s", v1alpha1.CopiedLabelKey) }, - pruning.PrunerFunc(func(csv *v1alpha1.ClusterServiceVersion) { - *csv = v1alpha1.ClusterServiceVersion{ - TypeMeta: csv.TypeMeta, - ObjectMeta: metav1.ObjectMeta{ - Name: csv.Name, - Namespace: csv.Namespace, - Labels: csv.Labels, - Annotations: csv.Annotations, - }, - Spec: v1alpha1.ClusterServiceVersionSpec{ - CustomResourceDefinitions: csv.Spec.CustomResourceDefinitions, - APIServiceDefinitions: csv.Spec.APIServiceDefinitions, - Replaces: csv.Spec.Replaces, - Version: csv.Spec.Version, - }, - Status: v1alpha1.ClusterServiceVersionStatus{ - Phase: csv.Status.Phase, - Reason: csv.Status.Reason, - }, - } - })), + ), &v1alpha1.ClusterServiceVersion{}, - resyncPeriod(), - cache.Indexers{ - cache.NamespaceIndex: cache.MetaNamespaceIndexFunc, + cache.SharedIndexInformerOptions{ + ResyncPeriod: resyncPeriod(), + Indexers: cache.Indexers{ + cache.NamespaceIndex: cache.MetaNamespaceIndexFunc, + }, }, ) + + // Transformed the CSV to be just the necessary data + prunedCSVTransformFunc := func(i interface{}) (interface{}, error) { + if csv, ok := i.(*v1alpha1.ClusterServiceVersion); ok { + *csv = v1alpha1.ClusterServiceVersion{ + TypeMeta: csv.TypeMeta, + ObjectMeta: metav1.ObjectMeta{ + Name: csv.Name, + Namespace: csv.Namespace, + Labels: csv.Labels, + Annotations: csv.Annotations, + }, + Spec: v1alpha1.ClusterServiceVersionSpec{ + CustomResourceDefinitions: csv.Spec.CustomResourceDefinitions, + APIServiceDefinitions: csv.Spec.APIServiceDefinitions, + Replaces: csv.Spec.Replaces, + Version: csv.Spec.Version, + }, + Status: v1alpha1.ClusterServiceVersionStatus{ + Phase: csv.Status.Phase, + Reason: csv.Status.Reason, + }, + } + return csv, nil + } + return nil, fmt.Errorf("Unable to convert input to CSV") + } + + if err := prunedCSVInformer.SetTransform(prunedCSVTransformFunc); err != nil { + return nil, err + } csvLister := operatorsv1alpha1listers.NewClusterServiceVersionLister(prunedCSVInformer.GetIndexer()) op.lister.OperatorsV1alpha1().RegisterClusterServiceVersionLister(metav1.NamespaceAll, csvLister) if err := op.RegisterInformer(prunedCSVInformer); err != nil { diff --git a/pkg/controller/operators/internal/pruning/listerwatcher.go b/pkg/controller/operators/internal/pruning/listerwatcher.go index 6aa3ce5271..b069de2259 100644 --- a/pkg/controller/operators/internal/pruning/listerwatcher.go +++ b/pkg/controller/operators/internal/pruning/listerwatcher.go @@ -8,45 +8,18 @@ import ( "k8s.io/apimachinery/pkg/watch" "k8s.io/client-go/tools/cache" - "github.com/operator-framework/api/pkg/operators/v1alpha1" "github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/clientset/versioned" ) -type Pruner interface { - Prune(*v1alpha1.ClusterServiceVersion) -} - -type PrunerFunc func(*v1alpha1.ClusterServiceVersion) - -func (f PrunerFunc) Prune(csv *v1alpha1.ClusterServiceVersion) { - f(csv) -} - -func NewListerWatcher(client versioned.Interface, namespace string, override func(*metav1.ListOptions), p Pruner) cache.ListerWatcher { +func NewListerWatcher(client versioned.Interface, namespace string, override func(*metav1.ListOptions)) cache.ListerWatcher { return &cache.ListWatch{ ListFunc: func(options metav1.ListOptions) (runtime.Object, error) { override(&options) - list, err := client.OperatorsV1alpha1().ClusterServiceVersions(namespace).List(context.TODO(), options) - if err != nil { - return list, err - } - for i := range list.Items { - p.Prune(&list.Items[i]) - } - return list, nil + return client.OperatorsV1alpha1().ClusterServiceVersions(namespace).List(context.TODO(), options) }, WatchFunc: func(options metav1.ListOptions) (watch.Interface, error) { override(&options) - w, err := client.OperatorsV1alpha1().ClusterServiceVersions(namespace).Watch(context.TODO(), options) - if err != nil { - return w, err - } - return watch.Filter(w, watch.FilterFunc(func(e watch.Event) (watch.Event, bool) { - if csv, ok := e.Object.(*v1alpha1.ClusterServiceVersion); ok { - p.Prune(csv) - } - return e, true - })), nil + return client.OperatorsV1alpha1().ClusterServiceVersions(namespace).Watch(context.TODO(), options) }, } } diff --git a/pkg/controller/operators/olm/operator.go b/pkg/controller/operators/olm/operator.go index f0841953ab..a72d13663d 100644 --- a/pkg/controller/operators/olm/operator.go +++ b/pkg/controller/operators/olm/operator.go @@ -43,8 +43,10 @@ import ( "github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/clientset/versioned" "github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/informers/externalversions" + operatorsv1alpha1listers "github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/listers/operators/v1alpha1" "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/certs" "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/install" + "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/internal/pruning" "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/labeller" "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/olm/overrides" "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/olm/plugins" @@ -63,6 +65,14 @@ import ( "github.com/operator-framework/operator-lifecycle-manager/pkg/metrics" ) +const ( + // These annotation keys are intentionally invalid -- all writes + // to copied CSVs are regenerated from the corresponding non-copied CSV, + // so it should never be transmitted back to the API server. + copyCSVStatusHash = "$copyhash-status" + copyCSVSpecHash = "$copyhash-spec" +) + var ( ErrRequirementsNotMet = errors.New("requirements were not met") ErrCRDOwnerConflict = errors.New("conflicting CRD owner in namespace") @@ -82,7 +92,7 @@ type Operator struct { client versioned.Interface lister operatorlister.OperatorLister protectedCopiedCSVNamespaces map[string]struct{} - copiedCSVLister metadatalister.Lister + copiedCSVLister operatorsv1alpha1listers.ClusterServiceVersionLister ogQueueSet *queueinformer.ResourceQueueSet csvQueueSet *queueinformer.ResourceQueueSet olmConfigQueue workqueue.TypedRateLimitingInterface[types.NamespacedName] @@ -294,20 +304,54 @@ func newOperatorWithConfig(ctx context.Context, config *operatorConfig) (*Operat return nil, err } - // A separate informer solely for CSV copies. Object metadata requests are used + // A separate informer solely for CSV copies. Fields + // are pruned from local copies of the objects managed // by this informer in order to reduce cached size. - gvr := v1alpha1.SchemeGroupVersion.WithResource("clusterserviceversions") - copiedCSVInformer := metadatainformer.NewFilteredMetadataInformer( - config.metadataClient, - gvr, - namespace, - config.resyncPeriod(), - cache.Indexers{cache.NamespaceIndex: cache.MetaNamespaceIndexFunc}, - func(options *metav1.ListOptions) { - options.LabelSelector = v1alpha1.CopiedLabelKey + copiedCSVInformer := cache.NewSharedIndexInformerWithOptions( + pruning.NewListerWatcher( + op.client, + namespace, + func(opts *metav1.ListOptions) { + opts.LabelSelector = v1alpha1.CopiedLabelKey + }, + ), + &v1alpha1.ClusterServiceVersion{}, + cache.SharedIndexInformerOptions{ + ResyncPeriod: config.resyncPeriod(), + Indexers: cache.Indexers{cache.NamespaceIndex: cache.MetaNamespaceIndexFunc}, }, - ).Informer() - op.copiedCSVLister = metadatalister.New(copiedCSVInformer.GetIndexer(), gvr) + ) + + // Transform the copied CSV to be just the Metadata + // However, because we have the full copied CSV, we can calculate + // a hash over the full CSV to store as an annotation + copiedCSVTransformFunc := func(i interface{}) (interface{}, error) { + if csv, ok := i.(*v1alpha1.ClusterServiceVersion); ok { + specHash, statusHash, err := copyableCSVHash(csv) + if err != nil { + return nil, err + } + *csv = v1alpha1.ClusterServiceVersion{ + TypeMeta: csv.TypeMeta, + ObjectMeta: csv.ObjectMeta, + Spec: v1alpha1.ClusterServiceVersionSpec{}, + Status: v1alpha1.ClusterServiceVersionStatus{}, + } + if csv.Annotations == nil { + csv.Annotations = make(map[string]string, 2) + } + // fake CSV hashes for tracking purposes only + csv.Annotations[copyCSVSpecHash] = specHash + csv.Annotations[copyCSVStatusHash] = statusHash + return csv, nil + } + return nil, fmt.Errorf("Unable to convert input to CSV") + } + + if err := copiedCSVInformer.SetTransform(copiedCSVTransformFunc); err != nil { + return nil, err + } + op.copiedCSVLister = operatorsv1alpha1listers.NewClusterServiceVersionLister(copiedCSVInformer.GetIndexer()) informersByNamespace[namespace].CopiedCSVInformer = copiedCSVInformer informersByNamespace[namespace].CopiedCSVLister = op.copiedCSVLister diff --git a/pkg/controller/operators/olm/operatorgroup.go b/pkg/controller/operators/olm/operatorgroup.go index fe867f95d8..5bfd062d6f 100644 --- a/pkg/controller/operators/olm/operatorgroup.go +++ b/pkg/controller/operators/olm/operatorgroup.go @@ -704,7 +704,7 @@ func (a *Operator) ensureCSVsInNamespaces(csv *v1alpha1.ClusterServiceVersion, o var copyPrototype v1alpha1.ClusterServiceVersion csvCopyPrototype(csv, ©Prototype) - nonstatus, status, err := copyableCSVHash(©Prototype) + specHash, statusHash, err := copyableCSVHash(©Prototype) if err != nil { return err } @@ -715,7 +715,7 @@ func (a *Operator) ensureCSVsInNamespaces(csv *v1alpha1.ClusterServiceVersion, o } if targets.Contains(ns.GetName()) { var targetCSV *v1alpha1.ClusterServiceVersion - if targetCSV, err = a.copyToNamespace(©Prototype, csv.GetNamespace(), ns.GetName(), nonstatus, status); err != nil { + if targetCSV, err = a.copyToNamespace(©Prototype, csv.GetNamespace(), ns.GetName(), specHash, statusHash); err != nil { logger.WithError(err).Debug("error copying to target") continue } @@ -779,29 +779,21 @@ func copyableCSVHash(original *v1alpha1.ClusterServiceVersion) (string, string, Spec: original.Spec, } - newHash, err := hashutil.DeepHashObject(&shallow) + specHash, err := hashutil.DeepHashObject(&shallow) if err != nil { return "", "", err } - originalHash, err := hashutil.DeepHashObject(&original.Status) + statusHash, err := hashutil.DeepHashObject(&original.Status) if err != nil { return "", "", err } - return newHash, originalHash, nil + return specHash, statusHash, nil } -const ( - nonStatusCopyHashAnnotation = "olm.operatorframework.io/nonStatusCopyHash" - statusCopyHashAnnotation = "olm.operatorframework.io/statusCopyHash" - // annotations for metadata drift guard - observedGenerationAnnotation = "olm.operatorframework.io/observedGeneration" - observedResourceVersionAnnotation = "olm.operatorframework.io/observedResourceVersion" -) - // 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) { +func (a *Operator) copyToNamespace(prototype *v1alpha1.ClusterServiceVersion, nsFrom, nsTo, specHash, statusHash string) (*v1alpha1.ClusterServiceVersion, error) { if nsFrom == nsTo { return nil, fmt.Errorf("bug: can not copy to active namespace %v", nsFrom) } @@ -810,9 +802,8 @@ func (a *Operator) copyToNamespace(prototype *v1alpha1.ClusterServiceVersion, ns prototype.ResourceVersion = "" prototype.UID = "" - existing, err := a.copiedCSVLister.Namespace(nsTo).Get(prototype.GetName()) + existing, err := a.copiedCSVLister.ClusterServiceVersions(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) @@ -821,75 +812,26 @@ 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 - // persist status-hash annotation - updatedCreated, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).Update(context.TODO(), prototype, metav1.UpdateOptions{}) - if err != nil { - return nil, fmt.Errorf("failed to update annotations after updating status: %w", err) - } - // record observed generation and resourceVersion for metadata-drift guard - updatedCreated.Annotations[observedGenerationAnnotation] = fmt.Sprint(updatedCreated.GetGeneration()) - updatedCreated.Annotations[observedResourceVersionAnnotation] = updatedCreated.ResourceVersion - if _, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).Update(context.TODO(), updatedCreated, metav1.UpdateOptions{}); err != nil { - return nil, fmt.Errorf("failed to update metadata guard annotations after creation: %w", err) - } return &v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ - Name: updatedCreated.Name, - Namespace: updatedCreated.Namespace, - UID: updatedCreated.UID, + Name: created.Name, + Namespace: created.Namespace, + UID: created.UID, }, }, nil } else if err != nil { return nil, err } - // metadata drift guard: detect manual modifications to spec or status - if og, orv := existing.Annotations[observedGenerationAnnotation], existing.Annotations[observedResourceVersionAnnotation]; (og != "" && og != fmt.Sprint(existing.GetGeneration())) || (orv != "" && orv != existing.ResourceVersion) { - // full resync for metadata drift - // prepare prototype for update - prototype.Namespace = existing.Namespace - prototype.ResourceVersion = existing.ResourceVersion - prototype.UID = existing.UID - // sync hash annotations - prototype.Annotations[nonStatusCopyHashAnnotation] = nonstatus - prototype.Annotations[statusCopyHashAnnotation] = status - // update spec and annotations - updated, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).Update(context.TODO(), prototype, metav1.UpdateOptions{}) - if err != nil { - return nil, fmt.Errorf("failed to resync spec for metadata drift guard: %w", err) - } - // update status subresource - updated.Status = prototype.Status - if _, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).UpdateStatus(context.TODO(), updated, metav1.UpdateOptions{}); err != nil { - return nil, fmt.Errorf("failed to resync status for metadata drift guard: %w", err) - } - // record observed generation and resourceVersion - updated.Annotations[observedGenerationAnnotation] = fmt.Sprint(updated.GetGeneration()) - updated.Annotations[observedResourceVersionAnnotation] = updated.ResourceVersion - if _, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).Update(context.TODO(), updated, metav1.UpdateOptions{}); err != nil { - return nil, fmt.Errorf("failed to update metadata guard annotations: %w", err) - } - return &v1alpha1.ClusterServiceVersion{ - ObjectMeta: metav1.ObjectMeta{ - Name: updated.Name, - Namespace: updated.Namespace, - UID: updated.UID, - }, - }, nil - } prototype.Namespace = existing.Namespace prototype.ResourceVersion = existing.ResourceVersion prototype.UID = existing.UID // Get the non-status and status hash of the existing copied CSV - existingNonStatus := existing.Annotations[nonStatusCopyHashAnnotation] - existingStatus := existing.Annotations[statusCopyHashAnnotation] + existingSpecHash := existing.Annotations[copyCSVSpecHash] + existingStatusHash := existing.Annotations[copyCSVStatusHash] var updated *v1alpha1.ClusterServiceVersion - // Always set the in-memory prototype's nonstatus annotation: - prototype.Annotations[nonStatusCopyHashAnnotation] = nonstatus - if existingNonStatus != nonstatus { - // include updates to the non-status hash annotation if there is a mismatch + if existingSpecHash != specHash { 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) } @@ -898,29 +840,11 @@ func (a *Operator) copyToNamespace(prototype *v1alpha1.ClusterServiceVersion, ns updated = prototype } - if existingStatus != status { + if existingStatusHash != statusHash { updated.Status = prototype.Status 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. - // persist status-hash annotation - prototype.Annotations[statusCopyHashAnnotation] = status - updated, err = a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).Update(context.TODO(), prototype, metav1.UpdateOptions{}) - if err != nil { - return nil, fmt.Errorf("failed to update: %w", err) - } - // record observed generation and resourceVersion for metadata-drift guard - updated.Annotations[observedGenerationAnnotation] = fmt.Sprint(updated.GetGeneration()) - updated.Annotations[observedResourceVersionAnnotation] = updated.ResourceVersion - if updated, err = a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).Update(context.TODO(), updated, metav1.UpdateOptions{}); err != nil { - return nil, fmt.Errorf("failed to update metadata guard annotations after status update: %w", err) - } - } else { - // Even if they're the same, ensure the returned prototype is annotated. - prototype.Annotations[statusCopyHashAnnotation] = status - updated = prototype } return &v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ @@ -932,7 +856,7 @@ func (a *Operator) copyToNamespace(prototype *v1alpha1.ClusterServiceVersion, ns } func (a *Operator) pruneFromNamespace(operatorGroupName, namespace string) error { - fetchedCSVs, err := a.copiedCSVLister.Namespace(namespace).List(labels.Everything()) + fetchedCSVs, err := a.copiedCSVLister.ClusterServiceVersions(namespace).List(labels.Everything()) if err != nil { return err } diff --git a/pkg/controller/operators/olm/operatorgroup_test.go b/pkg/controller/operators/olm/operatorgroup_test.go index 100510bf6c..c19193f13e 100644 --- a/pkg/controller/operators/olm/operatorgroup_test.go +++ b/pkg/controller/operators/olm/operatorgroup_test.go @@ -49,7 +49,7 @@ func TestCopyToNamespace_MetadataDriftGuard(t *testing.T) { Spec: v1alpha1.ClusterServiceVersionSpec{Replaces: "replacee"}, Status: v1alpha1.ClusterServiceVersionStatus{Phase: "waxing gibbous"}, } - nonstatus, status, err := copyableCSVHash(&prototype) + specHash, statusHash, err := copyableCSVHash(&prototype) require.NoError(t, err) // Existing partial copy with observedGeneration mismatched @@ -61,8 +61,8 @@ func TestCopyToNamespace_MetadataDriftGuard(t *testing.T) { ResourceVersion: "42", Generation: 2, Annotations: map[string]string{ - nonStatusCopyHashAnnotation: nonstatus, - statusCopyHashAnnotation: status, + nonStatusCopyHashAnnotation: specHash, + statusCopyHashAnnotation: statusHash, observedGenerationAnnotation: "1", observedResourceVersionAnnotation: "42", }, @@ -88,7 +88,7 @@ func TestCopyToNamespace_MetadataDriftGuard(t *testing.T) { o := &Operator{copiedCSVLister: lister, client: client, logger: logger} protoCopy := prototype.DeepCopy() - result, err := o.copyToNamespace(protoCopy, "from", "to", nonstatus, status) + result, err := o.copyToNamespace(protoCopy, "from", "to", specHash, statusHash) require.NoError(t, err) require.Equal(t, "name", result.GetName()) require.Equal(t, "to", result.GetNamespace()) diff --git a/pkg/controller/operators/olm/plugins/operator_plugin.go b/pkg/controller/operators/olm/plugins/operator_plugin.go index 2b87eb9a4b..fdedd9ed9a 100644 --- a/pkg/controller/operators/olm/plugins/operator_plugin.go +++ b/pkg/controller/operators/olm/plugins/operator_plugin.go @@ -8,6 +8,7 @@ import ( operatorsv1informers "github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/informers/externalversions/operators/v1" operatorsv1alpha1informers "github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/informers/externalversions/operators/v1alpha1" operatorsv2informers "github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/informers/externalversions/operators/v2" + operatorsv1alpha1listers "github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/listers/operators/v1alpha1" "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorclient" "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/queueinformer" "github.com/sirupsen/logrus" @@ -30,7 +31,7 @@ type HostOperator interface { type Informers struct { CSVInformer operatorsv1alpha1informers.ClusterServiceVersionInformer CopiedCSVInformer cache.SharedIndexInformer - CopiedCSVLister metadatalister.Lister + CopiedCSVLister operatorsv1alpha1listers.ClusterServiceVersionLister OperatorGroupInformer operatorsv1informers.OperatorGroupInformer OperatorConditionInformer operatorsv2informers.OperatorConditionInformer SubscriptionInformer operatorsv1alpha1informers.SubscriptionInformer From 50c46e033eb03672f9d070633022fa4f4b45e920 Mon Sep 17 00:00:00 2001 From: Todd Short Date: Tue, 10 Jun 2025 14:36:33 -0400 Subject: [PATCH 10/26] Update operatorgroup tests to compile Signed-off-by: Todd Short --- .../operators/olm/operatorgroup_test.go | 158 +++++------------- 1 file changed, 44 insertions(+), 114 deletions(-) diff --git a/pkg/controller/operators/olm/operatorgroup_test.go b/pkg/controller/operators/olm/operatorgroup_test.go index c19193f13e..ecb8ec841e 100644 --- a/pkg/controller/operators/olm/operatorgroup_test.go +++ b/pkg/controller/operators/olm/operatorgroup_test.go @@ -12,12 +12,12 @@ import ( "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" + listersv1alpha1 "github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/listers/operators/v1alpha1" "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/registry/resolver/cache" "github.com/operator-framework/operator-registry/pkg/registry" ) @@ -25,11 +25,11 @@ import ( // fakeCSVNamespaceLister implements metadatalister.NamespaceLister type fakeCSVNamespaceLister struct { namespace string - items []*metav1.PartialObjectMetadata + items []*v1alpha1.ClusterServiceVersion } -func (n *fakeCSVNamespaceLister) List(selector labels.Selector) ([]*metav1.PartialObjectMetadata, error) { - var result []*metav1.PartialObjectMetadata +func (n *fakeCSVNamespaceLister) List(selector labels.Selector) ([]*v1alpha1.ClusterServiceVersion, error) { + var result []*v1alpha1.ClusterServiceVersion for _, item := range n.items { if item != nil && item.Namespace == n.namespace { result = append(result, item) @@ -38,77 +38,7 @@ func (n *fakeCSVNamespaceLister) List(selector labels.Selector) ([]*metav1.Parti return result, nil } -// Test full resync via metadata drift guard when observedGeneration mismatches -func TestCopyToNamespace_MetadataDriftGuard(t *testing.T) { - // Prepare prototype CSV and compute hashes - prototype := v1alpha1.ClusterServiceVersion{ - ObjectMeta: metav1.ObjectMeta{ - Name: "name", - Annotations: map[string]string{}, - }, - Spec: v1alpha1.ClusterServiceVersionSpec{Replaces: "replacee"}, - Status: v1alpha1.ClusterServiceVersionStatus{Phase: "waxing gibbous"}, - } - specHash, statusHash, err := copyableCSVHash(&prototype) - require.NoError(t, err) - - // Existing partial copy with observedGeneration mismatched - existingCopy := &metav1.PartialObjectMetadata{ - ObjectMeta: metav1.ObjectMeta{ - Name: "name", - Namespace: "to", - UID: "uid", - ResourceVersion: "42", - Generation: 2, - Annotations: map[string]string{ - nonStatusCopyHashAnnotation: specHash, - statusCopyHashAnnotation: statusHash, - observedGenerationAnnotation: "1", - observedResourceVersionAnnotation: "42", - }, - }, - } - // Full CSV for fake client - full := &v1alpha1.ClusterServiceVersion{ - ObjectMeta: metav1.ObjectMeta{ - Name: existingCopy.Name, - Namespace: existingCopy.Namespace, - UID: existingCopy.UID, - ResourceVersion: existingCopy.ResourceVersion, - Generation: existingCopy.Generation, - Annotations: existingCopy.Annotations, - }, - Spec: prototype.Spec, - Status: prototype.Status, - } - - client := fake.NewSimpleClientset(full) - lister := &fakeCSVLister{items: []*metav1.PartialObjectMetadata{existingCopy}} - logger, _ := test.NewNullLogger() - o := &Operator{copiedCSVLister: lister, client: client, logger: logger} - - protoCopy := prototype.DeepCopy() - result, err := o.copyToNamespace(protoCopy, "from", "to", specHash, statusHash) - require.NoError(t, err) - require.Equal(t, "name", result.GetName()) - require.Equal(t, "to", result.GetNamespace()) - require.Equal(t, "uid", string(result.GetUID())) - - actions := client.Actions() - // Expect: update(spec), updateStatus, update(guard annotations) - require.Len(t, actions, 3) - // First action: spec update - ua0 := actions[0].(ktesting.UpdateAction) - require.Equal(t, "", ua0.GetSubresource()) - // Second action: status subresource - ua1 := actions[1].(ktesting.UpdateAction) - require.Equal(t, "status", ua1.GetSubresource()) - // Third action: metadata guard update - ua2 := actions[2].(ktesting.UpdateAction) - require.Equal(t, "", ua2.GetSubresource()) -} - -func (n *fakeCSVNamespaceLister) Get(name string) (*metav1.PartialObjectMetadata, error) { +func (n *fakeCSVNamespaceLister) Get(name string) (*v1alpha1.ClusterServiceVersion, error) { for _, item := range n.items { if item != nil && item.Namespace == n.namespace && item.Name == name { return item, nil @@ -117,19 +47,19 @@ func (n *fakeCSVNamespaceLister) Get(name string) (*metav1.PartialObjectMetadata return nil, errors.NewNotFound(v1alpha1.Resource("clusterserviceversion"), name) } -// fakeCSVLister implements the full metadatalister.Lister interface +// fakeCSVLister implements the full listersv1alpha1.ClusterServiceVersionLister interface // so that Operator.copiedCSVLister = &fakeCSVLister{...} works. type fakeCSVLister struct { - items []*metav1.PartialObjectMetadata + items []*v1alpha1.ClusterServiceVersion } // List returns all CSV metadata items, ignoring namespaces. -func (f *fakeCSVLister) List(selector labels.Selector) ([]*metav1.PartialObjectMetadata, error) { +func (f *fakeCSVLister) List(selector labels.Selector) ([]*v1alpha1.ClusterServiceVersion, error) { return f.items, nil } // Get returns the CSV by name, ignoring namespaces. -func (f *fakeCSVLister) Get(name string) (*metav1.PartialObjectMetadata, error) { +func (f *fakeCSVLister) Get(name string) (*v1alpha1.ClusterServiceVersion, error) { for _, item := range f.items { if item != nil && item.Name == name { return item, nil @@ -139,7 +69,7 @@ func (f *fakeCSVLister) Get(name string) (*metav1.PartialObjectMetadata, error) } // Namespace returns a namespace-scoped lister wrapper. -func (f *fakeCSVLister) Namespace(ns string) metadatalister.NamespaceLister { +func (f *fakeCSVLister) ClusterServiceVersions(ns string) listersv1alpha1.ClusterServiceVersionNamespaceLister { return &fakeCSVNamespaceLister{ namespace: ns, items: f.items, @@ -156,7 +86,7 @@ func TestCopyToNamespace(t *testing.T) { Hash string StatusHash string Prototype v1alpha1.ClusterServiceVersion - ExistingCopy *metav1.PartialObjectMetadata + ExistingCopy *v1alpha1.ClusterServiceVersion ExpectedResult *v1alpha1.ClusterServiceVersion ExpectedError error ExpectedActions []ktesting.Action @@ -264,15 +194,15 @@ func TestCopyToNamespace(t *testing.T) { Phase: "waxing gibbous", }, }, - ExistingCopy: &metav1.PartialObjectMetadata{ + ExistingCopy: &v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ Name: "name", Namespace: "to", UID: "uid", ResourceVersion: "42", Annotations: map[string]string{ - nonStatusCopyHashAnnotation: "hn-2", // differs => triggers normal Update - statusCopyHashAnnotation: "hs", // same => no status update + copyCSVSpecHash: "hn-2", // differs => triggers normal Update + copyCSVStatusHash: "hs", // same => no status update }, }, }, @@ -286,7 +216,7 @@ func TestCopyToNamespace(t *testing.T) { ResourceVersion: "42", // We'll set the new nonStatusCopyHashAnnotation = "hn-1" Annotations: map[string]string{ - nonStatusCopyHashAnnotation: "hn-1", + copyCSVSpecHash: "hn-1", }, }, Spec: v1alpha1.ClusterServiceVersionSpec{ @@ -323,7 +253,7 @@ func TestCopyToNamespace(t *testing.T) { Phase: "waxing gibbous", }, }, - ExistingCopy: &metav1.PartialObjectMetadata{ + ExistingCopy: &v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ Name: "name", Namespace: "to", @@ -331,9 +261,9 @@ func TestCopyToNamespace(t *testing.T) { ResourceVersion: "42", Annotations: map[string]string{ // non-status matches => no normal update - nonStatusCopyHashAnnotation: "hn", + copyCSVSpecHash: "hn", // status differs => subresource + normal update - statusCopyHashAnnotation: "hs-2", + copyCSVStatusHash: "hs-2", }, }, }, @@ -346,7 +276,7 @@ func TestCopyToNamespace(t *testing.T) { UID: "uid", ResourceVersion: "42", Annotations: map[string]string{ - nonStatusCopyHashAnnotation: "hn", + copyCSVSpecHash: "hn", }, }, Spec: v1alpha1.ClusterServiceVersionSpec{ @@ -364,8 +294,8 @@ func TestCopyToNamespace(t *testing.T) { UID: "uid", ResourceVersion: "42", Annotations: map[string]string{ - nonStatusCopyHashAnnotation: "hn", - statusCopyHashAnnotation: "hs-1", + copyCSVSpecHash: "hn", + copyCSVStatusHash: "hs-1", }, }, Spec: v1alpha1.ClusterServiceVersionSpec{ @@ -402,7 +332,7 @@ func TestCopyToNamespace(t *testing.T) { Phase: "waxing gibbous", }, }, - ExistingCopy: &metav1.PartialObjectMetadata{ + ExistingCopy: &v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ Name: "name", Namespace: "to", @@ -410,8 +340,8 @@ func TestCopyToNamespace(t *testing.T) { ResourceVersion: "42", Annotations: map[string]string{ // Both nonStatus and status mismatch - nonStatusCopyHashAnnotation: "hn-2", - statusCopyHashAnnotation: "hs-2", + copyCSVSpecHash: "hn-2", + copyCSVStatusHash: "hs-2", }, }, }, @@ -424,7 +354,7 @@ func TestCopyToNamespace(t *testing.T) { UID: "uid", ResourceVersion: "42", Annotations: map[string]string{ - nonStatusCopyHashAnnotation: "hn-1", + copyCSVSpecHash: "hn-1", }, }, Spec: v1alpha1.ClusterServiceVersionSpec{ @@ -442,7 +372,7 @@ func TestCopyToNamespace(t *testing.T) { UID: "uid", ResourceVersion: "42", Annotations: map[string]string{ - nonStatusCopyHashAnnotation: "hn-1", + copyCSVSpecHash: "hn-1", }, }, Spec: v1alpha1.ClusterServiceVersionSpec{ @@ -460,8 +390,8 @@ func TestCopyToNamespace(t *testing.T) { UID: "uid", ResourceVersion: "42", Annotations: map[string]string{ - nonStatusCopyHashAnnotation: "hn-1", - statusCopyHashAnnotation: "hs-1", + copyCSVSpecHash: "hn-1", + copyCSVStatusHash: "hs-1", }, }, Spec: v1alpha1.ClusterServiceVersionSpec{ @@ -492,14 +422,14 @@ func TestCopyToNamespace(t *testing.T) { Annotations: map[string]string{}, }, }, - ExistingCopy: &metav1.PartialObjectMetadata{ + ExistingCopy: &v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ Name: "name", Namespace: "to", UID: "uid", Annotations: map[string]string{ - nonStatusCopyHashAnnotation: "hn", - statusCopyHashAnnotation: "hs", + copyCSVSpecHash: "hn", + copyCSVStatusHash: "hs", }, }, }, @@ -516,17 +446,17 @@ func TestCopyToNamespace(t *testing.T) { t.Run(tc.Name, func(t *testing.T) { // Create a new fake clientset populated with the "existing copy" if any client := fake.NewSimpleClientset() - var lister metadatalister.Lister + var lister listersv1alpha1.ClusterServiceVersionLister // If we have an existing CSV in that target namespace, add it to the slice - items := []*metav1.PartialObjectMetadata{} + items := []*v1alpha1.ClusterServiceVersion{} if tc.ExistingCopy != nil { existingObj := &v1alpha1.ClusterServiceVersion{ ObjectMeta: tc.ExistingCopy.ObjectMeta, // ... if you want to set Spec/Status in the client, you can } client = fake.NewSimpleClientset(existingObj) - items = []*metav1.PartialObjectMetadata{tc.ExistingCopy} + items = []*v1alpha1.ClusterServiceVersion{tc.ExistingCopy} } // Create the full Lister @@ -549,9 +479,9 @@ func TestCopyToNamespace(t *testing.T) { // Ensure the in-memory 'proto' has the correct final annotations annotations := proto.GetObjectMeta().GetAnnotations() - require.Equal(t, tc.Hash, annotations[nonStatusCopyHashAnnotation], + require.Equal(t, tc.Hash, annotations[copyCSVSpecHash], "proto should have the non-status hash annotation set") - require.Equal(t, tc.StatusHash, annotations[statusCopyHashAnnotation], + require.Equal(t, tc.StatusHash, annotations[copyCSVStatusHash], "proto should have the status hash annotation set") } else { require.EqualError(t, err, tc.ExpectedError.Error()) @@ -572,10 +502,10 @@ func TestCopyToNamespace(t *testing.T) { } } -type FakeClusterServiceVersionLister []*metav1.PartialObjectMetadata +type FakeClusterServiceVersionLister []*v1alpha1.ClusterServiceVersion -func (l FakeClusterServiceVersionLister) List(selector labels.Selector) ([]*metav1.PartialObjectMetadata, error) { - var result []*metav1.PartialObjectMetadata +func (l FakeClusterServiceVersionLister) List(selector labels.Selector) ([]*v1alpha1.ClusterServiceVersion, error) { + var result []*v1alpha1.ClusterServiceVersion for _, csv := range l { if !selector.Matches(labels.Set(csv.GetLabels())) { continue @@ -585,8 +515,8 @@ func (l FakeClusterServiceVersionLister) List(selector labels.Selector) ([]*meta return result, nil } -func (l FakeClusterServiceVersionLister) Namespace(namespace string) metadatalister.NamespaceLister { - var filtered []*metav1.PartialObjectMetadata +func (l FakeClusterServiceVersionLister) ClusterServiceVersions(namespace string) listersv1alpha1.ClusterServiceVersionNamespaceLister { + var filtered []*v1alpha1.ClusterServiceVersion for _, csv := range l { if csv.GetNamespace() != namespace { continue @@ -596,7 +526,7 @@ func (l FakeClusterServiceVersionLister) Namespace(namespace string) metadatalis return FakeClusterServiceVersionLister(filtered) } -func (l FakeClusterServiceVersionLister) Get(name string) (*metav1.PartialObjectMetadata, error) { +func (l FakeClusterServiceVersionLister) Get(name string) (*v1alpha1.ClusterServiceVersion, error) { for _, csv := range l { if csv.GetName() == name { return csv, nil @@ -606,8 +536,8 @@ func (l FakeClusterServiceVersionLister) Get(name string) (*metav1.PartialObject } var ( - _ metadatalister.Lister = FakeClusterServiceVersionLister{} - _ metadatalister.NamespaceLister = FakeClusterServiceVersionLister{} + _ listersv1alpha1.ClusterServiceVersionLister = FakeClusterServiceVersionLister{} + _ listersv1alpha1.ClusterServiceVersionNamespaceLister = FakeClusterServiceVersionLister{} ) func TestCSVCopyPrototype(t *testing.T) { From d6bf66419ad1191905df156fc13624a8af422c44 Mon Sep 17 00:00:00 2001 From: Todd Short Date: Tue, 10 Jun 2025 14:53:23 -0400 Subject: [PATCH 11/26] Restore operatorgroup_test from master Remove metadatalister Signed-off-by: Todd Short --- .../operators/olm/operatorgroup_test.go | 261 +++--------------- 1 file changed, 46 insertions(+), 215 deletions(-) diff --git a/pkg/controller/operators/olm/operatorgroup_test.go b/pkg/controller/operators/olm/operatorgroup_test.go index ecb8ec841e..ae862c8b8a 100644 --- a/pkg/controller/operators/olm/operatorgroup_test.go +++ b/pkg/controller/operators/olm/operatorgroup_test.go @@ -4,7 +4,6 @@ import ( "fmt" "testing" - "github.com/google/go-cmp/cmp" "github.com/sirupsen/logrus/hooks/test" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -15,67 +14,13 @@ import ( 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" listersv1alpha1 "github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/listers/operators/v1alpha1" "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/registry/resolver/cache" + "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorlister/operatorlisterfakes" "github.com/operator-framework/operator-registry/pkg/registry" ) -// fakeCSVNamespaceLister implements metadatalister.NamespaceLister -type fakeCSVNamespaceLister struct { - namespace string - items []*v1alpha1.ClusterServiceVersion -} - -func (n *fakeCSVNamespaceLister) List(selector labels.Selector) ([]*v1alpha1.ClusterServiceVersion, error) { - var result []*v1alpha1.ClusterServiceVersion - for _, item := range n.items { - if item != nil && item.Namespace == n.namespace { - result = append(result, item) - } - } - return result, nil -} - -func (n *fakeCSVNamespaceLister) Get(name string) (*v1alpha1.ClusterServiceVersion, error) { - for _, item := range n.items { - if item != nil && item.Namespace == n.namespace && item.Name == name { - return item, nil - } - } - return nil, errors.NewNotFound(v1alpha1.Resource("clusterserviceversion"), name) -} - -// fakeCSVLister implements the full listersv1alpha1.ClusterServiceVersionLister interface -// so that Operator.copiedCSVLister = &fakeCSVLister{...} works. -type fakeCSVLister struct { - items []*v1alpha1.ClusterServiceVersion -} - -// List returns all CSV metadata items, ignoring namespaces. -func (f *fakeCSVLister) List(selector labels.Selector) ([]*v1alpha1.ClusterServiceVersion, error) { - return f.items, nil -} - -// Get returns the CSV by name, ignoring namespaces. -func (f *fakeCSVLister) Get(name string) (*v1alpha1.ClusterServiceVersion, error) { - for _, item := range f.items { - if item != nil && item.Name == name { - return item, nil - } - } - return nil, errors.NewNotFound(v1alpha1.Resource("clusterserviceversion"), name) -} - -// Namespace returns a namespace-scoped lister wrapper. -func (f *fakeCSVLister) ClusterServiceVersions(ns string) listersv1alpha1.ClusterServiceVersionNamespaceLister { - return &fakeCSVNamespaceLister{ - namespace: ns, - items: f.items, - } -} - func TestCopyToNamespace(t *testing.T) { gvr := v1alpha1.SchemeGroupVersion.WithResource("clusterserviceversions") @@ -101,12 +46,9 @@ 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{}, + Name: "name", }, Spec: v1alpha1.ClusterServiceVersionSpec{ Replaces: "replacee", @@ -115,16 +57,11 @@ func TestCopyToNamespace(t *testing.T) { Phase: "waxing gibbous", }, }, - // No ExistingCopy: means there's nothing in "to" namespace yet. ExpectedActions: []ktesting.Action{ - // Create the new CSV with nonStatusCopyHashAnnotation ktesting.NewCreateAction(gvr, "to", &v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ Name: "name", Namespace: "to", - Annotations: map[string]string{ - "olm.operatorframework.io/nonStatusCopyHash": "hn-1", - }, }, Spec: v1alpha1.ClusterServiceVersionSpec{ Replaces: "replacee", @@ -133,33 +70,10 @@ func TestCopyToNamespace(t *testing.T) { Phase: "waxing gibbous", }, }), - // UpdateStatus: note that name/namespace remain "name"/"to", - // and we still have nonStatusCopyHashAnnotation: "hn-1". ktesting.NewUpdateSubresourceAction(gvr, "status", "to", &v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ Name: "name", Namespace: "to", - Annotations: map[string]string{ - "olm.operatorframework.io/nonStatusCopyHash": "hn-1", - }, - }, - Spec: v1alpha1.ClusterServiceVersionSpec{ - Replaces: "replacee", - }, - Status: v1alpha1.ClusterServiceVersionStatus{ - Phase: "waxing gibbous", - }, - }), - // Normal Update for the statusCopyHashAnnotation = "hs" - // We still keep the "hn-1" annotation as well, plus Name/Namespace as is. - ktesting.NewUpdateAction(gvr, "to", &v1alpha1.ClusterServiceVersion{ - ObjectMeta: metav1.ObjectMeta{ - Name: "name", - Namespace: "to", - Annotations: map[string]string{ - "olm.operatorframework.io/nonStatusCopyHash": "hn-1", - "olm.operatorframework.io/statusCopyHash": "hs", - }, }, Spec: v1alpha1.ClusterServiceVersionSpec{ Replaces: "replacee", @@ -184,8 +98,7 @@ func TestCopyToNamespace(t *testing.T) { StatusHash: "hs", Prototype: v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ - Name: "name", - Annotations: map[string]string{}, + Name: "name", }, Spec: v1alpha1.ClusterServiceVersionSpec{ Replaces: "replacee", @@ -201,23 +114,25 @@ func TestCopyToNamespace(t *testing.T) { UID: "uid", ResourceVersion: "42", Annotations: map[string]string{ - copyCSVSpecHash: "hn-2", // differs => triggers normal Update - copyCSVStatusHash: "hs", // same => no status update + "$copyhash-nonstatus": "hn-2", + "$copyhash-status": "hs", }, }, }, + ExpectedResult: &v1alpha1.ClusterServiceVersion{ + ObjectMeta: metav1.ObjectMeta{ + Name: "name", + Namespace: "to", + UID: "uid", + }, + }, ExpectedActions: []ktesting.Action{ - // Non-status differs => 1 normal Update ktesting.NewUpdateAction(gvr, "to", &v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ Name: "name", Namespace: "to", UID: "uid", ResourceVersion: "42", - // We'll set the new nonStatusCopyHashAnnotation = "hn-1" - Annotations: map[string]string{ - copyCSVSpecHash: "hn-1", - }, }, Spec: v1alpha1.ClusterServiceVersionSpec{ Replaces: "replacee", @@ -227,13 +142,6 @@ func TestCopyToNamespace(t *testing.T) { }, }), }, - ExpectedResult: &v1alpha1.ClusterServiceVersion{ - ObjectMeta: metav1.ObjectMeta{ - Name: "name", - Namespace: "to", - UID: "uid", - }, - }, }, { Name: "copy status updated if status hash differs", @@ -243,8 +151,7 @@ func TestCopyToNamespace(t *testing.T) { StatusHash: "hs-1", Prototype: v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ - Name: "name", - Annotations: map[string]string{}, + Name: "name", }, Spec: v1alpha1.ClusterServiceVersionSpec{ Replaces: "replacee", @@ -260,43 +167,25 @@ func TestCopyToNamespace(t *testing.T) { UID: "uid", ResourceVersion: "42", Annotations: map[string]string{ - // non-status matches => no normal update - copyCSVSpecHash: "hn", - // status differs => subresource + normal update - copyCSVStatusHash: "hs-2", + "$copyhash-nonstatus": "hn", + "$copyhash-status": "hs-2", }, }, }, + ExpectedResult: &v1alpha1.ClusterServiceVersion{ + ObjectMeta: metav1.ObjectMeta{ + Name: "name", + Namespace: "to", + UID: "uid", + }, + }, ExpectedActions: []ktesting.Action{ - // UpdateStatus (we set the new status, and the in-memory CSV includes the matching nonStatus) ktesting.NewUpdateSubresourceAction(gvr, "status", "to", &v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ Name: "name", Namespace: "to", UID: "uid", ResourceVersion: "42", - Annotations: map[string]string{ - copyCSVSpecHash: "hn", - }, - }, - Spec: v1alpha1.ClusterServiceVersionSpec{ - Replaces: "replacee", - }, - Status: v1alpha1.ClusterServiceVersionStatus{ - Phase: "waxing gibbous", - }, - }), - // Normal Update to set statusCopyHashAnnotation = "hs-1" - ktesting.NewUpdateAction(gvr, "to", &v1alpha1.ClusterServiceVersion{ - ObjectMeta: metav1.ObjectMeta{ - Name: "name", - Namespace: "to", - UID: "uid", - ResourceVersion: "42", - Annotations: map[string]string{ - copyCSVSpecHash: "hn", - copyCSVStatusHash: "hs-1", - }, }, Spec: v1alpha1.ClusterServiceVersionSpec{ Replaces: "replacee", @@ -306,13 +195,6 @@ func TestCopyToNamespace(t *testing.T) { }, }), }, - ExpectedResult: &v1alpha1.ClusterServiceVersion{ - ObjectMeta: metav1.ObjectMeta{ - Name: "name", - Namespace: "to", - UID: "uid", - }, - }, }, { Name: "copy and copy status updated if both hashes differ", @@ -322,8 +204,7 @@ func TestCopyToNamespace(t *testing.T) { StatusHash: "hs-1", Prototype: v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ - Name: "name", - Annotations: map[string]string{}, + Name: "name", }, Spec: v1alpha1.ClusterServiceVersionSpec{ Replaces: "replacee", @@ -339,23 +220,25 @@ func TestCopyToNamespace(t *testing.T) { UID: "uid", ResourceVersion: "42", Annotations: map[string]string{ - // Both nonStatus and status mismatch - copyCSVSpecHash: "hn-2", - copyCSVStatusHash: "hs-2", + "$copyhash-nonstatus": "hn-2", + "$copyhash-status": "hs-2", }, }, }, + ExpectedResult: &v1alpha1.ClusterServiceVersion{ + ObjectMeta: metav1.ObjectMeta{ + Name: "name", + Namespace: "to", + UID: "uid", + }, + }, ExpectedActions: []ktesting.Action{ - // Normal update for the non-status mismatch ktesting.NewUpdateAction(gvr, "to", &v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ Name: "name", Namespace: "to", UID: "uid", ResourceVersion: "42", - Annotations: map[string]string{ - copyCSVSpecHash: "hn-1", - }, }, Spec: v1alpha1.ClusterServiceVersionSpec{ Replaces: "replacee", @@ -364,35 +247,12 @@ func TestCopyToNamespace(t *testing.T) { Phase: "waxing gibbous", }, }), - // UpdateStatus because status hash mismatch ktesting.NewUpdateSubresourceAction(gvr, "status", "to", &v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ Name: "name", Namespace: "to", UID: "uid", ResourceVersion: "42", - Annotations: map[string]string{ - copyCSVSpecHash: "hn-1", - }, - }, - Spec: v1alpha1.ClusterServiceVersionSpec{ - Replaces: "replacee", - }, - Status: v1alpha1.ClusterServiceVersionStatus{ - Phase: "waxing gibbous", - }, - }), - // Normal update for the new statusCopyHashAnnotation - ktesting.NewUpdateAction(gvr, "to", &v1alpha1.ClusterServiceVersion{ - ObjectMeta: metav1.ObjectMeta{ - Name: "name", - Namespace: "to", - UID: "uid", - ResourceVersion: "42", - Annotations: map[string]string{ - copyCSVSpecHash: "hn-1", - copyCSVStatusHash: "hs-1", - }, }, Spec: v1alpha1.ClusterServiceVersionSpec{ Replaces: "replacee", @@ -402,13 +262,6 @@ func TestCopyToNamespace(t *testing.T) { }, }), }, - ExpectedResult: &v1alpha1.ClusterServiceVersion{ - ObjectMeta: metav1.ObjectMeta{ - Name: "name", - Namespace: "to", - UID: "uid", - }, - }, }, { Name: "no action taken if neither hash differs", @@ -418,8 +271,7 @@ func TestCopyToNamespace(t *testing.T) { StatusHash: "hs", Prototype: v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ - Name: "name", - Annotations: map[string]string{}, + Name: "name", }, }, ExistingCopy: &v1alpha1.ClusterServiceVersion{ @@ -428,8 +280,8 @@ func TestCopyToNamespace(t *testing.T) { Namespace: "to", UID: "uid", Annotations: map[string]string{ - copyCSVSpecHash: "hn", - copyCSVStatusHash: "hs", + "$copyhash-nonstatus": "hn", + "$copyhash-status": "hs", }, }, }, @@ -440,64 +292,43 @@ func TestCopyToNamespace(t *testing.T) { UID: "uid", }, }, - ExpectedActions: nil, // no update calls if neither hash differs }, } { t.Run(tc.Name, func(t *testing.T) { - // Create a new fake clientset populated with the "existing copy" if any + lister := &operatorlisterfakes.FakeOperatorLister{} + v1alpha1lister := &operatorlisterfakes.FakeOperatorsV1alpha1Lister{} + lister.OperatorsV1alpha1Returns(v1alpha1lister) client := fake.NewSimpleClientset() - var lister listersv1alpha1.ClusterServiceVersionLister - // If we have an existing CSV in that target namespace, add it to the slice - items := []*v1alpha1.ClusterServiceVersion{} if tc.ExistingCopy != nil { - existingObj := &v1alpha1.ClusterServiceVersion{ + client = fake.NewSimpleClientset(&v1alpha1.ClusterServiceVersion{ ObjectMeta: tc.ExistingCopy.ObjectMeta, - // ... if you want to set Spec/Status in the client, you can - } - client = fake.NewSimpleClientset(existingObj) - items = []*v1alpha1.ClusterServiceVersion{tc.ExistingCopy} - } - - // Create the full Lister - lister = &fakeCSVLister{ - items: items, + }) + } else { + v1alpha1lister.ClusterServiceVersionListerReturns(FakeClusterServiceVersionLister(nil)) } logger, _ := test.NewNullLogger() o := &Operator{ - copiedCSVLister: lister, + copiedCSVLister: v1alpha1lister.ClusterServiceVersionLister(), client: client, logger: logger, } - proto := tc.Prototype.DeepCopy() - result, err := o.copyToNamespace(proto, tc.FromNamespace, tc.ToNamespace, tc.Hash, tc.StatusHash) + result, err := o.copyToNamespace(tc.Prototype.DeepCopy(), tc.FromNamespace, tc.ToNamespace, tc.Hash, tc.StatusHash) if tc.ExpectedError == nil { require.NoError(t, err) - - // Ensure the in-memory 'proto' has the correct final annotations - annotations := proto.GetObjectMeta().GetAnnotations() - require.Equal(t, tc.Hash, annotations[copyCSVSpecHash], - "proto should have the non-status hash annotation set") - require.Equal(t, tc.StatusHash, annotations[copyCSVStatusHash], - "proto should have the status hash annotation set") } else { require.EqualError(t, err, tc.ExpectedError.Error()) } - - if diff := cmp.Diff(tc.ExpectedResult, result); diff != "" { - t.Errorf("incorrect result: %v", diff) - } + assert.Equal(t, tc.ExpectedResult, result) actions := client.Actions() if len(actions) == 0 { actions = nil } - if diff := cmp.Diff(tc.ExpectedActions, actions); diff != "" { - t.Errorf("incorrect actions: %v", diff) - } + assert.Equal(t, tc.ExpectedActions, actions) }) } } From 379e8434b35bf560bf98d3895f69bbb6a40dd511 Mon Sep 17 00:00:00 2001 From: Todd Short Date: Tue, 10 Jun 2025 15:15:28 -0400 Subject: [PATCH 12/26] Remove more PartialObjectMetadata Signed-off-by: Todd Short --- pkg/controller/operators/olm/operator.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/controller/operators/olm/operator.go b/pkg/controller/operators/olm/operator.go index a72d13663d..39aefa8bbe 100644 --- a/pkg/controller/operators/olm/operator.go +++ b/pkg/controller/operators/olm/operator.go @@ -1317,7 +1317,7 @@ func (a *Operator) handleClusterServiceVersionDeletion(obj interface{}) { } } -func (a *Operator) removeDanglingChildCSVs(csv *metav1.PartialObjectMetadata) error { +func (a *Operator) removeDanglingChildCSVs(csv *v1alpha1.ClusterServiceVersion) error { logger := a.logger.WithFields(logrus.Fields{ "id": queueinformer.NewLoopID(), "csv": csv.GetName(), @@ -1365,7 +1365,7 @@ func (a *Operator) removeDanglingChildCSVs(csv *metav1.PartialObjectMetadata) er return nil } -func (a *Operator) deleteChild(csv *metav1.PartialObjectMetadata, logger *logrus.Entry) error { +func (a *Operator) deleteChild(csv *v1alpha1.ClusterServiceVersion, logger *logrus.Entry) error { logger.Debug("gcing csv") return a.client.OperatorsV1alpha1().ClusterServiceVersions(csv.GetNamespace()).Delete(context.TODO(), csv.GetName(), metav1.DeleteOptions{}) } @@ -1910,7 +1910,7 @@ func (a *Operator) createCSVCopyingDisabledEvent(csv *v1alpha1.ClusterServiceVer } func (a *Operator) syncGcCsv(obj interface{}) (syncError error) { - clusterServiceVersion, ok := obj.(*metav1.PartialObjectMetadata) + clusterServiceVersion, ok := obj.(*v1alpha1.ClusterServiceVersion) if !ok { a.logger.Debugf("wrong type: %#v", obj) return fmt.Errorf("casting ClusterServiceVersion failed") From c50f880c6f33111b89a3d9500bf179004e5e4fb7 Mon Sep 17 00:00:00 2001 From: Todd Short Date: Tue, 10 Jun 2025 15:17:07 -0400 Subject: [PATCH 13/26] Remove hashes from operator_test Signed-off-by: Todd Short --- pkg/controller/operators/olm/operator_test.go | 6 ------ 1 file changed, 6 deletions(-) diff --git a/pkg/controller/operators/olm/operator_test.go b/pkg/controller/operators/olm/operator_test.go index c117cbffb6..fbaa32492a 100644 --- a/pkg/controller/operators/olm/operator_test.go +++ b/pkg/controller/operators/olm/operator_test.go @@ -5053,8 +5053,6 @@ func TestSyncOperatorGroups(t *testing.T) { withAnnotations(targetCSV.DeepCopy(), map[string]string{ operatorsv1.OperatorGroupAnnotationKey: "operator-group-1", operatorsv1.OperatorGroupNamespaceAnnotationKey: operatorNamespace, - "olm.operatorframework.io/nonStatusCopyHash": "9ZxHAHQTkxzAQd7Qkk4Qjz3VAkA8lXwuX9mDX6", - "olm.operatorframework.io/statusCopyHash": "bedtcmN999WBSJ1RHvM7JfN2NJITrUjJ0g0MoH", }), labels.Merge(targetCSV.GetLabels(), map[string]string{v1alpha1.CopiedLabelKey: operatorNamespace}), ), @@ -5163,8 +5161,6 @@ func TestSyncOperatorGroups(t *testing.T) { withAnnotations(targetCSV.DeepCopy(), map[string]string{ operatorsv1.OperatorGroupAnnotationKey: "operator-group-1", operatorsv1.OperatorGroupNamespaceAnnotationKey: operatorNamespace, - "olm.operatorframework.io/nonStatusCopyHash": "9ZxHAHQTkxzAQd7Qkk4Qjz3VAkA8lXwuX9mDX6", - "olm.operatorframework.io/statusCopyHash": "bedtcmN999WBSJ1RHvM7JfN2NJITrUjJ0g0MoH", }), labels.Merge(targetCSV.GetLabels(), map[string]string{v1alpha1.CopiedLabelKey: operatorNamespace}), ), @@ -5325,8 +5321,6 @@ func TestSyncOperatorGroups(t *testing.T) { withAnnotations(targetCSV.DeepCopy(), map[string]string{ operatorsv1.OperatorGroupAnnotationKey: "operator-group-1", operatorsv1.OperatorGroupNamespaceAnnotationKey: operatorNamespace, - "olm.operatorframework.io/nonStatusCopyHash": "9ZxHAHQTkxzAQd7Qkk4Qjz3VAkA8lXwuX9mDX6", - "olm.operatorframework.io/statusCopyHash": "bedtcmN999WBSJ1RHvM7JfN2NJITrUjJ0g0MoH", }), labels.Merge(targetCSV.GetLabels(), map[string]string{v1alpha1.CopiedLabelKey: operatorNamespace}), ), From fc367b7ed4cbd844db98940b9077d63770e41afe Mon Sep 17 00:00:00 2001 From: Todd Short Date: Tue, 10 Jun 2025 16:09:43 -0400 Subject: [PATCH 14/26] Fix error messages for static-analysis Signed-off-by: Todd Short --- pkg/controller/operators/catalog/operator.go | 2 +- pkg/controller/operators/olm/operator.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/controller/operators/catalog/operator.go b/pkg/controller/operators/catalog/operator.go index 2d54e77aac..dc2ec67537 100644 --- a/pkg/controller/operators/catalog/operator.go +++ b/pkg/controller/operators/catalog/operator.go @@ -271,7 +271,7 @@ func NewOperator(ctx context.Context, kubeconfigPath string, clock utilclock.Clo } return csv, nil } - return nil, fmt.Errorf("Unable to convert input to CSV") + return nil, fmt.Errorf("unable to convert input to CSV") } if err := prunedCSVInformer.SetTransform(prunedCSVTransformFunc); err != nil { diff --git a/pkg/controller/operators/olm/operator.go b/pkg/controller/operators/olm/operator.go index 39aefa8bbe..164189d770 100644 --- a/pkg/controller/operators/olm/operator.go +++ b/pkg/controller/operators/olm/operator.go @@ -345,7 +345,7 @@ func newOperatorWithConfig(ctx context.Context, config *operatorConfig) (*Operat csv.Annotations[copyCSVStatusHash] = statusHash return csv, nil } - return nil, fmt.Errorf("Unable to convert input to CSV") + return nil, fmt.Errorf("unable to convert input to CSV") } if err := copiedCSVInformer.SetTransform(copiedCSVTransformFunc); err != nil { From 3e3568f974ce07017ad57bf02164ced96a35f80b Mon Sep 17 00:00:00 2001 From: Todd Short Date: Tue, 10 Jun 2025 16:27:59 -0400 Subject: [PATCH 15/26] Update test annotations and test client Signed-off-by: Todd Short --- .../operators/olm/operatorgroup_test.go | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/pkg/controller/operators/olm/operatorgroup_test.go b/pkg/controller/operators/olm/operatorgroup_test.go index ae862c8b8a..bf7c2f688e 100644 --- a/pkg/controller/operators/olm/operatorgroup_test.go +++ b/pkg/controller/operators/olm/operatorgroup_test.go @@ -167,8 +167,8 @@ func TestCopyToNamespace(t *testing.T) { UID: "uid", ResourceVersion: "42", Annotations: map[string]string{ - "$copyhash-nonstatus": "hn", - "$copyhash-status": "hs-2", + "$copyhash-spec": "hn", + "$copyhash-status": "hs-2", }, }, }, @@ -220,8 +220,8 @@ func TestCopyToNamespace(t *testing.T) { UID: "uid", ResourceVersion: "42", Annotations: map[string]string{ - "$copyhash-nonstatus": "hn-2", - "$copyhash-status": "hs-2", + "$copyhash-spec": "hn-2", + "$copyhash-status": "hs-2", }, }, }, @@ -280,8 +280,8 @@ func TestCopyToNamespace(t *testing.T) { Namespace: "to", UID: "uid", Annotations: map[string]string{ - "$copyhash-nonstatus": "hn", - "$copyhash-status": "hs", + "$copyhash-spec": "hn", + "$copyhash-status": "hs", }, }, }, @@ -301,9 +301,8 @@ func TestCopyToNamespace(t *testing.T) { client := fake.NewSimpleClientset() if tc.ExistingCopy != nil { - client = fake.NewSimpleClientset(&v1alpha1.ClusterServiceVersion{ - ObjectMeta: tc.ExistingCopy.ObjectMeta, - }) + client = fake.NewSimpleClientset(tc.ExistingCopy) + v1alpha1lister.ClusterServiceVersionListerReturns(FakeClusterServiceVersionLister{tc.ExistingCopy}) } else { v1alpha1lister.ClusterServiceVersionListerReturns(FakeClusterServiceVersionLister(nil)) } From fa8469577733ce49d112b2a99beb8c584439310f Mon Sep 17 00:00:00 2001 From: Todd Short Date: Wed, 11 Jun 2025 10:02:23 -0400 Subject: [PATCH 16/26] Rename pruning to listerwatcher Signed-off-by: Todd Short --- pkg/controller/operators/catalog/operator.go | 4 ++-- .../internal/{pruning => listerwatcher}/listerwatcher.go | 2 +- pkg/controller/operators/olm/operator.go | 5 +++-- 3 files changed, 6 insertions(+), 5 deletions(-) rename pkg/controller/operators/internal/{pruning => listerwatcher}/listerwatcher.go (97%) diff --git a/pkg/controller/operators/catalog/operator.go b/pkg/controller/operators/catalog/operator.go index dc2ec67537..c26c3da927 100644 --- a/pkg/controller/operators/catalog/operator.go +++ b/pkg/controller/operators/catalog/operator.go @@ -64,7 +64,7 @@ import ( olmerrors "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/errors" "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/install" "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/catalog/subscription" - "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/internal/pruning" + "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/internal/listerwatcher" "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/registry" "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/registry/grpc" "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/registry/reconciler" @@ -231,7 +231,7 @@ func NewOperator(ctx context.Context, kubeconfigPath string, clock utilclock.Clo // Fields are pruned from local copies of the objects managed // by this informer in order to reduce cached size. prunedCSVInformer := cache.NewSharedIndexInformerWithOptions( - pruning.NewListerWatcher( + listerwatcher.NewListerWatcher( op.client, metav1.NamespaceAll, func(options *metav1.ListOptions) { diff --git a/pkg/controller/operators/internal/pruning/listerwatcher.go b/pkg/controller/operators/internal/listerwatcher/listerwatcher.go similarity index 97% rename from pkg/controller/operators/internal/pruning/listerwatcher.go rename to pkg/controller/operators/internal/listerwatcher/listerwatcher.go index b069de2259..59bac9f9f4 100644 --- a/pkg/controller/operators/internal/pruning/listerwatcher.go +++ b/pkg/controller/operators/internal/listerwatcher/listerwatcher.go @@ -1,4 +1,4 @@ -package pruning +package listerwatcher import ( "context" diff --git a/pkg/controller/operators/olm/operator.go b/pkg/controller/operators/olm/operator.go index 164189d770..7f5960dca6 100644 --- a/pkg/controller/operators/olm/operator.go +++ b/pkg/controller/operators/olm/operator.go @@ -46,7 +46,7 @@ import ( operatorsv1alpha1listers "github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/listers/operators/v1alpha1" "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/certs" "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/install" - "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/internal/pruning" + "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/internal/listerwatcher" "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/labeller" "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/olm/overrides" "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/olm/plugins" @@ -307,8 +307,9 @@ func newOperatorWithConfig(ctx context.Context, config *operatorConfig) (*Operat // A separate informer solely for CSV copies. Fields // are pruned from local copies of the objects managed // by this informer in order to reduce cached size. + copiedCSVInformer := cache.NewSharedIndexInformerWithOptions( - pruning.NewListerWatcher( + listerwatcher.NewListerWatcher( op.client, namespace, func(opts *metav1.ListOptions) { From 15f090ad8905204d902d9e7e05174fdd74970a19 Mon Sep 17 00:00:00 2001 From: Todd Short Date: Wed, 11 Jun 2025 14:21:16 -0400 Subject: [PATCH 17/26] Set resync to 6h Signed-off-by: Todd Short --- cmd/catalog/main.go | 2 +- cmd/olm/main.go | 2 +- pkg/controller/operators/olm/config.go | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/cmd/catalog/main.go b/cmd/catalog/main.go index b82f1689cb..40d71c15de 100644 --- a/cmd/catalog/main.go +++ b/cmd/catalog/main.go @@ -27,7 +27,7 @@ import ( const ( catalogNamespaceEnvVarName = "GLOBAL_CATALOG_NAMESPACE" - defaultWakeupInterval = 15 * time.Minute + defaultWakeupInterval = 6 * time.Hour defaultCatalogNamespace = "olm" defaultConfigMapServerImage = "quay.io/operator-framework/configmap-operator-registry:latest" defaultOPMImage = "quay.io/operator-framework/opm:latest" diff --git a/cmd/olm/main.go b/cmd/olm/main.go index 715ae9aea0..5e81424c33 100644 --- a/cmd/olm/main.go +++ b/cmd/olm/main.go @@ -33,7 +33,7 @@ import ( ) const ( - defaultWakeupInterval = 5 * time.Minute + defaultWakeupInterval = 6 * time.Hour defaultOperatorName = "" defaultPackageServerStatusName = "" ) diff --git a/pkg/controller/operators/olm/config.go b/pkg/controller/operators/olm/config.go index fa472c7130..bc12683bd4 100644 --- a/pkg/controller/operators/olm/config.go +++ b/pkg/controller/operators/olm/config.go @@ -101,7 +101,7 @@ func (o *operatorConfig) validate() (err error) { func defaultOperatorConfig() *operatorConfig { return &operatorConfig{ - resyncPeriod: queueinformer.ResyncWithJitter(30*time.Second, 0.2), + resyncPeriod: queueinformer.ResyncWithJitter(6*time.Hour, 0.2), operatorNamespace: "default", watchedNamespaces: []string{metav1.NamespaceAll}, clock: utilclock.RealClock{}, From 93a78a8d691094a00234d2d4188f767a588e668d Mon Sep 17 00:00:00 2001 From: Todd Short Date: Wed, 11 Jun 2025 14:27:44 -0400 Subject: [PATCH 18/26] Add CSV copy revert syncer Signed-off-by: Todd Short --- pkg/controller/operators/olm/operator.go | 85 ++++++++++++++++++- pkg/controller/operators/olm/operatorgroup.go | 5 ++ 2 files changed, 86 insertions(+), 4 deletions(-) diff --git a/pkg/controller/operators/olm/operator.go b/pkg/controller/operators/olm/operator.go index 7f5960dca6..0c7e4e8241 100644 --- a/pkg/controller/operators/olm/operator.go +++ b/pkg/controller/operators/olm/operator.go @@ -307,7 +307,6 @@ func newOperatorWithConfig(ctx context.Context, config *operatorConfig) (*Operat // A separate informer solely for CSV copies. Fields // are pruned from local copies of the objects managed // by this informer in order to reduce cached size. - copiedCSVInformer := cache.NewSharedIndexInformerWithOptions( listerwatcher.NewListerWatcher( op.client, @@ -328,6 +327,7 @@ func newOperatorWithConfig(ctx context.Context, config *operatorConfig) (*Operat // a hash over the full CSV to store as an annotation copiedCSVTransformFunc := func(i interface{}) (interface{}, error) { if csv, ok := i.(*v1alpha1.ClusterServiceVersion); ok { + config.logger.WithField("name", fmt.Sprintf("%s/%s", csv.Namespace, csv.Name)).Info("Transforming copied CSV") specHash, statusHash, err := copyableCSVHash(csv) if err != nil { return nil, err @@ -356,6 +356,28 @@ func newOperatorWithConfig(ctx context.Context, config *operatorConfig) (*Operat informersByNamespace[namespace].CopiedCSVInformer = copiedCSVInformer informersByNamespace[namespace].CopiedCSVLister = op.copiedCSVLister + // Register separate queue for reverting copied csvs + copiedCSVRevertQueue := workqueue.NewTypedRateLimitingQueueWithConfig[types.NamespacedName]( + workqueue.DefaultTypedControllerRateLimiter[types.NamespacedName](), + workqueue.TypedRateLimitingQueueConfig[types.NamespacedName]{ + Name: fmt.Sprintf("%s/csv-revert", namespace), + }) + op.copiedCSVGCQueueSet.Set(namespace, copiedCSVRevertQueue) + copiedCSVRevertQueueInformer, err := queueinformer.NewQueueInformer( + ctx, + queueinformer.WithInformer(copiedCSVInformer), + queueinformer.WithLogger(op.logger), + queueinformer.WithQueue(copiedCSVRevertQueue), + queueinformer.WithIndexer(copiedCSVInformer.GetIndexer()), + queueinformer.WithSyncer(queueinformer.LegacySyncHandler(op.syncRevertCsv).ToSyncer()), + ) + if err != nil { + return nil, err + } + if err := op.RegisterQueueInformer(copiedCSVRevertQueueInformer); err != nil { + return nil, err + } + // Register separate queue for gcing copied csvs copiedCSVGCQueue := workqueue.NewTypedRateLimitingQueueWithConfig[types.NamespacedName]( workqueue.DefaultTypedControllerRateLimiter[types.NamespacedName](), @@ -1723,6 +1745,8 @@ func (a *Operator) syncCopyCSV(obj interface{}) (syncError error) { return fmt.Errorf("casting ClusterServiceVersion failed") } + a.logger.WithField("name", fmt.Sprintf("%s/%s", clusterServiceVersion.Namespace, clusterServiceVersion.Name)).Info("syncCopyCSV") + olmConfig, err := a.client.OperatorsV1().OLMConfigs().Get(context.TODO(), "cluster", metav1.GetOptions{}) if err != nil && !apierrors.IsNotFound(err) { return err @@ -1750,9 +1774,13 @@ func (a *Operator) syncCopyCSV(obj interface{}) (syncError error) { return } - logger.WithFields(logrus.Fields{ - "targetNamespaces": strings.Join(operatorGroup.Status.Namespaces, ","), - }).Debug("copying csv to targets") + if len(operatorGroup.Status.Namespaces) == 1 && operatorGroup.Status.Namespaces[0] == "" { + logger.Debug("copying csv to targets in all namespaces") + } else { + logger.WithFields(logrus.Fields{ + "targetNamespaces": strings.Join(operatorGroup.Status.Namespaces, ","), + }).Debug("copying csv to targets") + } copiedCSVsAreEnabled, err := a.copiedCSVsAreEnabled() if err != nil { @@ -1910,6 +1938,55 @@ func (a *Operator) createCSVCopyingDisabledEvent(csv *v1alpha1.ClusterServiceVer return nil } +func GetCopiedNamespace(c *v1alpha1.ClusterServiceVersion) string { + annotations := c.GetAnnotations() + if annotations != nil { + operatorNamespace, ok := annotations[v1alpha1.OperatorGroupNamespaceAnnotationKey] + if ok && c.GetNamespace() != operatorNamespace { + return operatorNamespace + } + } + + if labels := c.GetLabels(); labels != nil { + if l, ok := labels[v1alpha1.CopiedLabelKey]; ok { + return l + } + } + return "" +} + +func (a *Operator) syncRevertCsv(obj interface{}) error { + csv, ok := obj.(*v1alpha1.ClusterServiceVersion) + if !ok { + a.logger.Debugf("wrong type: %#v", obj) + return fmt.Errorf("casting ClusterServiceVersion failed") + } + logger := a.logger.WithField("csv", fmt.Sprintf("%s/%s", csv.GetNamespace(), csv.GetName())) + logger.Info("syncRevertCsv") + ns := GetCopiedNamespace(csv) + if ns == "" { + logger.Info("syncRevertCsv: Unable to get copied-from namespace") + return nil + } + prototype, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(ns).Get(context.TODO(), csv.GetName(), metav1.GetOptions{}) + if err != nil { + logger.Info("syncRevertCsv: Unable to get prototype") + return err + } + var copyPrototype v1alpha1.ClusterServiceVersion + csvCopyPrototype(prototype, ©Prototype) + specHash, statusHash, err := copyableCSVHash(©Prototype) + if err != nil { + logger.Info("syncRevertCsv: Unable to hash") + return err + } + _, err = a.copyToNamespace(©Prototype, ns, csv.GetNamespace(), specHash, statusHash) + if err != nil { + logger.WithError(err).Info("syncRevertCsv: copyToNamespace failed") + } + return err +} + func (a *Operator) syncGcCsv(obj interface{}) (syncError error) { clusterServiceVersion, ok := obj.(*v1alpha1.ClusterServiceVersion) if !ok { diff --git a/pkg/controller/operators/olm/operatorgroup.go b/pkg/controller/operators/olm/operatorgroup.go index 5bfd062d6f..ae4e08ed83 100644 --- a/pkg/controller/operators/olm/operatorgroup.go +++ b/pkg/controller/operators/olm/operatorgroup.go @@ -798,6 +798,11 @@ func (a *Operator) copyToNamespace(prototype *v1alpha1.ClusterServiceVersion, ns return nil, fmt.Errorf("bug: can not copy to active namespace %v", nsFrom) } + a.logger.WithFields(logrus.Fields{ + "nsFrom": nsFrom, + "nsTo": nsTo, + }).Info("copyToNamespace") + prototype.Namespace = nsTo prototype.ResourceVersion = "" prototype.UID = "" From d067f2c5972f09b91a6c8cee2412c4d5ceb279bf Mon Sep 17 00:00:00 2001 From: Todd Short Date: Wed, 11 Jun 2025 14:31:37 -0400 Subject: [PATCH 19/26] Log tweaks Signed-off-by: Todd Short --- pkg/controller/operators/olm/operatorgroup.go | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/pkg/controller/operators/olm/operatorgroup.go b/pkg/controller/operators/olm/operatorgroup.go index ae4e08ed83..49635cd655 100644 --- a/pkg/controller/operators/olm/operatorgroup.go +++ b/pkg/controller/operators/olm/operatorgroup.go @@ -798,11 +798,6 @@ func (a *Operator) copyToNamespace(prototype *v1alpha1.ClusterServiceVersion, ns return nil, fmt.Errorf("bug: can not copy to active namespace %v", nsFrom) } - a.logger.WithFields(logrus.Fields{ - "nsFrom": nsFrom, - "nsTo": nsTo, - }).Info("copyToNamespace") - prototype.Namespace = nsTo prototype.ResourceVersion = "" prototype.UID = "" @@ -840,6 +835,10 @@ func (a *Operator) copyToNamespace(prototype *v1alpha1.ClusterServiceVersion, ns 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) } + a.logger.WithFields(logrus.Fields{ + "nsFrom": nsFrom, + "nsTo": nsTo, + }).Info("copyToNamespace: updated Metadata+Spec") } else { // Avoid mutating cached copied CSV. updated = prototype @@ -850,6 +849,10 @@ 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) } + a.logger.WithFields(logrus.Fields{ + "nsFrom": nsFrom, + "nsTo": nsTo, + }).Info("copyToNamespace: updated Status") } return &v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ From 95f01aa70e384885c6abdd961be34061c5f0354f Mon Sep 17 00:00:00 2001 From: Todd Short Date: Wed, 11 Jun 2025 14:51:28 -0400 Subject: [PATCH 20/26] Consolidate revert and gc syncers Signed-off-by: Todd Short --- pkg/controller/operators/olm/operator.go | 70 ++++++++---------------- 1 file changed, 24 insertions(+), 46 deletions(-) diff --git a/pkg/controller/operators/olm/operator.go b/pkg/controller/operators/olm/operator.go index 0c7e4e8241..5a124124b5 100644 --- a/pkg/controller/operators/olm/operator.go +++ b/pkg/controller/operators/olm/operator.go @@ -356,33 +356,11 @@ func newOperatorWithConfig(ctx context.Context, config *operatorConfig) (*Operat informersByNamespace[namespace].CopiedCSVInformer = copiedCSVInformer informersByNamespace[namespace].CopiedCSVLister = op.copiedCSVLister - // Register separate queue for reverting copied csvs - copiedCSVRevertQueue := workqueue.NewTypedRateLimitingQueueWithConfig[types.NamespacedName]( - workqueue.DefaultTypedControllerRateLimiter[types.NamespacedName](), - workqueue.TypedRateLimitingQueueConfig[types.NamespacedName]{ - Name: fmt.Sprintf("%s/csv-revert", namespace), - }) - op.copiedCSVGCQueueSet.Set(namespace, copiedCSVRevertQueue) - copiedCSVRevertQueueInformer, err := queueinformer.NewQueueInformer( - ctx, - queueinformer.WithInformer(copiedCSVInformer), - queueinformer.WithLogger(op.logger), - queueinformer.WithQueue(copiedCSVRevertQueue), - queueinformer.WithIndexer(copiedCSVInformer.GetIndexer()), - queueinformer.WithSyncer(queueinformer.LegacySyncHandler(op.syncRevertCsv).ToSyncer()), - ) - if err != nil { - return nil, err - } - if err := op.RegisterQueueInformer(copiedCSVRevertQueueInformer); err != nil { - return nil, err - } - // Register separate queue for gcing copied csvs copiedCSVGCQueue := workqueue.NewTypedRateLimitingQueueWithConfig[types.NamespacedName]( workqueue.DefaultTypedControllerRateLimiter[types.NamespacedName](), workqueue.TypedRateLimitingQueueConfig[types.NamespacedName]{ - Name: fmt.Sprintf("%s/csv-gc", namespace), + Name: fmt.Sprintf("%s/csv-gc-revert", namespace), }) op.copiedCSVGCQueueSet.Set(namespace, copiedCSVGCQueue) copiedCSVGCQueueInformer, err := queueinformer.NewQueueInformer( @@ -391,7 +369,7 @@ func newOperatorWithConfig(ctx context.Context, config *operatorConfig) (*Operat queueinformer.WithLogger(op.logger), queueinformer.WithQueue(copiedCSVGCQueue), queueinformer.WithIndexer(copiedCSVInformer.GetIndexer()), - queueinformer.WithSyncer(queueinformer.LegacySyncHandler(op.syncGcCsv).ToSyncer()), + queueinformer.WithSyncer(queueinformer.LegacySyncHandler(op.syncRevertGcCsv).ToSyncer()), ) if err != nil { return nil, err @@ -1955,51 +1933,51 @@ func GetCopiedNamespace(c *v1alpha1.ClusterServiceVersion) string { return "" } -func (a *Operator) syncRevertCsv(obj interface{}) error { +func (a *Operator) syncRevertGcCsv(obj interface{}) error { csv, ok := obj.(*v1alpha1.ClusterServiceVersion) if !ok { a.logger.Debugf("wrong type: %#v", obj) return fmt.Errorf("casting ClusterServiceVersion failed") } - logger := a.logger.WithField("csv", fmt.Sprintf("%s/%s", csv.GetNamespace(), csv.GetName())) - logger.Info("syncRevertCsv") + if !v1alpha1.IsCopied(csv) { + return nil + } + + logger := a.logger.WithField("csv", fmt.Sprintf("%s/%s", csv.Namespace, csv.Name)) + + // check for any garbage collection + logger.Info("syncRevertGcCsv: removeDanglingChildCSVs") + err := a.removeDanglingChildCSVs(csv) + if err != nil { + return err + } + + // Check for a revert ns := GetCopiedNamespace(csv) if ns == "" { - logger.Info("syncRevertCsv: Unable to get copied-from namespace") + logger.Info("syncRevertGcCsv: Failed to get copied-from namespace") return nil } - prototype, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(ns).Get(context.TODO(), csv.GetName(), metav1.GetOptions{}) + prototype, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(ns).Get(context.TODO(), csv.Name, metav1.GetOptions{}) if err != nil { - logger.Info("syncRevertCsv: Unable to get prototype") + logger.Info("syncRevertGcCsv: Failed to get prototype") return err } var copyPrototype v1alpha1.ClusterServiceVersion csvCopyPrototype(prototype, ©Prototype) specHash, statusHash, err := copyableCSVHash(©Prototype) if err != nil { - logger.Info("syncRevertCsv: Unable to hash") + logger.Info("syncRevertGcCsv: Failed to hash") return err } - _, err = a.copyToNamespace(©Prototype, ns, csv.GetNamespace(), specHash, statusHash) + logger.Info("syncRevertGcCsv: copyToNamespace") + _, err = a.copyToNamespace(©Prototype, ns, csv.Namespace, specHash, statusHash) if err != nil { - logger.WithError(err).Info("syncRevertCsv: copyToNamespace failed") + logger.WithError(err).Info("syncRevertGcCsv: copyToNamespace failed") } return err } -func (a *Operator) syncGcCsv(obj interface{}) (syncError error) { - clusterServiceVersion, ok := obj.(*v1alpha1.ClusterServiceVersion) - if !ok { - a.logger.Debugf("wrong type: %#v", obj) - return fmt.Errorf("casting ClusterServiceVersion failed") - } - if v1alpha1.IsCopied(clusterServiceVersion) { - syncError = a.removeDanglingChildCSVs(clusterServiceVersion) - return - } - return -} - // operatorGroupFromAnnotations returns the OperatorGroup for the CSV only if the CSV is active one in the group func (a *Operator) operatorGroupFromAnnotations(logger *logrus.Entry, csv *v1alpha1.ClusterServiceVersion) *operatorsv1.OperatorGroup { annotations := csv.GetAnnotations() From 2cd56b40081032280390041fa1a1cd71b7c20292 Mon Sep 17 00:00:00 2001 From: Todd Short Date: Wed, 11 Jun 2025 15:50:33 -0400 Subject: [PATCH 21/26] Add logging and reduce the amount of metadata in the TransformFunc Signed-off-by: Todd Short --- pkg/controller/operators/olm/operator.go | 20 +++++++++++++++++-- pkg/controller/operators/olm/operatorgroup.go | 7 +++++++ 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/pkg/controller/operators/olm/operator.go b/pkg/controller/operators/olm/operator.go index 5a124124b5..7b786968d1 100644 --- a/pkg/controller/operators/olm/operator.go +++ b/pkg/controller/operators/olm/operator.go @@ -338,8 +338,24 @@ func newOperatorWithConfig(ctx context.Context, config *operatorConfig) (*Operat Spec: v1alpha1.ClusterServiceVersionSpec{}, Status: v1alpha1.ClusterServiceVersionStatus{}, } - if csv.Annotations == nil { - csv.Annotations = make(map[string]string, 2) + // copy only the nececessary labels + labels := csv.Labels + csv.Labels = make(map[string]string, 2) + if l, ok := labels[v1alpha1.CopiedLabelKey]; ok { + csv.Labels[v1alpha1.CopiedLabelKey] = l + } + if l, ok := labels[install.OLMManagedLabelKey]; ok { + csv.Labels[install.OLMManagedLabelKey] = l + } + + // copy only the nececessary annotations + annotations := csv.Annotations + csv.Annotations = make(map[string]string, 4) + if a, ok := annotations[operatorsv1.OperatorGroupAnnotationKey]; ok { + csv.Annotations[operatorsv1.OperatorGroupAnnotationKey] = a + } + if a, ok := annotations[operatorsv1.OperatorGroupNamespaceAnnotationKey]; ok { + csv.Annotations[operatorsv1.OperatorGroupNamespaceAnnotationKey] = a } // fake CSV hashes for tracking purposes only csv.Annotations[copyCSVSpecHash] = specHash diff --git a/pkg/controller/operators/olm/operatorgroup.go b/pkg/controller/operators/olm/operatorgroup.go index 49635cd655..c9f6802df8 100644 --- a/pkg/controller/operators/olm/operatorgroup.go +++ b/pkg/controller/operators/olm/operatorgroup.go @@ -812,6 +812,11 @@ 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) } + a.logger.WithFields(logrus.Fields{ + "nsFrom": nsFrom, + "nsTo": nsTo, + "csv": prototype.Name, + }).Info("copyToNamespace: created") return &v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ Name: created.Name, @@ -838,6 +843,7 @@ func (a *Operator) copyToNamespace(prototype *v1alpha1.ClusterServiceVersion, ns a.logger.WithFields(logrus.Fields{ "nsFrom": nsFrom, "nsTo": nsTo, + "csv": updated.Name, }).Info("copyToNamespace: updated Metadata+Spec") } else { // Avoid mutating cached copied CSV. @@ -852,6 +858,7 @@ func (a *Operator) copyToNamespace(prototype *v1alpha1.ClusterServiceVersion, ns a.logger.WithFields(logrus.Fields{ "nsFrom": nsFrom, "nsTo": nsTo, + "csv": updated.Name, }).Info("copyToNamespace: updated Status") } return &v1alpha1.ClusterServiceVersion{ From ed9cb76f7cf8cb3cc51467d9a2025ecab8901f3d Mon Sep 17 00:00:00 2001 From: Todd Short Date: Wed, 11 Jun 2025 16:50:30 -0400 Subject: [PATCH 22/26] Handle the copy CSV revert via a requeue of the primary CSV Signed-off-by: Todd Short --- pkg/controller/operators/olm/operator.go | 56 ++++--------------- pkg/controller/operators/olm/operatorgroup.go | 15 ----- 2 files changed, 10 insertions(+), 61 deletions(-) diff --git a/pkg/controller/operators/olm/operator.go b/pkg/controller/operators/olm/operator.go index 7b786968d1..b2a88d1007 100644 --- a/pkg/controller/operators/olm/operator.go +++ b/pkg/controller/operators/olm/operator.go @@ -327,7 +327,6 @@ func newOperatorWithConfig(ctx context.Context, config *operatorConfig) (*Operat // a hash over the full CSV to store as an annotation copiedCSVTransformFunc := func(i interface{}) (interface{}, error) { if csv, ok := i.(*v1alpha1.ClusterServiceVersion); ok { - config.logger.WithField("name", fmt.Sprintf("%s/%s", csv.Namespace, csv.Name)).Info("Transforming copied CSV") specHash, statusHash, err := copyableCSVHash(csv) if err != nil { return nil, err @@ -385,7 +384,7 @@ func newOperatorWithConfig(ctx context.Context, config *operatorConfig) (*Operat queueinformer.WithLogger(op.logger), queueinformer.WithQueue(copiedCSVGCQueue), queueinformer.WithIndexer(copiedCSVInformer.GetIndexer()), - queueinformer.WithSyncer(queueinformer.LegacySyncHandler(op.syncRevertGcCsv).ToSyncer()), + queueinformer.WithSyncer(queueinformer.LegacySyncHandler(op.syncResyncCsv).ToSyncer()), ) if err != nil { return nil, err @@ -1739,8 +1738,6 @@ func (a *Operator) syncCopyCSV(obj interface{}) (syncError error) { return fmt.Errorf("casting ClusterServiceVersion failed") } - a.logger.WithField("name", fmt.Sprintf("%s/%s", clusterServiceVersion.Namespace, clusterServiceVersion.Name)).Info("syncCopyCSV") - olmConfig, err := a.client.OperatorsV1().OLMConfigs().Get(context.TODO(), "cluster", metav1.GetOptions{}) if err != nil && !apierrors.IsNotFound(err) { return err @@ -1932,24 +1929,7 @@ func (a *Operator) createCSVCopyingDisabledEvent(csv *v1alpha1.ClusterServiceVer return nil } -func GetCopiedNamespace(c *v1alpha1.ClusterServiceVersion) string { - annotations := c.GetAnnotations() - if annotations != nil { - operatorNamespace, ok := annotations[v1alpha1.OperatorGroupNamespaceAnnotationKey] - if ok && c.GetNamespace() != operatorNamespace { - return operatorNamespace - } - } - - if labels := c.GetLabels(); labels != nil { - if l, ok := labels[v1alpha1.CopiedLabelKey]; ok { - return l - } - } - return "" -} - -func (a *Operator) syncRevertGcCsv(obj interface{}) error { +func (a *Operator) syncResyncCsv(obj interface{}) error { csv, ok := obj.(*v1alpha1.ClusterServiceVersion) if !ok { a.logger.Debugf("wrong type: %#v", obj) @@ -1959,39 +1939,23 @@ func (a *Operator) syncRevertGcCsv(obj interface{}) error { return nil } - logger := a.logger.WithField("csv", fmt.Sprintf("%s/%s", csv.Namespace, csv.Name)) + name := csv.GetName() + copiedNamespace := csv.GetNamespace() + a.logger.WithField("csv", fmt.Sprintf("%s/%s", copiedNamespace, name)).Debug("syncing copied CSV") // check for any garbage collection - logger.Info("syncRevertGcCsv: removeDanglingChildCSVs") err := a.removeDanglingChildCSVs(csv) if err != nil { return err } - // Check for a revert - ns := GetCopiedNamespace(csv) - if ns == "" { - logger.Info("syncRevertGcCsv: Failed to get copied-from namespace") + // Requeue parent CSV to deal with any changes to the copied CSV + copiedFromNamespace, ok := csv.GetLabels()[v1alpha1.CopiedLabelKey] + if !ok { + a.logger.Infof("no %q label found in CSV, skipping requeue", v1alpha1.CopiedLabelKey) return nil } - prototype, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(ns).Get(context.TODO(), csv.Name, metav1.GetOptions{}) - if err != nil { - logger.Info("syncRevertGcCsv: Failed to get prototype") - return err - } - var copyPrototype v1alpha1.ClusterServiceVersion - csvCopyPrototype(prototype, ©Prototype) - specHash, statusHash, err := copyableCSVHash(©Prototype) - if err != nil { - logger.Info("syncRevertGcCsv: Failed to hash") - return err - } - logger.Info("syncRevertGcCsv: copyToNamespace") - _, err = a.copyToNamespace(©Prototype, ns, csv.Namespace, specHash, statusHash) - if err != nil { - logger.WithError(err).Info("syncRevertGcCsv: copyToNamespace failed") - } - return err + return a.csvCopyQueueSet.Requeue(copiedFromNamespace, name) } // operatorGroupFromAnnotations returns the OperatorGroup for the CSV only if the CSV is active one in the group diff --git a/pkg/controller/operators/olm/operatorgroup.go b/pkg/controller/operators/olm/operatorgroup.go index c9f6802df8..5bfd062d6f 100644 --- a/pkg/controller/operators/olm/operatorgroup.go +++ b/pkg/controller/operators/olm/operatorgroup.go @@ -812,11 +812,6 @@ 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) } - a.logger.WithFields(logrus.Fields{ - "nsFrom": nsFrom, - "nsTo": nsTo, - "csv": prototype.Name, - }).Info("copyToNamespace: created") return &v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ Name: created.Name, @@ -840,11 +835,6 @@ func (a *Operator) copyToNamespace(prototype *v1alpha1.ClusterServiceVersion, ns 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) } - a.logger.WithFields(logrus.Fields{ - "nsFrom": nsFrom, - "nsTo": nsTo, - "csv": updated.Name, - }).Info("copyToNamespace: updated Metadata+Spec") } else { // Avoid mutating cached copied CSV. updated = prototype @@ -855,11 +845,6 @@ 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) } - a.logger.WithFields(logrus.Fields{ - "nsFrom": nsFrom, - "nsTo": nsTo, - "csv": updated.Name, - }).Info("copyToNamespace: updated Status") } return &v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ From 7d53e449ccfe40a5fb48423bcd32990de5dd3fb1 Mon Sep 17 00:00:00 2001 From: Todd Short Date: Wed, 11 Jun 2025 16:51:02 -0400 Subject: [PATCH 23/26] Revert "Set resync to 6h" This reverts commit 855f940a2199bd4071c51f14ef44728550bf13cf. Signed-off-by: Todd Short --- cmd/catalog/main.go | 2 +- cmd/olm/main.go | 2 +- pkg/controller/operators/olm/config.go | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/cmd/catalog/main.go b/cmd/catalog/main.go index 40d71c15de..b82f1689cb 100644 --- a/cmd/catalog/main.go +++ b/cmd/catalog/main.go @@ -27,7 +27,7 @@ import ( const ( catalogNamespaceEnvVarName = "GLOBAL_CATALOG_NAMESPACE" - defaultWakeupInterval = 6 * time.Hour + defaultWakeupInterval = 15 * time.Minute defaultCatalogNamespace = "olm" defaultConfigMapServerImage = "quay.io/operator-framework/configmap-operator-registry:latest" defaultOPMImage = "quay.io/operator-framework/opm:latest" diff --git a/cmd/olm/main.go b/cmd/olm/main.go index 5e81424c33..715ae9aea0 100644 --- a/cmd/olm/main.go +++ b/cmd/olm/main.go @@ -33,7 +33,7 @@ import ( ) const ( - defaultWakeupInterval = 6 * time.Hour + defaultWakeupInterval = 5 * time.Minute defaultOperatorName = "" defaultPackageServerStatusName = "" ) diff --git a/pkg/controller/operators/olm/config.go b/pkg/controller/operators/olm/config.go index bc12683bd4..fa472c7130 100644 --- a/pkg/controller/operators/olm/config.go +++ b/pkg/controller/operators/olm/config.go @@ -101,7 +101,7 @@ func (o *operatorConfig) validate() (err error) { func defaultOperatorConfig() *operatorConfig { return &operatorConfig{ - resyncPeriod: queueinformer.ResyncWithJitter(6*time.Hour, 0.2), + resyncPeriod: queueinformer.ResyncWithJitter(30*time.Second, 0.2), operatorNamespace: "default", watchedNamespaces: []string{metav1.NamespaceAll}, clock: utilclock.RealClock{}, From baca995ef214984c1205d5ead77951e28b6315e5 Mon Sep 17 00:00:00 2001 From: Todd Short Date: Thu, 12 Jun 2025 09:59:23 -0400 Subject: [PATCH 24/26] Add delete handler for copied csv Signed-off-by: Todd Short --- pkg/controller/operators/olm/operator.go | 68 ++++++++++++------- pkg/controller/operators/olm/operatorgroup.go | 2 +- 2 files changed, 45 insertions(+), 25 deletions(-) diff --git a/pkg/controller/operators/olm/operator.go b/pkg/controller/operators/olm/operator.go index b2a88d1007..b83243a6fb 100644 --- a/pkg/controller/operators/olm/operator.go +++ b/pkg/controller/operators/olm/operator.go @@ -97,7 +97,7 @@ type Operator struct { csvQueueSet *queueinformer.ResourceQueueSet olmConfigQueue workqueue.TypedRateLimitingInterface[types.NamespacedName] csvCopyQueueSet *queueinformer.ResourceQueueSet - copiedCSVGCQueueSet *queueinformer.ResourceQueueSet + copiedCSVQueueSet *queueinformer.ResourceQueueSet nsQueueSet workqueue.TypedRateLimitingInterface[types.NamespacedName] apiServiceQueue workqueue.TypedRateLimitingInterface[types.NamespacedName] csvIndexers map[string]cache.Indexer @@ -216,8 +216,8 @@ func newOperatorWithConfig(ctx context.Context, config *operatorConfig) (*Operat Name: "olmConfig", }), - csvCopyQueueSet: queueinformer.NewEmptyResourceQueueSet(), - copiedCSVGCQueueSet: queueinformer.NewEmptyResourceQueueSet(), + csvCopyQueueSet: queueinformer.NewEmptyResourceQueueSet(), + copiedCSVQueueSet: queueinformer.NewEmptyResourceQueueSet(), apiServiceQueue: workqueue.NewTypedRateLimitingQueueWithConfig[types.NamespacedName]( workqueue.DefaultTypedControllerRateLimiter[types.NamespacedName](), workqueue.TypedRateLimitingQueueConfig[types.NamespacedName]{ @@ -371,25 +371,26 @@ func newOperatorWithConfig(ctx context.Context, config *operatorConfig) (*Operat informersByNamespace[namespace].CopiedCSVInformer = copiedCSVInformer informersByNamespace[namespace].CopiedCSVLister = op.copiedCSVLister - // Register separate queue for gcing copied csvs - copiedCSVGCQueue := workqueue.NewTypedRateLimitingQueueWithConfig[types.NamespacedName]( + // Register separate queue for gcing/syncing/deletion of copied csvs + copiedCSVQueue := workqueue.NewTypedRateLimitingQueueWithConfig[types.NamespacedName]( workqueue.DefaultTypedControllerRateLimiter[types.NamespacedName](), workqueue.TypedRateLimitingQueueConfig[types.NamespacedName]{ - Name: fmt.Sprintf("%s/csv-gc-revert", namespace), + Name: fmt.Sprintf("%s/csv-copied-sync", namespace), }) - op.copiedCSVGCQueueSet.Set(namespace, copiedCSVGCQueue) - copiedCSVGCQueueInformer, err := queueinformer.NewQueueInformer( + op.copiedCSVQueueSet.Set(namespace, copiedCSVQueue) + copiedCSVQueueInformer, err := queueinformer.NewQueueInformer( ctx, queueinformer.WithInformer(copiedCSVInformer), queueinformer.WithLogger(op.logger), - queueinformer.WithQueue(copiedCSVGCQueue), + queueinformer.WithQueue(copiedCSVQueue), queueinformer.WithIndexer(copiedCSVInformer.GetIndexer()), - queueinformer.WithSyncer(queueinformer.LegacySyncHandler(op.syncResyncCsv).ToSyncer()), + queueinformer.WithSyncer(queueinformer.LegacySyncHandler(op.syncCopiedCsv).ToSyncer()), + queueinformer.WithDeletionHandler(op.deleteCopiedCsv), ) if err != nil { return nil, err } - if err := op.RegisterQueueInformer(copiedCSVGCQueueInformer); err != nil { + if err := op.RegisterQueueInformer(copiedCSVQueueInformer); err != nil { return nil, err } @@ -1266,7 +1267,7 @@ func (a *Operator) handleClusterServiceVersionDeletion(obj interface{}) { for _, namespace := range namespaces { if namespace != operatorNamespace { logger.WithField("targetNamespace", namespace).Debug("requeueing child csv for deletion") - if err := a.copiedCSVGCQueueSet.Requeue(namespace, clusterServiceVersion.GetName()); err != nil { + if err := a.copiedCSVQueueSet.Requeue(namespace, clusterServiceVersion.GetName()); err != nil { logger.WithError(err).Warn("unable to requeue") } } @@ -1929,7 +1930,35 @@ func (a *Operator) createCSVCopyingDisabledEvent(csv *v1alpha1.ClusterServiceVer return nil } -func (a *Operator) syncResyncCsv(obj interface{}) error { +func (a *Operator) requeueParentCsv(csv *v1alpha1.ClusterServiceVersion) error { + name := csv.GetName() + copiedNamespace := csv.GetNamespace() + a.logger.WithField("csv", fmt.Sprintf("%s/%s", copiedNamespace, name)).Debug("syncing copied CSV") + + // Requeue parent CSV to deal with any changes to the copied CSV + copiedFromNamespace, ok := csv.GetLabels()[v1alpha1.CopiedLabelKey] + if !ok { + a.logger.Infof("no %q label found in CSV, skipping requeue", v1alpha1.CopiedLabelKey) + return nil + } + return a.csvCopyQueueSet.Requeue(copiedFromNamespace, name) +} + +func (a *Operator) deleteCopiedCsv(obj interface{}) { + csv, ok := obj.(*v1alpha1.ClusterServiceVersion) + if !ok { + a.logger.Debugf("casting ClusterServiceVersion failed: wrong type: %#v", obj) + return + } + if !v1alpha1.IsCopied(csv) { + return + } + + // Trigger partent reconciliation + a.requeueParentCsv(csv) +} + +func (a *Operator) syncCopiedCsv(obj interface{}) error { csv, ok := obj.(*v1alpha1.ClusterServiceVersion) if !ok { a.logger.Debugf("wrong type: %#v", obj) @@ -1939,23 +1968,14 @@ func (a *Operator) syncResyncCsv(obj interface{}) error { return nil } - name := csv.GetName() - copiedNamespace := csv.GetNamespace() - a.logger.WithField("csv", fmt.Sprintf("%s/%s", copiedNamespace, name)).Debug("syncing copied CSV") - // check for any garbage collection err := a.removeDanglingChildCSVs(csv) if err != nil { return err } - // Requeue parent CSV to deal with any changes to the copied CSV - copiedFromNamespace, ok := csv.GetLabels()[v1alpha1.CopiedLabelKey] - if !ok { - a.logger.Infof("no %q label found in CSV, skipping requeue", v1alpha1.CopiedLabelKey) - return nil - } - return a.csvCopyQueueSet.Requeue(copiedFromNamespace, name) + // Trigger partent reconciliation + return a.requeueParentCsv(csv) } // operatorGroupFromAnnotations returns the OperatorGroup for the CSV only if the CSV is active one in the group diff --git a/pkg/controller/operators/olm/operatorgroup.go b/pkg/controller/operators/olm/operatorgroup.go index 5bfd062d6f..ec3c307245 100644 --- a/pkg/controller/operators/olm/operatorgroup.go +++ b/pkg/controller/operators/olm/operatorgroup.go @@ -864,7 +864,7 @@ func (a *Operator) pruneFromNamespace(operatorGroupName, namespace string) error for _, csv := range fetchedCSVs { if v1alpha1.IsCopied(csv) && csv.GetAnnotations()[operatorsv1.OperatorGroupAnnotationKey] == operatorGroupName { a.logger.Debugf("Found CSV '%v' in namespace %v to delete", csv.GetName(), namespace) - if err := a.copiedCSVGCQueueSet.Requeue(csv.GetNamespace(), csv.GetName()); err != nil { + if err := a.copiedCSVQueueSet.Requeue(csv.GetNamespace(), csv.GetName()); err != nil { return err } } From 4c5d1be666ab521f560b71a410b7e2eaec907ca9 Mon Sep 17 00:00:00 2001 From: Todd Short Date: Thu, 12 Jun 2025 11:21:08 -0400 Subject: [PATCH 25/26] Revert whitespace change Signed-off-by: Todd Short --- pkg/controller/operators/olm/operatorgroup.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/controller/operators/olm/operatorgroup.go b/pkg/controller/operators/olm/operatorgroup.go index ec3c307245..739541ce9f 100644 --- a/pkg/controller/operators/olm/operatorgroup.go +++ b/pkg/controller/operators/olm/operatorgroup.go @@ -941,6 +941,7 @@ 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 90919ed2db32ead8159e2e2717fdc1bd0b710802 Mon Sep 17 00:00:00 2001 From: Todd Short Date: Wed, 18 Jun 2025 16:38:05 +0200 Subject: [PATCH 26/26] Rename function, fix comment Signed-off-by: Todd Short --- pkg/controller/operators/olm/operator.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/controller/operators/olm/operator.go b/pkg/controller/operators/olm/operator.go index b83243a6fb..9bb58dabe9 100644 --- a/pkg/controller/operators/olm/operator.go +++ b/pkg/controller/operators/olm/operator.go @@ -385,7 +385,7 @@ func newOperatorWithConfig(ctx context.Context, config *operatorConfig) (*Operat queueinformer.WithQueue(copiedCSVQueue), queueinformer.WithIndexer(copiedCSVInformer.GetIndexer()), queueinformer.WithSyncer(queueinformer.LegacySyncHandler(op.syncCopiedCsv).ToSyncer()), - queueinformer.WithDeletionHandler(op.deleteCopiedCsv), + queueinformer.WithDeletionHandler(op.handleCopiedCSVDeletion), ) if err != nil { return nil, err @@ -1944,7 +1944,7 @@ func (a *Operator) requeueParentCsv(csv *v1alpha1.ClusterServiceVersion) error { return a.csvCopyQueueSet.Requeue(copiedFromNamespace, name) } -func (a *Operator) deleteCopiedCsv(obj interface{}) { +func (a *Operator) handleCopiedCSVDeletion(obj interface{}) { csv, ok := obj.(*v1alpha1.ClusterServiceVersion) if !ok { a.logger.Debugf("casting ClusterServiceVersion failed: wrong type: %#v", obj) @@ -1954,7 +1954,7 @@ func (a *Operator) deleteCopiedCsv(obj interface{}) { return } - // Trigger partent reconciliation + // Trigger parent reconciliation a.requeueParentCsv(csv) } @@ -1974,7 +1974,7 @@ func (a *Operator) syncCopiedCsv(obj interface{}) error { return err } - // Trigger partent reconciliation + // Trigger parent reconciliation return a.requeueParentCsv(csv) }