Skip to content

Commit 3740fa3

Browse files
openshift-merge-robotopenshift-merge-robot
authored
Merge pull request #1485 from awgreene/generation-bug-4.3
Bug 1827822: Generation bug 4.3
2 parents 1702292 + 70a8441 commit 3740fa3

File tree

5 files changed

+260
-3
lines changed

5 files changed

+260
-3
lines changed

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

Lines changed: 19 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
@@ -67,11 +72,22 @@ func (e *NamespaceGenerationEvolver) checkForUpdates() error {
6772
if err != nil {
6873
return errors.Wrap(err, "error parsing bundle")
6974
}
70-
if err := e.gen.AddOperator(o); err != nil {
75+
o.SetReplaces(op.Identifier())
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 {
7187
return errors.Wrap(err, "error calculating generation changes due to new bundle")
7288
}
73-
e.gen.RemoveOperator(op)
7489
}
90+
7591
return nil
7692
}
7793

‎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: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -349,6 +349,55 @@ func TestNamespaceResolver(t *testing.T) {
349349
},
350350
},
351351
},
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+
},
352401
{
353402
name: "InstalledSub/UpdateInHead/SkipRange",
354403
clusterState: []runtime.Object{
@@ -365,6 +414,43 @@ func TestNamespaceResolver(t *testing.T) {
365414
},
366415
},
367416
},
417+
{
418+
// This test uses logic that implements the FakeSourceQuerier to ensure
419+
// that the required API is provided by the new Operator.
420+
//
421+
// Background:
422+
// OLM used to add the new operator to the generation before removing
423+
// the old operator from the generation. The logic that removes an operator
424+
// from the current generation removes the APIs it provides from the list of
425+
// "available" APIs. This caused OLM to search for an operator that provides the API.
426+
// If the operator that provides the API uses a skipRange rather than the Spec.Replaces
427+
// field, the Replaces field is set to an empty string, causing OLM to fail to upgrade.
428+
name: "InstalledSubs/ExistingOperators/OldCSVsReplaced",
429+
clusterState: []runtime.Object{
430+
existingSub(namespace, "a.v1", "a", "alpha", catalog),
431+
existingSub(namespace, "b.v1", "b", "beta", catalog),
432+
existingOperator(namespace, "a.v1", "a", "alpha", "", nil, Requires1, nil, nil),
433+
existingOperator(namespace, "b.v1", "b", "beta", "", Provides1, nil, nil, nil),
434+
},
435+
querier: NewFakeSourceQuerier(map[CatalogKey][]*api.Bundle{
436+
catalog: {
437+
bundle("a.v1", "a", "alpha", "", nil, nil, nil, nil),
438+
bundle("a.v2", "a", "alpha", "a.v1", nil, Requires1, nil, nil),
439+
bundle("b.v1", "b", "beta", "", Provides1, nil, nil, nil),
440+
bundle("b.v2", "b", "beta", "b.v1", Provides1, nil, nil, nil),
441+
},
442+
}),
443+
out: out{
444+
steps: [][]*v1alpha1.Step{
445+
bundleSteps(bundle("a.v2", "a", "alpha", "a.v1", nil, Requires1, nil, nil), namespace, "", catalog),
446+
bundleSteps(bundle("b.v2", "b", "beta", "b.v1", Provides1, nil, nil, nil), namespace, "", catalog),
447+
},
448+
subs: []*v1alpha1.Subscription{
449+
updatedSub(namespace, "a.v2", "a", "alpha", catalog),
450+
updatedSub(namespace, "b.v2", "b", "beta", catalog),
451+
},
452+
},
453+
},
368454
}
369455
for _, tt := range tests {
370456
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
@@ -4,6 +4,7 @@ package e2e
44

55
import (
66
"fmt"
7+
"strings"
78
"testing"
89

910
"github.com/blang/semver"
@@ -693,6 +694,116 @@ func TestDeleteGRPCRegistryPodTriggersRecreation(t *testing.T) {
693694
require.Equal(t, 1, len(registryPods.Items), "unexpected number of replacement registry pods found")
694695
}
695696

697+
func TestDependencySkipRange(t *testing.T) {
698+
// Create a CatalogSource that contains the busybox v1 and busybox-dependency v1 images
699+
// Create a Subscription for busybox v1, which has a dependency on busybox-dependency v1.
700+
// Wait for the busybox and busybox2 Subscriptions to succeed
701+
// Wait for the CSVs to succeed
702+
// Update the catalog to point to an image that contains the busybox v2 and busybox-dependency v2 images.
703+
// Wait for the new Subscriptions to succeed and check if they include the new CSVs
704+
// Wait for the CSVs to succeed and confirm that the have the correct Spec.Replaces fields.
705+
defer cleaner.NotifyTestComplete(t, true)
706+
707+
sourceName := genName("catalog-")
708+
packageName := "busybox"
709+
channelName := "alpha"
710+
711+
catSrcImage := "quay.io/olmtest/busybox-dependencies-index"
712+
713+
// Create gRPC CatalogSource
714+
source := &v1alpha1.CatalogSource{
715+
TypeMeta: metav1.TypeMeta{
716+
Kind: v1alpha1.CatalogSourceKind,
717+
APIVersion: v1alpha1.CatalogSourceCRDAPIVersion,
718+
},
719+
ObjectMeta: metav1.ObjectMeta{
720+
Name: sourceName,
721+
Namespace: testNamespace,
722+
},
723+
Spec: v1alpha1.CatalogSourceSpec{
724+
SourceType: v1alpha1.SourceTypeGrpc,
725+
Image: catSrcImage + ":1.0.0",
726+
},
727+
}
728+
729+
crc := newCRClient(t)
730+
source, err := crc.OperatorsV1alpha1().CatalogSources(source.GetNamespace()).Create(source)
731+
require.NoError(t, err)
732+
defer func() {
733+
require.NoError(t, crc.OperatorsV1alpha1().CatalogSources(source.GetNamespace()).Delete(source.GetName(), &metav1.DeleteOptions{}))
734+
}()
735+
736+
// Create a Subscription for busybox
737+
subscriptionName := genName("sub-")
738+
cleanupSubscription := createSubscriptionForCatalog(t, crc, source.GetNamespace(), subscriptionName, source.GetName(), packageName, channelName, "", v1alpha1.ApprovalAutomatic)
739+
defer cleanupSubscription()
740+
741+
// Wait for the Subscription to succeed
742+
subscription, err := fetchSubscription(t, crc, testNamespace, subscriptionName, subscriptionStateAtLatestChecker)
743+
require.NoError(t, err)
744+
require.NotNil(t, subscription)
745+
require.Equal(t, subscription.Status.InstalledCSV, "busybox.v1.0.0")
746+
747+
// Confirm that a subscription was created for busybox-dependency
748+
subscriptionList, err := crc.OperatorsV1alpha1().Subscriptions(source.GetNamespace()).List(metav1.ListOptions{})
749+
require.NoError(t, err)
750+
dependencySubscriptionName := ""
751+
for _, sub := range subscriptionList.Items {
752+
if strings.HasPrefix(sub.GetName(), "busybox-dependency") {
753+
dependencySubscriptionName = sub.GetName()
754+
}
755+
}
756+
757+
require.NotEmpty(t, dependencySubscriptionName)
758+
// Wait for the Subscription to succeed
759+
subscription, err = fetchSubscription(t, crc, testNamespace, dependencySubscriptionName, subscriptionStateAtLatestChecker)
760+
require.NoError(t, err)
761+
require.NotNil(t, subscription)
762+
require.Equal(t, subscription.Status.InstalledCSV, "busybox-dependency.v1.0.0")
763+
764+
// Update the catalog image
765+
err = wait.PollImmediate(pollInterval, pollDuration, func() (bool, error) {
766+
existingSource, err := crc.OperatorsV1alpha1().CatalogSources(source.GetNamespace()).Get(sourceName, metav1.GetOptions{})
767+
if err != nil {
768+
return false, err
769+
}
770+
existingSource.Spec.Image = catSrcImage + ":2.0.0"
771+
772+
source, err = crc.OperatorsV1alpha1().CatalogSources(source.GetNamespace()).Update(existingSource)
773+
if err == nil {
774+
return true, nil
775+
}
776+
return false, nil
777+
})
778+
require.NoError(t, err)
779+
780+
// Wait for the busybox v2 Subscription to succeed
781+
subChecker := func(sub *v1alpha1.Subscription) bool {
782+
return sub.Status.InstalledCSV == "busybox.v2.0.0"
783+
}
784+
subscription, err = fetchSubscription(t, crc, testNamespace, subscriptionName, subChecker)
785+
require.NoError(t, err)
786+
require.NotNil(t, subscription)
787+
788+
// Wait for busybox v2 csv to succeed and check the replaces field
789+
csv, err := fetchCSV(t, crc, subscription.Status.CurrentCSV, subscription.GetNamespace(), csvSucceededChecker)
790+
require.NoError(t, err)
791+
require.Equal(t, "busybox.v1.0.0", csv.Spec.Replaces)
792+
793+
// Wait for the busybox-dependency v2 Subscription to succeed
794+
subChecker = func(sub *v1alpha1.Subscription) bool {
795+
return sub.Status.InstalledCSV == "busybox-dependency.v2.0.0"
796+
}
797+
subscription, err = fetchSubscription(t, crc, testNamespace, dependencySubscriptionName, subChecker)
798+
require.NoError(t, err)
799+
require.NotNil(t, subscription)
800+
801+
// Wait for busybox-dependency v2 csv to succeed and check the replaces field
802+
csv, err = fetchCSV(t, crc, subscription.Status.CurrentCSV, subscription.GetNamespace(), csvSucceededChecker)
803+
require.NoError(t, err)
804+
require.Equal(t, "busybox-dependency.v1.0.0", csv.Spec.Replaces)
805+
}
806+
696807
func getOperatorDeployment(c operatorclient.ClientInterface, namespace string, operatorLabels labels.Set) (*appsv1.Deployment, error) {
697808
deployments, err := c.ListDeploymentsWithLabels(namespace, operatorLabels)
698809
if err != nil || deployments == nil || len(deployments.Items) != 1 {

0 commit comments

Comments
 (0)