Repository: firecracker-microvm/firecracker-containerd
Pull request #106 (2019-02-18 21:00:00)

Snapshotter improvements
*Description of changes:*
Address a few issues I got in `containerd` [PR](https://github.com/containerd/containerd/pull/3022) review:
- Resume base thin-device when snapshot creation failed
- Better check whether thin-device is active (all thin devices gets deactivated on reboot, so we can't rely on metadata)
- Snapshotter no longer creates or reloads thin-pool, just opens it or fails.
- Rollback thin device on error (#107)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

/cc @sipsma


mxpv: > What changes for test setup? Do tests still use a loop device and take care of their own setup?

We still use sparse files, but added `dmsetup.Create` (see [here](https://github.com/firecracker-microvm/firecracker-containerd/pull/106/files#diff-8ac264ffe2e041eb8b322c927cf6c685R68))

Patch:
diff --git a/.buildkite/pipeline.yml b/.buildkite/pipeline.yml
index 2682952..0f6d436 100644
--- a/.buildkite/pipeline.yml
+++ b/.buildkite/pipeline.yml
@@ -1,4 +1,4 @@
-# Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved.
+# Copyright 2018-2019 Amazon.com, Inc. or its affiliates. All Rights Reserved.
 #
 # Licensed under the Apache License, Version 2.0 (the "License"). You may
 # not use this file except in compliance with the License. A copy of the
@@ -31,3 +31,8 @@ steps:
     commands:
       - "cd snapshotter"
       - "sudo make test EXTRAGOARGS='-v -count=1'"
+
+  - label: ':running_shirt_with_sash: runtime tests'
+    commands:
+      - "cd runtime"
+      - "make test EXTRAGOARGS='-v -count=1'"
\ No newline at end of file
diff --git a/go.mod b/go.mod
index 12b6f3a..d140bc4 100644
--- a/go.mod
+++ b/go.mod
@@ -38,5 +38,5 @@ require (
 	golang.org/x/sys v0.0.0-20181107165924-66b7b1311ac8
 	google.golang.org/genproto v0.0.0-20181109154231-b5d43981345b // indirect
 	google.golang.org/grpc v1.16.0
-	gotest.tools v2.2.0+incompatible // indirect
+	gotest.tools v2.2.0+incompatible
 )
diff --git a/runtime/Makefile b/runtime/Makefile
index aff63c2..1951a0a 100644
--- a/runtime/Makefile
+++ b/runtime/Makefile
@@ -11,16 +11,24 @@
 # express or implied. See the License for the specific language governing
 # permissions and limitations under the License.
 
+# Set this to pass additional commandline flags to the go compiler, e.g. "make test EXTRAGOARGS=-v"
+EXTRAGOARGS:=
+
+SOURCES:=$(shell find . -name '*.go')
+
 all: runtime
 
 runtime: containerd-shim-aws-firecracker
 
-containerd-shim-aws-firecracker: *.go
+containerd-shim-aws-firecracker: $(SOURCES)
 	go build -o containerd-shim-aws-firecracker
 
 install: containerd-shim-aws-firecracker
 	install -D -o root -g root -m755 -t $(INSTALLROOT)/bin containerd-shim-aws-firecracker
 
+test: $(SOURCES)
+	go test ./... $(EXTRAGOARGS)
+
 clean:
 	- rm -f containerd-shim-aws-firecracker
 
diff --git a/snapshotter/cmd/devmapper/main.go b/snapshotter/cmd/devmapper/main.go
index 528abfb..75b3b99 100644
--- a/snapshotter/cmd/devmapper/main.go
+++ b/snapshotter/cmd/devmapper/main.go
@@ -110,20 +110,8 @@ func applyStorageOpt(ctx context.Context, key, value string, config *devmapper.C
 
 		config.BaseImageSize = value
 		config.BaseImageSizeBytes = uint64(size)
-	case "dm.metadatadev":
-		config.MetadataDevice = value
-	case "dm.datadev":
-		config.DataDevice = value
 	case "dm.thinpooldev":
 		config.PoolName = strings.TrimPrefix(value, dmsetup.DevMapperDir)
-	case "dm.blocksize":
-		size, err := units.RAMInBytes(value)
-		if err != nil {
-			return err
-		}
-
-		config.DataBlockSize = value
-		config.DataBlockSizeSectors = uint32(size / dmsetup.SectorSize)
 	case "dm.fs":
 		// TODO: Support alternative file systems (https://github.com/firecracker-microvm/firecracker-containerd/issues/44)
 		if value != "ext4" {
diff --git a/snapshotter/devmapper/config.go b/snapshotter/devmapper/config.go
index f4ace00..60b5145 100644
--- a/snapshotter/devmapper/config.go
+++ b/snapshotter/devmapper/config.go
@@ -22,19 +22,6 @@ import (
 	"github.com/docker/go-units"
 	"github.com/hashicorp/go-multierror"
 	"github.com/pkg/errors"
-
-	"github.com/firecracker-microvm/firecracker-containerd/snapshotter/pkg/dmsetup"
-)
-
-const (
-	// See https://www.kernel.org/doc/Documentation/device-mapper/thin-provisioning.txt for details
-	dataBlockMinSize = 128
-	dataBlockMaxSize = 2097152
-)
-
-var (
-	errInvalidBlockSize      = errors.Errorf("block size should be between %d and %d", dataBlockMinSize, dataBlockMaxSize)
-	errInvalidBlockAlignment = errors.Errorf("block size should be multiple of %d sectors", dataBlockMinSize)
 )
 
 // Config represents device mapper configuration loaded from file.
@@ -46,19 +33,6 @@ type Config struct {
 	// Name for 'thin-pool' device to be used by snapshotter (without /dev/mapper/ prefix)
 	PoolName string `json:"pool_name"`
 
-	// Path to data volume to be used by thin-pool
-	DataDevice string `json:"data_device"`
-
-	// Path to metadata volume to be used by thin-pool
-	MetadataDevice string `json:"meta_device"`
-
-	// The size of allocation chunks in data file.
-	// Must be between 128 sectors (64KB) and 2097152 sectors (1GB) and a multiple of 128 sectors (64KB)
-	// Block size can't be changed after pool created.
-	// See https://www.kernel.org/doc/Documentation/device-mapper/thin-provisioning.txt
-	DataBlockSize        string `json:"data_block_size"`
-	DataBlockSizeSectors uint32 `json:"-"`
-
 	// Defines how much space to allocate when creating base image for container
 	BaseImageSize      string `json:"base_image_size"`
 	BaseImageSizeBytes uint64 `json:"-"`
@@ -96,23 +70,13 @@ func LoadConfig(path string) (*Config, error) {
 }
 
 func (c *Config) parse() error {
-	var result *multierror.Error
-
-	if c.DataBlockSize != "" {
-		if blockSize, err := units.RAMInBytes(c.DataBlockSize); err != nil {
-			result = multierror.Append(result, errors.Wrapf(err, "failed to parse data block size: %q", c.DataBlockSize))
-		} else {
-			c.DataBlockSizeSectors = uint32(blockSize / dmsetup.SectorSize)
-		}
-	}
-
-	if baseImageSize, err := units.RAMInBytes(c.BaseImageSize); err != nil {
-		result = multierror.Append(result, errors.Wrapf(err, "failed to parse base image size: %q", c.BaseImageSize))
-	} else {
-		c.BaseImageSizeBytes = uint64(baseImageSize)
+	baseImageSize, err := units.RAMInBytes(c.BaseImageSize)
+	if err != nil {
+		return errors.Wrapf(err, "failed to parse base image size: '%s'", c.BaseImageSize)
 	}
 
-	return result.ErrorOrNil()
+	c.BaseImageSizeBytes = uint64(baseImageSize)
+	return nil
 }
 
 // Validate makes sure configuration fields are valid
@@ -131,32 +95,5 @@ func (c *Config) Validate() error {
 		result = multierror.Append(result, fmt.Errorf("base_image_size is required"))
 	}
 
-	// The following fields are required only if we want to create or reload pool.
-	// Otherwise existing pool with 'PoolName' (prepared in advance) can be used by snapshotter.
-	if c.DataDevice != "" || c.MetadataDevice != "" || c.DataBlockSize != "" || c.DataBlockSizeSectors != 0 {
-		strChecks := []struct {
-			field string
-			name  string
-		}{
-			{c.DataDevice, "data_device"},
-			{c.MetadataDevice, "meta_device"},
-			{c.DataBlockSize, "data_block_size"},
-		}
-
-		for _, check := range strChecks {
-			if check.field == "" {
-				result = multierror.Append(result, errors.Errorf("%s is empty", check.name))
-			}
-		}
-
-		if c.DataBlockSizeSectors < dataBlockMinSize || c.DataBlockSizeSectors > dataBlockMaxSize {
-			result = multierror.Append(result, errInvalidBlockSize)
-		}
-
-		if c.DataBlockSizeSectors%dataBlockMinSize != 0 {
-			result = multierror.Append(result, errInvalidBlockAlignment)
-		}
-	}
-
 	return result.ErrorOrNil()
 }
diff --git a/snapshotter/devmapper/pool_device.go b/snapshotter/devmapper/pool_device.go
index ede0e5a..dfe1309 100644
--- a/snapshotter/devmapper/pool_device.go
+++ b/snapshotter/devmapper/pool_device.go
@@ -15,7 +15,6 @@ package devmapper
 
 import (
 	"context"
-	"os"
 	"path/filepath"
 
 	"github.com/containerd/containerd/log"
@@ -50,8 +49,10 @@ func NewPoolDevice(ctx context.Context, config *Config) (*PoolDevice, error) {
 		return nil, err
 	}
 
-	if err := openPool(ctx, config); err != nil {
-		return nil, err
+	// Make sure pool exists and available
+	poolPath := dmsetup.GetFullDevicePath(config.PoolName)
+	if _, err := dmsetup.Info(poolPath); err != nil {
+		return nil, errors.Wrapf(err, "failed to query pool %q", poolPath)
 	}
 
 	return &PoolDevice{
@@ -60,50 +61,6 @@ func NewPoolDevice(ctx context.Context, config *Config) (*PoolDevice, error) {
 	}, nil
 }
 
-func openPool(ctx context.Context, config *Config) error {
-	if err := config.Validate(); err != nil {
-		return err
-	}
-
-	var (
-		poolPath   = dmsetup.GetFullDevicePath(config.PoolName)
-		poolExists = false
-	)
-
-	if _, err := os.Stat(poolPath); err != nil && !os.IsNotExist(err) {
-		return errors.Wrapf(err, "failed to stat for %q", poolPath)
-	} else if err == nil {
-		poolExists = true
-	}
-
-	// Create new pool if not exists
-	if !poolExists {
-		log.G(ctx).Debug("creating new pool device")
-		if err := dmsetup.CreatePool(config.PoolName, config.DataDevice, config.MetadataDevice, config.DataBlockSizeSectors); err != nil {
-			return errors.Wrapf(err, "failed to create thin-pool with name %q", config.PoolName)
-		}
-
-		return nil
-	}
-
-	// Pool exists, check if it needs to be reloaded
-	if config.DataDevice != "" && config.MetadataDevice != "" {
-		log.G(ctx).Debugf("reloading existing pool %q", poolPath)
-		if err := dmsetup.ReloadPool(config.PoolName, config.DataDevice, config.MetadataDevice, config.DataBlockSizeSectors); err != nil {
-			return errors.Wrapf(err, "failed to reload pool %q", config.PoolName)
-		}
-
-		return nil
-	}
-
-	// If data and meta devices are not provided, use existing pool. Query info to make sure it's OK.
-	if _, err := dmsetup.Info(poolPath); err != nil {
-		return errors.Wrapf(err, "failed to query info for existing pool %q", poolPath)
-	}
-
-	return nil
-}
-
 // transition invokes 'updateStateFn' callback to perform devmapper operation and reflects device state changes/errors in meta store.
 // 'tryingState' will be set before invoking callback. If callback succeeded 'successState' will be set, otherwise
 // error details will be recorded in meta store.
@@ -148,7 +105,7 @@ func (p *PoolDevice) transition(ctx context.Context, deviceName string, tryingSt
 // CreateThinDevice creates new devmapper thin-device with given name and size.
 // Device ID for thin-device will be allocated from metadata store.
 // If allocation successful, device will be activated with /dev/mapper/<deviceName>
-func (p *PoolDevice) CreateThinDevice(ctx context.Context, deviceName string, virtualSizeBytes uint64) error {
+func (p *PoolDevice) CreateThinDevice(ctx context.Context, deviceName string, virtualSizeBytes uint64) (retErr error) {
 	info := &DeviceInfo{
 		Name:  deviceName,
 		Size:  virtualSizeBytes,
@@ -160,15 +117,46 @@ func (p *PoolDevice) CreateThinDevice(ctx context.Context, deviceName string, vi
 		return errors.Wrapf(err, "failed to save initial metadata for new thin device %q", deviceName)
 	}
 
+	defer func() {
+		if retErr == nil {
+			return
+		}
+
+		// Rollback metadata
+		retErr = multierror.Append(retErr, p.metadata.RemoveDevice(ctx, info.Name))
+	}()
+
 	// Create thin device
-	if err := p.transition(ctx, deviceName, Creating, Created, func() error {
+	if err := p.createDevice(ctx, info); err != nil {
+		return err
+	}
+
+	defer func() {
+		if retErr == nil {
+			return
+		}
+
+		// Rollback creation
+		retErr = multierror.Append(retErr, p.deleteDevice(ctx, info))
+	}()
+
+	return p.activateDevice(ctx, info)
+}
+
+// createDevice creates thin device
+func (p *PoolDevice) createDevice(ctx context.Context, info *DeviceInfo) error {
+	if err := p.transition(ctx, info.Name, Creating, Created, func() error {
 		return dmsetup.CreateDevice(p.poolName, info.DeviceID)
 	}); err != nil {
 		return errors.Wrapf(err, "failed to create new thin device %q (dev: %d)", info.Name, info.DeviceID)
 	}
 
-	// Activate thin device
-	if err := p.transition(ctx, deviceName, Activating, Activated, func() error {
+	return nil
+}
+
+// activateDevice activates thin device
+func (p *PoolDevice) activateDevice(ctx context.Context, info *DeviceInfo) error {
+	if err := p.transition(ctx, info.Name, Activating, Activated, func() error {
 		return dmsetup.ActivateDevice(p.poolName, info.Name, info.DeviceID, info.Size, "")
 	}); err != nil {
 		return errors.Wrapf(err, "failed to activate new thin device %q (dev: %d)", info.Name, info.DeviceID)
@@ -178,21 +166,23 @@ func (p *PoolDevice) CreateThinDevice(ctx context.Context, deviceName string, vi
 }
 
 // CreateSnapshotDevice creates and activates new thin-device from parent thin-device (makes snapshot)
-func (p *PoolDevice) CreateSnapshotDevice(ctx context.Context, deviceName string, snapshotName string, virtualSizeBytes uint64) error {
+func (p *PoolDevice) CreateSnapshotDevice(ctx context.Context, deviceName string, snapshotName string, virtualSizeBytes uint64) (retErr error) {
 	baseInfo, err := p.metadata.GetDevice(ctx, deviceName)
 	if err != nil {
 		return errors.Wrapf(err, "failed to query device metadata for %q", deviceName)
 	}
 
-	isActivated := baseInfo.State == Activated
-
-	// Suspend thin device if it was activated previously
+	// Suspend thin device if it was activated previously to avoid corruptions
+	isActivated := p.IsActivated(baseInfo.Name)
 	if isActivated {
-		if err := p.transition(ctx, baseInfo.Name, Suspending, Suspended, func() error {
-			return dmsetup.SuspendDevice(baseInfo.Name)
-		}); err != nil {
-			return errors.Wrapf(err, "failed to suspend device %q", baseInfo.Name)
+		if err := p.suspendDevice(ctx, baseInfo); err != nil {
+			return err
 		}
+
+		// Resume back base thin device on exit
+		defer func() {
+			retErr = multierror.Append(retErr, p.resumeDevice(ctx, baseInfo)).ErrorOrNil()
+		}()
 	}
 
 	snapInfo := &DeviceInfo{
@@ -207,33 +197,63 @@ func (p *PoolDevice) CreateSnapshotDevice(ctx context.Context, deviceName string
 		return errors.Wrapf(err, "failed to save initial metadata for snapshot %q", snapshotName)
 	}
 
+	defer func() {
+		if retErr == nil {
+			return
+		}
+
+		// Rollback metadata
+		retErr = multierror.Append(retErr, p.metadata.RemoveDevice(ctx, snapInfo.Name))
+	}()
+
 	// Create thin device snapshot
+	if err := p.createSnapshot(ctx, baseInfo, snapInfo); err != nil {
+		return err
+	}
+
+	defer func() {
+		if retErr == nil {
+			return
+		}
+
+		// Rollback snapshot creation
+		retErr = multierror.Append(retErr, p.deleteDevice(ctx, snapInfo))
+	}()
+
+	// Activate snapshot device
+	return p.activateDevice(ctx, snapInfo)
+}
+
+func (p *PoolDevice) suspendDevice(ctx context.Context, info *DeviceInfo) error {
+	if err := p.transition(ctx, info.Name, Suspending, Suspended, func() error {
+		return dmsetup.SuspendDevice(info.Name)
+	}); err != nil {
+		return errors.Wrapf(err, "failed to suspend device %q", info.Name)
+	}
+
+	return nil
+}
+
+func (p *PoolDevice) resumeDevice(ctx context.Context, info *DeviceInfo) error {
+	if err := p.transition(ctx, info.Name, Resuming, Resumed, func() error {
+		return dmsetup.ResumeDevice(info.Name)
+	}); err != nil {
+		return errors.Wrapf(err, "failed to resume device %q", info.Name)
+	}
+
+	return nil
+}
+
+func (p *PoolDevice) createSnapshot(ctx context.Context, baseInfo, snapInfo *DeviceInfo) error {
 	if err := p.transition(ctx, snapInfo.Name, Creating, Created, func() error {
 		return dmsetup.CreateSnapshot(p.poolName, snapInfo.DeviceID, baseInfo.DeviceID)
 	}); err != nil {
 		return errors.Wrapf(err,
-			"failed to create snapshot %q (dev: %d) from %q (dev: %d, activated: %t)",
+			"failed to create snapshot %q (dev: %d) from %q (dev: %d)",
 			snapInfo.Name,
 			snapInfo.DeviceID,
 			baseInfo.Name,
-			baseInfo.DeviceID,
-			isActivated)
-	}
-
-	if isActivated {
-		// Resume base thin-device
-		if err := p.transition(ctx, baseInfo.Name, Resuming, Resumed, func() error {
-			return dmsetup.ResumeDevice(baseInfo.Name)
-		}); err != nil {
-			return errors.Wrapf(err, "failed to resume device %q", deviceName)
-		}
-	}
-
-	// Activate snapshot
-	if err := p.transition(ctx, snapInfo.Name, Activating, Activated, func() error {
-		return dmsetup.ActivateDevice(p.poolName, snapInfo.Name, snapInfo.DeviceID, snapInfo.Size, "")
-	}); err != nil {
-		return errors.Wrapf(err, "failed to activate snapshot device %q (dev: %d)", snapInfo.Name, snapInfo.DeviceID)
+			baseInfo.DeviceID)
 	}
 
 	return nil
@@ -241,13 +261,8 @@ func (p *PoolDevice) CreateSnapshotDevice(ctx context.Context, deviceName string
 
 // DeactivateDevice deactivates thin device
 func (p *PoolDevice) DeactivateDevice(ctx context.Context, deviceName string, deferred bool) error {
-	devicePath := dmsetup.GetFullDevicePath(deviceName)
-	if _, err := os.Stat(devicePath); err != nil {
-		if os.IsNotExist(err) {
-			return ErrNotFound
-		}
-
-		return err
+	if !p.IsActivated(deviceName) {
+		return nil
 	}
 
 	opts := []dmsetup.RemoveDeviceOpt{dmsetup.RemoveWithForce, dmsetup.RemoveWithRetries}
@@ -264,6 +279,21 @@ func (p *PoolDevice) DeactivateDevice(ctx context.Context, deviceName string, de
 	return nil
 }
 
+// IsActivated returns true if thin-device is activated and not suspended
+func (p *PoolDevice) IsActivated(deviceName string) bool {
+	infos, err := dmsetup.Info(deviceName)
+	if err != nil || len(infos) != 1 {
+		// Couldn't query device info, device not active
+		return false
+	}
+
+	if devInfo := infos[0]; devInfo.Suspended {
+		return false
+	}
+
+	return true
+}
+
 // RemoveDevice completely wipes out thin device from thin-pool and frees it's device ID
 func (p *PoolDevice) RemoveDevice(ctx context.Context, deviceName string) error {
 	info, err := p.metadata.GetDevice(ctx, deviceName)
@@ -271,15 +301,12 @@ func (p *PoolDevice) RemoveDevice(ctx context.Context, deviceName string) error
 		return errors.Wrapf(err, "can't query metadata for device %q", deviceName)
 	}
 
-	if err := p.DeactivateDevice(ctx, deviceName, true); err != nil && err != ErrNotFound {
+	if err := p.DeactivateDevice(ctx, deviceName, true); err != nil {
 		return err
 	}
 
-	if err := p.transition(ctx, deviceName, Removing, Removed, func() error {
-		// Send 'delete' message to thin-pool
-		return dmsetup.DeleteDevice(p.poolName, info.DeviceID)
-	}); err != nil {
-		return errors.Wrapf(err, "failed to delete device %q (dev id: %d)", info.Name, info.DeviceID)
+	if err := p.deleteDevice(ctx, info); err != nil {
+		return err
 	}
 
 	// Remove record from meta store and free device ID
@@ -290,6 +317,17 @@ func (p *PoolDevice) RemoveDevice(ctx context.Context, deviceName string) error
 	return nil
 }
 
+func (p *PoolDevice) deleteDevice(ctx context.Context, info *DeviceInfo) error {
+	if err := p.transition(ctx, info.Name, Removing, Removed, func() error {
+		// Send 'delete' message to thin-pool
+		return dmsetup.DeleteDevice(p.poolName, info.DeviceID)
+	}); err != nil {
+		return errors.Wrapf(err, "failed to delete device %q (dev id: %d)", info.Name, info.DeviceID)
+	}
+
+	return nil
+}
+
 // RemovePool deactivates all child thin-devices and removes thin-pool device
 func (p *PoolDevice) RemovePool(ctx context.Context) error {
 	deviceNames, err := p.metadata.GetDeviceNames(ctx)
@@ -301,7 +339,7 @@ func (p *PoolDevice) RemovePool(ctx context.Context) error {
 
 	// Deactivate devices if any
 	for _, name := range deviceNames {
-		if err := p.DeactivateDevice(ctx, name, true); err != nil && err != ErrNotFound {
+		if err := p.DeactivateDevice(ctx, name, true); err != nil {
 			result = multierror.Append(result, errors.Wrapf(err, "failed to remove %q", name))
 		}
 	}
diff --git a/snapshotter/devmapper/snapshotter.go b/snapshotter/devmapper/snapshotter.go
index dbe2699..83c909a 100644
--- a/snapshotter/devmapper/snapshotter.go
+++ b/snapshotter/devmapper/snapshotter.go
@@ -275,7 +275,9 @@ func (s *Snapshotter) createSnapshot(ctx context.Context, kind snapshots.Kind, k
 		}
 
 		if err := s.mkfs(ctx, deviceName); err != nil {
-			return nil, err
+			// Rollback thin device creation if mkfs failed
+			return nil, multierror.Append(err,
+				s.pool.RemoveDevice(ctx, deviceName))
 		}
 	} else {
 		parentDeviceName := s.getDeviceName(snap.ParentIDs[0])


Test patch:
diff --git a/runtime/service_test.go b/runtime/service_test.go
index a2f2593..8d7fdac 100644
--- a/runtime/service_test.go
+++ b/runtime/service_test.go
@@ -40,7 +40,9 @@ func TestFindNextAvailableVsockCID(t *testing.T) {
 	}()
 
 	cid, err := findNextAvailableVsockCID(context.Background())
-	require.NoError(t, err)
+	require.NoError(t, err,
+		"Do you have permission to interact with /dev/vhost-vsock?\n"+
+			"Grant yourself permission with `sudo setfacl -m u:${USER}:rw /dev/vhost-vsock`")
 	require.EqualValues(t, uint32(3), cid)
 
 	ctx, cancel := context.WithCancel(context.Background())
diff --git a/snapshotter/cmd/devmapper/main_test.go b/snapshotter/cmd/devmapper/main_test.go
index 7d22824..9fd57f4 100644
--- a/snapshotter/cmd/devmapper/main_test.go
+++ b/snapshotter/cmd/devmapper/main_test.go
@@ -43,9 +43,5 @@ func TestApplyStorageOpt(t *testing.T) {
 
 	assert.Equal(t, "10gb", config.BaseImageSize)
 	assert.EqualValues(t, 10*1024*1024*1024, config.BaseImageSizeBytes)
-	assert.Equal(t, "/meta_dev", config.MetadataDevice)
-	assert.Equal(t, "/data_dev", config.DataDevice)
 	assert.Equal(t, "/pool_dev", config.PoolName)
-	assert.Equal(t, "100mb", config.DataBlockSize)
-	assert.EqualValues(t, 100*1024*1024/512, config.DataBlockSizeSectors)
 }
diff --git a/snapshotter/devmapper/config_test.go b/snapshotter/devmapper/config_test.go
index 4014bd0..314d712 100644
--- a/snapshotter/devmapper/config_test.go
+++ b/snapshotter/devmapper/config_test.go
@@ -17,7 +17,6 @@ import (
 	"encoding/json"
 	"io/ioutil"
 	"os"
-	"strings"
 	"testing"
 
 	"github.com/hashicorp/go-multierror"
@@ -27,12 +26,9 @@ import (
 
 func TestLoadConfig(t *testing.T) {
 	expected := Config{
-		RootPath:       "/tmp",
-		PoolName:       "test",
-		DataDevice:     "/dev/loop0",
-		MetadataDevice: "/dev/loop1",
-		DataBlockSize:  "1mb",
-		BaseImageSize:  "128Mb",
+		RootPath:      "/tmp",
+		PoolName:      "test",
+		BaseImageSize: "128Mb",
 	}
 
 	data, err := json.Marshal(&expected)
@@ -57,12 +53,8 @@ func TestLoadConfig(t *testing.T) {
 
 	assert.Equal(t, loaded.RootPath, expected.RootPath)
 	assert.Equal(t, loaded.PoolName, expected.PoolName)
-	assert.Equal(t, loaded.DataDevice, expected.DataDevice)
-	assert.Equal(t, loaded.MetadataDevice, expected.MetadataDevice)
-	assert.Equal(t, loaded.DataBlockSize, expected.DataBlockSize)
 	assert.Equal(t, loaded.BaseImageSize, expected.BaseImageSize)
 
-	assert.EqualValues(t, 1*1024*1024/512, loaded.DataBlockSizeSectors)
 	assert.EqualValues(t, 128*1024*1024, loaded.BaseImageSizeBytes)
 }
 
@@ -76,37 +68,25 @@ func TestLoadConfigInvalidPath(t *testing.T) {
 
 func TestParseInvalidData(t *testing.T) {
 	config := Config{
-		DataBlockSize: "x",
 		BaseImageSize: "y",
 	}
 
 	err := config.parse()
 	require.Error(t, err)
-
-	multErr := (err).(*multierror.Error)
-	require.Len(t, multErr.Errors, 2)
-
-	assert.True(t, strings.Contains(multErr.Errors[0].Error(), "failed to parse data block size: \"x\""))
-	assert.True(t, strings.Contains(multErr.Errors[1].Error(), "failed to parse base image size: \"y\""))
+	require.EqualError(t, err, "failed to parse base image size: 'y': invalid size: 'y'")
 }
 
 func TestFieldValidation(t *testing.T) {
-	config := &Config{DataBlockSizeSectors: 1}
+	config := &Config{}
 	err := config.Validate()
 	require.Error(t, err)
 
 	multErr := (err).(*multierror.Error)
-	require.Len(t, multErr.Errors, 8)
+	require.Len(t, multErr.Errors, 3)
 
 	assert.Error(t, multErr.Errors[0], "pool_name is empty")
 	assert.Error(t, multErr.Errors[1], "root_path is empty")
 	assert.Error(t, multErr.Errors[2], "base_image_size is empty")
-	assert.Error(t, multErr.Errors[3], "data_device is empty")
-	assert.Error(t, multErr.Errors[4], "meta_device is empty")
-	assert.Error(t, multErr.Errors[5], "data_block_size is empty")
-
-	assert.Equal(t, multErr.Errors[6], errInvalidBlockSize)
-	assert.Equal(t, multErr.Errors[7], errInvalidBlockAlignment)
 }
 
 func TestExistingPoolFieldValidation(t *testing.T) {
diff --git a/snapshotter/devmapper/pool_device_test.go b/snapshotter/devmapper/pool_device_test.go
index 502435a..5b59e02 100644
--- a/snapshotter/devmapper/pool_device_test.go
+++ b/snapshotter/devmapper/pool_device_test.go
@@ -15,18 +15,20 @@ package devmapper
 
 import (
 	"context"
+	"fmt"
 	"io/ioutil"
 	"os"
 	"os/exec"
 	"path/filepath"
 	"testing"
+	"time"
 
 	"github.com/docker/go-units"
-	"github.com/firecracker-microvm/firecracker-containerd/internal"
 	"github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
+	"github.com/firecracker-microvm/firecracker-containerd/internal"
 	"github.com/firecracker-microvm/firecracker-containerd/snapshotter/pkg/dmsetup"
 	"github.com/firecracker-microvm/firecracker-containerd/snapshotter/pkg/losetup"
 )
@@ -61,6 +63,10 @@ func TestPoolDevice(t *testing.T) {
 	_, loopDataDevice := createLoopbackDevice(t, tempDir)
 	_, loopMetaDevice := createLoopbackDevice(t, tempDir)
 
+	poolName := fmt.Sprintf("test-pool-device-%d", time.Now().Nanosecond())
+	err = dmsetup.CreatePool(poolName, loopDataDevice, loopMetaDevice, 64*1024/dmsetup.SectorSize)
+	require.NoErrorf(t, err, "failed to create pool %q", poolName)
+
 	defer func() {
 		// Detach loop devices and remove images
 		err := losetup.DetachLoopDevice(loopDataDevice, loopMetaDevice)
@@ -71,14 +77,10 @@ func TestPoolDevice(t *testing.T) {
 	}()
 
 	config := &Config{
-		PoolName:             "test-pool-device-1",
-		RootPath:             tempDir,
-		DataDevice:           loopDataDevice,
-		MetadataDevice:       loopMetaDevice,
-		DataBlockSize:        "65536",
-		DataBlockSizeSectors: 128,
-		BaseImageSize:        "16mb",
-		BaseImageSizeBytes:   16 * 1024 * 1024,
+		PoolName:           poolName,
+		RootPath:           tempDir,
+		BaseImageSize:      "16mb",
+		BaseImageSizeBytes: 16 * 1024 * 1024,
 	}
 
 	pool, err := NewPoolDevice(ctx, config)
@@ -187,12 +189,13 @@ func testDeactivateThinDevice(t *testing.T, pool *PoolDevice) {
 	}
 
 	for _, deviceName := range deviceList {
+		assert.True(t, pool.IsActivated(deviceName))
+
 		err := pool.DeactivateDevice(context.Background(), deviceName, false)
 		assert.NoErrorf(t, err, "failed to remove '%s'", deviceName)
-	}
 
-	err := pool.DeactivateDevice(context.Background(), "not-existing-device", false)
-	assert.Error(t, err, "should return an error if trying to remove not existing device")
+		assert.False(t, pool.IsActivated(deviceName))
+	}
 }
 
 func testRemoveThinDevice(t *testing.T, pool *PoolDevice) {
diff --git a/snapshotter/devmapper/snapshotter_test.go b/snapshotter/devmapper/snapshotter_test.go
index def2a4a..ed3fa6e 100644
--- a/snapshotter/devmapper/snapshotter_test.go
+++ b/snapshotter/devmapper/snapshotter_test.go
@@ -21,11 +21,11 @@ import (
 
 	"github.com/containerd/containerd/snapshots"
 	"github.com/containerd/containerd/snapshots/testsuite"
-	"github.com/firecracker-microvm/firecracker-containerd/internal"
+	"github.com/hashicorp/go-multierror"
 	"github.com/sirupsen/logrus"
-	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
+	"github.com/firecracker-microvm/firecracker-containerd/internal"
 	"github.com/firecracker-microvm/firecracker-containerd/snapshotter/pkg/dmsetup"
 	"github.com/firecracker-microvm/firecracker-containerd/snapshotter/pkg/losetup"
 )
@@ -54,22 +54,18 @@ func TestSnapshotterSuite(t *testing.T) {
 			return nil, nil, err
 		}
 
-		// Remove device mapper pool after test completes
+		// Remove device mapper pool and detach loop devices after test completes
 		removePool := func() error {
-			return snap.pool.RemovePool(ctx)
+			result := multierror.Append(
+				snap.pool.RemovePool(ctx),
+				losetup.DetachLoopDevice(loopDataDevice, loopMetaDevice))
+
+			return result.ErrorOrNil()
 		}
 
 		// Pool cleanup should be called before closing metadata store (as we need to retrieve device names)
 		snap.cleanupFn = append([]closeFunc{removePool}, snap.cleanupFn...)
 
-		return snap, func() error {
-			err := snap.Close()
-			assert.NoErrorf(t, err, "failed to close snapshotter")
-
-			err = losetup.DetachLoopDevice(loopDataDevice, loopMetaDevice)
-			assert.NoErrorf(t, err, "failed to detach loop devices")
-
-			return err
-		}, nil
+		return snap, snap.Close, nil
 	})
 }