| CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Bug fix for mount path handling #6651
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
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 Once the patch is verified, the new status will be reflected by the 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. |
|
@kevpar Please take a look. |
|
Fixes #6589 |
|
Is |
|
Build succeeded.
|
Yes, these are different.
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 |
|
Build succeeded.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change looks good. Can you squash the commits into one?
|
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 |
|
@jterry75 Good point. Providing |
ambarve
force-pushed
the
mount_path_fix
branch
2 times, most recently
from
cd3aca0 to
f70fcbd
Compare
|
Build succeeded.
|
|
Build succeeded.
|
|
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. |
|
Build succeeded.
|
|
Build succeeded.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. The linter looks like its static checks on unchanged files.
|
Rebasing against main should fix the linter issue. |
|
Looks like the pipe mount test is what failed. Can you take a look? |
|
Build succeeded.
|
|
Build succeeded.
|
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 <ambarve@microsoft.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
|
Build succeeded.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
estesp
added
cherry-picked/1.6.x
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 ambarve@microsoft.com