Remove oudated subscription update logic to improve resolution delay (#2369) (#2856)

Currently, olm logic checks for upgrade in subscription via another
obsolete API that is no longer in use for dependency solution. As a
result, sometimes, subscriptions display `UpgradeAvailable` status but
there will be no upgrades as the upgrade is not valid in the resolver.
Also, the `UpgradeAvailable` status is used to trigger the new resolution
even though that status is no longer a valid indicator of having a pending
upgrade. This leads to unwanted upgrade delay when the obsolete API works
properly.

This commit will remove the code that is using this obsolete API and
allow the resolution to happen when there is a subscription change.

Signed-off-by: Vu Dinh <[email protected]>
Signed-off-by: perdasilva <[email protected]>

Signed-off-by: Vu Dinh <[email protected]>
Signed-off-by: perdasilva <[email protected]>
Co-authored-by: Vu Dinh <[email protected]>

Author: perdasilva

pkg/controller/operators/catalog/operator.go

@@ -951,11 +951,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
@@ -1009,7 +1004,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")
@@ -1019,14 +1014,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
 	}
 

pkg/controller/registry/resolver/querier.go

@@ -31,6 +31,7 @@ type SourceQuerier interface {
 	FindProvider(api opregistry.APIKey, initialSource registry.CatalogKey, excludedPackages map[string]struct{}) (*api.Bundle, *registry.CatalogKey, error)
 	FindBundle(pkgName, channelName, bundleName string, initialSource registry.CatalogKey) (*api.Bundle, *registry.CatalogKey, error)
 	FindLatestBundle(pkgName, channelName string, initialSource registry.CatalogKey) (*api.Bundle, *registry.CatalogKey, error)
+	// 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
 }
@@ -123,6 +124,7 @@ func (q *NamespaceSourceQuerier) FindLatestBundle(pkgName, channelName string, i
 	return nil, nil, fmt.Errorf("%s/%s not found in any available CatalogSource", pkgName, channelName)
 }
 
+// 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{}
 

pkg/controller/registry/resolver/querier_test.go

@@ -2,19 +2,14 @@ package resolver
 
 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"
 	opregistry "github.com/operator-framework/operator-registry/pkg/registry"
 	"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/registry"
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/controller/registry/resolver/fakes"
 )
@@ -344,178 +339,3 @@ func TestNamespaceSourceQuerier_FindPackage(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)
-		})
-	}
-}

test/e2e/subscription_e2e_test.go

@@ -2044,7 +2044,7 @@ var _ = Describe("Subscription", func() {
 		}
 		updateInternalCatalog(GinkgoT(), kubeClient, crClient, catalogSourceName, testNamespace, []apiextensions.CustomResourceDefinition{crd, crd2}, []v1alpha1.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{})