From a1f6c9dd881ec2205cac54a7296557e39cba826a Mon Sep 17 00:00:00 2001 From: Kazuyoshi Kato Date: Thu, 30 Jul 2020 16:28:36 -0700 Subject: [PATCH] snapshots/devmapper: fix rollback The rollback mechanism is implemented by calling deleteDevice() and RemoveDevice(). But RemoveDevice() is internally calling deleteDevice() as well. Since a device will be deleted by first deleteDevice(), RemoveDevice() always will see ENODATA. The specific error must be ignored to remove the device's metadata correctly. Signed-off-by: Kazuyoshi Kato --- snapshots/devmapper/metadata.go | 2 +- snapshots/devmapper/pool_device.go | 74 ++++++++++++++----------- snapshots/devmapper/pool_device_test.go | 24 ++++++++ 3 files changed, 67 insertions(+), 33 deletions(-) diff --git a/snapshots/devmapper/metadata.go b/snapshots/devmapper/metadata.go index 65760f142..01cdb7970 100644 --- a/snapshots/devmapper/metadata.go +++ b/snapshots/devmapper/metadata.go @@ -101,7 +101,7 @@ func (m *PoolMetadata) AddDevice(ctx context.Context, info *DeviceInfo) error { // See https://github.com/containerd/containerd/pull/3436 for more context. var existing DeviceInfo if err := getObject(devicesBucket, info.Name, &existing); err == nil && existing.State != Faulty { - return ErrAlreadyExists + return errors.Wrapf(ErrAlreadyExists, "device %q is already there %+v", info.Name, existing) } // Find next available device ID diff --git a/snapshots/devmapper/pool_device.go b/snapshots/devmapper/pool_device.go index 77ad456c1..b34695603 100644 --- a/snapshots/devmapper/pool_device.go +++ b/snapshots/devmapper/pool_device.go @@ -162,7 +162,22 @@ func (p *PoolDevice) transition(ctx context.Context, deviceName string, tryingSt result = multierror.Append(result, uerr) } - return result.ErrorOrNil() + return unwrapError(result) +} + +// unwrapError converts multierror.Error to the original error when it is possible. +// multierror 1.1.0 has the similar function named Unwrap, but it requires Go 1.14. +func unwrapError(e *multierror.Error) error { + if e == nil { + return nil + } + + // If the error can be expressed without multierror, return the original error. + if len(e.Errors) == 1 { + return e.Errors[0] + } + + return e.ErrorOrNil() } // CreateThinDevice creates new devmapper thin-device with given name and size. @@ -184,21 +199,7 @@ func (p *PoolDevice) CreateThinDevice(ctx context.Context, deviceName string, vi defer func() { // We've created a devmapper device, but failed to activate it, try rollback everything if activeErr != nil { - // Delete the device first. - delErr := p.deleteDevice(ctx, info) - if delErr != nil { - // Failed to rollback, mark the device as faulty and keep metadata in order to - // preserve the faulty device ID - retErr = multierror.Append(retErr, delErr, p.metadata.MarkFaulty(ctx, info.Name)) - return - } - - // The devmapper device has been successfully deleted, deallocate device ID - if err := p.RemoveDevice(ctx, info.Name); err != nil { - retErr = multierror.Append(retErr, err) - return - } - + retErr = p.rollbackActivate(ctx, info, activeErr) return } @@ -230,6 +231,23 @@ func (p *PoolDevice) CreateThinDevice(ctx context.Context, deviceName string, vi return nil } +func (p *PoolDevice) rollbackActivate(ctx context.Context, info *DeviceInfo, activateErr error) error { + // Delete the device first. + delErr := p.deleteDevice(ctx, info) + if delErr != nil { + // Failed to rollback, mark the device as faulty and keep metadata in order to + // preserve the faulty device ID + return multierror.Append(activateErr, delErr, p.metadata.MarkFaulty(ctx, info.Name)) + } + + // The devmapper device has been successfully deleted, deallocate device ID + if err := p.RemoveDevice(ctx, info.Name); err != nil { + return multierror.Append(activateErr, err) + } + + return activateErr +} + // 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 { @@ -275,21 +293,7 @@ func (p *PoolDevice) CreateSnapshotDevice(ctx context.Context, deviceName string defer func() { // We've created a devmapper device, but failed to activate it, try rollback everything if activeErr != nil { - // Delete the device first. - delErr := p.deleteDevice(ctx, snapInfo) - if delErr != nil { - // Failed to rollback, mark the device as faulty and keep metadata in order to - // preserve the faulty device ID - retErr = multierror.Append(retErr, delErr, p.metadata.MarkFaulty(ctx, snapInfo.Name)) - return - } - - // The devmapper device has been successfully deleted, deallocate device ID - if err := p.RemoveDevice(ctx, snapInfo.Name); err != nil { - retErr = multierror.Append(retErr, err) - return - } - + retErr = p.rollbackActivate(ctx, snapInfo, activeErr) return } @@ -490,7 +494,13 @@ func (p *PoolDevice) RemoveDevice(ctx context.Context, deviceName string) error 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) + e := dmsetup.DeleteDevice(p.poolName, info.DeviceID) + + // Ignores the error if the device has been deleted already. + if e != nil && !errors.Is(e, unix.ENODATA) { + return e + } + return nil }); err != nil { return errors.Wrapf(err, "failed to delete device %q (dev id: %d)", info.Name, info.DeviceID) } diff --git a/snapshots/devmapper/pool_device_test.go b/snapshots/devmapper/pool_device_test.go index 7ee7301ef..cc729a3a0 100644 --- a/snapshots/devmapper/pool_device_test.go +++ b/snapshots/devmapper/pool_device_test.go @@ -152,6 +152,27 @@ func TestPoolDevice(t *testing.T) { t.Run("RemoveDevice", func(t *testing.T) { testRemoveThinDevice(t, pool) }) + + t.Run("rollbackActivate", func(t *testing.T) { + testCreateThinDevice(t, pool) + + ctx := context.Background() + + snapDevice := "snap2" + + err := pool.CreateSnapshotDevice(ctx, thinDevice1, snapDevice, device1Size) + assert.NilError(t, err) + + info, err := pool.metadata.GetDevice(ctx, snapDevice) + assert.NilError(t, err) + + // Simulate a case that the device cannot be activated. + err = pool.DeactivateDevice(ctx, info.Name, false, false) + assert.NilError(t, err) + + err = pool.rollbackActivate(ctx, info, err) + assert.NilError(t, err) + }) } func TestPoolDeviceMarkFaulty(t *testing.T) { @@ -256,6 +277,9 @@ func testDeactivateThinDevice(t *testing.T, pool *PoolDevice) { func testRemoveThinDevice(t *testing.T, pool *PoolDevice) { err := pool.RemoveDevice(testCtx, thinDevice1) assert.NilError(t, err, "should delete thin device from pool") + + err = pool.RemoveDevice(testCtx, thinDevice2) + assert.NilError(t, err, "should delete thin device from pool") } func getMounts(thinDeviceName string) []mount.Mount {