From 27ef35323b2ab5b4a881fe56189d6c6462e2f80e Mon Sep 17 00:00:00 2001 From: Vu Dinh Date: Tue, 21 Sep 2021 16:56:59 -0400 Subject: [PATCH] Remove oudated subscription update logic to improve resolution delay 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 +- pkg/controller/operators/catalog/querier.go | 2 + .../operators/catalog/querier_test.go | 182 ------------------ test/e2e/subscription_e2e_test.go | 2 +- 4 files changed, 5 insertions(+), 197 deletions(-) diff --git a/pkg/controller/operators/catalog/operator.go b/pkg/controller/operators/catalog/operator.go index e83e77c282..a66712d878 100644 --- a/pkg/controller/operators/catalog/operator.go +++ b/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 } diff --git a/pkg/controller/operators/catalog/querier.go b/pkg/controller/operators/catalog/querier.go index 1faf2c05cc..599210e257 100644 --- a/pkg/controller/operators/catalog/querier.go +++ b/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{} diff --git a/pkg/controller/operators/catalog/querier_test.go b/pkg/controller/operators/catalog/querier_test.go index 6a220a854d..022abeb4f3 100644 --- a/pkg/controller/operators/catalog/querier_test.go +++ b/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{¬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 a8ab2b676a..c679a27688 100644 --- a/test/e2e/subscription_e2e_test.go +++ b/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{})