new command: `minikube cp` to copy files into minikube #10198

anencore94 · 2021-01-21T09:58:10Z

Add New Feature which represents scp command

Until now, it is possible to copy specific local file to specific path in minikube basically.
I mean 'basically' since it is possible to only file not directory.
And there are many more remaining tasks until now. (remaining problems are written in source code as //TODO)

Fix #9930

k8s-ci-robot · 2021-01-21T09:58:18Z

Hi @anencore94. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

anencore94 · 2021-01-21T10:01:28Z

Before coding some other feature such as 'enable -r option to copy directory', 'enable progress bar option' and the others, I want to get some reviews from you guys to check whether I am going the right way.

I also wonder how to implement the test code for this. Is it ok make only functional test for scp cmd at minikube/test/integration/functional_test.go ? or somewhere else? Please let me know.

Thanks a lot in advance!

anencore94 · 2021-01-21T10:01:45Z

/assign @tstromberg

minikube-bot · 2021-01-21T10:03:04Z

Can one of the admins verify this patch?

medyagh

please remove TODO comments and add integration test then it will be good to do !

medyagh · 2021-02-15T19:54:31Z

@anencore94 sorry for the late reply on this PR, this sounds like a good PR, lets remove the TODO notes and yes lets add an integration test to our Funcitonal Test

it could be a subtest simmilar to other tests


	// Parallelized tests
	t.Run("parallel", func(t *testing.T) {
		tests := []struct {
			name      string
			validator validateFunc
		}{
			{"ConfigCmd", validateConfigCmd},
			{"DashboardCmd", validateDashboardCmd},
			{"DryRun", validateDryRun},
// ...add your test here
			{"ScpCmd", validateScp},
..

and u can make a dummy hello world file and try to copy it using scp and validate the file was copied by

minikube ssh cat /tmp/hello_scp.txt

anencore94 · 2021-02-17T04:32:39Z

Thanks for your review @medyagh !

I added the functional test for scp cmd as your comment. Could you re-check my PR?

the functional test for "PR / functional_docker_windows (pull_request)" and "PR / functional_hyperv_windows (pull_request)" failed since the scp command does not support on windows i guess. (or because filepath format is different)
So I made the functional test to skip when the os is windows.

priyawadhwa

Instead of shelling out to the scp command line tool directly, let's instead copy files into minikube the way we do in other parts of the code (for things like preload, or kubernets binaries). Instead of scp:

Create a FileAsset with the NewFileAsset function
Using a runner, run the built in Copy() function to copy the file asset into minikube

This way anyone can copy files in, regardless of if they have scp installed. This should also allow this command to run across all platforms, not just Linux. So, you shouldn't have to skip the functional test anymore.

Example of this in the code:

minikube/pkg/minikube/cruntime/containerd.go

Lines 350 to 357 in 6afefe6

    
           fa, err := assets.NewFileAsset(tarballPath, targetDir, targetName, "0644") 
        
           if err != nil { 
        
           	return errors.Wrap(err, "getting file asset") 
        
           } 
        
           t := time.Now() 
        
           if err := r.Runner.Copy(fa); err != nil { 
        
           	return errors.Wrap(err, "copying file") 
        
           }

If you need any more examples, searching for any call to asset.NewFileAsset should get you there.

medyagh · 2021-02-24T19:23:00Z

Instead of shelling out to the scp flag directly, let's instead copy files into minikube the way we do in other parts of the code (for things like preload, or kubernets binaries). Instead of scp:

Create a FileAsset with the NewFileAsset function

Using a runner, run the built in Copy() function to copy the file asset into minikube

This way anyone can copy files in, regardless of if they have scp installed. This should also allow this command to run across all platforms, not just Linux. So, you shouldn't have to skip the functional test anymore.

Example of this in the code:

minikube/pkg/minikube/cruntime/containerd.go

Lines 350 to 357 in 6afefe6

fa, err := assets.NewFileAsset(tarballPath, targetDir, targetName, "0644")

if err != nil {

return errors.Wrap(err, "getting file asset")

}

t := time.Now()

if err := r.Runner.Copy(fa); err != nil {

return errors.Wrap(err, "copying file")

}

If you need any more examples, searching for any call to asset.NewFileAsset should get you there.

@anencore94 I agree with @priyawadhwa lets do that

priyawadhwa · 2021-02-24T19:47:12Z

Since we're no longer using scp I would probably also rename this command to minikube cp (matches docker so is more intuitive)

anencore94 · 2021-02-25T10:41:59Z

@priyawadhwa I changed the implementation of scp by copy with your review comments, Thanks!
By the way, it seems the function Copy of Runner doesn't support directory copy on its own. So If we want to copy directory, I guess we have to make directory and copy every files in for loop or some other way with Copy function.
Thus, for now, I just implement "minikube cp" for only file.

I want to implement the directory copying feature with another PR. (or someone else could do this)

priyawadhwa

A couple more comments, this is almost there!

priyawadhwa · 2021-02-26T19:21:35Z

cmd/minikube/cmd/cp.go

+
+// cpCmd represents the cp command, similar to docker cp
+var cpCmd = &cobra.Command{
+	Use:   "cp <source file path> <target file path followed by '/home/docker/'>",


I'm a bit confused by this description, I thought the command would work something like:

minikube cp a.txt. /b.txt

Where does the /home/docker come in?

Additionally, we should probably insist that the second argument is an absolute path.

I agree with you the prefix_path '/home/docker/' is confusing :)
By the way, when we do 'minikube ssh', the starting path is '/home/docker' since the default username is docker.
Is it ok to copy on '/somewhere' where the permission is only for 'root user' ?

I think there maybe 3 options: (or you have any idea, please let me know :) )

minikube cp a.txt /home/docker/b.txt

make minikube user to write '/home/docker' to see exactly where the path is.

But, they should write the /home/docker path for every copying

minikube cp a.txt /b.txt

enable copy at root permissioned directory

minikube cp a.txt b.txt

minikube user doesn't care about the user name, but always will be at /home/docker/b.txt as what i've implemented with this version

What Option would be the best ? @priyawadhwa
In my opinion for now, option 1 or option 3 will be safe.

I think we should ask the user for an absolute path and try to copy to wherever they want the file copied. If it fails because the command requires sudo, then we should provide a --force or -f flag which will copy over the file with root permissions.

Thanks for your comment.
I've changed to minikube copy can copy anywhere in minikube as your opinion.
Could you re-check? @priyawadhwa

cmd/minikube/cmd/cp.go

medyagh

@anencore94 can u plz rebase the PR (pull upstream) ?

medyagh · 2021-03-06T06:36:27Z

/ok-to-test

minikube-pr-bot · 2021-03-06T06:52:59Z

kvm2 Driver
error collecting results for kvm2 driver: timing run 0 with Minikube (PR 10198): timing cmd: [/home/performance-monitor/.minikube/minikube-binaries/10198/minikube start --driver=kvm2]: starting cmd: fork/exec /home/performance-monitor/.minikube/minikube-binaries/10198/minikube: exec format error
docker Driver
error collecting results for docker driver: timing run 0 with Minikube (PR 10198): timing cmd: [/home/performance-monitor/.minikube/minikube-binaries/10198/minikube start --driver=docker]: starting cmd: fork/exec /home/performance-monitor/.minikube/minikube-binaries/10198/minikube: exec format error

- add new feature represents cp local file into minikube - add functional test for cp command Signed-off-by: anencore94 <anencore94@kaist.ac.kr>

- only absolute path is allowed Signed-off-by: anencore94 <anencore94@kaist.ac.kr>

anencore94 · 2021-03-08T02:37:56Z

@anencore94 can u plz rebase the PR (pull upstream) ?

Thanks, I've done @medyagh

medyagh · 2021-03-23T19:42:58Z

thank you @anencore94

k8s-ci-robot · 2021-03-23T19:43:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: anencore94, medyagh

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:

~~OWNERS~~ [medyagh]

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

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 21, 2021

k8s-ci-robot requested review from ilya-zuyev and tstromberg January 21, 2021 09:58

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 21, 2021

k8s-ci-robot assigned tstromberg Jan 21, 2021

anencore94 changed the title ~~[WIP][Feature] Add minikube scp command~~ [WIP] Add minikube scp command as new feature Jan 22, 2021

medyagh requested changes Feb 15, 2021

View reviewed changes

anencore94 force-pushed the minikube-scp branch 2 times, most recently from ae54fe2 to 5719576 Compare February 16, 2021 10:21

anencore94 changed the title ~~[WIP] Add minikube scp command as new feature~~ Add minikube scp command as new feature Feb 16, 2021

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 16, 2021

anencore94 force-pushed the minikube-scp branch 5 times, most recently from 245fda7 to 12fa99c Compare February 17, 2021 04:30

anencore94 requested a review from medyagh February 17, 2021 07:25

priyawadhwa suggested changes Feb 24, 2021

View reviewed changes

anencore94 force-pushed the minikube-scp branch 2 times, most recently from 4da3879 to 2e1635b Compare February 25, 2021 07:29

anencore94 changed the title ~~Add minikube scp command as new feature~~ Add minikube cp command as new feature Feb 25, 2021

anencore94 requested a review from priyawadhwa February 25, 2021 10:42

priyawadhwa suggested changes Feb 26, 2021

View reviewed changes

anencore94 force-pushed the minikube-scp branch from 3b444db to 6a92d90 Compare March 4, 2021 01:39

medyagh reviewed Mar 6, 2021

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 6, 2021

anencore94 added 2 commits March 8, 2021 10:51

[Feature] Add minikube cp command

76f3c72

- add new feature represents cp local file into minikube - add functional test for cp command Signed-off-by: anencore94 <anencore94@kaist.ac.kr>

make cp commmand allow target path as absolutepath

109b1e0

- only absolute path is allowed Signed-off-by: anencore94 <anencore94@kaist.ac.kr>

anencore94 force-pushed the minikube-scp branch from 6a92d90 to 109b1e0 Compare March 8, 2021 01:51

anencore94 requested a review from medyagh March 8, 2021 02:37

medyagh approved these changes Mar 23, 2021

View reviewed changes

medyagh merged commit 3e869e0 into kubernetes:master Mar 23, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 23, 2021

medyagh changed the title ~~Add minikube cp command as new feature~~ new command: minikube cp to copy files into minikube Mar 23, 2021

anencore94 deleted the minikube-scp branch March 24, 2021 04:10

medyagh mentioned this pull request Mar 24, 2021

TestFunctional/parallel/CpCmd failing on Windows: cat: /home/docker/hello_cp.txt: No such file or directory #10914

Closed

anencore94 mentioned this pull request Mar 25, 2021

Fix to minikube cp functional test work on windows os #10920

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new command: `minikube cp` to copy files into minikube #10198

new command: `minikube cp` to copy files into minikube #10198

anencore94 commented Jan 21, 2021 •

edited

Loading

k8s-ci-robot commented Jan 21, 2021

anencore94 commented Jan 21, 2021 •

edited

Loading

anencore94 commented Jan 21, 2021

minikube-bot commented Jan 21, 2021

medyagh left a comment

medyagh commented Feb 15, 2021

anencore94 commented Feb 17, 2021 •

edited

Loading

priyawadhwa left a comment •

edited

Loading

medyagh commented Feb 24, 2021

priyawadhwa commented Feb 24, 2021

anencore94 commented Feb 25, 2021

priyawadhwa left a comment

priyawadhwa Feb 26, 2021

anencore94 Feb 27, 2021 •

edited

Loading

anencore94 Mar 2, 2021

priyawadhwa Mar 2, 2021

anencore94 Mar 4, 2021

medyagh left a comment

medyagh commented Mar 6, 2021

minikube-pr-bot commented Mar 6, 2021

anencore94 commented Mar 8, 2021

medyagh commented Mar 23, 2021

k8s-ci-robot commented Mar 23, 2021

	fa, err := assets.NewFileAsset(tarballPath, targetDir, targetName, "0644")
	if err != nil {
	return errors.Wrap(err, "getting file asset")
	}
	t := time.Now()
	if err := r.Runner.Copy(fa); err != nil {
	return errors.Wrap(err, "copying file")
	}

new command: minikube cp to copy files into minikube #10198

new command: minikube cp to copy files into minikube #10198

Conversation

anencore94 commented Jan 21, 2021 • edited Loading

k8s-ci-robot commented Jan 21, 2021

anencore94 commented Jan 21, 2021 • edited Loading

anencore94 commented Jan 21, 2021

minikube-bot commented Jan 21, 2021

medyagh left a comment

Choose a reason for hiding this comment

medyagh commented Feb 15, 2021

anencore94 commented Feb 17, 2021 • edited Loading

priyawadhwa left a comment • edited Loading

Choose a reason for hiding this comment

medyagh commented Feb 24, 2021

priyawadhwa commented Feb 24, 2021

anencore94 commented Feb 25, 2021

priyawadhwa left a comment

Choose a reason for hiding this comment

priyawadhwa Feb 26, 2021

Choose a reason for hiding this comment

anencore94 Feb 27, 2021 • edited Loading

Choose a reason for hiding this comment

anencore94 Mar 2, 2021

Choose a reason for hiding this comment

priyawadhwa Mar 2, 2021

Choose a reason for hiding this comment

anencore94 Mar 4, 2021

Choose a reason for hiding this comment

medyagh left a comment

Choose a reason for hiding this comment

medyagh commented Mar 6, 2021

minikube-pr-bot commented Mar 6, 2021

anencore94 commented Mar 8, 2021

medyagh commented Mar 23, 2021

k8s-ci-robot commented Mar 23, 2021

new command: `minikube cp` to copy files into minikube #10198

new command: `minikube cp` to copy files into minikube #10198

anencore94 commented Jan 21, 2021 •

edited

Loading

anencore94 commented Jan 21, 2021 •

edited

Loading

anencore94 commented Feb 17, 2021 •

edited

Loading

priyawadhwa left a comment •

edited

Loading

anencore94 Feb 27, 2021 •

edited

Loading