Merge pull request #42081 from Random-Liu/remove-extra-operation-when-start-podsandbox

Remove extra operations when generating pod sandbox configuration.

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",

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
+}

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)
+	}
+}

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",

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")
 }

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()
+	_, cgroupParent := kl.GetPodCgroupParent(pod)
-	_, podContainerName := pcm.GetPodContainerName(pod)
+	opts := &kubecontainer.RunContainerOptions{CgroupParent: cgroupParent}
-	opts := &kubecontainer.RunContainerOptions{CgroupParent: podContainerName}
 	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)

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()

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",

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 {

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
+		containerPortMappings := kubecontainer.MakePortMappings(&c)
-		opts, err := m.runtimeHelper.GenerateRunContainerOptions(pod, &c, "")
-		if err != nil {
-			return nil, err
-		}
 
-		for idx := range opts.PortMappings {
+		for idx := range containerPortMappings {
-			port := opts.PortMappings[idx]
+			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

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",

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
 }