Two xfs file systems with same UUID can not be mounted on the same sy… #6650

henry118 · 2022-03-08T23:32:19Z

Fixes issue: #6587

Two xfs file systems with same UUID can not be mounted on the same system. However devmapper snapshots will have same UUID as original filesystem.

This patch fixes the bug by mounting a xfs file system with "nouuid" option.

Signed-off-by: Henry Wang henwang@amazon.com

k8s-ci-robot · 2022-03-08T23:32:28Z

Hi @henry118. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

kzys

The change looks good to me.

Is it possible to reproduce the issue by expanding https://github.com/containerd/containerd/blob/main/snapshots/devmapper/snapshotter_test.go? Does GitHub Actions have mkfs.xfs?

theopenlab-ci · 2022-03-09T00:33:14Z

Build succeeded.

containerd-build-arm64 : RETRY_LIMIT in 3m 37s (non-voting)
containerd-test-arm64 : RETRY_LIMIT in 5m 40s (non-voting)
containerd-integration-test-arm64 : RETRY_LIMIT in 25m 02s (non-voting)

henry118 · 2022-03-09T00:59:41Z

The errors happens after mkfs, when a snapshot is actually mounted for multiple containers. I am not sure if there is a way to reproduce it with unit tests. Any suggestions?

kzys · 2022-03-09T01:03:00Z

If we make two snapshots with xfs and mount both of them, would it reproduce the error? A lot of containerd tests actually run syscalls such as mount.

henry118 · 2022-03-09T01:06:27Z

Can you point me some sample unit tests as you mentioned? so I can use them as references for implementing the new test cases... Thanks!

kzys · 2022-03-09T01:10:13Z

How about this one? I believe that mount here is not mocked.

containerd/snapshots/devmapper/snapshotter_test.go

Lines 126 to 130 in b521429

    
           mounts, err := snapshotter.Prepare(ctx, "prepare-2", "layer-1") 
        
           assert.NilError(t, err) 
        
           err = mount.WithTempMount(ctx, mounts, baseApplier.Apply) 
        
           assert.NilError(t, err)

henry118 · 2022-03-09T01:12:11Z

Great thanks, will look into it.

kzys · 2022-03-09T23:47:34Z

snapshots/devmapper/snapshotter_test.go

+
+	ctx := context.Background()
+	ctx = namespaces.WithNamespace(ctx, "testsuite")
+	tempDir, err := os.MkdirTemp("", "snapshot-suite-usage")


You can use t.TempDir instead

this logic was taken from https://github.com/containerd/containerd/blob/main/snapshots/devmapper/snapshotter_test.go#L89

I think it was chosen because we want to explicitly specify a pattern for the temp directory?

I believe, we simply didn't have t.TempDir when we first wrote that code. It is added in Go 1.15.

Probably. However i still believe the original logic works better. For instance we can easily locate the path when we have to debug a failing test case with an explicitly specified dir pattern. I actually found it useful when I wrote this unit test. But if you have a strong opinion about this I am fine with making the change.

kzys · 2022-03-09T23:48:25Z

snapshots/devmapper/snapshotter_test.go

+	var (
+		sizeBytes   int64 = 1048576 // 1MB
+		baseApplier       = fstest.Apply(fstest.CreateRandomFile("/a", 12345679, sizeBytes, 0777))
+	)


We don't need them.

Why? baseApplier is needed to create a snapshot.

I think (not 100% sure, sorry) that Prepare() returns a XFS-formatted devmapper device. So mounting two of them is enough to reproduce the original issue. Would it work?

Nope. we have to prepare an additional snapshot from a base one.

kzys · 2022-03-09T23:48:49Z

snapshots/devmapper/snapshotter_test.go

+	err := dmsetup.CreatePool(config.PoolName, loopDataDevice, loopMetaDevice, 64*1024/dmsetup.SectorSize)
+	assert.NilError(t, err, "failed to create pool %q", config.PoolName)
+
+	snap, err := NewSnapshotter(context.Background(), config)


How about taking the passed ctx instead of making a new one?

Sounds good. Updated

Two xfs file systems with same UUID can not be mounted on the same system. However devmapper snapshots will have same UUID as original filesystem. This patch fixes the bug by mounting a xfs file system with "nouuid" option. Signed-off-by: Henry Wang <henwang@amazon.com>

kzys

LGTM

/ok-to-test

theopenlab-ci · 2022-03-10T01:02:18Z

Build succeeded.

containerd-build-arm64 : RETRY_LIMIT in 3m 37s (non-voting)
containerd-test-arm64 : RETRY_LIMIT in 5m 41s (non-voting)
containerd-integration-test-arm64 : RETRY_LIMIT in 24m 27s (non-voting)

FFFrog · 2022-03-10T01:49:33Z

/ok-to-test

FFFrog · 2022-03-10T03:21:39Z

recheck

FFFrog · 2022-03-10T03:34:47Z

recheck

theopenlab-ci · 2022-03-10T03:48:44Z

Build succeeded.

containerd-build-arm64 : RETRY_LIMIT in 3m 56s (non-voting)
containerd-test-arm64 : FAILURE in 6m 03s (non-voting)
containerd-integration-test-arm64 : FAILURE in 25m 06s (non-voting)

kzys · 2022-03-10T19:37:57Z

@FFFrog /ok-to-test works only if the comment is made by the people in this containerd org. We don't support recheck.

estesp

LGTM

k8s-ci-robot added the needs-ok-to-test label Mar 8, 2022

henry118 force-pushed the xfs branch from fa4d86a to 747727d Compare March 8, 2022 23:35

kzys reviewed Mar 8, 2022

View reviewed changes

henry118 force-pushed the xfs branch from 747727d to e5130ea Compare March 9, 2022 23:30

kzys requested changes Mar 9, 2022

View reviewed changes

henry118 force-pushed the xfs branch from e5130ea to 0d0b2bd Compare March 10, 2022 00:05

kzys approved these changes Mar 10, 2022

View reviewed changes

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Mar 10, 2022

estesp approved these changes Mar 11, 2022

View reviewed changes

estesp merged commit e771443 into containerd:main Mar 11, 2022

henry118 deleted the xfs branch March 11, 2022 17:34

kzys mentioned this pull request Mar 11, 2022

devmapper + xfs - "Filesystem has duplicate UUID - can't mount" #6587

Closed

estesp added the cherry-pick/1.6.x Change to be cherry picked to release/1.6 branch label Mar 14, 2022

thaJeztah mentioned this pull request Mar 24, 2022

[release/1.6 backport] Mount devmapper xfs file system with "nouuid" option. #6731

Merged

thaJeztah removed the cherry-pick/1.6.x Change to be cherry picked to release/1.6 branch label Mar 24, 2022

thaJeztah added the cherry-picked/1.6.x PR commits are cherry-picked into release/1.6 branch label Mar 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Two xfs file systems with same UUID can not be mounted on the same sy… #6650

Two xfs file systems with same UUID can not be mounted on the same sy… #6650

Two xfs file systems with same UUID can not be mounted on the same sy… #6650

Two xfs file systems with same UUID can not be mounted on the same sy… #6650

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment