rkt: Refactoring the construction of the mount points.

So that at most one volume object will be created for every unique
host path. Also the volume's name is random generated UUID to avoid
collision since the mount point's name passed by kubelet is not
guaranteed to be unique when 'subpath' is specified.

pkg/kubelet/container/runtime.go

@@ -342,6 +342,8 @@ type EnvVar struct {
 
 type Mount struct {
 	// Name of the volume mount.
+	// TODO(yifan): Remove this field, as this is not representing the unique name of the mount,
+	// but the volume name only.
 	Name string
 	// Path of the mount within the container.
 	ContainerPath string

pkg/kubelet/rkt/rkt.go

@@ -451,21 +451,16 @@ func mergeEnv(app *appctypes.App, optEnv []kubecontainer.EnvVar) {
 	}
 }
 
-// mergeMounts merges the optMounts with the image's mount points.
+// mergeMounts merges the mountPoints with the image's mount points.
 // The mount points defined in the image will be overridden by the ones
-// with the same name in optMounts.
+// with the same container path.
-func mergeMounts(app *appctypes.App, optMounts []kubecontainer.Mount) {
+func mergeMounts(app *appctypes.App, mountPoints []appctypes.MountPoint) {
-	mountMap := make(map[appctypes.ACName]appctypes.MountPoint)
+	mountMap := make(map[string]appctypes.MountPoint)
 	for _, m := range app.MountPoints {
-		mountMap[m.Name] = m
+		mountMap[m.Path] = m
 	}
-	for _, m := range optMounts {
+	for _, m := range mountPoints {
-		mpName := convertToACName(m.Name)
+		mountMap[m.Path] = m
-		mountMap[mpName] = appctypes.MountPoint{
-			Name:     mpName,
-			Path:     m.ContainerPath,
-			ReadOnly: m.ReadOnly,
-		}
 	}
 	app.MountPoints = nil
 	for _, mount := range mountMap {
@@ -473,21 +468,16 @@ func mergeMounts(app *appctypes.App, optMounts []kubecontainer.Mount) {
 	}
 }
 
-// mergePortMappings merges the optPortMappings with the image's port mappings.
+// mergePortMappings merges the containerPorts with the image's container ports.
 // The port mappings defined in the image will be overridden by the ones
 // with the same name in optPortMappings.
-func mergePortMappings(app *appctypes.App, optPortMappings []kubecontainer.PortMapping) {
+func mergePortMappings(app *appctypes.App, containerPorts []appctypes.Port) {
 	portMap := make(map[appctypes.ACName]appctypes.Port)
 	for _, p := range app.Ports {
 		portMap[p.Name] = p
 	}
-	for _, p := range optPortMappings {
+	for _, p := range containerPorts {
-		pName := convertToACName(p.Name)
+		portMap[p.Name] = p
-		portMap[pName] = appctypes.Port{
-			Name:     pName,
-			Protocol: string(p.Protocol),
-			Port:     uint(p.ContainerPort),
-		}
 	}
 	app.Ports = nil
 	for _, port := range portMap {
@@ -525,7 +515,10 @@ func setSupplementalGIDs(app *appctypes.App, podCtx *api.PodSecurityContext, sup
 }
 
 // setApp merges the container spec with the image's manifest.
-func setApp(imgManifest *appcschema.ImageManifest, c *api.Container, opts *kubecontainer.RunContainerOptions, ctx *api.SecurityContext, podCtx *api.PodSecurityContext, supplementalGids []int64) error {
+func setApp(imgManifest *appcschema.ImageManifest, c *api.Container,
+	mountPoints []appctypes.MountPoint, containerPorts []appctypes.Port, envs []kubecontainer.EnvVar,
+	ctx *api.SecurityContext, podCtx *api.PodSecurityContext, supplementalGids []int64) error {
+
 	app := imgManifest.App
 
 	// Set up Exec.
@@ -544,7 +537,7 @@ func setApp(imgManifest *appcschema.ImageManifest, c *api.Container, opts *kubec
 			return fmt.Errorf("cannot unmarshal CMD %q: %v", ag, err)
 		}
 	}
-	userCommand, userArgs := kubecontainer.ExpandContainerCommandAndArgs(c, opts.Envs)
+	userCommand, userArgs := kubecontainer.ExpandContainerCommandAndArgs(c, envs)
 
 	if len(userCommand) > 0 {
 		command = userCommand
@@ -589,9 +582,9 @@ func setApp(imgManifest *appcschema.ImageManifest, c *api.Container, opts *kubec
 
 	// Notes that we don't create Mounts section in the pod manifest here,
 	// as Mounts will be automatically generated by rkt.
-	mergeMounts(app, opts.Mounts)
+	mergeMounts(app, mountPoints)
-	mergeEnv(app, opts.Envs)
+	mergeEnv(app, envs)
-	mergePortMappings(app, opts.PortMappings)
+	mergePortMappings(app, containerPorts)
 
 	return setIsolators(app, c, ctx)
 }
@@ -752,6 +745,17 @@ func (r *Runtime) makeContainerLogMount(opts *kubecontainer.RunContainerOptions,
 }
 
 func (r *Runtime) newAppcRuntimeApp(pod *api.Pod, podIP string, c api.Container, requiresPrivileged bool, pullSecrets []api.Secret, manifest *appcschema.PodManifest) error {
+	var annotations appctypes.Annotations = []appctypes.Annotation{
+		{
+			Name:  *appctypes.MustACIdentifier(k8sRktContainerHashAnno),
+			Value: strconv.FormatUint(kubecontainer.HashContainer(&c), 10),
+		},
+		{
+			Name:  *appctypes.MustACIdentifier(types.KubernetesContainerNameLabel),
+			Value: c.Name,
+		},
+	}
+
 	if requiresPrivileged && !securitycontext.HasPrivilegedRequest(&c) {
 		return fmt.Errorf("cannot make %q: running a custom stage1 requires a privileged security context", format.Pod(pod))
 	}
@@ -782,93 +786,51 @@ func (r *Runtime) newAppcRuntimeApp(pod *api.Pod, podIP string, c api.Container,
 		return err
 	}
 
-	// create the container log file and make a mount pair.
+	// Create additional mount for termintation message path.
-	mnt, err := r.makeContainerLogMount(opts, &c)
+	mount, err := r.makeContainerLogMount(opts, &c)
 	if err != nil {
 		return err
 	}
+	mounts := append(opts.Mounts, *mount)
+	annotations = append(annotations, appctypes.Annotation{
+		Name:  *appctypes.MustACIdentifier(k8sRktTerminationMessagePathAnno),
+		Value: mount.HostPath,
+	})
 
-	// If run in 'hostnetwork' mode, then copy and mount the host's /etc/resolv.conf and /etc/hosts,
+	// If run in 'hostnetwork' mode, then copy the host's /etc/resolv.conf and /etc/hosts,
-	// and add volumes.
+	// and add mounts.
-	var hostsMnt, resolvMnt *kubecontainer.Mount
 	if kubecontainer.IsHostNetworkPod(pod) {
-		hostsMnt, resolvMnt, err = makeHostNetworkMount(opts)
+		hostsMount, resolvMount, err := makeHostNetworkMount(opts)
 		if err != nil {
 			return err
 		}
-		manifest.Volumes = append(manifest.Volumes, appctypes.Volume{
+		mounts = append(mounts, *hostsMount, *resolvMount)
-			Name:   convertToACName(hostsMnt.Name),
-			Kind:   "host",
-			Source: hostsMnt.HostPath,
-		})
-		manifest.Volumes = append(manifest.Volumes, appctypes.Volume{
-			Name:   convertToACName(resolvMnt.Name),
-			Kind:   "host",
-			Source: resolvMnt.HostPath,
-		})
 	}
 
 	supplementalGids := r.runtimeHelper.GetExtraSupplementalGroupsForPod(pod)
 	ctx := securitycontext.DetermineEffectiveSecurityContext(pod, &c)
-	if err := setApp(imgManifest, &c, opts, ctx, pod.Spec.SecurityContext, supplementalGids); err != nil {
+
+	volumes, mountPoints := convertKubeMounts(mounts)
+	containerPorts, hostPorts := convertKubePortMappings(opts.PortMappings)
+
+	if err := setApp(imgManifest, &c, mountPoints, containerPorts, opts.Envs, ctx, pod.Spec.SecurityContext, supplementalGids); err != nil {
 		return err
 	}
 
-	for _, mnt := range opts.Mounts {
-		readOnly := mnt.ReadOnly
-		manifest.Volumes = append(manifest.Volumes, appctypes.Volume{
-			Name:     convertToACName(mnt.Name),
-			Source:   mnt.HostPath,
-			Kind:     "host",
-			ReadOnly: &readOnly,
-		})
-	}
-
 	ra := appcschema.RuntimeApp{
-		Name:  convertToACName(c.Name),
+		Name:        convertToACName(c.Name),
-		Image: appcschema.RuntimeImage{ID: *hash},
+		Image:       appcschema.RuntimeImage{ID: *hash},
-		App:   imgManifest.App,
+		App:         imgManifest.App,
-		Annotations: []appctypes.Annotation{
+		Annotations: annotations,
-			{
-				Name:  *appctypes.MustACIdentifier(k8sRktContainerHashAnno),
-				Value: strconv.FormatUint(kubecontainer.HashContainer(&c), 10),
-			},
-			{
-				Name:  *appctypes.MustACIdentifier(types.KubernetesContainerNameLabel),
-				Value: c.Name,
-			},
-		},
 	}
 
 	if c.SecurityContext != nil && c.SecurityContext.ReadOnlyRootFilesystem != nil {
 		ra.ReadOnlyRootFS = *c.SecurityContext.ReadOnlyRootFilesystem
 	}
 
-	if mnt != nil {
-		ra.Annotations = append(ra.Annotations, appctypes.Annotation{
-			Name:  *appctypes.MustACIdentifier(k8sRktTerminationMessagePathAnno),
-			Value: mnt.HostPath,
-		})
-
-		manifest.Volumes = append(manifest.Volumes, appctypes.Volume{
-			Name:   convertToACName(mnt.Name),
-			Kind:   "host",
-			Source: mnt.HostPath,
-		})
-	}
-
 	manifest.Apps = append(manifest.Apps, ra)
-
+	manifest.Volumes = append(manifest.Volumes, volumes...)
-	// Set global ports.
+	manifest.Ports = append(manifest.Ports, hostPorts...)
-	for _, port := range opts.PortMappings {
-		if port.HostPort == 0 {
-			continue
-		}
-		manifest.Ports = append(manifest.Ports, appctypes.ExposedPort{
-			Name:     convertToACName(port.Name),
-			HostPort: uint(port.HostPort),
-		})
-	}
 
 	return nil
 }
@@ -2347,3 +2309,75 @@ func getOSReleaseInfo() (map[string]string, error) {
 	}
 	return result, nil
 }
+
+// convertKubeMounts creates appc volumes and mount points according to the given mounts.
+// Only one volume will be created for every unique host path.
+// Only one mount point will be created for every unique container path.
+func convertKubeMounts(mounts []kubecontainer.Mount) ([]appctypes.Volume, []appctypes.MountPoint) {
+	volumeMap := make(map[string]*appctypes.Volume)
+	mountPointMap := make(map[string]*appctypes.MountPoint)
+
+	for _, mnt := range mounts {
+		readOnly := mnt.ReadOnly
+
+		if _, existed := volumeMap[mnt.HostPath]; !existed {
+			volumeMap[mnt.HostPath] = &appctypes.Volume{
+				Name:     *appctypes.MustACName(string(uuid.NewUUID())),
+				Kind:     "host",
+				Source:   mnt.HostPath,
+				ReadOnly: &readOnly,
+			}
+		}
+
+		if _, existed := mountPointMap[mnt.ContainerPath]; existed {
+			glog.Warningf("Multiple mount points with the same container path %v, ignore it", mnt)
+			continue
+		}
+
+		mountPointMap[mnt.ContainerPath] = &appctypes.MountPoint{
+			Name:     volumeMap[mnt.HostPath].Name,
+			Path:     mnt.ContainerPath,
+			ReadOnly: readOnly,
+		}
+	}
+
+	volumes := make([]appctypes.Volume, 0, len(volumeMap))
+	mountPoints := make([]appctypes.MountPoint, 0, len(mountPointMap))
+
+	for _, vol := range volumeMap {
+		volumes = append(volumes, *vol)
+	}
+	for _, mnt := range mountPointMap {
+		mountPoints = append(mountPoints, *mnt)
+	}
+
+	return volumes, mountPoints
+}
+
+// convertKubePortMappings creates appc container ports and host ports according to the given port mappings.
+// The container ports and host ports are mapped by PortMapping.Name.
+func convertKubePortMappings(portMappings []kubecontainer.PortMapping) ([]appctypes.Port, []appctypes.ExposedPort) {
+	containerPorts := make([]appctypes.Port, 0, len(portMappings))
+	hostPorts := make([]appctypes.ExposedPort, 0, len(portMappings))
+
+	for _, p := range portMappings {
+		// This matches the docker code's behaviour.
+		if p.HostPort == 0 {
+			continue
+		}
+
+		portName := convertToACName(p.Name)
+		containerPorts = append(containerPorts, appctypes.Port{
+			Name:     portName,
+			Protocol: string(p.Protocol),
+			Port:     uint(p.ContainerPort),
+		})
+
+		hostPorts = append(hostPorts, appctypes.ExposedPort{
+			Name:     portName,
+			HostPort: uint(p.HostPort),
+		})
+	}
+
+	return containerPorts, hostPorts
+}

pkg/kubelet/rkt/rkt_test.go

@@ -949,7 +949,9 @@ func TestSetApp(t *testing.T) {
 
 	tests := []struct {
 		container        *api.Container
-		opts             *kubecontainer.RunContainerOptions
+		mountPoints      []appctypes.MountPoint
+		containerPorts   []appctypes.Port
+		envs             []kubecontainer.EnvVar
 		ctx              *api.SecurityContext
 		podCtx           *api.PodSecurityContext
 		supplementalGids []int64
@@ -959,7 +961,9 @@ func TestSetApp(t *testing.T) {
 		// Nothing should change, but the "User" and "Group" should be filled.
 		{
 			container:        &api.Container{},
-			opts:             &kubecontainer.RunContainerOptions{},
+			mountPoints:      []appctypes.MountPoint{},
+			containerPorts:   []appctypes.Port{},
+			envs:             []kubecontainer.EnvVar{},
 			ctx:              nil,
 			podCtx:           nil,
 			supplementalGids: nil,
@@ -969,8 +973,10 @@ func TestSetApp(t *testing.T) {
 
 		// error verifying non-root.
 		{
-			container: &api.Container{},
+			container:      &api.Container{},
-			opts:      &kubecontainer.RunContainerOptions{},
+			mountPoints:    []appctypes.MountPoint{},
+			containerPorts: []appctypes.Port{},
+			envs:           []kubecontainer.EnvVar{},
 			ctx: &api.SecurityContext{
 				RunAsNonRoot: &runAsNonRootTrue,
 				RunAsUser:    &rootUser,
@@ -986,7 +992,9 @@ func TestSetApp(t *testing.T) {
 			container: &api.Container{
 				Args: []string{"foo"},
 			},
-			opts:             &kubecontainer.RunContainerOptions{},
+			mountPoints:      []appctypes.MountPoint{},
+			containerPorts:   []appctypes.Port{},
+			envs:             []kubecontainer.EnvVar{},
 			ctx:              nil,
 			podCtx:           nil,
 			supplementalGids: nil,
@@ -1025,16 +1033,14 @@ func TestSetApp(t *testing.T) {
 					Requests: api.ResourceList{"cpu": resource.MustParse("5m"), "memory": resource.MustParse("5M")},
 				},
 			},
-			opts: &kubecontainer.RunContainerOptions{
+			mountPoints: []appctypes.MountPoint{
-				Envs: []kubecontainer.EnvVar{
+				{Name: *appctypes.MustACName("mnt-bar"), Path: "/mnt-bar", ReadOnly: true},
-					{Name: "env-bar", Value: "foo"},
+			},
-				},
+			containerPorts: []appctypes.Port{
-				Mounts: []kubecontainer.Mount{
+				{Name: *appctypes.MustACName("port-bar"), Protocol: "TCP", Port: 1234},
-					{Name: "mnt-bar", ContainerPath: "/mnt-bar", ReadOnly: true},
+			},
-				},
+			envs: []kubecontainer.EnvVar{
-				PortMappings: []kubecontainer.PortMapping{
+				{Name: "env-bar", Value: "foo"},
-					{Name: "port-bar", Protocol: api.ProtocolTCP, ContainerPort: 1234},
-				},
 			},
 			ctx: &api.SecurityContext{
 				Capabilities: &api.Capabilities{
@@ -1088,17 +1094,15 @@ func TestSetApp(t *testing.T) {
 					Requests: api.ResourceList{"memory": resource.MustParse("5M")},
 				},
 			},
-			opts: &kubecontainer.RunContainerOptions{
+			mountPoints: []appctypes.MountPoint{
-				Envs: []kubecontainer.EnvVar{
+				{Name: *appctypes.MustACName("mnt-foo"), Path: "/mnt-foo", ReadOnly: true},
-					{Name: "env-foo", Value: "foo"},
+			},
-					{Name: "env-bar", Value: "bar"},
+			containerPorts: []appctypes.Port{
-				},
+				{Name: *appctypes.MustACName("port-foo"), Protocol: "TCP", Port: 1234},
-				Mounts: []kubecontainer.Mount{
+			},
-					{Name: "mnt-foo", ContainerPath: "/mnt-bar", ReadOnly: true},
+			envs: []kubecontainer.EnvVar{
-				},
+				{Name: "env-foo", Value: "foo"},
-				PortMappings: []kubecontainer.PortMapping{
+				{Name: "env-bar", Value: "bar"},
-					{Name: "port-foo", Protocol: api.ProtocolTCP, ContainerPort: 1234},
-				},
 			},
 			ctx: &api.SecurityContext{
 				Capabilities: &api.Capabilities{
@@ -1124,7 +1128,7 @@ func TestSetApp(t *testing.T) {
 					{Name: "env-bar", Value: "bar"},
 				},
 				MountPoints: []appctypes.MountPoint{
-					{Name: *appctypes.MustACName("mnt-foo"), Path: "/mnt-bar", ReadOnly: true},
+					{Name: *appctypes.MustACName("mnt-foo"), Path: "/mnt-foo", ReadOnly: true},
 				},
 				Ports: []appctypes.Port{
 					{Name: *appctypes.MustACName("port-foo"), Protocol: "TCP", Port: 1234},
@@ -1142,7 +1146,11 @@ func TestSetApp(t *testing.T) {
 	for i, tt := range tests {
 		testCaseHint := fmt.Sprintf("test case #%d", i)
 		img := baseImageManifest(t)
-		err := setApp(img, tt.container, tt.opts, tt.ctx, tt.podCtx, tt.supplementalGids)
+
+		err := setApp(img, tt.container,
+			tt.mountPoints, tt.containerPorts, tt.envs,
+			tt.ctx, tt.podCtx, tt.supplementalGids)
+
 		if err == nil && tt.err != nil || err != nil && tt.err == nil {
 			t.Errorf("%s: expect %v, saw %v", testCaseHint, tt.err, err)
 		}