Merge pull request #4132 from jszczepkowski/master

Improve "constraint violation" error message.

pkg/constraint/constraint.go

@@ -17,11 +17,17 @@ limitations under the License.
 package constraint
 
 import (
+	"fmt"
+
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
 )
 
 // Allowed returns true if pods is a collection of bound pods
 // which can run without conflict on a single minion.
-func Allowed(pods []api.BoundPod) bool {
+func Allowed(pods []api.BoundPod) []error {
-	return !PortsConflict(pods)
+	errors := []error{}
+	for _, port := range hostPortsConflict(pods) {
+		errors = append(errors, fmt.Errorf("host port %v is already in use", port))
+	}
+	return errors
 }

pkg/constraint/constraint_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package constraint
 
 import (
+	"fmt"
 	"testing"
 
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
@@ -40,11 +41,11 @@ func podWithContainers(containers ...api.Container) api.BoundPod {
 
 func TestAllowed(t *testing.T) {
 	table := []struct {
-		allowed bool
+		err  string
-		pods    []api.BoundPod
+		pods []api.BoundPod
 	}{
 		{
-			allowed: true,
+			err: "[]",
 			pods: []api.BoundPod{
 				podWithContainers(
 					containerWithHostPorts(1, 2, 3),
@@ -57,7 +58,7 @@ func TestAllowed(t *testing.T) {
 			},
 		},
 		{
-			allowed: true,
+			err: "[]",
 			pods: []api.BoundPod{
 				podWithContainers(
 					containerWithHostPorts(0, 0),
@@ -70,7 +71,7 @@ func TestAllowed(t *testing.T) {
 			},
 		},
 		{
-			allowed: false,
+			err: "[host port 3 is already in use]",
 			pods: []api.BoundPod{
 				podWithContainers(
 					containerWithHostPorts(3, 3),
@@ -78,7 +79,7 @@ func TestAllowed(t *testing.T) {
 			},
 		},
 		{
-			allowed: false,
+			err: "[host port 6 is already in use]",
 			pods: []api.BoundPod{
 				podWithContainers(
 					containerWithHostPorts(6),
@@ -91,7 +92,7 @@ func TestAllowed(t *testing.T) {
 	}
 
 	for _, item := range table {
-		if e, a := item.allowed, Allowed(item.pods); e != a {
+		if e, a := item.err, Allowed(item.pods); e != fmt.Sprintf("%v", a) {
 			t.Errorf("Expected %v, got %v: \n%v\v", e, a, item.pods)
 		}
 	}

pkg/constraint/ports.go

@@ -20,10 +20,12 @@ import (
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
 )
 
-// PortsConflict returns true iff two containers attempt to expose
+// hostPortsConflict returns an array of host ports that at least two
-// the same host port.
+// containers attempt to expose. The array is empty if no such port
-func PortsConflict(pods []api.BoundPod) bool {
+// exists.
+func hostPortsConflict(pods []api.BoundPod) []int {
 	hostPorts := map[int]struct{}{}
+	conflictingPorts := []int{}
 	for _, pod := range pods {
 		for _, container := range pod.Spec.Containers {
 			for _, port := range container.Ports {
@@ -31,11 +33,11 @@ func PortsConflict(pods []api.BoundPod) bool {
 					continue
 				}
 				if _, exists := hostPorts[port.HostPort]; exists {
-					return true
+					conflictingPorts = append(conflictingPorts, port.HostPort)
 				}
 				hostPorts[port.HostPort] = struct{}{}
 			}
 		}
 	}
-	return false
+	return conflictingPorts
 }

pkg/registry/etcd/etcd.go

@@ -214,8 +214,8 @@ func (r *Registry) assignPod(ctx api.Context, podID string, machine string) erro
 	err = r.AtomicUpdate(contKey, &api.BoundPods{}, func(in runtime.Object) (runtime.Object, error) {
 		boundPodList := in.(*api.BoundPods)
 		boundPodList.Items = append(boundPodList.Items, *boundPod)
-		if !constraint.Allowed(boundPodList.Items) {
+		if errors := constraint.Allowed(boundPodList.Items); len(errors) > 0 {
-			return nil, fmt.Errorf("the assignment would cause a constraint violation")
+			return nil, fmt.Errorf("the assignment would cause the following constraints violation: %v", errors)
 		}
 		return boundPodList, nil
 	})