From 11799f4e0ec6256c65691828fb73501bda5d7eec Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Tue, 12 Mar 2019 15:53:08 -0400 Subject: Add named volumes for each container to database Signed-off-by: Matthew Heon --- libpod/container.go | 14 ++++++++++++++ libpod/options.go | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+) diff --git a/libpod/container.go b/libpod/container.go index 739406e42..b02e536c5 100644 --- a/libpod/container.go +++ b/libpod/container.go @@ -234,6 +234,8 @@ type ContainerConfig struct { // These include the SHM mount. // These must be unmounted before the container's rootfs is unmounted. Mounts []string `json:"mounts,omitempty"` + // NamedVolumes lists the named volumes to mount into the container. + NamedVolumes []*ContainerNamedVolume `json:"namedVolumes,omitempty"` // Security Config @@ -368,6 +370,18 @@ type ContainerConfig struct { HealthCheckConfig *manifest.Schema2HealthConfig `json:"healthcheck"` } +// ContainerNamedVolume is a named volume that will be mounted into the +// container. +type ContainerNamedVolume struct { + // Name is the name of the volume to mount in. + // Must resolve to a valid volume present in this Podman. + Name string `json:"volumeName"` + // Dest is the mount's destination + Dest string `json:"dest"` + // Options are fstab style mount options + Options []string `json:"options,omitempty"` +} + // ContainerStatus returns a string representation for users // of a container state func (t ContainerStatus) String() string { diff --git a/libpod/options.go b/libpod/options.go index 24f126e66..86f1747ce 100644 --- a/libpod/options.go +++ b/libpod/options.go @@ -1255,6 +1255,38 @@ func withIsInfra() CtrCreateOption { } } +// WithNamedVolumes adds the given named volumes to the container. +func WithNamedVolumes(volumes []*ContainerNamedVolume) CtrCreateOption { + return func(ctr *Container) error { + if ctr.valid { + return ErrCtrFinalized + } + + destinations := make(map[string]bool) + + for _, vol := range volumes { + // First check if libpod has the volumes + _, err := ctr.runtime.GetVolume(vol.Name) + if err != nil { + return errors.Wrapf(err, "error retrieving volume %s to add to container", vol.Name) + } + + if _, ok := destinations[vol.Dest]; ok { + return errors.Wrapf(err, "two volumes found with destination %s", vol.Dest) + } + destinations[vol.Dest] = true + + ctr.config.NamedVolumes = append(ctr.config.NamedVolumes, &ContainerNamedVolume{ + Name: vol.Name, + Dest: vol.Dest, + Options: vol.Options, + }) + } + + return nil + } +} + // Volume Creation Options // WithVolumeName sets the name of the volume. -- cgit v1.2.3-54-g00ecf From d245c6df29e0c124da25e25cd9fbc5f1095bb6f7 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Fri, 15 Mar 2019 10:01:23 -0400 Subject: Switch Libpod over to new explicit named volumes This swaps the previous handling (parse all volume mounts on the container and look for ones that might refer to named volumes) for the new, explicit named volume lists stored per-container. It also deprecates force-removing volumes that are in use. I don't know how we want to handle this yet, but leaving containers that depend on a volume that no longer exists is definitely not correct. Signed-off-by: Matthew Heon --- libpod/boltdb_state.go | 79 ++++++++++++++------------------------ libpod/boltdb_state_internal.go | 53 +++++++++++-------------- libpod/container.go | 18 ++++++++- libpod/container_internal.go | 16 -------- libpod/container_internal_linux.go | 18 +++++++++ libpod/in_memory_state.go | 32 +++------------ libpod/runtime_ctr.go | 58 ++++++++++++++-------------- libpod/runtime_volume_linux.go | 14 +++---- libpod/state.go | 4 -- 9 files changed, 126 insertions(+), 166 deletions(-) diff --git a/libpod/boltdb_state.go b/libpod/boltdb_state.go index 92a7b1538..d8cfa2bda 100644 --- a/libpod/boltdb_state.go +++ b/libpod/boltdb_state.go @@ -1358,56 +1358,6 @@ func (s *BoltState) AddVolume(volume *Volume) error { return err } -// RemoveVolCtrDep updates the container dependencies sub bucket of the given volume. -// It deletes it from the bucket when found. -// This is important when force removing a volume and we want to get rid of the dependencies. -func (s *BoltState) RemoveVolCtrDep(volume *Volume, ctrID string) error { - if ctrID == "" { - return nil - } - - if !s.valid { - return ErrDBBadConfig - } - - if !volume.valid { - return ErrVolumeRemoved - } - - volName := []byte(volume.Name()) - - db, err := s.getDBCon() - if err != nil { - return err - } - defer s.closeDBCon(db) - - err = db.Update(func(tx *bolt.Tx) error { - volBkt, err := getVolBucket(tx) - if err != nil { - return err - } - - volDB := volBkt.Bucket(volName) - if volDB == nil { - volume.valid = false - return errors.Wrapf(ErrNoSuchVolume, "no volume with name %s found in database", volume.Name()) - } - - // Make a subbucket for the containers using the volume - ctrDepsBkt := volDB.Bucket(volDependenciesBkt) - depCtrID := []byte(ctrID) - if depExists := ctrDepsBkt.Get(depCtrID); depExists != nil { - if err := ctrDepsBkt.Delete(depCtrID); err != nil { - return errors.Wrapf(err, "error deleting container dependencies %q for volume %s in ctrDependencies bucket in DB", ctrID, volume.Name()) - } - } - - return nil - }) - return err -} - // RemoveVolume removes the given volume from the state func (s *BoltState) RemoveVolume(volume *Volume) error { if !s.valid { @@ -1433,6 +1383,11 @@ func (s *BoltState) RemoveVolume(volume *Volume) error { return err } + ctrBkt, err := getCtrBucket(tx) + if err != nil { + return err + } + // Check if the volume exists volDB := volBkt.Bucket(volName) if volDB == nil { @@ -1448,6 +1403,18 @@ func (s *BoltState) RemoveVolume(volume *Volume) error { if volCtrsBkt != nil { var deps []string err = volCtrsBkt.ForEach(func(id, value []byte) error { + // Alright, this is ugly. + // But we need it to work around the change in + // volume dependency handling, to make sure that + // older Podman versions don't cause DB + // corruption. + // Look up all dependencies and see that they + // still exist before appending. + ctrExists := ctrBkt.Bucket(id) + if ctrExists == nil { + return nil + } + deps = append(deps, string(id)) return nil }) @@ -1629,6 +1596,11 @@ func (s *BoltState) VolumeInUse(volume *Volume) ([]string, error) { return err } + ctrBucket, err := getCtrBucket(tx) + if err != nil { + return err + } + volDB := volBucket.Bucket([]byte(volume.Name())) if volDB == nil { volume.valid = false @@ -1642,6 +1614,13 @@ func (s *BoltState) VolumeInUse(volume *Volume) ([]string, error) { // Iterate through and add dependencies err = dependsBkt.ForEach(func(id, value []byte) error { + // Look up all dependencies and see that they + // still exist before appending. + ctrExists := ctrBucket.Bucket(id) + if ctrExists == nil { + return nil + } + depCtrs = append(depCtrs, string(id)) return nil diff --git a/libpod/boltdb_state_internal.go b/libpod/boltdb_state_internal.go index b6a0759b1..a6900a6d3 100644 --- a/libpod/boltdb_state_internal.go +++ b/libpod/boltdb_state_internal.go @@ -564,23 +564,17 @@ func (s *BoltState) addContainer(ctr *Container, pod *Pod) error { } } - // Add container to volume dependencies bucket if container is using a named volume - if ctr.runtime.config.VolumePath == "" { - return nil - } - for _, vol := range ctr.config.Spec.Mounts { - if strings.Contains(vol.Source, ctr.runtime.config.VolumePath) { - volName := strings.Split(vol.Source[len(ctr.runtime.config.VolumePath)+1:], "/")[0] - volDB := volBkt.Bucket([]byte(volName)) - if volDB == nil { - return errors.Wrapf(ErrNoSuchVolume, "no volume with name %s found in database", volName) - } + // Add container to named volume dependencies buckets + for _, vol := range ctr.config.NamedVolumes { + volDB := volBkt.Bucket([]byte(vol.Name)) + if volDB == nil { + return errors.Wrapf(ErrNoSuchVolume, "no volume with name %s found in database when adding container %s", vol.Name, ctr.ID()) + } - ctrDepsBkt := volDB.Bucket(volDependenciesBkt) - if depExists := ctrDepsBkt.Get(ctrID); depExists == nil { - if err := ctrDepsBkt.Put(ctrID, ctrID); err != nil { - return errors.Wrapf(err, "error storing container dependencies %q for volume %s in ctrDependencies bucket in DB", ctr.ID(), volName) - } + ctrDepsBkt := volDB.Bucket(volDependenciesBkt) + if depExists := ctrDepsBkt.Get(ctrID); depExists == nil { + if err := ctrDepsBkt.Put(ctrID, ctrID); err != nil { + return errors.Wrapf(err, "error adding container %s to volume %s dependencies", ctr.ID(), vol.Name) } } } @@ -745,22 +739,19 @@ func (s *BoltState) removeContainer(ctr *Container, pod *Pod, tx *bolt.Tx) error } } - // Remove container from volume dependencies bucket if container is using a named volume - for _, vol := range ctr.config.Spec.Mounts { - if strings.Contains(vol.Source, ctr.runtime.config.VolumePath) { - volName := strings.Split(vol.Source[len(ctr.runtime.config.VolumePath)+1:], "/")[0] - - volDB := volBkt.Bucket([]byte(volName)) - if volDB == nil { - // Let's assume the volume was already deleted and continue to remove the container - continue - } + // Remove container from named volume dependencies buckets + for _, vol := range ctr.config.NamedVolumes { + volDB := volBkt.Bucket([]byte(vol.Name)) + if volDB == nil { + // Let's assume the volume was already deleted and + // continue to remove the container + continue + } - ctrDepsBkt := volDB.Bucket(volDependenciesBkt) - if depExists := ctrDepsBkt.Get(ctrID); depExists != nil { - if err := ctrDepsBkt.Delete(ctrID); err != nil { - return errors.Wrapf(err, "error deleting container dependencies %q for volume %s in ctrDependencies bucket in DB", ctr.ID(), volName) - } + ctrDepsBkt := volDB.Bucket(volDependenciesBkt) + if depExists := ctrDepsBkt.Get(ctrID); depExists == nil { + if err := ctrDepsBkt.Delete(ctrID); err != nil { + return errors.Wrapf(err, "error deleting container %s dependency on volume %s", ctr.ID(), vol.Name) } } } diff --git a/libpod/container.go b/libpod/container.go index b02e536c5..74dc5be5c 100644 --- a/libpod/container.go +++ b/libpod/container.go @@ -371,7 +371,7 @@ type ContainerConfig struct { } // ContainerNamedVolume is a named volume that will be mounted into the -// container. +// container. Each named volume is a libpod Volume present in the state. type ContainerNamedVolume struct { // Name is the name of the volume to mount in. // Must resolve to a valid volume present in this Podman. @@ -502,6 +502,22 @@ func (c *Container) StaticDir() string { return c.config.StaticDir } +// NamedVolumes returns the container's named volumes. +// The name of each is guaranteed to point to a valid libpod Volume present in +// the state. +func (c *Container) NamedVolumes() []*ContainerNamedVolume { + volumes := []*ContainerNamedVolume{} + for _, vol := range c.config.NamedVolumes { + newVol := new(ContainerNamedVolume) + newVol.Name = vol.Name + newVol.Dest = vol.Dest + newVol.Options = vol.Options + volumes = append(volumes, newVol) + } + + return volumes +} + // Privileged returns whether the container is privileged func (c *Container) Privileged() bool { return c.config.Privileged diff --git a/libpod/container_internal.go b/libpod/container_internal.go index daa32007a..22df36c11 100644 --- a/libpod/container_internal.go +++ b/libpod/container_internal.go @@ -1403,22 +1403,6 @@ func getExcludedCGroups() (excludes []string) { return } -// namedVolumes returns named volumes for the container -func (c *Container) namedVolumes() ([]string, error) { - var volumes []string - for _, vol := range c.config.Spec.Mounts { - if strings.HasPrefix(vol.Source, c.runtime.config.VolumePath) { - volume := strings.TrimPrefix(vol.Source, c.runtime.config.VolumePath+"/") - split := strings.Split(volume, "/") - volume = split[0] - if _, err := c.runtime.state.Volume(volume); err == nil { - volumes = append(volumes, volume) - } - } - } - return volumes, nil -} - // this should be from chrootarchive. func (c *Container) copyWithTarFromImage(src, dest string) error { mountpoint, err := c.mount() diff --git a/libpod/container_internal_linux.go b/libpod/container_internal_linux.go index 504d6c135..4d6bf61a3 100644 --- a/libpod/container_internal_linux.go +++ b/libpod/container_internal_linux.go @@ -195,6 +195,7 @@ func (c *Container) generateSpec(ctx context.Context) (*spec.Spec, error) { if err := c.makeBindMounts(); err != nil { return nil, err } + // Check if the spec file mounts contain the label Relabel flags z or Z. // If they do, relabel the source directory and then remove the option. for i := range g.Config.Mounts { @@ -218,6 +219,23 @@ func (c *Container) generateSpec(ctx context.Context) (*spec.Spec, error) { g.SetProcessSelinuxLabel(c.ProcessLabel()) g.SetLinuxMountLabel(c.MountLabel()) + + // Add named volumes + for _, namedVol := range c.config.NamedVolumes { + volume, err := c.runtime.GetVolume(namedVol.Name) + if err != nil { + return nil, errors.Wrapf(err, "error retrieving volume %s to add to container %s", namedVol.Name, c.ID()) + } + mountPoint := volume.MountPoint() + volMount := spec.Mount{ + Type: "bind", + Source: mountPoint, + Destination: namedVol.Dest, + Options: namedVol.Options, + } + g.AddMount(volMount) + } + // Add bind mounts to container for dstPath, srcPath := range c.state.BindMounts { newMount := spec.Mount{ diff --git a/libpod/in_memory_state.go b/libpod/in_memory_state.go index ab4fc8ba7..2669206df 100644 --- a/libpod/in_memory_state.go +++ b/libpod/in_memory_state.go @@ -249,11 +249,8 @@ func (s *InMemoryState) AddContainer(ctr *Container) error { } // Add container to volume dependencies - for _, vol := range ctr.config.Spec.Mounts { - if strings.Contains(vol.Source, ctr.runtime.config.VolumePath) { - volName := strings.Split(vol.Source[len(ctr.runtime.config.VolumePath)+1:], "/")[0] - s.addCtrToVolDependsMap(ctr.ID(), volName) - } + for _, vol := range ctr.config.NamedVolumes { + s.addCtrToVolDependsMap(ctr.ID(), vol.Name) } return nil @@ -306,12 +303,9 @@ func (s *InMemoryState) RemoveContainer(ctr *Container) error { s.removeCtrFromDependsMap(ctr.ID(), depCtr) } - // Remove container from volume dependencies - for _, vol := range ctr.config.Spec.Mounts { - if strings.Contains(vol.Source, ctr.runtime.config.VolumePath) { - volName := strings.Split(vol.Source[len(ctr.runtime.config.VolumePath)+1:], "/")[0] - s.removeCtrFromVolDependsMap(ctr.ID(), volName) - } + // Remove this container from volume dependencies + for _, vol := range ctr.config.NamedVolumes { + s.removeCtrFromVolDependsMap(ctr.ID(), vol.Name) } return nil @@ -492,22 +486,6 @@ func (s *InMemoryState) RemoveVolume(volume *Volume) error { return nil } -// RemoveVolCtrDep updates the container dependencies of the volume -func (s *InMemoryState) RemoveVolCtrDep(volume *Volume, ctrID string) error { - if !volume.valid { - return errors.Wrapf(ErrVolumeRemoved, "volume with name %s is not valid", volume.Name()) - } - - if _, ok := s.volumes[volume.Name()]; !ok { - return errors.Wrapf(ErrNoSuchVolume, "volume with name %s doesn't exists in state", volume.Name()) - } - - // Remove container that is using this volume - s.removeCtrFromVolDependsMap(ctrID, volume.Name()) - - return nil -} - // VolumeInUse checks if the given volume is being used by at least one container func (s *InMemoryState) VolumeInUse(volume *Volume) ([]string, error) { if !volume.valid { diff --git a/libpod/runtime_ctr.go b/libpod/runtime_ctr.go index da2399685..ec43d5dcb 100644 --- a/libpod/runtime_ctr.go +++ b/libpod/runtime_ctr.go @@ -173,26 +173,29 @@ func (r *Runtime) newContainer(ctx context.Context, rSpec *spec.Spec, options .. ctr.config.ConmonPidFile = filepath.Join(ctr.config.StaticDir, "conmon.pid") } - // Go through the volume mounts and check for named volumes - // If the named volme already exists continue, otherwise create - // the storage for the named volume. - for i, vol := range ctr.config.Spec.Mounts { - if vol.Source[0] != '/' && isNamedVolume(vol.Source) { - volInfo, err := r.state.Volume(vol.Source) - if err != nil { - newVol, err := r.newVolume(ctx, WithVolumeName(vol.Source), withSetCtrSpecific(), WithVolumeUID(ctr.RootUID()), WithVolumeGID(ctr.RootGID())) - if err != nil { - return nil, errors.Wrapf(err, "error creating named volume %q", vol.Source) - } - ctr.config.Spec.Mounts[i].Source = newVol.MountPoint() - if err := ctr.copyWithTarFromImage(ctr.config.Spec.Mounts[i].Destination, ctr.config.Spec.Mounts[i].Source); err != nil && !os.IsNotExist(err) { - return nil, errors.Wrapf(err, "failed to copy content into new volume mount %q", vol.Source) - } - continue - } - ctr.config.Spec.Mounts[i].Source = volInfo.MountPoint() - } - } + // // Go through the volume mounts and check for named volumes + // // If the named volme already exists continue, otherwise create + // // the storage for the named volume. + // for i, vol := range ctr.config.Spec.Mounts { + // if vol.Source[0] != '/' && isNamedVolume(vol.Source) { + // volInfo, err := r.state.Volume(vol.Source) + // if err != nil { + // newVol, err := r.newVolume(ctx, WithVolumeName(vol.Source), withSetCtrSpecific()) + // if err != nil { + // return nil, errors.Wrapf(err, "error creating named volume %q", vol.Source) + // } + // ctr.config.Spec.Mounts[i].Source = newVol.MountPoint() + // if err := os.Chown(ctr.config.Spec.Mounts[i].Source, ctr.RootUID(), ctr.RootGID()); err != nil { + // return nil, errors.Wrapf(err, "cannot chown %q to %d:%d", ctr.config.Spec.Mounts[i].Source, ctr.RootUID(), ctr.RootGID()) + // } + // if err := ctr.copyWithTarFromImage(ctr.config.Spec.Mounts[i].Destination, ctr.config.Spec.Mounts[i].Source); err != nil && !os.IsNotExist(err) { + // return nil, errors.Wrapf(err, "Failed to copy content into new volume mount %q", vol.Source) + // } + // continue + // } + // ctr.config.Spec.Mounts[i].Source = volInfo.MountPoint() + // } + // } if ctr.config.LogPath == "" { ctr.config.LogPath = filepath.Join(ctr.config.StaticDir, "ctr.log") @@ -344,13 +347,6 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, force bool, return errors.Wrapf(ErrCtrExists, "container %s has dependent containers which must be removed before it: %s", c.ID(), depsStr) } - var volumes []string - if removeVolume { - volumes, err = c.namedVolumes() - if err != nil { - logrus.Errorf("unable to retrieve builtin volumes for container %v: %v", c.ID(), err) - } - } var cleanupErr error // Remove the container from the state if c.config.Pod != "" { @@ -415,8 +411,12 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, force bool, } } - for _, v := range volumes { - if volume, err := runtime.state.Volume(v); err == nil { + if !removeVolume { + return cleanupErr + } + + for _, v := range c.config.NamedVolumes { + if volume, err := runtime.state.Volume(v.Name); err == nil { if !volume.IsCtrSpecific() { continue } diff --git a/libpod/runtime_volume_linux.go b/libpod/runtime_volume_linux.go index db5c29242..39c2f808d 100644 --- a/libpod/runtime_volume_linux.go +++ b/libpod/runtime_volume_linux.go @@ -98,14 +98,12 @@ func (r *Runtime) removeVolume(ctx context.Context, v *Volume, force bool) error if !force { return errors.Wrapf(ErrVolumeBeingUsed, "volume %s is being used by the following container(s): %s", v.Name(), depsStr) } - // If using force, log the warning that the volume is being used by at least one container - logrus.Warnf("volume %s is being used by the following container(s): %s", v.Name(), depsStr) - // Remove the container dependencies so we can go ahead and delete the volume - for _, dep := range deps { - if err := r.state.RemoveVolCtrDep(v, dep); err != nil { - return errors.Wrapf(err, "unable to remove container dependency %q from volume %q while trying to delete volume by force", dep, v.Name()) - } - } + + // TODO: force-removing a volume makes *no sense* + // I do not believe we should be allowing this at all. + // For now, return angry errors. + // TODO: need to do something more sane in this case + return errors.Wrapf(ErrVolumeBeingUsed, "TODO: FIXME - still refusing to remove because force-removing an in-use volume is not good.") } // Set volume as invalid so it can no longer be used diff --git a/libpod/state.go b/libpod/state.go index 4296fc3cd..d0ad1a1f8 100644 --- a/libpod/state.go +++ b/libpod/state.go @@ -192,10 +192,6 @@ type State interface { // AddVolume adds the specified volume to state. The volume's name // must be unique within the list of existing volumes AddVolume(volume *Volume) error - // RemoveVolCtrDep updates the list of container dependencies that the - // volume has. It either deletes the dependent container ID from - // the sub-bucket - RemoveVolCtrDep(volume *Volume, ctrID string) error // RemoveVolume removes the specified volume. // Only volumes that have no container dependencies can be removed RemoveVolume(volume *Volume) error -- cgit v1.2.3-54-g00ecf From ee770ad5b54845e345384be3e01c700e93926b44 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Fri, 22 Mar 2019 14:23:18 -0400 Subject: Create non-existing named volumes at container create Replaces old functionality we used for handling image volumes. Signed-off-by: Matthew Heon --- libpod/options.go | 53 ++++++++++++++++++++++++++------------------------- libpod/runtime_ctr.go | 49 ++++++++++++++++++++++------------------------- 2 files changed, 50 insertions(+), 52 deletions(-) diff --git a/libpod/options.go b/libpod/options.go index 86f1747ce..4f35a5c8c 100644 --- a/libpod/options.go +++ b/libpod/options.go @@ -1265,14 +1265,11 @@ func WithNamedVolumes(volumes []*ContainerNamedVolume) CtrCreateOption { destinations := make(map[string]bool) for _, vol := range volumes { - // First check if libpod has the volumes - _, err := ctr.runtime.GetVolume(vol.Name) - if err != nil { - return errors.Wrapf(err, "error retrieving volume %s to add to container", vol.Name) - } + // Don't check if they already exist. + // If they don't we will automatically create them. if _, ok := destinations[vol.Dest]; ok { - return errors.Wrapf(err, "two volumes found with destination %s", vol.Dest) + return errors.Wrapf(ErrInvalidArg, "two volumes found with destination %s", vol.Dest) } destinations[vol.Dest] = true @@ -1306,68 +1303,72 @@ func WithVolumeName(name string) VolumeCreateOption { } } -// WithVolumeUID sets the uid of the owner. -func WithVolumeUID(uid int) VolumeCreateOption { +// WithVolumeLabels sets the labels of the volume. +func WithVolumeLabels(labels map[string]string) VolumeCreateOption { return func(volume *Volume) error { if volume.valid { return ErrVolumeFinalized } - volume.config.UID = uid + + volume.config.Labels = make(map[string]string) + for key, value := range labels { + volume.config.Labels[key] = value + } + return nil } } -// WithVolumeGID sets the gid of the owner. -func WithVolumeGID(gid int) VolumeCreateOption { +// WithVolumeDriver sets the driver of the volume. +func WithVolumeDriver(driver string) VolumeCreateOption { return func(volume *Volume) error { if volume.valid { return ErrVolumeFinalized } - volume.config.GID = gid + + volume.config.Driver = driver + return nil } } -// WithVolumeLabels sets the labels of the volume. -func WithVolumeLabels(labels map[string]string) VolumeCreateOption { +// WithVolumeOptions sets the options of the volume. +func WithVolumeOptions(options map[string]string) VolumeCreateOption { return func(volume *Volume) error { if volume.valid { return ErrVolumeFinalized } - volume.config.Labels = make(map[string]string) - for key, value := range labels { - volume.config.Labels[key] = value + volume.config.Options = make(map[string]string) + for key, value := range options { + volume.config.Options[key] = value } return nil } } -// WithVolumeDriver sets the driver of the volume. -func WithVolumeDriver(driver string) VolumeCreateOption { +// WithVolumeUID sets the UID that the volume will be created as. +func WithVolumeUID(uid int) VolumeCreateOption { return func(volume *Volume) error { if volume.valid { return ErrVolumeFinalized } - volume.config.Driver = driver + volume.config.UID = uid return nil } } -// WithVolumeOptions sets the options of the volume. -func WithVolumeOptions(options map[string]string) VolumeCreateOption { +// WithVolumeGID sets the GID that the volume will be created as. +func WithVolumeGID(gid int) VolumeCreateOption { return func(volume *Volume) error { if volume.valid { return ErrVolumeFinalized } - volume.config.Options = make(map[string]string) - for key, value := range options { - volume.config.Options[key] = value - } + volume.config.GID = gid return nil } diff --git a/libpod/runtime_ctr.go b/libpod/runtime_ctr.go index ec43d5dcb..fafbc9969 100644 --- a/libpod/runtime_ctr.go +++ b/libpod/runtime_ctr.go @@ -99,9 +99,6 @@ func (r *Runtime) newContainer(ctx context.Context, rSpec *spec.Spec, options .. ctr.state.State = ContainerStateConfigured ctr.runtime = r - ctr.valid = true - ctr.state.State = ContainerStateConfigured - var pod *Pod if ctr.config.Pod != "" { // Get the pod from state @@ -173,29 +170,29 @@ func (r *Runtime) newContainer(ctx context.Context, rSpec *spec.Spec, options .. ctr.config.ConmonPidFile = filepath.Join(ctr.config.StaticDir, "conmon.pid") } - // // Go through the volume mounts and check for named volumes - // // If the named volme already exists continue, otherwise create - // // the storage for the named volume. - // for i, vol := range ctr.config.Spec.Mounts { - // if vol.Source[0] != '/' && isNamedVolume(vol.Source) { - // volInfo, err := r.state.Volume(vol.Source) - // if err != nil { - // newVol, err := r.newVolume(ctx, WithVolumeName(vol.Source), withSetCtrSpecific()) - // if err != nil { - // return nil, errors.Wrapf(err, "error creating named volume %q", vol.Source) - // } - // ctr.config.Spec.Mounts[i].Source = newVol.MountPoint() - // if err := os.Chown(ctr.config.Spec.Mounts[i].Source, ctr.RootUID(), ctr.RootGID()); err != nil { - // return nil, errors.Wrapf(err, "cannot chown %q to %d:%d", ctr.config.Spec.Mounts[i].Source, ctr.RootUID(), ctr.RootGID()) - // } - // if err := ctr.copyWithTarFromImage(ctr.config.Spec.Mounts[i].Destination, ctr.config.Spec.Mounts[i].Source); err != nil && !os.IsNotExist(err) { - // return nil, errors.Wrapf(err, "Failed to copy content into new volume mount %q", vol.Source) - // } - // continue - // } - // ctr.config.Spec.Mounts[i].Source = volInfo.MountPoint() - // } - // } + // Go through named volumes and add them. + // If they don't exist they will be created using basic options. + for _, vol := range ctr.config.NamedVolumes { + // Check if it exists already + _, err := r.state.Volume(vol.Name) + if err == nil { + // The volume exists, we're good + continue + } else if errors.Cause(err) != ErrNoSuchVolume { + return nil, errors.Wrapf(err, "error retrieving named volume %s for new 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())) + if err != nil { + return nil, errors.Wrapf(err, "error creating named volume %q", vol.Name) + } + + if err := ctr.copyWithTarFromImage(vol.Dest, newVol.MountPoint()); err != nil && !os.IsNotExist(err) { + return nil, errors.Wrapf(err, "Failed to copy content into new volume mount %q", vol.Name) + } + } if ctr.config.LogPath == "" { ctr.config.LogPath = filepath.Join(ctr.config.StaticDir, "ctr.log") -- cgit v1.2.3-54-g00ecf 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 --- libpod/runtime_ctr.go | 8 -------- pkg/spec/createconfig.go | 17 +++++++++++------ pkg/spec/spec.go | 41 ++++++++++++++++++++++++++++++++++++++--- 3 files changed, 49 insertions(+), 17 deletions(-) diff --git a/libpod/runtime_ctr.go b/libpod/runtime_ctr.go index fafbc9969..ba643150d 100644 --- a/libpod/runtime_ctr.go +++ b/libpod/runtime_ctr.go @@ -545,14 +545,6 @@ func (r *Runtime) GetLatestContainer() (*Container, error) { return ctrs[lastCreatedIndex], nil } -// Check if volName is a named volume and not one of the default mounts we add to containers -func isNamedVolume(volName string) bool { - if volName != "proc" && volName != "tmpfs" && volName != "devpts" && volName != "shm" && volName != "mqueue" && volName != "sysfs" && volName != "cgroup" { - return true - } - return false -} - // Export is the libpod portion of exporting a container to a tar file func (r *Runtime) Export(name string, path string) error { ctr, err := r.LookupContainer(name) 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 3e066e29202118e6cca60be516ddc08cc7a9060e Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Fri, 29 Mar 2019 10:20:16 -0400 Subject: Volume force-remove now removed dependent containers Signed-off-by: Matthew Heon --- docs/podman-volume-rm.1.md | 3 ++- libpod/runtime_volume_linux.go | 26 +++++++++++++++++++++----- 2 files changed, 23 insertions(+), 6 deletions(-) diff --git a/docs/podman-volume-rm.1.md b/docs/podman-volume-rm.1.md index c23d7675c..8c3765235 100644 --- a/docs/podman-volume-rm.1.md +++ b/docs/podman-volume-rm.1.md @@ -21,7 +21,8 @@ Remove all volumes. **-f**, **--force**="" -Remove a volume by force, even if it is being used by a container +Remove a volume by force. +If it is being used by containers, the containers will be removed first. **--help** diff --git a/libpod/runtime_volume_linux.go b/libpod/runtime_volume_linux.go index 39c2f808d..dcefcde4e 100644 --- a/libpod/runtime_volume_linux.go +++ b/libpod/runtime_volume_linux.go @@ -99,11 +99,27 @@ func (r *Runtime) removeVolume(ctx context.Context, v *Volume, force bool) error return errors.Wrapf(ErrVolumeBeingUsed, "volume %s is being used by the following container(s): %s", v.Name(), depsStr) } - // TODO: force-removing a volume makes *no sense* - // I do not believe we should be allowing this at all. - // For now, return angry errors. - // TODO: need to do something more sane in this case - return errors.Wrapf(ErrVolumeBeingUsed, "TODO: FIXME - still refusing to remove because force-removing an in-use volume is not good.") + // We need to remove all containers using the volume + for _, dep := range deps { + ctr, err := r.state.Container(dep) + if err != nil { + // If the container's removed, no point in + // erroring. + if errors.Cause(err) == ErrNoSuchCtr || errors.Cause(err) == ErrCtrRemoved { + continue + } + + return errors.Wrapf(err, "error removing container %s that depends on volume %s", dep, v.Name()) + } + + // TODO: do we want to set force here when removing + // containers? + // I'm inclined to say no, in case someone accidentally + // wipes a container they're using... + if err := r.removeContainer(ctx, ctr, false, true); err != nil { + return errors.Wrapf(err, "error removing container %s that depends on volume %s", ctr.ID(), v.Name()) + } + } } // Set volume as invalid so it can no longer be used -- 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(-) 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(-) 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 From 02c6110093ed23dd637a51611b0bce4fd4ab9ce9 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Thu, 4 Apr 2019 11:48:23 -0400 Subject: Fix E2E tests The Commit test is blatantly wrong and testing buggy behavior. We should be commiting the destination, if anything - and more likely nothing at all. When force-removing volumes, don't remove the volumes of containers we need to remove. This can lead to a chicken and the egg problem where the container removes the volume before we can. When we re-add volume locks this could lead to deadlocks. I don't really want to deal with this, and this doesn't seem a particularly harmful quirk, so we'll let this slide until we get a bug report. Signed-off-by: Matthew Heon --- libpod/runtime_volume_linux.go | 2 +- test/e2e/commit_test.go | 2 +- test/e2e/volume_rm_test.go | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/libpod/runtime_volume_linux.go b/libpod/runtime_volume_linux.go index dcefcde4e..40040fc52 100644 --- a/libpod/runtime_volume_linux.go +++ b/libpod/runtime_volume_linux.go @@ -116,7 +116,7 @@ func (r *Runtime) removeVolume(ctx context.Context, v *Volume, force bool) error // containers? // I'm inclined to say no, in case someone accidentally // wipes a container they're using... - if err := r.removeContainer(ctx, ctr, false, true); err != nil { + if err := r.removeContainer(ctx, ctr, false, false); err != nil { return errors.Wrapf(err, "error removing container %s that depends on volume %s", ctr.ID(), v.Name()) } } diff --git a/test/e2e/commit_test.go b/test/e2e/commit_test.go index bf9c88de5..fe4ae64cf 100644 --- a/test/e2e/commit_test.go +++ b/test/e2e/commit_test.go @@ -144,7 +144,7 @@ var _ = Describe("Podman commit", func() { inspect.WaitWithDefaultTimeout() Expect(inspect.ExitCode()).To(Equal(0)) image := inspect.InspectImageJSON() - _, ok := image[0].Config.Volumes["/tmp"] + _, ok := image[0].Config.Volumes["/foo"] Expect(ok).To(BeTrue()) r := podmanTest.Podman([]string{"run", "newimage"}) diff --git a/test/e2e/volume_rm_test.go b/test/e2e/volume_rm_test.go index 888474670..39628d56f 100644 --- a/test/e2e/volume_rm_test.go +++ b/test/e2e/volume_rm_test.go @@ -32,7 +32,7 @@ var _ = Describe("Podman volume rm", func() { }) - It("podman rm volume", func() { + It("podman volume rm", func() { session := podmanTest.Podman([]string{"volume", "create", "myvol"}) session.WaitWithDefaultTimeout() Expect(session.ExitCode()).To(Equal(0)) @@ -47,7 +47,7 @@ var _ = Describe("Podman volume rm", func() { Expect(len(session.OutputToStringArray())).To(Equal(0)) }) - It("podman rm with --force flag", func() { + It("podman volume rm with --force flag", func() { SkipIfRemote() session := podmanTest.Podman([]string{"create", "-v", "myvol:/myvol", ALPINE, "ls"}) cid := session.OutputToString() -- cgit v1.2.3-54-g00ecf