Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fixed volume mount issue(759) for Windows #902

Closed
wants to merge 6 commits into from
Closed

Fixed volume mount issue(759) for Windows #902

wants to merge 6 commits into from

Conversation

click2cloud-lamda
Copy link
Contributor

@click2cloud-lamda click2cloud-lamda commented Mar 16, 2022 •
edited

Fixed issue #759 :

  • volume binding issue in Windows containers
D:\GolandProjects\github.com\nerdctl2\cmd\nerdctl>nerdctl run -it -v C:\ProgramData\nerdctl\052055e3\volumes\default\test\_data:c:\src:rw,rprivate mcr.microsoft.com/windows/nanoserver:20H2
Microsoft Windows [Version 10.0.19042.1586]
(c) 2019 Microsoft Corporation. All rights reserved.

C:\Windows\system32>cd ../..

C:\>dir
 Volume in drive C has no label.
 Volume Serial Number is 7643-AF54

 Directory of C:\

03/03/2022  01:27 PM             5,510 License.txt
03/16/2022  07:00 PM    <DIR>          src
03/16/2022  07:50 PM    <DIR>          Users
03/16/2022  07:50 PM    <DIR>          Windows
               1 File(s)          5,510 bytes
               3 Dir(s)  21,302,005,760 bytes free

C:\>

When using nerdctl + containerd on Windows, I am able to mount volumes or bind mount to a new windows container and are also able to use bind propagation.

Signed-off-by: Raymond Mathew click2cloud-lamda@click2cloud.net

@jsturtevant jsturtevant added the platform/Windows/Non-WSL2 Microsoft Windows (non-WSL2) label Mar 16, 2022
@jsturtevant
Copy link
Contributor

@click2cloud-lamda could you sign off on commits? https://github.com/containerd/project/blob/main/CONTRIBUTING.md#sign-your-work

@jsturtevant
Copy link
Contributor

The windows build failure looks like it was maybe a flake in cirrus?

error obtaining VCS status: exec: "git": executable file not found in %PATH%
	Use -buildvcs=false to disable VCS stamping.

jsturtevant
jsturtevant reviewed Mar 16, 2022
View reviewed changes
cmd/nerdctl/run_mount_windows_test.go
"-v", fmt.Sprintf("%s:C:\\mnt3", rwVolName),
"-v", fmt.Sprintf("%s:C:\\mnt4ro", roVolName),
testutil.WindowsNano).AssertOK()
base.Cmd("container", "inspect", containerName).AssertOK()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This validates the container started ok but should we validate the mounts are in the right place?

cmd/nerdctl/run_mount_windows_test.go
base.Cmd("run",
"-d",
"--name", containerName,
"-v", fmt.Sprintf("%s:C:\\mnt1", rwDir),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a test for the left side having c:\?

jsturtevant
jsturtevant reviewed Mar 16, 2022
View reviewed changes
pkg/mountutil/mountutil.go Outdated
// assume src is a volume name
vol, err := volStore.Get(src)

if runtime.GOOS == "windows" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a good place to have separate files (_windows.go and _linux.go) for each implementation of the large nested if statement.

jsturtevant
jsturtevant reviewed Mar 16, 2022
View reviewed changes
pkg/mountutil/mountutil.go Outdated
fstype := "nullfs"
if runtime.GOOS != "freebsd" {
fstype = "none"
fstype = ""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this being dropped?

jsturtevant
jsturtevant reviewed Mar 16, 2022
View reviewed changes
pkg/mountutil/mountutil_windows.go Outdated
@@ -38,31 +41,195 @@ const DefaultPropagationMode = ""

// parseVolumeOptions parses specified optsRaw with using information of
// the volume type and the src directory when necessary.
//func parseVolumeOptions(vType, src, optsRaw string) ([]string, []oci.SpecOpts, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove unused code.

jsturtevant
jsturtevant reviewed Mar 16, 2022
View reviewed changes
pkg/mountutil/mountutil_windows.go Outdated
// - OCI Runtime Spec: https://github.com/opencontainers/runtime-spec/blob/v1.0.2/config-linux.md#rootfs-mount-propagation
// - runc implementation: https://github.com/opencontainers/runc/blob/v1.0.0/libcontainer/rootfs_linux.go#L771-L777
specOpts = append(specOpts, func(ctx context.Context, cli oci.Client, c *containers.Container, s *oci.Spec) error {
switch s.Linux.RootfsPropagation {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is there linux specific implementaitons details in the _windows.go file?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this copied from somewhere else? please leave attribution if so

jsturtevant
jsturtevant reviewed Mar 16, 2022
View reviewed changes
Copy link
Contributor

@jsturtevant jsturtevant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for looking at this. Left a few comments.

@AkihiroSuda
Copy link
Member

Please add DCO sign
https://github.com/apps/dco

@AkihiroSuda AkihiroSuda added the status/needs-DCO-sign Needs DCO sign (the “Signed-off-by” in the commit message) label Mar 18, 2022
@click2cloud-lamda
Fixed volume mount issue(issue 759) for Windows
340624d 
Signed-off-by: click2cloud-lamda <click2cloud-lamda@click2cloud.net>
@click2cloud-lamda
Merge remote-tracking branch 'origin/master'
4dea2b6 
# Conflicts:
#	pkg/mountutil/mountutil.go
#	pkg/mountutil/mountutil_windows.go
@AkihiroSuda
Copy link
Member

Signed-off-by: click2cloud-lamda

Please use real name

@click2cloud-lamda click2cloud-lamda changed the title Fixed volume mount issue(759) and bind propagation for Windows Fixed volume mount issue(759) for Windows Mar 23, 2022
@click2cloud-lamda
Added Mounts parameter for Container Inspect
1036a5a 
Signed-off-by: Raymond Mathew <click2cloud-lamda@click2cloud.net>

Signed-off-by: click2cloud-lamda <click2cloud-lamda@click2cloud.net>
@fahedouch fahedouch mentioned this pull request Nov 2, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsturtevant jsturtevant jsturtevant left review comments

Assignees
No one assigned
Labels
platform/Windows/Non-WSL2 Microsoft Windows (non-WSL2) status/needs-DCO-sign Needs DCO sign (the “Signed-off-by” in the commit message)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@click2cloud-lamda @jsturtevant @AkihiroSuda @click2cloud-Nagaraj