You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/02/26 00:51:31 UTC

[GitHub] [spark] yangwwei opened a new pull request #35663: [WIP][SPARK-37809] Add yunikorn feature step

yangwwei opened a new pull request #35663:
URL: https://github.com/apache/spark/pull/35663


   ### What changes were proposed in this pull request?
   Add yunikorn feature step in order to support yunikorn scheduler. [Apache yunikorn](https://yunikorn.apache.org/) is an open-source batch scheduler for K8s, designed to solve the problems of running batch workloads, e.g Spark, on K8s.  This PR will add a build profile yunikorn, the yunikorn feature step, UT, and integration tests.
   
   
   ### Why are the changes needed?
   This is a part of the effort [SPARK-36057](https://issues.apache.org/jira/browse/SPARK-36057), in order to make Spark natively support customized K8s schedulers. This PR particularly is similar to https://github.com/apache/spark/pull/35422.
   
   ### Does this PR introduce _any_ user-facing change?
   Once this is done, user can easily submit their jobs to yunikorn scheduler with the following configs:
   
   ```
   --conf spark.kubernete.driver.scheduler.name=yunikorn \
   --conf spark.kubernetes.driver.pod.featureSteps=org.apache.spark.deploy.k8s.features.scheduler.YuniKornFeatureStep
   ```
   on a yunikorn enabled cluster, the Spark job will be scheduled by the yunikorn scheduler instead of the default scheduler.
   
   ### How was this patch tested?
   
   To be added.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] dongjoon-hyun commented on pull request #35663: [SPARK-37809][K8S] Add yunikorn feature step

Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #35663:
URL: https://github.com/apache/spark/pull/35663#issuecomment-1056083608


   @yangwwei . It seems that you didn't pay attention for the content of my feedbacks.
   > Maybe this PR gives the impression that we just need to add some annotations, 
   
   My first comment was the following https://github.com/apache/spark/pull/35663#pullrequestreview-896742767 . I know you need to add more, but I don't think we need to duplicate Apache Spark like the original PR.
   > Sorry, but I don't think we need this at this stage.
   > - spark.kubernetes.scheduler.name can be used for any custom scheduler name.
   > - We can add any annotation names easily.
   
   My second comment was the following.
   
   > If then, please bring this back with more requirement at phase 2's real use case, @yangwwei .
   > I don't think see any values in this PR including that static annotation, too.
   
   My 3rd comment was 
   
   > It seems that you missed my point. This is just a duplication, @yangwwei . We want to avoid this kind of boiler templating as much as possible. Again, we already support --conf spark.kubernete.driver.scheduler.name=yunikorn in the master branch when we extend for volcano. Please be specific about missing part and what we really need.
   
   My review comments are consistent and the same. Please build the missing part by utilizing the existing one instead by simply claiming that you need the same copy of code again and again. We want to be extensible and support all future custom schedulers instead of duplicating for all customer schedulers.
   
   From my perspective, I'd like to recommend to add a new feature to YuniKorn to support custom ID annotation and allow Spark jobs to specify it to `spark-app-id`. YuniKorn is not a golden standard written on rock. Please improve it more flexible first.
   
   Lastly, we are reviewing the PR piece by piece. Please make a PR meaningful, complete, and reasonable. Unfortunately, I don't think this PR meets the criteria.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] Yikun commented on pull request #35663: [SPARK-37809][K8S] Add yunikorn feature step

Posted by GitBox <gi...@apache.org>.
Yikun commented on pull request #35663:
URL: https://github.com/apache/spark/pull/35663#issuecomment-1056101229


   @yangwwei 
   
   There are two alternative way to support `Yunikorn` at this stage:
   
   - New annotations placeholder support which @dongjoon-hyun  introduced. In theory, all annotations can be set by this way.(such as queue placeholder). It's very flexiable. This is very helpful for schdulers which only need annotaions set.
   
   - A separate feature step (this PR): this will help to simplify annotation configuration, we just need to set feature step to instead of setting many annotations conf. This way it's more friendly to schedulers which need to create CRD and also need many annotaions.
   
   I think dongjoon-hyun's concern is how to use the most simple way to integrate Yunikorn at this stage. And I also think even if we support by this way, `Yunikorn` can also doc in spark website, tell user how to use offcially.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] yangwwei commented on pull request #35663: [SPARK-37809][K8S] Add yunikorn feature step

Posted by GitBox <gi...@apache.org>.
yangwwei commented on pull request #35663:
URL: https://github.com/apache/spark/pull/35663#issuecomment-1055902284


   > It's because master branch already supports the following. I don't see any value addition in this PR at this stage.
   
   Sorry, I should add more context in the beginning.
   The additional thing is the change for adding some pod annotation, like what @holdenk pointed out: https://github.com/apache/spark/pull/35663#discussion_r817131398. And note, this is the base for yunikorn related changes, after we have the yunikorn feature step, I will create another PR to add queue-related configs, that will be tracked in https://issues.apache.org/jira/browse/SPARK-38310. For phase 1 (targeted for Spark 3.3), that's the main stuff to get spark jobs scheduled by yunikorn. For phase 2, there will be more optimizations coming. Hope this clarifies things.
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] yangwwei edited a comment on pull request #35663: [SPARK-37809][K8S] Add yunikorn feature step

Posted by GitBox <gi...@apache.org>.
yangwwei edited a comment on pull request #35663:
URL: https://github.com/apache/spark/pull/35663#issuecomment-1060230905


   Hi @dongjoon-hyun 
   
   I want to work with you and see what is the best way to solve this.  Apologies for this long comment, there are mainly 2 parts: 1) why yunikorn feature step; 2) if not feature step, what's the alternative.
   
   ### 1) why yunikorn feature step
   
   In the [proposal](https://docs.google.com/document/d/1xgQGRpaHQX6-QH_J9YV2C2Dh6RpXefUpLM7KGkzL6Fg), the goal is to provide users a customizable and consistent fashion to use a 3rd party K8s scheduler for Spark, we proposed the following user-facing changes when submitting spark jobs:
   
   ```
   --conf spark.kubernetes.driver.scheduler.name=xxx 
   --conf spark.kubernetes.driver.pod.featureSteps=org.apache.spark.deploy.k8s.features.scheduler.XxxFeatureStep
   --conf spark.kubernetes.job.queue default
   --conf spark.kubernetes.job.min.cpu 4
   --conf spark.kubernetes.job.min.memory 8G
   ```
   
   both Volcano and YuniKorn will honor these configs and set up driver/executor pods accordingly via feature step. That's why I was waiting for @Yikun to finish the general stuff in the K8s feature step code implementation and then submitted this PR. For yunikorn side logic, there is no major difference from Volcano. We need to set up a few pod annotations for appID, job queue, and also K8s CRD. In the case of yunikorn, it is application.yunikorn.apache.org, [CRD definition](https://github.com/apache/incubator-yunikorn-release/blob/master/helm-charts/yunikorn/templates/crds/application-definition.yaml). The only difference is, PodGroup is a mandatory resource required by Volcano, but app CRD is optional in YuniKorn. So in the 1st phase, my PR doesn't introduce the CRD creation, but at least we have the basic integration working.  BTW, yunikorn has already passed the ASF graduation votes, so it will become to be an Apache TLP in a few weeks.
   
    ### 2) if not feature step, what's the alternative
   
   @Yikun summarize the alternative [here](https://github.com/apache/spark/pull/35663#issuecomment-1056101229): use the annotation placeholders introduced via: https://github.com/apache/spark/pull/35704. I looked into this approach, that looks like we will need to set up something like:
   
    ```
   --conf spark.kubernetes.driver.scheduler.name=yunikorn 
   --conf spark.kubernetes.driver.annotation.yunikorn.apache.org/app-id={{APP_ID}}
   --conf spark.kubernetes.executor.annotation.yunikorn.apache.org/app-id={{APP_ID}}
   --conf spark.kubernetes.job.queue default
   --conf spark.kubernetes.job.min.cpu 4
   --conf spark.kubernetes.job.min.memory 8G
   ```
   
   this can work for the 1st phase. However, I am not sure how to achieve our 2nd phase target when the [CRD](https://github.com/apache/incubator-yunikorn-release/blob/master/helm-charts/yunikorn/templates/crds/application-definition.yaml) is introduced. Are you suggesting for the 1st phase we use this approach, and add the feature step in the 2nd phase? Will that lead to a different user experience for the end-users?
   
   I really appreciate it if you can share your thoughts. Thanks!
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] dongjoon-hyun commented on pull request #35663: [SPARK-37809][K8S] Add yunikorn feature step

Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #35663:
URL: https://github.com/apache/spark/pull/35663#issuecomment-1055892668


   > > Sorry, but I don't think we need this at this stage.
   > > 
   > > * `spark.kubernetes.scheduler.name` can be used for any custom scheduler name.
   > > * We can add any annotation names easily.
   > 
   > I am sorry I did not get your point. This is the same effort as what was done for volcano feature step #35422, a separate feature step for yunikorn. This was proposed together in the SPIP doc, why this is not needed?
   
   It's because `master` branch already supports the following. I don't see any value addition in this PR at this stage.
   ```
   --conf spark.kubernete.driver.scheduler.name=yunikorn \
   ```


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] yangwwei commented on pull request #35663: [SPARK-37809][K8S] Add yunikorn feature step

Posted by GitBox <gi...@apache.org>.
yangwwei commented on pull request #35663:
URL: https://github.com/apache/spark/pull/35663#issuecomment-1055974119


   hi @dongjoon-hyun 
   
   Sorry, I am not convinced. Without this, there is no integration with yunikorn. Volcano side changes already got merged, this is a very similar change. With this PR and the coming one for https://issues.apache.org/jira/browse/SPARK-38310. Users will have a very simple (and consistent) way to submit Spark jobs to yunikorn enabled cluster, that's one of the goals of [SPIP Spark-36057 Support Customized Kubernetes Schedulers Proposal](https://docs.google.com/document/d/1xgQGRpaHQX6-QH_J9YV2C2Dh6RpXefUpLM7KGkzL6Fg). Why do we have to wait?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins removed a comment on pull request #35663: [SPARK-37809][K8S] Add yunikorn feature step

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #35663:
URL: https://github.com/apache/spark/pull/35663#issuecomment-1053569329


   Can one of the admins verify this patch?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] martin-g commented on pull request #35663: [SPARK-37809][K8S] Add yunikorn feature step

Posted by GitBox <gi...@apache.org>.
martin-g commented on pull request #35663:
URL: https://github.com/apache/spark/pull/35663#issuecomment-1060357182


   @yangwwei I guess it was just a typo and copy paste error but `--conf spark.kubernete...` should be `--conf spark.kubernetes...`
   Also the `min` related configs should be `minCPU` and `minMemory`. There is no namespace `min`.
   Please also check whether [podTemplate](https://spark.apache.org/docs/3.2.1/running-on-kubernetes.html#pod-template) might be used for Yunikorn integration needs.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] yangwwei commented on pull request #35663: [SPARK-37809][K8S] Add yunikorn feature step

Posted by GitBox <gi...@apache.org>.
yangwwei commented on pull request #35663:
URL: https://github.com/apache/spark/pull/35663#issuecomment-1055993657


   For the scheduler name, that's fine. In this PR, the addition is we need to add this:
   
   > addToAnnotations(YuniKornFeatureStep.AppIdAnnotationKey, kubernetesConf.appId)
   
   this is needed in order to let yunikorn know which pods belong to which job. Otherwise, it won't be able to be scheduled. In the next PR for queue-related configs, I will add some other logic to load the queue configs from conf and add that to the pod annotation. 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] Yikun edited a comment on pull request #35663: [SPARK-37809][K8S] Add yunikorn feature step

Posted by GitBox <gi...@apache.org>.
Yikun edited a comment on pull request #35663:
URL: https://github.com/apache/spark/pull/35663#issuecomment-1056101229


   @yangwwei 
   
   There are two alternative way to support `Yunikorn` at this stage:
   
   - New annotation placeholder support which @dongjoon-hyun  introduced. In theory, all annotations can be set this way.(such as queue placeholder). It's very flexible. This is very helpful for custom schedulers which only need annotations set.
   
   - A separate feature step (this PR): this will help to simplify annotation configuration, we just need to set feature step to instead of setting many annotations conf. This way it's more friendly to schedulers which need to create extra CRD and also need many annotations.
   
   I think dongjoon-hyun's concern is how to use the most simple way to integrate Yunikorn at this stage. And I also think even if we support by this way, spark with `Yunikorn` can also doc in spark, telling users how to use it officially.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] martin-g commented on a change in pull request #35663: [SPARK-37809][K8S] Add yunikorn feature step

Posted by GitBox <gi...@apache.org>.
martin-g commented on a change in pull request #35663:
URL: https://github.com/apache/spark/pull/35663#discussion_r817489457



##########
File path: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/features/YuniKornFeatureStepSuite.scala
##########
@@ -0,0 +1,43 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.deploy.k8s.features
+
+import org.apache.spark.{SparkConf, SparkFunSuite}
+import org.apache.spark.deploy.k8s._
+
+class YuniKornFeatureStepSuite extends SparkFunSuite {
+
+  test("SPARK-37809: Driver Pod with YuniKorn annotations") {

Review comment:
       should these tests also use `yunikornTag` ?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] dongjoon-hyun commented on pull request #35663: [SPARK-37809][K8S] Add yunikorn feature step

Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #35663:
URL: https://github.com/apache/spark/pull/35663#issuecomment-1056035098


   Here is my answer. I believe this is the desirable way which Apache Spark wants to go, @yangwwei . We want to extensible and support all future custom scheduler instead of locking in any specific scheduler.
   - https://github.com/apache/spark/pull/35704


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] yangwwei commented on pull request #35663: [SPARK-37809][K8S] Add yunikorn feature step

Posted by GitBox <gi...@apache.org>.
yangwwei commented on pull request #35663:
URL: https://github.com/apache/spark/pull/35663#issuecomment-1056075063


   hi @dongjoon-hyun  thank you for sharing your thoughts in the [PR](https://github.com/apache/spark/pull/35704).
   
   Maybe this PR gives the impression that we just need to add some annotations, but the actual integration will be much more complicated than that. AppID and queue name are the very first thing for integrating with a scheduler, to take advantage of the scheduler features, there are more than that. If you look at [this section in the SPIP doc](https://docs.google.com/document/d/1xgQGRpaHQX6-QH_J9YV2C2Dh6RpXefUpLM7KGkzL6Fg/edit#heading=h.s1rofza4711d),  it gives some more context. A full story will require us to support many things that (most of them) are already supported in YARN, such as priority, preemption, gang scheduling, etc. For these features, we will need to add more logic to tweak pod spec, or add additional K8s resources. And different scheduler implementations have different semantics to support these features. That's why we want to introduce scheduler feature step, in order to customize this with e.g VolcanoFeatureStep, YuniKornFeatureStep. The 1st phase for yunikorn, as
  well as volcano, is simple: let the Spark job be able to be scheduled by a customized scheduler natively. But it doesn't stop here, based on the added feature step, we can do more integration in the 2nd, 3rd phases. Hope this clarifies things. Thanks!


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] yangwwei edited a comment on pull request #35663: [SPARK-37809][K8S] Add yunikorn feature step

Posted by GitBox <gi...@apache.org>.
yangwwei edited a comment on pull request #35663:
URL: https://github.com/apache/spark/pull/35663#issuecomment-1060230905


   Hi @dongjoon-hyun 
   
   I want to work with you and see what is the best way to solve this.  Apologies for this long comment, there are mainly 2 parts: 1) why yunikorn feature step; 2) if not feature step, what's the alternative.
   
   ### 1) why yunikorn feature step
   
   In the [proposal](https://docs.google.com/document/d/1xgQGRpaHQX6-QH_J9YV2C2Dh6RpXefUpLM7KGkzL6Fg), the goal is to provide users a customizable and consistent fashion to use a 3rd party K8s scheduler for Spark, we proposed the following user-facing changes when submitting spark jobs:
   
   ```
   --conf spark.kubernete.driver.scheduler.name=xxx 
   --conf spark.kubernetes.driver.pod.featureSteps=org.apache.spark.deploy.k8s.features.scheduler.XxxFeatureStep
   --conf spark.kubernete.job.queue default
   --conf spark.kubernete.job.min.cpu 4
   --conf spark.kubernete.job.min.memory 8G
   ```
   
   both Volcano and YuniKorn will honor these configs and set up driver/executor pods accordingly via feature step. That's why I was waiting for @Yikun to finish the general stuff in the K8s feature step code implementation and then submitted this PR. For yunikorn side logic, there is no major difference from Volcano. We need to set up a few pod annotations for appID, job queue, and also K8s CRD. In the case of yunikorn, it is application.yunikorn.apache.org, [CRD definition](https://github.com/apache/incubator-yunikorn-release/blob/master/helm-charts/yunikorn/templates/crds/application-definition.yaml). The only difference is, PodGroup is a mandatory resource required by Volcano, but app CRD is optional in YuniKorn. So in the 1st phase, my PR doesn't introduce the CRD creation, but at least we have the basic integration working.
   
    ### 2) if not feature step, what's the alternative
   
   @Yikun summarize the alternative [here](https://github.com/apache/spark/pull/35663#issuecomment-1056101229): use the annotation placeholders introduced via: https://github.com/apache/spark/pull/35704. I looked into this approach, that looks like we will need to set up something like:
   
    ```
   --conf spark.kubernete.driver.scheduler.name=yunikorn 
   --conf spark.kubernetes.driver.annotation.yunikorn.apache.org/app-id={{APP_ID}}
   --conf spark.kubernetes.executor.annotation.yunikorn.apache.org/app-id={{APP_ID}}
   --conf spark.kubernete.job.queue default
   --conf spark.kubernete.job.min.cpu 4
   --conf spark.kubernete.job.min.memory 8G
   ```
   
   this can work for the 1st phase. However, I am not sure how to achieve our 2nd phase target when the [CRD](https://github.com/apache/incubator-yunikorn-release/blob/master/helm-charts/yunikorn/templates/crds/application-definition.yaml) is introduced. Are you suggesting for the 1st phase we use this approach, and add the feature step in the 2nd phase? Will that lead to a different user experience for the end-users?
   
   I really appreciate it if you can share your thoughts. Thanks!
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] yangwwei commented on pull request #35663: [SPARK-37809][K8S] Add yunikorn feature step

Posted by GitBox <gi...@apache.org>.
yangwwei commented on pull request #35663:
URL: https://github.com/apache/spark/pull/35663#issuecomment-1060928826


   > @yangwwei I guess it was just a typo and copy paste error but `--conf spark.kubernete...` should be `--conf spark.kubernetes...` Also the `min` related configs should be `minCPU` and `minMemory`. There is no namespace `min`. Please also check whether [podTemplate](https://spark.apache.org/docs/3.2.1/running-on-kubernetes.html#pod-template) might be used for Yunikorn integration needs.
   
   Thanks!! Sorry for being lazy, I just copy them from the proposal doc. Fixed the typos in the doc as well.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] martin-g commented on a change in pull request #35663: [SPARK-37809] Add yunikorn feature step

Posted by GitBox <gi...@apache.org>.
martin-g commented on a change in pull request #35663:
URL: https://github.com/apache/spark/pull/35663#discussion_r815622354



##########
File path: project/SparkBuild.scala
##########
@@ -968,6 +973,12 @@ object Volcano {
   )
 }
 
+object YuniKorn {
+  // Exclude all yunikorn file for Compile and Test
+  lazy val settings = Seq(
+    unmanagedSources / excludeFilter := HiddenFileFilter || "*YuniKorn*.scala")

Review comment:
       @Yikun it seems `volcano` needs the same improvement.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] dongjoon-hyun commented on pull request #35663: [SPARK-37809][K8S] Add yunikorn feature step

Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #35663:
URL: https://github.com/apache/spark/pull/35663#issuecomment-1055846355


   > Jenkins ok to test
   
   @holdenk . As you know, AMP lab infra is gone.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] Yikun edited a comment on pull request #35663: [SPARK-37809][K8S] Add yunikorn feature step

Posted by GitBox <gi...@apache.org>.
Yikun edited a comment on pull request #35663:
URL: https://github.com/apache/spark/pull/35663#issuecomment-1056101229


   @yangwwei 
   
   There are two alternative way to support `Yunikorn` at this stage:
   
   - New annotation placeholder support which @dongjoon-hyun  introduced. In theory, all annotations can be set this way.(such as queue placeholder). It's very flexible. This is very helpful for custom schedulers which only need annotations set.
   
   - A separate feature step (this PR): this will help to simplify annotation configuration, we just need to set feature step to instead of setting many annotations conf. This way it's more friendly to schedulers which need to create extra CRD and also need many annotations.
   
   I think dongjoon-hyun's concern is how to use the most simple way to integrate Yunikorn at this stage. And I also think even if we support by any way, spark with `Yunikorn` can also doc in spark, telling users how to use it officially.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] yangwwei commented on a change in pull request #35663: [SPARK-37809] Add yunikorn feature step

Posted by GitBox <gi...@apache.org>.
yangwwei commented on a change in pull request #35663:
URL: https://github.com/apache/spark/pull/35663#discussion_r816549163



##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/YuniKornFeatureStep.scala
##########
@@ -0,0 +1,46 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.deploy.k8s.features
+
+import io.fabric8.kubernetes.api.model._
+
+import org.apache.spark.deploy.k8s.{KubernetesConf, KubernetesDriverConf, KubernetesExecutorConf, SparkPod}
+
+private[spark] class YuniKornFeatureStep extends KubernetesDriverCustomFeatureConfigStep
+  with KubernetesExecutorCustomFeatureConfigStep {
+
+  private var kubernetesConf: KubernetesConf = _
+
+  private val YUNIKORN_APP_ID_ANNOTATION = "yunikorn.apache.org/app-id"

Review comment:
       Good suggestion, just get that done in the last commit. Thanks, @martin-g .




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] Yikun commented on a change in pull request #35663: [WIP][SPARK-37809] Add yunikorn feature step

Posted by GitBox <gi...@apache.org>.
Yikun commented on a change in pull request #35663:
URL: https://github.com/apache/spark/pull/35663#discussion_r815313261



##########
File path: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/features/YuniKornFeatureStepSuite.scala
##########
@@ -0,0 +1,43 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.deploy.k8s.features
+
+import org.apache.spark.{SparkConf, SparkFunSuite}
+import org.apache.spark.deploy.k8s._
+
+class YuniKornFeatureStepSuite extends SparkFunSuite {
+
+  test("SPARK-37809: Driver Pod with YuniKorn labels") {
+    val sparkConf = new SparkConf()
+    val kubernetesConf = KubernetesTestConf.createDriverConf(sparkConf)
+    val step = new YuniKornFeatureStep()
+    step.init(kubernetesConf)
+    val configuredPod = step.configurePod(SparkPod.initialPod())
+    val annotations = configuredPod.pod.getMetadata.getAnnotations
+    assert(annotations.get("yunikorn.apache.org/app-id") === ${kubernetesConf.appId})

Review comment:
       ${kubernetesConf.appId} => kubernetesConf.appId

##########
File path: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/features/YuniKornFeatureStepSuite.scala
##########
@@ -0,0 +1,43 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.deploy.k8s.features
+
+import org.apache.spark.{SparkConf, SparkFunSuite}
+import org.apache.spark.deploy.k8s._
+
+class YuniKornFeatureStepSuite extends SparkFunSuite {
+
+  test("SPARK-37809: Driver Pod with YuniKorn labels") {

Review comment:
       nit: labels -> annotations

##########
File path: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/features/YuniKornFeatureStepSuite.scala
##########
@@ -0,0 +1,43 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.deploy.k8s.features
+
+import org.apache.spark.{SparkConf, SparkFunSuite}
+import org.apache.spark.deploy.k8s._
+
+class YuniKornFeatureStepSuite extends SparkFunSuite {
+
+  test("SPARK-37809: Driver Pod with YuniKorn labels") {
+    val sparkConf = new SparkConf()
+    val kubernetesConf = KubernetesTestConf.createDriverConf(sparkConf)
+    val step = new YuniKornFeatureStep()
+    step.init(kubernetesConf)
+    val configuredPod = step.configurePod(SparkPod.initialPod())
+    val annotations = configuredPod.pod.getMetadata.getAnnotations
+    assert(annotations.get("yunikorn.apache.org/app-id") === ${kubernetesConf.appId})
+  }
+
+  test("SPARK-37809: Executor Pod with YuniKorn labels") {

Review comment:
       same

##########
File path: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/features/YuniKornFeatureStepSuite.scala
##########
@@ -0,0 +1,43 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.deploy.k8s.features
+
+import org.apache.spark.{SparkConf, SparkFunSuite}
+import org.apache.spark.deploy.k8s._
+
+class YuniKornFeatureStepSuite extends SparkFunSuite {
+
+  test("SPARK-37809: Driver Pod with YuniKorn labels") {
+    val sparkConf = new SparkConf()
+    val kubernetesConf = KubernetesTestConf.createDriverConf(sparkConf)
+    val step = new YuniKornFeatureStep()
+    step.init(kubernetesConf)
+    val configuredPod = step.configurePod(SparkPod.initialPod())
+    val annotations = configuredPod.pod.getMetadata.getAnnotations
+    assert(annotations.get("yunikorn.apache.org/app-id") === ${kubernetesConf.appId})
+  }
+
+  test("SPARK-37809: Executor Pod with YuniKorn labels") {
+    val sparkConf = new SparkConf()
+    val kubernetesConf = KubernetesTestConf.createExecutorConf(sparkConf)
+    val step = new YuniKornFeatureStep()
+    step.init(kubernetesConf)
+    val configuredPod = step.configurePod(SparkPod.initialPod())
+    val annotations = configuredPod.pod.getMetadata.getAnnotations
+    assert(annotations.get("yunikorn.apache.org/app-id") === ${kubernetesConf.appId})

Review comment:
       same

##########
File path: resource-managers/kubernetes/integration-tests/src/test/scala/org/apache/spark/deploy/k8s/integrationtest/YuniKornSuite.scala
##########
@@ -0,0 +1,33 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.deploy.k8s.integrationtest
+
+import org.scalatest.Tag
+
+class YuniKornSuite extends KubernetesSuite with YuniKornTestsSuite {
+
+  override protected def setUpTest(): Unit = {
+    super.setUpTest()
+    sparkAppConf
+      .set("spark.kubernetes.driver.scheduler.name", "yunikorn")
+      .set("spark.kubernetes.executor.scheduler.name", "yunikorn")

Review comment:
       nit: these two configurations can be instead by `spark.kubernetes.scheduler.name`
   
   I will also update in volcano.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] yangwwei commented on a change in pull request #35663: [WIP][SPARK-37809] Add yunikorn feature step

Posted by GitBox <gi...@apache.org>.
yangwwei commented on a change in pull request #35663:
URL: https://github.com/apache/spark/pull/35663#discussion_r815403698



##########
File path: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/features/YuniKornFeatureStepSuite.scala
##########
@@ -0,0 +1,43 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.deploy.k8s.features
+
+import org.apache.spark.{SparkConf, SparkFunSuite}
+import org.apache.spark.deploy.k8s._
+
+class YuniKornFeatureStepSuite extends SparkFunSuite {
+
+  test("SPARK-37809: Driver Pod with YuniKorn labels") {
+    val sparkConf = new SparkConf()
+    val kubernetesConf = KubernetesTestConf.createDriverConf(sparkConf)
+    val step = new YuniKornFeatureStep()
+    step.init(kubernetesConf)
+    val configuredPod = step.configurePod(SparkPod.initialPod())
+    val annotations = configuredPod.pod.getMetadata.getAnnotations
+    assert(annotations.get("yunikorn.apache.org/app-id") === ${kubernetesConf.appId})

Review comment:
       done




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] Yikun edited a comment on pull request #35663: [SPARK-37809][K8S] Add yunikorn feature step

Posted by GitBox <gi...@apache.org>.
Yikun edited a comment on pull request #35663:
URL: https://github.com/apache/spark/pull/35663#issuecomment-1056101229


   @yangwwei 
   
   There are two alternative way to support `Yunikorn` at this stage:
   
   - New annotations placeholder support which @dongjoon-hyun  introduced. In theory, all annotations can be set by this way.(such as queue placeholder). It's very flexiable. This is very helpful for custom schdulers which only need annotaions set.
   
   - A separate feature step (this PR): this will help to simplify annotation configuration, we just need to set feature step to instead of setting many annotations conf. This way it's more friendly to schedulers which need to **create extra CRD and also need many annotaions**.
   
   I think dongjoon-hyun's concern is how to use the most simple way to integrate Yunikorn at this stage. And I also think even if we support by this way, `Yunikorn` can also doc in spark website, tell user how to use offcially.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] yangwwei commented on a change in pull request #35663: [SPARK-37809][K8S] Add yunikorn feature step

Posted by GitBox <gi...@apache.org>.
yangwwei commented on a change in pull request #35663:
URL: https://github.com/apache/spark/pull/35663#discussion_r817159354



##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/YuniKornFeatureStep.scala
##########
@@ -0,0 +1,49 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.deploy.k8s.features
+
+import io.fabric8.kubernetes.api.model._
+
+import org.apache.spark.deploy.k8s.{KubernetesConf, KubernetesDriverConf, KubernetesExecutorConf, SparkPod}
+
+private[spark] class YuniKornFeatureStep extends KubernetesDriverCustomFeatureConfigStep
+  with KubernetesExecutorCustomFeatureConfigStep {
+
+  private var kubernetesConf: KubernetesConf = _
+
+  override def init(config: KubernetesDriverConf): Unit = {
+    kubernetesConf = config
+  }
+
+  override def init(config: KubernetesExecutorConf): Unit = {
+    kubernetesConf = config
+  }
+
+  override def configurePod(pod: SparkPod): SparkPod = {
+    val k8sPodBuilder = new PodBuilder(pod.pod)
+      .editMetadata()
+      .addToAnnotations(YuniKornFeatureStep.AppIdAnnotationKey, kubernetesConf.appId)

Review comment:
       Correct. This is the one part needed to get this working for yunikorn. Except for this one, there is another one needed: https://issues.apache.org/jira/browse/SPARK-38310, I will be working on it once this one gets merged.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins commented on pull request #35663: [SPARK-37809] Add yunikorn feature step

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #35663:
URL: https://github.com/apache/spark/pull/35663#issuecomment-1053569329


   Can one of the admins verify this patch?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] Yikun edited a comment on pull request #35663: [SPARK-37809][K8S] Add yunikorn feature step

Posted by GitBox <gi...@apache.org>.
Yikun edited a comment on pull request #35663:
URL: https://github.com/apache/spark/pull/35663#issuecomment-1056101229


   @yangwwei 
   
   There are two alternative way to support `Yunikorn` at this stage:
   
   - New annotation placeholder support which @dongjoon-hyun  introduced. In theory, all annotations can be set this way.(such as queue placeholder). It's very flexible. This is very helpful for custom schedulers which only need annotations set.
   
   - A separate feature step (this PR): this will help to simplify annotation configuration, we just need to set feature step to instead of setting many annotations conf. This way it's more friendly to schedulers which need to create extra CRD and also need many annotations.
   
   I think dongjoon-hyun's concern is how to use the most simple way to integrate Yunikorn at this stage. And I also think even if we support by this way, `Yunikorn` can also doc in spark website, tell user how to use officially.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] Yikun edited a comment on pull request #35663: [SPARK-37809][K8S] Add yunikorn feature step

Posted by GitBox <gi...@apache.org>.
Yikun edited a comment on pull request #35663:
URL: https://github.com/apache/spark/pull/35663#issuecomment-1056101229


   @yangwwei 
   
   There are two alternative way to support `Yunikorn` at this stage:
   
   - New annotation placeholder support which @dongjoon-hyun  introduced. In theory, all annotations can be set this way.(such as queue placeholder). It's very flexible. This is very helpful for custom schedulers which only need annotations set.
   
   - A separate feature step (this PR): this will help to simplify annotation configuration, we just need to set feature step to instead of setting many annotations conf. This way it's more friendly to schedulers which need to create extra CRD and also need many annotations.
   
   I think dongjoon-hyun's concern is how to use the most simple way to integrate Yunikorn at this stage. And I also think even if we support by this way, spark with `Yunikorn` can also doc in spark website, tell user how to use officially.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] yangwwei commented on pull request #35663: [SPARK-37809][K8S] Add yunikorn feature step

Posted by GitBox <gi...@apache.org>.
yangwwei commented on pull request #35663:
URL: https://github.com/apache/spark/pull/35663#issuecomment-1060230905


   Hi @dongjoon-hyun 
   
   I want to work with you and see what is the best way to solve this.  Apologies for this long comment, there are mainly 2 parts: 1) why yunikorn feature step; 2) if not feature step, what's the alternative.
   
   ### 1) why yunikorn feature step
   
   In the [proposal](https://docs.google.com/document/d/1xgQGRpaHQX6-QH_J9YV2C2Dh6RpXefUpLM7KGkzL6Fg), the goal is to provide users a customizable and consistent fashion to use a 3rd party K8s scheduler for Spark, we proposed the following user-facing changes when submitting spark jobs:
   
   ```
   --conf spark.kubernete.driver.scheduler.name=xxx 
   --conf spark.kubernetes.driver.pod.featureSteps=org.apache.spark.deploy.k8s.features.scheduler.XxxFeatureStep
   --conf spark.kubernete.job.queue default
   --conf spark.kubernete.job.min.cpu 4
   --conf spark.kubernete.job.min.memory 8G
   ```
   
   both Volcano and YuniKorn will honor these configs and set up driver/executor pods accordingly via feature step. That's why I was waiting for @Yikun to finish the general stuff in the K8s feature step code implementation and then submitted this PR. For yunikorn side logic, there is no major difference from Volcano. We need to set up a few pod annotations for appID, job queue, and also K8s CRD. In the case of yunikorn, it is application.yunikorn.apache.org, [CRD definition](https://github.com/apache/incubator-yunikorn-release/blob/master/helm-charts/yunikorn/templates/crds/application-definition.yaml). The only difference is, PodGroup is a mandatory resource required by Volcano, but app CRD is optional in YuniKorn. So in the 1st phase, my PR doesn't introduce the CRD creation, but at least we have the basic integration working.
   
    ### 1) if not feature step, what's the alternative
   
   @Yikun summarize the alternative [here](https://github.com/apache/spark/pull/35663#issuecomment-1056101229): use the annotation placeholders introduced via: https://github.com/apache/spark/pull/35704. I looked into this approach, that looks like we will need to set up something like:
   
    ```
   --conf spark.kubernete.driver.scheduler.name=yunikorn 
   --conf spark.kubernetes.driver.annotation.yunikorn.apache.org/app-id={{APP_ID}}
   --conf spark.kubernetes.executor.annotation.yunikorn.apache.org/app-id={{APP_ID}}
   --conf spark.kubernete.job.queue default
   --conf spark.kubernete.job.min.cpu 4
   --conf spark.kubernete.job.min.memory 8G
   ```
   
   this can work for the 1st phase. However, I am not sure how to achieve our 2nd phase target when the [CRD](https://github.com/apache/incubator-yunikorn-release/blob/master/helm-charts/yunikorn/templates/crds/application-definition.yaml) is introduced. Are you suggesting for the 1st phase we use this approach, and add the feature step in the 2nd phase? Will that lead to a different user experience for the end-users?
   
   I really appreciate it if you can share your thoughts. Thanks!
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] yangwwei edited a comment on pull request #35663: [SPARK-37809][K8S] Add yunikorn feature step

Posted by GitBox <gi...@apache.org>.
yangwwei edited a comment on pull request #35663:
URL: https://github.com/apache/spark/pull/35663#issuecomment-1060230905


   Hi @dongjoon-hyun 
   
   I want to work with you and see what is the best way to solve this.  Apologies for this long comment, there are mainly 2 parts: 1) why yunikorn feature step; 2) if not feature step, what's the alternative.
   
   ### 1) why yunikorn feature step
   
   In the [proposal](https://docs.google.com/document/d/1xgQGRpaHQX6-QH_J9YV2C2Dh6RpXefUpLM7KGkzL6Fg), the goal is to provide users a customizable and consistent fashion to use a 3rd party K8s scheduler for Spark, we proposed the following user-facing changes when submitting spark jobs:
   
   ```
   --conf spark.kubernetes.driver.scheduler.name=xxx 
   --conf spark.kubernetes.driver.pod.featureSteps=org.apache.spark.deploy.k8s.features.scheduler.XxxFeatureStep
   --conf spark.kubernetes.job.queue default
   --conf spark.kubernetes.job.minCPU4
   --conf spark.kubernetes.job.minMemory8G
   ```
   
   both Volcano and YuniKorn will honor these configs and set up driver/executor pods accordingly via feature step. That's why I was waiting for @Yikun to finish the general stuff in the K8s feature step code implementation and then submitted this PR. For yunikorn side logic, there is no major difference from Volcano. We need to set up a few pod annotations for appID, job queue, and also K8s CRD. In the case of yunikorn, it is application.yunikorn.apache.org, [CRD definition](https://github.com/apache/incubator-yunikorn-release/blob/master/helm-charts/yunikorn/templates/crds/application-definition.yaml). The only difference is, PodGroup is a mandatory resource required by Volcano, but app CRD is optional in YuniKorn. So in the 1st phase, my PR doesn't introduce the CRD creation, but at least we have the basic integration working.  BTW, yunikorn has already passed the ASF graduation votes, so it will become to be an Apache TLP in a few weeks.
   
    ### 2) if not feature step, what's the alternative
   
   @Yikun summarize the alternative [here](https://github.com/apache/spark/pull/35663#issuecomment-1056101229): use the annotation placeholders introduced via: https://github.com/apache/spark/pull/35704. I looked into this approach, that looks like we will need to set up something like:
   
    ```
   --conf spark.kubernetes.driver.scheduler.name=yunikorn 
   --conf spark.kubernetes.driver.annotation.yunikorn.apache.org/app-id={{APP_ID}}
   --conf spark.kubernetes.executor.annotation.yunikorn.apache.org/app-id={{APP_ID}}
   --conf spark.kubernetes.job.queue default
   --conf spark.kubernetes.job.minCPU 4
   --conf spark.kubernetes.job.minMemory 8G
   ```
   
   this can work for the 1st phase. However, I am not sure how to achieve our 2nd phase target when the [CRD](https://github.com/apache/incubator-yunikorn-release/blob/master/helm-charts/yunikorn/templates/crds/application-definition.yaml) is introduced. Are you suggesting for the 1st phase we use this approach, and add the feature step in the 2nd phase? Will that lead to a different user experience for the end-users?
   
   I really appreciate it if you can share your thoughts. Thanks!
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] dongjoon-hyun commented on pull request #35663: [SPARK-37809][K8S] Add yunikorn feature step

Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #35663:
URL: https://github.com/apache/spark/pull/35663#issuecomment-1055978074


   It seems that you missed my point. This is just a duplication, @yangwwei . We want to avoid this kind of boiler templating as much as possible.
   >  this is a very similar change.
   
   Again, we already support `--conf spark.kubernete.driver.scheduler.name=yunikorn` in the master branch when we extend for volcano. Please be specific about missing part and what we really need.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] Yikun edited a comment on pull request #35663: [SPARK-37809][K8S] Add yunikorn feature step

Posted by GitBox <gi...@apache.org>.
Yikun edited a comment on pull request #35663:
URL: https://github.com/apache/spark/pull/35663#issuecomment-1056101229


   @yangwwei 
   
   There are two alternative way to support `Yunikorn` at this stage:
   
   - New annotations placeholder support which @dongjoon-hyun  introduced. In theory, all annotations can be set by this way.(such as queue placeholder). It's very flexiable. This is very helpful for custom schdulers which only need annotaions set.
   
   - A separate feature step (this PR): this will help to simplify annotation configuration, we just need to set feature step to instead of setting many annotations conf. This way it's more friendly to schedulers which need to create CRD and also need many annotaions.
   
   I think dongjoon-hyun's concern is how to use the most simple way to integrate Yunikorn at this stage. And I also think even if we support by this way, `Yunikorn` can also doc in spark website, tell user how to use offcially.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] holdenk commented on a change in pull request #35663: [SPARK-37809][K8S] Add yunikorn feature step

Posted by GitBox <gi...@apache.org>.
holdenk commented on a change in pull request #35663:
URL: https://github.com/apache/spark/pull/35663#discussion_r817131398



##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/YuniKornFeatureStep.scala
##########
@@ -0,0 +1,49 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.deploy.k8s.features
+
+import io.fabric8.kubernetes.api.model._
+
+import org.apache.spark.deploy.k8s.{KubernetesConf, KubernetesDriverConf, KubernetesExecutorConf, SparkPod}
+
+private[spark] class YuniKornFeatureStep extends KubernetesDriverCustomFeatureConfigStep
+  with KubernetesExecutorCustomFeatureConfigStep {
+
+  private var kubernetesConf: KubernetesConf = _
+
+  override def init(config: KubernetesDriverConf): Unit = {
+    kubernetesConf = config
+  }
+
+  override def init(config: KubernetesExecutorConf): Unit = {
+    kubernetesConf = config
+  }
+
+  override def configurePod(pod: SparkPod): SparkPod = {
+    val k8sPodBuilder = new PodBuilder(pod.pod)
+      .editMetadata()
+      .addToAnnotations(YuniKornFeatureStep.AppIdAnnotationKey, kubernetesConf.appId)

Review comment:
       So this is the one part that we can't do by just setting the scheduler name right now correct? Are there any other parts that you think we need to add to the featurestep in the future?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] Yikun commented on a change in pull request #35663: [SPARK-37809] Add yunikorn feature step

Posted by GitBox <gi...@apache.org>.
Yikun commented on a change in pull request #35663:
URL: https://github.com/apache/spark/pull/35663#discussion_r815549956



##########
File path: project/SparkBuild.scala
##########
@@ -968,6 +973,12 @@ object Volcano {
   )
 }
 
+object YuniKorn {
+  // Exclude all yunikorn file for Compile and Test
+  lazy val settings = Seq(
+    unmanagedSources / excludeFilter := HiddenFileFilter || "*YuniKorn*.scala")

Review comment:
       ```suggestion
       unmanagedSources / excludeFilter += HiddenFileFilter || "*YuniKorn*.scala")
   ```
   
   Looks like yunikorn setting overwrite accidently, try to use `+=` rather than `:=` in here.
   
   [1] https://www.scala-sbt.org/1.x/docs/Appending-Values.html




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] Yikun commented on a change in pull request #35663: [SPARK-37809] Add yunikorn feature step

Posted by GitBox <gi...@apache.org>.
Yikun commented on a change in pull request #35663:
URL: https://github.com/apache/spark/pull/35663#discussion_r815709388



##########
File path: project/SparkBuild.scala
##########
@@ -968,6 +973,13 @@ object Volcano {
   )
 }
 
+object YuniKorn {
+  // Exclude all yunikorn file for Compile and Test
+  lazy val settings = Seq(
+    unmanagedSources / excludeFilter ~= { _ || "*YuniKorn*.scala" }

Review comment:
       ```
   unmanagedSources / excludeFilter ~= { _ || "*YuniKorn*.scala" }
   ```
   which is equivalent to:
   ```
   unmanagedSources / excludeFilter <<=
   (unmanagedSources / excludeFilter) { (currentFilter: FileFilter) =>
   currentFilter || "*YuniKorn*.scala"
   }
   ```
   
   [1] https://groups.google.com/g/simple-build-tool/c/zVMyoWRAVWg?hl=en
   
   Note for myself and othre reviewers. : )




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] yangwwei commented on a change in pull request #35663: [WIP][SPARK-37809] Add yunikorn feature step

Posted by GitBox <gi...@apache.org>.
yangwwei commented on a change in pull request #35663:
URL: https://github.com/apache/spark/pull/35663#discussion_r815403809



##########
File path: resource-managers/kubernetes/integration-tests/src/test/scala/org/apache/spark/deploy/k8s/integrationtest/YuniKornSuite.scala
##########
@@ -0,0 +1,33 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.deploy.k8s.integrationtest
+
+import org.scalatest.Tag
+
+class YuniKornSuite extends KubernetesSuite with YuniKornTestsSuite {
+
+  override protected def setUpTest(): Unit = {
+    super.setUpTest()
+    sparkAppConf
+      .set("spark.kubernetes.driver.scheduler.name", "yunikorn")
+      .set("spark.kubernetes.executor.scheduler.name", "yunikorn")

Review comment:
       Done

##########
File path: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/features/YuniKornFeatureStepSuite.scala
##########
@@ -0,0 +1,43 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.deploy.k8s.features
+
+import org.apache.spark.{SparkConf, SparkFunSuite}
+import org.apache.spark.deploy.k8s._
+
+class YuniKornFeatureStepSuite extends SparkFunSuite {
+
+  test("SPARK-37809: Driver Pod with YuniKorn labels") {

Review comment:
       done

##########
File path: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/features/YuniKornFeatureStepSuite.scala
##########
@@ -0,0 +1,43 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.deploy.k8s.features
+
+import org.apache.spark.{SparkConf, SparkFunSuite}
+import org.apache.spark.deploy.k8s._
+
+class YuniKornFeatureStepSuite extends SparkFunSuite {
+
+  test("SPARK-37809: Driver Pod with YuniKorn labels") {
+    val sparkConf = new SparkConf()
+    val kubernetesConf = KubernetesTestConf.createDriverConf(sparkConf)
+    val step = new YuniKornFeatureStep()
+    step.init(kubernetesConf)
+    val configuredPod = step.configurePod(SparkPod.initialPod())
+    val annotations = configuredPod.pod.getMetadata.getAnnotations
+    assert(annotations.get("yunikorn.apache.org/app-id") === ${kubernetesConf.appId})
+  }
+
+  test("SPARK-37809: Executor Pod with YuniKorn labels") {

Review comment:
       done




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] yangwwei edited a comment on pull request #35663: [SPARK-37809][K8S] Add yunikorn feature step

Posted by GitBox <gi...@apache.org>.
yangwwei edited a comment on pull request #35663:
URL: https://github.com/apache/spark/pull/35663#issuecomment-1055993657


   For the scheduler name, that's fine. In this PR, the addition is we need to add this:
   
   > addToAnnotations(YuniKornFeatureStep.AppIdAnnotationKey, kubernetesConf.appId)
   
   this is needed in order to let yunikorn know which pods belong to which job. Otherwise, it won't be able to be scheduled. In the next PR for queue-related configs, I will add some other logic to load the queue configs from conf and add that to the pod annotation. 
   
   PS: Updated the description to include some more details why we need this. Hope this makes sense.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] Yikun edited a comment on pull request #35663: [SPARK-37809][K8S] Add yunikorn feature step

Posted by GitBox <gi...@apache.org>.
Yikun edited a comment on pull request #35663:
URL: https://github.com/apache/spark/pull/35663#issuecomment-1056101229


   @yangwwei 
   
   There are two alternative way to support `Yunikorn` at this stage:
   
   - New annotations placeholder support which @dongjoon-hyun  introduced. In theory, all annotations can be set by this way.(such as queue placeholder). It's very flexiable. This is very helpful for custom schdulers which only need annotaions set.
   
   - A separate feature step (this PR): this will help to simplify annotation configuration, we just need to set feature step to instead of setting many annotations conf. This way it's more friendly to schedulers which need to create extra CRD and also need many annotaions.
   
   I think dongjoon-hyun's concern is how to use the most simple way to integrate Yunikorn at this stage. And I also think even if we support by this way, `Yunikorn` can also doc in spark website, tell user how to use offcially.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] yangwwei edited a comment on pull request #35663: [SPARK-37809][K8S] Add yunikorn feature step

Posted by GitBox <gi...@apache.org>.
yangwwei edited a comment on pull request #35663:
URL: https://github.com/apache/spark/pull/35663#issuecomment-1060230905


   Hi @dongjoon-hyun 
   
   I want to work with you and see what is the best way to solve this.  Apologies for this long comment, there are mainly 2 parts: 1) why yunikorn feature step; 2) if not feature step, what's the alternative.
   
   ### 1) why yunikorn feature step
   
   In the [proposal](https://docs.google.com/document/d/1xgQGRpaHQX6-QH_J9YV2C2Dh6RpXefUpLM7KGkzL6Fg), the goal is to provide users a customizable and consistent fashion to use a 3rd party K8s scheduler for Spark, we proposed the following user-facing changes when submitting spark jobs:
   
   ```
   --conf spark.kubernete.driver.scheduler.name=xxx 
   --conf spark.kubernetes.driver.pod.featureSteps=org.apache.spark.deploy.k8s.features.scheduler.XxxFeatureStep
   --conf spark.kubernete.job.queue default
   --conf spark.kubernete.job.min.cpu 4
   --conf spark.kubernete.job.min.memory 8G
   ```
   
   both Volcano and YuniKorn will honor these configs and set up driver/executor pods accordingly via feature step. That's why I was waiting for @Yikun to finish the general stuff in the K8s feature step code implementation and then submitted this PR. For yunikorn side logic, there is no major difference from Volcano. We need to set up a few pod annotations for appID, job queue, and also K8s CRD. In the case of yunikorn, it is application.yunikorn.apache.org, [CRD definition](https://github.com/apache/incubator-yunikorn-release/blob/master/helm-charts/yunikorn/templates/crds/application-definition.yaml). The only difference is, PodGroup is a mandatory resource required by Volcano, but app CRD is optional in YuniKorn. So in the 1st phase, my PR doesn't introduce the CRD creation, but at least we have the basic integration working.  BTW, yunikorn has already passed the ASF graduation votes, so it will become to be an Apache TLP in a few weeks.
   
    ### 2) if not feature step, what's the alternative
   
   @Yikun summarize the alternative [here](https://github.com/apache/spark/pull/35663#issuecomment-1056101229): use the annotation placeholders introduced via: https://github.com/apache/spark/pull/35704. I looked into this approach, that looks like we will need to set up something like:
   
    ```
   --conf spark.kubernete.driver.scheduler.name=yunikorn 
   --conf spark.kubernetes.driver.annotation.yunikorn.apache.org/app-id={{APP_ID}}
   --conf spark.kubernetes.executor.annotation.yunikorn.apache.org/app-id={{APP_ID}}
   --conf spark.kubernete.job.queue default
   --conf spark.kubernete.job.min.cpu 4
   --conf spark.kubernete.job.min.memory 8G
   ```
   
   this can work for the 1st phase. However, I am not sure how to achieve our 2nd phase target when the [CRD](https://github.com/apache/incubator-yunikorn-release/blob/master/helm-charts/yunikorn/templates/crds/application-definition.yaml) is introduced. Are you suggesting for the 1st phase we use this approach, and add the feature step in the 2nd phase? Will that lead to a different user experience for the end-users?
   
   I really appreciate it if you can share your thoughts. Thanks!
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] Yikun commented on a change in pull request #35663: [SPARK-37809] Add yunikorn feature step

Posted by GitBox <gi...@apache.org>.
Yikun commented on a change in pull request #35663:
URL: https://github.com/apache/spark/pull/35663#discussion_r815709388



##########
File path: project/SparkBuild.scala
##########
@@ -968,6 +973,13 @@ object Volcano {
   )
 }
 
+object YuniKorn {
+  // Exclude all yunikorn file for Compile and Test
+  lazy val settings = Seq(
+    unmanagedSources / excludeFilter ~= { _ || "*YuniKorn*.scala" }

Review comment:
       [1] https://groups.google.com/g/simple-build-tool/c/zVMyoWRAVWg?hl=en
   [2] https://stackoverflow.com/questions/41627627/sbt-0-13-8-what-does-the-settingkey-method-do
   
   Note for myself and othre reviewers. : ) but looks like we couldn't find any offcial sbt doc




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] holdenk commented on pull request #35663: [SPARK-37809][K8S] Add yunikorn feature step

Posted by GitBox <gi...@apache.org>.
holdenk commented on pull request #35663:
URL: https://github.com/apache/spark/pull/35663#issuecomment-1055845501


   Jenkins ok to test


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] yangwwei commented on pull request #35663: [SPARK-37809][K8S] Add yunikorn feature step

Posted by GitBox <gi...@apache.org>.
yangwwei commented on pull request #35663:
URL: https://github.com/apache/spark/pull/35663#issuecomment-1055887127


   > Sorry, but I don't think we need this at this stage.
   > 
   > * `spark.kubernetes.scheduler.name` can be used for any custom scheduler name.
   > * We can add any annotation names easily.
   
   I am sorry I did not get your point. This is the same effort as what was done for volcano feature step https://github.com/apache/spark/pull/35422, a separate feature step for yunikorn. This was proposed together in the SPIP doc, why this is not needed? 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] martin-g commented on a change in pull request #35663: [SPARK-37809] Add yunikorn feature step

Posted by GitBox <gi...@apache.org>.
martin-g commented on a change in pull request #35663:
URL: https://github.com/apache/spark/pull/35663#discussion_r815624479



##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/YuniKornFeatureStep.scala
##########
@@ -0,0 +1,46 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.deploy.k8s.features
+
+import io.fabric8.kubernetes.api.model._
+
+import org.apache.spark.deploy.k8s.{KubernetesConf, KubernetesDriverConf, KubernetesExecutorConf, SparkPod}
+
+private[spark] class YuniKornFeatureStep extends KubernetesDriverCustomFeatureConfigStep
+  with KubernetesExecutorCustomFeatureConfigStep {
+
+  private var kubernetesConf: KubernetesConf = _
+
+  private val YUNIKORN_APP_ID_ANNOTATION = "yunikorn.apache.org/app-id"

Review comment:
       I think it would be good to extract this as a constant in a companion `object YuniKornFeatureStep` and reuse it in the test.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org