Add retries when removing device mapper target

internal/guest/storage/devicemapper/devicemapper.go

@@ -6,6 +6,8 @@ import (
 	"fmt"
 	"os"
 	"path"
+	"syscall"
+	"time"
 	"unsafe"
 
 	"golang.org/x/sys/unix"
@@ -19,6 +21,11 @@ const (
 	CreateReadOnly CreateFlags = 1 << iota
 )
 
+var (
+	removeDeviceWrapper = removeDevice
+	openMapperWrapper   = openMapper
+)
+
 const (
 	_IOC_WRITE    = 1
 	_IOC_READ     = 2
@@ -223,7 +230,7 @@ func makeTableIoctl(name string, targets []Target) *dmIoctl {
 // CreateDevice creates a device-mapper device with the given target spec. It returns
 // the path of the new device node.
 func CreateDevice(name string, flags CreateFlags, targets []Target) (_ string, err error) {
-	f, err := openMapper()
+	f, err := openMapperWrapper()
 	if err != nil {
 		return "", err
 	}
@@ -238,7 +245,7 @@ func CreateDevice(name string, flags CreateFlags, targets []Target) (_ string, e
 	}
 	defer func() {
 		if err != nil {
-			removeDevice(f, name)
+			removeDeviceWrapper(f, name)
 		}
 	}()
 
@@ -269,14 +276,30 @@ func CreateDevice(name string, flags CreateFlags, targets []Target) (_ string, e
 }
 
 // RemoveDevice removes a device-mapper device and its associated device node.
-func RemoveDevice(name string) error {
-	f, err := openMapper()
-	if err != nil {
-		return err
+func RemoveDevice(name string) (err error) {
+	rm := func() error {
+		f, err := openMapperWrapper()
+		if err != nil {
+			return err
+		}
+		defer f.Close()
+		os.Remove(path.Join("/dev/mapper", name))
+		return removeDeviceWrapper(f, name)
 	}
-	defer f.Close()
-	os.Remove(path.Join("/dev/mapper", name))
-	return removeDevice(f, name)
+
+	// This is workaround for "device or resource busy" error, which occasionally happens after the device mapper
+	// target has been unmounted.
+	for i := 0; i < 10; i++ {
+		if err = rm(); err != nil {
+			if e, ok := err.(*dmError); !ok || e.Err != syscall.EBUSY {
+				break
+			}
+			time.Sleep(10 * time.Millisecond)
+			continue
+		}
+		break
+	}
+	return
 }
 
 func removeDevice(f *os.File, name string) error {

internal/guest/storage/devicemapper/devicemapper_test.go

@@ -4,7 +4,9 @@ package devicemapper
 
 import (
 	"flag"
+	"io/ioutil"
 	"os"
+	"syscall"
 	"testing"
 	"unsafe"
 
@@ -15,6 +17,11 @@ var (
 	integration = flag.Bool("integration", false, "run integration tests")
 )
 
+func clearTestDependencies() {
+	removeDeviceWrapper = removeDevice
+	openMapperWrapper = openMapper
+}
+
 func TestMain(m *testing.M) {
 	flag.Parse()
 	m.Run()
@@ -75,6 +82,8 @@ func createDevice(name string, flags CreateFlags, targets []Target) (*device, er
 }
 
 func TestCreateError(t *testing.T) {
+	clearTestDependencies()
+
 	if !*integration {
 		t.Skip()
 	}
@@ -94,6 +103,8 @@ func TestCreateError(t *testing.T) {
 }
 
 func TestReadOnlyError(t *testing.T) {
+	clearTestDependencies()
+
 	if !*integration {
 		t.Skip()
 	}
@@ -113,6 +124,8 @@ func TestReadOnlyError(t *testing.T) {
 }
 
 func TestLinearError(t *testing.T) {
+	clearTestDependencies()
+
 	if !*integration {
 		t.Skip()
 	}
@@ -140,3 +153,73 @@ func TestLinearError(t *testing.T) {
 		t.Fatal(err)
 	}
 }
+
+func TestRemoveDeviceRetriesOnSyscallEBUSY(t *testing.T) {
+	clearTestDependencies()
+
+	rmDeviceCalled := false
+	retryDone := false
+	// Overrides openMapper to return temp file handle
+	openMapperWrapper = func() (*os.File, error) {
+		return ioutil.TempFile("", "")
+	}
+	removeDeviceWrapper = func(_ *os.File, _ string) error {
+		if !rmDeviceCalled {
+			rmDeviceCalled = true
+			return &dmError{
+				Op:  1,
+				Err: syscall.EBUSY,
+			}
+		}
+		if !retryDone {
+			retryDone = true
+			return nil
+		}
+		return nil
+	}
+
+	if err := RemoveDevice("test"); err != nil {
+		t.Fatalf("expected no error, got: %s", err)
+	}
+	if !rmDeviceCalled {
+		t.Fatalf("expected removeDevice to be called at least once")
+	}
+	if !retryDone {
+		t.Fatalf("expected removeDevice to be retried after initial failure")
+	}
+}
+
+func TestRemoveDeviceFailsOnNonSyscallEBUSY(t *testing.T) {
+	clearTestDependencies()
+
+	expectedError := &dmError{
+		Op:  0,
+		Err: syscall.EACCES,
+	}
+	rmDeviceCalled := false
+	retryDone := false
+	openMapperWrapper = func() (*os.File, error) {
+		return ioutil.TempFile("", "")
+	}
+	removeDeviceWrapper = func(_ *os.File, _ string) error {
+		if !rmDeviceCalled {
+			rmDeviceCalled = true
+			return expectedError
+		}
+		if !retryDone {
+			retryDone = true
+			return nil
+		}
+		return nil
+	}
+
+	if err := RemoveDevice("test"); err != expectedError {
+		t.Fatalf("expected error %q, instead got %q", expectedError, err)
+	}
+	if !rmDeviceCalled {
+		t.Fatalf("expected removeDevice to be called once")
+	}
+	if retryDone {
+		t.Fatalf("no retries should've been attempted")
+	}
+}

internal/guest/storage/pmem/pmem.go

@@ -143,14 +143,16 @@ func Unmount(ctx context.Context, devNumber uint32, target string, mappingInfo *
 	if verityInfo != nil {
 		dmVerityName := fmt.Sprintf(verityDeviceFmt, devNumber, verityInfo.RootDigest)
 		if err := dm.RemoveDevice(dmVerityName); err != nil {
-			return errors.Wrapf(err, "failed to remove dm verity target: %s", dmVerityName)
+			// The target is already unmounted at this point, ignore potential errors
+			log.G(ctx).WithError(err).Debugf("failed to remove dm verity target: %s", dmVerityName)
 		}
 	}
 
 	if mappingInfo != nil {
 		dmLinearName := fmt.Sprintf(linearDeviceFmt, devNumber, mappingInfo.DeviceOffsetInBytes, mappingInfo.DeviceSizeInBytes)
 		if err := dm.RemoveDevice(dmLinearName); err != nil {
-			return errors.Wrapf(err, "failed to remove dm linear target: %s", dmLinearName)
+			// The target is already unmounted at this point, ignore potential errors
+			log.G(ctx).WithError(err).Debugf("failed to remove dm linear target: %s", dmLinearName)
 		}
 	}