From e7cddf764be043f98d160de264d17938ea258b7d Mon Sep 17 00:00:00 2001 From: Vu Dinh Date: Wed, 29 Sep 2021 14:51:06 -0400 Subject: [PATCH] Remove oudated subscription update logic to improve resolution delay (#2369) 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 Signed-off-by: perdasilva --- pkg/controller/operators/catalog/operator.go | 16 +- pkg/controller/registry/resolver/querier.go | 2 + .../registry/resolver/querier_test.go | 180 ------------------ test/e2e/subscription_e2e_test.go | 2 +- 4 files changed, 5 insertions(+), 195 deletions(-) diff --git a/pkg/controller/operators/catalog/operator.go b/pkg/controller/operators/catalog/operator.go index 0041f57210..e17cb31e5a 100644 --- a/pkg/controller/operators/catalog/operator.go +++ b/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 } diff --git a/pkg/controller/registry/resolver/querier.go b/pkg/controller/registry/resolver/querier.go index 7acbc8a8a3..121c2563f4 100644 --- a/pkg/controller/registry/resolver/querier.go +++ b/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{} diff --git a/pkg/controller/registry/resolver/querier_test.go b/pkg/controller/registry/resolver/querier_test.go index 20d1c0e700..53b31e29a1 100644 --- a/pkg/controller/registry/resolver/querier_test.go +++ b/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{¬InRange, "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) - }) - } -} diff --git a/test/e2e/subscription_e2e_test.go b/test/e2e/subscription_e2e_test.go index 2124e3a915..ac9c74cf5d 100644 --- a/test/e2e/subscription_e2e_test.go +++ b/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{})