fix umount cleanup (#27)

* fix umount cleanup with new metadata location

Since eaa7b43, Umount was not cleaning up backing devices correctly, but
this was not caught in tests. Stacker's test suite had a test that
noticed.

It isn't clear that Umount as used in the `atomfs` cli tool was correct
before eaa7b43 either, because the underlying atom mount points were put
under the overmounted final mount point, and would thus not be shared by
other molecule mounts. This also wasn't being tested though.

This commit fixes Umount, and:
- consolidates all unmounting logic - out of `cmd/umount` and into the
  package in `atomfs.Umount`.
- adds a version of the test from Stacker that noticed this - that test
  belongs here instead of in stacker. Also fixed a couple seeming typos
  in that test that hid failures.
- fixes the lock path used in Umount to match that used in Mount, which
  was out of sync since eaa7b43
- splits out the verity.go squashfs.Umount code to allow for getting
  backing devices of an atom without unmounting it, in order to only
  clean up the backing dev if it is no longer in use

Signed-off-by: Michael McCracken <[email protected]>

* test: extend coverage tracking to bats tests

- updates codecov action version
- make batstest now builds with coverage and runs all bats tests with
  coverage, then merges into a file codecov knows about

Signed-off-by: Michael McCracken <[email protected]>

---------

Signed-off-by: Michael McCracken <[email protected]>

Author: mikemccracken

.github/workflows/build.yaml

@@ -67,10 +67,10 @@ jobs:
         run: |
           make batstest
       - name: Upload code coverage
-        uses: codecov/codecov-action@v4
+        uses: codecov/codecov-action@v5
         with:
           fail_ci_if_error: true # optional (default = false)
-          files: ./coverage.txt
+          files: ./unit-coverage.txt,./integ-coverage.txt
           token: ${{ secrets.CODECOV_TOKEN }} # required
       - name: Release
         uses: softprops/action-gh-release@v1

Makefile

@@ -14,6 +14,7 @@ BATS_VERSION := v1.10.0
 STACKER = $(TOOLS_D)/bin/stacker
 STACKER_VERSION := v1.0.0
 TOOLS_D := $(ROOT)/tools
+GOCOVERDIR ?= $(ROOT)
 
 export PATH := $(TOOLS_D)/bin:$(PATH)
 
@@ -25,11 +26,13 @@ gofmt: .made-gofmt
 		{ echo "gofmt made changes: $$o" 1>&2; exit 1; }
 	@touch $@
 
-atomfs: .made-gofmt $(GO_SRC)
-	cd $(ROOT)/cmd/atomfs && go build -buildvcs=false -ldflags "$(VERSION_LDFLAGS)" -o $(ROOT)/bin/atomfs ./...
+atomfs atomfs-cover: .made-gofmt $(GO_SRC)
+	cd $(ROOT)/cmd/atomfs && go build $(BUILDCOVERFLAGS) -buildvcs=false -ldflags "$(VERSION_LDFLAGS)" -o $(ROOT)/bin/$@ ./...
+
+atomfs-cover: BUILDCOVERFLAGS=-cover
 
 gotest: $(GO_SRC)
-	go test -coverprofile=coverage.txt -ldflags "$(VERSION_LDFLAGS)"  ./...
+	go test -coverprofile=unit-coverage.txt -ldflags "$(VERSION_LDFLAGS)"  ./...
 
 $(STACKER):
 	mkdir -p $(TOOLS_D)/bin
@@ -46,9 +49,21 @@ $(BATS):
 	git clone --depth 1 https://github.com/bats-core/bats-assert $(ROOT)/test/test_helper/bats-assert
 	git clone --depth 1 https://github.com/bats-core/bats-file $(ROOT)/test/test_helper/bats-file
 
-batstest: $(BATS) $(STACKER) atomfs test/random.txt
-	cd $(ROOT)/test; sudo $(BATS) --tap --timing priv-*.bats
-	cd $(ROOT)/test; $(BATS) --tap --timing unpriv-*.bats
+batstest: $(BATS) $(STACKER) atomfs-cover test/random.txt testimages
+	cd $(ROOT)/test; sudo GOCOVERDIR=$(GOCOVERDIR) $(BATS) --tap --timing priv-*.bats
+	cd $(ROOT)/test; GOCOVERDIR=$(GOCOVERDIR) $(BATS) --tap --timing unpriv-*.bats
+	go tool covdata textfmt -i $(GOCOVERDIR) -o integ-coverage.txt
+
+covreport: gotest batstest
+	go tool cover -func=unit-coverage.txt
+	go tool cover -func=integ-coverage.txt
+
+testimages: /tmp/atomfs-test-oci/.copydone
+	@echo "busybox image exists at /tmp/atomfs-test-oci already"
+
+/tmp/atomfs-test-oci/.copydone:
+	skopeo copy docker://public.ecr.aws/docker/library/busybox:stable oci:/tmp/atomfs-test-oci:busybox
+	touch $@
 
 test/random.txt:
 	dd if=/dev/random of=/dev/stdout count=2048 | base64 > test/random.txt

cmd/atomfs/umount.go

@@ -2,9 +2,7 @@ package main
 
 import (
 	"fmt"
-	"os"
 	"path/filepath"
-	"syscall"
 
 	"github.com/urfave/cli"
 	"machinerun.io/atomfs"
@@ -39,9 +37,7 @@ func doUmount(ctx *cli.Context) error {
 	}
 
 	mountpoint := ctx.Args()[0]
-
 	var err error
-	var errs []error
 
 	if !filepath.IsAbs(mountpoint) {
 		mountpoint, err = filepath.Abs(mountpoint)
@@ -50,53 +46,5 @@ func doUmount(ctx *cli.Context) error {
 		}
 	}
 
-	// We expect the argument to be the mountpoint of the overlay
-	err = syscall.Unmount(mountpoint, 0)
-	if err != nil {
-		errs = append(errs, fmt.Errorf("Failed unmounting %s: %v", mountpoint, err))
-	}
-
-	// We expect the following in the metadir
-	//
-	// $metadir/mounts/* - the original squashfs mounts
-	// $metadir/meta/config.json
-
-	// TODO: want to know mountnsname for a target mountpoint... not for our current proc???
-	mountNSName, err := atomfs.GetMountNSName()
-	if err != nil {
-		errs = append(errs, fmt.Errorf("Failed to get mount namespace name"))
-	}
-	metadir := filepath.Join(atomfs.RuntimeDir(ctx.String("metadir")), "meta", mountNSName, atomfs.ReplacePathSeparators(mountpoint))
-
-	mountsdir := filepath.Join(metadir, "mounts")
-	mounts, err := os.ReadDir(mountsdir)
-	if err != nil {
-		errs = append(errs, fmt.Errorf("Failed reading list of mounts: %v", err))
-		return fmt.Errorf("Encountered errors: %v", errs)
-	}
-
-	for _, m := range mounts {
-		p := filepath.Join(mountsdir, m.Name())
-		if !m.IsDir() || !isMountpoint(p) {
-			continue
-		}
-
-		err = syscall.Unmount(p, 0)
-		if err != nil {
-			errs = append(errs, fmt.Errorf("Failed unmounting squashfs dir %s: %v", p, err))
-		}
-	}
-
-	if len(errs) != 0 {
-		for i, e := range errs {
-			fmt.Printf("Error %d: %v\n", i, e)
-		}
-		return fmt.Errorf("Encountered errors %d: %v", len(errs), errs)
-	}
-
-	if err := os.RemoveAll(metadir); err != nil {
-		return fmt.Errorf("Failed removing %q: %v", metadir, err)
-	}
-
-	return nil
+	return atomfs.UmountWithMetadir(mountpoint, ctx.String("metadir"))
 }

molecule.go

@@ -287,14 +287,32 @@ func (m Molecule) Mount(dest string) error {
 	return nil
 }
 
+// Default Umount passes "" and uses /run/atomfs metadir, see RuntimeDir().
 func Umount(dest string) error {
+	return UmountWithMetadir(dest, "")
+}
+
+func UmountWithMetadir(dest, metadirArg string) error {
 	var err error
 	dest, err = filepath.Abs(dest)
 	if err != nil {
 		return errors.Wrapf(err, "couldn't create abs path for %v", dest)
 	}
 
-	lockfile, err := makeLock(dest)
+	// recreate molecule config as much as possible, for MetadataPath():
+	mol := Molecule{
+		config: MountOCIOpts{
+			Target:      dest,
+			MetadataDir: metadirArg,
+		},
+	}
+
+	metadir, err := mol.MetadataPath()
+	if err != nil {
+		return errors.WithStack(err)
+	}
+
+	lockfile, err := makeLock(metadir)
 	if err != nil {
 		return errors.WithStack(err)
 	}
@@ -310,77 +328,93 @@ func Umount(dest string) error {
 		return err
 	}
 
-	underlyingAtoms := []string{}
+	// Find all mountpoints underlying the current top Overlay MP
+	underlyingAtomRelPaths := []string{}
 	for _, m := range mounts {
 		if m.FSType != "overlay" {
 			continue
 		}
 
-		if m.Target != dest { // TODO is this still right
+		if m.Target != dest {
 			continue
 		}
 
-		underlyingAtoms, err = m.GetOverlayDirs()
+		underlyingAtomRelPaths, err = m.GetOverlayDirs()
 		if err != nil {
 			return err
 		}
 		break
 	}
 
+	underlyingAtoms := []string{}
+	// Ensure abs paths, as we compare it to the abs path of dest
+	for _, m := range underlyingAtomRelPaths {
+		abspath, err := filepath.Abs(m)
+		if err != nil {
+			return errors.Wrapf(err, "failed to get abs path for %q", m)
+		}
+		underlyingAtoms = append(underlyingAtoms, abspath)
+	}
+
 	if len(underlyingAtoms) == 0 {
 		return errors.Errorf("%s is not an atomfs mountpoint", dest)
 	}
 
+	// Unmount the top Overlay MP
 	if err := unix.Unmount(dest, 0); err != nil {
-		return err
+		return errors.Wrapf(err, "failed to unmount dest, %q", dest)
 	}
 
-	// now, "refcount" the remaining atoms and see if any of ours are
-	// unused
-	usedAtoms := map[string]int{}
-
-	mounts, err = mount.ParseMounts("/proc/self/mountinfo")
-	if err != nil {
-		return err
-	}
+	// For each underlying dir, we need to find the corresponding
+	// squashfs-verity device mounted on it, then unconditionally unmount the
+	// underlying dir (because its mount point is specific to `dest`) - then
+	// find the underlying device and optionally clean it up too
+	for _, a := range underlyingAtoms {
+		// the workaround dir isn't really a mountpoint, so don't unmount it
+		if path.Base(a) == "workaround" {
+			continue
+		}
 
-	for _, m := range mounts {
-		if m.FSType != "overlay" {
+		// overlaydirs includes the top level mount, ignore it
+		if a == dest {
 			continue
 		}
 
-		dirs, err := m.GetOverlayDirs()
+		backingDevice, err := squashfs.GetBackingDevice(a)
 		if err != nil {
 			return err
 		}
-		for _, d := range dirs {
-			usedAtoms[d]++
+		log.Debugf("Unmounting underlying atom =%q", a)
+		if err := unix.Unmount(a, 0); err != nil {
+			return err
 		}
-	}
 
-	// If any of the atoms underlying the target mountpoint are now unused,
-	// let's unmount them too.
-	for _, a := range underlyingAtoms {
-		_, used := usedAtoms[a]
-		if used {
-			continue
+		// if that was the last mountpoint for the dev, we can clean it up too
+		mounts, err = mount.ParseMounts("/proc/self/mountinfo")
+		if err != nil {
+			return err
 		}
-		/* TODO: some kind of logging
-		if !used {
-			log.Warnf("unused atom %s was part of this molecule?")
-			continue
+		backingDevIsUsed := false
+		for _, m := range mounts {
+			if m.Source == backingDevice {
+				backingDevIsUsed = true
+			}
 		}
-		*/
 
-		// the workaround dir isn't really a mountpoint, so don't unmount it
-		if path.Base(a) == "workaround" {
-			continue
+		if !backingDevIsUsed {
+			if err := squashfs.MaybeCleanupBackingDevice(backingDevice); err != nil {
+				return err
+			}
 		}
+	}
 
-		err = squashfs.Umount(a)
-		if err != nil {
-			return err
-		}
+	mountNSName, err := GetMountNSName()
+	if err != nil {
+		return err
+	}
+	destMetaDir := filepath.Join(RuntimeDir(metadir), "meta", mountNSName, ReplacePathSeparators(dest))
+	if err := os.RemoveAll(destMetaDir); err != nil {
+		return err
 	}
 
 	return nil

squashfs/verity.go

@@ -449,25 +449,42 @@ func findLoopBackingVerity(device string) (int64, error) {
 	return deviceNo, nil
 }
 
+// unmounts a squash mountpoint and detaches any verity / loop devices that back
+// it. Only use this if you are sure the underlying devices aren't in use by
+// other mount points.
 func Umount(mountpoint string) error {
+	devPath, err := GetBackingDevice(mountpoint)
+
+	err = unix.Unmount(mountpoint, 0)
+	if err != nil {
+		return errors.Wrapf(err, "failed unmounting %v", mountpoint)
+	}
+
+	return MaybeCleanupBackingDevice(devPath)
+}
+
+func GetBackingDevice(mountpoint string) (string, error) {
 	mounts, err := mount.ParseMounts("/proc/self/mountinfo")
 	if err != nil {
-		return err
+		return "", err
 	}
 
-	// first, find the verity device that backs the mount
 	theMount, found := mounts.FindMount(mountpoint)
 	if !found {
-		return errors.Errorf("%s is not a mountpoint", mountpoint)
-	}
-
-	err = unix.Unmount(mountpoint, 0)
-	if err != nil {
-		return errors.Wrapf(err, "failed unmounting %v", mountpoint)
+		return "", errors.Errorf("%s is not a mountpoint", mountpoint)
 	}
+	return theMount.Source, nil
+}
 
-	if _, err := os.Stat(theMount.Source); err != nil {
+// given a device path that had been used as the backing device for a squash
+// mountpoint, cleans up and detaches verity device if it still exists.
+//
+// If the device path does not exist, that is OK - this happens if the device
+// was a regular loopback and not -verity.
+func MaybeCleanupBackingDevice(devPath string) error {
+	if _, err := os.Stat(devPath); err != nil {
 		if os.IsNotExist(err) {
+			// It's been detached, this is fine.
 			return nil
 		}
 		return errors.WithStack(err)
@@ -476,9 +493,9 @@ func Umount(mountpoint string) error {
 	// was this a verity mount or a regular loopback mount? (if it's a
 	// regular loopback mount, we detached it above, so need to do anything
 	// special here; verity doesn't play as nicely)
-	if strings.HasSuffix(theMount.Source, veritySuffix) {
+	if strings.HasSuffix(devPath, veritySuffix) {
 		// find the loop device that backs the verity device
-		deviceNo, err := findLoopBackingVerity(theMount.Source)
+		deviceNo, err := findLoopBackingVerity(devPath)
 		if err != nil {
 			return err
 		}
@@ -488,18 +505,22 @@ func Umount(mountpoint string) error {
 		// above). the cryptsetup API allows us to pass NULL for the crypt
 		// device, but go-cryptsetup doesn't have a way to initialize a NULL
 		// crypt device short of making the struct by hand like this.
-		err = (&cryptsetup.Device{}).Deactivate(theMount.Source)
+		err = (&cryptsetup.Device{}).Deactivate(devPath)
 		if err != nil {
 			return errors.WithStack(err)
 		}
 
 		// finally, kill the loop dev
 		err = loopDev.Detach()
 		if err != nil {
-			return errors.Wrapf(err, "failed to detach loop dev for %v", theMount.Source)
+			return errors.Wrapf(err, "failed to detach loop dev for %v", devPath)
 		}
 	}
 
+	// NOTE: because of lazy device destruction, it is possible for a non-verity
+	// backing loop dev to be auto-detached but still exist. There's nothing for
+	// us to do in that case, so we return nil.
+
 	return nil
 }