[SPARK-21694][MESOS] Support Mesos CNI network labels by susanxhuynh · Pull Request #18910 · apache/spark

susanxhuynh · 2017-08-10T18:38:08Z

JIRA ticket: https://issues.apache.org/jira/browse/SPARK-21694

What changes were proposed in this pull request?

Spark already supports launching containers attached to a given CNI network by specifying it via the config spark.mesos.network.name.

This PR adds support to pass in network labels to CNI plugins via a new config option spark.mesos.network.labels. These network labels are key-value pairs that are set in the NetworkInfo of both the driver and executor tasks. More details in the related Mesos documentation: http://mesos.apache.org/documentation/latest/cni/#mesos-meta-data-to-cni-plugins

How was this patch tested?

Unit tests, for both driver and executor tasks.
Manual integration test to submit a job with the spark.mesos.network.labels option, hit the mesos/state.json endpoint, and check that the labels are set in the driver and executor tasks.

@ArtRand @skonto

…gins via

SparkQA · 2017-08-10T18:54:00Z

Test build #80498 has finished for PR 18910 at commit 255e519.

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

skonto · 2017-08-10T22:10:41Z

...esos/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerBackendUtil.scala

 import org.apache.mesos.Protos.ContainerInfo.{DockerInfo, MesosInfo}

 import org.apache.spark.{SparkConf, SparkException}
+import org.apache.spark.deploy.mesos.config._


Might be better to avoid wildcard import.

+1, this makes it more consistent with MesosClusterScheduler

skonto · 2017-08-10T22:15:01Z

...esos/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerBackendUtil.scala


    conf.getOption("spark.mesos.network.name").map { name =>
-      val info = NetworkInfo.newBuilder().setName(name).build()
+      val networkLabels = MesosProtoUtils.mesosLabels(conf.get(NETWORK_LABELS).getOrElse(""))


Not part of this PR but...shouldn't spark.mesos.network.name have a config name like labels have (NETWORK_LABELS)?
At some point I think all mesos specific config options should be placed in the config file to be easily managed and referenced.

+1 for making the NETWORK_LABELS alias.

skonto · 2017-08-10T22:17:01Z

@susanxhuynh have you tested how spark jobs behave with a real cni network this PR is enough right, just curious...

ArtRand

Thanks @susanxhuynh. Just a question about your doc string and testing.

ArtRand · 2017-08-11T15:33:29Z

resource-managers/mesos/src/main/scala/org/apache/spark/deploy/mesos/config.scala

+    ConfigBuilder("spark.mesos.network.labels")
+      .doc("Network labels to pass to CNI plugins.  This is a comma-separated list " +
+        "of key-value pairs, where each key-value pair has the format key:value. " +
+        "Example: key1=val1,key2=val2")


I thought we use : to separate the key-value pairs? So it would say:
`Example: key1:val1,key2:val2

ArtRand · 2017-08-11T15:34:09Z

...esos/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerBackendUtil.scala

 import org.apache.mesos.Protos.ContainerInfo.{DockerInfo, MesosInfo}

 import org.apache.spark.{SparkConf, SparkException}
+import org.apache.spark.deploy.mesos.config._


+1, this makes it more consistent with MesosClusterScheduler

ArtRand · 2017-08-11T15:34:30Z

...esos/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerBackendUtil.scala


    conf.getOption("spark.mesos.network.name").map { name =>
-      val info = NetworkInfo.newBuilder().setName(name).build()
+      val networkLabels = MesosProtoUtils.mesosLabels(conf.get(NETWORK_LABELS).getOrElse(""))


+1 for making the NETWORK_LABELS alias.

ArtRand · 2017-08-11T15:36:43Z

...scala/org/apache/spark/scheduler/cluster/mesos/MesosCoarseGrainedSchedulerBackendSuite.scala

+    assert(networkInfos.get(0).getLabels.getLabels(0).getKey == "key1")
+    assert(networkInfos.get(0).getLabels.getLabels(0).getValue == "val1")
+    assert(networkInfos.get(0).getLabels.getLabels(1).getKey == "key2")
+    assert(networkInfos.get(0).getLabels.getLabels(1).getValue == "val2")


Is there a way to test that the driver's worker tasks also get the labels? It's important that all of the ContainerInfos associated with a job (not jus the driver scheduler) get the labels for CNI to work.

@ArtRand I believe this is testing the worker tasks: (L.572) the setBackend() method creates a MesosCoarseGrainedSchedulerBackend, L.577-579 creates the requirements for an Executor, (L. 580) backend.resourceOffers() looks for an offer that matches the requirements and launches an Executor task.
There is a separate unit test above, in MesosClusterSchedulerSuite.scala, which checks for the labels in the driver itself.

…'NETWORK_NAME' to config object.

SparkQA · 2017-08-11T22:30:05Z

Test build #80555 has finished for PR 18910 at commit d261593.

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

susanxhuynh · 2017-08-11T22:36:28Z

@skonto @ArtRand Thanks for the feedback. I have fixed the documentation and added NETWORK_NAME to the config object. Please let me know what you think.

@skonto I have not tested this particular change on a real CNI network. I think the only difference is that a Spark job runs on a different network, but there's no change in the Spark functionality.

SparkQA · 2017-08-11T22:41:00Z

Test build #80556 has finished for PR 18910 at commit dc09312.

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

ArtRand · 2017-08-21T13:13:17Z

LGTM.

susanxhuynh · 2017-08-21T13:43:48Z

@vanzin Would you mind helping review this PR? Thanks.

susanxhuynh · 2017-08-23T15:34:10Z

@srowen Sean, would you like to review this PR? Thanks.

srowen · 2017-08-23T16:00:52Z

resource-managers/mesos/src/main/scala/org/apache/spark/deploy/mesos/config.scala

      .doubleConf
      .createWithDefault(0.0)
+
+  private [spark] val NETWORK_NAME =


Tiny nit, could you remove the space after private? that would make it consistent with the rest of the code.

I trust your judgment as to whether this is the right functional change for Mesos and the details and style look OK to me otherwise.

SparkQA · 2017-08-23T22:29:19Z

Test build #81051 has finished for PR 18910 at commit 38e3b4d.

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

susanxhuynh · 2017-08-23T22:34:03Z

@srowen Thanks for reviewing. I've removed those spaces; please take a look.

srowen · 2017-08-24T09:06:26Z

Merged to master

JIRA ticket: https://issues.apache.org/jira/browse/SPARK-21694 Spark already supports launching containers attached to a given CNI network by specifying it via the config `spark.mesos.network.name`. This PR adds support to pass in network labels to CNI plugins via a new config option `spark.mesos.network.labels`. These network labels are key-value pairs that are set in the `NetworkInfo` of both the driver and executor tasks. More details in the related Mesos documentation: http://mesos.apache.org/documentation/latest/cni/#mesos-meta-data-to-cni-plugins Unit tests, for both driver and executor tasks. Manual integration test to submit a job with the `spark.mesos.network.labels` option, hit the mesos/state.json endpoint, and check that the labels are set in the driver and executor tasks. ArtRand skonto Author: Susan X. Huynh <xhuynh@mesosphere.com> Closes apache#18910 from susanxhuynh/sh-mesos-cni-labels.

[SPARK-21694][MESOS] Allow the user to pass network labels to CNI plu…

255e519

…gins via

skonto reviewed Aug 10, 2017

View reviewed changes

ArtRand suggested changes Aug 11, 2017

View reviewed changes

susanxhuynh added 2 commits August 11, 2017 15:10

Addressed review feedback: fixed the example in documentation; added …

d261593

…'NETWORK_NAME' to config object.

One more doc fix.

dc09312

srowen reviewed Aug 23, 2017

View reviewed changes

Fixed style

38e3b4d

asfgit closed this in ce0d3bb Aug 24, 2017

susanxhuynh deleted the sh-mesos-cni-labels branch September 19, 2017 19:17

Conversation

susanxhuynh commented Aug 10, 2017

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Aug 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skonto Aug 10, 2017 • 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!

skonto commented Aug 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ArtRand 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Aug 11, 2017

Uh oh!

susanxhuynh commented Aug 11, 2017

Uh oh!

SparkQA commented Aug 11, 2017

Uh oh!

ArtRand commented Aug 21, 2017

Uh oh!

susanxhuynh commented Aug 21, 2017

Uh oh!

susanxhuynh commented Aug 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Aug 23, 2017

Uh oh!

susanxhuynh commented Aug 23, 2017

Uh oh!

srowen commented Aug 24, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Comments

skonto Aug 10, 2017 •

edited

Loading

skonto commented Aug 10, 2017 •

edited

Loading