feat: Add support for copying files in and out of stopped containers #2355

vsiravar · 2023-07-05T06:16:44Z

What does this PR do

This adds support to copy files in and out of non running containers only in rootful mode thereby improving feature compatibility with podman and docker. It fixes #1058

High level design

This PR adds logic in 4 packages

~~main(Modifies parsing logic for nerdctl cp command)~~
types(Modifies ContainerCpOptions struct)
container(Adds logic to search for container by long/short Id)
containerutil(Adds logic to copy files out and into non running containers)
rootlessutil(Add a function to retrieve rootless containerd socket address)

Fine level Details

cmd/nerdctl/container_cp_linux.go

Modifies processCpOptions function to include GlobalCommandOptions. Deletes logic for container inspect as we don't rely on inspect to find pid of the running container. For rootless mode options.GOptions.Address=/proc/<PID of containerd>/root/run/containerd/containerd.sock based on faq.md
~~Modifies cpAction to include containerd.Client which is used for searching container by long/short id and by SnapshotService to get the underlying Snapshotter.~~

main_linux.go

~~Modifies appNeedsRootlessParentMain to return true for nerdctl cp and nerdctl container cp. This is necessary to access containerd.sock in rootless mode.~~

pkg/api/types/container_types.go

Modifies ContainerCpOptions to add GlobalCommandOptions and ContainerReq. GlobalCommandOptions are necessary for determining namespace, conatinerd socket address and name of underlying snapshotter. ContainerReq required for finding container by short/long id.

pkg/cmd/container/cp_linux.go

Searches for container specified in nerdctl cp <containerid> command using ContainerWalker. If multiple containers are found it returns an error.
It returns an error when the specified container for copy is not found.

pkg/containerutil/cp_linux.go

Use containerd snapshot service to copy files into and out of created/stopped containers.
It tries to find [Task](https://github.com/containerd/containerd/blob/main/task.go#L154) from containerd. If it's not found(which is the case on nerdctl container create, it mounts the snapshot onto a temporary directory for copying. Any other type of error returns an error.
If the status of the containerd task is Running, then the root filesystem of the running process is used for copying files into /out of the container. (unchanged from previous commits)
If task is found and status is not Running(Exited container), then we fallback to the snapshot service for copying.
~~For rootless mode it deletes the nsenter command to enter the namespace of the container process since it's already in the namespace of the rootlesskit child pid.~~
Introduces a helper function mountSnapshotForContainer which returns the temp directory that the container shapshot is mounted on.

pkg/rootlessutil/rootlessutil_linux.go

Adds a function to retrieve rootless containerd socket address

Testing

Added tests for stopped containers for exactly the same existing test cases as running containers. Tests are skipped in rootless mode on stopped containers.

ktock · 2023-07-09T13:49:59Z

pkg/api/types/container_types.go

@@ -408,9 +408,10 @@ type ContainerListOptions struct {

 // ContainerCpOptions specifies options for `nerdctl (container) cp`
 type ContainerCpOptions struct {
+	// GOptions is the global options.
+	GOptions       GlobalCommandOptions
+	ContainerReq   string


Could you add godoc

ktock · 2023-07-09T13:59:57Z

pkg/containerutil/cp_linux.go

+		mount.Unmount(tempDir, 0)
+		os.RemoveAll(tempDir)


maybe RemoveAll should be called when unmount succeeds. And cleanup failure should print a warning.

ktock · 2023-07-09T14:01:30Z

pkg/containerutil/cp_linux.go

+	err = mount.All(resp, tempDir)
+	if err != nil {
+		cleanup := func() {
+			os.RemoveAll(tempDir)


why not remove it now?

ktock · 2023-07-09T14:04:36Z

pkg/containerutil/cp_linux.go

-	if rootlessutil.IsRootless() {
-		nsenter := []string{"nsenter", "-t", strconv.Itoa(pid), "-U", "--preserve-credentials", "--"}
-		if container2host {
-			tarC = append(nsenter, tarC...)
-		} else {
-			tarX = append(nsenter, tarX...)
-		}
-	}


Can it be safely removed when status.Status == containerd.Running?

I don't quite understand this part, in rootless mode its already in the namespace of the rootlesskit child pid. Am I missing anything here? The integration tests pass okay when status.Status == containerd.Running.

Update: After realizing that enabling copying out of stopped/created containers in rootless mode causes regression, reverted this back to the original state.

AkihiroSuda · 2023-07-17T16:40:16Z

Please try rebasing to make the CI green

AkihiroSuda · 2023-07-17T19:06:13Z

cmd/nerdctl/main_linux.go

@@ -39,16 +39,15 @@ func appNeedsRootlessParentMain(cmd *cobra.Command, args []string) bool {
 	switch commands[1] {
 	// completion, login, logout: false, because it shouldn't require the daemon to be running
 	// apparmor: false, because it requires the initial mount namespace to access /sys/kernel/security
-	// cp: false, because it requires the initial mount namespace to inspect file owners


Why was this changed?

I modified appNeedsRootlessParentMain to return true for nerdctl cp and nerdctl container cp. This was necessary to access containerd.sock in rootless mode to create containerd client. When it's running in rootless mode without this change, get /run/containerd/containerd.sock": no such file or directory . However this change does not preserve the uid/gid mapping while copying to container. Should we disable the copying behaviour for stopped/created containers in rootless mode and only support this feature in rootful.

this change does not preserve the uid/gid mapping while copying to container.

This is a regression.

Should we disable the copying behaviour for stopped/created containers in rootless mode and only support this feature in rootful.

Yes, at least for now.

@AkihiroSuda I updated the PR to

use rootless containerd sock based on faq.md in rootless mode.

Disabled copying behaviour for stopped/created containers in rootless mode and only support this feature in rootful(more info in PR description).

The uid/gid mapping is preserved when files are copied.

PTAL, thanks! I noticed that the docker-compatibility check fails although all the tests for container copy pass. Not sure if this is related to this change. PTAL, thanks for the review.

AkihiroSuda · 2023-07-17T19:06:28Z

cmd/nerdctl/main_linux.go

 		return false
 	case "container":
 		if len(commands) < 3 {
 			return true
 		}
 		switch commands[2] {
 		case "cp":
-			return false
+			return true


true is returned on L53

AkihiroSuda · 2023-07-20T14:43:04Z

pkg/rootlessutil/rootlessutil_linux.go

+	if err != nil {
+		return "", err
+	}
+	return path.Join(fmt.Sprintf("/proc/%d/root/run/containerd/containerd.sock", childPid)), nil


Suggested change

return path.Join(fmt.Sprintf("/proc/%d/root/run/containerd/containerd.sock", childPid)), nil

return filepath.Join(fmt.Sprintf("/proc/%d/root/run/containerd/containerd.sock", childPid)), nil

AkihiroSuda · 2023-07-20T14:44:05Z

pkg/containerutil/cp_linux.go

+	}
+	err = mount.All(resp, tempDir)
+	if err != nil {
+		os.RemoveAll(tempDir)


This is unsafe, as tempDir may contain partial mounts

AkihiroSuda · 2023-07-20T14:44:23Z

pkg/containerutil/cp_linux.go

+	task, err := container.Task(ctx, nil)
+	if err != nil {
+		// if the task is simply not found, we should try to mount the snapshot. any other type of error from Task() is fatal here.
+		// Rootless does not support copying out of stopped/created containers


Could you add the reason?

Added a comment explaining. Lmk if I need to additional details.

Signed-off-by: Vishwas Siravara <siravara@amazon.com>

AkihiroSuda

Thanks

vsiravar marked this pull request as draft July 5, 2023 06:17

vsiravar force-pushed the vsiravar/fix-1058 branch 2 times, most recently from 7476e98 to 0b2fbc2 Compare July 6, 2023 23:58

vsiravar marked this pull request as ready for review July 7, 2023 00:41

vsiravar force-pushed the vsiravar/fix-1058 branch from 0b2fbc2 to b805ee9 Compare July 7, 2023 23:50

vsiravar mentioned this pull request Jul 8, 2023

nerdctl cp: Support copying from unstarted containers #2363

Closed

AkihiroSuda requested review from fahedouch and ktock July 8, 2023 08:53

ktock reviewed Jul 9, 2023

View reviewed changes

vsiravar force-pushed the vsiravar/fix-1058 branch from b805ee9 to a2e5d95 Compare July 10, 2023 06:35

vsiravar requested a review from ktock July 10, 2023 06:42

vsiravar force-pushed the vsiravar/fix-1058 branch from a2e5d95 to d08c919 Compare July 10, 2023 17:34

AkihiroSuda added this to the v1.5.0 milestone Jul 16, 2023

vsiravar force-pushed the vsiravar/fix-1058 branch from d08c919 to d8ecbad Compare July 17, 2023 18:14

AkihiroSuda reviewed Jul 17, 2023

View reviewed changes

vsiravar force-pushed the vsiravar/fix-1058 branch 2 times, most recently from 4288d50 to cfb8af9 Compare July 20, 2023 00:29

AkihiroSuda reviewed Jul 20, 2023

View reviewed changes

Add functionality to copy files into and out of stopped containers.

0d0cbb8

Signed-off-by: Vishwas Siravara <siravara@amazon.com>

vsiravar force-pushed the vsiravar/fix-1058 branch from cfb8af9 to 0d0cbb8 Compare July 21, 2023 02:11

AkihiroSuda approved these changes Jul 24, 2023

View reviewed changes

AkihiroSuda merged commit b3a546f into containerd:main Jul 24, 2023
21 checks passed

AkihiroSuda mentioned this pull request Jul 25, 2023

v1.5 planning #2392

Closed

vsiravar mentioned this pull request Aug 3, 2023

Support stopped containers as source/target of finch cp runfinch/finch#243

Closed

ginglis13 mentioned this pull request Aug 8, 2023

build(deps): Bump submodules and dependencies runfinch/finch#521

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add support for copying files in and out of stopped containers #2355

feat: Add support for copying files in and out of stopped containers #2355

vsiravar commented Jul 5, 2023 •

edited

Loading

ktock Jul 9, 2023

vsiravar Jul 10, 2023

ktock Jul 9, 2023

vsiravar Jul 10, 2023

ktock Jul 9, 2023

vsiravar Jul 10, 2023

ktock Jul 9, 2023

vsiravar Jul 10, 2023 •

edited

Loading

AkihiroSuda commented Jul 17, 2023

AkihiroSuda Jul 17, 2023

vsiravar Jul 17, 2023

AkihiroSuda Jul 18, 2023

vsiravar Jul 20, 2023

AkihiroSuda Jul 17, 2023

AkihiroSuda Jul 20, 2023

AkihiroSuda Jul 20, 2023

AkihiroSuda Jul 20, 2023

vsiravar Jul 21, 2023

AkihiroSuda left a comment

	return path.Join(fmt.Sprintf("/proc/%d/root/run/containerd/containerd.sock", childPid)), nil
	return filepath.Join(fmt.Sprintf("/proc/%d/root/run/containerd/containerd.sock", childPid)), nil

feat: Add support for copying files in and out of stopped containers #2355

feat: Add support for copying files in and out of stopped containers #2355

Conversation

vsiravar commented Jul 5, 2023 • edited Loading

What does this PR do

High level design

Fine level Details

Testing

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

vsiravar Jul 10, 2023 • edited Loading

Choose a reason for hiding this comment

AkihiroSuda commented Jul 17, 2023

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

AkihiroSuda left a comment

Choose a reason for hiding this comment

vsiravar commented Jul 5, 2023 •

edited

Loading

vsiravar Jul 10, 2023 •

edited

Loading