Skip to content

[OBSDATA-13995] Backport service discovery fix (#19139) and add broker TCP connect timeout - #450

Merged
Sadananda Aithal (saithal-confluent) merged 2 commits into
34.0.0-confluentfrom
saithal/obsdata-13995
Jul 8, 2026
Merged

Sadananda Aithal (saithal-confluent) merged 2 commits into
34.0.0-confluentfrom
saithal/obsdata-13995

Conversation

@saithal-confluent

@saithal-confluent Sadananda Aithal (saithal-confluent) commented May 18, 2026 •

Copy link
Copy Markdown
Member

Fixes OBSDATA-13995.

Description

When a K8s node is abruptly terminated by CSP, fails health checks, or has its Druid container killed (e.g. OOM) while the pod object survives, the Druid broker keeps routing queries to the dead/unhealthy Historical for up to ~2 minutes. This PR addresses two compounding, independent gaps.

1. Backport apache#19139 — readiness-based service discovery

Backports apache/druid#19139 (Fix service discovery bug in kubernetes-extensions), already merged upstream and present on 37.0.0-confluent, onto this 34.0.0-confluent branch.

K8s service discovery previously kept a node in every service's discovered-services cache as long as the pod existed with announcement labels — even if the underlying container was killed and could not serve requests. The watcher also silently dropped all MODIFIED events.

19139 makes discovery use the pod readiness flag:

  • During the periodic listPods, pods that are not reporting ready are skipped.
  • In the watch stream, an unready pod on a MODIFIED event is remapped to a new synthetic NOT_READY type, which removes the host from the discovery cache. Unready pods on ADDED are skipped until they become ready. A ready pod on MODIFIED is remapped to ADDED so it is re-added on recovery.
  • BaseNodeRoleWatcher / K8sDruidNodeDiscoveryProvider handle NOT_READY as a removal and suppress the ERROR-level log noise from duplicate/idempotent add/remove.

This supersedes the earlier deletion-timestamp-only approach on this branch: readiness covers both the graceful terminating case and the container-killed-but-pod-alive case. The Confluent terminatingStateCheckDuration list filter is retained alongside the readiness check.

Note: DefaultK8sApiClientTest and the new K8sDruidNodeDiscoveryProviderTest cases were adapted from JUnit 5 to JUnit 4 and to the 4-arg listPods(..., Duration) signature to match this branch's kubernetes-extensions module conventions.

2. Wire TCP connect timeout into the Netty HTTP client

The Netty ClientBootstrap had no connectTimeoutMillis set, so a TCP SYN to an unreachable host hung for the OS default (~2 minutes on Linux). The existing druid.broker.http.clientConnectTimeout config (default 500ms) was only applied to the Jetty client used by the Router, not the Netty client used by the Broker.

The connectTimeout from DruidHttpClientConfig is now threaded through HttpClientConfig → HttpClientInit.createBootstrap() → bootstrap.setOption("connectTimeoutMillis", ...). Queries to unreachable Historicals now fail in 500ms (configurable) instead of hanging for ~2 minutes.

Release note

Druid brokers now remove K8s-discovered nodes from routing as soon as their pods stop reporting ready (via watch events, within seconds) and fail fast (500ms default, druid.broker.http.clientConnectTimeout) when connecting to unreachable servers, reducing query error rates during node failures and unhealthy pods.


Key changed/added classes in this PR
  • DefaultK8sApiClient — readiness-based filtering in listPods/watchPods, isPodReady helper (retains terminatingStateCheckDuration filter)
  • WatchResult — add MODIFIED and synthetic NOT_READY constants
  • K8sDruidNodeDiscoveryProvider — handle NOT_READY as removal, suppress log noise
  • BaseNodeRoleWatcher — idempotent add/remove handling for NOT_READY
  • HttpClientConfig — add connectTimeout field
  • HttpClientInit — set connectTimeoutMillis on the Netty bootstrap
  • HttpClientModule — wire clientConnectTimeout into the HttpClientConfig builder

This PR has:

  • been self-reviewed.
  • added unit tests or modified existing tests to cover new code paths.
  • been tested in a test Druid cluster.

final ClientBootstrap bootstrap = new ClientBootstrap(new NioClientSocketChannelFactory(bossPool, workerPool));

bootstrap.setOption("keepAlive", true);
bootstrap.setOption("connectTimeoutMillis", connectTimeoutMs);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is helpful as it sets a cap of 500 ms on New broker→historical TCP/TLS handshake.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is configurable as well.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let us revert change to DefaultK8sApiClient.java‎ and get this merged.
Thanks for making the change.

Watch.Response<V1Pod> item = watch.next();
if (item != null && item.type != null && !item.type.equals(WatchResult.BOOKMARK)) {
String effectiveType = item.type;
if (WatchResult.MODIFIED.equals(item.type)) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won’t help. Check the sequence of events below. I captured this after I tried this on pre-prod env.

https://confluentinc.atlassian.net/wiki/spaces/~62be365dc9f2df7b608c58d2/pages/5379195369/Reduce+the+time+taken+for+a+bad+node+to+get+removed+from+Druid+Service+Discovery#Current-Prod-state

Deletion timestamp is set after unreachable taint is applied to the node. unreachable taint is applied after at least node-monitor-grace-period of 40 secs (+ Druid toleration that I have set as 20 secs). For March Sev 1, node was tainted after 49 secs. On May 14, node was tainted after 62 secs. We dont really control when node gets tainted as above shows.

Feel free to try the steps on preprod where I saw Deletion timestamp is not set until after node is tainted.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true. The deletion timestamp is set after the grace period elapses at the node controller level. In such cases this won't help.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am working on an alternate way to fix this issue by not sending requests to the historical based on some heuristics that tell that Historical is not healthy. I'll send you that PR once my testing is successful. Thanks

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not wait for the next reconcile to happen. Instead, https://github.com/confluentinc/druid/pull/339/changes PR already uses the deletion timestamp annotation. So, this PR is same as setting the grace period to 0 in the other PR

@saithal-confluent

Copy link
Copy Markdown
Member Author

We do not wait for the next reconcile to happen. Instead, https://github.com/confluentinc/druid/pull/339/changes PR already uses the deletion timestamp annotation. So, this PR is same as setting the grace period to 0 in the other PR

Parth Agrawal (@pagrawal10) , the changes you mentioned in the PR are related to the periodic listing. The changes proposed in this PR are more real-time as it is based on streaming the events using the watch API.

@saithal-confluent

Copy link
Copy Markdown
Member Author

ravisharm Good point — deletionTimestamp is indeed delayed by the node-monitor-grace-period (~40s) + pod toleration (~20s), so this watch change doesn't help during the first ~60s after the node dies. The connect timeout fix covers that critical window.

However, once deletionTimestamp IS set (at ~60s), there's still a gap between the two detection paths:

  • Without the watch change: the broker waits for the next periodic listPods cycle (1 min interval) to notice the terminating pod. Worst case: ~60s (taint) + ~60s (list interval) = ~2 min before removal.
  • With the watch change: the MODIFIED event fires immediately when deletionTimestamp is set. Worst case: ~60s (taint) + ~1-5s (watch latency) = ~65s before removal.

So this shaves up to ~1 minute off the total detection time after the taint is applied. It's complementary to the connect timeout — the connect timeout makes queries fail fast during the entire window, while the watch change gets the dead server out of the routing view sooner once K8s marks it for deletion.

Happy to revert if you still feel it's not worth the added code though.

The Netty ClientBootstrap had no connectTimeoutMillis set, so TCP
SYN to an unreachable host would hang for the OS default (~2 min
on Linux). The existing druid.broker.http.clientConnectTimeout
config (default 500ms) was only applied to the Jetty client used
by the Router, not the Netty client used by the Broker.

Thread connectTimeout through HttpClientConfig into the Netty
bootstrap via bootstrap.setOption("connectTimeoutMillis", ...).
Queries to unreachable Historicals now fail in 500ms instead of
hanging for minutes.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@airlock-confluentinc
airlock-confluentinc Bot force-pushed the saithal/obsdata-13995 branch from 9e70846 to 6ccac59 Compare July 1, 2026 09:44
@saithal-confluent Sadananda Aithal (saithal-confluent) changed the title [OBSDATA-13995] Add TCP connect timeout and fast terminating pod detection for brokers [OBSDATA-13995] Backport service discovery fix (#19139) and add broker TCP connect timeout Jul 1, 2026
Sadananda Aithal (saithal-confluent) added a commit that referenced this pull request Jul 8, 2026
…ix (apache apache#19139) to druid 30 (#469)

* OBSDATA-13995 Wire TCP connect timeout into Netty HTTP client

The Netty ClientBootstrap had no connectTimeoutMillis set, so TCP
SYN to an unreachable host would hang for the OS default (~2 min
on Linux). The existing druid.broker.http.clientConnectTimeout
config (default 500ms) was only applied to the Jetty client used
by the Router, not the Netty client used by the Broker.

Thread connectTimeout through HttpClientConfig into the Netty
bootstrap via bootstrap.setOption("connectTimeoutMillis", ...).
Queries to unreachable Historicals now fail in 500ms instead of
hanging for minutes.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* Fix service discovery bug in `kubernetes-extensions` (apache#19139)

---------

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Co-authored-by: Lucas Capistrant <capistrant@users.noreply.github.com>
@saithal-confluent
Sadananda Aithal (saithal-confluent) merged commit b0f841b into 34.0.0-confluent Jul 8, 2026
2 checks passed
@saithal-confluent
Sadananda Aithal (saithal-confluent) deleted the saithal/obsdata-13995 branch July 8, 2026 11:26
Sadananda Aithal (saithal-confluent) added a commit that referenced this pull request Jul 20, 2026
…oker connect timeout (#474)

Scoped revert of the kubernetes-extensions / service-discovery half of
455f3e5 (OBSDATA-13995 Backport connect timeout (#450) and service
discovery fix (apache apache#19139) to druid 30 (#469)). The broker TCP
connect-timeout fix (HttpClientConfig.java, HttpClientInit.java,
HttpClientModule.java, DruidHttpClientConfig.java) is untouched and
remains in place - it is unrelated and unaffected by this incident.

The isPodReady() filter added to DefaultK8sApiClient.listPods()/
watchPods() requires k8s to already report a pod's container ready
before that pod can be treated as self-discovered. But a pod's
readiness/startup probe (still pointed at /status/selfDiscovered on
every environment using this image) only passes once self-discovery
succeeds - a circular dependency. This crash-restart-looped every
overlord pod on druid-devel-44409 (running druid-34, which carries
the identical filter) after it picked up an image containing this
filter; druid-30 is exposed to the same risk on its next fresh pod
rollout.

The real fix (a new /status/ready endpoint backed by a purely local
ServiceAnnouncementState flag, breaking the circularity) is up as
PR #472 on this branch's line, but the filter and the fix must ship
in the same image tag - landing them in different tags is what
caused the incident, and leaving the filter in now blocks other
in-flight work on this branch. Reverting here to unblock; the filter
will be re-added together with PR #472 once both are ready to ship
as a single image.

This reverts the isPodReady() filter and its tests to their pre-455f3e5715
state; docs/development/extensions-core/kubernetes.md and
website/.spelling are reverted alongside since both are attributable
solely to the service-discovery change.
Sadananda Aithal (saithal-confluent) added a commit that referenced this pull request Jul 20, 2026
…oker connect timeout (#473)

Scoped revert of the kubernetes-extensions / service-discovery half of
b0f841b ([OBSDATA-13995] Backport service discovery fix (apache#19139) and
add broker TCP connect timeout (#450)). The broker TCP connect-timeout
fix (HttpClientConfig.java, HttpClientInit.java, HttpClientModule.java)
is untouched and remains in place - it is unrelated and unaffected by
this incident.

The isPodReady() filter added to DefaultK8sApiClient.listPods()/
watchPods() requires k8s to already report a pod's container ready
before that pod can be treated as self-discovered. But a pod's
readiness/startup probe (still pointed at /status/selfDiscovered on
every environment using this image) only passes once self-discovery
succeeds - a circular dependency. This crash-restart-looped every
overlord pod on druid-devel-44409 after it picked up an image
containing this filter.

The real fix (a new /status/ready endpoint backed by a purely local
ServiceAnnouncementState flag, breaking the circularity) is up as
PR #471 on this branch's line, but the filter and the fix must ship
in the same image tag - landing them in different tags is what
caused the incident, and leaving the filter in now blocks other
in-flight work on this branch. Reverting here to unblock; the filter
will be re-added together with PR #471 once both are ready to ship
as a single image.

This reverts the isPodReady() filter and its tests to their pre-b0f841bfb3
state; docs/development/extensions-core/kubernetes.md and
website/.spelling are reverted alongside since both are attributable
solely to the service-discovery change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants