Skip to content

Commit 280a2a6

Browse files
openshift-merge-robotopenshift-merge-robot
authored
Merge pull request #1483 from awgreene/dependency-bug
Bug 1818788: Fix Operator Generation code
2 parents ed17b12 + 96bb690 commit 280a2a6

File tree

4 files changed

+257
-6
lines changed

4 files changed

+257
-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: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88
"net"
99
"os"
1010
"reflect"
11+
"strings"
1112
"time"
1213

1314
"github.com/blang/semver"
@@ -936,6 +937,115 @@ var _ = Describe("Catalog", func() {
936937
GinkgoT().Errorf("latest version of operator not installed: catalog souce update failed")
937938
}
938939
})
940+
It("Dependency has correct replaces field", func() {
941+
// Create a CatalogSource that contains the busybox v1 and busybox-dependency v1 images
942+
// Create a Subscription for busybox v1, which has a dependency on busybox-dependency v1.
943+
// Wait for the busybox and busybox2 Subscriptions to succeed
944+
// Wait for the CSVs to succeed
945+
// Update the catalog to point to an image that contains the busybox v2 and busybox-dependency v2 images.
946+
// Wait for the new Subscriptions to succeed and check if they include the new CSVs
947+
// Wait for the CSVs to succeed and confirm that the have the correct Spec.Replaces fields.
948+
defer cleaner.NotifyTestComplete(GinkgoT(), true)
949+
950+
sourceName := genName("catalog-")
951+
packageName := "busybox"
952+
channelName := "alpha"
953+
954+
catSrcImage := "quay.io/olmtest/busybox-dependencies-index"
955+
956+
// Create gRPC CatalogSource
957+
source := &v1alpha1.CatalogSource{
958+
TypeMeta: metav1.TypeMeta{
959+
Kind: v1alpha1.CatalogSourceKind,
960+
APIVersion: v1alpha1.CatalogSourceCRDAPIVersion,
961+
},
962+
ObjectMeta: metav1.ObjectMeta{
963+
Name: sourceName,
964+
Namespace: testNamespace,
965+
},
966+
Spec: v1alpha1.CatalogSourceSpec{
967+
SourceType: v1alpha1.SourceTypeGrpc,
968+
Image: catSrcImage + ":1.0.0",
969+
},
970+
}
971+
972+
crc := newCRClient(GinkgoT())
973+
source, err := crc.OperatorsV1alpha1().CatalogSources(source.GetNamespace()).Create(context.TODO(), source, metav1.CreateOptions{})
974+
require.NoError(GinkgoT(), err)
975+
defer func() {
976+
require.NoError(GinkgoT(), crc.OperatorsV1alpha1().CatalogSources(source.GetNamespace()).Delete(context.TODO(), source.GetName(), metav1.DeleteOptions{}))
977+
}()
978+
979+
// Create a Subscription for busybox
980+
subscriptionName := genName("sub-")
981+
cleanupSubscription := createSubscriptionForCatalog(GinkgoT(), crc, source.GetNamespace(), subscriptionName, source.GetName(), packageName, channelName, "", v1alpha1.ApprovalAutomatic)
982+
defer cleanupSubscription()
983+
984+
// Wait for the Subscription to succeed
985+
subscription, err := fetchSubscription(GinkgoT(), crc, testNamespace, subscriptionName, subscriptionStateAtLatestChecker)
986+
require.NoError(GinkgoT(), err)
987+
require.NotNil(GinkgoT(), subscription)
988+
require.Equal(GinkgoT(), subscription.Status.InstalledCSV, "busybox.v1.0.0")
989+
990+
// Confirm that a subscription was created for busybox-dependency
991+
subscriptionList, err := crc.OperatorsV1alpha1().Subscriptions(source.GetNamespace()).List(context.TODO(), metav1.ListOptions{})
992+
require.NoError(GinkgoT(), err)
993+
dependencySubscriptionName := ""
994+
for _, sub := range subscriptionList.Items {
995+
if strings.HasPrefix(sub.GetName(), "busybox-dependency") {
996+
dependencySubscriptionName = sub.GetName()
997+
}
998+
}
999+
1000+
require.NotEmpty(GinkgoT(), dependencySubscriptionName)
1001+
// Wait for the Subscription to succeed
1002+
subscription, err = fetchSubscription(GinkgoT(), crc, testNamespace, dependencySubscriptionName, subscriptionStateAtLatestChecker)
1003+
require.NoError(GinkgoT(), err)
1004+
require.NotNil(GinkgoT(), subscription)
1005+
require.Equal(GinkgoT(), subscription.Status.InstalledCSV, "busybox-dependency.v1.0.0")
1006+
1007+
// Update the catalog image
1008+
err = wait.PollImmediate(pollInterval, pollDuration, func() (bool, error) {
1009+
existingSource, err := crc.OperatorsV1alpha1().CatalogSources(source.GetNamespace()).Get(context.TODO(), sourceName, metav1.GetOptions{})
1010+
if err != nil {
1011+
return false, err
1012+
}
1013+
existingSource.Spec.Image = catSrcImage + ":2.0.0"
1014+
1015+
source, err = crc.OperatorsV1alpha1().CatalogSources(source.GetNamespace()).Update(context.TODO(), existingSource, metav1.UpdateOptions{})
1016+
if err == nil {
1017+
return true, nil
1018+
}
1019+
return false, nil
1020+
})
1021+
require.NoError(GinkgoT(), err)
1022+
1023+
// Wait for the busybox v2 Subscription to succeed
1024+
subChecker := func(sub *v1alpha1.Subscription) bool {
1025+
return sub.Status.InstalledCSV == "busybox.v2.0.0"
1026+
}
1027+
subscription, err = fetchSubscription(GinkgoT(), crc, testNamespace, subscriptionName, subChecker)
1028+
require.NoError(GinkgoT(), err)
1029+
require.NotNil(GinkgoT(), subscription)
1030+
1031+
// Wait for busybox v2 csv to succeed and check the replaces field
1032+
csv, err := fetchCSV(GinkgoT(), crc, subscription.Status.CurrentCSV, subscription.GetNamespace(), csvSucceededChecker)
1033+
require.NoError(GinkgoT(), err)
1034+
require.Equal(GinkgoT(), "busybox.v1.0.0", csv.Spec.Replaces)
1035+
1036+
// Wait for the busybox-dependency v2 Subscription to succeed
1037+
subChecker = func(sub *v1alpha1.Subscription) bool {
1038+
return sub.Status.InstalledCSV == "busybox-dependency.v2.0.0"
1039+
}
1040+
subscription, err = fetchSubscription(GinkgoT(), crc, testNamespace, dependencySubscriptionName, subChecker)
1041+
require.NoError(GinkgoT(), err)
1042+
require.NotNil(GinkgoT(), subscription)
1043+
1044+
// Wait for busybox-dependency v2 csv to succeed and check the replaces field
1045+
csv, err = fetchCSV(GinkgoT(), crc, subscription.Status.CurrentCSV, subscription.GetNamespace(), csvSucceededChecker)
1046+
require.NoError(GinkgoT(), err)
1047+
require.Equal(GinkgoT(), "busybox-dependency.v1.0.0", csv.Spec.Replaces)
1048+
})
9391049
})
9401050

9411051
const (

0 commit comments

Comments
 (0)