config: change default initial fetch timeout to 15s by ramaraochavali · Pull Request #7571 · envoyproxy/envoy

ramaraochavali · 2019-07-14T08:34:13Z

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

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

ramaraochavali · 2019-07-14T10:34:19Z

@mattklein123 @htuch as discussed changed this. PTAL.

mattklein123

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

mattklein123 · 2019-07-15T16:10:49Z

api/envoy/api/v2/core/config_source.proto

  // 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).


nit: I think you could just say "the default is 15s."

mattklein123 · 2019-07-15T16:11:29Z

api/envoy/api/v2/core/config_source.proto

  // 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;


Should this have a constraint to make this non-zero if it is set?

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?

doc already exists
// 0 means no timeout - Envoy will wait indefinitely for the first xDS config (unless another
// timeout applies)

OK sounds good.

mattklein123 · 2019-07-15T16:12:18Z

docs/root/intro/version_history.rst


 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.


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>

ramaraochavali · 2019-07-16T13:45:05Z

/retest

repokitteh-read-only · 2019-07-16T13:45:08Z

🤷‍♀️ nothing to rebuild.

🐱

Caused by: a #7571 (comment) was created by @ramaraochavali.

see: more, trace.

ramaraochavali · 2019-07-16T14:49:19Z

/retest

repokitteh-read-only · 2019-07-16T14:49:22Z

🤷‍♀️ nothing to rebuild.

🐱

Caused by: a #7571 (comment) was created by @ramaraochavali.

see: more, trace.

mattklein123

LGTM, thanks. I would still like to hear from @htuch on this one.

/azp run envoy-macos

mattklein123 · 2019-07-16T15:47:08Z

/azp run envoy-macos

azure-pipelines · 2019-07-16T15:47:17Z

Azure Pipelines successfully started running 1 pipeline(s).

htuch

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?

mattklein123 · 2019-07-17T04:10:02Z

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?

ramaraochavali · 2019-07-17T04:12:32Z

. 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.

mattklein123 · 2019-07-17T04:14:24Z

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.

htuch · 2019-07-17T20:02:11Z

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.

ramaraochavali · 2019-07-18T04:06:36Z

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>

htuch · 2019-07-19T03:34:18Z

We're good from an API stability perspective, so I'll leave this between you an @mattklein123

htuch

Thanks!

ramaraochavali · 2019-07-19T03:42:41Z

@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>

ramaraochavali · 2019-07-20T05:19:15Z

/azp run envoy-macos

azure-pipelines · 2019-07-20T05:19:19Z

Commenter does not have sufficient privileges for PR 7571 in repo envoyproxy/envoy

mattklein123

I think we can do this without runtime guarding. I will send an email to envoy-announce@ about this though. Thanks @ramaraochavali!

ramaraochavali added 3 commits July 14, 2019 14:01

default initial fetch timeout to 15s

3bc09cc

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

fix format and docs

d20d942

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

fix test case

8ba8a17

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

mattklein123 self-assigned this Jul 15, 2019

mattklein123 requested changes Jul 15, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Jul 15, 2019

mattklein123 assigned htuch Jul 15, 2019

address review comments

425074d

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

repokitteh-read-only bot removed the waiting label Jul 16, 2019

ramaraochavali added 2 commits July 16, 2019 09:48

Merge branch 'master' into fix/default_initial_fetch

de99d6f

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

Merge branch 'master' into fix/default_initial_fetch

cd44bb0

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

mattklein123 previously approved these changes Jul 16, 2019

View reviewed changes

htuch suggested changes Jul 16, 2019

View reviewed changes

htuch added the waiting:any label Jul 16, 2019

repokitteh-read-only bot removed the waiting:any label Jul 17, 2019

ramaraochavali added 2 commits July 18, 2019 09:38

Merge branch 'master' into fix/default_initial_fetch

fdd9225

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

fix version history conflict

7aa822b

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

ramaraochavali dismissed mattklein123’s stale review via 7aa822b July 18, 2019 04:10

ramaraochavali mentioned this pull request Jul 18, 2019

config: do not finish initialization on stream disconnection #7427

Merged

htuch previously approved these changes Jul 19, 2019

View reviewed changes

Merge remote-tracking branch 'upstream/master' into fix/default_initi…

ec2e32c

…al_fetch Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

ramaraochavali dismissed htuch’s stale review via ec2e32c July 20, 2019 03:40

mattklein123 approved these changes Jul 20, 2019

View reviewed changes

mattklein123 merged commit 7e74865 into envoyproxy:master Jul 20, 2019

ramaraochavali deleted the fix/default_initial_fetch branch July 21, 2019 02:07

ramaraochavali mentioned this pull request Dec 4, 2019

xds: undo the revert (#9044) of delta+SotW unification #9189

Closed

zyfjeff mentioned this pull request Jan 10, 2020

Envoy hot-restart doesn't respect drain-time-s and parent-shutdown-time-s #9638

Closed

Conversation

ramaraochavali commented Jul 14, 2019

Uh oh!

ramaraochavali commented Jul 14, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 Jul 15, 2019

Choose a reason for hiding this comment

Uh oh!

mattklein123 Jul 15, 2019

Choose a reason for hiding this comment

Uh oh!

ramaraochavali Jul 16, 2019

Choose a reason for hiding this comment

Uh oh!

ramaraochavali Jul 16, 2019

Choose a reason for hiding this comment

Uh oh!

mattklein123 Jul 16, 2019

Choose a reason for hiding this comment

Uh oh!

mattklein123 Jul 15, 2019

Choose a reason for hiding this comment

Uh oh!

ramaraochavali commented Jul 16, 2019

Uh oh!

repokitteh-read-only bot commented Jul 16, 2019

Uh oh!

ramaraochavali commented Jul 16, 2019

Uh oh!

repokitteh-read-only bot commented Jul 16, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Jul 16, 2019

Uh oh!

azure-pipelines bot commented Jul 16, 2019

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Jul 17, 2019

Uh oh!

ramaraochavali commented Jul 17, 2019

Uh oh!

mattklein123 commented Jul 17, 2019

Uh oh!

htuch commented Jul 17, 2019

Uh oh!

ramaraochavali commented Jul 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

htuch commented Jul 19, 2019

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

ramaraochavali commented Jul 19, 2019

Uh oh!

ramaraochavali commented Jul 20, 2019

Uh oh!

azure-pipelines bot commented Jul 20, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ramaraochavali commented Jul 18, 2019 •

edited

Loading