-
Notifications
You must be signed in to change notification settings - Fork 80
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(cluster): add https req timeout & show time left waiting for healthz #427
Conversation
d1ea1a3
to
4c635e9
Compare
Tested using the following setup with the client machine not going through a bastion or in the same VPC as the cluster.
See video demo below that shows new request timeout and updated information (the video is sped up mid-way for demo purposes). When the update does time out and eventually error, as would be expected in this setup w/o a bastion in use, it does so shortly after the 5 min endpoint test window, versus the 15+min that it's currently taking. https://drive.google.com/file/d/1nerx0eBXMCi1dWrI-gUimmO6nRg-WVRM/view cc @clstokes ^ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good other than the UX comment
4c635e9
to
0d1beec
Compare
@clstokes any feedback? |
0d1beec
to
bd242c1
Compare
Does `reqTimeoutMilliseconds = 1000;` mean it will timeout after 1 second?
Is this _too_ low of a value? How does this behave when deploying from the
U.S. to a region on the other side of the world (or vice versa)?
Otherwise, looks good to me.
…On Thu, Aug 20, 2020 at 12:08 Levi Blackstone ***@***.***> wrote:
***@***.**** approved this pull request.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#427 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACHXA7NBH6BXXOK5HKRVWDSBVYB7ANCNFSM4QEIOAOA>
.
|
yes, 1 second timeout for the HTTP request itself, and 5 seconds in between retries.
Ran tests without
There doesn't seem to be any issue or delays with the endpoint check for local or int'l regions. If the cluster's are inaccessible e.g. no bastion with private access on the cluster, they'd timeout and quit shortly after the 5 min. window. |
bd242c1
to
73db565
Compare
Proposed changes
fix(cluster): add https req timeout & show time left waiting for healthz
Related issues (optional)
Fixes #423