From c5f79845e7b07e75f0815664bc09e64459f3c1a2 Mon Sep 17 00:00:00 2001 From: Vu Dinh Date: Thu, 20 Aug 2020 17:50:24 -0400 Subject: [PATCH 1/2] Add skips information to Operator representation Operator object contains only `replaces` but not `skips` information which leads to an incomplete picture of upgrade graph. Now, the `skips` information is added to ensure we take skipped versions into the account for upgrade graph calculation. Signed-off-by: Vu Dinh --- pkg/controller/registry/resolver/operators.go | 7 +++++++ pkg/controller/registry/resolver/resolver.go | 16 ++++++++++------ 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/pkg/controller/registry/resolver/operators.go b/pkg/controller/registry/resolver/operators.go index b477dabfae..c41b0777d7 100644 --- a/pkg/controller/registry/resolver/operators.go +++ b/pkg/controller/registry/resolver/operators.go @@ -218,6 +218,7 @@ type OperatorSurface interface { Inline() bool Dependencies() []*api.Dependency Properties() []*api.Property + Skips() []string } type Operator struct { @@ -230,6 +231,7 @@ type Operator struct { sourceInfo *OperatorSourceInfo dependencies []*api.Dependency properties []*api.Property + skips []string } var _ OperatorSurface = &Operator{} @@ -307,6 +309,7 @@ func NewOperatorFromBundle(bundle *api.Bundle, startingCSV string, sourceKey reg sourceInfo: sourceInfo, dependencies: dependencies, properties: properties, + skips: bundle.Skips, }, nil } @@ -372,6 +375,10 @@ func (o *Operator) Replaces() string { return o.replaces } +func (o *Operator) Skips() []string { + return o.skips +} + func (o *Operator) SetReplaces(replacing string) { o.replaces = replacing } diff --git a/pkg/controller/registry/resolver/resolver.go b/pkg/controller/registry/resolver/resolver.go index ef01b809c1..1912b2f3ed 100644 --- a/pkg/controller/registry/resolver/resolver.go +++ b/pkg/controller/registry/resolver/resolver.go @@ -522,7 +522,7 @@ func (r *SatResolver) sortChannel(bundles []*Operator) ([]*Operator, error) { bundleLookup := map[string]*Operator{} - // if a replacedBy b, then replacedBy[b] = a + // if a replaces b, then replacedBy[b] = a replacedBy := map[*Operator]*Operator{} replaces := map[*Operator]*Operator{} @@ -531,12 +531,16 @@ func (r *SatResolver) sortChannel(bundles []*Operator) ([]*Operator, error) { } for _, b := range bundles { - if b.replaces == "" { - continue + if b.replaces != "" { + if r, ok := bundleLookup[b.replaces]; ok { + replacedBy[r] = b + replaces[b] = r + } } - if r, ok := bundleLookup[b.replaces]; ok { - replacedBy[r] = b - replaces[b] = r + for _, skip := range b.skips { + if r, ok := bundleLookup[skip]; ok { + replacedBy[r] = b + } } } From 44ac7a7d70e3f5d724b3c5595916eccb3682696e Mon Sep 17 00:00:00 2001 From: Vu Dinh Date: Tue, 1 Sep 2020 02:56:34 -0400 Subject: [PATCH 2/2] Handle mutliple skips and add unit test Signed-off-by: Vu Dinh --- pkg/controller/registry/resolver/resolver.go | 11 ++- .../registry/resolver/resolver_test.go | 82 ++++++++++++++++--- 2 files changed, 80 insertions(+), 13 deletions(-) diff --git a/pkg/controller/registry/resolver/resolver.go b/pkg/controller/registry/resolver/resolver.go index 1912b2f3ed..ea55a21af6 100644 --- a/pkg/controller/registry/resolver/resolver.go +++ b/pkg/controller/registry/resolver/resolver.go @@ -525,6 +525,7 @@ func (r *SatResolver) sortChannel(bundles []*Operator) ([]*Operator, error) { // if a replaces b, then replacedBy[b] = a replacedBy := map[*Operator]*Operator{} replaces := map[*Operator]*Operator{} + skipped := map[string]*Operator{} for _, b := range bundles { bundleLookup[b.Identifier()] = b @@ -540,6 +541,7 @@ func (r *SatResolver) sortChannel(bundles []*Operator) ([]*Operator, error) { for _, skip := range b.skips { if r, ok := bundleLookup[skip]; ok { replacedBy[r] = b + skipped[skip] = r } } } @@ -559,12 +561,19 @@ func (r *SatResolver) sortChannel(bundles []*Operator) ([]*Operator, error) { head := headCandidates[0] current := head + skip := false for { - channel = append(channel, current) + if skip == false { + channel = append(channel, current) + } + skip = false next, ok := replaces[current] if !ok { break } + if _, ok := skipped[current.Identifier()]; ok { + skip = true + } current = next } diff --git a/pkg/controller/registry/resolver/resolver_test.go b/pkg/controller/registry/resolver/resolver_test.go index f60797d338..e7b428457f 100644 --- a/pkg/controller/registry/resolver/resolver_test.go +++ b/pkg/controller/registry/resolver/resolver_test.go @@ -335,7 +335,7 @@ func TestSolveOperators_CatsrcPrioritySorting(t *testing.T) { }, operators: []*Operator{ genOperator("packageA.v1", "0.0.1", "packageA.v1", "packageA", "alpha", "community", namespace, nil, - nil, opToAddVersionDeps, "",false), + nil, opToAddVersionDeps, "", false), }, }, registry.CatalogKey{ @@ -348,7 +348,7 @@ func TestSolveOperators_CatsrcPrioritySorting(t *testing.T) { }, operators: []*Operator{ genOperator("packageB.v1", "0.0.1", "", "packageB", "alpha", "community-operator", - namespace, nil, nil, nil, "",false), + namespace, nil, nil, nil, "", false), }, }, registry.CatalogKey{ @@ -362,7 +362,7 @@ func TestSolveOperators_CatsrcPrioritySorting(t *testing.T) { priority: catalogSourcePriority(100), operators: []*Operator{ genOperator("packageB.v1", "0.0.1", "", "packageB", "alpha", "high-priority-operator", - namespace, nil, nil, nil, "",false), + namespace, nil, nil, nil, "", false), }, }, }, @@ -377,9 +377,9 @@ func TestSolveOperators_CatsrcPrioritySorting(t *testing.T) { assert.NoError(t, err) expected := OperatorSet{ "packageA.v1": genOperator("packageA.v1", "0.0.1", "packageA.v1", "packageA", "alpha", "community", "olm", - nil, nil, opToAddVersionDeps, "",false), + nil, nil, opToAddVersionDeps, "", false), "packageB.v1": genOperator("packageB.v1", "0.0.1", "", "packageB", "alpha", "high-priority-operator", "olm", - nil, nil, nil, "",false), + nil, nil, nil, "", false), } assert.Equal(t, 2, len(operators)) for k, e := range expected { @@ -398,7 +398,7 @@ func TestSolveOperators_CatsrcPrioritySorting(t *testing.T) { priority: catalogSourcePriority(100), operators: []*Operator{ genOperator("packageB.v1", "0.0.1", "", "packageB", "alpha", "community-operator", - namespace, nil, nil, nil, "",false), + namespace, nil, nil, nil, "", false), }, } @@ -410,9 +410,9 @@ func TestSolveOperators_CatsrcPrioritySorting(t *testing.T) { assert.NoError(t, err) expected = OperatorSet{ "packageA.v1": genOperator("packageA.v1", "0.0.1", "packageA.v1", "packageA", "alpha", "community", "olm", - nil, nil, opToAddVersionDeps, "",false), + nil, nil, opToAddVersionDeps, "", false), "packageB.v1": genOperator("packageB.v1", "0.0.1", "", "packageB", "alpha", "community-operator", "olm", - nil, nil, nil, "",false), + nil, nil, nil, "", false), } assert.Equal(t, 2, len(operators)) for k, e := range expected { @@ -430,9 +430,9 @@ func TestSolveOperators_CatsrcPrioritySorting(t *testing.T) { }, operators: []*Operator{ genOperator("packageA.v1", "0.0.1", "packageA.v1", "packageA", "alpha", "community", namespace, nil, - nil, opToAddVersionDeps, "",false), + nil, opToAddVersionDeps, "", false), genOperator("packageB.v1", "0.0.1", "", "packageB", "alpha", "community", - namespace, nil, nil, nil, "",false), + namespace, nil, nil, nil, "", false), }, } @@ -444,9 +444,9 @@ func TestSolveOperators_CatsrcPrioritySorting(t *testing.T) { assert.NoError(t, err) expected = OperatorSet{ "packageA.v1": genOperator("packageA.v1", "0.0.1", "packageA.v1", "packageA", "alpha", "community", "olm", - nil, nil, opToAddVersionDeps, "",false), + nil, nil, opToAddVersionDeps, "", false), "packageB.v1": genOperator("packageB.v1", "0.0.1", "", "packageB", "alpha", "community", "olm", - nil, nil, nil, "",false), + nil, nil, nil, "", false), } assert.Equal(t, 2, len(operators)) for k, e := range expected { @@ -1343,3 +1343,61 @@ func TestSolveOperators_WithoutDeprecated(t *testing.T) { } require.EqualValues(t, expected, operators) } + +func TestSolveOperators_WithSkips(t *testing.T) { + APISet := APISet{opregistry.APIKey{"g", "v", "k", "ks"}: struct{}{}} + Provides := APISet + + namespace := "olm" + catalog := registry.CatalogKey{"community", namespace} + + newSub := newSub(namespace, "packageB", "alpha", catalog) + subs := []*v1alpha1.Subscription{newSub} + + opToAddVersionDeps := []*api.Dependency{ + { + Type: "olm.gvk", + Value: `{"group":"g","kind":"k","version":"v"}`, + }, + } + + opB := genOperator("packageB.v1", "1.0.0", "", "packageB", "alpha", "community", "olm", nil, nil, opToAddVersionDeps, "", false) + op1 := genOperator("packageA.v1", "1.0.0", "", "packageA", "alpha", "community", "olm", nil, Provides, nil, "", false) + op2 := genOperator("packageA.v2", "2.0.0", "packageA.v1", "packageA", "alpha", "community", "olm", nil, Provides, nil, "", false) + op3 := genOperator("packageA.v3", "3.0.0", "packageA.v2", "packageA", "alpha", "community", "olm", nil, Provides, nil, "", false) + op4 := genOperator("packageA.v4", "4.0.0", "packageA.v2", "packageA", "alpha", "community", "olm", nil, Provides, nil, "", false) + op4.skips = []string{"packageA.v3"} + op5 := genOperator("packageA.v5", "5.0.0", "packageA.v1", "packageA", "alpha", "community", "olm", nil, Provides, nil, "", false) + op5.skips = []string{"packageA.v2", "packageA.v3", "packageA.v4"} + op6 := genOperator("packageA.v6", "6.0.0", "packageA.v5", "packageA", "alpha", "community", "olm", nil, Provides, nil, "", false) + + fakeNamespacedOperatorCache := NamespacedOperatorCache{ + snapshots: map[registry.CatalogKey]*CatalogSnapshot{ + registry.CatalogKey{ + Namespace: "olm", + Name: "community", + }: { + key: registry.CatalogKey{ + Namespace: "olm", + Name: "community", + }, + operators: []*Operator{ + opB, op1, op2, op3, op4, op5, op6, + }, + }, + }, + } + satResolver := SatResolver{ + cache: getFakeOperatorCache(fakeNamespacedOperatorCache), + log: logrus.New(), + } + + operators, err := satResolver.SolveOperators([]string{"olm"}, nil, subs) + assert.NoError(t, err) + + expected := OperatorSet{ + "packageB.v1": genOperator("packageB.v1", "1.0.0", "", "packageB", "alpha", "community", "olm", nil, nil, opToAddVersionDeps, "", false), + "packageA.v6": genOperator("packageA.v6", "6.0.0", "packageA.v5", "packageA", "alpha", "community", "olm", nil, Provides, nil, "", false), + } + require.EqualValues(t, expected, operators) +}