Improve servicePort validation performance.

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

@@ -13,6 +13,7 @@ import org.jgrapht.alg.CycleDetector
 import org.jgrapht.graph._
 
 import scala.collection.JavaConversions._
+import scala.collection.mutable
 
 case class Group(
     id: PathId,
@@ -254,26 +255,49 @@ object Group {
   private def validPorts: Validator[Group] = {
     new Validator[Group] {
       override def apply(group: Group): Result = {
-        val groupViolations = group.transitiveApps.flatMap { app =>
-          val servicePorts = app.servicePorts.toSet
-
-          val ruleViolations = for {
-            existingApp <- group.transitiveApps
-            if existingApp.id != app.id // in case of an update, do not compare the app against itself
-            existingServicePort <- existingApp.servicePorts
-            if existingServicePort != 0 // ignore zero ports, which will be chosen at random
-            if servicePorts contains existingServicePort
+        // Each service port should be unique across a cluster. We
+        // want to report conflicts if the same service port is used
+        // by multiple apps.
+        // We construct the mapping servicePort <- Set[AppKey]. This
+        // allows us to report any service port that has more than 1
+        // application using the same port.
+
+        // We keep track of the total number of ports to support an
+        // early exit condition.
+        var ports: Int = 0
+        var merged =
+          new mutable.HashMap[Int, mutable.Set[AppDefinition.AppKey]] with mutable.MultiMap[Int, AppDefinition.AppKey]
+
+        // Add each servicePort <- Application to the map.
+        for {
+          app <- group.transitiveApps
+          // We ignore randomly assigned ports identified by `0`.
+          port <- app.servicePorts if port != 0
+        } {
+          ports += 1
+          merged.addBinding(port, app.id)
+        }
+
+        // If the total number of unique ports is equal to the number
+        // of requested ports then we know there are no conflicts.
+        if (merged.size == ports) {
+          Success
+        } else {
+          // Otherwise we find all ports that have more than 1 app
+          // interested in them.
+
+          // We report all the conflicting apps along with which other
+          // apps they conflict with.
+          val violations = for {
+            (port, apps) <- merged if apps.size > 1
+            app <- apps
           } yield RuleViolation(
-            app.id,
-            s"Requested service port $existingServicePort conflicts with a service port in app ${existingApp.id}",
+            app,
+            s"Requested service port $port is used by more than 1 app: ${apps.mkString(", ")}",
             None)
 
-          if (ruleViolations.isEmpty) None
-          else Some(GroupViolation(app, "app contains conflicting ports", None, ruleViolations.toSet))
+          Failure(violations.toSet)
         }
-
-        if (groupViolations.isEmpty) Success
-        else Failure(groupViolations.toSet)
       }
     }
   }

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

@@ -55,7 +55,7 @@ class ModelValidationTest
     val result = validate(group)(Group.validRootGroup(maxApps = None))
 
     ValidationHelper.getAllRuleConstrains(result).exists(v =>
-      v.message == "Requested service port 3200 conflicts with a service port in app /app2") should be(true)
+      v.message.contains("Requested service port 3200 is used by more than 1 app")) should be(true)
   }
 
   test("Model validation should allow new apps that do not conflict with service ports in existing apps") {
@@ -77,7 +77,7 @@ class ModelValidationTest
     val result = validate(group)(Group.validRootGroup(maxApps = None))
 
     ValidationHelper.getAllRuleConstrains(result).exists(v =>
-      v.message == "Requested service port 3200 conflicts with a service port in app /app2") should be(true)
+      v.message.contains("Requested service port 3200 is used by more than 1 app")) should be(true)
   }
 
   test("Multiple errors within one field of a validator should be grouped into one array") {

src/test/scala/mesosphere/marathon/state/GroupTest.scala

@@ -256,7 +256,7 @@ class GroupTest extends FunSpec with GivenWhenThen with Matchers {
       val result = validate(root)(Group.validRootGroup(maxApps = None))
 
       Then("the validation returns an errror")
-      ValidationHelper.getAllRuleConstrains(result).head.message should include ("conflicts with a service port")
+      ValidationHelper.getAllRuleConstrains(result).head.message should include ("used by more than 1 app")
     }
 
     it("can marshal and unmarshal from to protos") {