Adjust the Endpoints Informer events. #2779
Conversation
There was a superfluous Endpoints event registered, which is leftover from the single-tenant Broker. There was a missing global resync due to changes in the shared Endpoints, which prevents Brokers from becoming ready if they are reconciled before the shared deployment have become ready.
knative-prow-robot
added
the
size/M
|
The following is the coverage report on the affected files.
|
|
/retest |
1 similar comment
|
/retest |
|
o |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mattmoor, n3wscott 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:
Approvers can indicate their approval by writing |
knative-prow-robot
added
the
approved
|
/retest |
| @@ -109,5 +106,29 @@ func NewController( | |||
| }, | |||
| )) | |||
|
|
|||
| // When the endpoints in our multi-tenant filter/ingress change, do a global resync. | |||
| // During installation, we might reconcile Brokers before our shared filter/ingress is | |||
| // ready, so when these endpoints change perform a global resync. | |||
There was a problem hiding this comment.
Why do we need a global resync instead of just enqueuing brokers? The way I read this "installation..." part is that even after reaching a steady state, if endpoints change, the brokers wouldn't be enqueued? Assuming that all the Brokers are using MT based one, this is fine since they would all need to get enqueued anyways, but just curious why simply enqueing affected brokers is not enough?
Seems like we should enqueue all Brokers that have the MTChannelBasedBroker annotation only instead of all the Brokers?
There was a problem hiding this comment.
All the global resync does is enqueue brokers. This is a filtered global resync, so it will only enqueue the MT brokers.
re: affected brokers
All of the MT brokers are affected by changes to the MT endpoint. 1->5 may not affect readiness, but 1->0 does (e.g. if the single pod crashes or is evicted) as would 0->1 (what I see)
|
/retest |
1 similar comment
|
/retest |
There was a superfluous Endpoints event registered, which is leftover from the single-tenant Broker.
There was a missing global resync due to changes in the shared Endpoints, which prevents Brokers from becoming ready if they are reconciled before the shared deployment have become ready.
Note: this logic is copied from similar logic in the Serving SKS controller, where we global resync on changes to the activator service.