Skip to content

Commit 70a8441

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 70a8441

File tree

4 files changed

+109
-67
lines changed

4 files changed

+109
-67
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/operators.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -356,6 +356,10 @@ func (o *Operator) Replaces() string {
356356
return o.replaces
357357
}
358358

359+
func (o *Operator) SetReplaces(replacing string) {
360+
o.replaces = replacing
361+
}
362+
359363
func (o *Operator) Package() string {
360364
return o.bundle.PackageName
361365
}

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

Lines changed: 49 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -99,40 +99,6 @@ func TestNamespaceResolver(t *testing.T) {
9999
},
100100
},
101101
{
102-
<<<<<<< HEAD
103-
=======
104-
name: "SingleNewSubscription/ResolveOne/BundlePath",
105-
clusterState: []runtime.Object{
106-
newSub(namespace, "a", "alpha", catalog),
107-
},
108-
querier: NewFakeSourceQuerier(map[CatalogKey][]*api.Bundle{
109-
catalog: {
110-
bundle("a.v1", "a", "alpha", "", nil, Requires1, nil, nil),
111-
stripManifests(withBundlePath(bundle("b.v1", "b", "beta", "", Provides1, nil, nil, nil), "quay.io/test/bundle@sha256:abcd")),
112-
},
113-
}),
114-
out: out{
115-
steps: [][]*v1alpha1.Step{
116-
bundleSteps(bundle("a.v1", "a", "alpha", "", nil, Requires1, nil, nil), namespace, "", catalog),
117-
subSteps(namespace, "b.v1", "b", "beta", catalog),
118-
},
119-
lookups: []v1alpha1.BundleLookup{
120-
{
121-
Path: "quay.io/test/bundle@sha256:abcd",
122-
Identifier: "b.v1",
123-
CatalogSourceRef: &corev1.ObjectReference{
124-
Namespace: catalog.Namespace,
125-
Name: catalog.Name,
126-
},
127-
},
128-
},
129-
subs: []*v1alpha1.Subscription{
130-
updatedSub(namespace, "a.v1", "a", "alpha", catalog),
131-
},
132-
},
133-
},
134-
{
135-
>>>>>>> fa4b13b9... Fix Operator Generation code
136102
name: "SingleNewSubscription/ResolveOne/AdditionalBundleObjects",
137103
clusterState: []runtime.Object{
138104
newSub(namespace, "a", "alpha", catalog),
@@ -223,34 +189,6 @@ func TestNamespaceResolver(t *testing.T) {
223189
},
224190
},
225191
{
226-
<<<<<<< HEAD
227-
=======
228-
name: "InstalledSub/UpdateAvailable/FromBundlePath",
229-
clusterState: []runtime.Object{
230-
existingSub(namespace, "a.v1", "a", "alpha", catalog),
231-
existingOperator(namespace, "a.v1", "a", "alpha", "", Provides1, nil, nil, nil),
232-
},
233-
querier: NewFakeSourceQuerierCustomReplacement(catalog, stripManifests(withBundlePath(bundle("a.v2", "a", "alpha", "a.v1", Provides1, nil, nil, nil), "quay.io/test/bundle@sha256:abcd"))),
234-
out: out{
235-
steps: [][]*v1alpha1.Step{},
236-
lookups: []v1alpha1.BundleLookup{
237-
{
238-
Path: "quay.io/test/bundle@sha256:abcd",
239-
Identifier: "a.v2",
240-
Replaces: "a.v1",
241-
CatalogSourceRef: &corev1.ObjectReference{
242-
Namespace: catalog.Namespace,
243-
Name: catalog.Name,
244-
},
245-
},
246-
},
247-
subs: []*v1alpha1.Subscription{
248-
updatedSub(namespace, "a.v2", "a", "alpha", catalog),
249-
},
250-
},
251-
},
252-
{
253-
>>>>>>> fa4b13b9... Fix Operator Generation code
254192
name: "InstalledSub/NoRunningOperator",
255193
clusterState: []runtime.Object{
256194
existingSub(namespace, "a.v1", "a", "alpha", catalog),
@@ -411,6 +349,55 @@ func TestNamespaceResolver(t *testing.T) {
411349
},
412350
},
413351
},
352+
{
353+
// This test verifies that ownership of an api can be migrated between two operators
354+
name: "OwnedAPITransfer",
355+
clusterState: []runtime.Object{
356+
existingSub(namespace, "a.v1", "a", "alpha", catalog),
357+
existingOperator(namespace, "a.v1", "a", "alpha", "", Provides1, nil, nil, nil),
358+
existingSub(namespace, "b.v1", "b", "alpha", catalog),
359+
existingOperator(namespace, "b.v1", "b", "alpha", "", nil, Requires1, nil, nil),
360+
},
361+
querier: NewFakeSourceQuerier(map[CatalogKey][]*api.Bundle{
362+
catalog: {
363+
bundle("a.v2", "a", "alpha", "a.v1", nil, nil, nil, nil),
364+
bundle("b.v2", "b", "alpha", "b.v1", Provides1, nil, nil, nil),
365+
},
366+
}),
367+
out: out{
368+
steps: [][]*v1alpha1.Step{
369+
bundleSteps(bundle("a.v2", "a", "alpha", "a.v1", nil, nil, nil, nil), namespace, "", catalog),
370+
bundleSteps(bundle("b.v2", "b", "alpha", "b.v1", Provides1, nil, nil, nil), namespace, "", catalog),
371+
},
372+
subs: []*v1alpha1.Subscription{
373+
updatedSub(namespace, "a.v2", "a", "alpha", catalog),
374+
updatedSub(namespace, "b.v2", "b", "alpha", catalog),
375+
},
376+
},
377+
},
378+
{
379+
name: "PicksOlderProvider",
380+
clusterState: []runtime.Object{
381+
newSub(namespace, "b", "alpha", catalog),
382+
},
383+
querier: NewFakeSourceQuerier(map[CatalogKey][]*api.Bundle{
384+
catalog: {
385+
bundle("a.v1", "a", "alpha", "", Provides1, nil, nil, nil),
386+
bundle("a.v2", "a", "alpha", "a.v1", nil, nil, nil, nil),
387+
bundle("b.v1", "b", "alpha", "", nil, Requires1, nil, nil),
388+
},
389+
}),
390+
out: out{
391+
steps: [][]*v1alpha1.Step{
392+
bundleSteps(bundle("a.v1", "a", "alpha", "", Provides1, nil, nil, nil), namespace, "", catalog),
393+
bundleSteps(bundle("b.v1", "b", "alpha", "", nil, Requires1, nil, nil), namespace, "", catalog),
394+
subSteps(namespace, "a.v1", "a", "alpha", catalog),
395+
},
396+
subs: []*v1alpha1.Subscription{
397+
updatedSub(namespace, "b.v1", "b", "alpha", catalog),
398+
},
399+
},
400+
},
414401
{
415402
name: "InstalledSub/UpdateInHead/SkipRange",
416403
clusterState: []runtime.Object{

0 commit comments

Comments
 (0)