From 5e8d70f76b5a2b7f87b879adb85ce6bbc82fb27b Mon Sep 17 00:00:00 2001 From: Eric Tune Date: Thu, 19 Feb 2015 13:27:41 -0800 Subject: [PATCH] Remove HostPort conflict checking. We decided to get rid of boundPods. Removing this check is a prerequisite for that. This check had some value before we had IP-per-Pod. However, AIUI, use of HostPort is strongly discouraged in Kubernetes. It still exists as part of a Pod spec because of ContainerVM, where it is used. But, this change does not affect ContainerVM, where there is no master. If someone did create pods with HostPort using kubernetes, the following would happen: - The scheduler would try not to put two conflicting pods on the same machine (pkg/scheduler/predicates.go : PodFitsPorts() ) - I'm not sure if it is currently possible for a race to occur where the PodFitsPorts check were bypassed. Maybe it could happen. - If the kubelet was sent conflicting pods, it would detect them in ( pkg/kubelet/kubelet.go : filterHostPortConflicts() ). It would arbitrarily pick one pod to run and another to ignore. - If all of the above happened and the user filed and issue on github, we might figure out that the user used HostPort and tell the user to stop. TODO: - e2e test - Several of the demos in examples/ use hostPort. Change them to not specify hostPort and have a service instead. --- pkg/constraint/constraint.go | 33 ----------- pkg/constraint/constraint_test.go | 99 ------------------------------- pkg/constraint/doc.go | 23 ------- pkg/constraint/ports.go | 43 -------------- pkg/registry/pod/etcd/etcd.go | 5 -- 5 files changed, 203 deletions(-) delete mode 100644 pkg/constraint/constraint.go delete mode 100644 pkg/constraint/constraint_test.go delete mode 100644 pkg/constraint/doc.go delete mode 100644 pkg/constraint/ports.go diff --git a/pkg/constraint/constraint.go b/pkg/constraint/constraint.go deleted file mode 100644 index 430c1a85cb..0000000000 --- a/pkg/constraint/constraint.go +++ /dev/null @@ -1,33 +0,0 @@ -/* -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 ( - "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) []error { - errors := []error{} - for _, port := range hostPortsConflict(pods) { - errors = append(errors, fmt.Errorf("host port %v is already in use", port)) - } - return errors -} diff --git a/pkg/constraint/constraint_test.go b/pkg/constraint/constraint_test.go deleted file mode 100644 index 7f76ddf196..0000000000 --- a/pkg/constraint/constraint_test.go +++ /dev/null @@ -1,99 +0,0 @@ -/* -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 ( - "fmt" - "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.ContainerPort{HostPort: p}) - } - return c -} - -func podWithContainers(containers ...api.Container) api.BoundPod { - m := api.BoundPod{} - for _, c := range containers { - m.Spec.Containers = append(m.Spec.Containers, c) - } - return m -} - -func TestAllowed(t *testing.T) { - table := []struct { - err string - pods []api.BoundPod - }{ - { - err: "[]", - pods: []api.BoundPod{ - podWithContainers( - containerWithHostPorts(1, 2, 3), - containerWithHostPorts(4, 5, 6), - ), - podWithContainers( - containerWithHostPorts(7, 8, 9), - containerWithHostPorts(10, 11, 12), - ), - }, - }, - { - err: "[]", - pods: []api.BoundPod{ - podWithContainers( - containerWithHostPorts(0, 0), - containerWithHostPorts(0, 0), - ), - podWithContainers( - containerWithHostPorts(0, 0), - containerWithHostPorts(0, 0), - ), - }, - }, - { - err: "[host port 3 is already in use]", - pods: []api.BoundPod{ - podWithContainers( - containerWithHostPorts(3, 3), - ), - }, - }, - { - err: "[host port 6 is already in use]", - pods: []api.BoundPod{ - podWithContainers( - containerWithHostPorts(6), - ), - podWithContainers( - containerWithHostPorts(6), - ), - }, - }, - } - - for _, item := range table { - 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/doc.go b/pkg/constraint/doc.go deleted file mode 100644 index 28d7218a43..0000000000 --- a/pkg/constraint/doc.go +++ /dev/null @@ -1,23 +0,0 @@ -/* -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 deleted file mode 100644 index b7398a6ea7..0000000000 --- a/pkg/constraint/ports.go +++ /dev/null @@ -1,43 +0,0 @@ -/* -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" -) - -// 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 { - if port.HostPort == 0 { - continue - } - if _, exists := hostPorts[port.HostPort]; exists { - conflictingPorts = append(conflictingPorts, port.HostPort) - } - hostPorts[port.HostPort] = struct{}{} - } - } - } - return conflictingPorts -} diff --git a/pkg/registry/pod/etcd/etcd.go b/pkg/registry/pod/etcd/etcd.go index c60d9b1c42..060b8df704 100644 --- a/pkg/registry/pod/etcd/etcd.go +++ b/pkg/registry/pod/etcd/etcd.go @@ -23,7 +23,6 @@ import ( "github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors" etcderr "github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors/etcd" "github.com/GoogleCloudPlatform/kubernetes/pkg/api/rest" - "github.com/GoogleCloudPlatform/kubernetes/pkg/constraint" "github.com/GoogleCloudPlatform/kubernetes/pkg/fields" "github.com/GoogleCloudPlatform/kubernetes/pkg/labels" "github.com/GoogleCloudPlatform/kubernetes/pkg/registry/generic" @@ -193,14 +192,10 @@ func (r *BindingREST) assignPod(ctx api.Context, podID string, machine string) e if err != nil { return err } - // Doing the constraint check this way provides atomicity guarantees. contKey := makeBoundPodsKey(machine) err = r.store.Helper.AtomicUpdate(contKey, &api.BoundPods{}, true, func(in runtime.Object) (runtime.Object, error) { boundPodList := in.(*api.BoundPods) boundPodList.Items = append(boundPodList.Items, *boundPod) - 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 }) if err != nil {