Follow-ups for #125

[varshaprasad96]

#125 enables operator objects to resurface errors when Bundle deployment fails. Some TODOs:

• Introduce a new reason for metav1.Unknown condition: [controller] resurface bundledeployment errors #125 (comment)
• Add code to handle the case when conditions are out of date: [controller] resurface bundledeployment errors #125 (comment)
• Introduce new reason for failure of Bundle Deployment: [controller] resurface bundledeployment errors #125 (comment)

[varshaprasad96]

@joelanford For (2), had a few questions. Comparing bundle deployment's status.ObservervedGeneration and spec.Generation seems to be very fragile. bd.Status.ObserverdGeneration is supposed to be eventually consistent with the spec when it changes. During the first reconcile, there is very high possibility that rukpak is still working on the BD, and status has not been updated yet. This means the BDs observed status generation will always be less than spec.Generation. It eventually becomes consistent, when the bd is created successfully. This is not necessarily the case when BD is out of date, but when it is just getting created.

The scenario when BD is stale and we end up mapping a stale BD condition to operator as in below (copied from our chat) should again eventually be correct (though not immediate) when BD changes its status after rukpak has updated it with the new image.

1. operator is created, bd is installed
2. Operator object or catalog gets updated such that a different image pops out of the resolver
3. Reconcile patches the BD with the new image.
4. The BD already has a status from the initial install.
5. We map that out of date status to the new operator status.

Given this, do we need to handle this case in Operator Controller or Rukpak? Wouldn't it be better if rukpak changes the BD status when it patches it with a new image (with a new condition?) so that we can stamp it in Operator status, than us comparing the status and spec generations which could be different for any other reason?

[joelanford]

During the first reconcile, there is very high possibility that rukpak is still working on the BD, and status has not been updated yet.

Yep, agreed -- except its a 100% chance. During the very first reconcile of an Operator object, its a guarantee that the BD we are looking at after we apply the SSA patch will not have a status. The data populated into the BD by that apply call by the API server is literally just whatever the result of admission webhooks + apiserver metadata changes (uid, generation, resourceVersion, etc).

This means the BDs observed status generation will always be less than spec.Generation ... This is not necessarily the case when BD is out of date.

Its always the case when we actually apply the patch. That patch could be an initial create or a subsequent update. When we apply a patch to the BD, we will never know the updated status of that BD during that same reconcile call (unless we go out of our way to block and wait for it to be updated by rukpak, which would be a big no-no).

Given this, do we need to handle this case in Operator Controller or Rukpak? Wouldn't it be better if rukpak changes the BD status when it patches it with a new image (with a new condition?) so that we can stamp it in Operator status, than us comparing the status and spec generations which could be different for any other reason?

There are probably various approaches to how to handle this, but at the end of the day what we must make sure NOT to do is:

1. Apply a patch to the BD
2. Compute an Operator status based on the content of the BD that is updated by the API server as a result of the patch call. The status of the BD object will ALWAYS be out-of-date.

I think a simple solution is probably along the lines of:

1. Is status.observedGeneration == spec.generation? If so, compute the Operator status based on the BD status, which we know is up-to-date.
2. Else (status is not up-to-date), Ready: False, "waiting for bundle deployment to be reconciled"

Over time I could see that getting more sophisticated (see #126), specifically:

I think we'll also need to consider various other condition types, either here, in rukpak, or both. Another obvious one to me is "Progressing". Combining Progressing and Installed could result in the following useful combinations:

1. Progressing = True, Installed = True - Previous state is installed, but we're progressing to the desired state
2. Progressing = True, Installed = False - Previous installation failed (or this is the initial install), but we're progressing to the desired state.
3. Progressing = False, Installed = True - Steady state, actual state matches desired state
4. Progressing = False, Installed = False - Maybe not possible, or maybe a failure that we know we can't progress forward unless current state changes in same way (e.g. change of desired state, or a new catalog shows up that resolves the issue).

When we get to this point, we may need the operator reconcile to be able to say "there is an out-of-date thing that's up and running, but we're progressing to something else"