Skip to content

Commit

Permalink
Remove oudated subscription update logic to improve resolution delay (#…
Browse files Browse the repository at this point in the history 
…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 <vudinh@outlook.com>
  • Loading branch information
@dinhxuanvu
dinhxuanvu committed Sep 29, 2021
1 parent afc0848 commit 81e7a60
Show file tree
Hide file tree
Showing 4 changed files with 5 additions and 197 deletions.
16 changes: 2 additions & 14 deletions pkg/controller/operators/catalog/operator.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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")
Expand All @@ -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
}

Expand Down
2 changes: 2 additions & 0 deletions pkg/controller/operators/catalog/querier.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -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{}

Expand Down
182 changes: 0 additions & 182 deletions pkg/controller/operators/catalog/querier_test.go
View file
Original file line number Diff line number Diff line change
@@ -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"
)
Expand Down Expand Up @@ -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{&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)
})
}
}
2 changes: 1 addition & 1 deletion test/e2e/subscription_e2e_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{})
Expand Down

0 comments on commit 81e7a60

Please sign in to comment.