Add dial timeout field to hosts toml configuration #11106
Conversation
|
Hi @phillebaba. Thanks for your PR. I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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-sigs/prow repository. |
c1c9e61 to
e9b1458
Compare
docs/hosts.md
Outdated
| ## dial_timeout field | ||
|
|
||
| `dial_timeout` is the maximum amount of time a dial will wait for | ||
| a connect to complete. |
There was a problem hiding this comment.
Can you be clear what the default the is and the use case for this. I assume the primary use case for this is to make it much much shorter for some mirrors
There was a problem hiding this comment.
Updated the description with the default and some more information about the use case.
| if host.dialTimeout != nil { | ||
| tr.DialContext = (&net.Dialer{ | ||
| Timeout: *host.dialTimeout, | ||
| KeepAlive: 30 * time.Second, | ||
| FallbackDelay: 300 * time.Millisecond, | ||
| }).DialContext | ||
| } |
There was a problem hiding this comment.
This will only take effect if line 184 is true
There was a problem hiding this comment.
Also, we should set the current default value(30 sec) if host.dialTimeout has not been set.
There was a problem hiding this comment.
Fixed this by adding a new condition which I thought I already had.
The default 30 seconds is set in the default transport which we clone. So if the dial timeout is not set it will be the default value.
|
@phillebaba Thanks for your PR. If you are busy, I'm happy to carry this PR. |
74c7668 to
8b5ae93
Compare
|
@utam0k sorry I was taking some time off during the holidays so have not looked at any of the review comments. I have now update the PR to address the review comments. |
8b5ae93 to
702c496
Compare
|
@dmcgowan @djdongjin Could we merge this PR? PTAL 🙏 |
|
@dmcgowan There seems to be a new release on February 13, so how about including that? |
@utam0k just to clarify this is just my pure guess based on previous release cadence as I'm not a maintainer :) but I feel this PR is in good shape. |
|
@djdongjin Ops, I missed. You are right. |
|
@dmcgowan cc: @AkihiroSuda |
|
@phillebaba can you rebase this PR to fix the CI. I think it's because your PR is based on a commit before 47c4dba (which removed |
Signed-off-by: Philip Laine <philip.laine@gmail.com>
702c496 to
c4982bf
Compare
[release/2.0] Backport Add dial timeout field to hosts toml configuration #11106
This change adds a dial timeout field to the hosts toml configuration to reduce the impact of a registry which is town or unable to connect to.
Fixes #10660