-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Dns: respecting DNS TTL #18408
Dns: respecting DNS TTL #18408
Conversation
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
merged up. PTAL |
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
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, just a few comments, otherwise this LG
@@ -10,6 +10,7 @@ Minor Behavior Changes | |||
*Changes that may cause incompatibilities for some users, but should not for most* | |||
|
|||
* config: the log message for "gRPC config stream closed" now uses the most recent error message, and reports seconds instead of milliseconds for how long the most recent status has been received. | |||
* dns: now respecting the returned DNS TTL for resolved hosts, rather than always relying on the hard-coded :ref:`dns_refresh_rate. <envoy_v3_api_field_config.cluster.v3.Cluster.dns_refresh_rate>` This behavior can be temporarily reverted by setting the runtime guard ``envoy.reloadable_features.use_dns_ttl`` to false. |
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.
two spaces between resolved hosts
envoy/common/backoff_strategy.h
Outdated
/** | ||
* Resets the interval with a (potentially) new starting point. | ||
*/ | ||
virtual void reset(uint64_t base) PURE; |
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.
base -> base_interval?
Also add @param comment?
const MonotonicTime stale_time = stale_at_time_; | ||
return time_source_.monotonicTime() > stale_time; |
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.
How come we don't just compare with stale_at_time_ directly?
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
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 api
@snowp any other requests? |
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!
cc @junr03 @buildbreaker if you see any changes here we can flip the flag back and let me know ASAP |
Changes the DNS cache to respect the advertised TTL, modulo a floor of 5s.
That part of the change is runtime guard. The part which is not, is that the backoff is done on a per-host basis not a global basis, so if one endpoint fails to resolve, it won't result in others backing off, and if one succeeds it won't result in changing backoff for failed hosts.
Risk Level: Medium
Testing: new unit tests
Docs Changes: inline
Release Notes: inline
Optional Runtime guard: envoy.reloadable_features.use_dns_ttl