c-ares: make qcache_max_ttl configurable by andy-fong · Pull Request #45073 · envoyproxy/envoy

andy-fong · 2026-05-14T17:09:02Z

Commit Message: c-ares: make qcache_max_ttl configurable
Additional Description:
expose the qcache_max_ttl setting and share DNSResolver if cares config is the same so the qcache can be shared. runtime guard "envoy.restart_features.shared_cares_dns_resolver" is set to true by default to enable sharing DNSResolver if cares config of the clusters are the same. Set to false to disable this behavior.

AI is used to generate the tests and write some of the comment but I did review and tune/modify all the tests added. All other code are hand written.

Risk Level: Low
Testing: unit test and manual test using tcpdump to verify multiple clusters with the same host (different port) only generate 1 dns lookup most of the time.
Docs Changes: None
Release Notes: Added qcache_max_ttl field to CaresDnsResolverConfig
Platform Specific Features: None
[Optional Runtime guard:] "envoy.restart_features.shared_cares_dns_resolver" is set to true by default to enable sharing DNSResolver if cares config of the clusters are the same. Set to false to disable this behavior.

Signed-off-by: Andy Fong <andy.fong@solo.io>

repokitteh-read-only · 2026-05-14T17:09:15Z

CC @envoyproxy/runtime-guard-changes: FYI only for changes made to (source/common/runtime/runtime_features.cc).
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @markdroth
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #45073 was opened by andy-fong.

see: more, trace.

andy-fong · 2026-05-14T17:17:57Z

There are some past issues in the past that are closed as not planned related to this problem I am trying to solve:

While the correct solution is probably to deduplicate the DNS lookups across clusters or add native DNS cache. They are probably too big of an effort. I know DNS cache is available in DFP but does not look like it can be easily applied to the DNS lookup used by DNS clusters. There is also the new Hickory DNS resolver that seems to support caching but it's still new and I have not tried that. This is kind of a middle of the ground approach to reduce the DNS lookup from envoy in a large env. It's not ideal that I put the resolver_map_ under the DNSResolver class but only really c-ares impl uses that but that's the simplest route without a lot of changes. Open to any suggestion.

…tl-main Signed-off-by: Andy Fong <andy.fong@solo.io>

kyessenov · 2026-05-15T17:16:44Z

cc @yanavlasov

markdroth · 2026-05-19T21:29:46Z

/lgtm api

tyxia · 2026-05-22T02:47:57Z

/assign @yanavlasov

…tl-main Signed-off-by: Andy Fong <andy.fong@solo.io>

andy-fong · 2026-05-28T14:50:19Z

random tests or download issue keep failing the precheck. I just use the opportunity to merge with main to kick the tests.

andy-fong · 2026-05-28T17:48:08Z

Latest fail is a segfault:

FAIL: //test/extensions/filters/http/ext_proc:ext_proc_integration_test (shard 4 of 8) (Segmentation fault) (see /build/bazel_root/base/execroot/envoy/bazel-out/k8-opt/testlogs/test/extensions/filters/http/ext_proc/ext_proc_integration_test/shard_4_of_8/test.log)

not completely sure it's related to this change.

andy-fong · 2026-05-28T22:47:10Z

ext_proc_integration_test

This test runs fine locally.

andy-fong · 2026-05-28T22:47:29Z

/retest

andy-fong · 2026-05-29T15:55:54Z

All tests passed for the first time!

yanavlasov · 2026-05-29T20:17:25Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces a configurable maximum TTL (qcache_max_ttl) for the c-ares DNS resolver cache and implements sharing of DNSResolver instances with identical configurations, controlled by a runtime feature flag. The reviewer's feedback highlights critical thread-safety issues, noting that the shared resolver_map_ in CaresDnsResolverFactory is accessed concurrently without synchronization and should be guarded by mutex_. Additionally, the reviewer points out unused dead code in DnsClusterFactory and suggests simplifying a helper function in the test suite using idiomatic Protobuf APIs.

gemini-code-assist · 2026-05-29T20:19:44Z

+    auto resolver = std::make_shared<Network::DnsResolverImpl>(
+        cares, dispatcher, csv_or_error.value(), api.rootScope());
+    if (Runtime::runtimeFeatureEnabled("envoy.restart_features.shared_cares_dns_resolver")) {
+      // clean up any nil resolver in the map so it doesn't keep growing
+      auto original_size = resolver_map_.size();
+      absl::erase_if(
+          resolver_map_,
+          [](const std::pair<const std::size_t, std::weak_ptr<Network::DnsResolver>>& entry) {
+            return entry.second.lock() == nullptr;
+          });
+      if (resolver_map_.size() < original_size) {
+        ENVOY_LOG(trace, "cleaned up {} entries in resolver_map_",
+                  original_size - resolver_map_.size());
+      }
+      resolver_map_.emplace(key, resolver);
+      ENVOY_LOG(trace, "resolver_map_ size after adding: {}", resolver_map_.size());
+    }
+    return resolver;


Guard the insertion and cleanup of resolver_map_ with mutex_ to prevent data races. Also, prefer using operator[] or insert_or_assign instead of emplace to ensure the map is correctly updated if the key already exists (even if expired).

auto resolver = std::make_shared<Network::DnsResolverImpl>( cares, dispatcher, csv_or_error.value(), api.rootScope()); if (Runtime::runtimeFeatureEnabled("envoy.restart_features.shared_cares_dns_resolver")) { absl::MutexLock lock(&mutex_); // clean up any nil resolver in the map so it doesn't keep growing auto original_size = resolver_map_.size(); absl::erase_if( resolver_map_, [](const std::pair<const std::size_t, std::weak_ptr<Network::DnsResolver>>& entry) { return entry.second.lock() == nullptr; }); if (resolver_map_.size() < original_size) { ENVOY_LOG(trace, "cleaned up {} entries in resolver_map_", original_size - resolver_map_.size()); } resolver_map_[key] = resolver; ENVOY_LOG(trace, "resolver_map_ size after adding: {}", resolver_map_.size()); } return resolver;

switched to use operator[] and ignored the data race comment because it's not true.

emm, why the data race issue is not true? Considering dns filter might call createDnsResolver() in worker threads concurrently (

envoy/source/extensions/filters/udp/dns_filter/dns_filter.cc

Line 304 in ad01ae3

resolver_ = std::make_unique<DnsFilterResolver>(

), then accessing resolver_map_ needs to be mutex protected, right?

#46577 to revert this PR. PTAL @andy-fong . Please feel free to let me if you think there is no race issue here.

gemini-code-assist · 2026-05-29T20:19:44Z

+
+  absl::flat_hash_map<std::size_t, std::weak_ptr<Network::DnsResolver>> resolver_map_;
 };


The resolver_map_ member variable added to DnsClusterFactory is completely unused and represents dead code. It should be removed.

};

gemini-code-assist · 2026-05-29T20:19:44Z

+bool setQcacheMaxTtl(
+    envoy::extensions::network::dns_resolver::cares::v3::CaresDnsResolverConfig& config,
+    uint32_t value) {
+  auto qcache_max_ttl = std::make_unique<Protobuf::UInt32Value>();
+  qcache_max_ttl->set_value(value);
+  config.set_allocated_qcache_max_ttl(qcache_max_ttl.release());
+  return true;
+}


Simplify setQcacheMaxTtl by using the idiomatic Protobuf mutable_qcache_max_ttl() helper instead of manual memory allocation and release(). Also, change the return type to void since the return value is always true and is ignored by callers.

void setQcacheMaxTtl( envoy::extensions::network::dns_resolver::cares::v3::CaresDnsResolverConfig& config, uint32_t value) { config.mutable_qcache_max_ttl()->set_value(value); }

This function is actually not needed, just directly called mutable_qcache_max_ttl()->set_value()

yanavlasov

The thread safety comments were hallucinated by Gemini. I have closed them. Please address a couple of remaining nits and it should be good to go. Thanks.

/wait

…tl-main Signed-off-by: Andy Fong <andy.fong@solo.io>

Signed-off-by: Andy Fong <andy.fong@solo.io>

…dy exists but not expired Signed-off-by: Andy Fong <andy.fong@solo.io>

andy-fong · 2026-06-11T21:44:46Z

Thank you @yanavlasov, I have updated the PR with the 3 changes, I replied to the gemini review comment. The test failures are unrelated:

CircuitBreaker Test core dump
Distroless image download timed out from the registry

…tl-main Signed-off-by: Andy Fong <andy.fong@solo.io>

botengyao · 2026-06-24T03:11:59Z

ping @yanavlasov for a further look.

andy-fong · 2026-07-14T22:38:48Z

@yanavlasov Thank you for merging this PR but I just found this issue:
#42836 (closed as not planed) that relies on the fact that when there is any c-ares config, it will create a resolver per cluster to workaround the problem. My change here by default will prevent that workaround from working unless the user turns it off with reloadable feature.

Another issue we found was that with shared resolvers and query_tries set to 1 and ~400 clusters for DNS lookup on start up, some DNS look up will timeout due to the kernel buffer being filled and dropping the udp packet. It only recovers on the next DNS lookup. Setting query_tries to 2 or disabling the shared resolver feature will prevent the DNS lookup timeout. This behavior is not observed by the default shared resolvers because the default query_tries is set to 4:

  // .. note::
  //   While the c-ares library defaults to 3 attempts, Envoy's default (if this field is unset) is 4 attempts.
  //   This adjustment was made to maintain the previous behavior after users reported an increase in DNS resolution times.
  //
  google.protobuf.UInt32Value query_tries = 7 [(validate.rules).uint32 = {gte: 1}];

Since even c-ares defaults to 3 tries, not sure how common for users to explicitly set it to 1.

What do you think about these 2 issues? I saw that 1.39 just released, what is the best cause of action?

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

AntonKanug · 2026-08-13T13:54:26Z

Flagging that I saw issues where two workers shared the same DNS resolver, corrupting its socket event state and causing a worker to spin at 100% CPU.

Disabling resolver sharing resolved it.

Commit Message: c-ares: disable shared resolver by default Additional Description: DnsFilter would call createDnsResolver() from a workerthread that would cause a race when sharing dns resolvers. Turn off this feature until the race is fixed. Relates to #46577 and #45073 Risk Level: low Testing: Docs Changes: Release Notes: Platform Specific Features: Signed-off-by: Andy Fong <andy.fong@solo.io>

c-ares: make qcache_max_ttl configurable

5ca834f

Signed-off-by: Andy Fong <andy.fong@solo.io>

andy-fong requested review from mattklein123 and yanavlasov as code owners May 14, 2026 17:09

repokitteh-read-only Bot added the api label May 14, 2026

repokitteh-read-only Bot assigned markdroth May 14, 2026

Merge remote-tracking branch 'origin/main' into af-cares-qcache-max-t…

6b01a7a

…tl-main Signed-off-by: Andy Fong <andy.fong@solo.io>

repokitteh-read-only Bot removed the api label May 19, 2026

repokitteh-read-only Bot assigned yanavlasov May 22, 2026

andy-fong added 3 commits May 26, 2026 11:03

Merge remote-tracking branch 'origin/main' into af-cares-qcache-max-t…

58b7981

…tl-main Signed-off-by: Andy Fong <andy.fong@solo.io>

Merge remote-tracking branch 'origin/main' into af-cares-qcache-max-t…

523ea4d

…tl-main Signed-off-by: Andy Fong <andy.fong@solo.io>

Merge remote-tracking branch 'origin/main' into af-cares-qcache-max-t…

d998991

…tl-main Signed-off-by: Andy Fong <andy.fong@solo.io>

gemini-code-assist Bot reviewed May 29, 2026

View reviewed changes

yanavlasov requested changes Jun 10, 2026

View reviewed changes

repokitteh-read-only Bot added the waiting label Jun 10, 2026

andy-fong added 2 commits June 11, 2026 14:57

Merge remote-tracking branch 'origin/main' into af-cares-qcache-max-t…

f1195d6

…tl-main Signed-off-by: Andy Fong <andy.fong@solo.io>

move changelogs and addressed review comments

db1fbed

Signed-off-by: Andy Fong <andy.fong@solo.io>

repokitteh-read-only Bot removed the waiting label Jun 11, 2026

use operator[] instead of emplace for resolver_map_ in case key alrea…

c927673

…dy exists but not expired Signed-off-by: Andy Fong <andy.fong@solo.io>

andy-fong added 3 commits June 12, 2026 22:34

Merge remote-tracking branch 'origin/main' into af-cares-qcache-max-t…

957965f

…tl-main Signed-off-by: Andy Fong <andy.fong@solo.io>

Merge remote-tracking branch 'origin/main' into af-cares-qcache-max-t…

2f31aa0

…tl-main Signed-off-by: Andy Fong <andy.fong@solo.io>

Merge remote-tracking branch 'origin/main' into af-cares-qcache-max-t…

532cde6

…tl-main Signed-off-by: Andy Fong <andy.fong@solo.io>

yanavlasov approved these changes Jul 1, 2026

View reviewed changes

yanavlasov enabled auto-merge (squash) July 1, 2026 22:53

yanavlasov merged commit 740ae0d into envoyproxy:main Jul 1, 2026
29 checks passed

yanjunxiang-google mentioned this pull request Aug 7, 2026

Revert #45073 c-ares: make qcache_max_ttl configurable #46577

Open

yanjunxiang-google added a commit to yanjunxiang-google/envoy that referenced this pull request Aug 7, 2026

Revert envoyproxy#45073

a202c4a

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

This was referenced Aug 7, 2026

c-ares: disable shared resolver by default #46585

Closed

c-ares: disable shared resolver by default #46622

Merged

c-ares: move dns shared resolver logic to upstream cluster #46657

Open


		absl::flat_hash_map<std::size_t, std::weak_ptr<Network::DnsResolver>> resolver_map_;
		};

Uh oh!

Conversation

andy-fong commented May 14, 2026

Uh oh!

repokitteh-read-only Bot commented May 14, 2026

Uh oh!

andy-fong commented May 14, 2026

Uh oh!

kyessenov commented May 15, 2026

Uh oh!

markdroth commented May 19, 2026

Uh oh!

tyxia commented May 22, 2026

Uh oh!

andy-fong commented May 28, 2026

Uh oh!

andy-fong commented May 28, 2026

Uh oh!

andy-fong commented May 28, 2026

Uh oh!

andy-fong commented May 28, 2026

Uh oh!

andy-fong commented May 29, 2026

Uh oh!

yanavlasov commented May 29, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

gemini-code-assist Bot May 29, 2026

Choose a reason for hiding this comment

Uh oh!

andy-fong Jun 11, 2026

Choose a reason for hiding this comment

Uh oh!

yanjunxiang-google Aug 7, 2026

Choose a reason for hiding this comment

Uh oh!

yanjunxiang-google Aug 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gemini-code-assist Bot May 29, 2026

Choose a reason for hiding this comment

Uh oh!

andy-fong Jun 11, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot May 29, 2026

Choose a reason for hiding this comment

Uh oh!

andy-fong Jun 11, 2026

Choose a reason for hiding this comment

Uh oh!

yanavlasov left a comment

Choose a reason for hiding this comment

Uh oh!

andy-fong commented Jun 11, 2026

Uh oh!

botengyao commented Jun 24, 2026

Uh oh!

Uh oh!

andy-fong commented Jul 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AntonKanug commented Aug 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

yanjunxiang-google Aug 7, 2026 •

edited

Loading

andy-fong commented Jul 14, 2026 •

edited

Loading