From a760e325f3180638f9fedd0ee79d4c6695d8ba64 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Tue, 3 Sep 2019 15:03:44 -0400 Subject: Add ability for volumes with options to mount/umount When volume options and the local volume driver are specified, the volume is intended to be mounted using the 'mount' command. Supported options will be used to volume the volume before the first container using it starts, and unmount the volume after the last container using it dies. This should work for any local filesystem, though at present I've only tested with tmpfs and btrfs. Signed-off-by: Matthew Heon --- libpod/container_internal_linux.go | 49 +++++++++++++++++++++++--------------- 1 file changed, 30 insertions(+), 19 deletions(-) (limited to 'libpod/container_internal_linux.go') diff --git a/libpod/container_internal_linux.go b/libpod/container_internal_linux.go index cb19b5484..33c57729b 100644 --- a/libpod/container_internal_linux.go +++ b/libpod/container_internal_linux.go @@ -61,7 +61,7 @@ func (c *Container) unmountSHM(mount string) error { // prepare mounts the container and sets up other required resources like net // namespaces -func (c *Container) prepare() (err error) { +func (c *Container) prepare() (Err error) { var ( wg sync.WaitGroup netNS ns.NetNS @@ -108,31 +108,42 @@ func (c *Container) prepare() (err error) { logrus.Debugf("Created root filesystem for container %s at %s", c.ID(), c.state.Mountpoint) }() - defer func() { - if err != nil { - if err2 := c.cleanupNetwork(); err2 != nil { - logrus.Errorf("Error cleaning up container %s network: %v", c.ID(), err2) - } - if err2 := c.cleanupStorage(); err2 != nil { - logrus.Errorf("Error cleaning up container %s storage: %v", c.ID(), err2) - } - } - }() - wg.Wait() + var createErr error if createNetNSErr != nil { - if mountStorageErr != nil { - logrus.Error(createNetNSErr) - return mountStorageErr - } - return createNetNSErr + createErr = createNetNSErr } if mountStorageErr != nil { - return mountStorageErr + logrus.Errorf("Error preparing container %s: %v", c.ID(), createErr) + createErr = mountStorageErr + } + + // Only trigger storage cleanup if mountStorage was successful. + // Otherwise, we may mess up mount counters. + if createNetNSErr != nil && mountStorageErr == nil { + if err := c.cleanupStorage(); err != nil { + // createErr is guaranteed non-nil, so print + // unconditionally + logrus.Errorf("Error preparing container %s: %v", c.ID(), createErr) + createErr = errors.Wrapf(err, "error unmounting storage for container %s after network create failure", c.ID()) + } + } + + // It's OK to unconditionally trigger network cleanup. If the network + // isn't ready it will do nothing. + if createErr != nil { + if err := c.cleanupNetwork(); err != nil { + logrus.Errorf("Error preparing container %s: %v", c.ID(), createErr) + createErr = errors.Wrapf(err, "error cleaning up container %s network after setup failure", c.ID()) + } + } + + if createErr != nil { + return createErr } - // Save the container + // Save changes to container state return c.save() } -- cgit v1.2.3-54-g00ecf From de9a394fcff19ae4422a3f65502c8790787351fd Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Thu, 5 Sep 2019 10:00:50 -0400 Subject: Correctly report errors on unmounting SHM When we fail to remove a container's SHM, that's an error, and we need to report it as such. This may be part of our lingering storage woes. Also, remove MNT_DETACH. It may be another cause of the storage removal failures. Signed-off-by: Matthew Heon --- libpod/boltdb_state.go | 3 +-- libpod/container_internal.go | 21 +++++++++++++-------- libpod/container_internal_linux.go | 8 ++++---- libpod/define/config.go | 4 ++++ libpod/runtime_volume_linux.go | 4 ++-- libpod/volume_internal.go | 2 +- libpod/volume_internal_linux.go | 2 +- pkg/varlinkapi/volumes.go | 3 +-- 8 files changed, 27 insertions(+), 20 deletions(-) (limited to 'libpod/container_internal_linux.go') diff --git a/libpod/boltdb_state.go b/libpod/boltdb_state.go index acba61f2a..31b551b36 100644 --- a/libpod/boltdb_state.go +++ b/libpod/boltdb_state.go @@ -1403,11 +1403,10 @@ func (s *BoltState) AddVolume(volume *Volume) error { // Volume state is allowed to not exist var volStateJSON []byte if volume.state != nil { - stateJSON, err := json.Marshal(volume.state) + volStateJSON, err = json.Marshal(volume.state) if err != nil { return errors.Wrapf(err, "error marshalling volume %s state to JSON", volume.Name()) } - volStateJSON = stateJSON } db, err := s.getDBCon() diff --git a/libpod/container_internal.go b/libpod/container_internal.go index 67503f148..ffc6c11ee 100644 --- a/libpod/container_internal.go +++ b/libpod/container_internal.go @@ -1253,10 +1253,10 @@ func (c *Container) mountStorage() (_ string, Err error) { return } vol.lock.Lock() - defer vol.lock.Unlock() if err := vol.unmount(false); err != nil { logrus.Errorf("Error unmounting volume %s after error mounting container %s: %v", vol.Name(), c.ID(), err) } + vol.lock.Unlock() }() } } @@ -1281,14 +1281,19 @@ func (c *Container) cleanupStorage() error { return nil } + var cleanupErr error + for _, containerMount := range c.config.Mounts { if err := c.unmountSHM(containerMount); err != nil { - return err + if cleanupErr != nil { + logrus.Errorf("Error unmounting container %s: %v", c.ID(), cleanupErr) + } + cleanupErr = err } } if c.config.Rootfs != "" { - return nil + return cleanupErr } if err := c.unmount(false); err != nil { @@ -1298,14 +1303,14 @@ func (c *Container) cleanupStorage() error { // state if errors.Cause(err) == storage.ErrNotAContainer || errors.Cause(err) == storage.ErrContainerUnknown { logrus.Errorf("Storage for container %s has been removed", c.ID()) - return nil + } else { + if cleanupErr != nil { + logrus.Errorf("Error cleaning up container %s storage: %v", c.ID(), cleanupErr) + } + cleanupErr = err } - - return err } - var cleanupErr error - // Request an unmount of all named volumes for _, v := range c.config.NamedVolumes { vol, err := c.runtime.state.Volume(v.Name) diff --git a/libpod/container_internal_linux.go b/libpod/container_internal_linux.go index 33c57729b..a84db9aa5 100644 --- a/libpod/container_internal_linux.go +++ b/libpod/container_internal_linux.go @@ -49,12 +49,12 @@ func (c *Container) mountSHM(shmOptions string) error { } func (c *Container) unmountSHM(mount string) error { - if err := unix.Unmount(mount, unix.MNT_DETACH); err != nil { + if err := unix.Unmount(mount, 0); err != nil { if err != syscall.EINVAL { - logrus.Warnf("container %s failed to unmount %s : %v", c.ID(), mount, err) - } else { - logrus.Debugf("container %s failed to unmount %s : %v", c.ID(), mount, err) + return errors.Wrapf(err, "error unmounting container %s SHM mount %s", c.ID(), mount) } + // If it's just an EINVAL, debug logs only + logrus.Debugf("container %s failed to unmount %s : %v", c.ID(), mount, err) } return nil } diff --git a/libpod/define/config.go b/libpod/define/config.go index d8d6ccf55..c66f67feb 100644 --- a/libpod/define/config.go +++ b/libpod/define/config.go @@ -18,3 +18,7 @@ type InfoData struct { Type string Data map[string]interface{} } + +// VolumeDriverLocal is the "local" volume driver. It is managed by libpod +// itself. +const VolumeDriverLocal = "local" diff --git a/libpod/runtime_volume_linux.go b/libpod/runtime_volume_linux.go index cf8055c7b..9df93faf3 100644 --- a/libpod/runtime_volume_linux.go +++ b/libpod/runtime_volume_linux.go @@ -44,11 +44,11 @@ func (r *Runtime) newVolume(ctx context.Context, options ...VolumeCreateOption) volume.config.Name = stringid.GenerateNonCryptoID() } if volume.config.Driver == "" { - volume.config.Driver = "local" + volume.config.Driver = define.VolumeDriverLocal } volume.config.CreatedTime = time.Now() - if volume.config.Driver == "local" { + if volume.config.Driver == define.VolumeDriverLocal { logrus.Debugf("Validating options for local driver") // Validate options for key := range volume.config.Options { diff --git a/libpod/volume_internal.go b/libpod/volume_internal.go index 8c11b907f..2e886e1b0 100644 --- a/libpod/volume_internal.go +++ b/libpod/volume_internal.go @@ -26,7 +26,7 @@ func (v *Volume) teardownStorage() error { // Volumes with options set, or a filesystem type, or a device to mount need to // be mounted and unmounted. func (v *Volume) needsMount() bool { - return len(v.config.Options) > 0 && v.config.Driver == "local" + return len(v.config.Options) > 0 && v.config.Driver == define.VolumeDriverLocal } // update() updates the volume state from the DB. diff --git a/libpod/volume_internal_linux.go b/libpod/volume_internal_linux.go index 68ddb3c0d..9ae4dcf69 100644 --- a/libpod/volume_internal_linux.go +++ b/libpod/volume_internal_linux.go @@ -67,7 +67,7 @@ func (v *Volume) mount() error { errPipe, err := mountCmd.StderrPipe() if err != nil { - return errors.Wrapf(err, "getting stderr pipe") + return errors.Wrapf(err, "error getting stderr pipe for mount") } if err := mountCmd.Start(); err != nil { out, err2 := ioutil.ReadAll(errPipe) diff --git a/pkg/varlinkapi/volumes.go b/pkg/varlinkapi/volumes.go index a27950b21..6dd86d831 100644 --- a/pkg/varlinkapi/volumes.go +++ b/pkg/varlinkapi/volumes.go @@ -67,8 +67,7 @@ func (i *LibpodAPI) GetVolumes(call iopodman.VarlinkCall, args []string, all boo Labels: v.Labels(), MountPoint: v.MountPoint(), Name: v.Name(), - // TODO change types here to be correct - //Options: v.Options(), + Options: v.Options(), } volumes = append(volumes, newVol) } -- cgit v1.2.3-54-g00ecf