Pod template task adapter #13896

nlippis · 2023-03-08T00:25:31Z

Kubernetes pod template task adapter for the Kubernetes task runner

Purpose
The existing Kubernetes task runner generates a pod spec for the peons by copying the pod spec of the master pod. While the existing implementation offers some configurability, it is not enough for some users who will need to run Peons with a completely different pod spec from the master pod.

Changes

Allow users to specify a Kubernetes pod spec per task type.
Move PeonCommandContext creation to TaskAdapter since some implementations will not need it

Note
This PR only introduces the PodTemplateTaskAdapter it will be wired up in a forthcoming within the KubernetesTaskRunnerFactory

Key changed/added classes in this PR

PodTemplateTaskAdapter
K8sTaskAdapter

This PR has:

been self-reviewed.
added documentation for new or modified features or behaviors.
a release note entry in the PR description.
added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
added or updated version, license, or notice information in licenses.yaml
added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
added integration tests.
been tested in a test Druid cluster.

...s-overlord-extensions/src/main/java/org/apache/druid/k8s/overlord/common/K8sTaskAdapter.java

...rd-extensions/src/main/java/org/apache/druid/k8s/overlord/common/PodTemplateTaskAdapter.java

churromorales · 2023-03-08T19:51:04Z

...rd-extensions/src/main/java/org/apache/druid/k8s/overlord/common/PodTemplateTaskAdapter.java

+        .addToAnnotations(getJobAnnotations(taskRunnerConfig, task))
+        .endMetadata()
+        .withNewSpec()
+        .withTemplate(podTemplate.getTemplate())


will this work with sidecars injected by a controller? Something like istio for example where you have a control plane which injects a sidecar container into your template. I think what would happen is your job would run and then never really complete....because the istio container would never shut down, you would only exit the main container. I don't know an easy way to do this unless you can do some more templating in yaml, but maybe at the very least I would call this out: If you have a service mesh, which injects sidecars (like istio) this approach may not work.

One thing you can do (since the template overrides the run command) is include something after the main run command that kills the istio pod.

e.g.
java ... CliPeon...
exit_code=$(echo $?); curl -fsI -X POST http://localhost:15020/quitquitquit && exit $exit_code

The user can write their pod spec in any way that they like in order to conform to the peculiarities of their Kubernetes infra. As @georgew5656 mentioned there are a few ways to handle sidecars, from our perspective Druid need not be opinionated about how one would handle starting, stopping and configuring them.

churromorales · 2023-03-08T19:53:24Z

Could you update the README for this extension on how to use this? That would help a lot in reviewing this. I am a bit unclear how this is initialized, what properties need to be set, vs configurations, etc...

churromorales · 2023-03-08T19:58:11Z

...es-overlord-extensions/src/main/java/org/apache/druid/k8s/overlord/KubernetesTaskRunner.java

-    );
-    return command;
-  }
-


can you explain how the CliPeon is going to work with this approach? How is AbstractTask going to look as well?

No changes are required to CliPeon or AbstractTask. With the PodTemplateTaskAdapter everything is up to the user to specify. Our plan moving forward will be for sane defaults to be set within helm but those changes will be made in a future PR.

nlippis · 2023-03-08T20:19:34Z

Could you update the README for this extension on how to use this? That would help a lot in reviewing this. I am a bit unclear how this is initialized, what properties need to be set, vs configurations, etc...

This PR just introduces the PodTemplateTaskAdapter, a following PR will wire it up. I will include public facing documentation about how to use it there.

That said, the user will specify druid.indexer.runner.k8s.adapter.type={SingleContainer|MultiContainer|PodTemplate}. The user can then specify a base pod template to be used in case they haven't specified per task overrides i.e druid.indexer.runner.k8s.podTemplate.base=/path/to/base-pod-template.yml, and then per task overrides i.e druid.indexer.runner.k8s.podTemplate.{taskType}=/path/to/{taskType}-pod-template.yml

georgew5656

this generally looks good to me, I left one clarification comment and one small nit

georgew5656 · 2023-03-09T19:50:28Z

...etes-overlord-extensions/src/main/java/org/apache/druid/k8s/overlord/common/TaskAdapter.java

+public interface TaskAdapter
 {

-  V fromTask(Task task, PeonCommandContext context) throws IOException;


i think there was some idea that we would want to leave open the option of non K8s Task Runners (e.g. fargate) which would imply the need for non K8s TaskAdapters, is there a need to override this class? wouldn't K8sTaskAdapter implements TaskAdapter<Job, Pod> still work here?

Realizing TaskAdapter is actually a K8s only concept so this comment is not relevant.

I do think TaskAdapter being only for k8s to be a little confusing though, i think it would make more sense if the current TaskAdapter was called K8sTaskAdapter and the current K8sTaskAdapter was called K8sFromOverlordTaskAdapter or something similar

We would implement Fargate within a FargateTaskRunner. The TaskAdapter concept is a KubernetesTaskRunner only concept not a Druid one.

why would Fargate be implemented through a different runner? doesn't fargate offer k8s api?

I'm unfamiliar with the api that Fargate provides. If it does offer a k8s api then yes we could continue to use the KubernetesTaskRunner.

georgew5656 · 2023-03-09T19:55:50Z

...rd-extensions/src/main/java/org/apache/druid/k8s/overlord/common/PodTemplateTaskAdapter.java

+    MapperConfig config = mapper.getDeserializationConfig();
+    AnnotatedClass cls = AnnotatedClassResolver.resolveWithoutSuperTypes(config, Task.class);
+    Collection<NamedType> taskSubtypes = mapper.getSubtypeResolver().collectAndResolveSubtypesByClass(config, cls);
+    for (NamedType namedType : taskSubtypes) {


is there a use case that we'd want multiple specs for a single task type? like e.g. a bigger index_kafka job vs a smaller index_kafka job? I don't think that needs to be supported in this PR but it would be good to still have the option to implement something like that in the future.

I guess with this implementation you could just set druid.indexer.runner.k8s.podTemplate.index_parallel_custom or something, and assuming there's some field on the task (customJobTemplate or similar) that specifies to use this template, to reference that field?

abhishekagarwal87

LGTM except some minor comments

...rd-extensions/src/main/java/org/apache/druid/k8s/overlord/common/PodTemplateTaskAdapter.java

abhishekagarwal87 · 2023-03-17T11:24:46Z

...rd-extensions/src/main/java/org/apache/druid/k8s/overlord/common/PodTemplateTaskAdapter.java

+  public static String TYPE = "PodTemplate";
+
+  private static final Logger log = new Logger(PodTemplateTaskAdapter.class);
+  private static final String TASK_PROPERTY = IndexingServiceModuleHelper.INDEXER_RUNNER_PROPERTY_PREFIX + ".k8s.podTemplate.%s";


are you going to document this in a follow up PR?

Yes, these will be documented in the next PR that hooks the PodTemplateTaskAdapter up.

abhishekagarwal87 · 2023-03-17T11:29:01Z

...etes-overlord-extensions/src/main/java/org/apache/druid/k8s/overlord/common/TaskAdapter.java

+public interface TaskAdapter
 {

-  V fromTask(Task task, PeonCommandContext context) throws IOException;


why would Fargate be implemented through a different runner? doesn't fargate offer k8s api?

...-extensions/src/test/java/org/apache/druid/k8s/overlord/KubernetesTaskRunnerFactoryTest.java

abhishekagarwal87 · 2023-03-17T11:33:48Z

...-extensions/src/test/java/org/apache/druid/k8s/overlord/KubernetesTaskRunnerFactoryTest.java

+
+    KubernetesTaskRunner runner = factory.build();
+
+    Assert.assertNotNull(runner);


same comment as above here.

...rd-extensions/src/main/java/org/apache/druid/k8s/overlord/common/PodTemplateTaskAdapter.java

abhishekagarwal87 · 2023-03-17T11:50:23Z

...xtensions/src/test/java/org/apache/druid/k8s/overlord/common/PodTemplateTaskAdapterTest.java

+  @Test
+  public void test_fromTask_withoutBasePodTemplateInRuntimeProperites_raisesIAE()
+  {
+    Assert.assertThrows(IAE.class, () -> new PodTemplateTaskAdapter(


Please also verify that the correct error message is included in the exception.

abhishekagarwal87 · 2023-03-17T11:52:23Z

...xtensions/src/test/java/org/apache/druid/k8s/overlord/common/PodTemplateTaskAdapterTest.java

Please do add error message verification as well.

…-adapter

Pod template task adapter

476267b

github-advanced-security bot found potential problems Mar 8, 2023

View reviewed changes

...s-overlord-extensions/src/main/java/org/apache/druid/k8s/overlord/common/K8sTaskAdapter.java Fixed Show fixed Hide fixed

...rd-extensions/src/main/java/org/apache/druid/k8s/overlord/common/PodTemplateTaskAdapter.java Fixed Show fixed Hide fixed

Nicholas Lippis added 5 commits March 8, 2023 08:25

Use getBaseTaskDirPaths

126dad4

Remove unused task from getEnv

3590ad6

Use Optional.ifPresent() instead of Optional.map()

9125411

Pass absolute path

a7f638b

Don't pass task to getEnv

db728a2

churromorales reviewed Mar 8, 2023

View reviewed changes

georgew5656 reviewed Mar 9, 2023

View reviewed changes

georgew5656 approved these changes Mar 9, 2023

View reviewed changes

asdf2014 added Kubernetes Area - Ingestion labels Mar 17, 2023

abhishekagarwal87 reviewed Mar 17, 2023

View reviewed changes

Nicholas Lippis added 4 commits March 21, 2023 12:46

Merge remote-tracking branch 'upstream/master' into pod-template-task…

791f354

…-adapter

Assert the correct adapter is created

5476b86

Javadocs and Comments

6db6d2d

Add exception message to assertions

ce33f79

abhishekagarwal87 approved these changes Mar 22, 2023

View reviewed changes

dclim merged commit d81d13b into apache:master Mar 22, 2023

nlippis deleted the pod-template-task-adapter branch March 22, 2023 20:21

clintropolis added this to the 26.0 milestone Apr 10, 2023

techdocsmith mentioned this pull request Apr 12, 2023

[DRAFT] 26.0.0 release notes #14064

Closed

GWphua mentioned this pull request Jul 4, 2025

Make K8s tasks persist task report + Fix MSQ INSERT/REPLACE querying problem. #18206

Merged

8 tasks


		KubernetesTaskRunner runner = factory.build();

		Assert.assertNotNull(runner);

Pod template task adapter #13896

Pod template task adapter #13896

Uh oh!

Conversation

nlippis commented Mar 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Kubernetes pod template task adapter for the Kubernetes task runner

Key changed/added classes in this PR

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!

churromorales commented Mar 8, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nlippis commented Mar 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

georgew5656 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

georgew5656 Mar 9, 2023 • 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!

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!

abhishekagarwal87 left a comment

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

nlippis commented Mar 8, 2023 •

edited

Loading

nlippis commented Mar 8, 2023 •

edited

Loading

georgew5656 Mar 9, 2023 •

edited

Loading