From 7309e38ddda4245a786bb92d036f7836328818c9 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Tue, 26 Mar 2019 15:29:20 -0400 Subject: Add handling for new named volumes code in pkg/spec Now that named volumes must be explicitly enumerated rather than passed in with all other volumes, we need to split normal and named volumes up before passing them into libpod. This PR does this. Signed-off-by: Matthew Heon --- pkg/spec/createconfig.go | 17 +++++++++++------ pkg/spec/spec.go | 41 ++++++++++++++++++++++++++++++++++++++--- 2 files changed, 49 insertions(+), 9 deletions(-) (limited to 'pkg') diff --git a/pkg/spec/createconfig.go b/pkg/spec/createconfig.go index a433fc16d..86c36e6a1 100644 --- a/pkg/spec/createconfig.go +++ b/pkg/spec/createconfig.go @@ -130,11 +130,12 @@ type CreateConfig struct { Mounts []spec.Mount //mounts Volumes []string //volume VolumesFrom []string - WorkDir string //workdir - LabelOpts []string //SecurityOpts - NoNewPrivs bool //SecurityOpts - ApparmorProfile string //SecurityOpts - SeccompProfilePath string //SecurityOpts + NamedVolumes []*libpod.ContainerNamedVolume // Filled in by CreateConfigToOCISpec + WorkDir string //workdir + LabelOpts []string //SecurityOpts + NoNewPrivs bool //SecurityOpts + ApparmorProfile string //SecurityOpts + SeccompProfilePath string //SecurityOpts SecurityOpts []string Rootfs string LocalVolumes []spec.Mount //Keeps track of the built-in volumes of container used in the --volumes-from flag @@ -217,7 +218,7 @@ func (c *CreateConfig) initFSMounts() []spec.Mount { return mounts } -//GetVolumeMounts takes user provided input for bind mounts and creates Mount structs +// GetVolumeMounts takes user provided input for bind mounts and creates Mount structs func (c *CreateConfig) GetVolumeMounts(specMounts []spec.Mount) ([]spec.Mount, error) { m := c.LocalVolumes for _, i := range c.Volumes { @@ -423,6 +424,10 @@ func (c *CreateConfig) GetContainerCreateOptions(runtime *libpod.Runtime, pod *l options = append(options, libpod.WithUserVolumes(volumes)) } + if len(c.NamedVolumes) != 0 { + options = append(options, libpod.WithNamedVolumes(c.NamedVolumes)) + } + if len(c.LocalVolumes) != 0 { options = append(options, libpod.WithLocalVolumes(c.LocalVolumes)) } diff --git a/pkg/spec/spec.go b/pkg/spec/spec.go index a61741f73..53aad21d7 100644 --- a/pkg/spec/spec.go +++ b/pkg/spec/spec.go @@ -6,6 +6,7 @@ import ( "path/filepath" "strings" + "github.com/containers/libpod/libpod" "github.com/containers/libpod/pkg/rootless" "github.com/containers/storage/pkg/mount" pmount "github.com/containers/storage/pkg/mount" @@ -19,6 +20,7 @@ import ( ) const cpuPeriod = 100000 +const bindMount = "bind" func supercedeUserMounts(mounts []spec.Mount, configMount []spec.Mount) []spec.Mount { if len(mounts) > 0 { @@ -48,6 +50,33 @@ func supercedeUserMounts(mounts []spec.Mount, configMount []spec.Mount) []spec.M return configMount } +// Split named volumes from normal volumes +func splitNamedVolumes(mounts []spec.Mount) ([]spec.Mount, []*libpod.ContainerNamedVolume) { + newMounts := make([]spec.Mount, 0) + namedVolumes := make([]*libpod.ContainerNamedVolume, 0) + for _, mount := range mounts { + // If it's not a named volume, append unconditionally + if mount.Type != bindMount { + newMounts = append(newMounts, mount) + continue + } + // Volumes that are not named volumes must be an absolute or + // relative path. + // Volume names may not begin with a non-alphanumeric character + // so the HasPrefix() check is safe here. + if strings.HasPrefix(mount.Source, "/") || strings.HasPrefix(mount.Source, ".") { + newMounts = append(newMounts, mount) + } else { + namedVolume := new(libpod.ContainerNamedVolume) + namedVolume.Name = mount.Source + namedVolume.Dest = mount.Destination + namedVolume.Options = mount.Options + namedVolumes = append(namedVolumes, namedVolume) + } + } + return newMounts, namedVolumes +} + func getAvailableGids() (int64, error) { idMap, err := user.ParseIDMapFile("/proc/self/gid_map") if err != nil { @@ -99,7 +128,7 @@ func CreateConfigToOCISpec(config *CreateConfig) (*spec.Spec, error) { //nolint } sysMnt := spec.Mount{ Destination: "/sys", - Type: "bind", + Type: bindMount, Source: "/sys", Options: []string{"rprivate", "nosuid", "noexec", "nodev", r, "rbind"}, } @@ -126,7 +155,7 @@ func CreateConfigToOCISpec(config *CreateConfig) (*spec.Spec, error) { //nolint g.RemoveMount("/dev/mqueue") devMqueue := spec.Mount{ Destination: "/dev/mqueue", - Type: "bind", + Type: bindMount, Source: "/dev/mqueue", Options: []string{"bind", "nosuid", "noexec", "nodev"}, } @@ -136,7 +165,7 @@ func CreateConfigToOCISpec(config *CreateConfig) (*spec.Spec, error) { //nolint g.RemoveMount("/proc") procMount := spec.Mount{ Destination: "/proc", - Type: "bind", + Type: bindMount, Source: "/proc", Options: []string{"rbind", "nosuid", "noexec", "nodev"}, } @@ -377,6 +406,12 @@ func CreateConfigToOCISpec(config *CreateConfig) (*spec.Spec, error) { //nolint configSpec.Mounts = supercedeUserMounts(volumeMounts, configSpec.Mounts) //--mount configSpec.Mounts = supercedeUserMounts(config.initFSMounts(), configSpec.Mounts) + + // Split normal mounts and named volumes + newMounts, namedVolumes := splitNamedVolumes(configSpec.Mounts) + configSpec.Mounts = newMounts + config.NamedVolumes = namedVolumes + // BLOCK IO blkio, err := config.CreateBlockIO() if err != nil { -- cgit v1.2.3-54-g00ecf From 42c95eed2cedc1769987984a073e2ec71970e123 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Wed, 3 Apr 2019 19:44:51 -0400 Subject: Major rework of --volumes-from flag The flag should be substantially more durable, and no longer relies on the create artifact. This should allow it to properly handle our new named volume implementation. Signed-off-by: Matthew Heon --- libpod/runtime_ctr.go | 2 ++ pkg/spec/createconfig.go | 89 +++++++++++++++++++++++++++++++----------------- test/e2e/run_test.go | 1 - 3 files changed, 59 insertions(+), 33 deletions(-) (limited to 'pkg') diff --git a/libpod/runtime_ctr.go b/libpod/runtime_ctr.go index ba643150d..800b42851 100644 --- a/libpod/runtime_ctr.go +++ b/libpod/runtime_ctr.go @@ -182,6 +182,8 @@ func (r *Runtime) newContainer(ctx context.Context, rSpec *spec.Spec, options .. return nil, errors.Wrapf(err, "error retrieving named volume %s for new container", vol.Name) } + logrus.Debugf("Creating new volume %s for container", vol.Name) + // The volume does not exist, so we need to create it. newVol, err := r.newVolume(ctx, WithVolumeName(vol.Name), withSetCtrSpecific(), WithVolumeUID(ctr.RootUID()), WithVolumeGID(ctr.RootGID())) diff --git a/pkg/spec/createconfig.go b/pkg/spec/createconfig.go index 86c36e6a1..bcaa2333b 100644 --- a/pkg/spec/createconfig.go +++ b/pkg/spec/createconfig.go @@ -1,7 +1,6 @@ package createconfig import ( - "encoding/json" "fmt" "net" "os" @@ -256,6 +255,8 @@ func (c *CreateConfig) GetVolumeMounts(specMounts []spec.Mount) ([]spec.Mount, e mount.Source = "tmpfs" mount.Options = append(mount.Options, "tmpcopyup") } else { + // TODO: Move support for this and tmpfs into libpod + // Should tmpfs also be handled as named volumes? Wouldn't be hard // This will cause a new local Volume to be created on your system mount.Source = stringid.GenerateNonCryptoID() mount.Options = append(mount.Options, "bind") @@ -269,13 +270,12 @@ func (c *CreateConfig) GetVolumeMounts(specMounts []spec.Mount) ([]spec.Mount, e // GetVolumesFrom reads the create-config artifact of the container to get volumes from // and adds it to c.Volumes of the current container. func (c *CreateConfig) GetVolumesFrom() error { - var options string - if os.Geteuid() != 0 { return nil } for _, vol := range c.VolumesFrom { + options := "" splitVol := strings.SplitN(vol, ":", 2) if len(splitVol) == 2 { options = splitVol[1] @@ -284,41 +284,60 @@ func (c *CreateConfig) GetVolumesFrom() error { if err != nil { return errors.Wrapf(err, "error looking up container %q", splitVol[0]) } - inspect, err := ctr.Inspect(false) - if err != nil { - return errors.Wrapf(err, "error inspecting %q", splitVol[0]) - } - var createArtifact CreateConfig - artifact, err := ctr.GetArtifact("create-config") - if err != nil { - return errors.Wrapf(err, "error getting create-config artifact for %q", splitVol[0]) + + logrus.Debugf("Adding volumes from container %s", ctr.ID()) + + // Look up the container's user volumes. This gets us the + // destinations of all mounts the user added to the container. + userVolumesArr := ctr.UserVolumes() + + // We're going to need to access them a lot, so convert to a map + // to reduce looping. + // We'll also use the map to indicate if we missed any volumes along the way. + userVolumes := make(map[string]bool) + for _, dest := range userVolumesArr { + userVolumes[dest] = false } - if err := json.Unmarshal(artifact, &createArtifact); err != nil { - return err + + // Now we get the container's spec and loop through its volumes + // and append them in if we can find them. + spec := ctr.Spec() + if spec == nil { + return errors.Errorf("error retrieving container %s spec", ctr.ID()) } - for key := range createArtifact.BuiltinImgVolumes { - for _, m := range inspect.Mounts { - if m.Destination == key { - c.LocalVolumes = append(c.LocalVolumes, m) - break + for _, mnt := range spec.Mounts { + if mnt.Type != "bind" { + continue + } + if _, exists := userVolumes[mnt.Destination]; exists { + userVolumes[mnt.Destination] = true + localOptions := options + if localOptions == "" { + localOptions = strings.Join(mnt.Options, ",") } + c.Volumes = append(c.Volumes, fmt.Sprintf("%s:%s:%s", mnt.Source, mnt.Destination, localOptions)) } } - for _, i := range createArtifact.Volumes { - // Volumes format is host-dir:ctr-dir[:options], so get the host and ctr dir - // and add on the options given by the user to the flag. - spliti := strings.SplitN(i, ":", 3) - // Throw error if mounting volume from container with Z option (private label) - // Override this by adding 'z' to options. - if len(spliti) > 2 && strings.Contains(spliti[2], "Z") && !strings.Contains(options, "z") { - return errors.Errorf("volume mounted with private option 'Z' in %q. Use option 'z' to mount in current container", ctr.ID()) + // We're done with the spec mounts. Add named volumes. + // Add these unconditionally - none of them are automatically + // part of the container, as some spec mounts are. + namedVolumes := ctr.NamedVolumes() + for _, namedVol := range namedVolumes { + if _, exists := userVolumes[namedVol.Dest]; exists { + userVolumes[namedVol.Dest] = true } - if options == "" { - // Mount the volumes with the default options - c.Volumes = append(c.Volumes, createArtifact.Volumes...) - } else { - c.Volumes = append(c.Volumes, spliti[0]+":"+spliti[1]+":"+options) + localOptions := options + if localOptions == "" { + localOptions = strings.Join(namedVol.Options, ",") + } + c.Volumes = append(c.Volumes, fmt.Sprintf("%s:%s:%s", namedVol.Name, namedVol.Dest, localOptions)) + } + + // Check if we missed any volumes + for volDest, found := range userVolumes { + if !found { + logrus.Warnf("Unable to match volume %s from container %s for volumes-from", volDest, ctr.ID()) } } } @@ -418,7 +437,13 @@ func (c *CreateConfig) GetContainerCreateOptions(runtime *libpod.Runtime, pod *l // others, if they are included volumes := make([]string, 0, len(c.Volumes)) for _, vol := range c.Volumes { - volumes = append(volumes, strings.SplitN(vol, ":", 2)[0]) + // We always want the volume destination + splitVol := strings.SplitN(vol, ":", 3) + if len(splitVol) > 1 { + volumes = append(volumes, splitVol[1]) + } else { + volumes = append(volumes, splitVol[0]) + } } options = append(options, libpod.WithUserVolumes(volumes)) diff --git a/test/e2e/run_test.go b/test/e2e/run_test.go index 2daf2fe5b..a89ee491b 100644 --- a/test/e2e/run_test.go +++ b/test/e2e/run_test.go @@ -611,7 +611,6 @@ USER mail` session.WaitWithDefaultTimeout() Expect(session.ExitCode()).To(Equal(0)) Expect(session.OutputToString()).To(ContainSubstring("data")) - }) It("podman run --volumes flag with multiple volumes", func() { -- cgit v1.2.3-54-g00ecf From 1fdc89f616fead07b9b49e949f7c7cebba951070 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Wed, 3 Apr 2019 19:49:09 -0400 Subject: Drop LocalVolumes from our the database We were never using it. It's actually a potentially quite sizable field (very expensive to decode an array of structs!). Removing it should do no harm. Signed-off-by: Matthew Heon --- libpod/container.go | 3 --- libpod/options.go | 19 ------------------- pkg/spec/createconfig.go | 27 ++++++++++----------------- pkg/spec/spec.go | 9 ++++----- 4 files changed, 14 insertions(+), 44 deletions(-) (limited to 'pkg') diff --git a/libpod/container.go b/libpod/container.go index 74dc5be5c..6d5e063ab 100644 --- a/libpod/container.go +++ b/libpod/container.go @@ -356,9 +356,6 @@ type ContainerConfig struct { // ExitCommand is the container's exit command. // This Command will be executed when the container exits ExitCommand []string `json:"exitCommand,omitempty"` - // LocalVolumes are the built-in volumes we get from the --volumes-from flag - // It picks up the built-in volumes of the container used by --volumes-from - LocalVolumes []spec.Mount // IsInfra is a bool indicating whether this container is an infra container used for // sharing kernel namespaces in a pod IsInfra bool `json:"pause"` diff --git a/libpod/options.go b/libpod/options.go index 4f35a5c8c..9326e54e4 100644 --- a/libpod/options.go +++ b/libpod/options.go @@ -13,7 +13,6 @@ import ( "github.com/containers/storage" "github.com/containers/storage/pkg/idtools" "github.com/cri-o/ocicni/pkg/ocicni" - spec "github.com/opencontainers/runtime-spec/specs-go" "github.com/pkg/errors" ) @@ -1111,24 +1110,6 @@ func WithUserVolumes(volumes []string) CtrCreateOption { } } -// WithLocalVolumes sets the built-in volumes of the container retrieved -// from a container passed in to the --volumes-from flag. -// This stores the built-in volume information in the Config so we can -// add them when creating the container. -func WithLocalVolumes(volumes []spec.Mount) CtrCreateOption { - return func(ctr *Container) error { - if ctr.valid { - return ErrCtrFinalized - } - - if volumes != nil { - ctr.config.LocalVolumes = append(ctr.config.LocalVolumes, volumes...) - } - - return nil - } -} - // WithEntrypoint sets the entrypoint of the container. // This is not used to change the container's spec, but will instead be used // during commit to populate the entrypoint of the new image. diff --git a/pkg/spec/createconfig.go b/pkg/spec/createconfig.go index bcaa2333b..e71d9d3db 100644 --- a/pkg/spec/createconfig.go +++ b/pkg/spec/createconfig.go @@ -22,18 +22,16 @@ import ( "golang.org/x/sys/unix" ) -type mountType string - // Type constants const ( bps = iota iops // TypeBind is the type for mounting host dir - TypeBind mountType = "bind" + TypeBind = "bind" // TypeVolume is the type for remote storage volumes - // TypeVolume mountType = "volume" // re-enable upon use + // TypeVolume = "volume" // re-enable upon use // TypeTmpfs is the type for mounting tmpfs - TypeTmpfs mountType = "tmpfs" + TypeTmpfs = "tmpfs" ) // CreateResourceConfig represents resource elements in CreateConfig @@ -137,8 +135,7 @@ type CreateConfig struct { SeccompProfilePath string //SecurityOpts SecurityOpts []string Rootfs string - LocalVolumes []spec.Mount //Keeps track of the built-in volumes of container used in the --volumes-from flag - Syslog bool // Whether to enable syslog on exit commands + Syslog bool // Whether to enable syslog on exit commands } func u32Ptr(i int64) *uint32 { u := uint32(i); return &u } @@ -172,9 +169,9 @@ func (c *CreateConfig) AddContainerInitBinary(path string) error { c.Command = append([]string{"/dev/init", "--"}, c.Command...) c.Mounts = append(c.Mounts, spec.Mount{ Destination: "/dev/init", - Type: "bind", + Type: TypeBind, Source: path, - Options: []string{"bind", "ro"}, + Options: []string{TypeBind, "ro"}, }) return nil } @@ -219,7 +216,7 @@ func (c *CreateConfig) initFSMounts() []spec.Mount { // GetVolumeMounts takes user provided input for bind mounts and creates Mount structs func (c *CreateConfig) GetVolumeMounts(specMounts []spec.Mount) ([]spec.Mount, error) { - m := c.LocalVolumes + m := []spec.Mount{} for _, i := range c.Volumes { var options []string spliti := strings.Split(i, ":") @@ -259,7 +256,7 @@ func (c *CreateConfig) GetVolumeMounts(specMounts []spec.Mount) ([]spec.Mount, e // Should tmpfs also be handled as named volumes? Wouldn't be hard // This will cause a new local Volume to be created on your system mount.Source = stringid.GenerateNonCryptoID() - mount.Options = append(mount.Options, "bind") + mount.Options = append(mount.Options, TypeBind) } m = append(m, mount) } @@ -306,7 +303,7 @@ func (c *CreateConfig) GetVolumesFrom() error { return errors.Errorf("error retrieving container %s spec", ctr.ID()) } for _, mnt := range spec.Mounts { - if mnt.Type != "bind" { + if mnt.Type != TypeBind { continue } if _, exists := userVolumes[mnt.Destination]; exists { @@ -453,10 +450,6 @@ func (c *CreateConfig) GetContainerCreateOptions(runtime *libpod.Runtime, pod *l options = append(options, libpod.WithNamedVolumes(c.NamedVolumes)) } - if len(c.LocalVolumes) != 0 { - options = append(options, libpod.WithLocalVolumes(c.LocalVolumes)) - } - if len(c.Command) != 0 { options = append(options, libpod.WithCommand(c.Command)) } @@ -568,7 +561,7 @@ func (c *CreateConfig) GetContainerCreateOptions(runtime *libpod.Runtime, pod *l options = append(options, libpod.WithPrivileged(c.Privileged)) - useImageVolumes := c.ImageVolumeType == "bind" + useImageVolumes := c.ImageVolumeType == TypeBind // Gather up the options for NewContainer which consist of With... funcs options = append(options, libpod.WithRootFSFromImage(c.ImageID, c.Image, useImageVolumes)) options = append(options, libpod.WithSecLabels(c.LabelOpts)) diff --git a/pkg/spec/spec.go b/pkg/spec/spec.go index 53aad21d7..9b6bd089e 100644 --- a/pkg/spec/spec.go +++ b/pkg/spec/spec.go @@ -20,7 +20,6 @@ import ( ) const cpuPeriod = 100000 -const bindMount = "bind" func supercedeUserMounts(mounts []spec.Mount, configMount []spec.Mount) []spec.Mount { if len(mounts) > 0 { @@ -56,7 +55,7 @@ func splitNamedVolumes(mounts []spec.Mount) ([]spec.Mount, []*libpod.ContainerNa namedVolumes := make([]*libpod.ContainerNamedVolume, 0) for _, mount := range mounts { // If it's not a named volume, append unconditionally - if mount.Type != bindMount { + if mount.Type != TypeBind { newMounts = append(newMounts, mount) continue } @@ -128,7 +127,7 @@ func CreateConfigToOCISpec(config *CreateConfig) (*spec.Spec, error) { //nolint } sysMnt := spec.Mount{ Destination: "/sys", - Type: bindMount, + Type: TypeBind, Source: "/sys", Options: []string{"rprivate", "nosuid", "noexec", "nodev", r, "rbind"}, } @@ -155,7 +154,7 @@ func CreateConfigToOCISpec(config *CreateConfig) (*spec.Spec, error) { //nolint g.RemoveMount("/dev/mqueue") devMqueue := spec.Mount{ Destination: "/dev/mqueue", - Type: bindMount, + Type: TypeBind, Source: "/dev/mqueue", Options: []string{"bind", "nosuid", "noexec", "nodev"}, } @@ -165,7 +164,7 @@ func CreateConfigToOCISpec(config *CreateConfig) (*spec.Spec, error) { //nolint g.RemoveMount("/proc") procMount := spec.Mount{ Destination: "/proc", - Type: bindMount, + Type: TypeBind, Source: "/proc", Options: []string{"rbind", "nosuid", "noexec", "nodev"}, } -- cgit v1.2.3-54-g00ecf