Skip to content

Commit f8ef76c

Browse files
openshift-merge-robotopenshift-merge-robot
authored
Merge pull request #1484 from awgreene/generation-bug-4.4
Bug 1827821: Generation bug 4.4 Backport
2 parents a6acf50 + e11d566 commit f8ef76c

File tree

4 files changed

+258
-6
lines changed

4 files changed

+258
-6
lines changed

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

Lines changed: 18 additions & 3 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,11 +73,21 @@ func (e *NamespaceGenerationEvolver) checkForUpdates() error {
6873
return errors.Wrap(err, "error parsing bundle")
6974
}
7075
o.SetReplaces(op.Identifier())
71-
if err := e.gen.AddOperator(o); err != nil {
76+
updates[op.Identifier()] = o
77+
}
78+
79+
// remove any operators we found updates for
80+
for old := range updates {
81+
e.gen.RemoveOperator(e.gen.Operators().Snapshot()[old])
82+
}
83+
84+
// add the new operators we found
85+
for _, new := range updates {
86+
if err := e.gen.AddOperator(new); err != nil {
7287
return errors.Wrap(err, "error calculating generation changes due to new bundle")
7388
}
74-
e.gen.RemoveOperator(op)
7589
}
90+
7691
return nil
7792
}
7893

‎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: 89 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,7 @@ func TestNamespaceResolver(t *testing.T) {
120120
},
121121
lookups: []v1alpha1.BundleLookup{
122122
{
123-
Path: "quay.io/test/bundle@sha256:abcd",
123+
Path: "quay.io/test/bundle@sha256:abcd",
124124
Identifier: "b.v1",
125125
CatalogSourceRef: &corev1.ObjectReference{
126126
Namespace: catalog.Namespace,
@@ -234,9 +234,9 @@ func TestNamespaceResolver(t *testing.T) {
234234
steps: [][]*v1alpha1.Step{},
235235
lookups: []v1alpha1.BundleLookup{
236236
{
237-
Path: "quay.io/test/bundle@sha256:abcd",
237+
Path: "quay.io/test/bundle@sha256:abcd",
238238
Identifier: "a.v2",
239-
Replaces: "a.v1",
239+
Replaces: "a.v1",
240240
CatalogSourceRef: &corev1.ObjectReference{
241241
Namespace: catalog.Namespace,
242242
Name: catalog.Name,
@@ -409,6 +409,55 @@ func TestNamespaceResolver(t *testing.T) {
409409
},
410410
},
411411
},
412+
{
413+
// This test verifies that ownership of an api can be migrated between two operators
414+
name: "OwnedAPITransfer",
415+
clusterState: []runtime.Object{
416+
existingSub(namespace, "a.v1", "a", "alpha", catalog),
417+
existingOperator(namespace, "a.v1", "a", "alpha", "", Provides1, nil, nil, nil),
418+
existingSub(namespace, "b.v1", "b", "alpha", catalog),
419+
existingOperator(namespace, "b.v1", "b", "alpha", "", nil, Requires1, nil, nil),
420+
},
421+
querier: NewFakeSourceQuerier(map[CatalogKey][]*api.Bundle{
422+
catalog: {
423+
bundle("a.v2", "a", "alpha", "a.v1", nil, nil, nil, nil),
424+
bundle("b.v2", "b", "alpha", "b.v1", Provides1, nil, nil, nil),
425+
},
426+
}),
427+
out: out{
428+
steps: [][]*v1alpha1.Step{
429+
bundleSteps(bundle("a.v2", "a", "alpha", "a.v1", nil, nil, nil, nil), namespace, "", catalog),
430+
bundleSteps(bundle("b.v2", "b", "alpha", "b.v1", Provides1, nil, nil, nil), namespace, "", catalog),
431+
},
432+
subs: []*v1alpha1.Subscription{
433+
updatedSub(namespace, "a.v2", "a", "alpha", catalog),
434+
updatedSub(namespace, "b.v2", "b", "alpha", catalog),
435+
},
436+
},
437+
},
438+
{
439+
name: "PicksOlderProvider",
440+
clusterState: []runtime.Object{
441+
newSub(namespace, "b", "alpha", catalog),
442+
},
443+
querier: NewFakeSourceQuerier(map[CatalogKey][]*api.Bundle{
444+
catalog: {
445+
bundle("a.v1", "a", "alpha", "", Provides1, nil, nil, nil),
446+
bundle("a.v2", "a", "alpha", "a.v1", nil, nil, nil, nil),
447+
bundle("b.v1", "b", "alpha", "", nil, Requires1, nil, nil),
448+
},
449+
}),
450+
out: out{
451+
steps: [][]*v1alpha1.Step{
452+
bundleSteps(bundle("a.v1", "a", "alpha", "", Provides1, nil, nil, nil), namespace, "", catalog),
453+
bundleSteps(bundle("b.v1", "b", "alpha", "", nil, Requires1, nil, nil), namespace, "", catalog),
454+
subSteps(namespace, "a.v1", "a", "alpha", catalog),
455+
},
456+
subs: []*v1alpha1.Subscription{
457+
updatedSub(namespace, "b.v1", "b", "alpha", catalog),
458+
},
459+
},
460+
},
412461
{
413462
name: "InstalledSub/UpdateInHead/SkipRange",
414463
clusterState: []runtime.Object{
@@ -425,6 +474,43 @@ func TestNamespaceResolver(t *testing.T) {
425474
},
426475
},
427476
},
477+
{
478+
// This test uses logic that implements the FakeSourceQuerier to ensure
479+
// that the required API is provided by the new Operator.
480+
//
481+
// Background:
482+
// OLM used to add the new operator to the generation before removing
483+
// the old operator from the generation. The logic that removes an operator
484+
// from the current generation removes the APIs it provides from the list of
485+
// "available" APIs. This caused OLM to search for an operator that provides the API.
486+
// If the operator that provides the API uses a skipRange rather than the Spec.Replaces
487+
// field, the Replaces field is set to an empty string, causing OLM to fail to upgrade.
488+
name: "InstalledSubs/ExistingOperators/OldCSVsReplaced",
489+
clusterState: []runtime.Object{
490+
existingSub(namespace, "a.v1", "a", "alpha", catalog),
491+
existingSub(namespace, "b.v1", "b", "beta", catalog),
492+
existingOperator(namespace, "a.v1", "a", "alpha", "", nil, Requires1, nil, nil),
493+
existingOperator(namespace, "b.v1", "b", "beta", "", Provides1, nil, nil, nil),
494+
},
495+
querier: NewFakeSourceQuerier(map[CatalogKey][]*api.Bundle{
496+
catalog: {
497+
bundle("a.v1", "a", "alpha", "", nil, nil, nil, nil),
498+
bundle("a.v2", "a", "alpha", "a.v1", nil, Requires1, nil, nil),
499+
bundle("b.v1", "b", "beta", "", Provides1, nil, nil, nil),
500+
bundle("b.v2", "b", "beta", "b.v1", Provides1, nil, nil, nil),
501+
},
502+
}),
503+
out: out{
504+
steps: [][]*v1alpha1.Step{
505+
bundleSteps(bundle("a.v2", "a", "alpha", "a.v1", nil, Requires1, nil, nil), namespace, "", catalog),
506+
bundleSteps(bundle("b.v2", "b", "beta", "b.v1", Provides1, nil, nil, nil), namespace, "", catalog),
507+
},
508+
subs: []*v1alpha1.Subscription{
509+
updatedSub(namespace, "a.v2", "a", "alpha", catalog),
510+
updatedSub(namespace, "b.v2", "b", "beta", catalog),
511+
},
512+
},
513+
},
428514
}
429515
for _, tt := range tests {
430516
t.Run(tt.name, func(t *testing.T) {

‎test/e2e/catalog_e2e_test.go

Lines changed: 111 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"fmt"
77
"net"
88
"reflect"
9+
"strings"
910
"testing"
1011
"time"
1112

@@ -941,6 +942,116 @@ func TestCatalogImageUpdate(t *testing.T) {
941942
}
942943
}
943944

945+
func TestDependencySkipRange(t *testing.T) {
946+
// Create a CatalogSource that contains the busybox v1 and busybox-dependency v1 images
947+
// Create a Subscription for busybox v1, which has a dependency on busybox-dependency v1.
948+
// Wait for the busybox and busybox2 Subscriptions to succeed
949+
// Wait for the CSVs to succeed
950+
// Update the catalog to point to an image that contains the busybox v2 and busybox-dependency v2 images.
951+
// Wait for the new Subscriptions to succeed and check if they include the new CSVs
952+
// Wait for the CSVs to succeed and confirm that the have the correct Spec.Replaces fields.
953+
defer cleaner.NotifyTestComplete(t, true)
954+
955+
sourceName := genName("catalog-")
956+
packageName := "busybox"
957+
channelName := "alpha"
958+
959+
catSrcImage := "quay.io/olmtest/busybox-dependencies-index"
960+
961+
// Create gRPC CatalogSource
962+
source := &v1alpha1.CatalogSource{
963+
TypeMeta: metav1.TypeMeta{
964+
Kind: v1alpha1.CatalogSourceKind,
965+
APIVersion: v1alpha1.CatalogSourceCRDAPIVersion,
966+
},
967+
ObjectMeta: metav1.ObjectMeta{
968+
Name: sourceName,
969+
Namespace: testNamespace,
970+
},
971+
Spec: v1alpha1.CatalogSourceSpec{
972+
SourceType: v1alpha1.SourceTypeGrpc,
973+
Image: catSrcImage + ":1.0.0",
974+
},
975+
}
976+
977+
crc := newCRClient(t)
978+
source, err := crc.OperatorsV1alpha1().CatalogSources(source.GetNamespace()).Create(source)
979+
require.NoError(t, err)
980+
defer func() {
981+
require.NoError(t, crc.OperatorsV1alpha1().CatalogSources(source.GetNamespace()).Delete(source.GetName(), &metav1.DeleteOptions{}))
982+
}()
983+
984+
// Create a Subscription for busybox
985+
subscriptionName := genName("sub-")
986+
cleanupSubscription := createSubscriptionForCatalog(t, crc, source.GetNamespace(), subscriptionName, source.GetName(), packageName, channelName, "", v1alpha1.ApprovalAutomatic)
987+
defer cleanupSubscription()
988+
989+
// Wait for the Subscription to succeed
990+
subscription, err := fetchSubscription(t, crc, testNamespace, subscriptionName, subscriptionStateAtLatestChecker)
991+
require.NoError(t, err)
992+
require.NotNil(t, subscription)
993+
require.Equal(t, subscription.Status.InstalledCSV, "busybox.v1.0.0")
994+
995+
// Confirm that a subscription was created for busybox-dependency
996+
subscriptionList, err := crc.OperatorsV1alpha1().Subscriptions(source.GetNamespace()).List(metav1.ListOptions{})
997+
require.NoError(t, err)
998+
dependencySubscriptionName := ""
999+
for _, sub := range subscriptionList.Items {
1000+
if strings.HasPrefix(sub.GetName(), "busybox-dependency") {
1001+
dependencySubscriptionName = sub.GetName()
1002+
}
1003+
}
1004+
1005+
require.NotEmpty(t, dependencySubscriptionName)
1006+
// Wait for the Subscription to succeed
1007+
subscription, err = fetchSubscription(t, crc, testNamespace, dependencySubscriptionName, subscriptionStateAtLatestChecker)
1008+
require.NoError(t, err)
1009+
require.NotNil(t, subscription)
1010+
require.Equal(t, subscription.Status.InstalledCSV, "busybox-dependency.v1.0.0")
1011+
1012+
// Update the catalog image
1013+
err = wait.PollImmediate(pollInterval, pollDuration, func() (bool, error) {
1014+
existingSource, err := crc.OperatorsV1alpha1().CatalogSources(source.GetNamespace()).Get(sourceName, metav1.GetOptions{})
1015+
if err != nil {
1016+
return false, err
1017+
}
1018+
existingSource.Spec.Image = catSrcImage + ":2.0.0"
1019+
1020+
source, err = crc.OperatorsV1alpha1().CatalogSources(source.GetNamespace()).Update(existingSource)
1021+
if err == nil {
1022+
return true, nil
1023+
}
1024+
return false, nil
1025+
})
1026+
require.NoError(t, err)
1027+
1028+
// Wait for the busybox v2 Subscription to succeed
1029+
subChecker := func(sub *v1alpha1.Subscription) bool {
1030+
return sub.Status.InstalledCSV == "busybox.v2.0.0"
1031+
}
1032+
subscription, err = fetchSubscription(t, crc, testNamespace, subscriptionName, subChecker)
1033+
require.NoError(t, err)
1034+
require.NotNil(t, subscription)
1035+
1036+
// Wait for busybox v2 csv to succeed and check the replaces field
1037+
csv, err := fetchCSV(t, crc, subscription.Status.CurrentCSV, subscription.GetNamespace(), csvSucceededChecker)
1038+
require.NoError(t, err)
1039+
require.Equal(t, "busybox.v1.0.0", csv.Spec.Replaces)
1040+
1041+
// Wait for the busybox-dependency v2 Subscription to succeed
1042+
subChecker = func(sub *v1alpha1.Subscription) bool {
1043+
return sub.Status.InstalledCSV == "busybox-dependency.v2.0.0"
1044+
}
1045+
subscription, err = fetchSubscription(t, crc, testNamespace, dependencySubscriptionName, subChecker)
1046+
require.NoError(t, err)
1047+
require.NotNil(t, subscription)
1048+
1049+
// Wait for busybox-dependency v2 csv to succeed and check the replaces field
1050+
csv, err = fetchCSV(t, crc, subscription.Status.CurrentCSV, subscription.GetNamespace(), csvSucceededChecker)
1051+
require.NoError(t, err)
1052+
require.Equal(t, "busybox-dependency.v1.0.0", csv.Spec.Replaces)
1053+
}
1054+
9441055
func getOperatorDeployment(c operatorclient.ClientInterface, namespace string, operatorLabels labels.Set) (*appsv1.Deployment, error) {
9451056
deployments, err := c.ListDeploymentsWithLabels(namespace, operatorLabels)
9461057
if err != nil || deployments == nil || len(deployments.Items) != 1 {

0 commit comments

Comments
 (0)