Add unit-testing for executorpodfactory #491

foxish · 2017-09-15T00:29:01Z

This is important for unit-testing executorpodfactory, which will be part of our first upstream PR.

cc @apache-spark-on-k8s/contributors @liyinan926

mccheah · 2017-09-19T18:24:36Z

...e/src/test/scala/org/apache/spark/scheduler/cluster/kubernetes/ExecutorPodFactorySuite.scala

+  private val executorPrefix: String = "base"
+  private val executorImage: String = "executor-image"
+  private val driverPod = new PodBuilder()
+    .withNewMetadata()


Indentation

mccheah · 2017-09-19T18:25:56Z

...e/src/test/scala/org/apache/spark/scheduler/cluster/kubernetes/ExecutorPodFactorySuite.scala

+      .set(KUBERNETES_DRIVER_POD_NAME, driverPodName)
+      .set(KUBERNETES_EXECUTOR_POD_NAME_PREFIX, executorPrefix)
+      .set(EXECUTOR_DOCKER_IMAGE, executorImage)
+    sc = new SparkContext("local", "test")


Can we use mocks here? Can we use a mock SecurityManager?

Would that be useful here?
The ExecutorPodFactoryImpl class doesn't use SecurityManager.

More just that we don't want to make a real SparkContext here. SparkContext.clearActiveContext could be problematic on parallel test runs. If we don't need to make a SparkContext, we should avoid it.

mccheah · 2017-09-21T21:19:37Z

...e/src/test/scala/org/apache/spark/scheduler/cluster/kubernetes/ExecutorPodFactorySuite.scala

+    // Default memory limit is 1024M + 384M (minimum overhead constant).
+    assert(executor.getSpec.getContainers.size() == 1)
+    assert(executor.getSpec.getContainers.get(0).getImage == executorImage)
+    assert(executor.getSpec.getContainers.get(0).getVolumeMounts.size() == 0)


Use isEmpty.

mccheah · 2017-09-21T21:20:09Z

...e/src/test/scala/org/apache/spark/scheduler/cluster/kubernetes/ExecutorPodFactorySuite.scala

+      getLimits.get("memory").getAmount == "1408Mi")
+
+    // The pod has no node selector, volumes.
+    assert(executor.getSpec.getNodeSelector.size() == 0)


isEmpty - replace this everywhere.

mccheah · 2017-09-21T21:21:06Z

...e/src/test/scala/org/apache/spark/scheduler/cluster/kubernetes/ExecutorPodFactorySuite.scala

+  test("basic executor pod has reasonable defaults") {
+    val factory = new ExecutorPodFactoryImpl(baseConf,
+      NodeAffinityExecutorPodModifierImpl, None, None, None, None, None)
+    val executor = factory.createExecutorPod("1", "dummy", "dummy",


Move all args to the next line and all on one line if they fit in 100 chars. If not - put each arg on its own line.

mccheah · 2017-09-21T21:21:41Z

...e/src/test/scala/org/apache/spark/scheduler/cluster/kubernetes/ExecutorPodFactorySuite.scala

+
+    val factory = new ExecutorPodFactoryImpl(conf,
+      NodeAffinityExecutorPodModifierImpl, None, None, None, None, None)
+    val executor = factory.createExecutorPod("1",


Move args to one per line.

mccheah · 2017-09-21T21:24:55Z

...e/src/test/scala/org/apache/spark/scheduler/cluster/kubernetes/ExecutorPodFactorySuite.scala

+    assert(executor.getSpec.getContainers.size() == 1)
+    assert(executor.getSpec.getContainers.get(0).getVolumeMounts.size() == 1)
+    assert(executor.getSpec.getContainers.get(0).getVolumeMounts.get(0).getName == "secret1-volume")
+    assert(executor.getSpec.getContainers.get(0).getVolumeMounts.get(0).


There's a couple of places where the dot is on the end of the line followed by the method or field being on the next line. I think we want to move the dot to the next line and continue the chain of calls from there.

mccheah · 2017-09-21T21:25:42Z

...e/src/test/scala/org/apache/spark/scheduler/cluster/kubernetes/ExecutorPodFactorySuite.scala

+      SparkTransportConf.fromSparkConf(conf, "shuffle"),
+      sc.env.securityManager,
+      sc.env.securityManager.isAuthenticationEnabled())
+    val shuffleManager = new KubernetesExternalShuffleManagerImpl(conf,


Don't use the impl - use a mock.

mccheah · 2017-09-21T21:25:58Z

...e/src/test/scala/org/apache/spark/scheduler/cluster/kubernetes/ExecutorPodFactorySuite.scala

+    val shuffleManager = new KubernetesExternalShuffleManagerImpl(conf,
+      kubernetesClient, kubernetesExternalShuffleClient)
+    val factory = new ExecutorPodFactoryImpl(conf,
+      NodeAffinityExecutorPodModifierImpl, None, None, None, None, Some(shuffleManager))


Don't use the impl, use a mock.

mccheah · 2017-09-21T21:26:30Z

...e/src/test/scala/org/apache/spark/scheduler/cluster/kubernetes/ExecutorPodFactorySuite.scala

+      kubernetesClient, kubernetesExternalShuffleClient)
+    val factory = new ExecutorPodFactoryImpl(conf,
+      NodeAffinityExecutorPodModifierImpl, None, None, None, None, Some(shuffleManager))
+    val executor = factory.createExecutorPod("1",


Argument formatting as stated elsewhere in the review - fix this in all applicable locations.

mccheah · 2017-09-21T21:27:00Z

...e/src/test/scala/org/apache/spark/scheduler/cluster/kubernetes/ExecutorPodFactorySuite.scala

+
+    assert(executor.getSpec.getContainers.size() == 1)
+    assert(executor.getSpec.getContainers.get(0).getVolumeMounts.size() == 1)
+    assert(executor.getSpec.getContainers.get(0).getVolumeMounts.get(0).getName == "0-tmp")


nit - all assertions should use triple-equals - === - instead of double equals.

mccheah · 2017-09-21T21:27:41Z

...e/src/test/scala/org/apache/spark/scheduler/cluster/kubernetes/ExecutorPodFactorySuite.scala

+      NodeAffinityExecutorPodModifierImpl, None, Some(smallFiles), None, None, None)
+    val executor = factory.createExecutorPod("1",
+      "dummy", "dummy", Seq[(String, String)](), driverPod, Map[String, Int]())
+


Extra whitespace here

mccheah · 2017-09-21T21:28:48Z

...e/src/test/scala/org/apache/spark/scheduler/cluster/kubernetes/ExecutorPodFactorySuite.scala

+  }
+
+  // Check that the expected environment variables are present.
+  private def checkEnv(executor: Pod, additionalEnvVars: Set[String]): Unit = {


Do we not also want to be checking the values of the environment variables we are setting, not just the keys? Hence the additionalEnvVars should be a Map, not a Set.

mccheah · 2017-09-26T17:54:10Z

...e/src/test/scala/org/apache/spark/scheduler/cluster/kubernetes/ExecutorPodFactorySuite.scala

+  test("init-container bootstrap step adds an init container") {
+    val conf = baseConf.clone()
+
+    val initContainerBootstrap = new SparkPodInitContainerBootstrapImpl(


Use a mock and not the impl. Add a separate unit test for this class if it does not already exist.

mccheah · 2017-09-26T17:54:20Z

...e/src/test/scala/org/apache/spark/scheduler/cluster/kubernetes/ExecutorPodFactorySuite.scala

+    conf.set(KUBERNETES_SHUFFLE_NAMESPACE, "default")
+    conf.set(KUBERNETES_SHUFFLE_DIR, "/tmp")
+
+    val kubernetesExternalShuffleClient = new KubernetesExternalShuffleClientImpl(


Use a mock, not the impl.

mccheah · 2017-09-26T17:54:39Z

...e/src/test/scala/org/apache/spark/scheduler/cluster/kubernetes/ExecutorPodFactorySuite.scala

+      SparkTransportConf.fromSparkConf(conf, "shuffle"),
+      sc.env.securityManager,
+      sc.env.securityManager.isAuthenticationEnabled())
+    val shuffleManager = new KubernetesExternalShuffleManagerImpl(


Use a mock, not an impl.

mccheah · 2017-09-26T17:54:59Z

...e/src/test/scala/org/apache/spark/scheduler/cluster/kubernetes/ExecutorPodFactorySuite.scala

+
+    val factory = new ExecutorPodFactoryImpl(
+      conf,
+      NodeAffinityExecutorPodModifierImpl,


Use a mock, not the impl. Add a separate unit test for this impl class.

mccheah · 2017-09-26T17:55:20Z

...e/src/test/scala/org/apache/spark/scheduler/cluster/kubernetes/ExecutorPodFactorySuite.scala

+    val secretsBootstrap = new MountSecretsBootstrapImpl(Map("secret1" -> "/var/secret1"))
+    val factory = new ExecutorPodFactoryImpl(
+      conf,
+      NodeAffinityExecutorPodModifierImpl,


mccheah · 2017-09-26T17:55:33Z

...e/src/test/scala/org/apache/spark/scheduler/cluster/kubernetes/ExecutorPodFactorySuite.scala

+  test("basic executor pod has reasonable defaults") {
+    val factory = new ExecutorPodFactoryImpl(
+      baseConf,
+      NodeAffinityExecutorPodModifierImpl,


Use a mock - replace this everywhere

erikerlandson · 2017-09-28T02:02:57Z

Does NodeAffinityExecutorPodModifierImpl object need to be mocked? The problem seemed to be around use of sc as dep for construction of shuffleManager

erikerlandson · 2017-09-28T02:03:37Z

CI build failing?

[error] /home/jenkins/workspace/PR-spark-k8s-full-build/resource-managers/kubernetes/core/src/test/scala/org/apache/spark/scheduler/cluster/kubernetes/ExecutorPodFactorySuite.scala:34: object kubernetes is not a member of package org.apache.spark.deploy
[error] import org.apache.spark.deploy.kubernetes.{constants, SparkPodInitContainerBootstrapImpl}
[error]                                ^

Trying to figure out if I broke something here, but I didn't change that import, and my edits were all local to the unit test file.

mccheah · 2017-09-28T17:56:34Z

The packages have been renamed to k8s right?

mccheah · 2017-09-28T17:57:30Z

Does NodeAffinityExecutorPodModifierImpl object need to be mocked? The problem seemed to be around use of sc as dep for construction of shuffleManager

Basically we only want to have the impl for ExecutorPodFactory - SparkContext and any other object should be mocks.

erikerlandson · 2017-09-28T21:18:16Z

I rebased this branch to the v2.2.0-kubernetes-0.4.0 release tag to pick up the k8s rename and the other remaining 0.4.0 commits

erikerlandson · 2017-09-28T22:27:44Z

@foxish I just rebased this to the 0.4.0 tag, to pick up the .k8s. rename - however this PR is currently targeted to branch-2.2.-kubernetes. Was the intent to merge it to branch-2.2.-kubernetes and then port it to #498 ?

foxish · 2017-09-28T22:29:50Z

Yes we should merge this into the 2.2 branch first; only a subset of the tests are valid for the first apache/spark PR.

…

On Thu, Sep 28, 2017 at 3:27 PM, Erik Erlandson ***@***.***> wrote: @foxish <https://github.com/foxish> I just rebased this to the 0.4.0 tag, to pick up the .k8s. rename - however this PR is currently targeted to branch-2.2.-kubernetes. Was the intent to merge it to branch-2.2.-kubernetes and then port it to #498 <#498> ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#491 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA3U5_586I5BPRTbzgvppAvvDQIFY4Vgks5snB1hgaJpZM4PYWCO> .

-- Anirudh Ramanathan

erikerlandson · 2017-09-28T22:30:48Z

OK I'll make sure its rebased to 2.2 branch before it merges

mccheah · 2017-09-28T23:12:07Z

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

+  private var baseConf: SparkConf = _
+
+  private val nodeAffinityExecutorPodModifier = mock(classOf[NodeAffinityExecutorPodModifier])
+  when(nodeAffinityExecutorPodModifier.addNodeAffinityAnnotationIfUseful(


Move this to the before block.

mccheah · 2017-09-28T23:12:22Z

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

+    any(classOf[Map[String, Int]]))).thenAnswer(AdditionalAnswers.returnsFirstArg())
+
+  before {
+    SparkContext.clearActiveContext()


Don't think we need to do this anymore.

mccheah · 2017-09-28T23:12:47Z

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

+  test("init-container bootstrap step adds an init container") {
+    val conf = baseConf.clone()
+
+    val initContainerBootstrap = new SparkPodInitContainerBootstrapImpl(


Don't use the impl, use a mock.

mccheah · 2017-09-28T23:13:00Z

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

+
+  test("Small-files add a secret & secret volume mount to the container") {
+    val conf = baseConf.clone()
+    val smallFiles = new MountSmallFilesBootstrapImpl("secret1", "/var/secret1")


Don't use the impl, use a mock

mccheah · 2017-09-29T16:57:38Z

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

+    val setEnvs = executor.getSpec.getContainers.get(0).getEnv.asScala.map {
+      x => x.getName
+    }.toSet
+    assert(defaultEnvs === setEnvs)


Do we want to check the values for these as well?

It's not obvious to me what the expected values for some of the env-vars ought to be. Possibly @foxish knows. I've been assuming the main logic being tested there is purely env propagation - to confirm what vars are being passed under what circumstances.

mccheah · 2017-09-29T16:59:40Z

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

+      .set(EXECUTOR_DOCKER_IMAGE, executorImage)
+    when(nodeAffinityExecutorPodModifier.addNodeAffinityAnnotationIfUseful(
+      any(classOf[Pod]),
+      any(classOf[Map[String, Int]]))).thenAnswer(AdditionalAnswers.returnsFirstArg())


This setup makes it such that we don't check if nodeAffinityExecutorPodModifier is used in the pod factory. Either:

Use Mockito.verify

Return meaningful modifications to the pod/container from the mock and test that such modifications were done in the appropriate locations.

mccheah · 2017-10-05T22:30:02Z

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

+          val container = invocation.getArgumentAt(1, classOf[Container])
+          val secretName = "secret1"
+          val secretMountPath = "/var/secret1"
+          val resolvedPod = new PodBuilder(pod)


These can be simplified. Just return the pod with an extra label/annotation, and the container with an extra environment variable. Verify that the pod has this extra label/annotation and the container has the appropriate environment variable.

Or, use Mockito.verify() - I'm still not sure which is the stronger/more idiomatic approach here.

I don't like that we're mocking the very behavior we're testing here. I'd actually be in favor of omitting mocks for this particular case entirely and just relying on a real stamped out MountSmallFilesBootstrapImpol since it's not nearly complex enough to need a mock IMO.

mccheah · 2017-10-09T22:37:59Z

@erikerlandson @foxish ping on latest comment?

foxish · 2017-10-10T03:34:58Z

Addressed comment - like I mentioned there, I see the value in mocks elsewhere, but not in that particular case, since using the actual class itself seems very straightforward. Also addressed comment about env-variable value checking in addition to just the key names. This should be ready to merge at this point. PTAL @mccheah.

mccheah

Might take another pass over later to remove the use of MountSecretsBootstrapImpl, but I want to move this along.

* Unit test for executorpodfactory * Fix test * Indentation fix * Fix isEmpty and split between lines * Address issues with multi-line code fragments * Replace == with === * mock shuffleManager * .kubernetes. => .k8s. * move to k8s subdir * fix package clause to k8s * mock nodeAffinityExecutorPodModifier * remove commented code * move when clause to before{} block * mock initContainerBootstrap, smallFiles * insert actual logic into smallFiles mock * verify application of nodeAffinityExecutorPodModifier * avoid cumulative invocation * Fixed env-var check to include values, removed mock for small files (cherry picked from commit 887fdce)

* Unit test for executorpodfactory * Fix test * Indentation fix * Fix isEmpty and split between lines * Address issues with multi-line code fragments * Replace == with === * mock shuffleManager * .kubernetes. => .k8s. * move to k8s subdir * fix package clause to k8s * mock nodeAffinityExecutorPodModifier * remove commented code * move when clause to before{} block * mock initContainerBootstrap, smallFiles * insert actual logic into smallFiles mock * verify application of nodeAffinityExecutorPodModifier * avoid cumulative invocation * Fixed env-var check to include values, removed mock for small files

foxish force-pushed the add-testing branch from af25faa to 6273b74 Compare September 15, 2017 02:12

mccheah reviewed Sep 19, 2017

View reviewed changes

This was referenced Sep 20, 2017

Spark on Kubernetes - basic scheduler backend #498

Closed

Upstreaming and pull request strategy for Spark on Kubernetes #441

Open

mccheah reviewed Sep 21, 2017

View reviewed changes

mccheah reviewed Sep 26, 2017

View reviewed changes

erikerlandson force-pushed the add-testing branch from bf62649 to 9bf7256 Compare September 28, 2017 22:22

foxish and others added 11 commits September 28, 2017 16:08

Fix test

810c4c4

Indentation fix

478ddc8

Fix isEmpty and split between lines

e702cbd

Address issues with multi-line code fragments

fa330ed

Replace == with ===

6ddb01e

mock shuffleManager

13a2dd5

.kubernetes. => .k8s.

3f769ca

move to k8s subdir

6171267

fix package clause to k8s

47ece87

mock nodeAffinityExecutorPodModifier

67a99d9

remove commented code

4f0beb5

erikerlandson force-pushed the add-testing branch from 9bf7256 to 4f0beb5 Compare September 28, 2017 23:09

mccheah suggested changes Sep 28, 2017

View reviewed changes

erikerlandson added 3 commits September 28, 2017 16:25

move when clause to before{} block

7014961

mock initContainerBootstrap, smallFiles

8627cd2

insert actual logic into smallFiles mock

36f5c25

mccheah reviewed Sep 29, 2017

View reviewed changes

erikerlandson added 2 commits October 2, 2017 15:06

verify application of nodeAffinityExecutorPodModifier

8fc10e6

avoid cumulative invocation

0892830

ifilonenko self-requested a review October 4, 2017 17:39

mccheah reviewed Oct 5, 2017

View reviewed changes

Fixed env-var check to include values, removed mock for small files

e278aca

mccheah approved these changes Oct 10, 2017

View reviewed changes

foxish merged commit 887fdce into branch-2.2-kubernetes Oct 10, 2017

foxish deleted the add-testing branch October 10, 2017 21:48

Add unit-testing for executorpodfactory #491

Add unit-testing for executorpodfactory #491

Conversation

foxish commented Sep 15, 2017

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

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

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

erikerlandson commented Sep 28, 2017

erikerlandson commented Sep 28, 2017 • edited Loading

mccheah commented Sep 28, 2017

mccheah commented Sep 28, 2017

erikerlandson commented Sep 28, 2017

erikerlandson commented Sep 28, 2017

foxish commented Sep 28, 2017 via email

erikerlandson commented Sep 28, 2017

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

mccheah Sep 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

foxish Oct 10, 2017 • edited Loading

Choose a reason for hiding this comment

mccheah commented Oct 9, 2017

foxish commented Oct 10, 2017

mccheah left a comment

Choose a reason for hiding this comment

erikerlandson commented Sep 28, 2017 •

edited

Loading

mccheah Sep 29, 2017 •

edited

Loading

foxish Oct 10, 2017 •

edited

Loading