Support copying files in volume #200

fahedouch · 2021-04-29T22:36:49Z

fixing #165

AkihiroSuda · 2021-04-30T13:20:28Z

run_mount.go

+
+		s := client.SnapshotService(clicontext.String("snapshotter"))
+
+		tempDir, err := ioutil.TempDir(os.Getenv("GOTMPDIR"), "initialC")


Why GOTMPDIR

it is just a choice. I can create tempDir in in the default location for OS. it doesn't change anything for me

updated to tempDir, err := ioutil.TempDir("", "initialC")

AkihiroSuda · 2021-04-30T13:22:01Z

run_mount.go

+		target := strings.Join([]string{tempDir, imgVol}, "")
+
+		//copying up initial contents of the mount point directory
+		copyExistingContents(target, anonVol.Mountpoint)


Named volumes need this as well

Also make sure to catch err

@AkihiroSuda I made modification. FYI : in docker named volume do not copy initial content ( that may override source volume)

@AkihiroSuda sorry I mixed mount bind and named volume. I added copyExistingContentsto named volume

AkihiroSuda · 2021-05-03T14:57:57Z

run_mount_test.go

+	base.Cmd("run", "-v", "/mnt", "--rm", imageName).AssertOut("hi")
+
+	//NamedVolume
+	base.Cmd("run", "-v", "/home:/mnt", "--rm", imageName).AssertFail()


This is a bind mount, not a named volume (nerdctl volume create)

AkihiroSuda · 2021-05-03T14:58:13Z

run_mount.go

+				imgVol := filepath.Clean(imgVolRaw)
+				target := strings.Join([]string{tempDir, imgVol}, "")
+
+				//Coyping content only for AnonymousVolume


AkihiroSuda · 2021-05-03T14:58:31Z

run_mount.go

+
+	if ensuredImage != nil {
+		imageVolumes = ensuredImage.ImageConfig.Volumes
+		client, ctx, cancel, err := newClient(clicontext)


No, we already have the client

Doesn’t seem fixed

it should be ok now!

AkihiroSuda · 2021-05-03T19:19:55Z

pkg/mountutil/mountutil.go

@@ -90,7 +96,7 @@ func ProcessFlagV(s string, volStore volumestore.VolumeStore) (*Processed, error
 		return nil, errors.Errorf("failed to parse %q", s)
 	}
 	res.Mount = specs.Mount{
-		Type:        "none",
+		Type:        string(vType),


This is an OCI Mount object, you can’t mix up Docker API type here

@AkihiroSuda does OCI provide types values ? or I may set them directly in the code ?

@AkihiroSuda is it ok for you ?

AkihiroSuda · 2021-05-03T19:30:27Z

run_mount_test.go

+	base.Cmd("volume","create", "copying-initial-content").AssertOK()
+	base.Cmd("run", "-v", "copying-initial-content:/mnt", "--rm", imageName).AssertOut("hi")
+	//mount bind
+	base.Cmd("run", "-v", "/home:/mnt", "--rm", imageName).AssertFail()


Mounting /home is dangerous, please use some ioutil.TempDir

AkihiroSuda · 2021-05-05T04:26:36Z

pkg/mountutil/mountutil.go

@@ -90,7 +93,7 @@ func ProcessFlagV(s string, volStore volumestore.VolumeStore) (*Processed, error
 		return nil, errors.Errorf("failed to parse %q", s)
 	}
 	res.Mount = specs.Mount{
-		Type:        "none",
+		Type:        vType,


Again, you can't have "volume" "bind" here

https://github.com/opencontainers/runtime-spec/blob/master/config.md#posix-platform-mounts

The type fIeld can be in this struct

nerdctl/pkg/mountutil/mountutil.go

Line 32 in 86e8f5f

type Processed struct {

@AkihiroSuda moved.

AkihiroSuda · 2021-05-06T18:53:17Z

run_mount_test.go

+	assert.NilError(t, err)
+	defer os.RemoveAll(tmpDir)
+
+	m := strings.Join([]string{"./", tmpDir, ":", "/mnt"}, "")


You don’t need ./ here

AkihiroSuda · 2021-05-06T18:54:35Z

pkg/mountutil/mountutil.go

@@ -32,6 +32,7 @@ import (
 type Processed struct {
 	Mount           specs.Mount
 	AnonymousVolume string // name
+	Type            string


enum (Bind = iota, Volume)

@AkihiroSuda can review plz.

fahedouch · 2021-05-07T09:42:27Z

@AkihiroSuda any idea what is disturbing the cgroup2 job ?

dmcgowan · 2021-05-07T17:34:30Z

run_mount.go

+			return nil, nil, err
+		}
+
+		if err := mount.All(mounts, tempDir); err != nil {


Where are the mounts getting cleaned up? The temp mount logic is ideal for these type of use cases

@dmcgowan I added unmount logic . The temp mount logic is ideal but not in this context because it push me to make same steps ( tmp dir creation then mount then unmount ) in each iteration flagVSlice

I think it depends on user cannot CONTRL-C during copy. Need to figure out how to clean up in this case

AkihiroSuda

Pushed a new test.

LGTM if green. Thanks and sorry for slow reviewing

run_mount.go

ktock · 2021-05-17T06:23:37Z

needs rebase

Conflicting files
go.mod

Signed-off-by: fahed dorgaa <fahed.dorgaa@gmail.com>

Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>

The commit c245c9c "support copying files in volume" (rebased as 89daa2b) improperly used `strings.Join` to join paths. The new commit replaces `strings.Join` with `securejoin.SecureJoin`. The problematic commit was not merged into the existing releases, so nobody was exposed to potential vulns. Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>

fahedouch · 2021-05-17T10:39:16Z

@AkihiroSuda should we test hardlink too ?

fuweid · 2021-05-18T06:40:06Z

run_mount.go

+		}()
+
+		if err := mount.All(mounts, tempDir); err != nil {
+			if err := s.Remove(ctx, tempDir); err != nil && !errdefs.IsNotFound(err) {


need to umount first and then remove

fuweid · 2021-05-18T06:42:55Z

Using tar and echo might be safety, like what kubectl cp does. It is hard to handle corner cases in nerdctl side.

WDYT? @AkihiroSuda @dmcgowan

AkihiroSuda · 2021-05-18T07:04:19Z

@fahedouch

should we test hardlink too ?

Yes, that's more preferrable.

@fuweid

Using tar and echo might be safety, like what kubectl cp does. It is hard to handle corner cases in nerdctl side.

How do you use echo?
Could you open an issue/PR?

fahedouch force-pushed the support_copying_files_in_volume branch 2 times, most recently from cf587a5 to d10e861 Compare April 29, 2021 22:44

fahedouch marked this pull request as draft April 29, 2021 22:44

fahedouch changed the title ~~Support copying files in volume~~ [WIP] Support copying files in volume Apr 29, 2021

fahedouch force-pushed the support_copying_files_in_volume branch 3 times, most recently from 7a00aaa to a82b0d0 Compare April 30, 2021 13:06

fahedouch marked this pull request as ready for review April 30, 2021 13:07

fahedouch changed the title ~~[WIP] Support copying files in volume~~ Support copying files in volume Apr 30, 2021

AkihiroSuda reviewed Apr 30, 2021

View reviewed changes

fahedouch force-pushed the support_copying_files_in_volume branch 2 times, most recently from fd20868 to 66e8281 Compare May 3, 2021 14:35

AkihiroSuda requested changes May 3, 2021

View reviewed changes

AkihiroSuda reviewed May 3, 2021

View reviewed changes

AkihiroSuda marked this pull request as draft May 3, 2021 19:20

AkihiroSuda reviewed May 3, 2021

View reviewed changes

fahedouch force-pushed the support_copying_files_in_volume branch 3 times, most recently from ff22da0 to 86e8f5f Compare May 4, 2021 22:44

fahedouch marked this pull request as ready for review May 4, 2021 22:44

AkihiroSuda requested changes May 5, 2021

View reviewed changes

AkihiroSuda marked this pull request as draft May 5, 2021 04:26

fahedouch marked this pull request as ready for review May 6, 2021 18:39

fahedouch force-pushed the support_copying_files_in_volume branch 2 times, most recently from cdff425 to 6a9b795 Compare May 6, 2021 18:40

AkihiroSuda reviewed May 6, 2021

View reviewed changes

fahedouch force-pushed the support_copying_files_in_volume branch 2 times, most recently from b63c6f0 to 05d4340 Compare May 6, 2021 21:56

dmcgowan reviewed May 7, 2021

View reviewed changes

fahedouch force-pushed the support_copying_files_in_volume branch 2 times, most recently from 0bb9b2e to c245c9c Compare May 9, 2021 23:24

AkihiroSuda added the priority/high label May 16, 2021

AkihiroSuda approved these changes May 17, 2021

View reviewed changes

AkihiroSuda requested review from fuweid and ktock May 17, 2021 05:12

ktock reviewed May 17, 2021

View reviewed changes

run_mount.go Outdated Show resolved Hide resolved

ktock reviewed May 17, 2021

View reviewed changes

run_mount.go Outdated Show resolved Hide resolved

fahedouch and others added 4 commits May 17, 2021 15:37

support copying files in volume

89daa2b

Signed-off-by: fahed dorgaa <fahed.dorgaa@gmail.com>

go.mod: clean up

860aac3

Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>

tests: add TestCopyingUpInitialContentsOnVolumeShouldRetainSymlink

6a8282b

Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>

AkihiroSuda force-pushed the support_copying_files_in_volume branch from 697d2e7 to a0390ad Compare May 17, 2021 06:39

ktock approved these changes May 17, 2021

View reviewed changes

AkihiroSuda merged commit 336690c into containerd:master May 17, 2021

AkihiroSuda mentioned this pull request May 17, 2021

volume should support copying up initial contents of the mount point directory #165

Closed

fuweid reviewed May 18, 2021

View reviewed changes

AkihiroSuda mentioned this pull request May 18, 2021

nerdctl cp design #212

Closed

fahedouch mentioned this pull request May 18, 2021

fix volume clean up & add test #216

Closed


		s := client.SnapshotService(clicontext.String("snapshotter"))

		tempDir, err := ioutil.TempDir(os.Getenv("GOTMPDIR"), "initialC")

Support copying files in volume #200

Support copying files in volume #200

Conversation

fahedouch commented Apr 29, 2021

Choose a reason for hiding this comment

fahedouch Apr 30, 2021 • edited Loading

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

fahedouch May 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AkihiroSuda May 3, 2021 • edited Loading

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

fahedouch May 9, 2021 • edited Loading

Choose a reason for hiding this comment

fahedouch commented May 7, 2021 • edited Loading

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

Choose a reason for hiding this comment

ktock commented May 17, 2021

fahedouch commented May 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fuweid commented May 18, 2021

AkihiroSuda commented May 18, 2021

fahedouch Apr 30, 2021 •

edited

Loading

fahedouch May 3, 2021 •

edited

Loading

AkihiroSuda May 3, 2021 •

edited

Loading

fahedouch May 9, 2021 •

edited

Loading

fahedouch commented May 7, 2021 •

edited

Loading