Better error recovery in device mapper #3470

mxpv · 2019-07-30T23:38:10Z

This PR attempts to mitigate some cases described in #3436

It introduces faulty state for devices and devmapper device ID.

A device that is failed to create/activate and failed to rollback (delete back), will be marked as faulty in metadata store (instead of cleaning up the metadata and ending up in an inconsistent state). The snapshotter will try to recreate this device with a different devID in a subsequent call. This would allow us to move on and prevent the snapshotter to entirely stuck on a first faulty ID.

A faulty device ID means that the snapshotter could not handle it (nor create nor rollback), and it needs manual handling. It'll be marked in metadata store and won't be reused by snapshotter until addressed and manually marked as ok.

@renzhengeek @jiazhiguang Could you please give it a try on your env.

Signed-off-by: Maksym Pavlenko <makpav@amazon.com>

theopenlab-ci · 2019-07-30T23:47:43Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 8m 31s (non-voting)

snapshots/devmapper/metadata.go

renzhengeek · 2019-07-31T09:49:31Z

snapshots/devmapper/pool_device.go

+	activeErr = p.activateDevice(ctx, info)
+	if activeErr != nil {
+		return activeErr
+	}



Hi,

What if crash happens in between these steps and rollback code doesn't get chance to run?

This possibility also applies to other places to change device or metadata, right?

What if crash happens in between these steps and rollback code doesn't get chance to run?

Each device mapper operation is wrapped with state transition function (have a look on 1 and 2). If we're failed to activate a device and crash happens, so no defer was run, you'll end up with something like this in your metadata database (meaning that the device didn't finish the transition):

DevName=X ID=9 State=Activating Error="failed to activate"

Essentially each operation with devmapper device is recorded (https://github.com/containerd/containerd/blob/master/snapshots/devmapper/device_info.go#L34), so you can get a clear picture about device state and last operation at any time. For example, for CreateThinDevice the following states will be used:

Unknown <-- preallocated metadata info and devID Creating <-- createDevice Created Activating <-- activateDevice Activated

Hi,

thanks for your detailed explanation ;-) Yes, I know, in the crash case, the metadata DB can reflect correctly the device state before crashing, such as creating, activating etc.

Actually, my question was, in that case, there is no chance to the rollback code to mark the device as faulty, right?

If so, after machine comes up and containerd get started again, we still have "object already exists" error from the AddDevice() - 878a320#diff-4132e94f99d36d21b09c446fb20687faL100.

If you crash right in between create and rollback, then yes, a device won't be marked as Faulty one. In this case, what behavior would you expect to see?

If a crash happens, containerd must restart again, so devmapper will be reloaded. Could we take the chance to scan the device list and mark the device as faulty who's state is not in a final state - Deactivated and Activated. Or I feel it's safe to cleanup that device from metadata and thin pool completely.

If you crash right in between create and rollback, then yes, a device won't be marked as Faulty one. In this case, what behavior would you expect to see?

Hi，

I just come up this idea. I'd like to know what do you think？thanks.

diff --git a/snapshots/devmapper/pool_device.go b/snapshots/devmapper/pool_device.go index 22e78c1..4eaab6e 100644 --- a/snapshots/devmapper/pool_device.go +++ b/snapshots/devmapper/pool_device.go @@ -150,6 +150,16 @@ func (p *PoolDevice) CreateThinDevice(ctx context.Context, deviceName string, vi retErr = multierror.Append(retErr, p.metadata.MarkFaulty(ctx, info.Name)) return } + + // If ErrAlreadyExists returns, it indicates the device name is already in metadata, + // but not aware by upper layer. This may occur when crash happens and there is no + // chance to mark the device faulty. So, mark it here and upper layer will retry with + // the same device name. + if metaErr == ErrAlreadyExists { + log.G(ctx).Warnf("conflicting thin device %s, mark it faulty", info.Name) + retErr = multierror.Append(retErr, p.metadata.MarkFaulty(ctx, info.Name)) + return + } }() // Save initial device metadata and allocate new device ID from store @@ -241,6 +251,12 @@ func (p *PoolDevice) CreateSnapshotDevice(ctx context.Context, deviceName string retErr = multierror.Append(retErr, p.metadata.MarkFaulty(ctx, snapInfo.Name)) return } + + if metaErr == ErrAlreadyExists { + log.G(ctx).Warnf("conflicting snapshot %s, mark it faulty", snapInfo.Name) + retErr = multierror.Append(retErr, p.metadata.MarkFaulty(ctx, snapInfo.Name)) + return + } }()

I've addressed this use case in #3489. In case of crash, the snapshotter will mark devices as faulty after restart. Can you check if it works for you?

I've addressed this use case in #3489. In case of crash, the snapshotter will mark devices as faulty after restart. Can you check if it works for you?

Great, thanks!

renzhengeek · 2019-07-31T09:54:49Z

Hmm,
this problem is a independent one, which once occurred and seems cannot be recovered.

37373c1

devmapper: deferred remove can break consistency

mxpv · 2019-07-31T18:25:39Z

this problem is a independent one, which once occurred and seems cannot be recovered.

This makes sense. devmapper needs more precise control when to do unmounts, and certainly that should happen before remove. Though I think we don't need needUmount check as it should work for all snapshotters.

Signed-off-by: Maksym Pavlenko <makpav@amazon.com>

theopenlab-ci · 2019-07-31T18:40:41Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 8m 20s (non-voting)

codecov-io · 2019-07-31T18:44:23Z

Codecov Report

Merging #3470 into master will decrease coverage by 0.02%.
The diff coverage is 47.29%.

@@            Coverage Diff             @@
##           master    #3470      +/-   ##
==========================================
- Coverage   44.24%   44.22%   -0.03%     
==========================================
  Files         124      124              
  Lines       13732    13780      +48     
==========================================
+ Hits         6076     6094      +18     
- Misses       6725     6747      +22     
- Partials      931      939       +8

Flag	Coverage Δ
#linux	`47.99% <47.29%> (-0.05%)`	⬇️
#windows	`39.86% <ø> (ø)`	⬆️

Impacted Files	Coverage Δ
snapshots/devmapper/device_info.go	`0% <0%> (ø)`	⬆️
snapshots/devmapper/pool_device.go	`55.22% <37.73%> (-6.18%)`	⬇️
snapshots/devmapper/metadata.go	`67.34% <78.94%> (+0.93%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a49df98...3741fd8. Read the comment docs.

renzhengeek · 2019-08-01T12:02:37Z

Though I think we don't need needUmount check as it should work for all snapshotters.

Yes, thanks.

samuelkarp · 2019-08-01T23:26:38Z

A faulty device ID means that the snapshotter could not handle it (nor create nor rollback), and it needs manual handling. It'll be marked in metadata store and won't be reused by snapshotter until addressed and manually marked as ok.

Do you plan to add some documentation as part of this PR for what a user can do to remediate the faulty device ID and to mark it as ok once it has been remediated? If not, is that planned for a follow-up PR?

renzhengeek · 2019-08-02T06:03:25Z

A faulty device ID means that the snapshotter could not handle it (nor create nor rollback), and it needs manual handling. It'll be marked in metadata store and won't be reused by snapshotter until addressed and manually marked as ok.

Do you plan to add some documentation as part of this PR for what a user can do to remediate the faulty device ID and to mark it as ok once it has been remediated? If not, is that planned for a follow-up PR?

Once it's correctly marked as faulty, I think it's safe to just delete those devices which are not aware by upper layer and contains user data.

mxpv · 2019-08-02T18:27:39Z

Do you plan to add some documentation as part of this PR for what a user can do to remediate the faulty device ID and to mark it as ok once it has been remediated? If not, is that planned for a follow-up PR?

@samuelkarp Yes, I plan to update the README in a follow up PR.

Could we take the chance to scan the device list and mark the device

@renzhengeek SGTM. At init time we can check devices that are not in final state and mark them faulty. However I'd prefer to play safe and leave any device deletion for user in order to prevent unexpected situations. Sometimes it's possible to restore a device instead of cleanup.

crosbymichael · 2019-08-05T16:56:17Z

LGTM

estesp

LGTM

estesp · 2019-08-05T17:10:40Z

Should any of this cleanup/err handling get taken back into release/1.2?

mxpv · 2019-08-05T18:14:43Z

@estesp no, this is devmapper specific err handling.

mxpv added 2 commits July 30, 2019 15:17

Better error recovery in devmapper

878a320

Signed-off-by: Maksym Pavlenko <makpav@amazon.com>

Mark faulty device in one transaction

4d5a0e1

Signed-off-by: Maksym Pavlenko <makpav@amazon.com>

renzhengeek reviewed Jul 31, 2019

View reviewed changes

snapshots/devmapper/metadata.go Show resolved Hide resolved

renzhengeek reviewed Jul 31, 2019

View reviewed changes

Remove deferred flag when removing devmapper device

3741fd8

Signed-off-by: Maksym Pavlenko <makpav@amazon.com>

mxpv force-pushed the devmapper_err branch from e02e771 to 3741fd8 Compare July 31, 2019 18:30

samuelkarp approved these changes Aug 2, 2019

View reviewed changes

estesp approved these changes Aug 5, 2019

View reviewed changes

estesp merged commit cb46663 into containerd:master Aug 5, 2019

mxpv deleted the devmapper_err branch August 5, 2019 18:15

mxpv mentioned this pull request Aug 5, 2019

Mark devices with invalid state as faulty #3489

Merged

renzhengeek mentioned this pull request Aug 7, 2019

Devmapper: consistency issue bugfixes #3436

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better error recovery in device mapper #3470

Better error recovery in device mapper #3470

mxpv commented Jul 30, 2019

theopenlab-ci bot commented Jul 30, 2019

renzhengeek Jul 31, 2019

mxpv Jul 31, 2019

mxpv Jul 31, 2019

renzhengeek Aug 1, 2019 •

edited

mxpv Aug 1, 2019

renzhengeek Aug 2, 2019

renzhengeek Aug 7, 2019 •

edited

mxpv Aug 7, 2019

renzhengeek Aug 7, 2019

renzhengeek commented Jul 31, 2019

mxpv commented Jul 31, 2019

theopenlab-ci bot commented Jul 31, 2019

codecov-io commented Jul 31, 2019 •

edited

renzhengeek commented Aug 1, 2019

samuelkarp commented Aug 1, 2019

renzhengeek commented Aug 2, 2019

mxpv commented Aug 2, 2019 •

edited

crosbymichael commented Aug 5, 2019

estesp left a comment

estesp commented Aug 5, 2019

mxpv commented Aug 5, 2019

Better error recovery in device mapper #3470

Better error recovery in device mapper #3470

Conversation

mxpv commented Jul 30, 2019

theopenlab-ci bot commented Jul 30, 2019

renzhengeek Jul 31, 2019

Choose a reason for hiding this comment

mxpv Jul 31, 2019

Choose a reason for hiding this comment

mxpv Jul 31, 2019

Choose a reason for hiding this comment

renzhengeek Aug 1, 2019 • edited

Choose a reason for hiding this comment

mxpv Aug 1, 2019

Choose a reason for hiding this comment

renzhengeek Aug 2, 2019

Choose a reason for hiding this comment

renzhengeek Aug 7, 2019 • edited

Choose a reason for hiding this comment

mxpv Aug 7, 2019

Choose a reason for hiding this comment

renzhengeek Aug 7, 2019

Choose a reason for hiding this comment

renzhengeek commented Jul 31, 2019

mxpv commented Jul 31, 2019

theopenlab-ci bot commented Jul 31, 2019

codecov-io commented Jul 31, 2019 • edited

Codecov Report

renzhengeek commented Aug 1, 2019

samuelkarp commented Aug 1, 2019

renzhengeek commented Aug 2, 2019

mxpv commented Aug 2, 2019 • edited

crosbymichael commented Aug 5, 2019

estesp left a comment

Choose a reason for hiding this comment

estesp commented Aug 5, 2019

mxpv commented Aug 5, 2019

renzhengeek Aug 1, 2019 •

edited

renzhengeek Aug 7, 2019 •

edited

codecov-io commented Jul 31, 2019 •

edited

mxpv commented Aug 2, 2019 •

edited