[controller] resurface bundledeployment errors

[varshaprasad96]

This PR enables the operator object to resurface the errors in Rukpak by setting its conditions as "BundleDeploymentFailed" if any of the BD errors.

[joelanford]

I don't think it's safe to assume that all Conditions impact the determination of whether the Operator Ready condition is failing.

I also don't think its safe to assume that a False condition indicates an error (e.g. what if we add Progressing as a new condition type to rukpak)

I think what we should probably do here is specifically check for known condition types and implement logic based on the various possibilities.

[varshaprasad96]

For now I'm checking only the TypeHasValidBundle and TypeInstalled conditions to be false explicitly. Is it sufficient, or can there ever be a situation in rukpak where the status are left Unknown for which we need to error?

[joelanford]

Would it work to check for != True for now?

I think we'll want to tighten this up to handle all those possibilities though. E.g. if installed and had valid bundle are both unknown, then ready should also be unknown.

[varshaprasad96]

It does, but just wanted to make sure if Unknown status is something for which we want to error for? Usually its an intermediate status before either failing or succeeding. But if there is a case where the status is just left Unknown, then we should be specifically checking that and erroring.

[joelanford]

I don't think we should assume anything about intermediate states of dependent objects. I think we should just come up with whatever mapping we think is best looking at state at that single point in time without any assumptions about whether or how that state might change.

[joelanford]

See my other comment about thinking of this more like a mapping of BD status to Operator Ready. Not "is this BD status an error or not". Maybe that helps?

[varshaprasad96]

@joelanford modified the PR based on the review comments.

I ran a few scenarios and have some questions regarding this:

1. Previously, when we were inferring a false status of BD as error, we were also returning the error on the controller, not just updating the status. Currently, the controller does not surface any error when bundleDeployment fails (imo it should) instead just silently only updates the status.
2. When a request is requeued for an Unknown condition, we are running the resolver again which is not necessary and could be an expensive operation by itself. Instead what we actually need, is to just wait for the rukpak to finish the steps for creating bundleDeployment. An option is to have a random backoff with requeueAfter (a fixed interval of time), but I’m not sure if that is still a valid approach to avoid running reconciliation for the same state again.
3. Do we need to requeue when the operator’s Ready status is false if we are not triggering it with a non-nil error? I think yes, because in future there maybe a change in catalogsource’s cache to let’s say include the package we want that changes resolver's input entities. But it again comes back to Q1, where the user will have no idea on why the request is getting requeued again when we do not return a valid error. When we return an error when deppy fails (saying resolution was unsuccessful), shouldn't we also do the same when rukpak fails (which basically means for whatever reason bundleDeployment wasn't successful and bundle is not installed)?

[joelanford]

Currently, the controller does not surface any error when bundleDeployment fails (imo it should) instead just silently only updates the status.

I think this is actually okay. In my mind, if the operator reconcile actually fails in some way, it makes sense to return an error. But if we did everything we were supposed to do successfully, and another component (in this case rukpak) is failing in some way, we can reflect that other component's error in our status, and then just wait to reconcile until we get another event on the the BD, the assumptions being that:

1. If we did everything successfully this time, its pretty likely we'll do everything successfully again if we reconcile immediately again with no other change in state (ergo requeue: true is likely to keep resulting in more requeue: true until some other state actually changes).
2. The other component will likely be the thing returning an error, re-reconciling, and updating the status of the underlying thing. We'll see those status updates and reconcile again based on that.

Instead what we actually need, is to just wait for the rukpak to finish the steps for creating bundleDeployment.

Exactly. The way we wait is to requeue events on the BD (with our Owns(BD) watch) as rukpak progresses on it and updates its status accordingly.

Do we need to requeue when the operator’s Ready status is false if we are not triggering it with a non-nil error? I think yes, because in future there maybe a change in catalogsource’s cache to let’s say include the package we want that changes resolver's input entities.

I think a better approach might be to have a way for the catalogsource cache to signal: "Hey, I've received a change. You should requeue all of the BDs"

[varshaprasad96]

/hold

[joelanford]

I'm thinking ReasonResolutionSucceeded is no longer the right thing. We might need to rename this to something like ReasonInstallSucceeded

[varshaprasad96]

+1! Not sure if we want to do in this PR, but I've created a separate commit with the change of ReasonResolutionSucceeded to ReasonInstallationSucceeded. If we want to remove it out of this PR, I can create a follow-up. We could make another iteration of re-naming condition type and reasons for addressing #126 in a follow-up.

[varshaprasad96]

Leaving it to Unknown instead of False, since an error in converting to typed object does not necessarily mean that install was unsuccessful.

[varshaprasad96]

/hold cancel

[joelanford]

Looking at the tests, I'm inclined to make Unknown map to failed rather than succeeded. But as a follow-up, maybe we need one or more new reason values to handle the unknown cases.

[varshaprasad96]

I'm fine either way, since its either saying that we don't know whether the state is going to successful or not. A better approach would be have a new reason, I can do that in follow up.

[joelanford]

Great work! I've jotted down a few follow-ups we discussed, but I think we should merge this and tackle those separately.

/lgtm

[joelanford]

For follow-up: change this to a non-success reason

[joelanford]

For follow up: add code to handle case where these conditions are out of date (based on comparison of bd.metadata.generation and bd.status.observedGeneration).

[joelanford]

For follow-up: don't leak rukpak names in Operator API (i.e. change this to "InstallationFailed")

[varshaprasad96]

Have created an issue for follow-ups #127. Will have a PR ready for those as soon as this merges.

[awgreene]

/approve
Looks good!