-
Notifications
You must be signed in to change notification settings - Fork 28.9k
[SPARK-33261][K8S] Add a developer API for custom feature steps #30206
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SPARK-33261][K8S] Add a developer API for custom feature steps #30206
Conversation
Test build #130463 has finished for PR 30206 at commit
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kubernetes integration test starting |
Kubernetes integration test status success |
That's a great suggestion @dongjoon-hyun , I'll try and get that done today :) |
Test build #130477 has finished for PR 30206 at commit
|
Kubernetes integration test starting |
Kubernetes integration test status success |
...ernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesDriverBuilder.scala
Outdated
Show resolved
Hide resolved
...s/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/KubernetesExecutorBuilder.scala
Outdated
Show resolved
Hide resolved
...es/core/src/test/scala/org/apache/spark/deploy/k8s/submit/KubernetesDriverBuilderSuite.scala
Outdated
Show resolved
Hide resolved
...ce-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/PodBuilderSuite.scala
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for the updates. It's great. I left a few minor comments about naming and a new test case.
...s/core/src/main/scala/org/apache/spark/deploy/k8s/features/KubernetesFeatureConfigStep.scala
Show resolved
Hide resolved
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Config.scala
Outdated
Show resolved
Hide resolved
a few other things is that SparkPod is private[spark] as well. This I think opens it up for user to do anything with the Pod, correct me if I'm wrong, which is good and possibly bad. Are there any security concerns here? Like seeing secrets they shouldn't be able to. I can't think of any in the Spark ones directly but wanted to ask more expert in the k8s area. I'm curious, what sort of thing can't be done via templates? |
So since we allow the user to specify templates anyways I don't see this as opening any additional security concerns. As for why templates are not enough, for some deployments I believe people want to be able to customize the pod at the end of the feature steps rather than at the start. I think also there are situations where we need to generate stuff based on pod name. |
Let's open up SparkPod to an Experimental dev API. WDYT @tgravescs ? |
yeah that seems fine to me to make SparkPod developer api |
Since we have an agreement, could you resolve the conflict to proceed further, @holdenk ? Also, please update the PR according to the existing comments. |
09febce
to
2117566
Compare
Thank you so much for your update, @holdenk ! |
is this targeted for 3.1? |
@tgravescs . I believe so because this is a part of K8s GA in Apache Spark 3.1. |
Please let us know if you have any concerns on this. We can postpone it to Apache Spark 3.2 if you want to do so. |
Test build #132510 has finished for PR 30206 at commit
|
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/SparkPod.scala
Outdated
Show resolved
Hide resolved
963b786
to
e990f33
Compare
Test build #132679 has finished for PR 30206 at commit
|
Kubernetes integration test starting |
Kubernetes integration test status success |
Test build #132684 has finished for PR 30206 at commit
|
Kubernetes integration test starting |
Kubernetes integration test status success |
Merging to the current dev branch for 3.2. |
### What changes were proposed in this pull request? Add a developer API for custom driver & executor feature steps. ### Why are the changes needed? While we allow templates for the basis of pod creation, some deployments need more flexibility in how the pods are configured. This adds a developer API for custom deployments. ### Does this PR introduce _any_ user-facing change? New developer API. ### How was this patch tested? Extended tests to verify custom step is applied when configured. Closes apache#30206 from holdenk/SPARK-33261-allow-people-to-extend-pod-feature-steps. Authored-by: Holden Karau <hkarau@apple.com> Signed-off-by: Holden Karau <hkarau@apple.com>
…Step developer api ### What changes were proposed in this pull request? This patch adds the support for extending user feature steps with configuration by adding 2 developer api: - `KubernetesDriverCustomFeatureConfigStep`: to help user extend custom feature step in executor side - `KubernetesExecutorCustomFeatureConfigStep`: to help user extend custom feature step in driver side Before this patch user can only add feature step like: - `class TestStep extends KubernetesFeatureConfigStep`: without any kubernetes conf After this patch user can add feature step with configuration like: - `class TestStepWithDriverConf extends KubernetesDriverCustomFeatureConfigStep`: only driver - `class TestStepWithExecConf extends KubernetesExecutorCustomFeatureConfigStep`: only executor - `class TestStepWithK8SConf extends KubernetesDriverCustomFeatureConfigStep with KubernetesExecutorCustomFeatureConfigStep`: both driver and executor ### Why are the changes needed? In #30206 , a developer API for custom feature steps has been added, but it didn't support initialize user feature step with kubernetes conf (like `KubernetesConf`/`KubernetesDriverConf`/`KubernetesExecutorConf`). In most of scenarios, users want to make corresponding changes in their feature steps according to the configuration. Such as, the customized scheduler scenario, user wants to configure pod according to passed job configuration. ### Does this PR introduce _any_ user-facing change? Improve the developer API for for custom feature steps. ### How was this patch tested? - Added UT - Runing k8s integration test manaully: `build/sbt -Pkubernetes -Pkubernetes-integration-tests -Dtest.exclude.tags=minikube,r "kubernetes-integration-tests/test` Closes: #34924 Closes #35345 from Yikun/SPARK-37145-alt. Lead-authored-by: Yikun Jiang <yikunkero@gmail.com> Co-authored-by: Dongjoon Hyun <dongjoon@apache.org> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
…cludedFeatureSteps` ### What changes were proposed in this pull request? This PR aims to support `spark.kubernetes.(driver|executor).pod.excludedFeatureSteps` configuration. ### Why are the changes needed? Since Apache Spark 3.2, we have been providing `spark.kubernetes.(driver|executor).pod.featureSteps`. - #30206 This PR aims to allow users to exclude feature steps selectively by configurations. Please note that this is designed to allow to exclude all steps including both built-in and user-provided steps. ### Does this PR introduce _any_ user-facing change? No because this is a new feature. ### How was this patch tested? Pass the CIs with newly added test cases. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #51522 from dongjoon-hyun/SPARK-52830. Authored-by: Dongjoon Hyun <dongjoon@apache.org> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
…cludedFeatureSteps` ### What changes were proposed in this pull request? This PR aims to support `spark.kubernetes.(driver|executor).pod.excludedFeatureSteps` configuration. ### Why are the changes needed? Since Apache Spark 3.2, we have been providing `spark.kubernetes.(driver|executor).pod.featureSteps`. - apache#30206 This PR aims to allow users to exclude feature steps selectively by configurations. Please note that this is designed to allow to exclude all steps including both built-in and user-provided steps. ### Does this PR introduce _any_ user-facing change? No because this is a new feature. ### How was this patch tested? Pass the CIs with newly added test cases. ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#51522 from dongjoon-hyun/SPARK-52830. Authored-by: Dongjoon Hyun <dongjoon@apache.org> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
What changes were proposed in this pull request?
Add a developer API for custom driver & executor feature steps.
Why are the changes needed?
While we allow templates for the basis of pod creation, some deployments need more flexibility in how the pods are configured. This adds a developer API for custom deployments.
Does this PR introduce any user-facing change?
New developer API.
How was this patch tested?
Extended tests to verify custom step is applied when configured.