(catsrc) set status reason/message on incorrect polling interval

[anik120]

This PR sets the status reason as InvalidIntervalError and a status message
if updateStrategy.RegistryPoll.ParsingError is set for the catsrc.

Description of the change:

Motivation for the change:

Reviewer Checklist

• Implementation matches the proposed design, or proposal is updated to match implementation
• Sufficient unit test coverage
• Sufficient end-to-end test coverage
• Docs updated or added to /doc
• Commit messages sensible and descriptive

[anik120]

/hold for operator-framework/api#169 to merge

[exdx]

is this UpdateStatus call required every catalogsource reconciliation loop if the ParsingError is not blank? Maybe we should only do the status update once? Is there any way to check if that status is set before attempting the update call?

[anik120]

Added a check here before going through with the update.

[exdx]

Maybe someone can clarify, but I'm not sure that we want to always flip the status.reason to CatalogSourceIntervalInvalidError from another status in the case where a parsing error was found on the catalog. This would cause us to stomp our other catalog status updates and potentially constantly update the status.

I think what we want is to provide some kind of long-lived indication that the catalog did have a invalid interval at some point (when it was created), which was changed to the default, with a message indicating what happened. WDYT about using a condition on the status instead of updating the status.Reason directly?

[anik120]

@exdx so

Good news: The way this is currently implemented, there's no chance of us stomping over a previously set reason. Because every single time we've called setError before this, it has been followed by return. So only when that error is resolved, do we move on to the next step, and the next, etc, until we reach the final segment where we check for the polling error and setError again if there is an error. Only this time, we don't return.

Bad news: It does make sense to set a condition for this actually now that you bring it up. BUT, when I set out to implement that, I realized that it'd need a change in api again. Here's a rough implementation anik120@e134397
This is definitely much more cleaner, but now that we've released an of/api version with the existing changes, do these changes translate to breaking changes? cc: @benluddy @joelanford

The silver lining is, with the current implementation, when the CatalogSource is edited to fix the error code, the reason is reset, so any future insertion (if any) of setError after these lines will have to worry about stomping over the message this PR is setting, but it seems unlikely we'll add anymore setErrors after these lines 🤷🏽

[exdx]

I see what you're saying, since we are checking the polling error in this final sequence of events, it may not necessarily be overwritten. But if some other condition earlier in the chain is met, we update the status, and then when it's resolved we will update here. This particular status (invalid polling interval) doesn't feel like a level-based error, if that makes sense. I'm still a little worried about the potential for hotlooping

Do we ever clear the spec.UpdateStrategy.RegistryPoll.ParsingError field? Because it feels like strange behavior to always update the status to point to an invalid catalog error, even if the polling error has been resolved (by defaulting) and the catalog is no longer invalid.

Also, I believe we can simply set

syncError = fmt.Errorf(out.Spec.UpdateStrategy.RegistryPoll.ParsingError)
out.SetError(v1alpha1.CatalogSourceIntervalInvalidError, syncError)
return

in the code and not have to call UpdateStatus() directly, the control loop will update the catalog for us

[anik120]

@exdx so the spec.UpdateStrategy.RegistryPoll.ParsingError does clear when we unmarshall the incoming object here. I added a test to verify that https://github.com/operator-framework/operator-lifecycle-manager/pull/2447/files#diff-65a3cf2eeb27e70487e6159bd540e1d057e3bc79bea13e4c874a147d6f147469R1195

[anik120]

I'm guessing if this test passes that'll take care of the hotlooping concerns too.

[anik120]

/hold cancel

[exdx]

/lgtm

[exdx]

/lgtm

[dinhxuanvu]

/approve

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: anik120, dinhxuanvu

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [dinhxuanvu]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment