Merge pull request #302 from WaberZhuang/main

check if snapshot holds mountpoint before remove

Author: liulanzheng

internal/log/helper.go

@@ -0,0 +1,30 @@
+/*
+   Copyright The Accelerated Container Image Authors
+
+   Licensed under the Apache License, Version 2.0 (the "License");
+   you may not use this file except in compliance with the License.
+   You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+*/
+
+package log
+
+import (
+	"context"
+	"fmt"
+
+	clog "github.com/containerd/log"
+)
+
+func TracedErrorf(ctx context.Context, format string, args ...any) error {
+	err := fmt.Errorf(format, args...)
+	clog.G(ctx).Error(err)
+	return err
+}

pkg/snapshot/overlay.go

@@ -29,6 +29,7 @@ import (
 	"github.com/containerd/accelerated-container-image/pkg/label"
 	"github.com/containerd/accelerated-container-image/pkg/snapshot/diskquota"
 
+	mylog "github.com/containerd/accelerated-container-image/internal/log"
 	"github.com/containerd/accelerated-container-image/pkg/metrics"
 	"github.com/data-accelerator/zdfs"
 	"github.com/sirupsen/logrus"
@@ -944,7 +945,7 @@ func (o *snapshotter) Remove(ctx context.Context, key string) (err error) {
 		if err == nil {
 			err = o.unmountAndDetachBlockDevice(ctx, id, key)
 			if err != nil {
-				return errors.Wrapf(err, "failed to destroy target device for snapshot %s", key)
+				return mylog.TracedErrorf(ctx, "failed to destroy target device for snapshot %s: %w", key, err)
 			}
 		}
 	}
@@ -961,12 +962,20 @@ func (o *snapshotter) Remove(ctx context.Context, key string) (err error) {
 			if st, err := o.identifySnapshotStorageType(ctx, s.ParentIDs[0], info); err == nil && st != storageTypeNormal {
 				err = o.unmountAndDetachBlockDevice(ctx, s.ParentIDs[0], "")
 				if err != nil {
-					return errors.Wrapf(err, "failed to destroy target device for snapshot %s", key)
+					return mylog.TracedErrorf(ctx, "failed to destroy target device for snapshot %s: %w", key, err)
 				}
 			}
 		}
 	}
 
+	// Just in case, check if snapshot contains mountpoint
+	mounted, err := o.isMounted(ctx, o.overlaybdMountpoint(id))
+	if err != nil {
+		return mylog.TracedErrorf(ctx, "failed to check mountpoint: %w", err)
+	} else if mounted {
+		return mylog.TracedErrorf(ctx, "try to remove snapshot %s which still have mountpoint", id)
+	}
+
 	_, _, err = storage.Remove(ctx, key)
 	if err != nil {
 		return errors.Wrap(err, "failed to remove")

pkg/snapshot/storage.go

@@ -68,21 +68,45 @@ const (
 	obdMaxDataAreaMB = 4
 )
 
+type mountMatcherFunc func(fields []string, separatorIndex int) bool
+
 func (o *snapshotter) checkOverlaybdInUse(ctx context.Context, dir string) (bool, error) {
+	matcher := func(fields []string, separatorIndex int) bool {
+		// ... but in Linux <= 3.9 mounting a cifs with spaces in a share name
+		// (like "//serv/My Documents") _may_ end up having a space in the last field
+		// of mountinfo (like "unc=//serv/My Documents"). Since kernel 3.10-rc1, cifs
+		// option unc= is ignored,  so a space should not appear. In here we ignore
+		// those "extra" fields caused by extra spaces.
+		fstype := fields[separatorIndex+1]
+		vfsOpts := fields[separatorIndex+3]
+		return fstype == "overlay" && strings.Contains(vfsOpts, dir)
+	}
+	return o.matchMounts(ctx, matcher)
+}
+
+func (o *snapshotter) isMounted(ctx context.Context, mountpoint string) (bool, error) {
+	matcher := func(fields []string, separatorIndex int) bool {
+		mp := fields[4]
+		return path.Clean(mountpoint) == path.Clean(mp)
+	}
+	return o.matchMounts(ctx, matcher)
+}
+
+func (o *snapshotter) matchMounts(ctx context.Context, matcher mountMatcherFunc) (bool, error) {
 	f, err := os.Open("/proc/self/mountinfo")
 	if err != nil {
 		return false, err
 	}
 	defer f.Close()
-	b, err := o.parseAndCheckMounted(ctx, f, dir)
+	b, err := o.parseAndCheckMounted(ctx, f, matcher)
 	if err != nil {
 		log.G(ctx).Errorf("Parsing mounts fields, error: %v", err)
 		return false, err
 	}
 	return b, nil
 }
 
-func (o *snapshotter) parseAndCheckMounted(ctx context.Context, r io.Reader, dir string) (bool, error) {
+func (o *snapshotter) parseAndCheckMounted(ctx context.Context, r io.Reader, matcher mountMatcherFunc) (bool, error) {
 	s := bufio.NewScanner(r)
 	for s.Scan() {
 		if err := s.Err(); err != nil {
@@ -139,21 +163,15 @@ func (o *snapshotter) parseAndCheckMounted(ctx context.Context, r io.Reader, dir
 			log.G(ctx).Warnf("Parsing '%s' failed: not enough fields after a separator", text)
 			continue
 		}
-		// ... but in Linux <= 3.9 mounting a cifs with spaces in a share name
-		// (like "//serv/My Documents") _may_ end up having a space in the last field
-		// of mountinfo (like "unc=//serv/My Documents"). Since kernel 3.10-rc1, cifs
-		// option unc= is ignored,  so a space should not appear. In here we ignore
-		// those "extra" fields caused by extra spaces.
-		fstype := fields[i+1]
-		vfsOpts := fields[i+3]
-		if fstype == "overlay" && strings.Contains(vfsOpts, dir) {
+
+		if matcher(fields, i) {
 			return true, nil
 		}
 	}
 	return false, nil
 }
 
-// unmountAndDetachBlockDevice
+// unmountAndDetachBlockDevice will do nothing if the device is already destroyed
 func (o *snapshotter) unmountAndDetachBlockDevice(ctx context.Context, snID string, snKey string) (err error) {
 	devName, err := os.ReadFile(o.overlaybdBackstoreMarkFile(snID))
 	if err != nil {