Merge pull request #4437 from kzys/fix-rollback

snapshots/devmapper: fix rollback

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

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.
+			retErr = p.rollbackActivate(ctx, info, activeErr)
-			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
-			}
-
 			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.
+			retErr = p.rollbackActivate(ctx, snapInfo, activeErr)
-			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
-			}
-
 			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)
 	}

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 {