From 8380148d48e79b87dda96751da60f74c17fbd818 Mon Sep 17 00:00:00 2001 From: Random-Liu Date: Fri, 24 Feb 2017 12:55:54 -0800 Subject: [PATCH] Remove extra operations when generating pod sandbox configuration. --- pkg/kubelet/container/BUILD | 1 + pkg/kubelet/container/helpers.go | 35 ++++++++++++ pkg/kubelet/container/helpers_test.go | 53 +++++++++++++++++++ pkg/kubelet/dockertools/BUILD | 1 + .../dockertools/docker_manager_test.go | 6 +++ pkg/kubelet/kubelet_pods.go | 44 ++++----------- pkg/kubelet/kubelet_test.go | 51 ------------------ pkg/kubelet/kuberuntime/BUILD | 1 + .../kuberuntime/fake_kuberuntime_manager.go | 5 ++ .../kuberuntime/kuberuntime_sandbox.go | 15 ++---- pkg/kubelet/rkt/BUILD | 1 + pkg/kubelet/rkt/fake_rkt_interface_test.go | 6 +++ 12 files changed, 124 insertions(+), 95 deletions(-) diff --git a/pkg/kubelet/container/BUILD b/pkg/kubelet/container/BUILD index ffac0682657..94375356ce2 100644 --- a/pkg/kubelet/container/BUILD +++ b/pkg/kubelet/container/BUILD @@ -29,6 +29,7 @@ go_library( "//pkg/api:go_default_library", "//pkg/api/v1:go_default_library", "//pkg/kubelet/api/v1alpha1/runtime:go_default_library", + "//pkg/kubelet/cm:go_default_library", "//pkg/kubelet/events:go_default_library", "//pkg/kubelet/util/format:go_default_library", "//pkg/kubelet/util/ioutils:go_default_library", diff --git a/pkg/kubelet/container/helpers.go b/pkg/kubelet/container/helpers.go index f64aeec72e2..12a742a961a 100644 --- a/pkg/kubelet/container/helpers.go +++ b/pkg/kubelet/container/helpers.go @@ -33,6 +33,7 @@ import ( "k8s.io/client-go/tools/record" "k8s.io/kubernetes/pkg/api/v1" runtimeapi "k8s.io/kubernetes/pkg/kubelet/api/v1alpha1/runtime" + "k8s.io/kubernetes/pkg/kubelet/cm" "k8s.io/kubernetes/pkg/kubelet/events" "k8s.io/kubernetes/pkg/kubelet/util/format" "k8s.io/kubernetes/pkg/kubelet/util/ioutils" @@ -50,6 +51,9 @@ type HandlerRunner interface { type RuntimeHelper interface { GenerateRunContainerOptions(pod *v1.Pod, container *v1.Container, podIP string) (*RunContainerOptions, error) GetClusterDNS(pod *v1.Pod) (dnsServers []string, dnsSearches []string, err error) + // GetPodCgroupParent returns the the CgroupName identifer, and its literal cgroupfs form on the host + // of a pod. + GetPodCgroupParent(pod *v1.Pod) (cm.CgroupName, string) GetPodDir(podUID types.UID) string GeneratePodHostNameAndDomain(pod *v1.Pod) (hostname string, hostDomain string, err error) // GetExtraSupplementalGroupsForPod returns a list of the extra @@ -306,3 +310,34 @@ func MakeCapabilities(capAdd []v1.Capability, capDrop []v1.Capability) ([]string } return addCaps, dropCaps } + +// MakePortMappings creates internal port mapping from api port mapping. +func MakePortMappings(container *v1.Container) (ports []PortMapping) { + names := make(map[string]struct{}) + for _, p := range container.Ports { + pm := PortMapping{ + HostPort: int(p.HostPort), + ContainerPort: int(p.ContainerPort), + Protocol: p.Protocol, + HostIP: p.HostIP, + } + + // We need to create some default port name if it's not specified, since + // this is necessary for rkt. + // http://issue.k8s.io/7710 + if p.Name == "" { + pm.Name = fmt.Sprintf("%s-%s:%d", container.Name, p.Protocol, p.ContainerPort) + } else { + pm.Name = fmt.Sprintf("%s-%s", container.Name, p.Name) + } + + // Protect against exposing the same protocol-port more than once in a container. + if _, ok := names[pm.Name]; ok { + glog.Warningf("Port name conflicted, %q is defined more than once", pm.Name) + continue + } + ports = append(ports, pm) + names[pm.Name] = struct{}{} + } + return +} diff --git a/pkg/kubelet/container/helpers_test.go b/pkg/kubelet/container/helpers_test.go index 13d65469060..c0d5436acd9 100644 --- a/pkg/kubelet/container/helpers_test.go +++ b/pkg/kubelet/container/helpers_test.go @@ -20,6 +20,8 @@ import ( "reflect" "testing" + "github.com/stretchr/testify/assert" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/kubernetes/pkg/api/v1" ) @@ -253,3 +255,54 @@ func TestHasPrivilegedContainer(t *testing.T) { } } } + +func TestMakePortMappings(t *testing.T) { + port := func(name string, protocol v1.Protocol, containerPort, hostPort int32, ip string) v1.ContainerPort { + return v1.ContainerPort{ + Name: name, + Protocol: protocol, + ContainerPort: containerPort, + HostPort: hostPort, + HostIP: ip, + } + } + portMapping := func(name string, protocol v1.Protocol, containerPort, hostPort int, ip string) PortMapping { + return PortMapping{ + Name: name, + Protocol: protocol, + ContainerPort: containerPort, + HostPort: hostPort, + HostIP: ip, + } + } + + tests := []struct { + container *v1.Container + expectedPortMappings []PortMapping + }{ + { + &v1.Container{ + Name: "fooContainer", + Ports: []v1.ContainerPort{ + port("", v1.ProtocolTCP, 80, 8080, "127.0.0.1"), + port("", v1.ProtocolTCP, 443, 4343, "192.168.0.1"), + port("foo", v1.ProtocolUDP, 555, 5555, ""), + // Duplicated, should be ignored. + port("foo", v1.ProtocolUDP, 888, 8888, ""), + // Duplicated, should be ignored. + port("", v1.ProtocolTCP, 80, 8888, ""), + }, + }, + []PortMapping{ + portMapping("fooContainer-TCP:80", v1.ProtocolTCP, 80, 8080, "127.0.0.1"), + portMapping("fooContainer-TCP:443", v1.ProtocolTCP, 443, 4343, "192.168.0.1"), + portMapping("fooContainer-foo", v1.ProtocolUDP, 555, 5555, ""), + }, + }, + } + + for i, tt := range tests { + actual := MakePortMappings(tt.container) + assert.Equal(t, tt.expectedPortMappings, actual, "[%d]", i) + } +} diff --git a/pkg/kubelet/dockertools/BUILD b/pkg/kubelet/dockertools/BUILD index 397edb53051..4bf14017748 100644 --- a/pkg/kubelet/dockertools/BUILD +++ b/pkg/kubelet/dockertools/BUILD @@ -107,6 +107,7 @@ go_test( "//pkg/api/v1:go_default_library", "//pkg/apis/componentconfig:go_default_library", "//pkg/credentialprovider:go_default_library", + "//pkg/kubelet/cm:go_default_library", "//pkg/kubelet/container:go_default_library", "//pkg/kubelet/container/testing:go_default_library", "//pkg/kubelet/events:go_default_library", diff --git a/pkg/kubelet/dockertools/docker_manager_test.go b/pkg/kubelet/dockertools/docker_manager_test.go index da0ca6bf15c..0d40a5f66bc 100644 --- a/pkg/kubelet/dockertools/docker_manager_test.go +++ b/pkg/kubelet/dockertools/docker_manager_test.go @@ -50,6 +50,7 @@ import ( "k8s.io/kubernetes/pkg/api/testapi" "k8s.io/kubernetes/pkg/api/v1" "k8s.io/kubernetes/pkg/apis/componentconfig" + "k8s.io/kubernetes/pkg/kubelet/cm" kubecontainer "k8s.io/kubernetes/pkg/kubelet/container" containertest "k8s.io/kubernetes/pkg/kubelet/container/testing" "k8s.io/kubernetes/pkg/kubelet/images" @@ -87,6 +88,7 @@ func (f *fakeHTTP) Get(url string) (*http.Response, error) { // fakeRuntimeHelper implementes kubecontainer.RuntimeHelper inter // faces for testing purposes. +// TODO(random-liu): Move this into pkg/kubelet/container/testing type fakeRuntimeHelper struct{} var _ kubecontainer.RuntimeHelper = &fakeRuntimeHelper{} @@ -106,6 +108,10 @@ func (f *fakeRuntimeHelper) GenerateRunContainerOptions(pod *v1.Pod, container * return &opts, nil } +func (f *fakeRuntimeHelper) GetPodCgroupParent(pod *v1.Pod) (cm.CgroupName, string) { + return "", "" +} + func (f *fakeRuntimeHelper) GetClusterDNS(pod *v1.Pod) ([]string, []string, error) { return nil, nil, fmt.Errorf("not implemented") } diff --git a/pkg/kubelet/kubelet_pods.go b/pkg/kubelet/kubelet_pods.go index cf3a0a83914..0f8953c3624 100644 --- a/pkg/kubelet/kubelet_pods.go +++ b/pkg/kubelet/kubelet_pods.go @@ -203,36 +203,6 @@ func ensureHostsFile(fileName, hostIP, hostName, hostDomainName string) error { return ioutil.WriteFile(fileName, buffer.Bytes(), 0644) } -func makePortMappings(container *v1.Container) (ports []kubecontainer.PortMapping) { - names := make(map[string]struct{}) - for _, p := range container.Ports { - pm := kubecontainer.PortMapping{ - HostPort: int(p.HostPort), - ContainerPort: int(p.ContainerPort), - Protocol: p.Protocol, - HostIP: p.HostIP, - } - - // We need to create some default port name if it's not specified, since - // this is necessary for rkt. - // http://issue.k8s.io/7710 - if p.Name == "" { - pm.Name = fmt.Sprintf("%s-%s:%d", container.Name, p.Protocol, p.ContainerPort) - } else { - pm.Name = fmt.Sprintf("%s-%s", container.Name, p.Name) - } - - // Protect against exposing the same protocol-port more than once in a container. - if _, ok := names[pm.Name]; ok { - glog.Warningf("Port name conflicted, %q is defined more than once", pm.Name) - continue - } - ports = append(ports, pm) - names[pm.Name] = struct{}{} - } - return -} - // truncatePodHostnameIfNeeded truncates the pod hostname if it's longer than 63 chars. func truncatePodHostnameIfNeeded(podName, hostname string) (string, error) { // Cap hostname at 63 chars (specification is 64bytes which is 63 chars and the null terminating char). @@ -293,13 +263,18 @@ func (kl *Kubelet) GeneratePodHostNameAndDomain(pod *v1.Pod) (string, string, er return hostname, hostDomain, nil } +// GetPodCgroupParent gets pod cgroup parent from container manager. +func (kl *Kubelet) GetPodCgroupParent(pod *v1.Pod) (cm.CgroupName, string) { + pcm := kl.containerManager.NewPodContainerManager() + return pcm.GetPodContainerName(pod) +} + // GenerateRunContainerOptions generates the RunContainerOptions, which can be used by // the container runtime to set parameters for launching a container. func (kl *Kubelet) GenerateRunContainerOptions(pod *v1.Pod, container *v1.Container, podIP string) (*kubecontainer.RunContainerOptions, error) { var err error - pcm := kl.containerManager.NewPodContainerManager() - _, podContainerName := pcm.GetPodContainerName(pod) - opts := &kubecontainer.RunContainerOptions{CgroupParent: podContainerName} + _, cgroupParent := kl.GetPodCgroupParent(pod) + opts := &kubecontainer.RunContainerOptions{CgroupParent: cgroupParent} hostname, hostDomainName, err := kl.GeneratePodHostNameAndDomain(pod) if err != nil { return nil, err @@ -308,7 +283,8 @@ func (kl *Kubelet) GenerateRunContainerOptions(pod *v1.Pod, container *v1.Contai podName := volumehelper.GetUniquePodName(pod) volumes := kl.volumeManager.GetMountedVolumesForPod(podName) - opts.PortMappings = makePortMappings(container) + opts.PortMappings = kubecontainer.MakePortMappings(container) + // TODO(random-liu): Move following convert functions into pkg/kubelet/container opts.Devices = makeDevices(container) opts.Mounts, err = makeMounts(pod, kl.getPodDir(pod.UID), container, hostname, hostDomainName, podIP, volumes) diff --git a/pkg/kubelet/kubelet_test.go b/pkg/kubelet/kubelet_test.go index b342c61235b..2b95d66bb37 100644 --- a/pkg/kubelet/kubelet_test.go +++ b/pkg/kubelet/kubelet_test.go @@ -1154,57 +1154,6 @@ func TestFilterOutTerminatedPods(t *testing.T) { assert.Equal(t, expected, actual) } -func TestMakePortMappings(t *testing.T) { - port := func(name string, protocol v1.Protocol, containerPort, hostPort int32, ip string) v1.ContainerPort { - return v1.ContainerPort{ - Name: name, - Protocol: protocol, - ContainerPort: containerPort, - HostPort: hostPort, - HostIP: ip, - } - } - portMapping := func(name string, protocol v1.Protocol, containerPort, hostPort int, ip string) kubecontainer.PortMapping { - return kubecontainer.PortMapping{ - Name: name, - Protocol: protocol, - ContainerPort: containerPort, - HostPort: hostPort, - HostIP: ip, - } - } - - tests := []struct { - container *v1.Container - expectedPortMappings []kubecontainer.PortMapping - }{ - { - &v1.Container{ - Name: "fooContainer", - Ports: []v1.ContainerPort{ - port("", v1.ProtocolTCP, 80, 8080, "127.0.0.1"), - port("", v1.ProtocolTCP, 443, 4343, "192.168.0.1"), - port("foo", v1.ProtocolUDP, 555, 5555, ""), - // Duplicated, should be ignored. - port("foo", v1.ProtocolUDP, 888, 8888, ""), - // Duplicated, should be ignored. - port("", v1.ProtocolTCP, 80, 8888, ""), - }, - }, - []kubecontainer.PortMapping{ - portMapping("fooContainer-TCP:80", v1.ProtocolTCP, 80, 8080, "127.0.0.1"), - portMapping("fooContainer-TCP:443", v1.ProtocolTCP, 443, 4343, "192.168.0.1"), - portMapping("fooContainer-foo", v1.ProtocolUDP, 555, 5555, ""), - }, - }, - } - - for i, tt := range tests { - actual := makePortMappings(tt.container) - assert.Equal(t, tt.expectedPortMappings, actual, "[%d]", i) - } -} - func TestSyncPodsSetStatusToFailedForPodsThatRunTooLong(t *testing.T) { testKubelet := newTestKubelet(t, false /* controllerAttachDetachEnabled */) defer testKubelet.Cleanup() diff --git a/pkg/kubelet/kuberuntime/BUILD b/pkg/kubelet/kuberuntime/BUILD index 644cc23c96b..a257e636e41 100644 --- a/pkg/kubelet/kuberuntime/BUILD +++ b/pkg/kubelet/kuberuntime/BUILD @@ -32,6 +32,7 @@ go_library( "//pkg/credentialprovider:go_default_library", "//pkg/kubelet/api:go_default_library", "//pkg/kubelet/api/v1alpha1/runtime:go_default_library", + "//pkg/kubelet/cm:go_default_library", "//pkg/kubelet/container:go_default_library", "//pkg/kubelet/dockertools:go_default_library", "//pkg/kubelet/events:go_default_library", diff --git a/pkg/kubelet/kuberuntime/fake_kuberuntime_manager.go b/pkg/kubelet/kuberuntime/fake_kuberuntime_manager.go index 78f7fd8796f..1b4777d8044 100644 --- a/pkg/kubelet/kuberuntime/fake_kuberuntime_manager.go +++ b/pkg/kubelet/kuberuntime/fake_kuberuntime_manager.go @@ -29,6 +29,7 @@ import ( "k8s.io/kubernetes/pkg/api/v1" "k8s.io/kubernetes/pkg/credentialprovider" internalapi "k8s.io/kubernetes/pkg/kubelet/api" + "k8s.io/kubernetes/pkg/kubelet/cm" kubecontainer "k8s.io/kubernetes/pkg/kubelet/container" "k8s.io/kubernetes/pkg/kubelet/images" "k8s.io/kubernetes/pkg/kubelet/lifecycle" @@ -49,6 +50,10 @@ func (f *fakeHTTP) Get(url string) (*http.Response, error) { // fakeRuntimeHelper implements kubecontainer.RuntimeHelper interfaces for testing purposes. type fakeRuntimeHelper struct{} +func (f *fakeRuntimeHelper) GetPodCgroupParent(pod *v1.Pod) (cm.CgroupName, string) { + return "", "" +} + func (f *fakeRuntimeHelper) GenerateRunContainerOptions(pod *v1.Pod, container *v1.Container, podIP string) (*kubecontainer.RunContainerOptions, error) { var opts kubecontainer.RunContainerOptions if len(container.TerminationMessagePath) != 0 { diff --git a/pkg/kubelet/kuberuntime/kuberuntime_sandbox.go b/pkg/kubelet/kuberuntime/kuberuntime_sandbox.go index 52f54a8d0b9..3980604aab6 100644 --- a/pkg/kubelet/kuberuntime/kuberuntime_sandbox.go +++ b/pkg/kubelet/kuberuntime/kuberuntime_sandbox.go @@ -95,17 +95,12 @@ func (m *kubeGenericRuntimeManager) generatePodSandboxConfig(pod *v1.Pod, attemp logDir := buildPodLogsDirectory(pod.UID) podSandboxConfig.LogDirectory = logDir - cgroupParent := "" portMappings := []*runtimeapi.PortMapping{} for _, c := range pod.Spec.Containers { - // TODO: use a separate interface to only generate portmappings - opts, err := m.runtimeHelper.GenerateRunContainerOptions(pod, &c, "") - if err != nil { - return nil, err - } + containerPortMappings := kubecontainer.MakePortMappings(&c) - for idx := range opts.PortMappings { - port := opts.PortMappings[idx] + for idx := range containerPortMappings { + port := containerPortMappings[idx] hostPort := int32(port.HostPort) containerPort := int32(port.ContainerPort) protocol := toRuntimeProtocol(port.Protocol) @@ -117,9 +112,9 @@ func (m *kubeGenericRuntimeManager) generatePodSandboxConfig(pod *v1.Pod, attemp }) } - // TODO: refactor kubelet to get cgroup parent for pod instead of containers - cgroupParent = opts.CgroupParent } + + _, cgroupParent := m.runtimeHelper.GetPodCgroupParent(pod) podSandboxConfig.Linux = m.generatePodSandboxLinuxConfig(pod, cgroupParent) if len(portMappings) > 0 { podSandboxConfig.PortMappings = portMappings diff --git a/pkg/kubelet/rkt/BUILD b/pkg/kubelet/rkt/BUILD index 679bbeb3467..04d66a3c090 100644 --- a/pkg/kubelet/rkt/BUILD +++ b/pkg/kubelet/rkt/BUILD @@ -71,6 +71,7 @@ go_test( tags = ["automanaged"], deps = [ "//pkg/api/v1:go_default_library", + "//pkg/kubelet/cm:go_default_library", "//pkg/kubelet/container:go_default_library", "//pkg/kubelet/container/testing:go_default_library", "//pkg/kubelet/lifecycle:go_default_library", diff --git a/pkg/kubelet/rkt/fake_rkt_interface_test.go b/pkg/kubelet/rkt/fake_rkt_interface_test.go index 77311b0361a..08830d10726 100644 --- a/pkg/kubelet/rkt/fake_rkt_interface_test.go +++ b/pkg/kubelet/rkt/fake_rkt_interface_test.go @@ -28,6 +28,7 @@ import ( "google.golang.org/grpc" "k8s.io/apimachinery/pkg/types" "k8s.io/kubernetes/pkg/api/v1" + "k8s.io/kubernetes/pkg/kubelet/cm" kubecontainer "k8s.io/kubernetes/pkg/kubelet/container" ) @@ -151,6 +152,7 @@ func (f *fakeSystemd) ResetFailedUnit(name string) error { } // fakeRuntimeHelper implementes kubecontainer.RuntimeHelper interfaces for testing purpose. +// TODO(random-liu): Move this into pkg/kubelet/container/testing type fakeRuntimeHelper struct { dnsServers []string dnsSearches []string @@ -163,6 +165,10 @@ func (f *fakeRuntimeHelper) GenerateRunContainerOptions(pod *v1.Pod, container * return nil, fmt.Errorf("Not implemented") } +func (f *fakeRuntimeHelper) GetPodCgroupParent(pod *v1.Pod) (cm.CgroupName, string) { + return "", "" +} + func (f *fakeRuntimeHelper) GetClusterDNS(pod *v1.Pod) ([]string, []string, error) { return f.dnsServers, f.dnsSearches, f.err }