Skip to content

[release-4.5] Bug 1862407: fix(resolver): be smarter about the way downgrades are attempted #1689

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 70 additions & 0 deletions pkg/controller/registry/resolver/evolver.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ type Evolver interface {
type NamespaceGenerationEvolver struct {
querier SourceQuerier
gen Generation
replacements map[OperatorSurface]OperatorSurface
}

func NewNamespaceGenerationEvolver(querier SourceQuerier, gen Generation) Evolver {
Expand Down Expand Up @@ -44,13 +45,20 @@ func (e *NamespaceGenerationEvolver) Evolve(add map[OperatorSourceInfo]struct{})
return err
}

// if apis are missing, attempt to contract back to a good set by trying all combinations of rollbacks to updates
if err := e.downgradeUpdates(); err != nil {
return err
}

// for any remaining missing APIs, attempt to downgrade the operator that required them
// this may contract the generation back to the original set!
e.downgradeAPIs()
return nil
}

func (e *NamespaceGenerationEvolver) checkForUpdates() error {
e.replacements = make(map[OperatorSurface]OperatorSurface)

// maps the old operator identifier to the new operator
updates := EmptyOperatorSet()

Expand All @@ -74,6 +82,7 @@ func (e *NamespaceGenerationEvolver) checkForUpdates() error {
}
o.SetReplaces(op.Identifier())
updates[op.Identifier()] = o
e.replacements[op] = o
}

// remove any operators we found updates for
Expand Down Expand Up @@ -153,7 +162,68 @@ func (e *NamespaceGenerationEvolver) queryForRequiredAPIs() error {
return nil
}

func (e *NamespaceGenerationEvolver) downgradeUpdates() error {
// no need to attempt downgrades
if len(e.gen.MissingAPIs()) == 0 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super nit: the caller for this function might be more obvious if this check happened outside the method itself.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is just keeping in line with the rest of the object, which has a series of methods which may or may not affect the generation - adding the check in Evolve would make it read weirdly, IMO

return nil
}

// smart downgrades are only supported if fewer than 64 updates are resolved at the same time
// (this should be all but pathological cases)
if len(e.replacements) > 64 {
return nil
}

old := make([]OperatorSurface, 0)
new := make([]OperatorSurface, 0)
for o, n := range e.replacements {
old = append(old, o)
new = append(new, n)
}
flagToIndex := make(map[uint64]int)
flags := make([]uint64, 0)
var max uint64
for i := 0; i < len(e.replacements); i++ {
var f uint64 = 1 << i
flags = append(flags, f)
max += f
flagToIndex[f] = i
}

var i uint64
var g Generation
for i = 0; i <= max; i++ {
g = NewEmptyGeneration()
for _, f := range flags {
idx := flagToIndex[f]
// if toggled, pick old
if f&i != 0 {
_ = g.AddOperator(old[idx])
} else {
_ = g.AddOperator(new[idx])
}
}
// we found a good set, update the real generation and quit
if len(g.MissingAPIs()) == 0 {
for _, f := range flags {
idx := flagToIndex[f]
// if toggled, remove new and add old
if f&i != 0 {
e.gen.RemoveOperator(new[idx])
if err := e.gen.AddOperator(old[idx]); err != nil {
return err
}
}
}
return nil
}
}

return nil
}

func (e *NamespaceGenerationEvolver) downgradeAPIs() {
// remove anything we can't satisfy after trying to downgrade all possible combinations
e.gen.ResetUnchecked()
for missingAPIs := e.gen.MissingAPIs(); len(missingAPIs) > 0; {
requirers := missingAPIs.PopAPIRequirers()
Expand Down
22 changes: 22 additions & 0 deletions pkg/controller/registry/resolver/evolver_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -451,6 +451,28 @@ func TestNamespaceGenerationEvolver(t *testing.T) {
NewFakeOperatorSurface("depender.v1", "depender", "channel", "", "catsrc", "", nil,[]opregistry.APIKey{{"g", "v", "k", "ks"}}, nil, nil),
),
},
{
name: "NoProviderInUpdateSet",
fields: fields{
querier: NewFakeSourceQuerier(map[CatalogKey][]*api.Bundle{
CatalogKey{"catsrc", "catsrc-namespace"}: {
bundle("original", "o", "c", "", APISet{opregistry.APIKey{"g", "v", "k", "ks"}: {}}, EmptyAPISet(), EmptyAPISet(), EmptyAPISet()),
bundle("original.v2", "o", "c", "original", EmptyAPISet(), EmptyAPISet(), EmptyAPISet(), EmptyAPISet()),
bundle("depender", "depender", "channel", "", EmptyAPISet(), APISet{opregistry.APIKey{"g", "v", "k", "ks"}: {}}, EmptyAPISet(), EmptyAPISet()),
bundle("depender.v2", "depender", "channel", "depender", EmptyAPISet(), APISet{opregistry.APIKey{"g", "v", "k", "ks"}: {}}, EmptyAPISet(), EmptyAPISet()),
},
}),
gen: NewGenerationFromOperators(
NewFakeOperatorSurface("original", "o", "c", "", "catsrc", "", []opregistry.APIKey{{"g", "v", "k", "ks"}}, nil, nil, nil),
NewFakeOperatorSurface("depender", "depender", "channel", "", "catsrc", "", nil, []opregistry.APIKey{{"g", "v", "k", "ks"}}, nil, nil),
),
},
args: args{},
wantGen: NewGenerationFromOperators(
NewFakeOperatorSurface("original", "o", "c", "", "catsrc", "", []opregistry.APIKey{{"g", "v", "k", "ks"}},nil, nil, nil),
NewFakeOperatorSurface("depender.v2", "depender", "channel", "depender", "catsrc", "", nil, []opregistry.APIKey{{"g", "v", "k", "ks"}}, nil, nil),
),
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
Expand Down