Merge pull request #211 from dinhxuanvu/upgrade-delay

Bug 2002276: Remove oudated subscription update logic to improve resolution delay

Author: openshift-merge-robot

staging/operator-lifecycle-manager/pkg/controller/operators/catalog/operator.go

@@ -1044,11 +1044,6 @@ func (o *Operator) syncSubscriptions(obj interface{}) error {
 }
 
 func (o *Operator) nothingToUpdate(logger *logrus.Entry, sub *v1alpha1.Subscription) bool {
-	// Only sync if catalog has been updated since last sync time
-	if o.sourcesLastUpdate.Before(sub.Status.LastUpdated.Time) && sub.Status.State != v1alpha1.SubscriptionStateNone && sub.Status.State != v1alpha1.SubscriptionStateUpgradeAvailable {
-		logger.Debugf("skipping update: no new updates to catalog since last sync at %s", sub.Status.LastUpdated.String())
-		return true
-	}
 	if sub.Status.InstallPlanRef != nil && sub.Status.State == v1alpha1.SubscriptionStateUpgradePending {
 		logger.Debugf("skipping update: installplan already created")
 		return true
@@ -1102,7 +1097,7 @@ func (o *Operator) ensureSubscriptionCSVState(logger *logrus.Entry, sub *v1alpha
 		return sub, false, nil
 	}
 
-	csv, err := o.client.OperatorsV1alpha1().ClusterServiceVersions(sub.GetNamespace()).Get(context.TODO(), sub.Status.CurrentCSV, metav1.GetOptions{})
+	_, err := o.client.OperatorsV1alpha1().ClusterServiceVersions(sub.GetNamespace()).Get(context.TODO(), sub.Status.CurrentCSV, metav1.GetOptions{})
 	out := sub.DeepCopy()
 	if err != nil {
 		logger.WithError(err).WithField("currentCSV", sub.Status.CurrentCSV).Debug("error fetching csv listed in subscription status")
@@ -1112,14 +1107,7 @@ func (o *Operator) ensureSubscriptionCSVState(logger *logrus.Entry, sub *v1alpha
 		if err := querier.Queryable(); err != nil {
 			return nil, false, err
 		}
-		b, _, _ := querier.FindReplacement(&csv.Spec.Version.Version, sub.Status.CurrentCSV, sub.Spec.Package, sub.Spec.Channel, registry.CatalogKey{Name: sub.Spec.CatalogSource, Namespace: sub.Spec.CatalogSourceNamespace})
-		if b != nil {
-			o.logger.Tracef("replacement %s bundle found for current bundle %s", b.CsvName, sub.Status.CurrentCSV)
-			out.Status.State = v1alpha1.SubscriptionStateUpgradeAvailable
-		} else {
-			out.Status.State = v1alpha1.SubscriptionStateAtLatest
-		}
-
+		out.Status.State = v1alpha1.SubscriptionStateAtLatest
 		out.Status.InstalledCSV = sub.Status.CurrentCSV
 	}
 

staging/operator-lifecycle-manager/pkg/controller/operators/catalog/querier.go

@@ -26,6 +26,7 @@ type SourceRef struct {
 }
 
 type SourceQuerier interface {
+	// Deprecated: This FindReplacement function will be deprecated soon
 	FindReplacement(currentVersion *semver.Version, bundleName, pkgName, channelName string, initialSource registry.CatalogKey) (*api.Bundle, *registry.CatalogKey, error)
 	Queryable() error
 }
@@ -49,6 +50,7 @@ func (q *NamespaceSourceQuerier) Queryable() error {
 	return nil
 }
 
+// Deprecated: This FindReplacement function will be deprecated soon
 func (q *NamespaceSourceQuerier) FindReplacement(currentVersion *semver.Version, bundleName, pkgName, channelName string, initialSource registry.CatalogKey) (*api.Bundle, *registry.CatalogKey, error) {
 	errs := []error{}
 

staging/operator-lifecycle-manager/pkg/controller/operators/catalog/querier_test.go

@@ -1,19 +1,12 @@
 package catalog
 
 import (
-	"context"
-	"encoding/json"
 	"fmt"
 	"testing"
 
-	"github.com/blang/semver/v4"
-	"github.com/operator-framework/operator-registry/pkg/api"
 	"github.com/operator-framework/operator-registry/pkg/client"
 	"github.com/stretchr/testify/require"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
-	"github.com/operator-framework/api/pkg/lib/version"
-	"github.com/operator-framework/api/pkg/operators/v1alpha1"
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/catalog/fakes"
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/controller/registry"
 )
@@ -112,178 +105,3 @@ func TestNamespaceSourceQuerier_Queryable(t *testing.T) {
 		})
 	}
 }
-
-func TestNamespaceSourceQuerier_FindReplacement(t *testing.T) {
-	// TODO: clean up this test setup
-	initialSource := fakes.FakeClientInterface{}
-	otherSource := fakes.FakeClientInterface{}
-	replacementSource := fakes.FakeClientInterface{}
-	replacementAndLatestSource := fakes.FakeClientInterface{}
-	replacementAndNoAnnotationLatestSource := fakes.FakeClientInterface{}
-
-	latestVersion := semver.MustParse("1.0.0-1556661308")
-	csv := v1alpha1.ClusterServiceVersion{
-		TypeMeta: metav1.TypeMeta{
-			Kind:       v1alpha1.ClusterServiceVersionKind,
-			APIVersion: v1alpha1.GroupVersion,
-		},
-		ObjectMeta: metav1.ObjectMeta{
-			Name:      "latest",
-			Namespace: "placeholder",
-			Annotations: map[string]string{
-				"olm.skipRange": ">= 1.0.0-0 < 1.0.0-1556661308",
-			},
-		},
-		Spec: v1alpha1.ClusterServiceVersionSpec{
-			CustomResourceDefinitions: v1alpha1.CustomResourceDefinitions{
-				Owned:    []v1alpha1.CRDDescription{},
-				Required: []v1alpha1.CRDDescription{},
-			},
-			APIServiceDefinitions: v1alpha1.APIServiceDefinitions{
-				Owned:    []v1alpha1.APIServiceDescription{},
-				Required: []v1alpha1.APIServiceDescription{},
-			},
-			Version: version.OperatorVersion{latestVersion},
-		},
-	}
-	csvJson, err := json.Marshal(csv)
-	require.NoError(t, err)
-
-	nextBundle := &api.Bundle{CsvName: "test.v1", PackageName: "testPkg", ChannelName: "testChannel"}
-	latestBundle := &api.Bundle{CsvName: "latest", PackageName: "testPkg", ChannelName: "testChannel", CsvJson: string(csvJson), Object: []string{string(csvJson)}, SkipRange: ">= 1.0.0-0 < 1.0.0-1556661308", Version: latestVersion.String()}
-
-	csv.SetAnnotations(map[string]string{})
-	csvUnstNoAnnotationJson, err := json.Marshal(csv)
-	require.NoError(t, err)
-	latestBundleNoAnnotation := &api.Bundle{CsvName: "latest", PackageName: "testPkg", ChannelName: "testChannel", CsvJson: string(csvUnstNoAnnotationJson), Object: []string{string(csvUnstNoAnnotationJson)}}
-
-	initialSource.GetReplacementBundleInPackageChannelStub = func(ctx context.Context, bundleName, pkgName, channelName string) (*api.Bundle, error) {
-		return nil, fmt.Errorf("not found")
-	}
-	replacementSource.GetReplacementBundleInPackageChannelStub = func(ctx context.Context, bundleName, pkgName, channelName string) (*api.Bundle, error) {
-		return nextBundle, nil
-	}
-	initialSource.GetBundleInPackageChannelStub = func(ctx context.Context, pkgName, channelName string) (*api.Bundle, error) {
-		if pkgName != latestBundle.PackageName {
-			return nil, fmt.Errorf("not found")
-		}
-		return latestBundle, nil
-	}
-	otherSource.GetBundleInPackageChannelStub = func(ctx context.Context, pkgName, channelName string) (*api.Bundle, error) {
-		if pkgName != latestBundle.PackageName {
-			return nil, fmt.Errorf("not found")
-		}
-		return latestBundle, nil
-	}
-	replacementAndLatestSource.GetReplacementBundleInPackageChannelStub = func(ctx context.Context, bundleName, pkgName, channelName string) (*api.Bundle, error) {
-		return nextBundle, nil
-	}
-	replacementAndLatestSource.GetBundleInPackageChannelStub = func(ctx context.Context, pkgName, channelName string) (*api.Bundle, error) {
-		return latestBundle, nil
-	}
-	replacementAndNoAnnotationLatestSource.GetReplacementBundleInPackageChannelStub = func(ctx context.Context, bundleName, pkgName, channelName string) (*api.Bundle, error) {
-		return nextBundle, nil
-	}
-	replacementAndNoAnnotationLatestSource.GetBundleInPackageChannelStub = func(ctx context.Context, pkgName, channelName string) (*api.Bundle, error) {
-		return latestBundleNoAnnotation, nil
-	}
-
-	initialKey := registry.CatalogKey{"initial", "ns"}
-	otherKey := registry.CatalogKey{"other", "other"}
-	replacementKey := registry.CatalogKey{"replacement", "ns"}
-	replacementAndLatestKey := registry.CatalogKey{"replat", "ns"}
-	replacementAndNoAnnotationLatestKey := registry.CatalogKey{"replatbad", "ns"}
-
-	sources := map[registry.CatalogKey]registry.ClientInterface{
-		initialKey:                          &initialSource,
-		otherKey:                            &otherSource,
-		replacementKey:                      &replacementSource,
-		replacementAndLatestKey:             &replacementAndLatestSource,
-		replacementAndNoAnnotationLatestKey: &replacementAndNoAnnotationLatestSource,
-	}
-
-	startVersion := semver.MustParse("1.0.0-0")
-	notInRange := semver.MustParse("1.0.0-1556661347")
-
-	type fields struct {
-		sources map[registry.CatalogKey]registry.ClientInterface
-	}
-	type args struct {
-		currentVersion *semver.Version
-		pkgName        string
-		channelName    string
-		bundleName     string
-		initialSource  registry.CatalogKey
-	}
-	type out struct {
-		bundle *api.Bundle
-		key    *registry.CatalogKey
-		err    error
-	}
-	tests := []struct {
-		name   string
-		fields fields
-		args   args
-		out    out
-	}{
-		{
-			name:   "FindsLatestInPrimaryCatalog",
-			fields: fields{sources: sources},
-			args:   args{&startVersion, "testPkg", "testChannel", "test.v1", initialKey},
-			out:    out{bundle: latestBundle, key: &initialKey, err: nil},
-		},
-		{
-			name:   "FindsLatestInSecondaryCatalog",
-			fields: fields{sources: sources},
-			args:   args{&startVersion, "testPkg", "testChannel", "test.v1", otherKey},
-			out:    out{bundle: latestBundle, key: &otherKey, err: nil},
-		},
-		{
-			name:   "PrefersLatestToReplaced/SameCatalog",
-			fields: fields{sources: sources},
-			args:   args{&startVersion, "testPkg", "testChannel", "test.v1", replacementAndLatestKey},
-			out:    out{bundle: latestBundle, key: &replacementAndLatestKey, err: nil},
-		},
-		{
-			name:   "PrefersLatestToReplaced/OtherCatalog",
-			fields: fields{sources: sources},
-			args:   args{&startVersion, "testPkg", "testChannel", "test.v1", initialKey},
-			out:    out{bundle: latestBundle, key: &initialKey, err: nil},
-		},
-		{
-			name:   "IgnoresLatestWithoutAnnotation",
-			fields: fields{sources: sources},
-			args:   args{&startVersion, "testPkg", "testChannel", "test.v1", replacementAndNoAnnotationLatestKey},
-			out:    out{bundle: nextBundle, key: &replacementAndNoAnnotationLatestKey, err: nil},
-		},
-		{
-			name:   "IgnoresLatestNotInRange",
-			fields: fields{sources: sources},
-			args:   args{&notInRange, "testPkg", "testChannel", "test.v1", replacementAndLatestKey},
-			out:    out{bundle: nextBundle, key: &replacementAndLatestKey, err: nil},
-		},
-		{
-			name:   "IgnoresLatestAtLatest",
-			fields: fields{sources: sources},
-			args:   args{&latestVersion, "testPkg", "testChannel", "test.v1", otherKey},
-			out:    out{bundle: nil, key: nil, err: nil},
-		},
-	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			q := &NamespaceSourceQuerier{
-				sources: tt.fields.sources,
-			}
-			var got *api.Bundle
-			var key *registry.CatalogKey
-			var err error
-			got, key, err = q.FindReplacement(tt.args.currentVersion, tt.args.bundleName, tt.args.pkgName, tt.args.channelName, tt.args.initialSource)
-			if err != nil {
-				t.Log(err.Error())
-			}
-			require.Equal(t, tt.out.err, err, "%v", err)
-			require.Equal(t, tt.out.bundle, got)
-			require.Equal(t, tt.out.key, key)
-		})
-	}
-}

staging/operator-lifecycle-manager/pkg/controller/registry/resolver/cache/cache.go

@@ -136,9 +136,9 @@ type NamespacedOperatorCache struct {
 func (c *NamespacedOperatorCache) Error() error {
 	var errs []error
 	for key, snapshot := range c.snapshots {
-		snapshot.m.Lock()
+		snapshot.m.RLock()
 		err := snapshot.err
-		snapshot.m.Unlock()
+		snapshot.m.RUnlock()
 		if err != nil {
 			errs = append(errs, fmt.Errorf("error using catalog %s (in namespace %s): %w", key.Name, key.Namespace, err))
 		}

staging/operator-lifecycle-manager/test/e2e/subscription_e2e_test.go

@@ -2066,7 +2066,7 @@ var _ = Describe("Subscription", func() {
 		}
 		updateInternalCatalog(GinkgoT(), kubeClient, crClient, catalogSourceName, testNamespace, []apiextensions.CustomResourceDefinition{crd, crd2}, []operatorsv1alpha1.ClusterServiceVersion{csvNewA, csvA, csvB}, manifests)
 		csvAsub := strings.Join([]string{packageName1, stableChannel, catalogSourceName, testNamespace}, "-")
-		_, err = fetchSubscription(crClient, testNamespace, csvAsub, subscriptionStateUpgradeAvailableChecker)
+		_, err = fetchSubscription(crClient, testNamespace, csvAsub, subscriptionStateAtLatestChecker)
 		require.NoError(GinkgoT(), err)
 		// Ensure csvNewA is not installed
 		_, err = crClient.OperatorsV1alpha1().ClusterServiceVersions(testNamespace).Get(context.Background(), csvNewA.Name, metav1.GetOptions{})