From 753d95fbe2b6ad7e3cddb986160f914e78926a6d Mon Sep 17 00:00:00 2001 From: Maksim An Date: Wed, 13 Oct 2021 01:11:32 -0700 Subject: [PATCH 1/7] 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 --- .../storage/devicemapper/devicemapper.go | 30 +++++++++++++++---- internal/guest/storage/pmem/pmem.go | 6 ++-- 2 files changed, 28 insertions(+), 8 deletions(-) diff --git a/internal/guest/storage/devicemapper/devicemapper.go b/internal/guest/storage/devicemapper/devicemapper.go index 01d712d37a..976b02133e 100644 --- a/internal/guest/storage/devicemapper/devicemapper.go +++ b/internal/guest/storage/devicemapper/devicemapper.go @@ -6,6 +6,7 @@ import ( "fmt" "os" "path" + "time" "unsafe" "golang.org/x/sys/unix" @@ -270,13 +271,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 + rm := func() error { + f, err := openMapper() + if err != nil { + return err + } + 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 { + if err := rm(); err != nil { + select { + case <-time.After(time.Second): + return fmt.Errorf("timeout removing device %s", name) + default: + time.Sleep(10 * time.Millisecond) + continue + } + } + return nil } - defer f.Close() - os.Remove(path.Join("/dev/mapper", name)) - return removeDevice(f, name) } func removeDevice(f *os.File, name string) error { diff --git a/internal/guest/storage/pmem/pmem.go b/internal/guest/storage/pmem/pmem.go index 681659f061..8826e3d00d 100644 --- a/internal/guest/storage/pmem/pmem.go +++ b/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) } } From ebd6fb36304d7172779f19c168158d72a845f308 Mon Sep 17 00:00:00 2001 From: Maksim An Date: Thu, 21 Oct 2021 20:42:57 -0700 Subject: [PATCH 2/7] pr feedback: make fixed number of retries and remove timeout Signed-off-by: Maksim An --- .../guest/storage/devicemapper/devicemapper.go | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/internal/guest/storage/devicemapper/devicemapper.go b/internal/guest/storage/devicemapper/devicemapper.go index 976b02133e..6ea25edcd3 100644 --- a/internal/guest/storage/devicemapper/devicemapper.go +++ b/internal/guest/storage/devicemapper/devicemapper.go @@ -270,7 +270,7 @@ 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 { +func RemoveDevice(name string) (err error) { rm := func() error { f, err := openMapper() if err != nil { @@ -283,18 +283,14 @@ func RemoveDevice(name string) error { // This is workaround for "device or resource busy" error, which occasionally happens after the device mapper // target has been unmounted. - for { - if err := rm(); err != nil { - select { - case <-time.After(time.Second): - return fmt.Errorf("timeout removing device %s", name) - default: - time.Sleep(10 * time.Millisecond) - continue - } + for i := 0; i < 10; i++ { + if err = rm(); err != nil { + time.Sleep(10 * time.Millisecond) + continue } - return nil + break } + return } func removeDevice(f *os.File, name string) error { From ad1b044f9385c6162efaee2b8b7665ec52d83688 Mon Sep 17 00:00:00 2001 From: Maksim An Date: Mon, 1 Nov 2021 09:51:36 -0700 Subject: [PATCH 3/7] pr feedback: retry on "device or resource busy" only Signed-off-by: Maksim An --- internal/guest/storage/devicemapper/devicemapper.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/guest/storage/devicemapper/devicemapper.go b/internal/guest/storage/devicemapper/devicemapper.go index 6ea25edcd3..3d945e3f7a 100644 --- a/internal/guest/storage/devicemapper/devicemapper.go +++ b/internal/guest/storage/devicemapper/devicemapper.go @@ -6,6 +6,7 @@ import ( "fmt" "os" "path" + "strings" "time" "unsafe" @@ -284,7 +285,7 @@ func RemoveDevice(name string) (err error) { // 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 err = rm(); err != nil && strings.Contains(err.Error(), "device or resource busy") { time.Sleep(10 * time.Millisecond) continue } From 6543dd8d214326d153befd92477ad216e5c27a2f Mon Sep 17 00:00:00 2001 From: Maksim An Date: Mon, 1 Nov 2021 14:38:51 -0700 Subject: [PATCH 4/7] tests: add devicemapper.RemoveDevice unit test Signed-off-by: Maksim An --- .../storage/devicemapper/devicemapper.go | 13 +++-- .../storage/devicemapper/devicemapper_test.go | 49 +++++++++++++++++++ 2 files changed, 58 insertions(+), 4 deletions(-) diff --git a/internal/guest/storage/devicemapper/devicemapper.go b/internal/guest/storage/devicemapper/devicemapper.go index 3d945e3f7a..2737517df4 100644 --- a/internal/guest/storage/devicemapper/devicemapper.go +++ b/internal/guest/storage/devicemapper/devicemapper.go @@ -21,6 +21,11 @@ const ( CreateReadOnly CreateFlags = 1 << iota ) +var ( + removeDeviceWrapper = removeDevice + openMapperWrapper = openMapper +) + const ( _IOC_WRITE = 1 _IOC_READ = 2 @@ -225,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 } @@ -240,7 +245,7 @@ func CreateDevice(name string, flags CreateFlags, targets []Target) (_ string, e } defer func() { if err != nil { - removeDevice(f, name) + removeDeviceWrapper(f, name) } }() @@ -273,13 +278,13 @@ 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) (err error) { rm := func() error { - f, err := openMapper() + f, err := openMapperWrapper() if err != nil { return err } defer f.Close() os.Remove(path.Join("/dev/mapper", name)) - return removeDevice(f, name) + return removeDeviceWrapper(f, name) } // This is workaround for "device or resource busy" error, which occasionally happens after the device mapper diff --git a/internal/guest/storage/devicemapper/devicemapper_test.go b/internal/guest/storage/devicemapper/devicemapper_test.go index 723fe1496a..cbd1f6cbba 100644 --- a/internal/guest/storage/devicemapper/devicemapper_test.go +++ b/internal/guest/storage/devicemapper/devicemapper_test.go @@ -4,6 +4,8 @@ package devicemapper import ( "flag" + "fmt" + "io/ioutil" "os" "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,39 @@ func TestLinearError(t *testing.T) { t.Fatal(err) } } + +func TestRemoveDeviceRetries(t *testing.T) { + clearTestDependencies() + + rmDeviceCalled := false + retryCalled := false + // Overrides openMapper to return temp file handle + openMapperWrapper = func() (*os.File, error) { + tmpFile, err := ioutil.TempFile("", "") + if err != nil { + return nil, err + } + return tmpFile, nil + } + removeDeviceWrapper = func(_ *os.File, name string) error { + if !rmDeviceCalled { + rmDeviceCalled = true + return fmt.Errorf("device-mapper device remove: device or resource busy") + } + if !retryCalled { + retryCalled = 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 !retryCalled { + t.Fatalf("expected removeDevice to be retried after initial failure") + } +} From 6f4a80009a9cf219b30a5cef2a933942140e225d Mon Sep 17 00:00:00 2001 From: Maksim An Date: Mon, 1 Nov 2021 16:09:31 -0700 Subject: [PATCH 5/7] pr feedback: use syscall.EBUSY instead of string cmp Signed-off-by: Maksim An --- internal/guest/storage/devicemapper/devicemapper.go | 4 ++-- internal/guest/storage/devicemapper/devicemapper_test.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/guest/storage/devicemapper/devicemapper.go b/internal/guest/storage/devicemapper/devicemapper.go index 2737517df4..44130a6d90 100644 --- a/internal/guest/storage/devicemapper/devicemapper.go +++ b/internal/guest/storage/devicemapper/devicemapper.go @@ -6,7 +6,7 @@ import ( "fmt" "os" "path" - "strings" + "syscall" "time" "unsafe" @@ -290,7 +290,7 @@ func RemoveDevice(name string) (err error) { // 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 && strings.Contains(err.Error(), "device or resource busy") { + if err = rm(); err != nil && err == syscall.EBUSY { time.Sleep(10 * time.Millisecond) continue } diff --git a/internal/guest/storage/devicemapper/devicemapper_test.go b/internal/guest/storage/devicemapper/devicemapper_test.go index cbd1f6cbba..8cb975230a 100644 --- a/internal/guest/storage/devicemapper/devicemapper_test.go +++ b/internal/guest/storage/devicemapper/devicemapper_test.go @@ -4,9 +4,9 @@ package devicemapper import ( "flag" - "fmt" "io/ioutil" "os" + "syscall" "testing" "unsafe" @@ -170,7 +170,7 @@ func TestRemoveDeviceRetries(t *testing.T) { removeDeviceWrapper = func(_ *os.File, name string) error { if !rmDeviceCalled { rmDeviceCalled = true - return fmt.Errorf("device-mapper device remove: device or resource busy") + return syscall.EBUSY } if !retryCalled { retryCalled = true From 44aa18c7f5073464f8fe6ab8e485db472ca26ca7 Mon Sep 17 00:00:00 2001 From: Maksim An Date: Mon, 1 Nov 2021 17:03:51 -0700 Subject: [PATCH 6/7] pr feedback: remove redundant error check and add another test Signed-off-by: Maksim An --- .../storage/devicemapper/devicemapper.go | 2 +- .../storage/devicemapper/devicemapper_test.go | 51 +++++++++++++++---- 2 files changed, 41 insertions(+), 12 deletions(-) diff --git a/internal/guest/storage/devicemapper/devicemapper.go b/internal/guest/storage/devicemapper/devicemapper.go index 44130a6d90..77835a5824 100644 --- a/internal/guest/storage/devicemapper/devicemapper.go +++ b/internal/guest/storage/devicemapper/devicemapper.go @@ -290,7 +290,7 @@ func RemoveDevice(name string) (err error) { // 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 && err == syscall.EBUSY { + if err = rm(); err == syscall.EBUSY { time.Sleep(10 * time.Millisecond) continue } diff --git a/internal/guest/storage/devicemapper/devicemapper_test.go b/internal/guest/storage/devicemapper/devicemapper_test.go index 8cb975230a..3483cb026b 100644 --- a/internal/guest/storage/devicemapper/devicemapper_test.go +++ b/internal/guest/storage/devicemapper/devicemapper_test.go @@ -4,6 +4,7 @@ package devicemapper import ( "flag" + "fmt" "io/ioutil" "os" "syscall" @@ -154,26 +155,22 @@ func TestLinearError(t *testing.T) { } } -func TestRemoveDeviceRetries(t *testing.T) { +func TestRemoveDeviceRetriesOnSyscallEBUSY(t *testing.T) { clearTestDependencies() rmDeviceCalled := false - retryCalled := false + retryDone := false // Overrides openMapper to return temp file handle openMapperWrapper = func() (*os.File, error) { - tmpFile, err := ioutil.TempFile("", "") - if err != nil { - return nil, err - } - return tmpFile, nil + return ioutil.TempFile("", "") } - removeDeviceWrapper = func(_ *os.File, name string) error { + removeDeviceWrapper = func(_ *os.File, _ string) error { if !rmDeviceCalled { rmDeviceCalled = true return syscall.EBUSY } - if !retryCalled { - retryCalled = true + if !retryDone { + retryDone = true return nil } return nil @@ -185,7 +182,39 @@ func TestRemoveDeviceRetries(t *testing.T) { if !rmDeviceCalled { t.Fatalf("expected removeDevice to be called at least once") } - if !retryCalled { + if !retryDone { t.Fatalf("expected removeDevice to be retried after initial failure") } } + +func TestRemoveDeviceFailsOnNonSyscallEBUSY(t *testing.T) { + clearTestDependencies() + + expectedError := fmt.Errorf("non syscall.BUSY error") + 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") + } +} From fd955b3b59ca5cafb88b23de9665fecd14e199da Mon Sep 17 00:00:00 2001 From: Maksim An Date: Tue, 2 Nov 2021 09:18:55 -0700 Subject: [PATCH 7/7] pr feedback: cast to dmError and update tests Signed-off-by: Maksim An --- internal/guest/storage/devicemapper/devicemapper.go | 5 ++++- .../guest/storage/devicemapper/devicemapper_test.go | 11 ++++++++--- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/internal/guest/storage/devicemapper/devicemapper.go b/internal/guest/storage/devicemapper/devicemapper.go index 77835a5824..c7157fe445 100644 --- a/internal/guest/storage/devicemapper/devicemapper.go +++ b/internal/guest/storage/devicemapper/devicemapper.go @@ -290,7 +290,10 @@ func RemoveDevice(name string) (err error) { // 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 == syscall.EBUSY { + if err = rm(); err != nil { + if e, ok := err.(*dmError); !ok || e.Err != syscall.EBUSY { + break + } time.Sleep(10 * time.Millisecond) continue } diff --git a/internal/guest/storage/devicemapper/devicemapper_test.go b/internal/guest/storage/devicemapper/devicemapper_test.go index 3483cb026b..278345b484 100644 --- a/internal/guest/storage/devicemapper/devicemapper_test.go +++ b/internal/guest/storage/devicemapper/devicemapper_test.go @@ -4,7 +4,6 @@ package devicemapper import ( "flag" - "fmt" "io/ioutil" "os" "syscall" @@ -167,7 +166,10 @@ func TestRemoveDeviceRetriesOnSyscallEBUSY(t *testing.T) { removeDeviceWrapper = func(_ *os.File, _ string) error { if !rmDeviceCalled { rmDeviceCalled = true - return syscall.EBUSY + return &dmError{ + Op: 1, + Err: syscall.EBUSY, + } } if !retryDone { retryDone = true @@ -190,7 +192,10 @@ func TestRemoveDeviceRetriesOnSyscallEBUSY(t *testing.T) { func TestRemoveDeviceFailsOnNonSyscallEBUSY(t *testing.T) { clearTestDependencies() - expectedError := fmt.Errorf("non syscall.BUSY error") + expectedError := &dmError{ + Op: 0, + Err: syscall.EACCES, + } rmDeviceCalled := false retryDone := false openMapperWrapper = func() (*os.File, error) {