Fixed volume mount issue(759) for Windows #924

click2cloud-lamda · 2022-03-23T13:11:53Z

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 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.

Signed-off-by: Raymond Mathew click2cloud-lamda@click2cloud.net
Signed-off-by: Dasara Nagaraju dasara.nagaraju@click2cloud.net

Zheaoli · 2022-03-23T14:32:25Z

go.mod

@@ -118,6 +118,7 @@ require (
 	github.com/mitchellh/go-homedir v1.1.0 // indirect
 	github.com/mitchellh/mapstructure v1.4.3 // indirect
 	github.com/moby/locker v1.0.1 // indirect
+	github.com/moby/moby v20.10.13+incompatible // indirect


I'm not sure should we add a full moby dependency here?

Zheaoli · 2022-03-23T14:39:50Z

pkg/mountutil/mountutil_freebsd.go

@@ -66,3 +74,65 @@ func ProcessFlagTmpfs(s string) (*Processed, error) {
 func ProcessFlagMount(s string, volStore volumestore.VolumeStore) (*Processed, error) {
 	return nil, errdefs.ErrNotImplemented
 }
+func ProcessSplit(s string, volStore volumestore.VolumeStore) (Processed, error) {


I think the ProcessSpllit code is as same as the code in mountutil_linux.go. FYI https://github.com/containerd/nerdctl/pull/924/files#diff-ab0f47dd8154315baddf1347290f120ed5e9975a660c4bfb838d17dcd2418c50R434

Maybe we can add a new file named mountutils_otthers.go and add //go:build !windows at the top of the code and move ProcessSpllit here

Or maybe we can keep the conditions of ProcessSplit for linux and freebsd in mountutil.go and keep the windows one as it is.

Yes, I think it's a better choice

Zheaoli · 2022-03-23T14:41:37Z

pkg/mountutil/mountutil_freebsd.go

+
+var (
+	res      Processed
+	src, dst string


IMHO, I think using global mutable variables here would not be the best choice.

Would you mind telling me why we need global mutable variables here?

Zheaoli · 2022-03-23T14:42:06Z

pkg/mountutil/mountutil.go

-	"runtime"
-	"strings"
-
-	"github.com/containerd/containerd/errdefs"
 	"github.com/containerd/containerd/oci"


Fix the lint error plz

AkihiroSuda · 2022-03-25T10:27:03Z

Commits 25

Please remove irrelevant commits and squash your ones.
Just like other PRs: https://github.com/containerd/nerdctl/pulls?q=is%3Apr+is%3Amerged

Zheaoli

Maybe we got some rebase issue here?

Zheaoli

Maybe we got some rebase issue here?

Zheaoli · 2022-03-30T13:44:20Z

cmd/nerdctl/build.go

@@ -75,7 +75,7 @@ func newBuildCommand() *cobra.Command {
 }

 func getBuildkitHost(cmd *cobra.Command) (string, error) {
-	if cmd.Flags().Changed("buildkit-host") {
+	if cmd.Flags().Changed("buildkit-host") || os.Getenv("BUILDKIT_HOST") != "" {


IMHO, The piece of code here is may belong to the #932

Maybe we got some rebase issue here?

AkihiroSuda · 2022-04-04T11:06:08Z

pkg/inspecttypes/dockercompat/dockercompat.go

@@ -439,3 +451,52 @@ func NetworkFromNative(n *native.Network) (*Network, error) {

 	return &res, nil
 }
+
+type Specs struct {


Seems a duplicate of https://github.com/opencontainers/runtime-spec/blob/6969a0a09ab162a574cafcc9ac814e498962c943/specs-go/config.go#L6

AkihiroSuda · 2022-04-04T11:06:26Z

pkg/mountutil/mountutil_linux.go

@@ -422,3 +422,7 @@ func getTmpfsSize(size int64) string {

 	return fmt.Sprintf("size=%d%s", size, suffix)
 }
+func ProcessSplit(s string, volStore volumestore.VolumeStore, res Processed, src string, dst string, options []string) (string, string, []string, error) {


Needs godoc

AkihiroSuda · 2022-04-04T11:07:02Z

pkg/mountutil/mountutil_windows.go

@@ -68,3 +71,81 @@ func ProcessFlagTmpfs(s string) (*Processed, error) {
 func ProcessFlagMount(s string, volStore volumestore.VolumeStore) (*Processed, error) {
 	return nil, errdefs.ErrNotImplemented
 }
+
+func ProcessSplit(s string, volStore volumestore.VolumeStore, res Processed, src string, dst string, options []string) (string, string, []string, error) {


Needs godoc.
Also, can we have unit tests?

AkihiroSuda · 2022-04-12T01:44:55Z

pkg/inspecttypes/dockercompat/dockercompat.go

@@ -106,7 +106,7 @@ type Container struct {
 	// TODO: SizeRw     *int64 `json:",omitempty"`
 	// TODO: SizeRootFs *int64 `json:",omitempty"`

-	// TODO: Mounts          []MountPoint
+	Mounts []specs.Mount


This should be []MountPont, not []specs.Mount for Docker compatibility

https://github.com/moby/moby/blob/f0014860c1b3345e1fcc7ed81c491298de2633fb/api/types/types.go#L416-L427
https://github.com/opencontainers/runtime-spec/blob/6969a0a09ab162a574cafcc9ac814e498962c943/specs-go/config.go#L110-L120

Addressed in #822

AkihiroSuda · 2022-04-18T07:37:41Z

pkg/inspecttypes/dockercompat/dockercompat.go

 	"github.com/docker/go-connections/nat"
+	"github.com/moby/moby/api/types"


We do not import this package. Please see other struct definitions and keep the coding style consistent.

AkihiroSuda · 2022-04-18T07:38:57Z

pkg/inspecttypes/dockercompat/dockercompat.go

@@ -195,12 +196,26 @@ type NetworkEndpointSettings struct {

 // ContainerFromNative instantiates a Docker-compatible Container from containerd-native Container.
 func ContainerFromNative(n *native.Container) (*Container, error) {
+	var mounts *specs.Spec
+	// for unmarshalling mount details
+	err := json.Unmarshal(n.Container.Spec.Value, &mounts)


We already have specs.Spec. Please see the if sp, ok := n.Spec.(*specs.Spec); ok { line below.

jsturtevant

sorry for the delay, been on PTO for a couple weeks.

jsturtevant · 2022-04-26T00:00:17Z

pkg/mountutil/mountutil.go

+	if runtime.GOOS == "windows" {
+		sr, ds, optns, err := ProcessSplit(s, volStore, res, src, dst, options)


Why are adding a check for goos if we are doing the process split logic in os specific files? it feels strange to have the case statements tucked away for Windows but for other OS (linux) process split is in this file especially since process split doesn't do anything for linux/freebsd and are exactly the same

I will just keep the process split logic to respective os files and remove the os logic.

jsturtevant · 2022-04-26T00:02:45Z

pkg/mountutil/mountutil_freebsd.go

+func ProcessSplit(s string, volStore volumestore.VolumeStore, res Processed, src string, dst string, options []string) (string, string, []string, error) {
+	var x []string
+	return "", "", x, nil
+}


freebsd implementation is the same as linux, why do we have it duplicated? (see other comment about the goos check for processsplit in mountutil.go)

jsturtevant · 2022-04-26T00:04:57Z

cmd/nerdctl/run_mount_windows_test.go

+		"-v", fmt.Sprintf("%s:C:\\mnt3", rwVolName),
+		"-v", fmt.Sprintf("%s:C:\\mnt4ro", roVolName),
+		imageName).AssertOK()
+	base.Cmd("container", "inspect", containerName).AssertOK()


this asserts that the container created, do we also want to make sure that the volume is actually there (maybe with something in it)? How is this tested on Linux?

TestRunAnonymousVolume and TestRunNamedVolume checks whether the volume is mounted or not. For linux, we havent added any test as the problem was with the windows part.

I guess we some coverage that the volume is created. Another thing noticed now, is it seem like we are also testing read/write behavior here. I would think we want to verify that the in the test.

For verifying the read/write behavior, inspect should print the details regarding it. But in inspect for windows, it doesn't work.

D:\GolandProjects\github.com\nerdctlmst\cmd\nerdctl>nerdctl inspect 1f44fa83de [ { "Id": "1f44fa83de31867ec49d2639a15f4d04fa6003765660afb9e50c51e887e89815", "Created": "2022-04-28T11:03:48.0466636Z", "Path": "c:\\windows\\system32\\cmd.exe", "Args": null, "State": { "Status": "exited", "Running": false, "Paused": false, "Pid": 25496, "ExitCode": 0, "FinishedAt": "2022-04-28T11:03:50.2407926Z" }, "Image": "mcr.microsoft.com/windows/nanoserver:20H2", "ResolvConfPath": "", "HostnamePath": "", "LogPath": "", "Name": "nanoserver-1f44f", "Driver": "windows", "Platform": "windows", "AppArmorProfile": "", "Mounts": [ { "Source": "C:\\ProgramData\\nerdctl\\052055e3\\volumes\\default\\testVolume\\_data", "Destination": "C:\\src", "Mode": "", "RW": true, "Propagation": "" } ], "NetworkSettings": { "GlobalIPv6Address": "", "GlobalIPv6PrefixLen": 0, "IPAddress": "", "IPPrefixLen": 0, "MacAddress": "", "Networks": {} } } ]

This is the output Im getting.

So the inspect output for the volume is there but the rw/ro info isn't valid? Does the actual directory in the container have the correct attributes? If it is just inspect that isn't return properly lets create an issue to resolve later.

This issue is only with windows where I cant get the output for read/write behavior.

jsturtevant · 2022-05-04T15:37:34Z

pkg/mountutil/mountutil_freebsd.go

+
+// ProcessSplit splits volume mount information received through command line into respective source, destination
+// and optsRaw fields according to the volume type chosen and returns them accordingly.
+func ProcessSplit(s string, volStore volumestore.VolumeStore, res *Processed, src string, dst string, options []string) (string, string, []string, error) {


This function is exactly the same for freebsd and linux. Can we reduce this so the code isn't copy pasted in two places?

jsturtevant · 2022-05-04T15:38:17Z

pkg/mountutil/mountutil.go

 		options = append(options, "rbind")
 	}
 	res.Mount = specs.Mount{
-		Type:        fstype,


Why is this being dropped? I don't see it moved anywhere.

Because of this the mounted type during inspect was null. Now I get this output:

D:\GolandProjects\github.com\nerdctl\cmd\nerdctl>nerdctl inspect 3c46c12ed1f1 [ { "Id": "3c46c12ed1f1f6272b4ab3f93996a19e2e52667313c2a2cbbf94f87b08350dad", "Created": "2022-05-06T09:54:49.1905099Z", "Path": "c:\\windows\\system32\\cmd.exe", "Args": null, "State": { "Status": "exited", "Running": false, "Paused": false, "Restarting": false, "Pid": 10928, "ExitCode": 0, "FinishedAt": "2022-05-06T09:54:51.2735242Z" }, "Image": "mcr.microsoft.com/windows/nanoserver:20H2", "ResolvConfPath": "", "HostnamePath": "", "LogPath": "", "Name": "nanoserver-3c46c", "RestartCount": 0, "Driver": "windows", "Platform": "windows", "AppArmorProfile": "", "Mounts": [ { "Type": "volume", "Name": "500907e8f1465b04f132f5a49b76c59c1e41f18aa9063faffa5c2bb7fc2e72a5", "Source": "C:\\ProgramData\\nerdctl\\052055e3\\volumes\\default\\500907e8f1465b04f132f5a49b76c59c1e41f18aa9063faffa5c2bb7fc2e72a5\\_data", "Destination": "C:\\src", "Driver": "local", "Mode": "", "RW": true, "Propagation": "" } ], "NetworkSettings": { "GlobalIPv6Address": "", "GlobalIPv6PrefixLen": 0, "IPAddress": "", "IPPrefixLen": 0, "MacAddress": "", "Networks": {} } } ]

Please check.

is only printing null an issue on Windows? I am concerned this change affects something on freebsd or linux. The tests aren't failing but maybe we are missing coverage here

If we pass "none" as fstype for windows, it gives this error: "time="2022-05-12T12:13:13+05:30" level=fatal msg="invalid OCI spec - Type 'none' not supported: unknown"".

why are we passing nullfs or none, when we are already passing mount types like bind, volume or tmps while splitting the source and destination.

I don't know why none/nullfs is passed here. @AkihiroSuda do you have any insight?

jsturtevant · 2022-05-10T20:00:49Z

pkg/mountutil/mountutil.go

 		options = append(options, "rbind")
 	}
 	res.Mount = specs.Mount{
-		Type:        fstype,


is only printing null an issue on Windows? I am concerned this change affects something on freebsd or linux. The tests aren't failing but maybe we are missing coverage here

jsturtevant · 2022-05-10T20:01:03Z

pkg/mountutil/process_split.go

@@ -0,0 +1,100 @@
+//go:build freebsd || linux


This file should be named mountutil_others.go or mountutil_linux.go to stay consistent that this a function that is specific to runtimes related to mountutil.

If we pass "none" as fstype for windows, it gives this error: "time="2022-05-12T12:13:13+05:30" level=fatal msg="invalid OCI spec - Type 'none' not supported: unknown"".

why are we passing nullfs or none, when we are already passing mount types like bind, volume or tmps while splitting the source and destination.

These comments look to have landed on the wrong thread. I've responded here: #924 (comment)

Signed-off-by: Raymond Mathew <raymond.mathew@click2cloud.net>

jsturtevant · 2022-06-07T14:30:08Z

I think we've gotten pretty far on this. I would be inclined to merge and iterate. The last comment I have a concern on is https://github.com/containerd/nerdctl/pull/924/files#r871565970. @AkihiroSuda do you know what that fstype is used for and if it is a required for freebsd or linux?

TBBle · 2023-03-11T15:39:41Z

pkg/mountutil/mountutil.go

-		res.Opts = append(res.Opts, specOpts...)
-	default:
-		return nil, fmt.Errorf("failed to parse %q", s)
+	src, dst, options, err = ProcessSplit(s, volStore, &res, src, dst, options)


Looking at the implementations, src, dst, and options are never consumed, so should probably not be parameters, but local vars in ProcessSplit. That would allow simplifying this call slightly to src, dst, options, err := ProcessSplit(s, volStore, &res), trimming the top-of-function var lines, rather than growing them.

Also, does it need to be public? processSplit seems like it should work, all uses are in pkg/mountutil AFAICT, and it seems slightly safer to hide platform-split functions to control the blast radius in case of future divergence.

Zheaoli reviewed Mar 23, 2022

View reviewed changes

click2cloud-lamda force-pushed the master branch from fab7e61 to 47d33bd Compare March 24, 2022 06:19

click2cloud-Nagaraj force-pushed the master branch from f15f961 to 15d243b Compare March 25, 2022 10:11

click2cloud-Nagaraj force-pushed the master branch from 9340c9b to 6a6bc2e Compare March 25, 2022 11:12

click2cloud-lamda force-pushed the master branch 5 times, most recently from 79ddf50 to f913684 Compare March 30, 2022 13:02

Zheaoli reviewed Mar 30, 2022

View reviewed changes

click2cloud-lamda force-pushed the master branch 3 times, most recently from 0b52580 to 09b85d6 Compare April 4, 2022 09:05

AkihiroSuda reviewed Apr 4, 2022

View reviewed changes

click2cloud-lamda force-pushed the master branch from 09b85d6 to d7cd274 Compare April 6, 2022 06:45

AkihiroSuda requested a review from jsturtevant April 6, 2022 06:58

AkihiroSuda added the platform/Windows/Non-WSL2 Microsoft Windows (non-WSL2) label Apr 6, 2022

click2cloud-lamda force-pushed the master branch 2 times, most recently from 6384e3f to 53bc54e Compare April 6, 2022 08:55

AkihiroSuda reviewed Apr 12, 2022

View reviewed changes

AkihiroSuda mentioned this pull request Apr 12, 2022

Added fixes for issue #971(Mounted volume can be deleted) #974

Merged

click2cloud-lamda force-pushed the master branch 3 times, most recently from fe9fbb6 to 637d289 Compare April 18, 2022 07:33

AkihiroSuda reviewed Apr 18, 2022

View reviewed changes

click2cloud-lamda force-pushed the master branch 3 times, most recently from 53bc54e to a61f22c Compare April 25, 2022 06:04

jsturtevant reviewed Apr 26, 2022

View reviewed changes

click2cloud-lamda force-pushed the master branch 9 times, most recently from 8d3f808 to e2764b8 Compare May 3, 2022 10:59

click2cloud-lamda requested a review from jsturtevant May 3, 2022 11:30

jsturtevant reviewed May 4, 2022

View reviewed changes

click2cloud-lamda force-pushed the master branch 4 times, most recently from 16ad204 to f369883 Compare May 6, 2022 11:06

click2cloud-lamda requested a review from jsturtevant May 9, 2022 05:03

jsturtevant reviewed May 10, 2022

View reviewed changes

click2cloud-lamda force-pushed the master branch 2 times, most recently from 006f7fb to 17119fa Compare May 12, 2022 07:34

click2cloud-lamda and others added 2 commits May 12, 2022 13:05

Added changes for windows volume mount issue

17119fa

Signed-off-by: Raymond Mathew <raymond.mathew@click2cloud.net>

Merge branch 'master' into master

3b4b569

jsturtevant mentioned this pull request Jul 22, 2022

Unable to mount volumes or bind mount on Windows #759

Open

TBBle reviewed Mar 11, 2023

View reviewed changes

aznashwan mentioned this pull request Apr 6, 2023

Windows: Ensure image VOLUME temp paths are well-formed. #2160

Open

jsturtevant mentioned this pull request Dec 8, 2023

Allow for binding a named pipe on Windows #2661

Merged

		"github.com/docker/go-connections/nat"
		"github.com/moby/moby/api/types"

		if runtime.GOOS == "windows" {
		sr, ds, optns, err := ProcessSplit(s, volStore, res, src, dst, options)

Fixed volume mount issue(759) for Windows #924

Are you sure you want to change the base?

Fixed volume mount issue(759) for Windows #924

Conversation

click2cloud-lamda commented Mar 23, 2022 • edited

Fixed issue #759 :

Choose a reason for hiding this comment

Choose a reason for hiding this comment

click2cloud-lamda Mar 24, 2022 • edited

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

AkihiroSuda commented Mar 25, 2022

Zheaoli left a comment

Choose a reason for hiding this comment

Zheaoli left a 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

jsturtevant left a 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

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

jsturtevant commented Jun 7, 2022

Choose a reason for hiding this comment

click2cloud-lamda commented Mar 23, 2022 •

edited

click2cloud-lamda Mar 24, 2022 •

edited