Add copy/cp command #92

jandubois · 2021-06-29T00:39:59Z

This is a wrapper around scp. User doesn't have to lookup / specify the port number, and doesn't have to prefix remote names with 127.0.0.1:; a single : is enough. And the connection goes through the ControlPath.

Example:

limactl cp default :/etc/os-release .

cmd/limactl/copy.go

AkihiroSuda · 2021-06-29T02:29:35Z

Thanks, but I heard scp is deprecated https://lists.mindrot.org/pipermail/openssh-unix-dev/2019-March/037672.html

Can we use sftp or rsync?

AkihiroSuda · 2021-06-29T02:41:53Z

cmd/limactl/copy.go

+	Name:        "copy",
+	Aliases:     []string{"cp", "scp"},
+	Usage:       "Copy files between host and guest",
+	Description: "Prefix guest filenames with a colon. Example: limactl copy default :/etc/os-release .",


Do we really need colon?

We need some mechanism to tell which file is on the host, and which is on the guest. If we don't use the colon, how would you specify the direction:

# copy from guest to host limactl cp default :/etc/hostname hostname # copy from host to guest limactl cp default file1 file2 file3 :.

An alternative syntax: limactl cp default:/etc/hostname hostname.

This syntax can be also extended to limactl cp inst1:/foo inst2:/foo to copy files across instances.

This can be also extended to limactl cp inst1 inst2 to copy instances, though it might be confusing.
But given that limactl ls lists instances and limactl delete deletes instances, limactl cp inst1 inst2 might be probably acceptable.

This syntax can be also extended to limactl cp inst1:/foo inst2:/foo to copy files across instances.

I like it. I just did a manual test, and it seems to work:

$ scp -i ~/.lima/_config/user -o NoHostAuthenticationForLocalhost=yes -3 scp://[email protected]:60020//etc/hostname scp://[email protected]:60022/myname $ limactl shell default sh -c 'cat ~/myname' lima-alpine

I was confused for a bit about the obscure syntax: the first slash after the port is just a separator and not part of the path. You need a double-slash to specify an absolute path. At least it means relative paths are still supported.

The copy still has to go through the host (using -3) and not directly between the instances because they use different ports.

I guess theoretically you could use ssh to run the scp command from within the source instances, but that seems like unnecessary complexity.

I don't think it will be possible to use the ControlPath when we copy between instances, so this will complicate the logic some more...

I'll take a look into it tomorrow.

Thanks, limactl cp inst1:/foo inst2:/foo can be implemented later, we just have to determine the CLI syntax here.

I just implemented it:

$ limactl cp alpine:/etc/os-release opensuse:release-info $ limactl shell opensuse sh -c 'cat ~/release-info' NAME="Alpine Linux" ID=alpine VERSION_ID=3.13.5 PRETTY_NAME="Alpine Linux v3.13" HOME_URL="https://alpinelinux.org/" BUG_REPORT_URL="https://bugs.alpinelinux.org/"

scp will not use the ControlPath, but I think that is fine; it is just an optimization.

jandubois · 2021-06-29T02:53:29Z

Thanks, but I heard scp is deprecated https://lists.mindrot.org/pipermail/openssh-unix-dev/2019-March/037672.html

It is my understanding that the scp protocol is deprecated, not the cli tool: openssh/openssh-portable#194

Can we use sftp or rsync?

I prefer to use scp because it is part of OpenSSH, so we don't need to require additional software to be installed on the guest.

AkihiroSuda · 2021-06-29T07:39:27Z

pkg/sshutil/sshutil.go

+	if err != nil {
+		return nil, err
+	}
+	args, err := SCPArgs()


This is confusing, can we just use a single function like func SSHArgs(instDir string, useControlMaster bool) ?

This is confusing,

Not sure why this is confusing: the SCP arguments are just a subset of the SSH arguments. There are also potential differences (not relevant here), like -p for SSH is -P for SCP because -p for SCP means "preserve access mode and file times"...

can we just use a single function like func SSHArgs(instDir string, useControlMaster bool) ?

So you would call it with an empty string as the instDir from the copyCommand? Because we no longer have a global instDir, it can be different for each source argument.

Please reconsider! I think the current implementation is less confusing than forcing both commands to use the same function.

I was confused because "SCPArgs" is called even when we do not use scp.

Just renaming SCPArgs() to CommonArgs() and let var SCPArgs = CommonArgs might be less confusing, but anyway I don't have a strong opinion.

I've renamed SCPArgs to CommonArgs now.

AkihiroSuda · 2021-06-29T07:40:38Z

cmd/limactl/copy.go

+		return err
+	}
+	args = append(args, "-3", "--")
+	for _, arg := range clicontext.Args().Slice() {


I think we will eventually want unit tests for the parser, but can be added in separate PR in future

AkihiroSuda · 2021-06-29T07:41:23Z

cmd/limactl/copy.go

+		return err
+	}
+	args = append(args, "-3", "--")
+	for _, arg := range clicontext.Args().Slice() {


We may want to limit the length of the slice to be 2?

Why? You can specify multiple source arguments when the destination is a directory:

$ limactl cp alpine:/etc/os-release default:/etc/hostname opensuse:. $ limactl shell opensuse sh -c 'ls -l ~' total 12 drwxr-xr-x 2 jan users 6 Mar 3 11:52 bin -rw-r--r-- 1 jan users 13 Jun 29 07:47 hostname -rw-r--r-- 1 jan users 164 Jun 29 07:47 os-release

Ah, that's awesome 👍

AkihiroSuda · 2021-06-29T07:42:50Z

cmd/limactl/copy.go

+	for _, arg := range clicontext.Args().Slice() {
+		path := strings.Split(arg, ":")
+		switch len(path) {
+		case 1:


Can we require / to be included in a host file path?

Because we may want limactl cp inst1 inst2 to duplicate an instance in future.

Can we require / to be included in a host file path?

We could, but it feels slightly wrong to do so. Wouldn't it make more sense to say if none of the arguments contain a colon (i.e. all arguments are host references), then the arguments must be 2 instance names, of which the first one must already exist, and the second one must not?

Because limactl cp inst1 inst2 would otherwise just be a local copy between 2 files, which doesn't need limactl cp, you can use just plain cp for that. 😄

Ok, I guess we are unlikely going to overload limactl cp for copying instances.
For copying instances we can just have limactl clone inst1 inst2 or maybe limactl copy-instance inst1 inst2.

If a user attempted to run limactl cp inst1 inst2, we can just print a warning.

Ok, I guess we are unlikely going to overload limactl cp for copying instances.
For copying instances we can just have limactl clone inst1 inst2 or maybe limactl copy-instance inst1 inst2.

Good. I don't really like overloading commands with unrelated functionality, but couldn't think of an "obvious" name for copying an instance. I think clone is perfect for this.

If a user attempted to run limactl cp inst1 inst2, we can just print a warning.

Not sure if we need to warn; limectl cp foo bar really does the same thing as cp foo bar right now, and makes sense to me: if source and dest can be both remote (guest), then why shouldn't they both be allowed to be local (host) as well? And if the source file does not exist, we already get a warning:

$ limactl cp foo bar cp: foo: No such file or directory exit status 1

This is a convenience wrapper around scp. Example: limactl cp alpine:/etc/os-release default:alpine-release Signed-off-by: Jan Dubois <[email protected]>

jandubois force-pushed the copy-command branch 3 times, most recently from 812dbb1 to 190d1b1 Compare June 29, 2021 01:15

AkihiroSuda reviewed Jun 29, 2021

View reviewed changes

cmd/limactl/copy.go Outdated Show resolved Hide resolved

AkihiroSuda reviewed Jun 29, 2021

View reviewed changes

jandubois force-pushed the copy-command branch from 190d1b1 to 2618195 Compare June 29, 2021 02:56

jandubois added the enhancement New feature or request label Jun 29, 2021

AkihiroSuda added this to the v0.5.0 milestone Jun 29, 2021

jandubois force-pushed the copy-command branch 2 times, most recently from 0a05876 to 0d8ecf1 Compare June 29, 2021 07:32

AkihiroSuda reviewed Jun 29, 2021

View reviewed changes

AkihiroSuda approved these changes Jun 29, 2021

View reviewed changes

Add copy/cp/scp command

cfe3cc9

This is a convenience wrapper around scp. Example: limactl cp alpine:/etc/os-release default:alpine-release Signed-off-by: Jan Dubois <[email protected]>

jandubois force-pushed the copy-command branch from 0d8ecf1 to cfe3cc9 Compare June 29, 2021 08:12

AkihiroSuda merged commit 2c82e4f into lima-vm:master Jun 29, 2021

jandubois deleted the copy-command branch June 29, 2021 17:15

AkihiroSuda changed the title ~~Add copy/cp/scp command~~ Add copy/cp command Jul 6, 2021

Add copy/cp command #92

Add copy/cp command #92

Uh oh!

Conversation

jandubois commented Jun 29, 2021

Uh oh!

Uh oh!

AkihiroSuda commented Jun 29, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jandubois commented Jun 29, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jandubois Jun 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jandubois Jun 29, 2021 •

edited

Loading