[GitHub] [spark] liyinan926 commented on a change in pull request #24703: [SPARK-27362][K8S] Resource Scheduling support for k8s
liyinan926 commented on a change in pull request #24703: [SPARK-27362][K8S]
Resource Scheduling support for k8s
URL: https://github.com/apache/spark/pull/24703#discussion_r288761025
##
File path:
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesConf.scala
##
@@ -199,4 +199,13 @@ private[spark] object KubernetesConf {
.replaceAll("[^a-z0-9\\-]", "")
.replaceAll("-+", "-")
}
+
+ /**
+ * Build a resources name based on the vendor device plugin naming
+ * convention of: vendor-domain/resource. For example, an Nvidia GPU is
+ * advertised as nvidia.com/gpu.
+ */
+ def buildKubernetesResourceName(vendor: String, resourceName: String):
String = {
+s"${vendor}/${resourceName}"
Review comment:
Agreed with @erikerlandson. The current option name sounds fine to me as
long as we document clearly what values we expect users to set.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] liyinan926 commented on a change in pull request #24703: [SPARK-27362][K8S] Resource Scheduling support for k8s
liyinan926 commented on a change in pull request #24703: [SPARK-27362][K8S]
Resource Scheduling support for k8s
URL: https://github.com/apache/spark/pull/24703#discussion_r288710024
##
File path:
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesConf.scala
##
@@ -199,4 +199,13 @@ private[spark] object KubernetesConf {
.replaceAll("[^a-z0-9\\-]", "")
.replaceAll("-+", "-")
}
+
+ /**
+ * Build a resources name based on the vendor device plugin naming
+ * convention of: vendor-domain/resource. For example, an Nvidia GPU is
+ * advertised as nvidia.com/gpu.
+ */
+ def buildKubernetesResourceName(vendor: String, resourceName: String):
String = {
+s"${vendor}/${resourceName}"
Review comment:
Can you update the documentation of the two new options to make it clear
that they expect vendor domain names?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] liyinan926 commented on a change in pull request #24703: [SPARK-27362][K8S] Resource Scheduling support for k8s
liyinan926 commented on a change in pull request #24703: [SPARK-27362][K8S]
Resource Scheduling support for k8s
URL: https://github.com/apache/spark/pull/24703#discussion_r288706042
##
File path:
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesConf.scala
##
@@ -199,4 +199,13 @@ private[spark] object KubernetesConf {
.replaceAll("[^a-z0-9\\-]", "")
.replaceAll("-+", "-")
}
+
+ /**
+ * Build a resources name based on the vendor device plugin naming
+ * convention of: vendor-domain/resource. For example, an Nvidia GPU is
+ * advertised as nvidia.com/gpu.
+ */
+ def buildKubernetesResourceName(vendor: String, resourceName: String):
String = {
+s"${vendor}/${resourceName}"
Review comment:
https://kubernetes.io/docs/tasks/manage-gpus/scheduling-gpus/. It seems from
the doc that only the vendor name needs to be configurable. But the doc you
referred to suggests that the resource name can be arbitrary domain name. So I
guess it's safer to use the full domain name.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] liyinan926 commented on a change in pull request #24703: [SPARK-27362][K8S] Resource Scheduling support for k8s
liyinan926 commented on a change in pull request #24703: [SPARK-27362][K8S]
Resource Scheduling support for k8s
URL: https://github.com/apache/spark/pull/24703#discussion_r288707710
##
File path:
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/BasicDriverFeatureStep.scala
##
@@ -129,6 +132,7 @@ private[spark] class BasicDriverFeatureStep(conf:
KubernetesDriverConf)
.addToLimits(maybeCpuLimitQuantity.toMap.asJava)
.addToRequests("memory", driverMemoryQuantity)
.addToLimits("memory", driverMemoryQuantity)
+.addToLimits(driverResourceQuantities.toMap.asJava)
Review comment:
You are right. The request will be set by default to the limit value.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] liyinan926 commented on a change in pull request #24703: [SPARK-27362][K8S] Resource Scheduling support for k8s
liyinan926 commented on a change in pull request #24703: [SPARK-27362][K8S]
Resource Scheduling support for k8s
URL: https://github.com/apache/spark/pull/24703#discussion_r288706042
##
File path:
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesConf.scala
##
@@ -199,4 +199,13 @@ private[spark] object KubernetesConf {
.replaceAll("[^a-z0-9\\-]", "")
.replaceAll("-+", "-")
}
+
+ /**
+ * Build a resources name based on the vendor device plugin naming
+ * convention of: vendor-domain/resource. For example, an Nvidia GPU is
+ * advertised as nvidia.com/gpu.
+ */
+ def buildKubernetesResourceName(vendor: String, resourceName: String):
String = {
+s"${vendor}/${resourceName}"
Review comment:
https://kubernetes.io/docs/tasks/manage-gpus/scheduling-gpus/. It seems from
the doc that only the vendor name needs to be configurable.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] liyinan926 commented on a change in pull request #24703: [SPARK-27362][K8S] Resource Scheduling support for k8s
liyinan926 commented on a change in pull request #24703: [SPARK-27362][K8S]
Resource Scheduling support for k8s
URL: https://github.com/apache/spark/pull/24703#discussion_r288704228
##
File path:
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesConf.scala
##
@@ -199,4 +199,13 @@ private[spark] object KubernetesConf {
.replaceAll("[^a-z0-9\\-]", "")
.replaceAll("-+", "-")
}
+
+ /**
+ * Build a resources name based on the vendor device plugin naming
+ * convention of: vendor-domain/resource. For example, an Nvidia GPU is
+ * advertised as nvidia.com/gpu.
+ */
+ def buildKubernetesResourceName(vendor: String, resourceName: String):
String = {
+s"${vendor}/${resourceName}"
Review comment:
What's the reason of requiring the full vendor domain name?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] liyinan926 commented on a change in pull request #24703: [SPARK-27362][K8S] Resource Scheduling support for k8s
liyinan926 commented on a change in pull request #24703: [SPARK-27362][K8S]
Resource Scheduling support for k8s
URL: https://github.com/apache/spark/pull/24703#discussion_r288693711
##
File path:
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesConf.scala
##
@@ -199,4 +199,13 @@ private[spark] object KubernetesConf {
.replaceAll("[^a-z0-9\\-]", "")
.replaceAll("-+", "-")
}
+
+ /**
+ * Build a resources name based on the vendor device plugin naming
+ * convention of: vendor-domain/resource. For example, an Nvidia GPU is
+ * advertised as nvidia.com/gpu.
+ */
+ def buildKubernetesResourceName(vendor: String, resourceName: String):
String = {
+s"${vendor}/${resourceName}"
Review comment:
The Kubernetes gpu resource name uses the format `.com/gpu`. Are
`vendor` expected to contain the full domain name like `nvidia.com` or just the
vendor name?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] liyinan926 commented on a change in pull request #24703: [SPARK-27362][K8S] Resource Scheduling support for k8s
liyinan926 commented on a change in pull request #24703: [SPARK-27362][K8S]
Resource Scheduling support for k8s
URL: https://github.com/apache/spark/pull/24703#discussion_r288693711
##
File path:
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesConf.scala
##
@@ -199,4 +199,13 @@ private[spark] object KubernetesConf {
.replaceAll("[^a-z0-9\\-]", "")
.replaceAll("-+", "-")
}
+
+ /**
+ * Build a resources name based on the vendor device plugin naming
+ * convention of: vendor-domain/resource. For example, an Nvidia GPU is
+ * advertised as nvidia.com/gpu.
+ */
+ def buildKubernetesResourceName(vendor: String, resourceName: String):
String = {
+s"${vendor}/${resourceName}"
Review comment:
The Kubernetes gpu resource name uses the format `.com/gpu`. Are
`vendor` expected to contain the full domain name like `nvidia.com` or just the
vendor name to follow the Kubernetes pattern?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] liyinan926 commented on a change in pull request #24703: [SPARK-27362][K8S] Resource Scheduling support for k8s
liyinan926 commented on a change in pull request #24703: [SPARK-27362][K8S]
Resource Scheduling support for k8s
URL: https://github.com/apache/spark/pull/24703#discussion_r288694830
##
File path:
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/BasicDriverFeatureStep.scala
##
@@ -129,6 +132,7 @@ private[spark] class BasicDriverFeatureStep(conf:
KubernetesDriverConf)
.addToLimits(maybeCpuLimitQuantity.toMap.asJava)
.addToRequests("memory", driverMemoryQuantity)
.addToLimits("memory", driverMemoryQuantity)
+.addToLimits(driverResourceQuantities.toMap.asJava)
Review comment:
Why only apply them to the limit?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] liyinan926 commented on a change in pull request #24703: [SPARK-27362][K8S] Resource Scheduling support for k8s
liyinan926 commented on a change in pull request #24703: [SPARK-27362][K8S]
Resource Scheduling support for k8s
URL: https://github.com/apache/spark/pull/24703#discussion_r288693711
##
File path:
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesConf.scala
##
@@ -199,4 +199,13 @@ private[spark] object KubernetesConf {
.replaceAll("[^a-z0-9\\-]", "")
.replaceAll("-+", "-")
}
+
+ /**
+ * Build a resources name based on the vendor device plugin naming
+ * convention of: vendor-domain/resource. For example, an Nvidia GPU is
+ * advertised as nvidia.com/gpu.
+ */
+ def buildKubernetesResourceName(vendor: String, resourceName: String):
String = {
+s"${vendor}/${resourceName}"
Review comment:
The Kubernetes resource name uses the format `.com/gpu`. Are
`vendor` expected to contain the full domain name like `nvidia.com` or just the
vendor name?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
