-
Notifications
You must be signed in to change notification settings - Fork 118
New API for custom labels and annotations. #346
New API for custom labels and annotations. #346
Conversation
This APi allows for these labels and annotations to have = and , characters, which is hard to accomplish in the old scheme.
docs/running-on-kubernetes.md
Outdated
<td><code>spark.kubernetes.driver.labels</code></td> | ||
<td>(none)</td> | ||
<td> | ||
<i>Deprecated.</i> Use <code>spark.kubernetes.driver.label.<labelKey></code> instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mention here that this is to support label values with =
and ,
characters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
docs/running-on-kubernetes.md
Outdated
@@ -537,6 +578,7 @@ from the other deployment modes. See the [configuration page](configuration.html | |||
<td><code>spark.kubernetes.driver.annotations</code></td> | |||
<td>(none)</td> | |||
<td> | |||
<i>Deprecated.</i> Use <code>spark.kubernetes.driver.annotation.<annotationKey></code> instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mention here that this is to support annotation values with =
and ,
characters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
docs/running-on-kubernetes.md
Outdated
@@ -545,6 +587,7 @@ from the other deployment modes. See the [configuration page](configuration.html | |||
<td><code>spark.kubernetes.executor.labels</code></td> | |||
<td>(none)</td> | |||
<td> | |||
<i>Deprecated.</i> Use <code>spark.kubernetes.executor.label.<labelKey></code> instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
docs/running-on-kubernetes.md
Outdated
@@ -554,6 +597,7 @@ from the other deployment modes. See the [configuration page](configuration.html | |||
<td><code>spark.kubernetes.executor.annotations</code></td> | |||
<td>(none)</td> | |||
<td> | |||
<i>Deprecated.</i> Use <code>spark.kubernetes.executor.annotation.<annotationKey></code> instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -82,17 +82,36 @@ private[spark] class Client( | |||
def run(): Unit = { | |||
validateNoDuplicateFileNames(sparkJars) | |||
validateNoDuplicateFileNames(sparkFiles) | |||
val parsedCustomLabels = ConfigurationUtils.parseKeyValuePairs( | |||
|
|||
val parsedCustomLabelsDeprecated = ConfigurationUtils.parseKeyValuePairs( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
put driver
in the variable name, e.g. parsedDriverCustomLabelsDeprecated
val allLabels = parsedCustomLabels ++ Map( | ||
// Remark: getAllWithPrefix strips out the prefix in the returned array. | ||
val customLabelsFromPrefixConf = sparkConf.getAllWithPrefix(KUBERNETES_DRIVER_LABEL_PREFIX) | ||
val allCustomLabels = parsedCustomLabelsDeprecated.toSeq ++ customLabelsFromPrefixConf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
allCustomDriverLabels
s" operations.") | ||
val allLabels = parsedCustomLabels ++ Map( | ||
// Remark: getAllWithPrefix strips out the prefix in the returned array. | ||
val customLabelsFromPrefixConf = sparkConf.getAllWithPrefix(KUBERNETES_DRIVER_LABEL_PREFIX) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
customDriverLabelsFromPrefixConf
s" $SPARK_APP_ID_LABEL is not allowed as it is reserved for Spark bookkeeping" + | ||
s" operations.") | ||
|
||
val parsedCustomAnnotationsDeprecated = ConfigurationUtils.parseKeyValuePairs( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parsedCustomDriverAnnotationsDeprecated
require(!allCustomAnnotations.map(_._1).contains(SPARK_APP_NAME_ANNOTATION), | ||
s"Annotation with key $SPARK_APP_NAME_ANNOTATION is not allowed as it is reserved for" + | ||
s" Spark bookkeeping operations.") | ||
val allLabels = allCustomLabels.toMap ++ Map( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is getting long -- we should extract out a method that produces both allLabels
and allAnnotations
(named as driverLabels
and driverAnnotations
)
} | ||
require(!allCustomLabels.map(_._1).contains(SPARK_APP_ID_LABEL), s"Label with key " + | ||
s" $SPARK_APP_ID_LABEL is not allowed as it is reserved for Spark bookkeeping" + | ||
s" operations.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also should log a warning when using the deprecated config saying that it's deprecated and should be replaced with the new form (labels and annotations for drivers and executors)
…he-spark-on-k8s/spark into reformat-labels-and-annotations-api
.set(KUBERNETES_DRIVER_ANNOTATIONS, | ||
s"$DEPRECATED_CUSTOM_ANNOTATION_KEY=$DEPRECATED_CUSTOM_ANNOTATION_VALUE") | ||
.set(s"$KUBERNETES_DRIVER_LABEL_PREFIX$CUSTOM_LABEL_KEY", CUSTOM_LABEL_VALUE) | ||
.set(s"$KUBERNETES_DRIVER_ANNOTATION_PREFIX$CUSTOM_ANNOTATION_KEY", CUSTOM_ANNOTATION_VALUE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mccheah is it reasonable to check the executor labels/annotations here too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't parse those until we hit KubernetesClusterSchedulerBackend
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok then, this seems good then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, let's merge when build passes
* New API for custom labels and annotations. This APi allows for these labels and annotations to have = and , characters, which is hard to accomplish in the old scheme. * Compare correct values in requirements * Use helper method * Address comments. * Fix scalastyle * Use variable * Remove unused import
Update to upstream
* New API for custom labels and annotations. This APi allows for these labels and annotations to have = and , characters, which is hard to accomplish in the old scheme. * Compare correct values in requirements * Use helper method * Address comments. * Fix scalastyle * Use variable * Remove unused import
Closes #322.
This APi allows for these labels and annotations to have = and , characters, which is hard to accomplish in the old scheme.