From 0a5cb844c6b2b4fd6db993f0d45b1029d9b32067 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 --- pkg/controller/operators/catalog/operator.go | 16 +- .../registry/resolver/querier_test.go | 180 ------------------ test/e2e/subscription_e2e_test.go | 2 +- 3 files changed, 3 insertions(+), 195 deletions(-) diff --git a/pkg/controller/operators/catalog/operator.go b/pkg/controller/operators/catalog/operator.go index 0041f57210b..e17cb31e5a8 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_test.go b/pkg/controller/registry/resolver/querier_test.go index 20d1c0e7001..53b31e29a1d 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 2124e3a915f..ac9c74cf5dc 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{})