Bug fix for mount path handling by ambarve · Pull Request #6651 · containerd/containerd

ambarve · 2022-03-09T00:29:55Z

Currently when handling 'container_path' elements in container mounts we simply call
filepath.Clean on those paths. However, filepath.Clean adds an extra '.' if the path is a
simple drive letter ('E:' or 'Z:' etc.). Similarly, a drive path like Z:\ is also kept as
it is with an ending ''. Either of these type of paths cause failures (with incorrect
parameter error) when creating containers via hcsshim. This commit correctly removes the
'.' or '' at the end of the paths.

Signed-off-by: Amit Barve [email protected]

k8s-ci-robot · 2022-03-09T00:30:04Z

Hi @ambarve. 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.

Details

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.

ambarve · 2022-03-09T00:30:34Z

@kevpar Please take a look.

ambarve · 2022-03-09T00:31:38Z

Fixes #6589

kzys · 2022-03-09T00:44:02Z

Is C:\ different from C: universally in Windows? Should we wrap filepath.Clean to be more Windows-friendly?

theopenlab-ci · 2022-03-09T01:26:41Z

Build succeeded.

containerd-build-arm64 : RETRY_LIMIT in 3m 39s (non-voting)
containerd-test-arm64 : RETRY_LIMIT in 6m 30s (non-voting)
containerd-integration-test-arm64 : RETRY_LIMIT in 28m 38s (non-voting)

kevpar · 2022-03-09T06:02:35Z

Is C:\ different from C: universally in Windows? I wonder whether we should wrap filepath.Clean or not.

Yes, these are different. C:\ represents the "root directory" of the C drive, whereas C: can have two different meanings based on context:

It could mean we are talking about the C drive itself. This is the desired meaning here, where we are mapping a directory into the container as a drive that doesn't currently exist.
To indicate the working directory on the C drive. To understand this, we need to know that for various weird historical reasons, Windows has (at least in some places) support for actually tracking a working directory per drive. In this case the drive letter and colon can be used to refer to a path relative to that drive's working directory. So for instance D:foo\bar.txt refers to foo\bar.txt relative to the D drive's working directory.
Presumably for compatibility, Go seems to have carried over this behavior into filepath.Clean. In this case it is converting D: into D:., which is correct when you consider how this form is used for a relative path.

For our case here, we don't care about relative paths for mounts, and we want to treat the drive letter as a special case, because it allows us to mount a directory as a new drive in the container.

Maybe an argument could be made that we should also support D:\ to mount to a new drive. I'm not sure if that's a good idea or not.

theopenlab-ci · 2022-03-09T07:27:27Z

Build succeeded.

containerd-build-arm64 : RETRY_LIMIT in 3m 46s (non-voting)
containerd-test-arm64 : RETRY_LIMIT in 5m 46s (non-voting)
containerd-integration-test-arm64 : RETRY_LIMIT in 26m 02s (non-voting)

kzys

The change looks good. Can you squash the commits into one?

jterry75 · 2022-03-09T18:27:43Z

I'm confused. I'm sure you tested this but I thought a Windows mount had to be to a nonexistent folder? I didn't know you could do:

C:\Path\On\Host -> C: (in the container)

Does it just place the files from Host alongside whatever is already present in C:? How are conflicts resolved?

ambarve · 2022-03-09T19:12:51Z

@jterry75 Good point. Providing C: as the destination path fails (with invalid data error) because that's not supported (not sure if there is even a way to properly support it) but we don't have a check for that here. I will add that check.
AFAIK containers don't start with other drives and so this question won't arise for other drives.

theopenlab-ci · 2022-03-09T20:42:25Z

Build succeeded.

containerd-build-arm64 : RETRY_LIMIT in 3m 44s (non-voting)
containerd-test-arm64 : RETRY_LIMIT in 5m 47s (non-voting)
containerd-integration-test-arm64 : RETRY_LIMIT in 25m 46s (non-voting)

theopenlab-ci · 2022-03-16T07:17:34Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 4m 31s (non-voting)
containerd-test-arm64 : FAILURE in 5m 41s (non-voting)
containerd-integration-test-arm64 : SUCCESS in 25m 53s (non-voting)

theopenlab-ci · 2022-03-18T12:29:12Z

Merge Failed.

This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset.

theopenlab-ci · 2022-03-18T13:06:57Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 4m 43s (non-voting)
containerd-test-arm64 : SUCCESS in 5m 36s (non-voting)
containerd-integration-test-arm64 : FAILURE in 30m 05s (non-voting)

theopenlab-ci · 2022-03-18T13:48:19Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 4m 25s (non-voting)
containerd-test-arm64 : SUCCESS in 5m 39s (non-voting)
containerd-integration-test-arm64 : SUCCESS in 25m 38s (non-voting)

jterry75

LGTM. The linter looks like its static checks on unchanged files.

kzys · 2022-03-18T21:18:58Z

Rebasing against main should fix the linter issue.

ambarve · 2022-03-18T21:30:41Z

@kzys & @jterry75 actually I moved the entire dst path check out of the named pipe if condition since those checks can & should happen regardless of whether src is a pipe or not. I also rebased on master. PTAL and feel free to merge it if it looks good.

jterry75 · 2022-03-18T22:46:33Z

Looks like the pipe mount test is what failed. Can you take a look?

theopenlab-ci · 2022-03-18T23:55:22Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 4m 26s (non-voting)
containerd-test-arm64 : SUCCESS in 5m 58s (non-voting)
containerd-integration-test-arm64 : SUCCESS in 25m 44s (non-voting)

theopenlab-ci · 2022-03-19T01:26:17Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 4m 34s (non-voting)
containerd-test-arm64 : SUCCESS in 5m 48s (non-voting)
containerd-integration-test-arm64 : FAILURE in 26m 59s (non-voting)

Currently when handling 'container_path' elements in container mounts we simply call filepath.Clean on those paths. However, filepath.Clean adds an extra '.' if the path is a simple drive letter ('E:' or 'Z:' etc.). These type of paths cause failures (with incorrect parameter error) when creating containers via hcsshim. This commit checks for such paths and doesn't call filepath.Clean on them. It also adds a new check to error out if the destination path is a C drive and moves the dst path checks out of the named pipe condition. Signed-off-by: Amit Barve <[email protected]>

ambarve · 2022-03-21T17:43:50Z

@kevpar, @jterry75, @kzys looks like all the tests are passing now (I fixed the issue). Feel free to merge this if it looks good.

kzys

LGTM!

theopenlab-ci · 2022-03-21T18:12:49Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 10m 29s (non-voting)
containerd-test-arm64 : SUCCESS in 7m 02s (non-voting)
containerd-integration-test-arm64 : SUCCESS in 27m 08s (non-voting)

jterry75

LGTM

estesp

LGTM

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

kevpar reviewed Mar 9, 2022

View reviewed changes

Comment thread pkg/cri/opts/spec_windows.go Outdated

kzys reviewed Mar 9, 2022

View reviewed changes

ambarve force-pushed the mount_path_fix branch 2 times, most recently from cd3aca0 to f70fcbd Compare March 9, 2022 19:48

kzys reviewed Mar 9, 2022

View reviewed changes

Comment thread pkg/cri/opts/spec_windows.go Outdated

ambarve force-pushed the mount_path_fix branch from f70fcbd to 814fcc3 Compare March 16, 2022 06:49

jterry75 reviewed Mar 16, 2022

View reviewed changes

Comment thread pkg/cri/opts/spec_test.go Outdated

jterry75 reviewed Mar 16, 2022

View reviewed changes

Comment thread pkg/cri/opts/spec_test.go Outdated

jterry75 reviewed Mar 16, 2022

View reviewed changes

Comment thread pkg/cri/opts/spec_windows.go

TBBle mentioned this pull request Mar 17, 2022

mountPath behavior changed from docker to containerd on Windows #6589

Closed

ambarve force-pushed the mount_path_fix branch from 814fcc3 to 1e4b1da Compare March 18, 2022 12:28

ambarve force-pushed the mount_path_fix branch from 1e4b1da to 4e739a6 Compare March 18, 2022 12:34

ambarve force-pushed the mount_path_fix branch from 4e739a6 to e387bc3 Compare March 18, 2022 13:20

jterry75 approved these changes Mar 18, 2022

View reviewed changes

ambarve force-pushed the mount_path_fix branch from e387bc3 to 0dd621a Compare March 18, 2022 21:29

ambarve force-pushed the mount_path_fix branch from 0dd621a to 369fe00 Compare March 18, 2022 21:38

kzys approved these changes Mar 18, 2022

View reviewed changes

ambarve force-pushed the mount_path_fix branch from 369fe00 to 17613da Compare March 19, 2022 00:57

ambarve force-pushed the mount_path_fix branch from 17613da to bfde58e Compare March 21, 2022 16:40

kzys approved these changes Mar 21, 2022

View reviewed changes

jterry75 approved these changes Mar 22, 2022

View reviewed changes

estesp approved these changes Mar 23, 2022

View reviewed changes

estesp merged commit b06938c into containerd:main Mar 23, 2022

estesp mentioned this pull request May 11, 2022

[release/1.6] Bug fix for mount path handling #6929

Merged

estesp added cherry-picked/1.6.x PR commits are cherry-picked into release/1.6 branch and removed needs-ok-to-test labels May 11, 2022

junnplus mentioned this pull request Jun 25, 2022

Unable to mount volumes or bind mount on Windows containerd/nerdctl#759

Closed

Conversation

ambarve commented Mar 9, 2022

Uh oh!

k8s-ci-robot commented Mar 9, 2022

Uh oh!

ambarve commented Mar 9, 2022

Uh oh!

ambarve commented Mar 9, 2022

Uh oh!

Uh oh!

kzys commented Mar 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theopenlab-ci Bot commented Mar 9, 2022

Uh oh!

kevpar commented Mar 9, 2022

Uh oh!

theopenlab-ci Bot commented Mar 9, 2022

Uh oh!

kzys left a comment

Choose a reason for hiding this comment

Uh oh!

jterry75 commented Mar 9, 2022

Uh oh!

ambarve commented Mar 9, 2022

Uh oh!

theopenlab-ci Bot commented Mar 9, 2022

Uh oh!

Uh oh!

theopenlab-ci Bot commented Mar 16, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

theopenlab-ci Bot commented Mar 18, 2022

Uh oh!

theopenlab-ci Bot commented Mar 18, 2022

Uh oh!

theopenlab-ci Bot commented Mar 18, 2022

Uh oh!

jterry75 left a comment

Choose a reason for hiding this comment

Uh oh!

kzys commented Mar 18, 2022

Uh oh!

ambarve commented Mar 18, 2022

Uh oh!

jterry75 commented Mar 18, 2022

Uh oh!

theopenlab-ci Bot commented Mar 18, 2022

Uh oh!

theopenlab-ci Bot commented Mar 19, 2022

Uh oh!

ambarve commented Mar 21, 2022

Uh oh!

kzys left a comment

Choose a reason for hiding this comment

Uh oh!

theopenlab-ci Bot commented Mar 21, 2022

Uh oh!

jterry75 left a comment

Choose a reason for hiding this comment

Uh oh!

estesp left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

kzys commented Mar 9, 2022 •

edited

Loading