Skip to content

add rsync flag option to copy files using rsync #3143

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

add rsync flag option to copy files using rsync #3143

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
148 changes: 127 additions & 21 deletions cmd/limactl/copy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,13 @@ Prefix guest filenames with the instance name and a colon.
Example: limactl copy default:/etc/os-release .
`

type copyTool string

const (
rsync copyTool = "rsync"
scp copyTool = "scp"
)

func newCopyCommand() *cobra.Command {
copyCommand := &cobra.Command{
Use: "copy SOURCE ... TARGET",
Expand Down Expand Up @@ -49,13 +56,6 @@ func copyAction(cmd *cobra.Command, args []string) error {
return err
}

arg0, err := exec.LookPath("scp")
if err != nil {
return err
}
instances := make(map[string]*store.Instance)
scpFlags := []string{}
scpArgs := []string{}
debug, err := cmd.Flags().GetBool("debug")
if err != nil {
return err
Expand All @@ -65,6 +65,45 @@ func copyAction(cmd *cobra.Command, args []string) error {
verbose = true
}

cpTool := rsync
arg0, err := exec.LookPath(string(cpTool))
if err != nil {
arg0, err = exec.LookPath(string(cpTool))
if err != nil {
return err
}
}
logrus.Infof("using copy tool %q", arg0)

var copyCmd *exec.Cmd
switch cpTool {
case scp:
copyCmd, err = scpCommand(arg0, args, verbose, recursive)
case rsync:
copyCmd, err = rsyncCommand(arg0, args, verbose, recursive)
default:
err = fmt.Errorf("invalid copy tool %q", cpTool)
}
if err != nil {
return err
}

copyCmd.Stdin = cmd.InOrStdin()
copyCmd.Stdout = cmd.OutOrStdout()
copyCmd.Stderr = cmd.ErrOrStderr()
logrus.Debugf("executing %v (may take a long time)", copyCmd)

// TODO: use syscall.Exec directly (results in losing tty?)
return copyCmd.Run()
}

func scpCommand(command string, args []string, verbose, recursive bool) (*exec.Cmd, error) {
instances := make(map[string]*store.Instance)

scpFlags := []string{}
scpArgs := []string{}
var err error

if verbose {
scpFlags = append(scpFlags, "-v")
} else {
Expand All @@ -74,6 +113,7 @@ func copyAction(cmd *cobra.Command, args []string) error {
if recursive {
scpFlags = append(scpFlags, "-r")
}

// this assumes that ssh and scp come from the same place, but scp has no -V
legacySSH := sshutil.DetectOpenSSHVersion("ssh").LessThan(*semver.New("8.0.0"))
for _, arg := range args {
Expand All @@ -86,12 +126,12 @@ func copyAction(cmd *cobra.Command, args []string) error {
inst, err := store.Inspect(instName)
if err != nil {
if errors.Is(err, os.ErrNotExist) {
return fmt.Errorf("instance %q does not exist, run `limactl create %s` to create a new instance", instName, instName)
return nil, fmt.Errorf("instance %q does not exist, run `limactl create %s` to create a new instance", instName, instName)
}
return err
return nil, err
}
if inst.Status == store.StatusStopped {
return fmt.Errorf("instance %q is stopped, run `limactl start %s` to start the instance", instName, instName)
return nil, fmt.Errorf("instance %q is stopped, run `limactl start %s` to start the instance", instName, instName)
}
if legacySSH {
scpFlags = append(scpFlags, "-P", fmt.Sprintf("%d", inst.SSHLocalPort))
Expand All @@ -101,11 +141,11 @@ func copyAction(cmd *cobra.Command, args []string) error {
}
instances[instName] = inst
default:
return fmt.Errorf("path %q contains multiple colons", arg)
return nil, fmt.Errorf("path %q contains multiple colons", arg)
}
}
if legacySSH && len(instances) > 1 {
return errors.New("more than one (instance) host is involved in this command, this is only supported for openSSH v8.0 or higher")
return nil, errors.New("more than one (instance) host is involved in this command, this is only supported for openSSH v8.0 or higher")
}
scpFlags = append(scpFlags, "-3", "--")
scpArgs = append(scpFlags, scpArgs...)
Expand All @@ -118,24 +158,90 @@ func copyAction(cmd *cobra.Command, args []string) error {
for _, inst := range instances {
sshOpts, err = sshutil.SSHOpts("ssh", inst.Dir, *inst.Config.User.Name, false, false, false, false)
if err != nil {
return err
return nil, err
}
}
} else {
// Copying among multiple hosts; we can't pass in host-specific options.
sshOpts, err = sshutil.CommonOpts("ssh", false)
if err != nil {
return err
return nil, err
}
}
sshArgs := sshutil.SSHArgsFromOpts(sshOpts)

sshCmd := exec.Command(arg0, append(sshArgs, scpArgs...)...)
sshCmd.Stdin = cmd.InOrStdin()
sshCmd.Stdout = cmd.OutOrStdout()
sshCmd.Stderr = cmd.ErrOrStderr()
logrus.Debugf("executing scp (may take a long time): %+v", sshCmd.Args)
return exec.Command(command, append(sshArgs, scpArgs...)...), nil
}

// TODO: use syscall.Exec directly (results in losing tty?)
return sshCmd.Run()
func rsyncCommand(command string, args []string, verbose, recursive bool) (*exec.Cmd, error) {
instances := make(map[string]*store.Instance)

var instName string

rsyncFlags := []string{}
rsyncArgs := []string{}

if verbose {
rsyncFlags = append(rsyncFlags, "-v", "--progress")
} else {
rsyncFlags = append(rsyncFlags, "-q")
}

if recursive {
rsyncFlags = append(rsyncFlags, "-r")
}

for _, arg := range args {
path := strings.Split(arg, ":")
switch len(path) {
case 1:
inst, ok := instances[instName]
Copy link
Member

Choose a reason for hiding this comment

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

How this will work? instances is an empty map created in this function.

I think we have only one case - instance:path, and it is common to both scp and rsync, so this check should be done first before we consider the copy tool. Input validation must always be done first.

if !ok {
return nil, fmt.Errorf("instance %q does not exist, run `limactl create %s` to create a new instance", instName, instName)
}
guestVM := fmt.Sprintf("%s@127.0.0.1:%s", *inst.Config.User.Name, path[0])
rsyncArgs = append(rsyncArgs, guestVM)
case 2:
instName = path[0]
inst, err := store.Inspect(instName)
if err != nil {
if errors.Is(err, os.ErrNotExist) {
return nil, fmt.Errorf("instance %q does not exist, run `limactl create %s` to create a new instance", instName, instName)
}
return nil, err
}
sshOpts, err := sshutil.SSHOpts("ssh", inst.Dir, *inst.Config.User.Name, false, false, false, false)
if err != nil {
return nil, err
}

sshArgs := sshutil.SSHArgsFromOpts(sshOpts)
sshStr := fmt.Sprintf("ssh -p %s %s", fmt.Sprintf("%d", inst.SSHLocalPort), strings.Join(sshArgs, " "))

destDir := args[1]
mkdirCmd := exec.Command(
"ssh",
"-p", fmt.Sprintf("%d", inst.SSHLocalPort),
)
mkdirCmd.Args = append(mkdirCmd.Args, sshArgs...)
mkdirCmd.Args = append(mkdirCmd.Args,
fmt.Sprintf("%s@%s", *inst.Config.User.Name, "127.0.0.1"),
fmt.Sprintf("sudo mkdir -p %q && sudo chown %s:%s %s", destDir, *inst.Config.User.Name, *inst.Config.User.Name, destDir),
)
mkdirCmd.Stdout = os.Stdout
mkdirCmd.Stderr = os.Stderr
if err := mkdirCmd.Run(); err != nil {
return nil, fmt.Errorf("failed to create directory %q on remote: %w", destDir, err)
}

rsyncArgs = append(rsyncArgs, "-avz", "-e", sshStr, path[1])
instances[instName] = inst
Copy link
Member

Choose a reason for hiding this comment

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

I see, you add the instance to the map so the next iteration will find it. This is very complicated and hard to follow way to handle the arguments.

We should instead process the argument first, and convert them to internal structure that will be used to construct the command. This should be common to scp and rsync, so it should be done before we consider the tool.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should instead process the argument first, and convert them to internal structure that will be used to construct the command. This should be common to scp and rsync, so it should be done before we consider the tool.

The argument processing for scp and rsync is quite different, so combining them into a single function could make it complex and harder to maintain. II think it’s better to keep them separate and focus on consistent output structures instead.

default:
return nil, fmt.Errorf("path %q contains multiple colons", arg)
}
}

rsyncArgs = append(rsyncFlags, rsyncArgs...)

return exec.Command(command, rsyncArgs...), nil
}
3 changes: 3 additions & 0 deletions pkg/cidata/cidata.TEMPLATE.d/user-data
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@ package_upgrade: true
package_reboot_if_required: true
{{- end }}

packages:
- rsync

{{- if or .RosettaEnabled (or (eq .MountType "9p") (eq .MountType "virtiofs")) }}
mounts:
{{- if .RosettaEnabled }}{{/* Mount the rosetta volume before systemd-binfmt.service(8) starts */}}
Expand Down
1 change: 1 addition & 0 deletions pkg/hostagent/hostagent.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -439,6 +439,7 @@ func (a *HostAgent) startHostAgentRoutines(ctx context.Context) error {
if err := a.waitForRequirements("essential", a.essentialRequirements()); err != nil {
errs = append(errs, err)
}

if *a.instConfig.SSH.ForwardAgent {
faScript := `#!/bin/bash
set -eux -o pipefail
Expand Down
Loading