Expand use of Druid side timeouts for fabric8 kubernetesclient timeouts by capistrant · Pull Request #18587 · apache/druid

capistrant · 2025-09-30T17:38:42Z

Description

Expand on #18444 to cover more surface area. While putting saveLogs on a strict timer, helped prevent some issues with LogWatch interaction hanging, I have still seen issues in the path where KubernetesWorkItem calls shutdown which starts a LogWatch. Even the initialization of a LogWatch object can hang indefinitely.

Release note

new config for users of the kubernetes-overlord-extensions for K8s TaskRunner.

| `druid.indexer.runner.podLogOperationTimeout` | `Duration` | Timeout for async operations that interact with `k8s` pod logs | `PT300S` | NO |

Key changed/added classes in this PR

KubernetesPeonLifecycle

This PR has:

…ache#18444)" This reverts commit 90be682.

kfaraz · 2025-10-01T11:35:51Z

-    this.logSaveTimeoutMs = logSaveTimeoutMs;
+    this.logWatchInitializationTimeoutMs = logWatchInitializationTimeoutMs;
+    this.logWatchCopyLogTimeoutMs = logWatchCopyLogTimeoutMs;
+    this.logWatchExecutor = Executors.newSingleThreadExecutor(


I feel like the original approach of creating an ExecutorService on the fly when needed made more sense.
We don't really need this executor until we need to watch/copy over the logs, so there is no reason to have it take up unnecessary memory upfront.

ack. I don't like either approach 😨 but it makes sense to not eagerly consume memory if the log watch stuff is involved in a small chunk of the overall peon lifecycle

kfaraz · 2025-10-01T11:37:09Z

 | `druid.indexer.runner.capacity` | `Integer` | Number of concurrent jobs that can be sent to Kubernetes. | `2147483647` | No |
 | `druid.indexer.runner.cpuCoreInMicro` | `Integer` | Number of CPU micro core for the task. | `1000` | No |
 | `druid.indexer.runner.logSaveTimeout` | `Duration` | How long to wait for task logs to be saved before giving up. | `PT300S` | NO |
+| `druid.indexer.runner.logWatchInitializationTimeout` | `Duration` | How long to wait when initializing a log watch for a peon pod before giving up. | `PT30S` | NO |


Let's avoid this new config and just use the original one for both the purposes.
A default timeout of 5 minutes is good enough for both initializing the logWatch field as well as
downloading the logs.

ok, I generalized the single config name a bit then

…hem on demand The idea is that these will only exist for short time per task, so creating them eagerly can be wasteful and cause memory pressure

kfaraz

Thanks for addressing the feedback, @capistrant ! Left some final suggestions.

kfaraz · 2025-10-02T02:50:11Z

 | `druid.indexer.runner.capacity` | `Integer` | Number of concurrent jobs that can be sent to Kubernetes. | `2147483647` | No |
 | `druid.indexer.runner.cpuCoreInMicro` | `Integer` | Number of CPU micro core for the task. | `1000` | No |
-| `druid.indexer.runner.logSaveTimeout` | `Duration` | How long to wait for task logs to be saved before giving up. | `PT300S` | NO |
+| `druid.indexer.runner.podLogOperationTimeout` | `Duration` | Timeout for async operations that interact with `k8s` pod logs  | `PT300S` | NO |


logOperationTimeout seems more ambiguous than logSaveTimeout.
Do you prefer this name since the same timeout is used for multiple operations like copy and log watch initialization?

I think it is fine to continue calling the config logSaveTimeout since both are related to the saving of logs.
Although, please update the description to call out exactly which operations use this timeout and what is the result of that timeout (i.e. will logs for that task ever be accessible).

maybe I was over thinking it. I was concerned about doing the init under the same config as the save. But I suppose you are right that it is probably ok to just stick with it. I reverted and updated the doc with more info on what it is for.

kfaraz · 2025-10-02T02:51:15Z

  @NotNull
  // how long to wait for log saving operations to complete
-  private Period logSaveTimeout = new Period("PT300S");
+  private Period podLogOperationTimeout = new Period("PT300S");


If we continue with the same config name, we can just revert the changes made to this and some other files.

kfaraz · 2025-10-02T03:37:17Z

+               logWatchOperationTimeoutMs, taskId.getOriginalTaskId());
+    }
+    catch (InterruptedException e) {
+      Thread.currentThread().interrupt();


Do we need to mark the thread as interrupted?
Is this status read by the KubernetesTaskRunner?

hmmm. I guess this may just be force of habit to be defensive and do this whenever I catch an interrupted exception. Is there negatives to it even if the status isn't read?

kfaraz · 2025-10-02T03:39:57Z

  }

-  private void doSaveLogs()
+  protected void saveLogs()


I feel we might benefit from adding a common method which can be used both for logWatch init as well as saveLogs:

private <T> T executeWithTimeout(Callable<T> runnable, long timeoutMillis, String operationName) { // Create the executor // Start it // Handle the exceptions // Finally shutdown the executor }

good point, I took a shot at this

kfaraz

Minor suggestions on the log lines etc.
Approach makes sense. 👍🏻

kfaraz · 2025-10-03T10:02:26Z

      Map<String, String> annotations,
      Integer capacity,
-      Period taskJoinTimeout
+      Period taskJoinTimeout,


Please revert the changes to this file since they are not needed anymore.
We can stick to the original order of the constructor args.

kfaraz · 2025-10-03T10:04:05Z

  private final TaskStateListener stateListener;
  private final SettableFuture<Boolean> taskStartedSuccessfullyFuture;
-  private final long logSaveTimeoutMs;
+  private final long logSaveTimeout;


Nit: Please continue using logSaveTimeoutMs or logSaveTimeoutMillis as it removes any ambiguity regarding the time unit.

Alternatively, you may pass in a Duration object and use the name logSaveTimeout,
but I don't think that is needed here.

kfaraz · 2025-10-06T03:49:15Z

+      log.warn("Operation [%s] timed out after %d ms for task [%s]. %s", operationName, timeoutMillis, taskId.getOriginalTaskId(), errorMessage);
+    }
+    catch (InterruptedException e) {
+      Thread.currentThread().interrupt();


@capistrant , regarding the previous discussion, I think we should avoid marking this thread as interrupted here since we are not reading/clearing this interrupted status anywhere which might have unintended side effects.

We can add it in the future if we need it.

…Adds back explicit context

Co-authored-by: Kashif Faraz <kashif.faraz@gmail.com>

…stion without realizing it wouldn't actually refactor the code magically

capistrant added 2 commits September 30, 2025 11:34

Revert "Create Kubernetes peon lifecycle task log persist timeout (ap…

131f9a6

…ache#18444)" This reverts commit 90be682.

Expand LogWatch druid side timeouts to cover more surface area

c77dbf8

github-actions Bot added Area - Documentation Kubernetes labels Sep 30, 2025

update doc

dfcd7f2

github-advanced-security AI found potential problems Sep 30, 2025

View reviewed changes

Comment thread ...rlord-extensions/src/main/java/org/apache/druid/k8s/overlord/KubernetesTaskRunnerConfig.java Fixed

Comment thread ...rlord-extensions/src/main/java/org/apache/druid/k8s/overlord/KubernetesTaskRunnerConfig.java Fixed

capistrant requested a review from kfaraz September 30, 2025 20:15

kfaraz reviewed Oct 1, 2025

View reviewed changes

Reduce memory hit on overlord for the per pod Executors by creating t…

bc0f1b4

…hem on demand The idea is that these will only exist for short time per task, so creating them eagerly can be wasteful and cause memory pressure

github-advanced-security AI found potential problems Oct 1, 2025

View reviewed changes

Comment thread ...rlord-extensions/src/main/java/org/apache/druid/k8s/overlord/KubernetesTaskRunnerConfig.java Fixed

capistrant added the jacoco:skip label Oct 1, 2025

capistrant closed this Oct 1, 2025

capistrant reopened this Oct 1, 2025

capistrant added 2 commits October 1, 2025 18:04

Test new fabric8 logwatch timeouts and shut down executors

633475d

fix formatting

391b8f7

kfaraz reviewed Oct 2, 2025

View reviewed changes

capistrant added 2 commits October 2, 2025 09:09

Extract the repetitive try with timeout code into a reusable method

a8f48bd

refactor config back to original name

26b0972

github-advanced-security AI found potential problems Oct 2, 2025

View reviewed changes

Comment thread ...rlord-extensions/src/main/java/org/apache/druid/k8s/overlord/KubernetesTaskRunnerConfig.java Fixed

fix bad import

52605ab

capistrant requested a review from kfaraz October 6, 2025 01:34

kfaraz approved these changes Oct 6, 2025

View reviewed changes

capistrant and others added 8 commits October 6, 2025 14:05

Remove setting thread as interrupted in k8speonlifcecyle log watch code

30f8c5c

revert some non-functional changes to clean up diff

c2d05ce

revert change that removed unit info from logSaveTimeoutMs variable. …

98f1247

…Adds back explicit context

Apply log message and doc cleanup suggestions from code review

da51d5f

Co-authored-by: Kashif Faraz <kashif.faraz@gmail.com>

Fix compilation error caused by me accepting a refactor request sugge…

cd71a0d

…stion without realizing it wouldn't actually refactor the code magically

revert unnecessary diff in a test file

80412da

remove unnecesarry diff changes in another test file

291b7f6

further diff cleanup

94ff774

capistrant merged commit 2887e52 into apache:master Oct 7, 2025
60 checks passed

cecemei added this to the 35.0.0 milestone Oct 21, 2025

Conversation

capistrant commented Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Release note

Key changed/added classes in this PR

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

kfaraz 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!

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!

Uh oh!

kfaraz 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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

capistrant commented Sep 30, 2025 •

edited

Loading