Enable testing against GCE clusters #243

foxish · 2017-04-25T22:02:27Z

Changes to the test framework to run a subset of the tests we have now against a GCE cluster
I'll convert the other tests to work against multi-node clusters in subsequent PRs.

cc @mccheah @ash211

foxish · 2017-04-25T22:08:20Z

I'd like to get this one reviewed before all else, because it makes the workflow a bit simpler without minikube.

mccheah · 2017-04-25T22:12:28Z

...sts/src/test/scala/org/apache/spark/deploy/kubernetes/integrationtest/KubernetesClient.scala

+  val SingleNode, MultiNode = Value
+}
+
+object KubernetesClient {


Can this be put into KubernetesTestComponents? https://github.com/apache-spark-on-k8s/spark/blob/branch-2.1-kubernetes/resource-managers/kubernetes/integration-tests/src/test/scala/org/apache/spark/deploy/kubernetes/integrationtest/KubernetesTestComponents.scala

Or perhaps renamed to KubernetesIntegrationTestsFixtures. Primarily it would be good to:

Contrast this with KubernetesTestComponents if they are different, and

Rename from KubernetesClient since this is also used by the Fabric8 client - maybe KubernetesTestClient?

Some Scaladoc on this class and its purpose could be a good idea, e.g. "Controls the Kubernetes cluster that integration tests are run against."

KubernetesTestClient makes sense. Will do & add doc.

foxish · 2017-04-25T22:17:29Z

rerun unit tests please

mccheah · 2017-04-25T22:23:21Z

...test/scala/org/apache/spark/deploy/kubernetes/integrationtest/KubernetesTestComponents.scala

+  val SingleNode, MultiNode = Value
+}
+
+object KubernetesClient {


I don't think this necessarily has to be an object as a static singleton. Similarly for Minikube - we did start off with that being an object but it would probably be cleaner to use classes and dynamic instances if only to move away from global state.

If KubernetesV1Suite and KubernetesV2Suite need to share instances of these, they can be passed into the constructors from the parent KubernetesSuite#nestedSuites.

Passing it around everywhere seemed less than ideal. Since this will be shared across all testsuites, I felt this was easier to understand. Is it an anti-pattern here?

For Minikube considering it's managing global state anyways (the execution of the Minikube binary) that one perhaps makes sense to stay as an object. Otherwise, I think making the sharing explicit in the method contract would make the test dependencies easier to trace around. For example, right now KubernetesV2Suite can be executed on its own e.g. in IntelliJ but when it does it won't have the lifecycle management that KubernetesSuite is setting up.

Hmm.. I see. That's a good point. It seems like if I put in those args, we'll lose the ability to run each of those tests separately from intellij. Any way to keep both? What do you think of having each testsuite (v1 and v2) initialize its own client? Since eventually we're only going to have one, it would be temporary.

I think this was the idea we had before - KubernetesTestComponents is responsible for the Kubernetes client, but the parent suite KubernetesSuite is responsible for the global state, that is, the Minikube cluster. Global state should still be kept in the Minikube object - the user can run each individual test by starting Minikube and building the docker images themselves manually, for example. We essentially want to preserve these semantics.

mccheah · 2017-04-25T22:24:49Z

...test/scala/org/apache/spark/deploy/kubernetes/integrationtest/KubernetesTestComponents.scala

+  }
+
+  private def createDefaultClient(): Unit = {
+    System.getProperty("spark.docker.test.master") match {


Option(System.getProperty(...)).map(...).getOrElse(...)

mccheah · 2017-04-25T22:28:48Z

...tegration-tests/src/test/scala/org/apache/spark/deploy/kubernetes/integrationtest/Util.scala

+import org.apache.spark.internal.Logging
+import org.apache.spark.util.Utils
+
+object Util extends Logging {


Use specific names for different kinds of utilities. E.g. UnixProcessUtils.

Javadoc this method also.

mccheah · 2017-04-25T22:37:51Z

...test/scala/org/apache/spark/deploy/kubernetes/integrationtest/KubernetesTestComponents.scala

+
+object KubernetesClient {
+  var defaultClient: DefaultKubernetesClient = _
+  var testBackend: TestBackend.Value = _


This could perhaps be thought of in a polymorphic approach. What if we had IntegrationTestBackend as the parent class with methods like initialize (Minikube builds docker images), getKubernetesClient and cleanUp, then, a factory class switches on the setting of the system property to choose the implementation.

Instead of getKubernetesClient being an abstract method on such an abstract parent class, maybe getKubernetesClient is a final method which invokes the polymorphic getKubernetesClientConfig which varies depending on the backend.

That's a good idea. I'm tempted to punt it for later though, since right now there's still quite a bit of Minikube being used directly in a lot of places. Ideally, once we move all of it out and clearly define the boundaries of backends, we can abstract out all the cluster specific operations into IntegrationTestBackend abstract parent, or trait. WDYT?

I believe that the polymorphic test architecture could belong in the scope of this PR - if we're moving towards multiple back ends, it would be nice if we captured the overall architecture on this pass. If we are concerned that such a change would create too large of a diff, can we create a separate PR that merges into this one that implements the polymorphic architecture, merge said separate PR first into this branch, and then merge this PR?

This is primarily to ease review. Much of what could be commented on for this PR could be contingent on whether or not a polymorphic approach is used.

Dependent PR works. I'll send one out later today then.

Opened #248

tnachen · 2017-04-26T00:34:23Z

+1!

* Restructured the test backends * Address comments * var -> val

mccheah · 2017-04-29T02:36:17Z

...test/scala/org/apache/spark/deploy/kubernetes/integrationtest/KubernetesTestComponents.scala

+    servicePortName: String,
+    servicePath: String = ""): T = synchronized {
+    val kubernetesMaster = s"${defaultClient.getMasterUrl}"
+    val httpClient = getHttpClient(kubernetesClient.asInstanceOf[BaseClient])


Do we use this anywhere?

mccheah

Missed a tiny detail from the upstream PR. There also appears to be an unused http client field. After those changes I'm ok to merge this.

mccheah · 2017-04-29T02:38:01Z

...cala/org/apache/spark/deploy/kubernetes/integrationtest/backend/IntegrationTestBackend.scala

+private[spark] object IntegrationTestBackendFactory {
+  def getTestBackend(): IntegrationTestBackend = {
+    Option(System.getProperty("spark.kubernetes.test.master")).map {
+      master =>


Nit - Can use .map(new GCETestBackend(_)) where the underscore passes in the master.

foxish · 2017-05-01T21:11:40Z

Looks like the integration test is wedged. @kimoonkim @varunkatta could you please restart it?

foxish · 2017-05-01T22:41:42Z

@mccheah tests passed. ok to merge?

mccheah · 2017-05-01T22:43:35Z

...test/scala/org/apache/spark/deploy/kubernetes/integrationtest/KubernetesTestComponents.scala

+    HttpClientUtil.createClient[T](Set(url), 5, sslContext.getSocketFactory, trustManager)
+  }
+
+  def getHttpClient(client: BaseClient): OkHttpClient = {


Do we use this anywhere?

Looks like I missed that one. Sorry!
We'll likely need that http client later, because I think we shouldn't rely on minikube credentials to hit the api-server proxy endpoint, but I'll get rid of it now.

* Part 1: making test code cluster-agnostic * Final checked * Move all test code into KubernetesTestComponents * Addressed comments * Fixed doc * Restructure the test backends (#248) * Restructured the test backends * Address comments * var -> val * Comments * removed deadcode

* Part 1: making test code cluster-agnostic * Final checked * Move all test code into KubernetesTestComponents * Addressed comments * Fixed doc * Restructure the test backends (apache-spark-on-k8s#248) * Restructured the test backends * Address comments * var -> val * Comments * removed deadcode

foxish added 2 commits April 25, 2017 14:33

Part 1: making test code cluster-agnostic

cb24332

Final checked

3f6bc24

mccheah reviewed Apr 25, 2017

View reviewed changes

Move all test code into KubernetesTestComponents

1f63014

mccheah reviewed Apr 25, 2017

View reviewed changes

foxish added 2 commits April 25, 2017 16:51

Addressed comments

ed1cc27

Fixed doc

b7ac04e

foxish mentioned this pull request Apr 27, 2017

Restructure the test backends #248

Merged

Restructure the test backends (#248)

d4d2e9c

* Restructured the test backends * Address comments * var -> val

mccheah reviewed Apr 29, 2017

View reviewed changes

mccheah suggested changes Apr 29, 2017

View reviewed changes

Comments

de0df15

mccheah reviewed May 1, 2017

View reviewed changes

removed deadcode

d83f8d1

mccheah approved these changes May 2, 2017

View reviewed changes

mccheah merged commit 6b489c2 into branch-2.1-kubernetes May 2, 2017

foxish mentioned this pull request May 3, 2017

Run tests against any specified k8s cluster #100

Closed

foxish deleted the multi-k8s-tests branch July 25, 2017 00:48

ifilonenko pushed a commit to ifilonenko/spark that referenced this pull request Feb 26, 2019

Redact passwords from conda output (apache-spark-on-k8s#243)

692e6f8

Enable testing against GCE clusters #243

Enable testing against GCE clusters #243

Uh oh!

Conversation

foxish commented Apr 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

foxish commented Apr 25, 2017

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!

foxish commented Apr 25, 2017

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnachen commented Apr 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mccheah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

foxish commented May 1, 2017

Uh oh!

foxish commented May 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

foxish commented Apr 25, 2017 •

edited

Loading