Add jitter to strict and logical dns clusters #35745
Conversation
|
Hi @Stevenjin8, welcome and thank you for your contribution. We will try to review your Pull Request as quickly as possible. In the meantime, please take a look at the contribution guidelines if you have not done so already. |
Signed-off-by: Steven Jin Xuan <[email protected]>
|
Thanks for adding this feature. |
My guess is yes, but the original issue mentions |
| if (jitter >= ttl_refresh_rate) { | ||
| jitter = std::chrono::milliseconds(0); | ||
| } | ||
| final_refresh_rate = ttl_refresh_rate - jitter; |
There was a problem hiding this comment.
Should we do +- jitter? Or always subtract
There was a problem hiding this comment.
I did subtract so to avoid having expired entries.
Signed-off-by: Steven Jin Xuan <[email protected]>
|
@adisuissa Do you have any tips on mocking the random generator? The problem is that currently, the random generator is a private member that is initialized within the constructor, so I can't find a way to access it or make it a mock. I could change the initializer and A simpler way would be to test that the values affected by the random generator fall within a range. For example, instead of EXPECT_CALL(*resolver.timer_, enableTimer(std::chrono::milliseconds(5000), _));we would have something like EXPECT_CALL(*resolver.timer_, enableTimer(testing::Between(std::chrono::milliseconds(5000 - 512), std::chrono::milliseconds(5000)), _)); |
| @@ -74,6 +77,7 @@ class StrictDnsClusterImpl : public BaseDynamicClusterImpl { | |||
| Network::DnsLookupFamily dns_lookup_family_; | |||
| uint32_t overprovisioning_factor_; | |||
| bool weighted_priority_health_; | |||
| Random::RandomGeneratorImpl random_generator_; | |||
There was a problem hiding this comment.
we should always use the random generator from factory context.
There was a problem hiding this comment.
@wbpcode Where is the random generator from the factory context? I'm looking in cluster_factory.h and I see that it includes random_generator.h, but I don't see any usages of a random generator.
There is one from a a grandparent class, so I think I'll use that. Maybe its the same one.
Signed-off-by: Steven Jin Xuan <[email protected]>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
Signed-off-by: Steven Jin Xuan <[email protected]>
Signed-off-by: Steven Jin Xuan <[email protected]>
Signed-off-by: Steven Jin Xuan <[email protected]>
Signed-off-by: Steven Jin Xuan <[email protected]>
Signed-off-by: Steven Jin Xuan <[email protected]>
Signed-off-by: Steven Jin Xuan <[email protected]>
|
@envoyproxy/senior-maintainers assignee is @mattklein123 |
Co-authored-by: Adi (Suissa) Peleg <[email protected]> Signed-off-by: Steven Jin <[email protected]>
Signed-off-by: Steven Jin Xuan <[email protected]>
Co-authored-by: Adi (Suissa) Peleg <[email protected]> Signed-off-by: Steven Jin <[email protected]>
Signed-off-by: Steven Jin Xuan <[email protected]>
Thank you for the review |
Signed-off-by: Steven Jin <[email protected]>
Stevenjin8
changed the title
Resolves #35641. Adding DNS jitter to resolvers makes it so that envoy doesnt stampede the DNS server when it has multiple entries with the same expiration.
Testing is still WIP. I am open to any suggestions.
Commit Message: dns: add jitter to strict dns
Additional Description:
Risk Level: low
Testing: unit tests
Docs Changes:
Release Notes:
for the :ref:
strict DNS <arch_overview_service_discovery_types_strict_dns>and :ref:logical DNS <arch_overview_service_discovery_types_logical_dns>cluster types,the new :ref:
dns_jitter <envoy_v3_api_field_config.cluster.v3.Cluster.dns_jitter>field, ifprovided, will causes the cluster to refresh DNS entries later by a random amount of time as to
avoid stampedes of DNS requests. This field sets the upper bound (exclusive) for the random amount.
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]