From ad4c2ee630a4ac780cdba86a16027b541742adb7 Mon Sep 17 00:00:00 2001 From: Jerzy Szczepkowski Date: Wed, 4 Feb 2015 23:41:27 +0100 Subject: [PATCH 1/3] Improve "constraint violation" error message. --- pkg/constraint/constraint.go | 9 +++++++-- pkg/constraint/constraint_test.go | 13 +++++++------ pkg/registry/etcd/etcd.go | 4 ++-- 3 files changed, 16 insertions(+), 10 deletions(-) diff --git a/pkg/constraint/constraint.go b/pkg/constraint/constraint.go index 8acc8c073d8..da0c3749e14 100644 --- a/pkg/constraint/constraint.go +++ b/pkg/constraint/constraint.go @@ -17,11 +17,16 @@ 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 { - return !PortsConflict(pods) +func Allowed(pods []api.BoundPod) error { + if (PortsConflict(pods)) { + return fmt.Errorf("conflicting ports"); + } + return nil; } diff --git a/pkg/constraint/constraint_test.go b/pkg/constraint/constraint_test.go index 1677d935df5..530428f91cc 100644 --- a/pkg/constraint/constraint_test.go +++ b/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 error pods []api.BoundPod }{ { - allowed: true, + err: nil, pods: []api.BoundPod{ podWithContainers( containerWithHostPorts(1, 2, 3), @@ -57,7 +58,7 @@ func TestAllowed(t *testing.T) { }, }, { - allowed: true, + err: nil, pods: []api.BoundPod{ podWithContainers( containerWithHostPorts(0, 0), @@ -70,7 +71,7 @@ func TestAllowed(t *testing.T) { }, }, { - allowed: false, + err: fmt.Errorf("conflicting ports"), pods: []api.BoundPod{ podWithContainers( containerWithHostPorts(3, 3), @@ -78,7 +79,7 @@ func TestAllowed(t *testing.T) { }, }, { - allowed: false, + err: fmt.Errorf("conflicting ports"), 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 != a && e.Error() != a.Error() { t.Errorf("Expected %v, got %v: \n%v\v", e, a, item.pods) } } diff --git a/pkg/registry/etcd/etcd.go b/pkg/registry/etcd/etcd.go index d2bd576cb52..49c32eff69e 100644 --- a/pkg/registry/etcd/etcd.go +++ b/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) { - return nil, fmt.Errorf("the assignment would cause a constraint violation") + if e := constraint.Allowed(boundPodList.Items); e != nil { + return nil, fmt.Errorf("the assignment would cause the following constraint violation: %v", e) } return boundPodList, nil }) From 0010e0214854daf301ac2750832e4b14233fee5c Mon Sep 17 00:00:00 2001 From: Jerzy Szczepkowski Date: Thu, 5 Feb 2015 00:52:34 +0100 Subject: [PATCH 2/3] Fixed formatting. --- pkg/constraint/constraint.go | 6 +++--- pkg/constraint/constraint_test.go | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/pkg/constraint/constraint.go b/pkg/constraint/constraint.go index da0c3749e14..078ca6d59d1 100644 --- a/pkg/constraint/constraint.go +++ b/pkg/constraint/constraint.go @@ -25,8 +25,8 @@ import ( // 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) error { - if (PortsConflict(pods)) { - return fmt.Errorf("conflicting ports"); + if PortsConflict(pods) { + return fmt.Errorf("conflicting ports") } - return nil; + return nil } diff --git a/pkg/constraint/constraint_test.go b/pkg/constraint/constraint_test.go index 530428f91cc..bb6f84fc89d 100644 --- a/pkg/constraint/constraint_test.go +++ b/pkg/constraint/constraint_test.go @@ -41,8 +41,8 @@ func podWithContainers(containers ...api.Container) api.BoundPod { func TestAllowed(t *testing.T) { table := []struct { - err error - pods []api.BoundPod + err error + pods []api.BoundPod }{ { err: nil, From 18c5fa1ae76b557a6dcb9ddd9c0017f0bc0cd221 Mon Sep 17 00:00:00 2001 From: Jerzy Szczepkowski Date: Thu, 5 Feb 2015 20:32:51 +0100 Subject: [PATCH 3/3] Applied comments. --- pkg/constraint/constraint.go | 9 +++++---- pkg/constraint/constraint_test.go | 12 ++++++------ pkg/constraint/ports.go | 12 +++++++----- pkg/registry/etcd/etcd.go | 4 ++-- 4 files changed, 20 insertions(+), 17 deletions(-) diff --git a/pkg/constraint/constraint.go b/pkg/constraint/constraint.go index 078ca6d59d1..430c1a85cb2 100644 --- a/pkg/constraint/constraint.go +++ b/pkg/constraint/constraint.go @@ -24,9 +24,10 @@ import ( // 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) error { - if PortsConflict(pods) { - return fmt.Errorf("conflicting ports") +func Allowed(pods []api.BoundPod) []error { + errors := []error{} + for _, port := range hostPortsConflict(pods) { + errors = append(errors, fmt.Errorf("host port %v is already in use", port)) } - return nil + return errors } diff --git a/pkg/constraint/constraint_test.go b/pkg/constraint/constraint_test.go index bb6f84fc89d..eae45221270 100644 --- a/pkg/constraint/constraint_test.go +++ b/pkg/constraint/constraint_test.go @@ -41,11 +41,11 @@ func podWithContainers(containers ...api.Container) api.BoundPod { func TestAllowed(t *testing.T) { table := []struct { - err error + err string pods []api.BoundPod }{ { - err: nil, + err: "[]", pods: []api.BoundPod{ podWithContainers( containerWithHostPorts(1, 2, 3), @@ -58,7 +58,7 @@ func TestAllowed(t *testing.T) { }, }, { - err: nil, + err: "[]", pods: []api.BoundPod{ podWithContainers( containerWithHostPorts(0, 0), @@ -71,7 +71,7 @@ func TestAllowed(t *testing.T) { }, }, { - err: fmt.Errorf("conflicting ports"), + err: "[host port 3 is already in use]", pods: []api.BoundPod{ podWithContainers( containerWithHostPorts(3, 3), @@ -79,7 +79,7 @@ func TestAllowed(t *testing.T) { }, }, { - err: fmt.Errorf("conflicting ports"), + err: "[host port 6 is already in use]", pods: []api.BoundPod{ podWithContainers( containerWithHostPorts(6), @@ -92,7 +92,7 @@ func TestAllowed(t *testing.T) { } for _, item := range table { - if e, a := item.err, Allowed(item.pods); e != a && e.Error() != a.Error() { + 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) } } diff --git a/pkg/constraint/ports.go b/pkg/constraint/ports.go index f5c95f5fb2d..b7398a6ea78 100644 --- a/pkg/constraint/ports.go +++ b/pkg/constraint/ports.go @@ -20,10 +20,12 @@ import ( "github.com/GoogleCloudPlatform/kubernetes/pkg/api" ) -// PortsConflict returns true iff two containers attempt to expose -// the same host port. -func PortsConflict(pods []api.BoundPod) bool { +// hostPortsConflict returns an array of host ports that at least two +// containers attempt to expose. The array is empty if no such port +// 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 } diff --git a/pkg/registry/etcd/etcd.go b/pkg/registry/etcd/etcd.go index 49c32eff69e..c5114a8320b 100644 --- a/pkg/registry/etcd/etcd.go +++ b/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 e := constraint.Allowed(boundPodList.Items); e != nil { - return nil, fmt.Errorf("the assignment would cause the following constraint violation: %v", e) + if errors := constraint.Allowed(boundPodList.Items); len(errors) > 0 { + return nil, fmt.Errorf("the assignment would cause the following constraints violation: %v", errors) } return boundPodList, nil })