Improve parsing of mounts

Specifically, we were needlessly doing a double lookup to find which config mounts were user volumes. Improve this by refactoring a bit of code from inspect

Signed-off-by: Peter Hunt <pehunt@redhat.com>

libpod/container_inspect.go

@@ -368,58 +368,41 @@ func (c *Container) getInspectMounts(ctrSpec *spec.Spec) ([]InspectMount, error)
 		return inspectMounts, nil
 	}
 
-	// We need to parse all named volumes and mounts into maps, so we don't
-	// end up with repeated lookups for each user volume.
-	// Map destination to struct, as destination is what is stored in
-	// UserVolumes.
-	namedVolumes := make(map[string]*ContainerNamedVolume)
-	mounts := make(map[string]spec.Mount)
-	for _, namedVol := range c.config.NamedVolumes {
-		namedVolumes[namedVol.Dest] = namedVol
-	}
-	for _, mount := range ctrSpec.Mounts {
-		mounts[mount.Destination] = mount
-	}
+	namedVolumes, mounts := c.sortUserVolumes(ctrSpec)
+	for _, volume := range namedVolumes {
+		mountStruct := InspectMount{}
+		mountStruct.Type = "volume"
+		mountStruct.Destination = volume.Dest
+		mountStruct.Name = volume.Name
 
-	for _, vol := range c.config.UserVolumes {
-		// We need to look up the volumes.
-		// First: is it a named volume?
-		if volume, ok := namedVolumes[vol]; ok {
-			mountStruct := InspectMount{}
-			mountStruct.Type = "volume"
-			mountStruct.Destination = volume.Dest
-			mountStruct.Name = volume.Name
-
-			// For src and driver, we need to look up the named
-			// volume.
-			volFromDB, err := c.runtime.state.Volume(volume.Name)
-			if err != nil {
-				return nil, errors.Wrapf(err, "error looking up volume %s in container %s config", volume.Name, c.ID())
-			}
-			mountStruct.Driver = volFromDB.Driver()
-			mountStruct.Source = volFromDB.MountPoint()
-
-			parseMountOptionsForInspect(volume.Options, &mountStruct)
-
-			inspectMounts = append(inspectMounts, mountStruct)
-		} else if mount, ok := mounts[vol]; ok {
-			// It's a mount.
-			// Is it a tmpfs? If so, discard.
-			if mount.Type == "tmpfs" {
-				continue
-			}
-
-			mountStruct := InspectMount{}
-			mountStruct.Type = "bind"
-			mountStruct.Source = mount.Source
-			mountStruct.Destination = mount.Destination
-
-			parseMountOptionsForInspect(mount.Options, &mountStruct)
-
-			inspectMounts = append(inspectMounts, mountStruct)
+		// For src and driver, we need to look up the named
+		// volume.
+		volFromDB, err := c.runtime.state.Volume(volume.Name)
+		if err != nil {
+			return nil, errors.Wrapf(err, "error looking up volume %s in container %s config", volume.Name, c.ID())
 		}
-		// We couldn't find a mount. Log a warning.
-		logrus.Warnf("Could not find mount at destination %q when building inspect output for container %s", vol, c.ID())
+		mountStruct.Driver = volFromDB.Driver()
+		mountStruct.Source = volFromDB.MountPoint()
+
+		parseMountOptionsForInspect(volume.Options, &mountStruct)
+
+		inspectMounts = append(inspectMounts, mountStruct)
+	}
+	for _, mount := range mounts {
+		// It's a mount.
+		// Is it a tmpfs? If so, discard.
+		if mount.Type == "tmpfs" {
+			continue
+		}
+
+		mountStruct := InspectMount{}
+		mountStruct.Type = "bind"
+		mountStruct.Source = mount.Source
+		mountStruct.Destination = mount.Destination
+
+		parseMountOptionsForInspect(mount.Options, &mountStruct)
+
+		inspectMounts = append(inspectMounts, mountStruct)
 	}
 
 	return inspectMounts, nil

libpod/container_internal.go

@@ -1534,3 +1534,34 @@ func (c *Container) prepareCheckpointExport() (err error) {
 
 	return nil
 }
+
+// sortUserVolumes sorts the volumes specified for a container
+// between named and normal volumes
+func (c *Container) sortUserVolumes(ctrSpec *spec.Spec) ([]*ContainerNamedVolume, []spec.Mount) {
+	namedUserVolumes := []*ContainerNamedVolume{}
+	userMounts := []spec.Mount{}
+
+	// We need to parse all named volumes and mounts into maps, so we don't
+	// end up with repeated lookups for each user volume.
+	// Map destination to struct, as destination is what is stored in
+	// UserVolumes.
+	namedVolumes := make(map[string]*ContainerNamedVolume)
+	mounts := make(map[string]spec.Mount)
+	for _, namedVol := range c.config.NamedVolumes {
+		namedVolumes[namedVol.Dest] = namedVol
+	}
+	for _, mount := range ctrSpec.Mounts {
+		mounts[mount.Destination] = mount
+	}
+
+	for _, vol := range c.config.UserVolumes {
+		if volume, ok := namedVolumes[vol]; ok {
+			namedUserVolumes = append(namedUserVolumes, volume)
+		} else if mount, ok := mounts[vol]; ok {
+			userMounts = append(userMounts, mount)
+		} else {
+			logrus.Warnf("Could not find mount at destination %q when parsing user volumes for container %s", vol, c.ID())
+		}
+	}
+	return namedUserVolumes, userMounts
+}

libpod/kube.go

@@ -340,40 +340,58 @@ func libpodMaxAndMinToResourceList(c *Container) (v1.ResourceList, v1.ResourceLi
 	return maxResources, minResources
 }
 
-func generateKubeVolumeMount(hostSourcePath string, mounts []specs.Mount) (v1.VolumeMount, v1.Volume, error) {
+// libpodMountsToKubeVolumeMounts converts the containers mounts to a struct kube understands
+func libpodMountsToKubeVolumeMounts(c *Container) ([]v1.VolumeMount, []v1.Volume, error) {
+	var vms []v1.VolumeMount
+	var vos []v1.Volume
+
+	// TjDO when named volumes are supported in play kube, also parse named volumes here
+	_, mounts := c.sortUserVolumes(c.config.Spec)
+	for _, m := range mounts {
+		vm, vo, err := generateKubeVolumeMount(m)
+		if err != nil {
+			return vms, vos, err
+		}
+		vms = append(vms, vm)
+		vos = append(vos, vo)
+	}
+	return vms, vos, nil
+}
+
+// generateKubeVolumeMount takes a user specfied mount and returns
+// a kubernetes VolumeMount (to be added to the container) and a kubernetes Volume
+// (to be added to the pod)
+func generateKubeVolumeMount(m specs.Mount) (v1.VolumeMount, v1.Volume, error) {
 	vm := v1.VolumeMount{}
 	vo := v1.Volume{}
-	for _, m := range mounts {
-		if m.Destination == hostSourcePath {
-			name, err := convertVolumePathToName(m.Source)
-			if err != nil {
-				return vm, vo, err
-			}
-			vm.Name = name
-			vm.MountPath = m.Destination
-			if util.StringInSlice("ro", m.Options) {
-				vm.ReadOnly = true
-			}
 
-			vo.Name = name
-			vo.HostPath = &v1.HostPathVolumeSource{}
-			vo.HostPath.Path = m.Source
-			isDir, err := isHostPathDirectory(m.Source)
-			// neither a directory or a file lives here, default to creating a directory
-			// TODO should this be an error instead?
-			var hostPathType v1.HostPathType
-			if err != nil {
-				hostPathType = v1.HostPathDirectoryOrCreate
-			} else if isDir {
-				hostPathType = v1.HostPathDirectory
-			} else {
-				hostPathType = v1.HostPathFile
-			}
-			vo.HostPath.Type = &hostPathType
-			return vm, vo, nil
-		}
+	name, err := convertVolumePathToName(m.Source)
+	if err != nil {
+		return vm, vo, err
 	}
-	return vm, vo, errors.New("unable to find mount source")
+	vm.Name = name
+	vm.MountPath = m.Destination
+	if util.StringInSlice("ro", m.Options) {
+		vm.ReadOnly = true
+	}
+
+	vo.Name = name
+	vo.HostPath = &v1.HostPathVolumeSource{}
+	vo.HostPath.Path = m.Source
+	isDir, err := isHostPathDirectory(m.Source)
+	// neither a directory or a file lives here, default to creating a directory
+	// TODO should this be an error instead?
+	var hostPathType v1.HostPathType
+	if err != nil {
+		hostPathType = v1.HostPathDirectoryOrCreate
+	} else if isDir {
+		hostPathType = v1.HostPathDirectory
+	} else {
+		hostPathType = v1.HostPathFile
+	}
+	vo.HostPath.Type = &hostPathType
+
+	return vm, vo, nil
 }
 
 func isHostPathDirectory(hostPathSource string) (bool, error) {
@@ -400,23 +418,6 @@ func convertVolumePathToName(hostSourcePath string) (string, error) {
 	return strings.Replace(strings.Trim(hostSourcePath, "/"), "/", "-", -1), nil
 }
 
-// libpodMountsToKubeVolumeMounts converts the containers mounts to a struct kube understands
-func libpodMountsToKubeVolumeMounts(c *Container) ([]v1.VolumeMount, []v1.Volume, error) {
-	// At this point, I dont think we can distinguish between the default
-	// volume mounts and user added ones.  For now, we pass them all.
-	var vms []v1.VolumeMount
-	var vos []v1.Volume
-	for _, hostSourcePath := range c.config.UserVolumes {
-		vm, vo, err := generateKubeVolumeMount(hostSourcePath, c.config.Spec.Mounts)
-		if err != nil {
-			return vms, vos, err
-		}
-		vms = append(vms, vm)
-		vos = append(vos, vo)
-	}
-	return vms, vos, nil
-}
-
 func determineCapAddDropFromCapabilities(defaultCaps, containerCaps []string) *v1.Capabilities {
 	var (
 		drop []v1.Capability