[SPARK-49598][K8S] Support user-defined labels for OnDemand PVCs #48079

prathit06 · 2024-09-11T16:20:03Z

What changes were proposed in this pull request?

Currently when user sets
volumes.persistentVolumeClaim.[VolumeName].options.claimName=OnDemand
PVCs are created with only 1 label i.e. spark-app-selector = spark.app.id.
Objective of this PR is to allow support of custom labels for onDemand PVCs

Why are the changes needed?

Changes are needed so users can set custom labels to PVCs

Does this PR introduce any user-facing change?

It does not break any existing behaviour but adds a new feature/improvement to enable custom label additions in ondemand PVCs

How was this patch tested?

This was tested in internal/production k8 cluster

Was this patch authored or co-authored using generative AI tooling?

No

…ondemand PVCs

dongjoon-hyun

Thank you for making a PR, @prathit06 .

The proposed pattern options.labels is inconsistent from Apache Spark's way; spark.kubernetes.driver.label.something=true and
spark.kubernetes.executor.label.something=true. Please follow the standard way.

dongjoon-hyun · 2024-09-11T16:29:25Z

...agers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesVolumeUtils.scala

+   * @param labels labels in format : k1=v1,k2=v2
+   * @return Map[k1->v1, k2->v2]
+   */
+  private def convertStringLabelsToMap(labels: Option[String]): Map[String, String] = {


I believe we don't need this helper.

prathit06 · 2024-09-11T17:26:28Z

Thank you for making a PR, @prathit06 .

The proposed pattern options.labels is inconsistent from Apache Spark's way; spark.kubernetes.driver.label.something=true and spark.kubernetes.executor.label.something=true. Please follow the standard way.

Thanks for the review @dongjoon-hyun , will work on the provided suggestion & update here once the PR is ready for review again.

dongjoon-hyun · 2024-09-11T17:28:13Z

Thank you.

I'm preparing Apache Spark 4.0.0-preview2 RC1 for next Monday. It would be great if we can ship your contribution together.

…nges

prathit06 · 2024-09-12T10:36:30Z

Hi @dongjoon-hyun i have updated the PR with suggested changes, Please re-review
adding for reference :
following the config standard way, labels for volume is being picked in below format in this PR

spark.kubernetes.driver.volumes.label.[VolumeType].[VolumeName].[LabelName]: ["LabelValue"]

dongjoon-hyun · 2024-09-12T14:23:26Z

Thank you for update, @prathit06 . I'll start a second-round review.

dongjoon-hyun · 2024-09-12T14:27:53Z

docs/running-on-kubernetes.md

@@ -1182,6 +1182,15 @@ See the [configuration page](configuration.html) for information on Spark config
  </td>
  <td>2.4.0</td>
 </tr>
+<tr>
+  <td><code>spark.kubernetes.driver.volumes.label.[VolumeType].[VolumeName].[LabelName]</code></td>


Well this looks wrong to me because label could be interpreted as [VolumeType] in the existing pattern.

Can we follow options location like you did previously? I expected the following specifically

spark.kubernetes.driver.volumes.[VolumeType].[VolumeName].label.[LabelName]

dongjoon-hyun · 2024-09-12T14:28:08Z

docs/running-on-kubernetes.md

+  <td>(none)</td>
+  <td>
+   Configure <a href="https://kubernetes.io/docs/concepts/storage/volumes/">Kubernetes Volume</a> labels passed to the Kubernetes with <code>LabelName</code> as key having specified value, must conform with Kubernetes label format. For example,
+   <code>spark.kubernetes.driver.volumes.label.persistentVolumeClaim.checkpointpvc.foo=bar</code>.


Please update this too.

- spark.kubernetes.driver.volumes.label.persistentVolumeClaim.checkpointpvc.foo=bar + spark.kubernetes.driver.volumes.persistentVolumeClaim.checkpointpvc.label.foo=bar

dongjoon-hyun · 2024-09-12T14:29:34Z

docs/running-on-kubernetes.md

@@ -1218,6 +1227,15 @@ See the [configuration page](configuration.html) for information on Spark config
  </td>
  <td>2.4.0</td>
 </tr>
+<tr>
+  <td><code>spark.kubernetes.executor.volumes.label.[VolumeType].[VolumeName].[LabelName]</code></td>


dongjoon-hyun · 2024-09-12T14:29:41Z

docs/running-on-kubernetes.md

+  <td>(none)</td>
+  <td>
+   Configure <a href="https://kubernetes.io/docs/concepts/storage/volumes/">Kubernetes Volume</a> labels passed to the Kubernetes with <code>LabelName</code> as key having specified value, must conform with Kubernetes label format. For example,
+   <code>spark.kubernetes.executor.volumes.label.persistentVolumeClaim.checkpointpvc.foo=bar</code>.


dongjoon-hyun · 2024-09-12T14:31:29Z

.../kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/KubernetesVolumeUtilsSuite.scala

@@ -59,6 +59,37 @@ class KubernetesVolumeUtilsSuite extends SparkFunSuite {
      KubernetesPVCVolumeConf("claimName"))
  }

+  test("Parses persistentVolumeClaim volumes correctly with labels") {


Please add a test prefix.

- test("Parses persistentVolumeClaim volumes correctly with labels") { + test("SPARK-49598: Parses persistentVolumeClaim volumes correctly with labels") {

dongjoon-hyun · 2024-09-12T14:31:57Z

.../kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/KubernetesVolumeUtilsSuite.scala

+        labels = Map("env" -> "test", "foo" -> "bar")))
+  }
+
+  test("Parses persistentVolumeClaim volumes & puts labels as empty Map if not provided") {


Please add a test prefix.

dongjoon-hyun · 2024-09-12T14:33:34Z

.../core/src/test/scala/org/apache/spark/deploy/k8s/features/MountVolumesFeatureStepSuite.scala

+      "",
+      true,
+      KubernetesPVCVolumeConf(MountVolumesFeatureStep.PVC_ON_DEMAND,
+        labels = Map("foo" -> "bar", "env" -> "test"),


Although this is supported by language, we recommend to keep the parameter order. labels is the last parameter.

dongjoon-hyun · 2024-09-12T14:34:40Z

...netes/core/src/main/scala/org/apache/spark/deploy/k8s/features/MountVolumesFeatureStep.scala

@@ -86,12 +87,15 @@ private[spark] class MountVolumesFeatureStep(conf: KubernetesConf)
                .replaceAll(PVC_ON_DEMAND, s"${conf.resourceNamePrefix}-driver$PVC_POSTFIX-$i")
          }
          if (storageClass.isDefined && size.isDefined) {
+            val volumeLabels = (labels ++
+              Map(SPARK_APP_ID_LABEL -> conf.appId)).asJava
+            logDebug(s"Adding $volumeLabels to $claimName PVC ")


Let's remove this.

dongjoon-hyun · 2024-09-12T14:35:02Z

...netes/core/src/main/scala/org/apache/spark/deploy/k8s/features/MountVolumesFeatureStep.scala


 private[spark] class MountVolumesFeatureStep(conf: KubernetesConf)
-  extends KubernetesFeatureConfigStep {
+  extends KubernetesFeatureConfigStep with Logging {


Let's remove this after removing logDebug.

dongjoon-hyun · 2024-09-12T14:35:38Z

.../core/src/test/scala/org/apache/spark/deploy/k8s/features/MountVolumesFeatureStepSuite.scala

+      "",
+      true,
+      KubernetesPVCVolumeConf(MountVolumesFeatureStep.PVC_ON_DEMAND,
+        labels = Map("foo1" -> "bar1", "env" -> "exec-test"),


dongjoon-hyun · 2024-09-12T14:35:43Z

.../core/src/test/scala/org/apache/spark/deploy/k8s/features/MountVolumesFeatureStepSuite.scala

+      "",
+      true,
+      KubernetesPVCVolumeConf("pvcClaim1",
+        labels = Map("foo1" -> "bar1", "env1" -> "exec-test-1"),


dongjoon-hyun · 2024-09-12T14:35:49Z

.../core/src/test/scala/org/apache/spark/deploy/k8s/features/MountVolumesFeatureStepSuite.scala

+      "",
+      true,
+      KubernetesPVCVolumeConf("pvcClaim2",
+        labels = Map("foo2" -> "bar2", "env2" -> "exec-test-2"),


dongjoon-hyun · 2024-09-12T14:36:49Z

...nagers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesVolumeSpec.scala

@@ -24,7 +24,8 @@ private[spark] case class KubernetesHostPathVolumeConf(hostPath: String)
 private[spark] case class KubernetesPVCVolumeConf(
    claimName: String,
    storageClass: Option[String] = None,
-    size: Option[String] = None)
+    size: Option[String] = None,
+    labels: Map[String, String] = Map())


Please follow the existing semantic of Option.

- labels: Map[String, String] = Map()) + labels: Option[Map[String, String]] = None)

dongjoon-hyun

I finished the second round review, @prathit06 .

prathit06 · 2024-09-12T17:08:25Z

I finished the second round review, @prathit06 .

Hi @dongjoon-hyun , i have updated the PR as per the suggestions, kindly re-review

dongjoon-hyun · 2024-09-12T17:09:11Z

Thank you. I'm starting a review now.

dongjoon-hyun · 2024-09-12T17:14:43Z

...agers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesVolumeUtils.scala

@@ -45,13 +45,21 @@ object KubernetesVolumeUtils {
      val pathKey = s"$volumeType.$volumeName.$KUBERNETES_VOLUMES_MOUNT_PATH_KEY"
      val readOnlyKey = s"$volumeType.$volumeName.$KUBERNETES_VOLUMES_MOUNT_READONLY_KEY"
      val subPathKey = s"$volumeType.$volumeName.$KUBERNETES_VOLUMES_MOUNT_SUBPATH_KEY"
+      val labelsKey = s"$volumeType.$volumeName.label."


Change the variable name from labelsKey to labelKey to match the plural.

Define and use KUBERNETES_VOLUMES_LABEL_KEY at Config.scala like

spark/resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Config.scala

Line 773 in 98f0d9f

val KUBERNETES_VOLUMES_OPTIONS_PATH_KEY = "options.path"

dongjoon-hyun · 2024-09-12T17:15:01Z

...agers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesVolumeUtils.scala

@@ -45,13 +45,21 @@ object KubernetesVolumeUtils {
      val pathKey = s"$volumeType.$volumeName.$KUBERNETES_VOLUMES_MOUNT_PATH_KEY"
      val readOnlyKey = s"$volumeType.$volumeName.$KUBERNETES_VOLUMES_MOUNT_READONLY_KEY"
      val subPathKey = s"$volumeType.$volumeName.$KUBERNETES_VOLUMES_MOUNT_SUBPATH_KEY"
+      val labelsKey = s"$volumeType.$volumeName.label."
+
+      val volumeSpecificLabelsMap = properties


volumeSpecificLabelsMap -> volumeLabelsMap

dongjoon-hyun · 2024-09-12T17:18:23Z

...managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/KubernetesTestConf.scala

@@ -142,6 +147,7 @@ object KubernetesTestConf {
      }
      conf.set(key(vtype, spec.volumeName, KUBERNETES_VOLUMES_MOUNT_READONLY_KEY),
        spec.mountReadOnly.toString)
+


Please remove this.

dongjoon-hyun · 2024-09-12T17:18:54Z

.../core/src/test/scala/org/apache/spark/deploy/k8s/features/MountVolumesFeatureStepSuite.scala

@@ -120,7 +120,7 @@ class MountVolumesFeatureStepSuite extends SparkFunSuite {
      "/tmp",
      "",
      true,
-      KubernetesPVCVolumeConf("OnDemand")
+      KubernetesPVCVolumeConf(MountVolumesFeatureStep.PVC_ON_DEMAND)


Please don't touch irrelevant line.

dongjoon-hyun · 2024-09-12T17:19:26Z

.../core/src/test/scala/org/apache/spark/deploy/k8s/features/MountVolumesFeatureStepSuite.scala

+      "",
+      true,
+      KubernetesPVCVolumeConf(claimName = MountVolumesFeatureStep.PVC_ON_DEMAND,
+        storageClass = Some("gp"),


nit. Let's use the latest storage class; "gp" -> "gp3"

dongjoon-hyun · 2024-09-12T17:19:43Z

.../core/src/test/scala/org/apache/spark/deploy/k8s/features/MountVolumesFeatureStepSuite.scala

+      "",
+      true,
+      KubernetesPVCVolumeConf(claimName = MountVolumesFeatureStep.PVC_ON_DEMAND,
+        storageClass = Some("gp"),


gp -> gp3

dongjoon-hyun · 2024-09-12T17:20:24Z

.../core/src/test/scala/org/apache/spark/deploy/k8s/features/MountVolumesFeatureStepSuite.scala

+      "",
+      true,
+      KubernetesPVCVolumeConf(claimName = "pvcClaim1",
+        storageClass = Some("gp"),


dongjoon-hyun · 2024-09-12T17:20:31Z

.../core/src/test/scala/org/apache/spark/deploy/k8s/features/MountVolumesFeatureStepSuite.scala

+      "",
+      true,
+      KubernetesPVCVolumeConf(claimName = "pvcClaim2",
+        storageClass = Some("gp"),


dongjoon-hyun

In general, it looks good to me. Please address some comments, @prathit06 .

dongjoon-hyun

+1, LGTM (Pending CIs). Thank you again, @prathit06 .

dongjoon-hyun

Could you fix the test case failure, @prathit06 ?

[info] - Parses persistentVolumeClaim volumes correctly *** FAILED *** (29 milliseconds)
[info]   KubernetesPVCVolumeConf("claimName", None, None, Some(Map())) did not equal KubernetesPVCVolumeConf("claimName", None, None, None) (KubernetesVolumeUtilsSuite.scala:58)
[

dongjoon-hyun · 2024-09-12T19:24:25Z

The remaining failure is your forked repository setting issue.

ERROR: failed to solve: failed to push ghcr.io/prathit06/apache-spark-ci-image:master-10836346141: unexpected status from POST request to https://ghcr.io/v2/prathit06/apache-spark-ci-image/blobs/uploads/: 403 Forbidden

dongjoon-hyun · 2024-09-12T19:24:55Z

Let me merge this since this passes all K8s-related tests.

dongjoon-hyun · 2024-09-12T19:26:51Z

Merged to master for Apache Spark 4.0.0-preview2.

Welcome to the Apache Spark community, @prathit06 .
I added you to the Apache Spark contributor group and assigned SPARK-49598 to you.
And, congratulations for your first commit.

prathit06 · 2024-09-13T04:06:24Z

Thanks @dongjoon-hyun ,for your continuous support and guidance throughout the PR review.

### What changes were proposed in this pull request? Currently when user sets `volumes.persistentVolumeClaim.[VolumeName].options.claimName=OnDemand` PVCs are created with only 1 label i.e. spark-app-selector = spark.app.id. Objective of this PR is to allow support of custom labels for onDemand PVCs ### Why are the changes needed? Changes are needed so users can set custom labels to PVCs ### Does this PR introduce _any_ user-facing change? It does not break any existing behaviour but adds a new feature/improvement to enable custom label additions in ondemand PVCs ### How was this patch tested? This was tested in internal/production k8 cluster ### Was this patch authored or co-authored using generative AI tooling? No Closes apache#48079 from prathit06/ondemand-pvc-labels. Lead-authored-by: prathit06 <malik.prathit@gmail.com> Co-authored-by: Prathit malik <53890994+prathit06@users.noreply.github.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

prathit06 and others added 2 commits September 11, 2024 20:28

[SPARK-49598] Added support to create custom user-defined labels for …

945f9af

…ondemand PVCs

Merge branch 'apache:master' into ondemand-pvc-labels

12c1f30

prathit06 changed the title ~~Ondemand pvc labels~~ [SPARK-49598] Added support to create custom user-defined labels for onDemand PVCs Sep 11, 2024

github-actions bot added the KUBERNETES label Sep 11, 2024

dongjoon-hyun requested changes Sep 11, 2024

View reviewed changes

dongjoon-hyun reviewed Sep 11, 2024

View reviewed changes

dongjoon-hyun changed the title ~~[SPARK-49598] Added support to create custom user-defined labels for onDemand PVCs~~ [SPARK-49598] Support user-defined labels for OnDemand PVCs Sep 11, 2024

dongjoon-hyun changed the title ~~[SPARK-49598] Support user-defined labels for OnDemand PVCs~~ [SPARK-49598][K8S] Support user-defined labels for OnDemand PVCs Sep 11, 2024

prathit06 and others added 3 commits September 12, 2024 15:22

Merge branch 'apache:master' into ondemand-pvc-labels

98e12ae

using standard way to pick labels for volumes & applied suggested cha…

3a4b210

…nges

added label properties in docs

cf7ccfe

github-actions bot added the DOCS label Sep 12, 2024

prathit06 changed the title ~~[SPARK-49598][K8S] Support user-defined labels for OnDemand PVCs~~ [WIP][SPARK-49598][K8S] Support user-defined labels for OnDemand PVCs Sep 12, 2024

removed todo comment

3c974f5

prathit06 changed the title ~~[WIP][SPARK-49598][K8S] Support user-defined labels for OnDemand PVCs~~ [SPARK-49598][K8S] Support user-defined labels for OnDemand PVCs Sep 12, 2024

minor refactoring

04070c1

dongjoon-hyun reviewed Sep 12, 2024

View reviewed changes

dongjoon-hyun requested changes Sep 12, 2024

View reviewed changes

changed pattern for volume labels & change in docs for the same

7edb8ff

dongjoon-hyun reviewed Sep 12, 2024

View reviewed changes

added volume label key in configs & addressed comments

d017e41

dongjoon-hyun approved these changes Sep 12, 2024

View reviewed changes

dongjoon-hyun reviewed Sep 12, 2024

View reviewed changes

fix test case failure

740b47e

dongjoon-hyun closed this in d2d293e Sep 12, 2024

[SPARK-49598][K8S] Support user-defined labels for OnDemand PVCs #48079

[SPARK-49598][K8S] Support user-defined labels for OnDemand PVCs #48079

Conversation

prathit06 commented Sep 11, 2024 • edited Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Was this patch authored or co-authored using generative AI tooling?

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prathit06 commented Sep 11, 2024

dongjoon-hyun commented Sep 11, 2024

prathit06 commented Sep 12, 2024 • edited Loading

dongjoon-hyun commented Sep 12, 2024

Choose a reason for hiding this comment

dongjoon-hyun Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dongjoon-hyun Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dongjoon-hyun Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dongjoon-hyun left a comment

Choose a reason for hiding this comment

prathit06 commented Sep 12, 2024

dongjoon-hyun commented Sep 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dongjoon-hyun left a comment

Choose a reason for hiding this comment

dongjoon-hyun left a comment

Choose a reason for hiding this comment

dongjoon-hyun left a comment

Choose a reason for hiding this comment

dongjoon-hyun commented Sep 12, 2024

dongjoon-hyun commented Sep 12, 2024

dongjoon-hyun commented Sep 12, 2024

prathit06 commented Sep 13, 2024

prathit06 commented Sep 11, 2024 •

edited

Loading

prathit06 commented Sep 12, 2024 •

edited

Loading

dongjoon-hyun Sep 12, 2024 •

edited

Loading

dongjoon-hyun Sep 12, 2024 •

edited

Loading

dongjoon-hyun Sep 12, 2024 •

edited

Loading