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.

Author: ecordell

pkg/controller/registry/resolver/evolver.go

@@ -51,8 +51,13 @@ func (e *NamespaceGenerationEvolver) Evolve(add map[OperatorSourceInfo]struct{})
 }
 
 func (e *NamespaceGenerationEvolver) checkForUpdates() error {
+	// maps the old operator identifier to the new operator
+	updates := EmptyOperatorSet()
+
 	// take a snapshot of the current generation so that we don't update the same operator twice in one resolution
-	for _, op := range e.gen.Operators().Snapshot() {
+	snapshot := e.gen.Operators().Snapshot()
+
+	for _, op := range snapshot {
 		// only check for updates if we have sourceinfo
 		if op.SourceInfo() == &ExistingOperator {
 			continue
@@ -68,15 +73,21 @@ func (e *NamespaceGenerationEvolver) checkForUpdates() error {
 			return errors.Wrap(err, "error parsing bundle")
 		}
 		o.SetReplaces(op.Identifier())
+		updates[op.Identifier()] = o
+	}
 
-		// Remove the old operator and the APIs it provides before adding the new operator to the generation.
-		e.gen.RemoveOperator(op)
+	// remove any operators we found updates for
+	for old := range updates {
+		e.gen.RemoveOperator(e.gen.Operators().Snapshot()[old])
+	}
 
-		// Add the new operator and the APIs it provides to the generation.
-		if err := e.gen.AddOperator(o); err != nil {
+	// add the new operators we found
+	for _, new := range updates {
+		if err := e.gen.AddOperator(new); err != nil {
 			return errors.Wrap(err, "error calculating generation changes due to new bundle")
 		}
 	}
+
 	return nil
 }
 

pkg/controller/registry/resolver/evolver_test.go

@@ -411,6 +411,46 @@ func TestNamespaceGenerationEvolver(t *testing.T) {
 					"original"),
 			),
 		},
+		{
+			name: "OwnershipTransfer",
+			fields: fields{
+				querier: NewFakeSourceQuerier(map[CatalogKey][]*api.Bundle{
+					CatalogKey{"catsrc", "catsrc-namespace"}: {
+						bundle("depender.v2", "depender", "channel", "depender.v1",
+							APISet{opregistry.APIKey{"g", "v", "k", "ks"}: {}}, EmptyAPISet(), EmptyAPISet(), EmptyAPISet()),
+						bundle("original.v2", "o", "c", "original", EmptyAPISet(), EmptyAPISet(), EmptyAPISet(), EmptyAPISet()),
+					},
+				}),
+				gen: NewGenerationFromOperators(
+					NewFakeOperatorSurface("original", "o", "c", "", "catsrc", "", []opregistry.APIKey{{"g", "v", "k", "ks"}}, nil, nil, nil),
+					NewFakeOperatorSurface("depender.v1", "depender", "channel", "", "catsrc", "", nil, []opregistry.APIKey{{"g", "v", "k", "ks"}}, nil, nil),
+				),
+			},
+			args: args{},
+			wantGen: NewGenerationFromOperators(
+				NewFakeOperatorSurface("original.v2", "o", "c", "original", "catsrc", "", nil, nil, nil, nil),
+				NewFakeOperatorSurface("depender.v2", "depender", "channel", "depender.v1", "catsrc", "", []opregistry.APIKey{{"g", "v", "k", "ks"}}, nil, nil, nil),
+			),
+		},
+		{
+			name: "PicksOlderProvider",
+			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()),
+					},
+				}),
+				gen: NewGenerationFromOperators(
+					NewFakeOperatorSurface("depender.v1", "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.v1", "depender", "channel", "", "catsrc", "", nil,[]opregistry.APIKey{{"g", "v", "k", "ks"}}, nil, nil),
+			),
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {

pkg/controller/registry/resolver/resolver_test.go

@@ -409,6 +409,55 @@ func TestNamespaceResolver(t *testing.T) {
 				},
 			},
 		},
+		{
+			// This test verifies that ownership of an api can be migrated between two operators
+			name: "OwnedAPITransfer",
+			clusterState: []runtime.Object{
+				existingSub(namespace, "a.v1", "a", "alpha", catalog),
+				existingOperator(namespace, "a.v1", "a", "alpha", "", Provides1, nil, nil, nil),
+				existingSub(namespace, "b.v1", "b", "alpha", catalog),
+				existingOperator(namespace, "b.v1", "b", "alpha", "", nil, Requires1, nil, nil),
+			},
+			querier: NewFakeSourceQuerier(map[CatalogKey][]*api.Bundle{
+				catalog: {
+					bundle("a.v2", "a", "alpha", "a.v1", nil, nil, nil, nil),
+					bundle("b.v2", "b", "alpha", "b.v1", Provides1, nil, nil, nil),
+				},
+			}),
+			out: out{
+				steps: [][]*v1alpha1.Step{
+					bundleSteps(bundle("a.v2", "a", "alpha", "a.v1", nil, nil, nil, nil), namespace, "", catalog),
+					bundleSteps(bundle("b.v2", "b", "alpha", "b.v1", Provides1, nil, nil, nil), namespace, "", catalog),
+				},
+				subs: []*v1alpha1.Subscription{
+					updatedSub(namespace, "a.v2", "a", "alpha", catalog),
+					updatedSub(namespace, "b.v2", "b", "alpha", catalog),
+				},
+			},
+		},
+		{
+			name: "PicksOlderProvider",
+			clusterState: []runtime.Object{
+				newSub(namespace, "b", "alpha", catalog),
+			},
+			querier: NewFakeSourceQuerier(map[CatalogKey][]*api.Bundle{
+				catalog: {
+					bundle("a.v1", "a", "alpha", "", Provides1, nil, nil, nil),
+					bundle("a.v2", "a", "alpha", "a.v1", nil, nil, nil, nil),
+					bundle("b.v1", "b", "alpha", "", nil, Requires1, nil, nil),
+				},
+			}),
+			out: out{
+				steps: [][]*v1alpha1.Step{
+					bundleSteps(bundle("a.v1", "a", "alpha", "", Provides1, nil, nil, nil), namespace, "", catalog),
+					bundleSteps(bundle("b.v1", "b", "alpha", "", nil, Requires1, nil, nil), namespace, "", catalog),
+					subSteps(namespace, "a.v1", "a", "alpha", catalog),
+				},
+				subs: []*v1alpha1.Subscription{
+					updatedSub(namespace, "b.v1", "b", "alpha", catalog),
+				},
+			},
+		},
 		{
 			name: "InstalledSub/UpdateInHead/SkipRange",
 			clusterState: []runtime.Object{