Skip to content

Fix Condition check for Reconcile InstallPlan status #3153

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 12, 2024
Merged

Fix Condition check for Reconcile InstallPlan status #3153

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 39 additions & 38 deletions test/e2e/subscription_e2e_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ import (
"github.com/operator-framework/operator-lifecycle-manager/pkg/controller/install"
"github.com/operator-framework/operator-lifecycle-manager/pkg/controller/registry"
"github.com/operator-framework/operator-lifecycle-manager/pkg/controller/registry/resolver/projection"
"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/comparison"
"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorclient"
"github.com/operator-framework/operator-lifecycle-manager/test/e2e/ctx"
registryapi "github.com/operator-framework/operator-registry/pkg/api"
Expand Down Expand Up @@ -1029,29 +1028,7 @@ var _ = Describe("Subscription", func() {
})

It("can reconcile InstallPlan status", func() {
By(`TestSubscriptionInstallPlanStatus ensures that a Subscription has the appropriate status conditions for possible referenced`)
By(`InstallPlan states.`)
By(` BySteps:`)
By(`- Utilize the namespace and OG targeting that namespace created in the BeforeEach clause`)
By(`- Create CatalogSource, cs, in ns`)
By(`- Create Subscription to a package of cs in ns, sub`)
By(`- Wait for the package from sub to install successfully with no remaining InstallPlan status conditions`)
By(`- Store conditions for later comparision`)
By(`- Get the InstallPlan`)
By(`- Set the InstallPlan's approval mode to Manual`)
By(`- Set the InstallPlan's phase to None`)
By(`- Wait for sub to have status condition SubscriptionInstallPlanPending true and reason InstallPlanNotYetReconciled`)
By(`- Get the latest IntallPlan and set the phase to InstallPlanPhaseRequiresApproval`)
By(`- Wait for sub to have status condition SubscriptionInstallPlanPending true and reason RequiresApproval`)
By(`- Get the latest InstallPlan and set the phase to InstallPlanPhaseInstalling`)
By(`- Wait for sub to have status condition SubscriptionInstallPlanPending true and reason Installing`)
By(`- Get the latest InstallPlan and set the phase to InstallPlanPhaseFailed and remove all status conditions`)
By(`- Wait for sub to have status condition SubscriptionInstallPlanFailed true and reason InstallPlanFailed`)
By(`- Get the latest InstallPlan and set status condition of type Installed to false with reason InstallComponentFailed`)
By(`- Wait for sub to have status condition SubscriptionInstallPlanFailed true and reason InstallComponentFailed`)
By(`- Delete the referenced InstallPlan`)
By(`- Wait for sub to have status condition SubscriptionInstallPlanMissing true`)
By(`- Ensure original non-InstallPlan status conditions remain after InstallPlan transitions`)
By(`TestSubscriptionInstallPlanStatus ensures that a Subscription has the appropriate status conditions for possible referenced InstallPlan states.`)
c := newKubeClient()
crc := newCRClient()

Expand Down Expand Up @@ -1099,6 +1076,7 @@ var _ = Describe("Subscription", func() {
ref := sub.Status.InstallPlanRef
Expect(ref).ToNot(BeNil())

By(`Get the InstallPlan`)
plan := &operatorsv1alpha1.InstallPlan{}
plan.SetNamespace(ref.Namespace)
plan.SetName(ref.Name)
Expand Down Expand Up @@ -1151,14 +1129,14 @@ var _ = Describe("Subscription", func() {
return true
}

By(`Sometimes the transition from installing to complete can be so quick that the test does not capture`)
By(`the condition in the subscription before it is removed. To mitigate this, we check if the installplan`)
By(`has transitioned to complete and exit out the fetch subscription loop if so.`)
By(`This is a mitigation. We should probably fix this test appropriately.`)
By(`issue: https://github.com/operator-framework/operator-lifecycle-manager/issues/2667`)
// Sometimes the transition from installing to complete can be so quick that the test does not capture
// the condition in the subscription before it is removed. To mitigate this, we check if the installplan
// has transitioned to complete and exit out the fetch subscription loop if so.
// This is a mitigation. We should probably fix this test appropriately.
// issue: https://github.com/operator-framework/operator-lifecycle-manager/issues/2667
ip, err := crc.OperatorsV1alpha1().InstallPlans(generatedNamespace.GetName()).Get(context.TODO(), plan.Name, metav1.GetOptions{})
if err != nil {
By(`retry on failure`)
// retry on failure
return false
}
isInstallPlanComplete := ip.Status.Phase == operatorsv1alpha1.InstallPlanPhaseComplete
Expand Down Expand Up @@ -1210,16 +1188,13 @@ var _ = Describe("Subscription", func() {
Expect(err).ToNot(HaveOccurred())
Expect(sub).ToNot(BeNil())

By(`Ensure original non-InstallPlan status conditions remain after InstallPlan transitions`)
hashEqual := comparison.NewHashEqualitor()
By(`Ensure InstallPlan-related status conditions match what we're expecting`)
for _, cond := range conds {
switch condType := cond.Type; condType {
case operatorsv1alpha1.SubscriptionInstallPlanPending, operatorsv1alpha1.SubscriptionInstallPlanFailed:
require.FailNowf(GinkgoT(), "failed", "subscription contains unexpected installplan condition: %v", cond)
case operatorsv1alpha1.SubscriptionInstallPlanMissing:
require.Equal(GinkgoT(), operatorsv1alpha1.ReferencedInstallPlanNotFound, cond.Reason)
default:
require.True(GinkgoT(), hashEqual(cond, sub.Status.GetCondition(condType)), "non-installplan status condition changed")
}
}
})
Expand Down Expand Up @@ -3218,27 +3193,53 @@ func subscriptionHasCurrentCSV(currentCSV string) subscriptionStateChecker {
}

func subscriptionHasCondition(condType operatorsv1alpha1.SubscriptionConditionType, status corev1.ConditionStatus, reason, message string) subscriptionStateChecker {
var lastCond operatorsv1alpha1.SubscriptionCondition
lastTime := time.Now()
// if status/reason/message meet expectations, then subscription state is considered met/true
// IFF this is the result of a recent change of status/reason/message
// else, cache the current status/reason/message for next loop/comparison
return func(subscription *operatorsv1alpha1.Subscription) bool {
cond := subscription.Status.GetCondition(condType)
if cond.Status == status && cond.Reason == reason && cond.Message == message {
fmt.Printf("subscription condition met %v\n", cond)
if lastCond.Status != cond.Status && lastCond.Reason != cond.Reason && lastCond.Message == cond.Message {
GinkgoT().Logf("waited %s subscription condition met %v\n", time.Since(lastTime), cond)
lastTime = time.Now()
lastCond = cond
}
return true
}

fmt.Printf("subscription condition not met: %v\n", cond)
if lastCond.Status != cond.Status && lastCond.Reason != cond.Reason && lastCond.Message == cond.Message {
GinkgoT().Logf("waited %s subscription condition not met: %v\n", time.Since(lastTime), cond)
lastTime = time.Now()
lastCond = cond
}
return false
Comment on lines +3204 to 3217
Copy link
Contributor

@grokspawn grokspawn Jan 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would love if there were a way to write this for greater readability.
Maybe a comment something like

// if status/reason/message meet expectations, then subscription state is considered met/true IFF this is the result of a recent change of status/reason
// else, cache the current status/reason for next loop/comparison

I also have a concern that this makes this test edge-set instead of level-set. I'm not sure how purist we need to be for an e2e test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the general trend to reduce repetitive logging, and I admit, the comparisons are ugly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BUT... that's not the reason for the change... The change reduces the logging when conditions don't change, the reason for getting out of the loop doesn't change, just the output.

}
}

func subscriptionDoesNotHaveCondition(condType operatorsv1alpha1.SubscriptionConditionType) subscriptionStateChecker {
var lastStatus corev1.ConditionStatus
lastTime := time.Now()
// if status meets expectations, then subscription state is considered met/true
// IFF this is the result of a recent change of status
// else, cache the current status for next loop/comparison
return func(subscription *operatorsv1alpha1.Subscription) bool {
cond := subscription.Status.GetCondition(condType)
if cond.Status == corev1.ConditionUnknown {
fmt.Printf("subscription condition not found\n")
if cond.Status != lastStatus {
GinkgoT().Logf("waited %s subscription condition not found\n", time.Since(lastTime))
lastStatus = cond.Status
lastTime = time.Now()
}
return true
}

fmt.Printf("subscription condition found: %v\n", cond)
if cond.Status != lastStatus {
GinkgoT().Logf("waited %s subscription condition found: %v\n", time.Since(lastTime), cond)
lastStatus = cond.Status
lastTime = time.Now()
}
return false
}
}
Expand Down