Skip to content
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

Reflect BundleUnpacking cond removal in missed e2e test #3170

Merged
merged 1 commit into from
Feb 5, 2024
Merged

Reflect BundleUnpacking cond removal in missed e2e test #3170

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
2 changes: 1 addition & 1 deletion test/e2e/subscription_e2e_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2806,7 +2806,7 @@ properties:
if err != nil {
return err
}
if cond := fetched.Status.GetCondition(v1alpha1.SubscriptionBundleUnpacking); cond.Status != corev1.ConditionFalse {
if cond := fetched.Status.GetCondition(v1alpha1.SubscriptionBundleUnpacking); cond.Status != corev1.ConditionUnknown {
Copy link
Contributor

Choose a reason for hiding this comment

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

sidenote that a condition being present with conditionunknown is not the same thing as a condition being absent, the GetCondition() impl is sufficiently surprising ... we might want to clean up that semantic in the future

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is interesting. The more I dig into this area the more messy it looks. I'd love to create an issue so that we capture "what should ideally happen" so that we can actually clean these things up. For capturing "what should happen" in the issue, @stevekuznetsov what's the canonical way of proving condition isn't present?

Copy link
Member

Choose a reason for hiding this comment

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

I suspect the upstream API conventions are the reason for that logic:

Controllers should apply their conditions to a resource the first time they visit the resource, even if the status is Unknown. This allows other components in the system to know that the condition exists and the controller is making progress on reconciling that resource.

Not all controllers will observe the previous advice about reporting "Unknown" or "False" values. For known conditions, the absence of a condition status should be interpreted the same as Unknown, and typically indicates that reconciliation has not yet finished (or that the resource state may not yet be observable).

https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#typical-status-properties

Copy link
Contributor Author

@anik120 anik120 Feb 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ah okay. So setting the conditions as False is in fact the right way to go. Which means, in Ankita's PR, she was doing the right thing by setting the condition to false. However, "the mess" I kept uncovering was because after Ankita did the right thing for one condition, there existed a mixture of how we handle the conditions in the CR, some were being removed, while one was being set to false. And this confusion of some conditions being removed and one condition being set to False led to the original "why isn't the ResolutionFailed condition being reset even after the error was resolved by means of a Healthy catalog?" confusion -> bug.

Creating an issue to capture the task of "set conditions to false", so that we can follow upstream API conventions for our conditions in future releases, while avoiding confusing bugs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#3171

Copy link
Contributor

Choose a reason for hiding this comment

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

I think given the context your change is perfectly acceptable. Orthogonally to what Joe's citing, a method that searches for a condition should just return nil or add some boolean to the signature and return false when no such condition exists on the object. Returning some default value and imposing some semantic on top is surprising. For instance, if we were to be writing a controller using this GetCondition() function and we wanted to know - have we already set the thing to Unknown or do we need to add one? Today, not possible with GetCondition().

return fmt.Errorf("subscription condition %s has unexpected value %s, expected %s", v1alpha1.SubscriptionBundleUnpacking, cond.Status, corev1.ConditionFalse)
}
if cond := fetched.Status.GetCondition(v1alpha1.SubscriptionBundleUnpackFailed); cond.Status != corev1.ConditionUnknown {
Expand Down