Skip to content

Commit 7dea9bb

Browse files
ecordellawgreene
ecordell
authored and
awgreene
committed
fix(resolver): Transferring API Ownership
Problem: In an upgrade where v1 of Operator A provides an API and v1 of Operator B depends on the API, where the ownership of the API is transferred from Operator A to Operator B during the upgrade, OLM may run into a situation where Operator B is updated first. This causes OLM to fail to calculate the generation because multiple operator provide the same API. Solution: Add and remove updates as a set to prevent the situation described above.
1 parent 1af7373 commit 7dea9bb

File tree

3 files changed

+105
-11
lines changed

3 files changed

+105
-11
lines changed

‎pkg/controller/registry/resolver/evolver.go

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -51,8 +51,13 @@ func (e *NamespaceGenerationEvolver) Evolve(add map[OperatorSourceInfo]struct{})
5151
}
5252

5353
func (e *NamespaceGenerationEvolver) checkForUpdates() error {
54+
// maps the old operator identifier to the new operator
55+
updates := EmptyOperatorSet()
56+
5457
// take a snapshot of the current generation so that we don't update the same operator twice in one resolution
55-
for _, op := range e.gen.Operators().Snapshot() {
58+
snapshot := e.gen.Operators().Snapshot()
59+
60+
for _, op := range snapshot {
5661
// only check for updates if we have sourceinfo
5762
if op.SourceInfo() == &ExistingOperator {
5863
continue
@@ -68,15 +73,21 @@ func (e *NamespaceGenerationEvolver) checkForUpdates() error {
6873
return errors.Wrap(err, "error parsing bundle")
6974
}
7075
o.SetReplaces(op.Identifier())
76+
updates[op.Identifier()] = o
77+
}
7178

72-
// Remove the old operator and the APIs it provides before adding the new operator to the generation.
73-
e.gen.RemoveOperator(op)
79+
// remove any operators we found updates for
80+
for old := range updates {
81+
e.gen.RemoveOperator(e.gen.Operators().Snapshot()[old])
82+
}
7483

75-
// Add the new operator and the APIs it provides to the generation.
76-
if err := e.gen.AddOperator(o); err != nil {
84+
// add the new operators we found
85+
for _, new := range updates {
86+
if err := e.gen.AddOperator(new); err != nil {
7787
return errors.Wrap(err, "error calculating generation changes due to new bundle")
7888
}
7989
}
90+
8091
return nil
8192
}
8293

‎pkg/controller/registry/resolver/evolver_test.go

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -411,6 +411,46 @@ func TestNamespaceGenerationEvolver(t *testing.T) {
411411
"original"),
412412
),
413413
},
414+
{
415+
name: "OwnershipTransfer",
416+
fields: fields{
417+
querier: NewFakeSourceQuerier(map[CatalogKey][]*api.Bundle{
418+
CatalogKey{"catsrc", "catsrc-namespace"}: {
419+
bundle("depender.v2", "depender", "channel", "depender.v1",
420+
APISet{opregistry.APIKey{"g", "v", "k", "ks"}: {}}, EmptyAPISet(), EmptyAPISet(), EmptyAPISet()),
421+
bundle("original.v2", "o", "c", "original", EmptyAPISet(), EmptyAPISet(), EmptyAPISet(), EmptyAPISet()),
422+
},
423+
}),
424+
gen: NewGenerationFromOperators(
425+
NewFakeOperatorSurface("original", "o", "c", "", "catsrc", "", []opregistry.APIKey{{"g", "v", "k", "ks"}}, nil, nil, nil),
426+
NewFakeOperatorSurface("depender.v1", "depender", "channel", "", "catsrc", "", nil, []opregistry.APIKey{{"g", "v", "k", "ks"}}, nil, nil),
427+
),
428+
},
429+
args: args{},
430+
wantGen: NewGenerationFromOperators(
431+
NewFakeOperatorSurface("original.v2", "o", "c", "original", "catsrc", "", nil, nil, nil, nil),
432+
NewFakeOperatorSurface("depender.v2", "depender", "channel", "depender.v1", "catsrc", "", []opregistry.APIKey{{"g", "v", "k", "ks"}}, nil, nil, nil),
433+
),
434+
},
435+
{
436+
name: "PicksOlderProvider",
437+
fields: fields{
438+
querier: NewFakeSourceQuerier(map[CatalogKey][]*api.Bundle{
439+
CatalogKey{"catsrc", "catsrc-namespace"}: {
440+
bundle("original", "o", "c", "", APISet{opregistry.APIKey{"g", "v", "k", "ks"}: {}}, EmptyAPISet(), EmptyAPISet(), EmptyAPISet()),
441+
bundle("original.v2", "o", "c", "original", EmptyAPISet(), EmptyAPISet(), EmptyAPISet(), EmptyAPISet()),
442+
},
443+
}),
444+
gen: NewGenerationFromOperators(
445+
NewFakeOperatorSurface("depender.v1", "depender", "channel", "", "catsrc", "", nil, []opregistry.APIKey{{"g", "v", "k", "ks"}}, nil, nil),
446+
),
447+
},
448+
args: args{},
449+
wantGen: NewGenerationFromOperators(
450+
NewFakeOperatorSurface("original", "o", "c", "", "catsrc", "", []opregistry.APIKey{{"g", "v", "k", "ks"}},nil, nil, nil),
451+
NewFakeOperatorSurface("depender.v1", "depender", "channel", "", "catsrc", "", nil,[]opregistry.APIKey{{"g", "v", "k", "ks"}}, nil, nil),
452+
),
453+
},
414454
}
415455
for _, tt := range tests {
416456
t.Run(tt.name, func(t *testing.T) {

‎pkg/controller/registry/resolver/resolver_test.go

Lines changed: 49 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -99,8 +99,6 @@ func TestNamespaceResolver(t *testing.T) {
9999
},
100100
},
101101
{
102-
<<<<<<< HEAD
103-
=======
104102
name: "SingleNewSubscription/ResolveOne/BundlePath",
105103
clusterState: []runtime.Object{
106104
newSub(namespace, "a", "alpha", catalog),
@@ -132,7 +130,6 @@ func TestNamespaceResolver(t *testing.T) {
132130
},
133131
},
134132
{
135-
>>>>>>> fa4b13b9... Fix Operator Generation code
136133
name: "SingleNewSubscription/ResolveOne/AdditionalBundleObjects",
137134
clusterState: []runtime.Object{
138135
newSub(namespace, "a", "alpha", catalog),
@@ -223,8 +220,6 @@ func TestNamespaceResolver(t *testing.T) {
223220
},
224221
},
225222
{
226-
<<<<<<< HEAD
227-
=======
228223
name: "InstalledSub/UpdateAvailable/FromBundlePath",
229224
clusterState: []runtime.Object{
230225
existingSub(namespace, "a.v1", "a", "alpha", catalog),
@@ -250,7 +245,6 @@ func TestNamespaceResolver(t *testing.T) {
250245
},
251246
},
252247
{
253-
>>>>>>> fa4b13b9... Fix Operator Generation code
254248
name: "InstalledSub/NoRunningOperator",
255249
clusterState: []runtime.Object{
256250
existingSub(namespace, "a.v1", "a", "alpha", catalog),
@@ -411,6 +405,55 @@ func TestNamespaceResolver(t *testing.T) {
411405
},
412406
},
413407
},
408+
{
409+
// This test verifies that ownership of an api can be migrated between two operators
410+
name: "OwnedAPITransfer",
411+
clusterState: []runtime.Object{
412+
existingSub(namespace, "a.v1", "a", "alpha", catalog),
413+
existingOperator(namespace, "a.v1", "a", "alpha", "", Provides1, nil, nil, nil),
414+
existingSub(namespace, "b.v1", "b", "alpha", catalog),
415+
existingOperator(namespace, "b.v1", "b", "alpha", "", nil, Requires1, nil, nil),
416+
},
417+
querier: NewFakeSourceQuerier(map[CatalogKey][]*api.Bundle{
418+
catalog: {
419+
bundle("a.v2", "a", "alpha", "a.v1", nil, nil, nil, nil),
420+
bundle("b.v2", "b", "alpha", "b.v1", Provides1, nil, nil, nil),
421+
},
422+
}),
423+
out: out{
424+
steps: [][]*v1alpha1.Step{
425+
bundleSteps(bundle("a.v2", "a", "alpha", "a.v1", nil, nil, nil, nil), namespace, "", catalog),
426+
bundleSteps(bundle("b.v2", "b", "alpha", "b.v1", Provides1, nil, nil, nil), namespace, "", catalog),
427+
},
428+
subs: []*v1alpha1.Subscription{
429+
updatedSub(namespace, "a.v2", "a", "alpha", catalog),
430+
updatedSub(namespace, "b.v2", "b", "alpha", catalog),
431+
},
432+
},
433+
},
434+
{
435+
name: "PicksOlderProvider",
436+
clusterState: []runtime.Object{
437+
newSub(namespace, "b", "alpha", catalog),
438+
},
439+
querier: NewFakeSourceQuerier(map[CatalogKey][]*api.Bundle{
440+
catalog: {
441+
bundle("a.v1", "a", "alpha", "", Provides1, nil, nil, nil),
442+
bundle("a.v2", "a", "alpha", "a.v1", nil, nil, nil, nil),
443+
bundle("b.v1", "b", "alpha", "", nil, Requires1, nil, nil),
444+
},
445+
}),
446+
out: out{
447+
steps: [][]*v1alpha1.Step{
448+
bundleSteps(bundle("a.v1", "a", "alpha", "", Provides1, nil, nil, nil), namespace, "", catalog),
449+
bundleSteps(bundle("b.v1", "b", "alpha", "", nil, Requires1, nil, nil), namespace, "", catalog),
450+
subSteps(namespace, "a.v1", "a", "alpha", catalog),
451+
},
452+
subs: []*v1alpha1.Subscription{
453+
updatedSub(namespace, "b.v1", "b", "alpha", catalog),
454+
},
455+
},
456+
},
414457
{
415458
name: "InstalledSub/UpdateInHead/SkipRange",
416459
clusterState: []runtime.Object{

0 commit comments

Comments
 (0)