WIP: DO NOT MERGE: fix problem with docker USER networking w/ portMappings and ipAddress #4008

jdef · 2016-06-20T23:24:17Z

#3998 was merged prematurely. this PR attempts to fix additional bugs found in integration testing w/ development builds of dc/os

TODO: unit test for PORT envvar generation that tests error condition reported (and fixed) below
- scenarios: (a) no ports are defined; (b) portMappings = [{}]
TODO: see UGLY WIP: DO NOT MERGE: fix problem with docker USER networking w/ portMappings and ipAddress #4008 (comment)
- this probably is not a blocker for this PR, but should have a follow-up PR to fix

jdef · 2016-06-20T23:29:57Z

src/main/scala/mesosphere/marathon/state/GroupManager.scala

@@ -218,19 +218,22 @@ class GroupManager @Inject() (
      port
    }

-    def mergeServicePortsAndPortDefinitions(portDefinitions: Seq[PortDefinition], servicePorts: Seq[Int]) = {
-      portDefinitions.zipAll(servicePorts, AppDefinition.RandomPortDefinition, AppDefinition.RandomPortValue).map {


This is inserting service ports even when portDefinitions is empty! So I fixed it, and then service ports weren't merging properly with container port mappings. So I changed those bits too.

jdef · 2016-06-21T00:09:08Z

src/test/scala/mesosphere/marathon/api/v2/AppsResourceTest.scala

@@ -341,6 +342,89 @@ class AppsResourceTest extends MarathonSpec with MarathonActorSupport with Match
    JsonTestHelper.assertThatJsonString(response.getEntity.asInstanceOf[String]).correspondsToJsonOf(expected)
  }

+  /*
+  test("Create a new app with IP/CT with virtual network foo w/ Docker") {


I really want this version of the unit test to work but I get a validation error with ClassCastExceptions that follow. no comprendo.

@gkleiman @aquamatthias suggestions welcome

The idea of the tests in this class is to exercise AppsResource's methods with mocked dependencies, not with a proper GroupManager.

Do we already have a test in GroupManager that checks that it assign the right ports with an app like the one in this test? If the answer is yes, then we don't need this one. If it is not, then maybe we should add one there =).

I tried to make this test pass, but there are multiple problems with it, and I am not sure if we really need it.

I started hacking on the group manager tests because this seemed futile. Was hoping that I was just missing something simple but from your explanation it seems I'm pushing the limits of what this suite of unit tests is aimed at.

There's probably a case to make for tests that live somewhere in between simpler unit testing and full-fledged integration testing. For example I want to round-trip test from a JSON create-app request down through an actual group manager, without the hassle of setting up a mesos environment that supports overlay networking.

We can probably remove this commented test for now since I've tried to augment via group manager tests, even though it's less ideal IMHO.

jdef · 2016-06-21T03:47:44Z

seeing a new error on a hot-patched cluster during integration testing:

Jun 21 03:46:50 ip-10-10-0-199 java[20164]: [2016-06-21 03:46:50,183] ERROR empty.head (akka.actor.OneForOneStrategy:marathon-akka.actor.default-dispatcher-9)
Jun 21 03:46:50 ip-10-10-0-199 java[20164]: java.lang.UnsupportedOperationException: empty.head
Jun 21 03:46:50 ip-10-10-0-199 java[20164]: at scala.collection.immutable.Vector.head(Vector.scala:193)
Jun 21 03:46:50 ip-10-10-0-199 java[20164]: at mesosphere.mesos.TaskBuilder$.portsEnv(TaskBuilder.scala:418)
Jun 21 03:46:50 ip-10-10-0-199 java[20164]: at mesosphere.mesos.TaskBuilder$.commandInfo(TaskBuilder.scala:301)
Jun 21 03:46:50 ip-10-10-0-199 java[20164]: at mesosphere.mesos.TaskBuilder.build(TaskBuilder.scala:131)
Jun 21 03:46:50 ip-10-10-0-199 java[20164]: at mesosphere.mesos.TaskBuilder.build(TaskBuilder.scala:66)
Jun 21 03:46:50 ip-10-10-0-199 java[20164]: at mesosphere.mesos.TaskBuilder.buildIfMatches(TaskBuilder.scala:85)
Jun 21 03:46:50 ip-10-10-0-199 java[20164]: at mesosphere.marathon.core.launcher.impl.TaskOpFactoryImpl.inferNormalTaskOp(TaskOpFactoryImpl.scala:50)
Jun 21 03:46:50 ip-10-10-0-199 java[20164]: at mesosphere.marathon.core.launcher.impl.TaskOpFactoryImpl.buildTaskOp(TaskOpFactoryImpl.scala:43)
Jun 21 03:46:50 ip-10-10-0-199 java[20164]: at mesosphere.marathon.core.launchqueue.impl.TaskLauncherActor$$anonfun$receiveProcessOffers$1.applyOrElse(TaskLauncherActor.scala:363)
Jun 21 03:46:50 ip-10-10-0-199 java[20164]: at scala.PartialFunction$OrElse.applyOrElse(PartialFunction.scala:170)
Jun 21 03:46:50 ip-10-10-0-199 java[20164]: at scala.PartialFunction$OrElse.applyOrElse(PartialFunction.scala:171)
Jun 21 03:46:50 ip-10-10-0-199 java[20164]: at scala.PartialFunction$OrElse.applyOrElse(PartialFunction.scala:171)
Jun 21 03:46:50 ip-10-10-0-199 java[20164]: at scala.PartialFunction$OrElse.applyOrElse(PartialFunction.scala:171)
Jun 21 03:46:50 ip-10-10-0-199 java[20164]: at scala.PartialFunction$OrElse.applyOrElse(PartialFunction.scala:171)
Jun 21 03:46:50 ip-10-10-0-199 java[20164]: at scala.PartialFunction$OrElse.applyOrElse(PartialFunction.scala:171)
Jun 21 03:46:50 ip-10-10-0-199 java[20164]: at scala.PartialFunction$OrElse.applyOrElse(PartialFunction.scala:171)
Jun 21 03:46:50 ip-10-10-0-199 java[20164]: at akka.actor.Actor$class.aroundReceive(Actor.scala:467)
Jun 21 03:46:50 ip-10-10-0-199 java[20164]: at mesosphere.marathon.core.launchqueue.impl.TaskLauncherActor.aroundReceive(TaskLauncherActor.scala:76)
Jun 21 03:46:50 ip-10-10-0-199 java[20164]: at akka.actor.ActorCell.receiveMessage(ActorCell.scala:516)
Jun 21 03:46:50 ip-10-10-0-199 java[20164]: at akka.actor.ActorCell.invoke(ActorCell.scala:487)
Jun 21 03:46:50 ip-10-10-0-199 java[20164]: at akka.dispatch.Mailbox.processMailbox(Mailbox.scala:238)
Jun 21 03:46:50 ip-10-10-0-199 java[20164]: at akka.dispatch.Mailbox.run(Mailbox.scala:220)
Jun 21 03:46:50 ip-10-10-0-199 java[20164]: at akka.dispatch.ForkJoinExecutorConfigurator$AkkaForkJoinTask.exec(AbstractDispatcher.scala:397)
Jun 21 03:46:50 ip-10-10-0-199 java[20164]: at scala.concurrent.forkjoin.ForkJoinTask.doExec(ForkJoinTask.java:260)
Jun 21 03:46:50 ip-10-10-0-199 java[20164]: at scala.concurrent.forkjoin.ForkJoinPool$WorkQueue.runTask(ForkJoinPool.java:1339)
Jun 21 03:46:50 ip-10-10-0-199 java[20164]: at scala.concurrent.forkjoin.ForkJoinPool.runWorker(ForkJoinPool.java:1979)
Jun 21 03:46:50 ip-10-10-0-199 java[20164]: at scala.concurrent.forkjoin.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:107)

jdef · 2016-06-21T05:47:04Z

previous reported error caused with this app JSON (note: hostport == None; only one port mapping)

{
  "id": "jdef-networking",
  "cmd": "ip -o addr; sleep 30",
  "cpus": 0.10,
  "mem": 64,
  "instances": 1,
  "backoffFactor": 1.14472988585,
  "backoffSeconds": 5,
  "ipAddress": { "networkName": "overlay-1" },
  "container": {
    "type": "DOCKER",
    "docker": {
      "network": "USER",
      "image": "busybox",
      "portMappings": [{
        "containerPort": 123, "servicePort": 80, "name": "foo"
      }]
    }
  }
}

jdef · 2016-06-21T14:01:29Z

src/main/scala/mesosphere/marathon/state/GroupManager.scala

  private[state] def assignDynamicServicePorts(from: Group, to: Group): Group = {
    val portRange = Range(config.localPortMin(), config.localPortMax())
-    var taken = from.transitiveApps.flatMap(_.portNumbers) ++ to.transitiveApps.flatMap(_.portNumbers)
+    var taken = (from.transitiveApps.flatMap(app => app.hostPorts.flatten ++ app.servicePorts) ++
+      to.transitiveApps.flatMap(app => app.hostPorts.flatten ++ app.servicePorts)).toSet


TODO: write unit test for this change.
Symptom: multiple apps using port mappings that didn't specify service ports would result in Marathon attempting to reassign the same service port (that was already assigned to app1) to app2. (e.g. portMappings: [{}])

jdef · 2016-06-21T15:07:12Z

PORTS still seems inconsistent (because the value of PORT0 isn't in the PORTS list):

PORTS=23409,10003
PORT0=123
PORT1=23409
PORT2=10003
PORT=23409
PORT_23409=23409
PORT_123=123
PORT_10003=10003

given app def:

{
  "id": "/jdef-networking",
  "cmd": "env; ip -o addr; sleep 30",
  "cpus": 0.1, "mem": 64, "instances": 1,
  "container": {
    "type": "DOCKER",
    "docker": {
      "image": "busybox",
      "network": "USER",
      "portMappings": [
        { "containerPort": 123 },
        { "hostPort": 23409 },
        {}
      ]
    }
  }
}

gkleiman · 2016-06-22T11:18:58Z

src/main/scala/mesosphere/mesos/TaskBuilder.scala

@@ -415,7 +415,7 @@ object TaskBuilder {
      }

      val allAssigned = effectivePorts.flatten ++ generatedPorts.values
-      env += ("PORT" -> allAssigned.head.toString)
+      allAssigned.headOption.foreach { port => env += ("PORT" -> port.toString) }


Should we add a test in which no ports are requested?

yes. I've elaborated on the TODO in the top-level desc of this PR

gkleiman · 2016-06-22T14:08:51Z

src/main/scala/mesosphere/marathon/state/GroupManager.scala

  private[state] def assignDynamicServicePorts(from: Group, to: Group): Group = {
    val portRange = Range(config.localPortMin(), config.localPortMax())
-    var taken = from.transitiveApps.flatMap(_.portNumbers) ++ to.transitiveApps.flatMap(_.portNumbers)
+    var taken = (from.transitiveApps.flatMap(app => app.hostPorts.flatten ++ app.servicePorts) ++


Would the following also work?

var taken = from.transitiveApps.flatMap(_.servicePorts) ++ to.transitiveApps.flatMap(_.servicePorts)

I'm seeing a theme here where you want to use service ports instead of host ports everywhere. I don't understand it. I'm probably missing something obvious. My understanding of portNumbers was that they were cousins of hostPorts. servicePorts in my mind are completely different

jdef · 2016-06-22T19:38:43Z

UGLY: when in docker USER network mode generated container ports (when containerPort == 0 and hostPort == None) do indeed result in PORT0 being populated. so that's useful from an app perspective but it's not aligned with the service port and the appdef isn't updated with the generated container port -- which means that nothing outside of the app can associate the PORT0 value with the service port. not very helpful.

jdef · 2016-06-22T21:03:16Z

src/main/scala/mesosphere/mesos/TaskBuilder.scala

+
+      containerPorts.getOrElse(runSpec.portNumbers)
+    }
+    val portNames = {


fixes #4005

aquamatthias · 2016-06-23T15:28:02Z

Thanks!

…ss (#4008)

fix problem with docker USER networking w/ portMappings and ipAddress

401d971

jdef added the in progress label Jun 20, 2016

jdef reviewed Jun 20, 2016
View reviewed changes

jdef changed the title ~~fix problem with docker USER networking w/ portMappings and ipAddress~~ WIP: DO NOT MERGE: fix problem with docker USER networking w/ portMappings and ipAddress Jun 20, 2016

James DeFelice added 2 commits June 20, 2016 23:35

DO NOT MERGE THIS TO MASTER: commented out failing unit test

523a90c

restored original AppsResourceTest

0fb6eb9

jdef reviewed Jun 21, 2016
View reviewed changes

bugfix: handle the case wherein no ports are actually assigned

ec56c92

jdef mentioned this pull request Jun 21, 2016

update to marathon TC snapshot publish 199 karya0/dcos#2

Merged

jdef added the bug label Jun 21, 2016

avoid reassigning service ports that are already taken

b9745f7

jdef reviewed Jun 21, 2016
View reviewed changes

unit tests for assignDynamicServicePorts

f1d2aa9

jdef assigned jdef, gkleiman and aquamatthias Jun 21, 2016

gkleiman reviewed Jun 22, 2016
View reviewed changes

gkleiman unassigned gkleiman and aquamatthias Jun 22, 2016

James DeFelice added 3 commits June 22, 2016 20:28

added TODO for port name generation

eb75f2e

cleanup as per code review comments

063bd8b

generate port names for containers using portMappings

76c987d

jdef reviewed Jun 22, 2016
View reviewed changes

gkleiman added 2 commits June 23, 2016 15:30

Cleanup service ports allocation code

dc26a56

Add missing tests

cf6039b

gkleiman assigned aquamatthias and unassigned jdef Jun 23, 2016

aquamatthias merged commit 55b0d0d into master Jun 23, 2016

aquamatthias deleted the jdef_fix_merge_service_ports_and_port_definitions branch June 23, 2016 15:27

aquamatthias removed the in progress label Jun 23, 2016

aquamatthias added a commit that referenced this pull request Jun 23, 2016

Fixes problem with docker USER networking w/ portMappings and ipAddre…

7a56fde

…ss (#4008)

aquamatthias added a commit that referenced this pull request Jun 23, 2016

Fixes problem with docker USER networking w/ portMappings and ipAddre…

0d8310b

…ss (#4008)

kensipe mentioned this pull request Jun 29, 2016

Also create 'PORT_<NAME>' env variables for ports defined in port mappings #4005

Closed

marcomonaco added the pr label Mar 6, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: DO NOT MERGE: fix problem with docker USER networking w/ portMappings and ipAddress #4008

WIP: DO NOT MERGE: fix problem with docker USER networking w/ portMappings and ipAddress #4008

jdef commented Jun 20, 2016 •

edited

Loading

jdef Jun 20, 2016

jdef Jun 21, 2016

jdef Jun 21, 2016

gkleiman Jun 22, 2016

jdef Jun 22, 2016

jdef commented Jun 21, 2016

jdef commented Jun 21, 2016

jdef Jun 21, 2016

jdef Jun 21, 2016

jdef commented Jun 21, 2016 •

edited

Loading

gkleiman Jun 22, 2016

jdef Jun 22, 2016

gkleiman Jun 22, 2016

jdef Jun 22, 2016

jdef commented Jun 22, 2016

jdef Jun 22, 2016

aquamatthias commented Jun 23, 2016

WIP: DO NOT MERGE: fix problem with docker USER networking w/ portMappings and ipAddress #4008

WIP: DO NOT MERGE: fix problem with docker USER networking w/ portMappings and ipAddress #4008

Conversation

jdef commented Jun 20, 2016 • edited Loading

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

jdef commented Jun 21, 2016

jdef commented Jun 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdef commented Jun 21, 2016 • edited Loading

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

jdef commented Jun 22, 2016

Choose a reason for hiding this comment

aquamatthias commented Jun 23, 2016

jdef commented Jun 20, 2016 •

edited

Loading

jdef commented Jun 21, 2016 •

edited

Loading