mark apiservices without reachable discovery as bad #14595
Conversation
|
[test] |
| req.WithContext(ctx) | ||
| resp, err := httpClient.Do(req) | ||
| if err != nil { | ||
| availableCondition.Status = apiregistration.ConditionFalse |
There was a problem hiding this comment.
what's the retry behaviour if this fails?
There was a problem hiding this comment.
what's the retry behaviour if this fails?
It will retry
- when the apiservice resyncs
- when the service changes
- when the endpoints change
We can add a separate timer if you like, but those ought not take too long.
There was a problem hiding this comment.
30min or more is long, 5min would be ok IMO.
There was a problem hiding this comment.
30min or more is long, 5min would be ok IMO.
Resync is 5 minutes.
There was a problem hiding this comment.
Perfect. A comment here wouldn't harm.
| @@ -194,6 +223,28 @@ func (c *AvailableConditionController) sync(key string) error { | |||
| return err | |||
| } | |||
|
|
|||
| func getDestinationHost(apiService *apiregistration.APIService, serviceLister v1listers.ServiceLister) string { | |||
There was a problem hiding this comment.
how will this look like in 3.7? We get Walter's dialer code, right? Maybe add a Rebase 3.7 note here.
There was a problem hiding this comment.
how will this look like in 3.7? We get Walter's dialer code, right? Maybe add a Rebase 3.7 note here.
|
Comments added. [merge][severity:blocker] (edited by liggitt to add blocker) |
|
Evaluated for origin merge up to 48d3654 |
|
[test] |
|
Evaluated for origin test up to 48d3654 |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/2250/) (Base Commit: 811a267) |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/1006/) (Base Commit: ec11c0d) (Extended Tests: blocker) (Image: devenv-rhel7_6361) |
xref #14500
The upstream change for the discovery endpoint check doesn't match the upstream pull, but the spirit is the same.
@jwforres @bparees this was enough for me to restart successfully, though I haven't ruled out races. I think I may need to add some of the healthz protections into the openshift all in one.