Always call updateStatus after Reconcile.#1321
Always call updateStatus after Reconcile.#1321google-prow-robot merged 2 commits intoknative:masterfrom
Conversation
google-prow-robot
added
approved
dprotaso
left a comment
dprotaso
left a comment
There was a problem hiding this comment.
Curious there's an opportunity to further refactor some updateStatus calls when we receive events about builds, deployments, endpoints etc. Essentially each controller's Sync* calls
| // updates regardless of whether the reconciliation errored out. | ||
| err = c.reconcile(ctx, config) | ||
| if _, err := c.updateStatus(config); err != nil { | ||
| return err |
There was a problem hiding this comment.
Any reason why you removed logging when calling updateStatus fails
updateStatus function has no logging
| // updates regardless of whether the reconciliation errored out. | ||
| err = c.reconcile(ctx, rev) | ||
| if _, err := c.updateStatus(rev); err != nil { | ||
| return err |
There was a problem hiding this comment.
likewise the revision's updateStatus doesn't log any errors
| // updates regardless of whether the reconciliation errored out. | ||
| err = c.reconcile(ctx, route) | ||
| if _, err := c.updateStatus(ctx, route); err != nil { | ||
| return err |
There was a problem hiding this comment.
route's updateStatus has logging so this is great
| } | ||
|
|
||
| // Reflect any status changes that occurred during reconciliation. | ||
| if _, err := c.updateStatus(config); err != nil { |
There was a problem hiding this comment.
Curious what you think about not splitting these methods but having this as a top level defer call
There was a problem hiding this comment.
I thought about this, but this felt clearer (less magical) to me. One thing that makes hard is returning an error on updateStatus (you need to use named return values).
LMK if you feel strongly (the naming could also be more original, but nothing else felt appreciably better).
There was a problem hiding this comment.
I don't feel strongly - was just wondering if you considered it
mattmoor
left a comment
mattmoor
left a comment
There was a problem hiding this comment.
Curious there's an opportunity to further refactor some updateStatus calls when we receive events about builds, deployments, endpoints etc. Essentially each controller's Sync* calls
Yes, but those are destined to be cleaned up in the move to more level-based reconciliation, so I'm more reluctant to change them.
| // updates regardless of whether the reconciliation errored out. | ||
| err = c.reconcile(ctx, rev) | ||
| if _, err := c.updateStatus(rev); err != nil { | ||
| return err |
| // updates regardless of whether the reconciliation errored out. | ||
| err = c.reconcile(ctx, config) | ||
| if _, err := c.updateStatus(config); err != nil { | ||
| return err |
This splits the main `Reconcile` methods of the controllers, so that regardless of whether they return an error we can call `updateStatus` with any changes made to the object being reconciled regardless of the success or failure of the reconciliation. This also removes all of the `updateStatus` calls from the various methods with the exception of the methods called directly by the informer. Related: knative/serving#1107
|
Fixing failures due to merge conflicts... |
|
Ok, should be RFAL. |
|
The following is the coverage report on pkg/. Say
*TestCoverage feature is being tested, do not rely on any info here yet |
|
/test pull-knative-serving-unit-tests |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dprotaso, mattmoor The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
5 participants
This splits the main
Reconcilemethods of the controllers, so that regardless of whether they return an error we can callupdateStatuswith any changes made to the object being reconciled regardless of the success or failure of the reconciliation. This also removes all of theupdateStatuscalls from the various methods with the exception of the methods called directly by the informer.Eventually when all of the actual reconciliation is performed by
Reconcile, these should be the only calls toupdateStatusand we could conceivably inline them (defensively to avoid people adding new calls in violation of this model.Related: #1107