gcp: document gcp-routes service interdependency#3512
gcp: document gcp-routes service interdependency#3512sttts wants to merge 1 commit intoopenshift:masterfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@sttts: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
| resource "google_compute_health_check" "api_internal" { | ||
| name = "${var.cluster_id}-api-internal" | ||
|
|
||
| // CAUTION: the gcp-routes mechanism must be _faster_ than this value: |
There was a problem hiding this comment.
Nit,
Can you maybe add docs/dev/gcp readme that details this and point to that file here?
Otherwise this causes noise here...
There was a problem hiding this comment.
I can put the link into the readme. But I definitely want to keep this warning here. It's too easy to change this and break all the sensitive timing.
There was a problem hiding this comment.
I can put the link into the readme. But I definitely want to keep this warning here. It's too easy to change this and break all the sensitive timing.
// IMPORTANT: <link>
seems like good enough for approvers and reviewers
There was a problem hiding this comment.
I do what you suggest. But for the record, I don't think it is clever to move documentation into the readme.
There was a problem hiding this comment.
There is no readme yet. Feel free to take over this PR and create it. For me this comment is more than adequat.
| // local node in 1/3 of cases, which is blackholed (due to missing | ||
| // hairpinning support). | ||
| healthy_threshold = 3 | ||
| // CAUTION: the gcp-routes mechanism must be _slower_ than this value: |
There was a problem hiding this comment.
Hrm. If both healthy_threshold and unhealthy_threshold are 3, how can the former be faster than the gcp-routes mechanism while the latter is slower than that same gcp-routes mechanism?
There was a problem hiding this comment.
You assume that the gcp-routes mechanism is symmetric. It isn't:
There was a problem hiding this comment.
So in theory:
3*2s(LB noticing green readyz) >1*2s(gcp-routes noticing green readyz)3*2s(LB noticing red readyz) <8*2s(gcp-routes noticing red readyz)
There was a problem hiding this comment.
But this might be an issue, cc @squeed:
gcp-routes only polls every 5s. So 1*2s could become 1*2s+5s which is bigger than 3*2s.
|
Closing this as it has stalled for very long. Please feel free to re-open in case you intent to keep working on it, or consider tracking the work in an issue if this requires agreement for implementation. /close |
|
@abhinavdahiya: Closed this PR. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
4 participants
The readiness probes for the internal gcp LBs must match the gcp-routes timing values.
Today:
gcp LB: 6-8s to notice health, 6-8s to notice failure (plus some unknown time to reconcile the LB)
gcp-routes: 10-15s to notice health. 50-55s to notice failure (plus 1-2s to reconcile iptables), compare:
https://github.com/openshift/machine-config-operator/blob/cecd8348f1d9da2ef5988dca4d6d3efbedebcd4b/cmd/gcp-routes-controller/run.go#L90
Note: the latter is wrong (too slow) for the health case. To be fixed in MCO in a follow-up.