upstream: allow clusters to skip waiting on warmup for initialization by junr03 · Pull Request #17179 · envoyproxy/envoy

junr03 · 2021-06-28T23:52:34Z

Commit Message: upstream -- allow clusters to skip waiting on warmup for initialization
Additional Description: this reduces server startup time, but might cause initial 500s for requests made on unresolved clusters. Further work might be needed there if operators use this new config option.
Risk Level: low - new config option. Default uses legacy behavior.
Testing: added unit tests
Docs Changes: added.
Release Notes: updated

Signed-off-by: Jose Nino <jnino@lyft.com>

repokitteh-read-only · 2021-06-28T23:52:40Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
envoyproxy/api-shepherds assignee is @markdroth
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #17179 was opened by junr03.

see: more, trace.

junr03 · 2021-06-28T23:52:51Z

api/envoy/config/cluster/v3/cluster.proto

+  // or :ref:`LOGICAL_DNS<envoy_v3_api_enum_value_config.cluster.v3.Cluster.DiscoveryType.LOGICAL_DNS>`.
+  // If true, server startup blocks on async DNS resolution. If false, server will complete
+  // initialization whether or not DNS resolution has completed. Defaults to true.
+  google.protobuf.BoolValue wait_for_dns_on_init = 54;


open to a different name.

how about wait_for_warm_on_init or some thing like that so that it can be applied for EDS/other type of clusters if needed in future and need not be dns cluster specific?

+1, I think this seems like a useful general option and we could just have it apply everywhere.

Great point. Will update. I can can land this PR for only dns based clusters, and if there is appetite I can do a subsequent one for other cluster types.

Signed-off-by: Jose Nino <jnino@lyft.com>

mattklein123

Thanks LGTM with some small comments.

/wait

mattklein123 · 2021-06-30T14:56:39Z

api/envoy/config/cluster/v3/cluster.proto

  // DNS resolution configuration which includes the underlying dns resolver addresses and options.
  core.v3.DnsResolutionConfig dns_resolution_config = 53;

+  // Optional configuration for having server init block on cluster warm-up. Currently, only applicable for


It's not only "server init" that this applies to. In practice it would also apply to CDS updates. I would make that more clear here and below.

Agreed. Let me know if this is better.

mattklein123 · 2021-06-30T14:57:48Z

docs/root/version_history/current.rst

 * bootstrap: added :ref:`dns_resolution_config <envoy_v3_api_field_config.bootstrap.v3.Bootstrap.dns_resolution_config>` to aggregate all of the DNS resolver configuration in a single message. By setting one such configuration option ``no_default_search_domain`` as true the DNS resolver will not use the default search domains. And by setting the configuration ``resolvers`` we can specify the external DNS servers to be used for external DNS query.
 * cluster: added :ref:`dns_resolution_config <envoy_v3_api_field_config.cluster.v3.Cluster.dns_resolution_config>` to aggregate all of the DNS resolver configuration in a single message. By setting one such configuration option ``no_default_search_domain`` as true the DNS resolver will not use the default search domains.
 * cluster: added :ref:`host_rewrite_literal <envoy_v3_api_field_config.route.v3.WeightedCluster.ClusterWeight.host_rewrite_literal>` to WeightedCluster.
+* cluster: added :ref:`wait_for_warm_on_init <envoy_v3_api_field_config.cluster.v3.Cluster.wait_for_warm_on_init>`, which allows clusters to not-block server initialization on warmup. It is true by default, which preserves existing behavior. Currently, only applicable for DNS-based clusters.


Same comment here about server init.

mattklein123 · 2021-06-30T14:58:10Z

source/common/upstream/logical_dns_cluster.cc

      respect_dns_ttl_(cluster.respect_dns_ttl()),
      resolve_timer_(
          factory_context.dispatcher().createTimer([this]() -> void { startResolve(); })),
+      wait_for_warm_on_init_(PROTOBUF_GET_WRAPPED_OR_DEFAULT(cluster, wait_for_warm_on_init, true)),


I would move this into the cluster base class, even if not yet implemented for the other clusters.

markdroth · 2021-07-01T15:21:36Z

(Matt, since you're reviewing this one, I'll remove myself as API reviewer.)

Signed-off-by: Jose Nino <jnino@lyft.com>

mattklein123

LGTM, thanks!

junr03 · 2021-07-02T17:09:31Z

/retest

repokitteh-read-only · 2021-07-02T17:09:34Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #17179 (comment) was created by @junr03.

see: more, trace.

junr03 · 2021-07-02T20:47:52Z

/retest

repokitteh-read-only · 2021-07-02T20:47:55Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #17179 (comment) was created by @junr03.

see: more, trace.

* main: listener: match rebalancer to listener IP family type (envoyproxy#16914) jwt_authn: implementation of www-authenticate header (envoyproxy#16216) listener: reset the file event in framework instead of listener filter doing itself (envoyproxy#17227) Small typo fix (envoyproxy#17247) Doc: Clarify request/response attributes are http-only (envoyproxy#17204) bazel/README.md: add aspell comment (envoyproxy#17072) docs: Fix broken URL links in HTTP upgrades doc (envoyproxy#17225) remove the wrong comment on test (envoyproxy#17233) upstream: allow clusters to skip waiting on warmup for initialization (envoyproxy#17179) JwtAuthn: support completing padding on forward jwt payload header (envoyproxy#16752) remove support for v2 UNSUPPORTED_REST_LEGACY (envoyproxy#16968) metrics service: fix wrong argument arrange on MetricsServiceSink (envoyproxy#17127) deps: update cel-cpp to 0.6.1 (envoyproxy#16293) Add ability to filter ConfigDump. (envoyproxy#16774) examples: fix Wasm example. (envoyproxy#17218) upstream: update host's socket factory when metadata is updated. (envoyproxy#16708) Signed-off-by: Garrett Bourg <bourg@squareup.com>

…envoyproxy#17179) Commit Message: upstream -- allow clusters to skip waiting on warmup for initialization Additional Description: this reduces server startup time, but might cause initial 500s for requests made on unresolved clusters. Further work might be needed there if operators use this new config option. Risk Level: low - new config option. Default uses legacy behavior. Testing: added unit tests Docs Changes: added. Release Notes: updated Signed-off-by: Jose Nino <jnino@lyft.com> Signed-off-by: chris.xin <xinchuantao@qq.com>

…envoyproxy#17179) Commit Message: upstream -- allow clusters to skip waiting on warmup for initialization Additional Description: this reduces server startup time, but might cause initial 500s for requests made on unresolved clusters. Further work might be needed there if operators use this new config option. Risk Level: low - new config option. Default uses legacy behavior. Testing: added unit tests Docs Changes: added. Release Notes: updated Signed-off-by: Jose Nino <jnino@lyft.com>

Jose Nino added 3 commits June 28, 2021 12:46

api change

e6ac3c9

Signed-off-by: Jose Nino <jnino@lyft.com>

implementation

2e072c7

Signed-off-by: Jose Nino <jnino@lyft.com>

release notes

f1840e3

Signed-off-by: Jose Nino <jnino@lyft.com>

repokitteh-read-only bot added the api label Jun 28, 2021

repokitteh-read-only bot assigned markdroth Jun 28, 2021

junr03 commented Jun 28, 2021

View reviewed changes

junr03 assigned snowp and mattklein123 and unassigned snowp Jun 28, 2021

mattklein123 added the waiting label Jun 29, 2021

Jose Nino added 2 commits June 29, 2021 15:02

update

00f2d17

Signed-off-by: Jose Nino <jnino@lyft.com>

update

7119f49

Signed-off-by: Jose Nino <jnino@lyft.com>

repokitteh-read-only bot removed the waiting label Jun 29, 2021

fmt

41cfa6d

Signed-off-by: Jose Nino <jnino@lyft.com>

mattklein123 requested changes Jun 30, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Jun 30, 2021

markdroth removed their assignment Jul 1, 2021

update

3ded32e

Signed-off-by: Jose Nino <jnino@lyft.com>

repokitteh-read-only bot removed the waiting label Jul 1, 2021

mattklein123 approved these changes Jul 2, 2021

View reviewed changes

repokitteh-read-only bot removed the api label Jul 2, 2021

junr03 changed the title ~~server init: allow server init to finish without initial resolution for DNS based clusters~~ upstream: allow clusters to skip waiting on warmup for initialization Jul 2, 2021

junr03 merged commit f2906f5 into envoyproxy:main Jul 2, 2021

junr03 mentioned this pull request Jul 2, 2021

update envoy: f2906f5ce37281 & skip waiting on stats cluster warmup envoyproxy/envoy-mobile#1568

Merged

adisuissa mentioned this pull request Feb 27, 2024

Support WaitForWarmOnInit for EDS clusters #32531

Closed

Conversation

junr03 commented Jun 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Jun 28, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markdroth commented Jul 1, 2021

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

junr03 commented Jul 2, 2021

Uh oh!

repokitteh-read-only bot commented Jul 2, 2021

Uh oh!

junr03 commented Jul 2, 2021

Uh oh!

repokitteh-read-only bot commented Jul 2, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

junr03 commented Jun 28, 2021 •

edited

Loading