Skip to content
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

Jump to bottom

[virtctl] Add a scp subcommand to virtctl #7441

Merged
merged 9 commits into from Apr 23, 2022
Merged

[virtctl] Add a scp subcommand to virtctl #7441

merged 9 commits into from Apr 23, 2022

Conversation

rmohr
Copy link
Member

@rmohr rmohr commented Mar 24, 2022

What this PR does / why we need it:

Allow copying files and directories between local machines and VMIs and VMs. The remote VMs need to have a running ssh server and scp installed.

Invocations may look like this:

# Copy a file to the remote home folder of user jdoe
virtctl scp myfile.bin jdoe@testvmi:myfile.bin
# Copy a directory to the remote home folder of user jdoe
virtctl scp --recursive ~/mydir/ jdoe@testvmi:./mydir
# Copy a file to the remote home folder of user jdoe without specifying a file name on the target
virtctl scp myfile.bin jdoe@testvmi:.
# Copy a file to 'testvm' in 'mynamespace' namespace
virtctl scp myfile.bin jdoe@testvmi.mynamespace:myfile.bin
# Copy a file from the remote location to a local folder
virtctl scp jdoe@testvmi:myfile.bin ~/myfile.bin`

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Release note:

Add `virtctl scp` to ease copying files from and to VMs and VMIs

@kubevirt-bot kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/XXL labels Mar 24, 2022
@rmohr rmohr changed the title Add a scp subcommand to virtctl [virtctl] Add a scp subcommand to virtctl Mar 24, 2022
@rmohr
Copy link
Member Author

rmohr commented Mar 24, 2022

/retest

@rmohr
Copy link
Member Author

rmohr commented Mar 25, 2022

/cc @dominikholler

FYI

@rmohr
Copy link
Member Author

rmohr commented Mar 25, 2022

/cc @0xFelix

@kubevirt-bot kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 1, 2022
0xFelix
0xFelix suggested changes Apr 4, 2022
View reviewed changes
Copy link
Member

@0xFelix 0xFelix left a comment

Choose a reason for hiding this comment

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

Overall looks good to me. Just a remark about the recursive option and a nit about the import order.

Could this also be implemented with the local scp binary wrapped like the local ssh binary in the ssh client?

pkg/virtctl/root.go Outdated Show resolved Hide resolved
pkg/virtctl/scp/scp.go Show resolved Hide resolved
pkg/virtctl/scp/scp.go Show resolved Hide resolved
pkg/virtctl/ssh/native.go Outdated Show resolved Hide resolved
pkg/virtctl/ssh/ssh.go Outdated Show resolved Hide resolved
tests/virtctl/scp.go Show resolved Hide resolved
@dominikholler
Copy link

@rmohr Is there any chance to allow using the native scp command?

@rmohr
Copy link
Member Author

rmohr commented Apr 11, 2022

@rmohr Is there any chance to allow using the native scp command?

I don't mind if someone wants to add it :)

@kubevirt-bot kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 11, 2022
@rmohr rmohr force-pushed the scp branch 2 times, most recently from 0cc4119 to 7ff32bc Compare April 11, 2022 13:44
@rmohr
Copy link
Member Author

rmohr commented Apr 11, 2022

/retest

@kubevirt-bot kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 11, 2022
@kubevirt-bot kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 13, 2022
@rmohr rmohr force-pushed the scp branch 2 times, most recently from 7fec7eb to f2e0e2b Compare April 13, 2022 09:16
@rmohr
Copy link
Member Author

rmohr commented Apr 13, 2022

/retest

@rmohr
Copy link
Member Author

rmohr commented Apr 14, 2022

/retest

@rmohr rmohr requested a review from 0xFelix April 19, 2022 13:43
0xFelix
0xFelix reviewed Apr 20, 2022
View reviewed changes
Copy link
Member

@0xFelix 0xFelix left a comment

Choose a reason for hiding this comment

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

I think a bit of logic regarding files is missing.

pkg/virtctl/scp/scp.go Show resolved Hide resolved
pkg/virtctl/scp/scp.go Show resolved Hide resolved
pkg/virtctl/scp/scp.go Outdated
return err
}
} else {
err = scpClient.CopyFileToRemote(local.Path, remote.Path, &scp.FileTransferOption{PreserveProp: o.preserve})
Copy link
Member

Choose a reason for hiding this comment

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

This could be a dir using CopyFileToRemote. What would happen then?

Copy link
Member Author

Choose a reason for hiding this comment

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

It will fail.

Copy link
Member

Choose a reason for hiding this comment

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

With a comprehensible error message?

Copy link
Member Author

Choose a reason for hiding this comment

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

Improved.

pkg/virtctl/scp/scp.go Show resolved Hide resolved
pkg/virtctl/scp/scp.go Show resolved Hide resolved
@rmohr
Copy link
Member Author

rmohr commented Apr 21, 2022

I think a bit of logic regarding files is missing.

Yeah thanks for the detailed analysis. I think in the case of the --recursive flag I really love the current behaviour compared to scp. It is like cp behaves and I would want to try this more consistent behaviour, unless there are concerns. We can also change it if it gets requested, scp is new and not yet "stable" immediately. What do you think?

@rmohr
Copy link
Member Author

rmohr commented Apr 21, 2022

/retest

@0xFelix
Copy link
Member

0xFelix commented Apr 21, 2022

I have no concerns about the behavior. Only the error messages should be clear.

@rmohr
Properly isolate native ssh client for better reuse
116839e 
Signed-off-by: Roman Mohr <rmohr@redhat.com>
@rmohr
Isolate common command creation parts which require ssh
d266cbc 
Signed-off-by: Roman Mohr <rmohr@redhat.com>
@rmohr
Add scp command to virtctl
808bd84 
Signed-off-by: Roman Mohr <rmohr@redhat.com>
@rmohr
Add directory copy support to virtctl scp
4a338da 
Signed-off-by: Roman Mohr <rmohr@redhat.com>
@rmohr
Add basic scp e2e test
c266397 
Signed-off-by: Roman Mohr <rmohr@redhat.com>
@rmohr
Allow preserving timestamps and file modes
90f95ef 
Signed-off-by: Roman Mohr <rmohr@redhat.com>
@rmohr
Implement local directory management
7148d43 
copy-dir-to-remote: Fail with a clean error message early if the local
path is not a directory.
copy-dir-to-local: Create local directory if it does not exist or
determine if a sub-directory needs to be created and create it.

Signed-off-by: Roman Mohr <rmohr@redhat.com>
@rmohr
Add the github.com/povsister/scp dependency
f8e4d51 
Signed-off-by: Roman Mohr <rmohr@redhat.com>
@rmohr
Add e2e test to validate directory copy
534f482 
Signed-off-by: Roman Mohr <rmohr@redhat.com>
@rmohr
Copy link
Member Author

rmohr commented Apr 21, 2022

I have no concerns about the behavior. Only the error messages should be clear.

👍 improved.

@rmohr
Copy link
Member Author

rmohr commented Apr 21, 2022

/retest

@0xFelix
Copy link
Member

0xFelix commented Apr 22, 2022

Great! 👍

/lgtm
/retest

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 22, 2022
@rmohr
Copy link
Member Author

rmohr commented Apr 22, 2022

/approve

@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rmohr

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 22, 2022
@kubevirt-commenter-bot
Copy link

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@rmohr
Copy link
Member Author

rmohr commented Apr 22, 2022

/retest

@kubevirt-commenter-bot
Copy link

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

3 similar comments
@kubevirt-commenter-bot
Copy link

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@kubevirt-commenter-bot
Copy link

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@kubevirt-commenter-bot
Copy link

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@kubevirt-bot kubevirt-bot merged commit fc50931 into kubevirt:main Apr 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xFelix 0xFelix 0xFelix requested changes

@dominikholler dominikholler Awaiting requested review from dominikholler

Assignees

@0xFelix 0xFelix

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@rmohr @dominikholler @0xFelix @kubevirt-bot @kubevirt-commenter-bot