Bug 1827821: Generation bug 4.4 Backport

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,11 +73,21 @@ func (e *NamespaceGenerationEvolver) checkForUpdates() error {
 			return errors.Wrap(err, "error parsing bundle")
 		}
 		o.SetReplaces(op.Identifier())
-		if err := e.gen.AddOperator(o); err != nil {
+		updates[op.Identifier()] = o
+	}
+
+	// remove any operators we found updates for
+	for old := range updates {
+		e.gen.RemoveOperator(e.gen.Operators().Snapshot()[old])
+	}
+
+	// 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")
 		}
-		e.gen.RemoveOperator(op)
 	}
+
 	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

@@ -120,7 +120,7 @@ func TestNamespaceResolver(t *testing.T) {
 				},
 				lookups: []v1alpha1.BundleLookup{
 					{
-						Path: "quay.io/test/bundle@sha256:abcd",
+						Path:       "quay.io/test/bundle@sha256:abcd",
 						Identifier: "b.v1",
 						CatalogSourceRef: &corev1.ObjectReference{
 							Namespace: catalog.Namespace,
@@ -234,9 +234,9 @@ func TestNamespaceResolver(t *testing.T) {
 				steps: [][]*v1alpha1.Step{},
 				lookups: []v1alpha1.BundleLookup{
 					{
-						Path:     "quay.io/test/bundle@sha256:abcd",
+						Path:       "quay.io/test/bundle@sha256:abcd",
 						Identifier: "a.v2",
-						Replaces: "a.v1",
+						Replaces:   "a.v1",
 						CatalogSourceRef: &corev1.ObjectReference{
 							Namespace: catalog.Namespace,
 							Name:      catalog.Name,
@@ -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{
@@ -425,6 +474,43 @@ func TestNamespaceResolver(t *testing.T) {
 				},
 			},
 		},
+		{
+			// This test uses logic that implements the FakeSourceQuerier to ensure
+			// that the required API is provided by the new Operator.
+			//
+			// Background:
+			// OLM used to add the new operator to the generation before removing
+			// the old operator from the generation. The logic that removes an operator
+			// from the current generation removes the APIs it provides from the list of
+			// "available" APIs. This caused OLM to search for an operator that provides the API.
+			// If the operator that provides the API uses a skipRange rather than the Spec.Replaces
+			// field, the Replaces field is set to an empty string, causing OLM to fail to upgrade.
+			name: "InstalledSubs/ExistingOperators/OldCSVsReplaced",
+			clusterState: []runtime.Object{
+				existingSub(namespace, "a.v1", "a", "alpha", catalog),
+				existingSub(namespace, "b.v1", "b", "beta", catalog),
+				existingOperator(namespace, "a.v1", "a", "alpha", "", nil, Requires1, nil, nil),
+				existingOperator(namespace, "b.v1", "b", "beta", "", Provides1, nil, nil, nil),
+			},
+			querier: NewFakeSourceQuerier(map[CatalogKey][]*api.Bundle{
+				catalog: {
+					bundle("a.v1", "a", "alpha", "", nil, nil, nil, nil),
+					bundle("a.v2", "a", "alpha", "a.v1", nil, Requires1, nil, nil),
+					bundle("b.v1", "b", "beta", "", Provides1, nil, nil, nil),
+					bundle("b.v2", "b", "beta", "b.v1", Provides1, nil, nil, nil),
+				},
+			}),
+			out: out{
+				steps: [][]*v1alpha1.Step{
+					bundleSteps(bundle("a.v2", "a", "alpha", "a.v1", nil, Requires1, nil, nil), namespace, "", catalog),
+					bundleSteps(bundle("b.v2", "b", "beta", "b.v1", Provides1, nil, nil, nil), namespace, "", catalog),
+				},
+				subs: []*v1alpha1.Subscription{
+					updatedSub(namespace, "a.v2", "a", "alpha", catalog),
+					updatedSub(namespace, "b.v2", "b", "beta", catalog),
+				},
+			},
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {

test/e2e/catalog_e2e_test.go

@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"net"
 	"reflect"
+	"strings"
 	"testing"
 	"time"
 
@@ -941,6 +942,116 @@ func TestCatalogImageUpdate(t *testing.T) {
 	}
 }
 
+func TestDependencySkipRange(t *testing.T) {
+	// Create a CatalogSource that contains the busybox v1 and busybox-dependency v1 images
+	// Create a Subscription for busybox v1, which has a dependency on busybox-dependency v1.
+	// Wait for the busybox and busybox2 Subscriptions to succeed
+	// Wait for the CSVs to succeed
+	// Update the catalog to point to an image that contains the busybox v2 and busybox-dependency v2 images.
+	// Wait for the new Subscriptions to succeed and check if they include the new CSVs
+	// Wait for the CSVs to succeed and confirm that the have the correct Spec.Replaces fields.
+	defer cleaner.NotifyTestComplete(t, true)
+
+	sourceName := genName("catalog-")
+	packageName := "busybox"
+	channelName := "alpha"
+
+	catSrcImage := "quay.io/olmtest/busybox-dependencies-index"
+
+	// Create gRPC CatalogSource
+	source := &v1alpha1.CatalogSource{
+		TypeMeta: metav1.TypeMeta{
+			Kind:       v1alpha1.CatalogSourceKind,
+			APIVersion: v1alpha1.CatalogSourceCRDAPIVersion,
+		},
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      sourceName,
+			Namespace: testNamespace,
+		},
+		Spec: v1alpha1.CatalogSourceSpec{
+			SourceType: v1alpha1.SourceTypeGrpc,
+			Image:      catSrcImage + ":1.0.0",
+		},
+	}
+
+	crc := newCRClient(t)
+	source, err := crc.OperatorsV1alpha1().CatalogSources(source.GetNamespace()).Create(source)
+	require.NoError(t, err)
+	defer func() {
+		require.NoError(t, crc.OperatorsV1alpha1().CatalogSources(source.GetNamespace()).Delete(source.GetName(), &metav1.DeleteOptions{}))
+	}()
+
+	// Create a Subscription for busybox
+	subscriptionName := genName("sub-")
+	cleanupSubscription := createSubscriptionForCatalog(t, crc, source.GetNamespace(), subscriptionName, source.GetName(), packageName, channelName, "", v1alpha1.ApprovalAutomatic)
+	defer cleanupSubscription()
+
+	// Wait for the Subscription to succeed
+	subscription, err := fetchSubscription(t, crc, testNamespace, subscriptionName, subscriptionStateAtLatestChecker)
+	require.NoError(t, err)
+	require.NotNil(t, subscription)
+	require.Equal(t, subscription.Status.InstalledCSV, "busybox.v1.0.0")
+
+	// Confirm that a subscription was created for busybox-dependency
+	subscriptionList, err := crc.OperatorsV1alpha1().Subscriptions(source.GetNamespace()).List(metav1.ListOptions{})
+	require.NoError(t, err)
+	dependencySubscriptionName := ""
+	for _, sub := range subscriptionList.Items {
+		if strings.HasPrefix(sub.GetName(), "busybox-dependency") {
+			dependencySubscriptionName = sub.GetName()
+		}
+	}
+
+	require.NotEmpty(t, dependencySubscriptionName)
+	// Wait for the Subscription to succeed
+	subscription, err = fetchSubscription(t, crc, testNamespace, dependencySubscriptionName, subscriptionStateAtLatestChecker)
+	require.NoError(t, err)
+	require.NotNil(t, subscription)
+	require.Equal(t, subscription.Status.InstalledCSV, "busybox-dependency.v1.0.0")
+
+	// Update the catalog image
+	err = wait.PollImmediate(pollInterval, pollDuration, func() (bool, error) {
+		existingSource, err := crc.OperatorsV1alpha1().CatalogSources(source.GetNamespace()).Get(sourceName, metav1.GetOptions{})
+		if err != nil {
+			return false, err
+		}
+		existingSource.Spec.Image = catSrcImage + ":2.0.0"
+
+		source, err = crc.OperatorsV1alpha1().CatalogSources(source.GetNamespace()).Update(existingSource)
+		if err == nil {
+			return true, nil
+		}
+		return false, nil
+	})
+	require.NoError(t, err)
+
+	// Wait for the busybox v2 Subscription to succeed
+	subChecker := func(sub *v1alpha1.Subscription) bool {
+		return sub.Status.InstalledCSV == "busybox.v2.0.0"
+	}
+	subscription, err = fetchSubscription(t, crc, testNamespace, subscriptionName, subChecker)
+	require.NoError(t, err)
+	require.NotNil(t, subscription)
+
+	// Wait for busybox v2 csv to succeed and check the replaces field
+	csv, err := fetchCSV(t, crc, subscription.Status.CurrentCSV, subscription.GetNamespace(), csvSucceededChecker)
+	require.NoError(t, err)
+	require.Equal(t, "busybox.v1.0.0", csv.Spec.Replaces)
+
+	// Wait for the busybox-dependency v2 Subscription to succeed
+	subChecker = func(sub *v1alpha1.Subscription) bool {
+		return sub.Status.InstalledCSV == "busybox-dependency.v2.0.0"
+	}
+	subscription, err = fetchSubscription(t, crc, testNamespace, dependencySubscriptionName, subChecker)
+	require.NoError(t, err)
+	require.NotNil(t, subscription)
+
+	// Wait for busybox-dependency v2 csv to succeed and check the replaces field
+	csv, err = fetchCSV(t, crc, subscription.Status.CurrentCSV, subscription.GetNamespace(), csvSucceededChecker)
+	require.NoError(t, err)
+	require.Equal(t, "busybox-dependency.v1.0.0", csv.Spec.Replaces)
+}
+
 func getOperatorDeployment(c operatorclient.ClientInterface, namespace string, operatorLabels labels.Set) (*appsv1.Deployment, error) {
 	deployments, err := c.ListDeploymentsWithLabels(namespace, operatorLabels)
 	if err != nil || deployments == nil || len(deployments.Items) != 1 {