Add constraint package to let us reject invalid assignments.

duda · Aug 25, 2014 · ddba004 · ddba004
1 parent 0a1dfa3
commit ddba004
Show file tree

Hide file tree

Showing 8 changed files with 196 additions and 3 deletions.
diff --git a/pkg/constraint/constraint.go b/pkg/constraint/constraint.go
@@ -0,0 +1,27 @@
+/*
+Copyright 2014 Google Inc. All rights reserved.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package constraint
+
+import (
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
+)
+
+// Allowed returns true if manifests is a collection of manifests
+// which can run without conflict on a single minion.
+func Allowed(manifests []api.ContainerManifest) bool {
+	return !PortsConflict(manifests)
+}
diff --git a/pkg/constraint/constraint_test.go b/pkg/constraint/constraint_test.go
@@ -0,0 +1,98 @@
+/*
+Copyright 2014 Google Inc. All rights reserved.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package constraint
+
+import (
+	"testing"
+
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
+)
+
+func containerWithHostPorts(ports ...int) api.Container {
+	c := api.Container{}
+	for _, p := range ports {
+		c.Ports = append(c.Ports, api.Port{HostPort: p})
+	}
+	return c
+}
+
+func manifestWithContainers(containers ...api.Container) api.ContainerManifest {
+	m := api.ContainerManifest{}
+	for _, c := range containers {
+		m.Containers = append(m.Containers, c)
+	}
+	return m
+}
+
+func TestAllowed(t *testing.T) {
+	table := []struct {
+		allowed   bool
+		manifests []api.ContainerManifest
+	}{
+		{
+			allowed: true,
+			manifests: []api.ContainerManifest{
+				manifestWithContainers(
+					containerWithHostPorts(1, 2, 3),
+					containerWithHostPorts(4, 5, 6),
+				),
+				manifestWithContainers(
+					containerWithHostPorts(7, 8, 9),
+					containerWithHostPorts(10, 11, 12),
+				),
+			},
+		},
+		{
+			allowed: true,
+			manifests: []api.ContainerManifest{
+				manifestWithContainers(
+					containerWithHostPorts(0, 0),
+					containerWithHostPorts(0, 0),
+				),
+				manifestWithContainers(
+					containerWithHostPorts(0, 0),
+					containerWithHostPorts(0, 0),
+				),
+			},
+		},
+		{
+			allowed: false,
+			manifests: []api.ContainerManifest{
+				manifestWithContainers(
+					containerWithHostPorts(3, 3),
+				),
+			},
+		},
+		{
+			allowed: false,
+			manifests: []api.ContainerManifest{
+				manifestWithContainers(
+					containerWithHostPorts(6),
+				),
+				manifestWithContainers(
+					containerWithHostPorts(6),
+				),
+			},
+		},
+	}
+
+	for _, item := range table {
+		if e, a := item.allowed, Allowed(item.manifests); e != a {
+			t.Errorf("Expected %v, got %v: \n%v\v", e, a, item.manifests)
+		}
+	}
+}
diff --git a/pkg/constraint/doc.go b/pkg/constraint/doc.go
@@ -0,0 +1,23 @@
+/*
+Copyright 2014 Google Inc. All rights reserved.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+// Package constraint has functions for ensuring that collections of
+// containers are allowed to run together on a single host.
+//
+// TODO: Add resource math. Phrase this code in a way that makes it easy
+// to call from schedulers as well as from the feasiblity check that
+// apiserver performs.
+package constraint
diff --git a/pkg/constraint/ports.go b/pkg/constraint/ports.go
@@ -0,0 +1,41 @@
+/*
+Copyright 2014 Google Inc. All rights reserved.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package constraint
+
+import (
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
+)
+
+// PortsConflict returns true iff two containers attempt to expose
+// the same host port.
+func PortsConflict(manifests []api.ContainerManifest) bool {
+	hostPorts := map[int]struct{}{}
+	for _, manifest := range manifests {
+		for _, container := range manifest.Containers {
+			for _, port := range container.Ports {
+				if port.HostPort == 0 {
+					continue
+				}
+				if _, exists := hostPorts[port.HostPort]; exists {
+					return true
+				}
+				hostPorts[port.HostPort] = struct{}{}
+			}
+		}
+	}
+	return false
+}
diff --git a/pkg/master/pod_cache.go b/pkg/master/pod_cache.go
@@ -48,6 +48,7 @@ func NewPodCache(info client.PodInfoGetter, pods pod.Registry) *PodCache {
 
 // GetPodInfo Implements the PodInfoGetter.GetPodInfo.
 // The returned value should be treated as read-only.
+// TODO: Remove the host from this call, it's totally unnecessary.
 func (p *PodCache) GetPodInfo(host, podID string) (api.PodInfo, error) {
 	p.podLock.Lock()
 	defer p.podLock.Unlock()

diff --git a/pkg/registry/etcd/etcd.go b/pkg/registry/etcd/etcd.go
@@ -21,6 +21,7 @@ import (
 
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/apiserver"
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/constraint"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/labels"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/registry/minion"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/tools"
@@ -149,6 +150,9 @@ func (r *Registry) assignPod(podID string, machine string) error {
 	err = r.AtomicUpdate(contKey, &api.ContainerManifestList{}, func(in interface{}) (interface{}, error) {
 		manifests := *in.(*api.ContainerManifestList)
 		manifests.Items = append(manifests.Items, manifest)
+		if !constraint.Allowed(manifests.Items) {
+			return nil, fmt.Errorf("The assignment would cause a constraint violation")
+		}
 		return manifests, nil
 	})
 	if err != nil {

diff --git a/pkg/registry/pod/storage_test.go b/pkg/registry/pod/storage_test.go
@@ -418,7 +418,7 @@ func TestFillPodInfo(t *testing.T) {
 	storage := RegistryStorage{
 		podCache: &fakeGetter,
 	}
-	pod := api.Pod{}
+	pod := api.Pod{DesiredState: api.PodState{Host: "foo"}}
 	storage.fillPodInfo(&pod)
 	if !reflect.DeepEqual(fakeGetter.info, pod.CurrentState.Info) {
 		t.Errorf("Expected: %#v, Got %#v", fakeGetter.info, pod.CurrentState.Info)
@@ -441,7 +441,7 @@ func TestFillPodInfoNoData(t *testing.T) {
 	storage := RegistryStorage{
 		podCache: &fakeGetter,
 	}
-	pod := api.Pod{}
+	pod := api.Pod{DesiredState: api.PodState{Host: "foo"}}
 	storage.fillPodInfo(&pod)
 	if !reflect.DeepEqual(fakeGetter.info, pod.CurrentState.Info) {
 		t.Errorf("Expected %#v, Got %#v", fakeGetter.info, pod.CurrentState.Info)

diff --git a/pkg/tools/etcd_tools.go b/pkg/tools/etcd_tools.go
@@ -477,7 +477,6 @@ func (w *etcdWatcher) sendResult(res *etcd.Response) {
 	var action watch.EventType
 	var data []byte
 	var index uint64
-	glog.Infof("watching: got %v", res.Action)
 	switch res.Action {
 	case "create":
 		if res.Node == nil {