Skip to content

Commit 4596d05

Browse files
hasbro17hasbro17
committed
Consider InstallPlan phase=Complete as terminal
Also added e2e test for missing ServiceAccountRef in OperatorGroup and reformatted e2e tests to use Gomega matchers
1 parent bc37ac4 commit 4596d05

File tree

3 files changed

+147
-116
lines changed

3 files changed

+147
-116
lines changed

‎pkg/controller/operators/catalog/operator.go

Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1331,7 +1331,8 @@ func (o *Operator) syncInstallPlans(obj interface{}) (syncError error) {
13311331
return
13321332
}
13331333

1334-
if plan.Status.Phase == v1alpha1.InstallPlanPhaseFailed {
1334+
// Complete and Failed are terminal phases
1335+
if plan.Status.Phase == v1alpha1.InstallPlanPhaseFailed || plan.Status.Phase == v1alpha1.InstallPlanPhaseComplete {
13351336
return
13361337
}
13371338

@@ -1446,16 +1447,19 @@ func (o *Operator) syncInstallPlans(obj interface{}) (syncError error) {
14461447

14471448
func (o *Operator) requeueSubscriptionForInstallPlan(plan *v1alpha1.InstallPlan, logger *logrus.Entry) {
14481449
// Notify subscription loop of installplan changes
1449-
if owners := ownerutil.GetOwnersByKind(plan, v1alpha1.SubscriptionKind); len(owners) > 0 {
1450-
for _, owner := range owners {
1451-
logger.WithField("owner", owner).Debug("requeueing installplan owner")
1452-
if err := o.subQueueSet.Requeue(plan.GetNamespace(), owner.Name); err != nil {
1453-
logger.WithError(err).Warn("error requeuing installplan owner")
1454-
}
1455-
}
1450+
owners := ownerutil.GetOwnersByKind(plan, v1alpha1.SubscriptionKind)
1451+
1452+
if len(owners) == 0 {
1453+
logger.Trace("no installplan owner subscriptions found to requeue")
14561454
return
14571455
}
1458-
logger.Trace("no installplan owner subscriptions found to requeue")
1456+
1457+
for _, owner := range owners {
1458+
logger.WithField("owner", owner).Debug("requeueing installplan owner")
1459+
if err := o.subQueueSet.Requeue(plan.GetNamespace(), owner.Name); err != nil {
1460+
logger.WithError(err).Warn("error requeuing installplan owner")
1461+
}
1462+
}
14591463
}
14601464

14611465
type installPlanTransitioner interface {

‎pkg/controller/operators/catalog/operator_test.go

Lines changed: 12 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -162,33 +162,33 @@ func TestSyncInstallPlanUnhappy(t *testing.T) {
162162
)
163163

164164
tests := []struct {
165-
testName string
166-
in *v1alpha1.InstallPlan
167-
err error
168-
expectedPhase v1alpha1.InstallPlanPhase
169-
operatorGroups []*operatorsv1.OperatorGroup
170-
clientObjs []runtime.Object
165+
testName string
166+
err error
167+
in *v1alpha1.InstallPlan
168+
expectedPhase v1alpha1.InstallPlanPhase
169+
170+
clientObjs []runtime.Object
171171
}{
172172
{
173173
testName: "NoStatus",
174-
in: installPlan("p", namespace, v1alpha1.InstallPlanPhaseNone),
175174
err: nil,
176175
expectedPhase: v1alpha1.InstallPlanPhaseNone,
176+
in: installPlan("p", namespace, v1alpha1.InstallPlanPhaseNone),
177177
},
178178
{
179179
// This checks that an installplan is marked as failed when no operatorgroup is present
180180
testName: "HasSteps/NoOperatorGroup",
181-
in: ipWithSteps,
182181
err: nil,
183182
expectedPhase: v1alpha1.InstallPlanPhaseFailed,
183+
in: ipWithSteps,
184184
},
185185
{
186186
// This checks that an installplan is marked as failed when multiple operator groups are present for the same namespace
187187
testName: "HasSteps/TooManyOperatorGroups",
188-
in: ipWithSteps,
189188
err: nil,
190189
expectedPhase: v1alpha1.InstallPlanPhaseFailed,
191-
operatorGroups: []*operatorsv1.OperatorGroup{
190+
in: ipWithSteps,
191+
clientObjs: []runtime.Object{
192192
operatorGroup("og1", "sa", namespace,
193193
&corev1.ObjectReference{
194194
Kind: "ServiceAccount",
@@ -206,10 +206,10 @@ func TestSyncInstallPlanUnhappy(t *testing.T) {
206206
{
207207
// This checks that an installplan is marked as failed when no service account is synced for the operator group, i.e the service account ref doesn't exist
208208
testName: "HasSteps/NonExistentServiceAccount",
209-
in: ipWithSteps,
210209
err: nil,
211210
expectedPhase: v1alpha1.InstallPlanPhaseFailed,
212-
operatorGroups: []*operatorsv1.OperatorGroup{
211+
in: ipWithSteps,
212+
clientObjs: []runtime.Object{
213213
operatorGroup("og", "sa1", namespace, nil),
214214
},
215215
},
@@ -221,9 +221,6 @@ func TestSyncInstallPlanUnhappy(t *testing.T) {
221221
defer cancel()
222222

223223
tt.clientObjs = append(tt.clientObjs, tt.in)
224-
for _, og := range tt.operatorGroups {
225-
tt.clientObjs = append(tt.clientObjs, og)
226-
}
227224

228225
op, err := NewFakeOperator(ctx, namespace, []string{namespace}, withClientObjs(tt.clientObjs...))
229226
require.NoError(t, err)

‎test/e2e/installplan_e2e_test.go

Lines changed: 122 additions & 92 deletions
Original file line numberDiff line numberDiff line change
@@ -2650,11 +2650,7 @@ var _ = Describe("Install Plan", func() {
26502650
require.Equal(GinkgoT(), 1, len(ips.Items), "If this test fails it should be taken seriously and not treated as a flake. \n%v", ips.Items)
26512651
})
26522652

2653-
It("should fail an InstallPlan when no operatorgroup is present", func() {
2654-
2655-
log := func(s string) {
2656-
GinkgoT().Logf("%s: %s", time.Now().Format("15:04:05.9999"), s)
2657-
}
2653+
It("should fail an InstallPlan when no OperatorGroup is present", func() {
26582654

26592655
ns := &corev1.Namespace{}
26602656
ns.SetName(genName("ns-"))
@@ -2664,61 +2660,33 @@ var _ = Describe("Install Plan", func() {
26642660

26652661
// Create a namespace
26662662
ns, err := c.KubernetesInterface().CoreV1().Namespaces().Create(context.TODO(), ns, metav1.CreateOptions{})
2667-
require.NoError(GinkgoT(), err)
2663+
Expect(err).NotTo(HaveOccurred())
2664+
26682665
deleteOpts := &metav1.DeleteOptions{}
26692666
defer func() {
2670-
require.NoError(GinkgoT(), c.KubernetesInterface().CoreV1().Namespaces().Delete(context.TODO(), ns.GetName(), *deleteOpts))
2667+
err := c.KubernetesInterface().CoreV1().Namespaces().Delete(context.TODO(), ns.GetName(), *deleteOpts)
2668+
Expect(err).NotTo(HaveOccurred())
26712669
}()
26722670

2673-
mainPackageName := genName("nginx-")
2674-
mainPackageStable := fmt.Sprintf("%s-stable", mainPackageName)
2675-
stableChannel := "stable"
2676-
mainCSV := newCSV(mainPackageStable, ns.GetName(), "", semver.MustParse("0.1.0"), nil, nil, nil)
2671+
// Create InstallPlan
2672+
installPlanName := "ip"
2673+
ip := newInstallPlanWithDummySteps(installPlanName, ns.GetName(), operatorsv1alpha1.InstallPlanPhaseNone)
2674+
outIP, err := crc.OperatorsV1alpha1().InstallPlans(ns.GetName()).Create(context.TODO(), ip, metav1.CreateOptions{})
2675+
Expect(err).NotTo(HaveOccurred())
26772676

2678-
defer func() {
2679-
require.NoError(GinkgoT(), crc.OperatorsV1alpha1().Subscriptions(ns.GetName()).DeleteCollection(context.TODO(), metav1.DeleteOptions{}, metav1.ListOptions{}))
2680-
}()
2681-
2682-
mainCatalogName := genName("mock-ocs-main-")
2683-
mainManifests := []registry.PackageManifest{
2684-
{
2685-
PackageName: mainPackageName,
2686-
Channels: []registry.PackageChannel{
2687-
{Name: stableChannel, CurrentCSVName: mainPackageStable},
2688-
},
2689-
DefaultChannelName: stableChannel,
2690-
},
2691-
}
2692-
2693-
// Create the main catalog source
2694-
_, cleanupMainCatalogSource := createInternalCatalogSource(c, crc, mainCatalogName, ns.GetName(), mainManifests, nil, []operatorsv1alpha1.ClusterServiceVersion{mainCSV})
2695-
defer cleanupMainCatalogSource()
2696-
2697-
// Attempt to get the catalog source before creating install plan
2698-
_, err = fetchCatalogSourceOnStatus(crc, mainCatalogName, ns.GetName(), catalogSourceRegistryPodSynced)
2699-
require.NoError(GinkgoT(), err)
2700-
2701-
subscriptionName := genName("sub-nginx-")
2702-
subscriptionCleanup := createSubscriptionForCatalog(crc, ns.GetName(), subscriptionName, mainCatalogName, mainPackageName, stableChannel, "", operatorsv1alpha1.ApprovalAutomatic)
2703-
defer subscriptionCleanup()
2704-
2705-
subscription, err := fetchSubscription(crc, ns.GetName(), subscriptionName, subscriptionHasInstallPlanChecker)
2706-
require.NoError(GinkgoT(), err)
2707-
require.NotNil(GinkgoT(), subscription)
2708-
2709-
installPlanName := subscription.Status.InstallPlanRef.Name
2677+
// The status gets ignored on create so we need to update it else the InstallPlan sync ignores
2678+
// InstallPlans without any steps or bundle lookups
2679+
outIP.Status = ip.Status
2680+
_, err = crc.OperatorsV1alpha1().InstallPlans(ns.GetName()).UpdateStatus(context.TODO(), outIP, metav1.UpdateOptions{})
2681+
Expect(err).NotTo(HaveOccurred())
27102682

27112683
fetchedInstallPlan, err := fetchInstallPlanWithNamespace(GinkgoT(), crc, installPlanName, ns.GetName(), buildInstallPlanPhaseCheckFunc(operatorsv1alpha1.InstallPlanPhaseFailed))
2712-
require.NoError(GinkgoT(), err)
2713-
log(fmt.Sprintf("Install plan %s fetched with phase %s", fetchedInstallPlan.GetName(), fetchedInstallPlan.Status.Phase))
2714-
log(fmt.Sprintf("Install plan %s fetched with conditions %+v", fetchedInstallPlan.GetName(), fetchedInstallPlan.Status.Conditions))
2684+
Expect(err).NotTo(HaveOccurred())
2685+
ctx.Ctx().Logf(fmt.Sprintf("Install plan %s fetched with phase %s", fetchedInstallPlan.GetName(), fetchedInstallPlan.Status.Phase))
2686+
ctx.Ctx().Logf(fmt.Sprintf("Install plan %s fetched with conditions %+v", fetchedInstallPlan.GetName(), fetchedInstallPlan.Status.Conditions))
27152687
})
27162688

2717-
It("should fail an InstallPlan when multiple operatorgroups are present", func() {
2718-
2719-
log := func(s string) {
2720-
GinkgoT().Logf("%s: %s", time.Now().Format("15:04:05.9999"), s)
2721-
}
2689+
It("should fail an InstallPlan when multiple OperatorGroups are present", func() {
27222690

27232691
ns := &corev1.Namespace{}
27242692
ns.SetName(genName("ns-"))
@@ -2728,40 +2696,13 @@ var _ = Describe("Install Plan", func() {
27282696

27292697
// Create a namespace
27302698
ns, err := c.KubernetesInterface().CoreV1().Namespaces().Create(context.TODO(), ns, metav1.CreateOptions{})
2699+
Expect(err).NotTo(HaveOccurred())
27312700
require.NoError(GinkgoT(), err)
27322701
deleteOpts := &metav1.DeleteOptions{}
27332702
defer func() {
27342703
require.NoError(GinkgoT(), c.KubernetesInterface().CoreV1().Namespaces().Delete(context.TODO(), ns.GetName(), *deleteOpts))
27352704
}()
27362705

2737-
mainPackageName := genName("nginx-")
2738-
mainPackageStable := fmt.Sprintf("%s-stable", mainPackageName)
2739-
stableChannel := "stable"
2740-
mainCSV := newCSV(mainPackageStable, ns.GetName(), "", semver.MustParse("0.1.0"), nil, nil, nil)
2741-
2742-
defer func() {
2743-
require.NoError(GinkgoT(), crc.OperatorsV1alpha1().Subscriptions(ns.GetName()).DeleteCollection(context.TODO(), metav1.DeleteOptions{}, metav1.ListOptions{}))
2744-
}()
2745-
2746-
mainCatalogName := genName("mock-ocs-main-")
2747-
mainManifests := []registry.PackageManifest{
2748-
{
2749-
PackageName: mainPackageName,
2750-
Channels: []registry.PackageChannel{
2751-
{Name: stableChannel, CurrentCSVName: mainPackageStable},
2752-
},
2753-
DefaultChannelName: stableChannel,
2754-
},
2755-
}
2756-
2757-
// Create the main catalog source
2758-
_, cleanupMainCatalogSource := createInternalCatalogSource(c, crc, mainCatalogName, ns.GetName(), mainManifests, nil, []operatorsv1alpha1.ClusterServiceVersion{mainCSV})
2759-
defer cleanupMainCatalogSource()
2760-
2761-
// Attempt to get the catalog source before creating install plan
2762-
_, err = fetchCatalogSourceOnStatus(crc, mainCatalogName, ns.GetName(), catalogSourceRegistryPodSynced)
2763-
require.NoError(GinkgoT(), err)
2764-
27652706
// Create 2 operatorgroups in the same namespace
27662707
og1 := &operatorsv1.OperatorGroup{
27672708
ObjectMeta: metav1.ObjectMeta{
@@ -2784,20 +2725,78 @@ var _ = Describe("Install Plan", func() {
27842725
_, err = crc.OperatorsV1().OperatorGroups(ns.GetName()).Create(context.TODO(), og2, metav1.CreateOptions{})
27852726
Expect(err).ToNot(HaveOccurred())
27862727

2787-
subscriptionName := genName("sub-nginx-")
2788-
subscriptionCleanup := createSubscriptionForCatalog(crc, ns.GetName(), subscriptionName, mainCatalogName, mainPackageName, stableChannel, "", operatorsv1alpha1.ApprovalAutomatic)
2789-
defer subscriptionCleanup()
2728+
installPlanName := "ip"
2729+
ip := newInstallPlanWithDummySteps(installPlanName, ns.GetName(), operatorsv1alpha1.InstallPlanPhaseNone)
2730+
outIP, err := crc.OperatorsV1alpha1().InstallPlans(ns.GetName()).Create(context.TODO(), ip, metav1.CreateOptions{})
2731+
Expect(err).NotTo(HaveOccurred())
27902732

2791-
subscription, err := fetchSubscription(crc, ns.GetName(), subscriptionName, subscriptionHasInstallPlanChecker)
2733+
// The status gets ignored on create so we need to update it else the InstallPlan sync ignores
2734+
// InstallPlans without any steps or bundle lookups
2735+
outIP.Status = ip.Status
2736+
_, err = crc.OperatorsV1alpha1().InstallPlans(ns.GetName()).UpdateStatus(context.TODO(), outIP, metav1.UpdateOptions{})
2737+
Expect(err).NotTo(HaveOccurred())
2738+
2739+
fetchedInstallPlan, err := fetchInstallPlanWithNamespace(GinkgoT(), crc, installPlanName, ns.GetName(), buildInstallPlanPhaseCheckFunc(operatorsv1alpha1.InstallPlanPhaseFailed))
27922740
require.NoError(GinkgoT(), err)
2793-
require.NotNil(GinkgoT(), subscription)
2741+
ctx.Ctx().Logf(fmt.Sprintf("Install plan %s fetched with status %s", fetchedInstallPlan.GetName(), fetchedInstallPlan.Status.Phase))
2742+
ctx.Ctx().Logf(fmt.Sprintf("Install plan %s fetched with conditions %+v", fetchedInstallPlan.GetName(), fetchedInstallPlan.Status.Conditions))
2743+
})
27942744

2795-
installPlanName := subscription.Status.InstallPlanRef.Name
2745+
It("should fail an InstallPlan when an OperatorGroup specifies an unsynced ServiceAccount with no ServiceAccountRef", func() {
2746+
2747+
ns := &corev1.Namespace{}
2748+
ns.SetName(genName("ns-"))
2749+
2750+
c := newKubeClient()
2751+
crc := newCRClient()
2752+
2753+
// Create a namespace
2754+
ns, err := c.KubernetesInterface().CoreV1().Namespaces().Create(context.TODO(), ns, metav1.CreateOptions{})
2755+
Expect(err).NotTo(HaveOccurred())
2756+
require.NoError(GinkgoT(), err)
2757+
deleteOpts := &metav1.DeleteOptions{}
2758+
defer func() {
2759+
require.NoError(GinkgoT(), c.KubernetesInterface().CoreV1().Namespaces().Delete(context.TODO(), ns.GetName(), *deleteOpts))
2760+
}()
2761+
2762+
// OperatorGroup with serviceaccount specified
2763+
og := &operatorsv1.OperatorGroup{
2764+
ObjectMeta: metav1.ObjectMeta{
2765+
Name: "og",
2766+
},
2767+
Spec: operatorsv1.OperatorGroupSpec{
2768+
TargetNamespaces: []string{ns.GetName()},
2769+
ServiceAccountName: "foobar",
2770+
},
2771+
}
2772+
og, err = crc.OperatorsV1().OperatorGroups(ns.GetName()).Create(context.TODO(), og, metav1.CreateOptions{})
2773+
Expect(err).ToNot(HaveOccurred())
2774+
2775+
// Update OperatorGroup status with namespace but no service account reference
2776+
now := metav1.Now()
2777+
og.Status = operatorsv1.OperatorGroupStatus{
2778+
Namespaces: []string{ns.GetName()},
2779+
ServiceAccountRef: nil,
2780+
LastUpdated: &now,
2781+
}
2782+
_, err = crc.OperatorsV1().OperatorGroups(ns.GetName()).UpdateStatus(context.TODO(), og, metav1.UpdateOptions{})
2783+
Expect(err).ToNot(HaveOccurred())
2784+
2785+
installPlanName := "ip"
2786+
ip := newInstallPlanWithDummySteps(installPlanName, ns.GetName(), operatorsv1alpha1.InstallPlanPhaseNone)
2787+
outIP, err := crc.OperatorsV1alpha1().InstallPlans(ns.GetName()).Create(context.TODO(), ip, metav1.CreateOptions{})
2788+
Expect(err).NotTo(HaveOccurred())
2789+
2790+
// The status gets ignored on create so we need to update it else the InstallPlan sync ignores
2791+
// InstallPlans without any steps or bundle lookups
2792+
outIP.Status = ip.Status
2793+
_, err = crc.OperatorsV1alpha1().InstallPlans(ns.GetName()).UpdateStatus(context.TODO(), outIP, metav1.UpdateOptions{})
2794+
Expect(err).NotTo(HaveOccurred())
27962795

27972796
fetchedInstallPlan, err := fetchInstallPlanWithNamespace(GinkgoT(), crc, installPlanName, ns.GetName(), buildInstallPlanPhaseCheckFunc(operatorsv1alpha1.InstallPlanPhaseFailed))
27982797
require.NoError(GinkgoT(), err)
2799-
log(fmt.Sprintf("Install plan %s fetched with status %s", fetchedInstallPlan.GetName(), fetchedInstallPlan.Status.Phase))
2800-
log(fmt.Sprintf("Install plan %s fetched with conditions %+v", fetchedInstallPlan.GetName(), fetchedInstallPlan.Status.Conditions))
2798+
ctx.Ctx().Logf(fmt.Sprintf("Install plan %s fetched with status %s", fetchedInstallPlan.GetName(), fetchedInstallPlan.Status.Phase))
2799+
ctx.Ctx().Logf(fmt.Sprintf("Install plan %s fetched with conditions %+v", fetchedInstallPlan.GetName(), fetchedInstallPlan.Status.Conditions))
28012800
})
28022801

28032802
It("compresses installplan step resource manifests to configmap references", func() {
@@ -2959,14 +2958,14 @@ var _ = Describe("Install Plan", func() {
29592958
// Wait for the OperatorGroup to be synced and have a status.ServiceAccountRef
29602959
// before moving on. Otherwise the catalog operator treats it as an invalid OperatorGroup
29612960
// and fails the InstallPlan
2962-
Eventually(func() (bool, error) {
2961+
Eventually(func() (*corev1.ObjectReference, error) {
29632962
outOG, err := crc.OperatorsV1().OperatorGroups(ns.GetName()).Get(context.TODO(), og.Name, metav1.GetOptions{})
29642963
if err != nil {
2965-
return false, err
2964+
return nil, err
29662965
}
2967-
fmt.Fprintf(GinkgoWriter, "[DEBUG] Operator Group Status: %+v\n", outOG.Status)
2968-
return outOG.Status.ServiceAccountRef != nil, nil
2969-
}).Should(BeTrue())
2966+
ctx.Ctx().Logf("[DEBUG] Operator Group Status: %+v\n", outOG.Status)
2967+
return outOG.Status.ServiceAccountRef, nil
2968+
}).Should(BeNil())
29702969

29712970
crd := apiextensionsv1.CustomResourceDefinition{
29722971
ObjectMeta: metav1.ObjectMeta{
@@ -3626,3 +3625,34 @@ func newCSV(name, namespace, replaces string, version semver.Version, owned []ap
36263625

36273626
return csv
36283627
}
3628+
3629+
func newInstallPlanWithDummySteps(name, namespace string, phase operatorsv1alpha1.InstallPlanPhase) *operatorsv1alpha1.InstallPlan {
3630+
return &operatorsv1alpha1.InstallPlan{
3631+
ObjectMeta: metav1.ObjectMeta{
3632+
Name: name,
3633+
Namespace: namespace,
3634+
},
3635+
Spec: operatorsv1alpha1.InstallPlanSpec{
3636+
ClusterServiceVersionNames: []string{"foobar"},
3637+
Approval: operatorsv1alpha1.ApprovalAutomatic,
3638+
Approved: true,
3639+
},
3640+
Status: operatorsv1alpha1.InstallPlanStatus{
3641+
CatalogSources: []string{"catalog"},
3642+
Phase: phase,
3643+
Plan: []*operatorsv1alpha1.Step{
3644+
{
3645+
Resource: operatorsv1alpha1.StepResource{
3646+
CatalogSource: "catalog",
3647+
CatalogSourceNamespace: namespace,
3648+
Group: "",
3649+
Version: "v1",
3650+
Kind: "Foo",
3651+
Name: "bar",
3652+
},
3653+
Status: operatorsv1alpha1.StepStatusUnknown,
3654+
},
3655+
},
3656+
},
3657+
}
3658+
}

0 commit comments

Comments
 (0)