Support Dynamic Peon Pod Template Selection in K8s extension #16510

YongGang · 2024-05-29T05:49:31Z

Description

This PR introduces a new feature to dynamically select Kubernetes pod templates for task execution in Druid. This functionality aims to optimize resource utilization and improve task execution efficiency by tailoring pod specifications to the needs of different task characteristics.
Druid operator can define execution strategies and associate them with different task characteristics through the new dynamic config interface. The system will apply these strategies dynamically as tasks are scheduled for execution.
This feature is a step towards making Apache Druid more adaptable and efficient in Kubernetes environments, addressing the need for more granular control over resource allocation and task scheduling.

Example Configuration:

We define two template keys in the configuration—low-throughput and medium-throughput—each associated with specific task conditions and arranged in a priority order.

Low Throughput Template: This is the first template evaluated and has the highest priority. Tasks that have a context tag billingCategory=streaming_ingestion and a datasource of wikipedia will be classified under the low-throughput template. This classification directs such tasks to utilize a predefined pod template optimized for low throughput requirements.
Medium Throughput Template: If a task does not meet the low-throughput criteria, the system will then evaluate it against the next selector in order. In this example, if the task type is index_kafka, it will fall into the medium-throughput template.

{
  "type": "default",
  "podTemplateSelectStrategy":
  {
    "type": "selectorBased",
    "selectors": [
      {
        "selectionKey": "low-throughput",
        "context.tags":
        {
          "billingCategory": ["streaming_ingestion"]
        },
        "dataSource": ["wikipedia"]
      },
      {
        "selectionKey": "medium-throughput",
        "type": ["index_kafka"]
      }
    ],
    "defaultKey": "base"
  }
}

Release note

The Dynamic Pod Template Selection feature enhances the K8s extension by enabling more flexible and dynamic selection of pod templates based on task properties.

Key changed/added classes in this PR

KubernetesTaskRunnerDynamicConfig.java Represents the configuration for task execution within a Kubernetes environment. This interface allows for dynamic configuration of task execution strategies based on specified behavior strategies.
KubernetesTaskExecutionConfigResource.java Resource that manages Kubernetes-specific execution configurations for running tasks.
PodTemplateSelectStrategy.java Defines a strategy for selecting Pod template of tasks based on specific conditions.

This PR has:

...etes-overlord-extensions/src/main/java/org/apache/druid/k8s/overlord/execution/Selector.java

...tensions/src/main/java/org/apache/druid/k8s/overlord/taskadapter/PodTemplateTaskAdapter.java

georgew5656 · 2024-05-30T19:31:21Z

...erlord-extensions/src/main/java/org/apache/druid/k8s/overlord/execution/ExecutionConfig.java

+})
+public interface ExecutionConfig
+{
+  String CONFIG_KEY = "k8s.taskrunner.config";


whats something that would go under this ExecutionConfig but not under ExecutionBehaviorStrategy? does it make more sense to call this KubernetesTaskRunnerRefreshableConfig or something like that?

ExecutionConfig can have config other than ExecutionBehaviorStrategy, we may can move RunnerStrategy to this dynamic config (or something similar in the future):

{ "type": "default", "behaviorStrategy": { "type": "default", "categorySelectors": [ ] }, "runnerStrategy": { ... } }

In this case it's not only guide KubernetesTaskRunner behavior but also whether task should run in Worker, so I think the general ExecutionConfig name is making more sense.

okay, ExecutionConfig makes sense to me then. or maybe TaskExecutionConfig?

would categoryStrategy or templateStrategy make more sense for the second level thing then? since that config is really about choosing what template to run a task with?

It named as behaviorStrategy since not only it will choose category and map template, this strategy will also be used in the future task laning work e.g. choose a task lane.

would task laning not be a separate field? like laneStrategy?

I’m considering using a single strategy but with different fields returned. Since the implementation of the DynamicTaskExecutionBehaviorStrategy relies on the same Selector matching rules mechanism, the rules for matching categories and lanes are quite similar. Therefore, there’s no need to introduce a separate strategy for these functions.

public interface ExecutionBehaviorStrategy { String getTaskCategory(Task task); String getTaskLane(Task task); }

I too was confused about this name. I think having a more narrowly scoped interface will be easier to understand and maintain.

YongGang and I discussed this offline, and I better understand the intent of this config object. It seems like this is trying to provide similar functionality as KubernetesTaskRunnerConfig but via the dynamic config. It makes sense to have an encompassing dynamic config object for this extension.

Some suggested names for this that better indicate it's purpose KubernetesTaskRunnerDynamicConfig KubernetesPeonDynamicConfig KubernetesTaskExecutionConfig

arunramani · 2024-05-31T17:52:40Z

If we want Selector to be general purpose, we need to clean it up a bit. When implementing a selector, you need to have 3 things: the list of selections, the evaluation criteria and the selection key. For this case, we could simplify it to just the list of selections and the selection key. The evaluation criteria can be defaulted to a simple "AND all of the keys except the select key". An example of how it should look

"behaviorStrategy": {
    "type": "default",
    "selectKey": "category",
    "categorySelectors": [
      {
        "category": "low-throughput",
        "context.tags": {
          "billingCategory": [
            "streaming_ingestion"
          ]
        },
        "task": {
          "datasource": [
            "wikipedia"
          ]
        }
      },
      {
        "category": "medium-throughput",
        "task": {
          "type": [
            "index_kafka"
          ]
        }
      }
    ]
  }

So now the evaluator would look at all of the keys EXCEPT category and one a match, it will return the category.

What do you think?

YongGang · 2024-05-31T19:19:48Z

If we want Selector to be general purpose, we need to clean it up a bit. When implementing a selector, you need to have 3 things: the list of selections, the evaluation criteria and the selection key. For this case, we could simplify it to just the list of selections and the selection key. The evaluation criteria can be defaulted to a simple "AND all of the keys except the select key". An example of how it should look
"behaviorStrategy": {
    "type": "default",
    "selectKey": "category",
    "categorySelectors": [
      {
        "category": "low-throughput",
        "context.tags": {
          "billingCategory": [
            "streaming_ingestion"
          ]
        },
        "task": {
          "datasource": [
            "wikipedia"
          ]
        }
      },
      {
        "category": "medium-throughput",
        "task": {
          "type": [
            "index_kafka"
          ]
        }
      }
    ]
  }
So now the evaluator would look at all of the keys EXCEPT category and one a match, it will return the category.

What do you think?

I think if fully implement this proposal, it needs to be on reflection based, otherwise still need to preknown what fields to look at. Given the concerns we have on reflection (e.g. perf overhead, not safe etc) and we don't expect some very different fields to look at in the near future so I think the current solution is good enough.

...tensions/src/main/java/org/apache/druid/k8s/overlord/taskadapter/PodTemplateTaskAdapter.java

docs/development/extensions-contrib/k8s-jobs.md

suneet-s

-1 on this change because I do not understand the use of the new interfaces. It seems like what we want here is a config that is scoped to the pod template adapter, but this PR introduces configs for general use across all of the k8s extension. Because of this the names of the interfaces and their uses are not clear to me eg. ExecutionConfig that returns an ExecutionBehaviorStrategy that gets a "category" from the task. It seems like all we need here is something like a PodTemplateNamingStrategy or a PodTemplateSelector

The other thing that feels clunky is the selector class. It should be an interface so that it can be extended in the future. The current selector class implementation does not provide good errors to users if they reference a field that is not currently supported - like group id. It feels like the Selector class is trying to implement a Predicate<Task>

I'd recommend introducing an interface called PodTemplateSelector that returns a PodTemplate given a Task object (similar to the BehaviorSelector classes introduced in this patch). For the Selectors - I'd recommend renaming them to Matchers that implement Predicate<Task>. We could then introduce and, not, or matchers and matchers that match on dataSource, tags, any context, task type, etc. The config would then look like

"podTemplateSelectorStrategy" : {
  "type": matcherBased,
  "templateMatchers": [
    {
      "template": "template0",
      "matcher": {
        "type": or,
        "matchers": [
          {
            "type": "dataSource",
            "matchingNames": ["ds0"]
          },
          {
            "type": "context",
            "field": "myContextKey"
            "matchingNames": ["anyValue"]
          },
        ]
      }
    },
    {
      "template": "template1",
      "matcher": {
        "type": "taskType",
        "matchingTypes": ["index_kafka", "index_kinesis"]
      }
    }
  ]
}

suneet-s · 2024-06-05T04:49:30Z

docs/development/extensions-contrib/k8s-jobs.md

@@ -217,6 +217,66 @@ data:
        druid.peon.mode=remote
        druid.indexer.task.encapsulatedTask=true
 ```
+#### Dynamic Pod Template Selection Config


note to self: doc should be re-written. remove use of new feature, more flexible, etc.

What is the right point to talk about this config

suneet-s · 2024-06-05T05:29:01Z

...verlord-extensions/src/main/java/org/apache/druid/k8s/overlord/KubernetesOverlordModule.java

@@ -98,6 +103,8 @@ public void configure(Binder binder)
          .toProvider(RunnerStrategyProvider.class)
          .in(LazySingleton.class);
    configureTaskLogs(binder);
+
+    Jerseys.addResource(binder, KubernetesResource.class);


KubernetesResource does not indicate what the resource is actually for. Suggested rename KubernetesTaskExecutionConfigResource

suneet-s · 2024-06-05T05:32:59Z

...lord-extensions/src/main/java/org/apache/druid/k8s/overlord/common/KubernetesPeonClient.java

+    if (executionConfig != null && executionConfig.getBehaviorStrategy() != null) {
+      metricBuilder.setDimensionIfNotNull(
+          "category",
+          executionConfig.getBehaviorStrategy().getTaskCategory(task)
+      );
+    }


This seems incorrect. The executionConfig could have changed from the time the task was converted to a job to when executionConfig.getBehaviorStrategy() is called in this function.

suneet-s · 2024-06-05T05:40:35Z

...ensions/src/main/java/org/apache/druid/k8s/overlord/execution/ExecutionBehaviorStrategy.java

+    @JsonSubTypes.Type(name = "default", value = DefaultExecutionBehaviorStrategy.class),
+    @JsonSubTypes.Type(name = "dynamicTask", value = DynamicTaskExecutionBehaviorStrategy.class),
+})
+public interface ExecutionBehaviorStrategy


I don't understand the name of this interface. What is the execution behavior strategy? It looks like this is just getting the name of a category from a Task

suneet-s · 2024-06-05T05:44:41Z

...extensions/src/main/java/org/apache/druid/k8s/overlord/execution/DefaultExecutionConfig.java

+
+import java.util.Objects;
+
+public class DefaultExecutionConfig implements ExecutionConfig


rename to TaskTypeExecutionConfig to indicate what it is doing

suneet-s · 2024-06-05T05:46:41Z

.../src/main/java/org/apache/druid/k8s/overlord/execution/DefaultExecutionBehaviorStrategy.java

+ * This implementation categorizes tasks by simply returning the type of the task,
+ * making it a straightforward, type-based categorization strategy.
+ */
+public class DefaultExecutionBehaviorStrategy implements ExecutionBehaviorStrategy


Rename to TaskTypeExecutionBehaviorStrategy instead of Default to be more descriptive of what this class is trying to do.

suneet-s · 2024-06-05T05:48:38Z

...erlord-extensions/src/main/java/org/apache/druid/k8s/overlord/execution/ExecutionConfig.java

+})
+public interface ExecutionConfig
+{
+  String CONFIG_KEY = "k8s.taskrunner.config";


I too was confused about this name. I think having a more narrowly scoped interface will be easier to understand and maintain.

suneet-s · 2024-06-05T06:00:31Z

...ord-extensions/src/main/java/org/apache/druid/k8s/overlord/execution/KubernetesResource.java

+@Path("/druid/indexer/v1/k8s/runner")
+public class KubernetesResource


Suggested change

@Path("/druid/indexer/v1/k8s/runner")

public class KubernetesResource

@Path("/druid/indexer/v1/k8s/taskRunner")

public class KubernetesTaskRunnerResource

OR

Suggested change

@Path("/druid/indexer/v1/k8s/runner")

public class KubernetesResource

@Path("/druid/indexer/v1/k8s/taskRunner/executionConfig")

public class KubernetesTaskRunnerExecutionConfigResource

...ord-extensions/src/main/java/org/apache/druid/k8s/overlord/execution/KubernetesResource.java

docs/development/extensions-contrib/k8s-jobs.md

YongGang · 2024-06-06T20:09:42Z

-1 on this change because I do not understand the use of the new interfaces. It seems like what we want here is a config that is scoped to the pod template adapter, but this PR introduces configs for general use across all of the k8s extension. Because of this the names of the interfaces and their uses are not clear to me eg. ExecutionConfig that returns an ExecutionBehaviorStrategy that gets a "category" from the task. It seems like all we need here is something like a PodTemplateNamingStrategy or a PodTemplateSelector

The other thing that feels clunky is the selector class. It should be an interface so that it can be extended in the future. The current selector class implementation does not provide good errors to users if they reference a field that is not currently supported - like group id. It feels like the Selector class is trying to implement a Predicate<Task>

I'd recommend introducing an interface called PodTemplateSelector that returns a PodTemplate given a Task object (similar to the BehaviorSelector classes introduced in this patch). For the Selectors - I'd recommend renaming them to Matchers that implement Predicate<Task>. We could then introduce and, not, or matchers and matchers that match on dataSource, tags, any context, task type, etc. The config would then look like
"podTemplateSelectorStrategy" : {
  "type": matcherBased,
  "templateMatchers": [
    {
      "template": "template0",
      "matcher": {
        "type": or,
        "matchers": [
          {
            "type": "dataSource",
            "matchingNames": ["ds0"]
          },
          {
            "type": "context",
            "field": "myContextKey"
            "matchingNames": ["anyValue"]
          },
        ]
      }
    },
    {
      "template": "template1",
      "matcher": {
        "type": "taskType",
        "matchingTypes": ["index_kafka", "index_kinesis"]
      }
    }
  ]
}

@suneet-s I have addressed most of the comments except for the Matchers proposal. While it’s a good suggestion, I don’t believe it’s the right fit for our scenario for a couple of reasons:

Complexity and Applicability of Operators: Matchers provide a variety of operators such as and, not, and or for evaluation. Our current design with sequence-based selectors is already complex and requires careful attention to detail. Introducing additional operators would further complicate the understanding of the dynamic config. Specifically, the not and or operators might not be very useful for our focused criteria of template selection. For example, the not operator could yield unexpected results, especially as new Task types are continuously added, potentially covering unintended scenarios without careful usage.
Relevance of Typed Matchers: While the typed Matcher approach is excellent for extensibility and appears future-proof, it is more aligned with scenarios like Druid’s Filter, where many implementations exist for type safety and performance enhancements. In our case, however, the field comparisons are predominantly string-based. Given that our main input in this K8s extension is a Task, we do not anticipate needing significantly different criteria from what is currently covered. Should future requirements drastically diverge, adopting a new strategy would be more appropriate than extending the current one.

Additionally, implementing Matchers would require introducing many small Jackson classes, which could bloat the codebase. Our current implementation of Selector is cleaner and more streamlined in comparison.

suneet-s

Thanks for the updates @YongGang. I have reviewed the src/main classes.

I am still -1 on this change because of the way the selector class is written.
The Selector contains a selection key and implicit matching conditions. These matching conditions are specific to a usecase, but it is not clear why these specific fields are chosen. IMO this poses a UX problem for people adopting this feature. The other issue with this approach is that we are not able to provide good user validation when a user passes in a key to the task map that is not currently supported.

Please separate the selection key from what is being used to match the task with a selection key into a separate object aka instead of

{
  "selectionKey": "someKey",
  "context.tags": {..},
  "task": {...}
}

make it matching part it's own object

{
  "selectionKey": "someKey",
  "matcher": {
    "type": "myCustomMatcher",
    "context.tags": {..},
    "task": {...}
  }
}

In the example above, I can not think of a good name that would explain why someone would want to use that matcher, which is why I am proposing smaller matchers - like a DataSourceMatcher, TaskTypeMatcher, TagsMatcher, etc.

Regarding your concerns about smaller, more composable matchers:

RE: Complexity of operators: We do not need to introduce any matchers that make it harder to understand what the rules are trying to say. The minimum work needed in this patch is to provide an and matcher, a tags matcher, a dataSource matcher and a taskType matcher. It is no more complex than the current proposal with these 4 matchers.
RE: Relevance of Typed matchers: I do not understand this concern.
RE: small jackson classes / code bloat: Smaller classes are easier to test and maintain as there is less logic in it. I do not understand the code bloat concern.

Also, please limit the use of nulls in the code. Please try to make everything non-null as this makes it less likely to introduce null handling bugs. If something really needs to be nullable, consider using Optionals as it forces the devs and the reviewers to think about what to do when the optional is absent.

...verlord-extensions/src/main/java/org/apache/druid/k8s/overlord/KubernetesOverlordModule.java

...lord-extensions/src/main/java/org/apache/druid/k8s/overlord/common/KubernetesPeonClient.java

.../main/java/org/apache/druid/k8s/overlord/execution/DynamicTaskPodTemplateSelectStrategy.java

...ensions/src/main/java/org/apache/druid/k8s/overlord/execution/PodTemplateSelectStrategy.java

...main/java/org/apache/druid/k8s/overlord/execution/KubernetesTaskExecutionConfigResource.java

YongGang · 2024-06-07T22:08:17Z

Thanks @suneet-s for your comments:

The Selector contains a selection key and implicit matching conditions. These matching conditions are specific to a usecase, but it is not clear why these specific fields are chosen.

If we rename Selector to TaskPropertiesSelector should address this concern?

For this following suggestion:

{
  "selectionKey": "someKey",
  "matcher": {
    "type": "myCustomMatcher",
    "context.tags": {..},
    "task": {...}
  }
}

are we actually suggesting to do this?:

{
  "selectionKey": "someKey",
  "matchers": [
    {
      "type": "dataSource",
      "matchingNames": ["ds0"]
    },
    {
      "type": "context",
      "field": "myContextKey"
      "matchingNames": ["anyValue"]
    },
  ]
}

Otherwise if we still have a single matcher within a selector, I'm not sure about the benefit of introducing another layer of Jackson object.

So the full dynamic config based on the one from PR description will be like:

{
  "type": "default",
  "podTemplateSelectStrategy": {
    "type": "dynamicTask",
    "templateSelectors": [
      {
        "selectionKey": "low-throughput",
        "matchers": [
          {
            "type": "context.tags",
            "field": "billingCategory"
            "matchingNames": ["streaming_ingestion"]
          },
          {
            "type": "dataSource",
            "matchingNames": ["wikipedia"]
          },
        ]
      },
      {
        "selectionKey": "medium-throughput",
        "matchers": [
          {
            "type": "type",
            "matchingNames": ["index_kafka"]
          },
        ]
      }
    ]
  }
}

There are 3 level of arrays in the config, want to make sure that's the target one we'd like to have.

YongGang · 2024-06-10T18:52:21Z

@suneet-s I have addressed your comments, please take a look. Thanks.

suneet-s

Thanks for incorporating the suggestions @YongGang

...in/java/org/apache/druid/k8s/overlord/execution/TaskPropertiesPodTemplateSelectStrategy.java

YongGang added 3 commits May 27, 2024 22:21

initial commit

b74e5b8

add Javadocs

615669b

refine JSON input config

f34033a

github-actions bot added the Kubernetes label May 29, 2024

arunramani reviewed May 29, 2024

View reviewed changes

...etes-overlord-extensions/src/main/java/org/apache/druid/k8s/overlord/execution/Selector.java Outdated Show resolved Hide resolved

arunramani reviewed May 29, 2024

View reviewed changes

...tensions/src/main/java/org/apache/druid/k8s/overlord/taskadapter/PodTemplateTaskAdapter.java Outdated Show resolved Hide resolved

georgew5656 reviewed May 30, 2024

View reviewed changes

more test and fix build

c3b0312

extract existing behavior as default strategy

ee9b8d1

github-actions bot added the Area - Dependencies label Jun 3, 2024

YongGang marked this pull request as ready for review June 3, 2024 05:11

georgew5656 reviewed Jun 3, 2024

View reviewed changes

...tensions/src/main/java/org/apache/druid/k8s/overlord/taskadapter/PodTemplateTaskAdapter.java Outdated Show resolved Hide resolved

YongGang added 2 commits June 3, 2024 11:00

change template mapping fallback

0dd6df2

add docs

de59b8a

github-actions bot added the Area - Documentation label Jun 4, 2024

georgew5656 reviewed Jun 4, 2024

View reviewed changes

docs/development/extensions-contrib/k8s-jobs.md Outdated Show resolved Hide resolved

georgew5656 approved these changes Jun 4, 2024

View reviewed changes

YongGang added 2 commits June 4, 2024 09:41

update doc

b4b3c31

fix doc

07c0209

suneet-s added the Design Review label Jun 4, 2024

suneet-s requested changes Jun 5, 2024

View reviewed changes

address comments

903d815

suneet-s requested changes Jun 7, 2024

View reviewed changes

YongGang added 3 commits June 10, 2024 09:30

define Matcher interface

d882598

fix test coverage

e1c9711

use lower case for endpoint path

8e9308e

update Json name

c71c805

YongGang added 2 commits June 10, 2024 14:58

add more tests

18432bf

refactoring Selector class

57e2831

suneet-s approved these changes Jun 12, 2024

View reviewed changes

...in/java/org/apache/druid/k8s/overlord/execution/TaskPropertiesPodTemplateSelectStrategy.java Outdated Show resolved Hide resolved

suneet-s merged commit 46dbc74 into apache:master Jun 12, 2024
88 checks passed

suneet-s mentioned this pull request Jun 13, 2024

Update docs for K8s TaskRunner Dynamic Config #16600

Merged

2 tasks

kfaraz added this to the 31.0.0 milestone Oct 4, 2024

kfaraz mentioned this pull request Oct 11, 2024

[DRAFT] 31.0.0 Release Notes #17332

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Dynamic Peon Pod Template Selection in K8s extension #16510

Support Dynamic Peon Pod Template Selection in K8s extension #16510

YongGang commented May 29, 2024 •

edited

Loading

georgew5656 May 30, 2024

YongGang May 31, 2024

georgew5656 May 31, 2024 •

edited

Loading

YongGang May 31, 2024

georgew5656 Jun 3, 2024

YongGang Jun 3, 2024

suneet-s Jun 5, 2024

suneet-s Jun 6, 2024 •

edited

Loading

arunramani commented May 31, 2024

YongGang commented May 31, 2024

suneet-s left a comment •

edited

Loading

suneet-s Jun 5, 2024

suneet-s Jun 5, 2024

suneet-s Jun 5, 2024

suneet-s Jun 5, 2024

suneet-s Jun 5, 2024

suneet-s Jun 5, 2024

suneet-s Jun 5, 2024

suneet-s Jun 5, 2024

YongGang commented Jun 6, 2024

suneet-s left a comment

YongGang commented Jun 7, 2024 •

edited

Loading

YongGang commented Jun 10, 2024

suneet-s left a comment


		import java.util.Objects;

		public class DefaultExecutionConfig implements ExecutionConfig

		@Path("/druid/indexer/v1/k8s/runner")
		public class KubernetesResource

Support Dynamic Peon Pod Template Selection in K8s extension #16510

Support Dynamic Peon Pod Template Selection in K8s extension #16510

Conversation

YongGang commented May 29, 2024 • edited Loading

Description

Example Configuration:

Release note

Key changed/added classes in this PR

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georgew5656 May 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suneet-s Jun 6, 2024 • edited Loading

Choose a reason for hiding this comment

arunramani commented May 31, 2024

YongGang commented May 31, 2024

suneet-s left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YongGang commented Jun 6, 2024

suneet-s left a comment

Choose a reason for hiding this comment

YongGang commented Jun 7, 2024 • edited Loading

YongGang commented Jun 10, 2024

suneet-s left a comment

Choose a reason for hiding this comment

YongGang commented May 29, 2024 •

edited

Loading

georgew5656 May 31, 2024 •

edited

Loading

suneet-s Jun 6, 2024 •

edited

Loading

suneet-s left a comment •

edited

Loading

YongGang commented Jun 7, 2024 •

edited

Loading