-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
config: change default initial fetch timeout to 15s #7571
config: change default initial fetch timeout to 15s #7571
Conversation
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
@mattklein123 @htuch as discussed changed this. PTAL. |
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.
Thanks, at a high level I think this is a good change. I have some small comments. I would also like @htuch to take a look.
/wait
// When this timeout is specified, Envoy will wait no longer than the specified time for first | ||
// config response on this xDS subscription during the :ref:`initialization process | ||
// <arch_overview_initialization>`. After reaching the timeout, Envoy will move to the next | ||
// initialization phase, even if the first config is not delivered yet. The timer is activated | ||
// when the xDS API subscription starts, and is disarmed on first config update or on error. 0 | ||
// means no timeout - Envoy will wait indefinitely for the first xDS config (unless another | ||
// timeout applies). Default 0. | ||
// timeout applies). If not specified the default is 15000ms (15 seconds). |
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.
nit: I think you could just say "the default is 15s."
// When this timeout is specified, Envoy will wait no longer than the specified time for first | ||
// config response on this xDS subscription during the :ref:`initialization process | ||
// <arch_overview_initialization>`. After reaching the timeout, Envoy will move to the next | ||
// initialization phase, even if the first config is not delivered yet. The timer is activated | ||
// when the xDS API subscription starts, and is disarmed on first config update or on error. 0 | ||
// means no timeout - Envoy will wait indefinitely for the first xDS config (unless another | ||
// timeout applies). Default 0. | ||
// timeout applies). If not specified the default is 15000ms (15 seconds). | ||
google.protobuf.Duration initial_fetch_timeout = 4; |
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.
Should this have a constraint to make this non-zero if it is set?
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.
I was thinking about it, but decided against it because it may allow people get back old behaviour it they need. I was thinking of updating the doc with that info. WDYT?
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.
doc already exists
// 0 means no timeout - Envoy will wait indefinitely for the first xDS config (unless another
// timeout applies)
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.
OK sounds good.
docs/root/intro/version_history.rst
Outdated
@@ -3,6 +3,7 @@ Version history | |||
|
|||
1.12.0 (pending) | |||
================ | |||
* config: changed the default value of :ref:`initial_fetch_timeout <envoy_api_field_core.ConfigSource.initial_fetch_timeout>` from 0s to 15s. This is a change in behaviour in the sense that Envoy initialization always completes within 15s by default. Refer to :ref:`initialization process <arch_overview_initialization>` for more details. |
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.
I don't think it's true that Envoy will always complete init within 15s, given that other things have to happen like HC, etc. Can you rephrase?
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
/retest |
🤷♀️ nothing to rebuild. |
/retest |
🤷♀️ nothing to rebuild. |
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.
LGTM, thanks. I would still like to hear from @htuch on this one.
/azp run envoy-macos
/azp run envoy-macos |
Azure Pipelines successfully started running 1 pipeline(s). |
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.
I think it's reasonable to set a default like this, but I wonder if this counts as a breaking API change; we're totally changing the behavior of Envoy initialization within a major API version?
I suppose it's a breaking change, but IMO it's for the better so FWIW I think it's OK. If we are concerned perhaps we should runtime guard it? |
I agree with you that it is better change. Since the old behaviour is still supported with the same config, do we still want to guard with runtime? I am assuming runtime enables this by default. |
My preference would be to enable this by default, the runtime would just store the default which would allow people to revert back to the previous behavior. Let's see what @htuch thinks. |
Probably worth runtime guarding, since it will allow Envoy global reversion to the old behavior. I'm thinking through how this relates to #6271. Reading through the document, I think we decided that it doesn't matter what we set or change defaults for wrapped types to, since a management server that actually cares should set fields rather than relying on defaults. So, I think we are safe from a stable versioning perspective here. |
If we are safe from stable versioning perspective - I prefer not to introduce the runtime guard as it introduces another level of config for this field. From operational perspective would runtime provide better way to revert - some how it needs to be set for all running envoys right? |
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
We're good from an API stability perspective, so I'll leave this between you an @mattklein123 |
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.
Thanks!
@htuch Thank you. @mattklein123 WDYT? should we runtime guard this given the default value we set here is more sensible? |
…al_fetch Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
/azp run envoy-macos |
Commenter does not have sufficient privileges for PR 7571 in repo envoyproxy/envoy |
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.
I think we can do this without runtime guarding. I will send an email to envoy-announce@ about this though. Thanks @ramaraochavali!
Description: As discussed in PR #7427, changing the default initial fetch time out to 15s.
Risk Level: Low
Testing: Changed
Docs Changes: Updated
Release Notes: Added