[SPARK-22953][K8S] Avoids adding duplicated secret volumes when init-container is used #20148

liyinan926 · 2018-01-04T06:33:14Z

What changes were proposed in this pull request?

User-specified secrets are mounted into both the main container and init-container (when it is used) in a Spark driver/executor pod, using the MountSecretsBootstrap. Because MountSecretsBootstrap always adds new secret volumes for the secrets to the pod, the same secret volumes get added twice, one when mounting the secrets to the main container, and the other when mounting the secrets to the init-container. This PR fixes the issue by separating MountSecretsBootstrap.mountSecrets out into two methods: addSecretVolumes for adding secret volumes to a pod and mountSecrets for mounting secret volumes to a container, respectively. addSecretVolumes is only called once for each pod, whereas mountSecrets is called individually for the main container and the init-container (if it is used).

Ref: apache-spark-on-k8s#594.

How was this patch tested?

Unit tested.

@vanzin @jiangxb1987 @ueshin This is to fix a critical bug in 2.3. PTAL. Thanks!
@hex108 @foxish @kimoonkim

…container is used

foxish · 2018-01-04T06:46:56Z

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

   *
   * @param pod the pod into which the secret volumes are being added.
   * @param container the container into which the secret volumes are being mounted.
+   * @param addNewVolumes whether to add new secret volumes for the secrets.


I think this problem arose because we're conflating two things here - adding secret volumes (which are pod-scoped) and adding volume-mounts (which are container-scoped). I think we should separate these out. The branching may work for now, but we should have a future work item to separate these out.

cc/ @mccheah

Agreed. I didn't separate it out because we will touch this code as part of refactoring the steps code anyway as planned in https://issues.apache.org/jira/browse/SPARK-22839.

SparkQA · 2018-01-04T06:53:26Z

Test build #85664 has finished for PR 20148 at commit 9be26a8.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

foxish · 2018-01-04T06:54:04Z

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

   * @return the updated pod and container with the secrets mounted.
   */
-  def mountSecrets(pod: Pod, container: Container): (Pod, Container) = {
+  def mountSecrets(


Can we separate this method into addSecretVolumes and mountSecrets? The former would just need the pod argument, and the latter would take the container as argument. That way, we could probably separate this out better and it would make it more readable than the branching. WDYT?

Good point. Done.

hex108 · 2018-01-04T07:12:02Z

...tes/core/src/test/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodFactorySuite.scala

-      === "secret1-volume")
-    assert(executor.getSpec.getInitContainers.get(0).getVolumeMounts.get(0)
-      .getMountPath === "/var/secret1")
+    assert(SecretVolumeUtils.containerHasVolume(


It might be better to change None at line 168 to Some(secretBootstrap) and check volumes' number to avoid regression.

Thanks! We also need check volumes' num in pod spec.

// check volume mounted. assert(executor.getSpec.getVolumes.size() === 1) assert(executor.getSpec.getVolumes.get(0).getSecret.getSecretName === "secret1")

Done. 93e1d64.

SparkQA · 2018-01-04T07:30:22Z

Test build #85667 has finished for PR 20148 at commit 2e6f7db.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-01-04T07:38:00Z

Test build #85668 has finished for PR 20148 at commit 92251aa.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-01-04T07:49:43Z

Test build #85669 has finished for PR 20148 at commit 93e1d64.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

jiangxb1987 · 2018-01-04T09:52:53Z

LGTM, please also update the PR description.

liyinan926 · 2018-01-04T16:31:41Z

@jiangxb1987 Updated PR description.

vanzin · 2018-01-04T18:57:50Z

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

-  def podHasVolume(driverPod: Pod, volumeName: String): Boolean = {
-    driverPod.getSpec.getVolumes.asScala.exists(volume => volume.getName == volumeName)
+  def podHasVolume(pod: Pod, volumeName: String): Boolean = {
+    pod.getSpec.getVolumes.asScala.exists(volume => volume.getName == volumeName)


Since you're touching this, the style is always .exists { foo => ... }.

vanzin · 2018-01-04T18:58:00Z

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

-      mountPath: String): Boolean = {
-    driverContainer.getVolumeMounts.asScala.exists(volumeMount =>
+  def containerHasVolume(container: Container, volumeName: String, mountPath: String): Boolean = {
+    container.getVolumeMounts.asScala.exists(volumeMount =>


SparkQA · 2018-01-04T19:17:07Z

Test build #85690 has finished for PR 20148 at commit b17fe4d.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

liyinan926 · 2018-01-04T19:46:25Z

...e/src/main/scala/org/apache/spark/deploy/k8s/submit/steps/BasicDriverConfigurationStep.scala

A note on this change: it seems double quotes get literally added. With the double quotes appended explicitly here, when running SparkPi with an argument 5, an NumberFormatException will be thrown complaining that ""5"" is not parsable. So the explicit double quotes are not really needed.

Thanks for calling this out - let's try and test this using an integration test as well in https://github.com/apache-spark-on-k8s/spark-integration/. Would be good to put in manual integration testing results here until we have the PRB automation figured out.

Sure, I will add one to the PR under review.

SparkQA · 2018-01-04T20:04:43Z

Test build #85693 has finished for PR 20148 at commit 6636410.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-01-04T21:02:24Z

Test build #85694 has finished for PR 20148 at commit 92ef568.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-01-04T21:06:54Z

Test build #85695 has finished for PR 20148 at commit 5278ea7.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

liyinan926 · 2018-01-04T21:52:18Z

@vanzin Can this be merged?

vanzin · 2018-01-04T22:57:32Z

Merging to master / 2.3.

vanzin · 2018-01-04T22:58:33Z

Actually no. Please open the PR against master instead.

liyinan926 · 2018-01-04T22:59:53Z

Why master?

vanzin · 2018-01-04T23:01:51Z

Because all changes go to master first.

liyinan926 · 2018-01-04T23:05:58Z

Got it. @vanzin created #20159. Closing this one.

…container is used ## What changes were proposed in this pull request? User-specified secrets are mounted into both the main container and init-container (when it is used) in a Spark driver/executor pod, using the `MountSecretsBootstrap`. Because `MountSecretsBootstrap` always adds new secret volumes for the secrets to the pod, the same secret volumes get added twice, one when mounting the secrets to the main container, and the other when mounting the secrets to the init-container. This PR fixes the issue by separating `MountSecretsBootstrap.mountSecrets` out into two methods: `addSecretVolumes` for adding secret volumes to a pod and `mountSecrets` for mounting secret volumes to a container, respectively. `addSecretVolumes` is only called once for each pod, whereas `mountSecrets` is called individually for the main container and the init-container (if it is used). Ref: apache-spark-on-k8s#594. ## How was this patch tested? Unit tested and manually tested. vanzin This replaces #20148. hex108 foxish kimoonkim Author: Yinan Li <[email protected]> Closes #20159 from liyinan926/master. (cherry picked from commit e288fc8) Signed-off-by: Marcelo Vanzin <[email protected]>

…container is used ## What changes were proposed in this pull request? User-specified secrets are mounted into both the main container and init-container (when it is used) in a Spark driver/executor pod, using the `MountSecretsBootstrap`. Because `MountSecretsBootstrap` always adds new secret volumes for the secrets to the pod, the same secret volumes get added twice, one when mounting the secrets to the main container, and the other when mounting the secrets to the init-container. This PR fixes the issue by separating `MountSecretsBootstrap.mountSecrets` out into two methods: `addSecretVolumes` for adding secret volumes to a pod and `mountSecrets` for mounting secret volumes to a container, respectively. `addSecretVolumes` is only called once for each pod, whereas `mountSecrets` is called individually for the main container and the init-container (if it is used). Ref: apache-spark-on-k8s#594. ## How was this patch tested? Unit tested and manually tested. vanzin This replaces #20148. hex108 foxish kimoonkim Author: Yinan Li <[email protected]> Closes #20159 from liyinan926/master.

…597) * Avoids adding duplicated secret volumes when init-container is used Cherry-picked from apache#20148. * Added the missing commit from upstream

[SPARK-22953][K8S] Avoids adding duplicated secret volumes when init-…

9be26a8

…container is used

liyinan926 mentioned this pull request Jan 4, 2018

Duplicated secret volume in pod spec apache-spark-on-k8s/spark#594

Closed

foxish reviewed Jan 4, 2018

View reviewed changes

Addressed comments

2e6f7db

hex108 reviewed Jan 4, 2018

View reviewed changes

Addressed more comments

93e1d64

vanzin reviewed Jan 4, 2018

View reviewed changes

liyinan926 commented Jan 4, 2018

View reviewed changes

Addressed some more comments

5278ea7

liyinan926 mentioned this pull request Jan 4, 2018

[SPARK-22953][K8S] Avoids adding duplicated secret volumes when init-container is used #20159

Closed

liyinan926 closed this Jan 4, 2018

liyinan926 mentioned this pull request Jan 5, 2018

Fix duplicated secret volume in pod spec apache-spark-on-k8s/spark#595

Closed

liyinan926 deleted the branch-2.3 branch January 5, 2018 06:17

liyinan926 mentioned this pull request Jan 9, 2018

Avoids adding duplicated secret volumes when init-container is used apache-spark-on-k8s/spark#597

Merged

[SPARK-22953][K8S] Avoids adding duplicated secret volumes when init-container is used #20148

[SPARK-22953][K8S] Avoids adding duplicated secret volumes when init-container is used #20148

Uh oh!

Conversation

liyinan926 commented Jan 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liyinan926 Jan 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jan 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hex108 Jan 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jan 4, 2018

Uh oh!

SparkQA commented Jan 4, 2018

Uh oh!

SparkQA commented Jan 4, 2018

Uh oh!

jiangxb1987 commented Jan 4, 2018

Uh oh!

liyinan926 commented Jan 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jan 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

foxish Jan 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jan 4, 2018

Uh oh!

SparkQA commented Jan 4, 2018

Uh oh!

SparkQA commented Jan 4, 2018

Uh oh!

liyinan926 commented Jan 4, 2018

Uh oh!

vanzin commented Jan 4, 2018

Uh oh!

vanzin commented Jan 4, 2018

Uh oh!

liyinan926 commented Jan 4, 2018

Uh oh!

vanzin commented Jan 4, 2018

Uh oh!

liyinan926 commented Jan 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

liyinan926 commented Jan 4, 2018 •

edited

Loading

liyinan926 Jan 4, 2018 •

edited

Loading

hex108 Jan 4, 2018 •

edited

Loading

foxish Jan 4, 2018 •

edited

Loading

liyinan926 commented Jan 4, 2018 •

edited

Loading