From ddba004ad02544bef62722a3bd57387b5d30c229 Mon Sep 17 00:00:00 2001 From: Daniel Smith Date: Thu, 21 Aug 2014 14:30:49 -0700 Subject: [PATCH] Add constraint package to let us reject invalid assignments. --- pkg/constraint/constraint.go | 27 +++++++++ pkg/constraint/constraint_test.go | 98 +++++++++++++++++++++++++++++++ pkg/constraint/doc.go | 23 ++++++++ pkg/constraint/ports.go | 41 +++++++++++++ pkg/master/pod_cache.go | 1 + pkg/registry/etcd/etcd.go | 4 ++ pkg/registry/pod/storage_test.go | 4 +- pkg/tools/etcd_tools.go | 1 - 8 files changed, 196 insertions(+), 3 deletions(-) create mode 100644 pkg/constraint/constraint.go create mode 100644 pkg/constraint/constraint_test.go create mode 100644 pkg/constraint/doc.go create mode 100644 pkg/constraint/ports.go diff --git a/pkg/constraint/constraint.go b/pkg/constraint/constraint.go new file mode 100644 index 00000000000..0e08162792f --- /dev/null +++ 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 new file mode 100644 index 00000000000..4d168551fc7 --- /dev/null +++ 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 new file mode 100644 index 00000000000..28d7218a43d --- /dev/null +++ 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 new file mode 100644 index 00000000000..92622469a2c --- /dev/null +++ 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 index 085cbcf8e64..674c6c8b9c6 100644 --- 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 index 9747457e963..371454a9ba4 100644 --- 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 index 7134cec716f..70821c526b5 100644 --- 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 index f44f4de1f52..358d69ebefb 100644 --- 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 {