From f70bd92b82f758c8b846e654a6eae0030ff99be8 Mon Sep 17 00:00:00 2001 From: Vu Dinh Date: Wed, 29 Sep 2021 14:51:06 -0400 Subject: [PATCH 1/2] 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 Upstream-repository: operator-lifecycle-manager Upstream-commit: 81e7a60bc7a62da4a469041ce89e3867e9f47fde --- .../controller/operators/catalog/operator.go | 16 +- .../controller/operators/catalog/querier.go | 2 + .../operators/catalog/querier_test.go | 182 ------------------ .../test/e2e/subscription_e2e_test.go | 2 +- .../controller/operators/catalog/operator.go | 16 +- .../controller/operators/catalog/querier.go | 2 + 6 files changed, 9 insertions(+), 211 deletions(-) diff --git a/staging/operator-lifecycle-manager/pkg/controller/operators/catalog/operator.go b/staging/operator-lifecycle-manager/pkg/controller/operators/catalog/operator.go index e83e77c282..a66712d878 100644 --- a/staging/operator-lifecycle-manager/pkg/controller/operators/catalog/operator.go +++ b/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 } diff --git a/staging/operator-lifecycle-manager/pkg/controller/operators/catalog/querier.go b/staging/operator-lifecycle-manager/pkg/controller/operators/catalog/querier.go index 1faf2c05cc..599210e257 100644 --- a/staging/operator-lifecycle-manager/pkg/controller/operators/catalog/querier.go +++ b/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{} diff --git a/staging/operator-lifecycle-manager/pkg/controller/operators/catalog/querier_test.go b/staging/operator-lifecycle-manager/pkg/controller/operators/catalog/querier_test.go index 6a220a854d..022abeb4f3 100644 --- a/staging/operator-lifecycle-manager/pkg/controller/operators/catalog/querier_test.go +++ b/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{¬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/staging/operator-lifecycle-manager/test/e2e/subscription_e2e_test.go b/staging/operator-lifecycle-manager/test/e2e/subscription_e2e_test.go index a8ab2b676a..c679a27688 100644 --- a/staging/operator-lifecycle-manager/test/e2e/subscription_e2e_test.go +++ b/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{}) diff --git a/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/catalog/operator.go b/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/catalog/operator.go index e83e77c282..a66712d878 100644 --- a/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/catalog/operator.go +++ b/vendor/github.com/operator-framework/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 } diff --git a/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/catalog/querier.go b/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/catalog/querier.go index 1faf2c05cc..599210e257 100644 --- a/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/catalog/querier.go +++ b/vendor/github.com/operator-framework/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{} From 807c857a6f95fd9c93302bcdbaaf040dc7ceeb51 Mon Sep 17 00:00:00 2001 From: Tim Flannagan Date: Thu, 30 Sep 2021 17:15:28 -0400 Subject: [PATCH 2/2] pkg/controller: Migrate to using a read lock instead of a readwrite lock when checking for any namespace caching errors (#2382) Signed-off-by: timflannagan Upstream-repository: operator-lifecycle-manager Upstream-commit: 396b5461555c2498507b1fff20fccc3e7ae15420 --- .../pkg/controller/registry/resolver/cache/cache.go | 4 ++-- .../pkg/controller/registry/resolver/cache/cache.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/staging/operator-lifecycle-manager/pkg/controller/registry/resolver/cache/cache.go b/staging/operator-lifecycle-manager/pkg/controller/registry/resolver/cache/cache.go index 10ca6d07d8..8a42dd2540 100644 --- a/staging/operator-lifecycle-manager/pkg/controller/registry/resolver/cache/cache.go +++ b/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)) } diff --git a/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/controller/registry/resolver/cache/cache.go b/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/controller/registry/resolver/cache/cache.go index 10ca6d07d8..8a42dd2540 100644 --- a/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/controller/registry/resolver/cache/cache.go +++ b/vendor/github.com/operator-framework/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)) }