diff --git a/internal/log/helper.go b/internal/log/helper.go new file mode 100644 index 00000000..c6115bad --- /dev/null +++ b/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 +} diff --git a/pkg/snapshot/overlay.go b/pkg/snapshot/overlay.go index a83e2d8e..e0f66680 100644 --- a/pkg/snapshot/overlay.go +++ b/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") diff --git a/pkg/snapshot/storage.go b/pkg/snapshot/storage.go index bd5736f8..f0eb20f7 100644 --- a/pkg/snapshot/storage.go +++ b/pkg/snapshot/storage.go @@ -68,13 +68,37 @@ 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 @@ -82,7 +106,7 @@ func (o *snapshotter) checkOverlaybdInUse(ctx context.Context, dir string) (bool 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 {