Add retries when removing device mapper target (microsoft#1200)

Add retries when removing device mapper target

Additionally ignore the error from device mapper if target removal
still fails. The corresponding layer path is already unmounted
at that point and this avoids having an inconsistent state.

Signed-off-by: Maksim An <maksiman@microsoft.com>

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 {

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")
+	}
+}

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)
 		}
 	}