Added some coverage for executors and test cases for secret mounting and init-containers #15

liyinan926 · 2018-01-02T23:39:16Z

foxish · 2018-01-04T12:39:18Z

@liyinan926 given the recently uncovered bug, I'm curious - did these tests detect that issue?

foxish · 2018-01-04T12:40:03Z

.../test/scala/org/apache/spark/deploy/k8s/integrationtest/docker/SparkDockerImageBuilder.scala

@@ -64,6 +65,7 @@ private[spark] class SparkDockerImageBuilder
    buildImage("spark-base", BASE_DOCKER_FILE)
    buildImage("spark-driver", DRIVER_DOCKER_FILE)
    buildImage("spark-executor", EXECUTOR_DOCKER_FILE)
+    buildImage("spark-init-container", INIT_CONTAINER_DOCKER_FILE)


Please also add an option to allow the test to directly use an init container supplied externally from outside the test code, similar to the options for the driver and executor.

foxish

Mostly LGTM - just a couple of clarifications.

foxish · 2018-01-04T12:40:29Z

...ration-test/src/test/scala/org/apache/spark/deploy/k8s/integrationtest/KubernetesSuite.scala

+  val TEST_SECRET_NAME = "test-secret"
+  val TEST_SECRET_KEY = "test-key"
+  val TEST_SECRET_VALUE = "test-data"
+  val TEST_SECRET_MOUNT_PATH = "/wtc/secrets"


typo meant to be etc?

Yeah, fixed.

foxish · 2018-01-04T12:43:53Z

...ration-test/src/test/scala/org/apache/spark/deploy/k8s/integrationtest/KubernetesSuite.scala

+
+    checkTestSecretInContainer(pod.getSpec.getContainers.get(0))
+
+    if (withInitContainer) {


Is this option being exercised currently?

No, none of the tests uses an init-container yet.

liyinan926 · 2018-01-04T17:50:23Z

@foxish the current set of tests won't uncover the bug because none of them really trigger the use of an init-container. We need tests that use a remote dependency so an init-container is added and used.

kimoonkim

This looks good to me overall, pending @foxish's comments.

Did you test this by running it locally? I wonder if we want to have a Jenkins job trigger on PRs to this repo.

kimoonkim · 2018-01-04T19:26:52Z

rerun integration tests please

kimoonkim · 2018-01-04T19:33:54Z

rerun integration tests please

kimoonkim · 2018-01-04T19:38:47Z

rerun integration tests please

kimoonkim · 2018-01-04T20:47:20Z

A Jenkins run has passed with this PR, http://spark-k8s-jenkins.pepperdata.org:8080/job/PR-dev-spark-integration/2/console.

But it failed to post a status here. Mabye @foxish can give the Jenkins account the write access to this repo?

Setting status of 6a1353f to SUCCESS with url http://spark-k8s-jenkins.pepperdata.org:8080/job/PR-dev-spark-integration/2/ and message: 'Build completed successfully.
'
Using context: Integration Tests
FileNotFoundException means that the credentials Jenkins is using is probably wrong. Or the user account does not have write access to the repo.
java.io.FileNotFoundException: {"message":"Not Found","documentation_url":"https://developer.github.com/v3/repos/statuses/#create-a-status"}

kimoonkim · 2018-01-04T21:07:58Z

...ration-test/src/test/scala/org/apache/spark/deploy/k8s/integrationtest/KubernetesSuite.scala

@@ -74,6 +75,10 @@ private[spark] class KubernetesSuite extends FunSuite with BeforeAndAfterAll wit
    runSparkPiAndVerifyCompletion()
  }

+  test("Run SparkPi with an argument.") {
+    runSparkPiAndVerifyCompletion(appArgs = Array("5"))


Hmm. It seems appArgs are ignored in the spark-submit command line. Please see https://github.com/apache-spark-on-k8s/spark-integration/blob/master/integration-test/src/test/scala/org/apache/spark/deploy/k8s/integrationtest/KubernetesTestComponents.scala#L107

Good catch! Fixed.

kimoonkim · 2018-01-05T01:45:13Z

rerun integration tests please

liyinan926 · 2018-01-05T01:46:35Z

@kimoonkim @foxish I just added a couple of test cases that use a remote example jar hosted at https://storage.googleapis.com/spark-k8s-integration-tests/jars/spark-examples_2.11-2.3.0.jar. The tests will trigger the use of the init-container. See e2d20c9.

kimoonkim · 2018-01-05T01:51:01Z

Jenkins job is now running on this PR and commenting on it!

foxish · 2018-01-05T01:55:05Z

Awesome.. thanks Kimoon & Yinan! Will take another look shortly.

foxish · 2018-01-05T01:57:01Z

...ration-test/src/test/scala/org/apache/spark/deploy/k8s/integrationtest/KubernetesSuite.scala

+
+  test("Run SparkPi using the remote example jar.") {
+    sparkAppConf.set("spark.kubernetes.initContainer.image",
+      System.getProperty("spark.docker.test.initContainerImage", "spark-init-container:latest"))


Don't we call this spark-init elsewhere?

Yes, renamed to spark-init.

foxish · 2018-01-05T01:59:36Z

...ration-test/src/test/scala/org/apache/spark/deploy/k8s/integrationtest/KubernetesSuite.scala

+      })
+  }
+
+  test("Run SparkPi using the remote example jar with a test secret mounted into the driver and " +


Is this test using the remote example jar? I can't find it being referenced in the test itself.

liyinan926 · 2018-01-05T05:55:59Z

rerun integration tests please

kimoonkim · 2018-01-05T17:15:50Z

The latest Jenkins run failed with "spark-base/entrypoint.sh: no such file or directory":

*** RUN ABORTED ***
com.spotify.docker.client.exceptions.DockerException: ProgressMessage{id=null, status=null, stream=null, error=lstat spark-base/entrypoint.sh: no such file or directory, progress=null, progressDetail=null}
at com.spotify.docker.client.LoggingBuildHandler.progress(LoggingBuildHandler.java:33)
at com.spotify.docker.client.DefaultDockerClient.build(DefaultDockerClient.java:1157)
at org.apache.spark.deploy.k8s.integrationtest.docker.SparkDockerImageBuilder.buildImage(SparkDockerImageBuilder.scala:70)
at org.apache.spark.deploy.k8s.integrationtest.docker.SparkDockerImageBuilder.buildSparkDockerImages(SparkDockerImageBuilder.scala:64)
at org.apache.spark.deploy.k8s.integrationtest.backend.minikube.MinikubeTestBackend.initialize(MinikubeTestBackend.scala:31)
at org.apache.spark.deploy.k8s.integrationtest.KubernetesSuite.beforeAll(KubernetesSuite.scala:44)
at org.scalatest.BeforeAndAfterAll$class.beforeAll(BeforeAndAfterAll.scala:187)
at org.apache.spark.deploy.k8s.integrationtest.KubernetesSuite.beforeAll(KubernetesSuite.scala:35)
at org.scalatest.BeforeAndAfterAll$class.run(BeforeAndAfterAll.scala:253)
at org.apache.spark.deploy.k8s.integrationtest.KubernetesSuite.org$scalatest$BeforeAndAfter$$super$run(KubernetesSuite.scala:35)
...

foxish · 2018-01-05T18:13:18Z

Ah! This might be because the dockerfiles have changed in apache/spark#20154

liyinan926 · 2018-01-05T18:14:40Z

Oh, yes, it now requires ARG img_path.

foxish · 2018-01-05T18:21:28Z

I'm not sure how the builder plugin does this. I'm starting to think that since we'll be using the build-push-docker-images script to create release-images also in future, we should just invoke that to build images even for minikube. The script could be an e2e-minikube.sh, similar to e2e-cloud.sh in #16

liyinan926 · 2018-01-05T18:24:37Z

I'm investigating if I can add build arguments with the docker client.

liyinan926 · 2018-01-05T19:29:15Z

@foxish @kimoonkim It doesn't seem to be picking up my docker build argument changes at 5decbce. The build seemed still using the code already in the repo.

foxish

LGTM, pending a manual/automated run of the tests themselves.

foxish · 2018-01-05T21:33:27Z

@liyinan926, can you also add the init-container image option to the e2e/runner.py's invocation of the test, so that we supply the init-container to the test code, similar to https://github.com/apache-spark-on-k8s/spark-integration/blob/master/e2e/runner.sh#L115

liyinan926 · 2018-01-05T23:37:00Z

@foxish done.

foxish · 2018-01-05T23:39:49Z

Merging. Thanks!

liyinan926 · 2018-01-06T00:54:44Z

Manually ran the tests locally and all passed.

liyinan926 force-pushed the master branch 4 times, most recently from 8ec3662 to e92ef5b Compare January 3, 2018 03:08

foxish reviewed Jan 4, 2018

View reviewed changes

kimoonkim reviewed Jan 4, 2018

View reviewed changes

liyinan926 force-pushed the master branch from 5b95097 to 87647ed Compare January 4, 2018 21:46

liyinan926 changed the title ~~Added some coverage for executors and test cases for secret mounting~~ Added some coverage for executors and test cases for secret mounting and init-containers Jan 5, 2018

foxish reviewed Jan 5, 2018

View reviewed changes

liyinan926 force-pushed the master branch from 225b7c5 to 6726b11 Compare January 5, 2018 06:10

liyinan926 force-pushed the master branch from 9bc27c8 to 5decbce Compare January 5, 2018 18:59

liyinan926 force-pushed the master branch from 5decbce to 9ad07e2 Compare January 5, 2018 20:26

foxish approved these changes Jan 5, 2018

View reviewed changes

liyinan926 added 7 commits January 5, 2018 15:31

Added some coverage for executors and test cases for secret mounting

329caf6

Addressed comments

0b992d6

Added a test case of SparkPi with an argument

d474e11

Added test cases that trigger the use of the init-container

5372d79

Addressed comments

2aeaa5f

Fixed the test using a master url without a scheme

ac89463

Updated e2e/runner.sh

93546cb

liyinan926 force-pushed the master branch from 9ad07e2 to 93546cb Compare January 5, 2018 23:36

foxish approved these changes Jan 5, 2018

View reviewed changes

foxish merged commit 3ec521f into apache-spark-on-k8s:master Jan 5, 2018


		checkTestSecretInContainer(pod.getSpec.getContainers.get(0))

		if (withInitContainer) {

Added some coverage for executors and test cases for secret mounting and init-containers #15

Added some coverage for executors and test cases for secret mounting and init-containers #15

Uh oh!

Conversation

liyinan926 commented Jan 2, 2018

Uh oh!

foxish commented Jan 4, 2018

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!

foxish left a comment

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!

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!

liyinan926 commented Jan 4, 2018

Uh oh!

kimoonkim left a comment

Choose a reason for hiding this comment

Uh oh!

kimoonkim commented Jan 4, 2018

Uh oh!

kimoonkim commented Jan 4, 2018

Uh oh!

kimoonkim commented Jan 4, 2018

Uh oh!

kimoonkim commented Jan 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kimoonkim commented Jan 5, 2018

Uh oh!

liyinan926 commented Jan 5, 2018

Uh oh!

kimoonkim commented Jan 5, 2018

Uh oh!

foxish commented Jan 5, 2018

Uh oh!

foxish Jan 5, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liyinan926 commented Jan 5, 2018

Uh oh!

kimoonkim commented Jan 5, 2018

Uh oh!

foxish commented Jan 5, 2018

Uh oh!

liyinan926 commented Jan 5, 2018

Uh oh!

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

Uh oh!

Uh oh!

liyinan926 commented Jan 5, 2018

Uh oh!

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

Uh oh!

Uh oh!

foxish left a comment

Choose a reason for hiding this comment

Uh oh!

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

foxish Jan 4, 2018 •

edited

Loading

liyinan926 Jan 4, 2018 •

edited

Loading

foxish Jan 5, 2018 •

edited

Loading

foxish commented Jan 5, 2018 •

edited

Loading

liyinan926 commented Jan 5, 2018 •

edited

Loading

foxish commented Jan 5, 2018 •

edited

Loading