From 2313b72a7da5eeb2b046e69af8b8f8f171b99f58 Mon Sep 17 00:00:00 2001 From: tstromberg Date: Wed, 12 Feb 2020 14:26:38 -0800 Subject: [PATCH 1/4] sync: Fix path confusion and directory creation bugs --- pkg/minikube/cluster/filesync.go | 18 +++++++++++++++ test/integration/functional_test.go | 34 +++++++++++++++++++++++------ 2 files changed, 45 insertions(+), 7 deletions(-) diff --git a/pkg/minikube/cluster/filesync.go b/pkg/minikube/cluster/filesync.go index 6bf4559335..e5606a2c94 100644 --- a/pkg/minikube/cluster/filesync.go +++ b/pkg/minikube/cluster/filesync.go @@ -19,6 +19,7 @@ package cluster import ( "fmt" "os" + "os/exec" "path" "path/filepath" @@ -37,6 +38,20 @@ func syncLocalAssets(cr command.Runner) error { return err } + dirs := map[string]bool{} + for _, f := range fs { + dirs[f.GetTargetDir()] = true + } + + args := []string{"mkdir", "-p"} + for k, _ := range dirs { + args = append(args, k) + } + cmd := exec.Command("sudo", args...) + if _, err := cr.RunCmd(cmd); err != nil { + return err + } + for _, f := range fs { err := cr.Copy(f) if err != nil { @@ -79,6 +94,9 @@ func assetsFromDir(localRoot string, destRoot string, flatten bool) ([]assets.Co return err } + // On Windows, rel will be separated by \, which is not correct inside the VM + rel = filepath.ToSlash(rel) + // The conversion will strip the leading 0 if present, so add it back if necessary ps := fmt.Sprintf("%o", fi.Mode().Perm()) if len(ps) == 3 { diff --git a/test/integration/functional_test.go b/test/integration/functional_test.go index df85356bb5..c3a83abe6e 100644 --- a/test/integration/functional_test.go +++ b/test/integration/functional_test.go @@ -56,7 +56,13 @@ func TestFunctional(t *testing.T) { profile := UniqueProfileName("functional") ctx, cancel := context.WithTimeout(context.Background(), 40*time.Minute) - defer CleanupWithLogs(t, profile, cancel) + defer func() { + p := localSyncTestPath() + if err := os.Remove(p); err != nil { + t.Logf("unable to remove %s: %v", p, err) + } + CleanupWithLogs(t, profile, cancel) + }() // Serial tests t.Run("serial", func(t *testing.T) { @@ -626,11 +632,21 @@ func validateMySQL(ctx context.Context, t *testing.T, profile string) { } } +// vmSyncTestPath is where the test file will be synced into the VM +func vmSyncTestPath() string { + return fmt.Sprintf("/etc/test/nested/copy/%d/hosts", os.Getpid()) +} + +// localSyncTestPath is where the test file will be synced into the VM +func localSyncTestPath() string { + return filepath.Join(localpath.MiniPath(), "/files", vmSyncTestPath()) +} + // Copy extra file into minikube home folder for file sync test func setupFileSync(ctx context.Context, t *testing.T, profile string) { - // 1. copy random file to MINIKUBE_HOME/files/etc - f := filepath.Join(localpath.MiniPath(), "/files/etc/sync.test") - err := copy.Copy("./testdata/sync.test", f) + p := localSyncTestPath() + t.Logf("local sync path: %s", p) + err := copy.Copy("./testdata/sync.test", p) if err != nil { t.Fatalf("copy: %v", err) } @@ -641,18 +657,22 @@ func validateFileSync(ctx context.Context, t *testing.T, profile string) { if NoneDriver() { t.Skipf("skipping: ssh unsupported by none") } - // check file existence - rr, err := Run(t, exec.CommandContext(ctx, Target(), "-p", profile, "ssh", "cat /etc/sync.test")) + + vp := vmSyncTestPath() + t.Logf("Checking for existence of %s within VM", vp) + rr, err := Run(t, exec.CommandContext(ctx, Target(), "-p", profile, "ssh", fmt.Sprintf("cat %s", vp))) if err != nil { t.Errorf("%s failed: %v", rr.Args, err) } + got := rr.Stdout.String() + t.Logf("file sync test content: %s", got) expected, err := ioutil.ReadFile("./testdata/sync.test") if err != nil { t.Errorf("test file not found: %v", err) } - if diff := cmp.Diff(string(expected), rr.Stdout.String()); diff != "" { + if diff := cmp.Diff(string(expected), got); diff != "" { t.Errorf("/etc/sync.test content mismatch (-want +got):\n%s", diff) } } From 52eec8cf7776bf2fdb25851da0c617537f748dcb Mon Sep 17 00:00:00 2001 From: tstromberg Date: Wed, 12 Feb 2020 15:12:02 -0800 Subject: [PATCH 2/4] Add more tests --- pkg/minikube/cluster/filesync.go | 34 +++++++++++++++++---------- pkg/minikube/cluster/filesync_test.go | 30 +++++++++++++++++++++++ 2 files changed, 52 insertions(+), 12 deletions(-) diff --git a/pkg/minikube/cluster/filesync.go b/pkg/minikube/cluster/filesync.go index e5606a2c94..74f63b3370 100644 --- a/pkg/minikube/cluster/filesync.go +++ b/pkg/minikube/cluster/filesync.go @@ -77,6 +77,23 @@ func localAssets() ([]assets.CopyableFile, error) { return fs, nil } +// syncDest returns the path within a VM for a local asset +func syncDest(localRoot string, localPath string, destRoot string, flatten bool) (string, error) { + rel, err := filepath.Rel(localRoot, localPath) + if err != nil { + return "", err + } + + // On Windows, rel will be separated by \, which is not correct inside the VM + rel = filepath.ToSlash(rel) + + // If flatten is set, dump everything into the same destination directory + if flatten { + return path.Join(destRoot, filepath.Base(localPath)), nil + } + return path.Join(destRoot, rel), nil +} + // assetsFromDir generates assets from a local filepath, with/without a flattened hierarchy func assetsFromDir(localRoot string, destRoot string, flatten bool) ([]assets.CopyableFile, error) { glog.Infof("Scanning %s for local assets ...", localRoot) @@ -89,26 +106,19 @@ func assetsFromDir(localRoot string, destRoot string, flatten bool) ([]assets.Co return nil } - rel, err := filepath.Rel(localRoot, localPath) - if err != nil { - return err - } - - // On Windows, rel will be separated by \, which is not correct inside the VM - rel = filepath.ToSlash(rel) - // The conversion will strip the leading 0 if present, so add it back if necessary ps := fmt.Sprintf("%o", fi.Mode().Perm()) if len(ps) == 3 { ps = fmt.Sprintf("0%s", ps) } - dest := path.Join(destRoot, rel) + dest, err := syncDest(localRoot, localPath, destRoot, flatten) + if err != nil { + return err + } targetDir := path.Dir(dest) targetName := path.Base(dest) - if flatten { - targetDir = destRoot - } + glog.Infof("local asset: %s -> %s in %s", localPath, targetName, targetDir) f, err := assets.NewFileAsset(localPath, targetDir, targetName, ps) if err != nil { diff --git a/pkg/minikube/cluster/filesync_test.go b/pkg/minikube/cluster/filesync_test.go index 5898afbf66..2f222264d9 100644 --- a/pkg/minikube/cluster/filesync_test.go +++ b/pkg/minikube/cluster/filesync_test.go @@ -158,3 +158,33 @@ func TestAssetsFromDir(t *testing.T) { } } + +func TestSyncDest(t *testing.T) { + tests := []struct { + description string + localParts []string + destRoot string + flatten bool + want string + }{ + {"simple", []string{"etc", "hosts"}, "/", false, "/etc/hosts"}, + {"nested", []string{"etc", "nested", "hosts"}, "/", false, "/etc/nested/hosts"}, + {"flat", []string{"etc", "nested", "hosts"}, "/test", true, "/test/hosts"}, + } + + for _, test := range tests { + t.Run(test.description, func(t *testing.T) { + // Generate paths using filepath to mimic OS-specific issues + localRoot := localpath.MakeMiniPath("sync") + localParts := append([]string{localRoot}, test.localParts...) + localPath := filepath.Join(localParts...) + got, err := syncDest(localRoot, localPath, test.destRoot, test.flatten) + if err != nil { + t.Fatalf("syncDest(%s, %s, %v) unexpected err: %v", localRoot, localPath, test.flatten, err) + } + if got != test.want { + t.Errorf("syncDest(%s, %s, %v) = %s, want: %s", localRoot, localPath, test.flatten, got, test.want) + } + }) + } +} From 1f346e43e815c720f24bbe67dc533fc56f077311 Mon Sep 17 00:00:00 2001 From: tstromberg Date: Wed, 12 Feb 2020 15:20:58 -0800 Subject: [PATCH 3/4] Remove unnecessary range arg --- pkg/minikube/cluster/filesync.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/minikube/cluster/filesync.go b/pkg/minikube/cluster/filesync.go index 74f63b3370..c8356debb2 100644 --- a/pkg/minikube/cluster/filesync.go +++ b/pkg/minikube/cluster/filesync.go @@ -44,7 +44,7 @@ func syncLocalAssets(cr command.Runner) error { } args := []string{"mkdir", "-p"} - for k, _ := range dirs { + for k := range dirs { args = append(args, k) } cmd := exec.Command("sudo", args...) From 19695856eee49cc722ddda4325a5e84d9d2c4644 Mon Sep 17 00:00:00 2001 From: Thomas Stromberg Date: Thu, 13 Feb 2020 11:47:19 -0800 Subject: [PATCH 4/4] Skip mkdir if there is no work to be done --- pkg/minikube/cluster/filesync.go | 37 ++++++++++++++++++++++++-------- 1 file changed, 28 insertions(+), 9 deletions(-) diff --git a/pkg/minikube/cluster/filesync.go b/pkg/minikube/cluster/filesync.go index c8356debb2..7007631426 100644 --- a/pkg/minikube/cluster/filesync.go +++ b/pkg/minikube/cluster/filesync.go @@ -31,6 +31,15 @@ import ( "k8s.io/minikube/pkg/minikube/vmpath" ) +// guaranteed are directories we don't need to attempt recreation of +var guaranteed = map[string]bool{ + "/": true, + "": true, + "/etc": true, + "/var": true, + "/tmp": true, +} + // syncLocalAssets syncs files from MINIKUBE_HOME into the cluster func syncLocalAssets(cr command.Runner) error { fs, err := localAssets() @@ -38,20 +47,30 @@ func syncLocalAssets(cr command.Runner) error { return err } - dirs := map[string]bool{} + if len(fs) == 0 { + return nil + } + + // Deduplicate the list of directories to create + seen := map[string]bool{} + create := []string{} for _, f := range fs { - dirs[f.GetTargetDir()] = true + dir := f.GetTargetDir() + if guaranteed[dir] || seen[dir] { + continue + } + create = append(create, dir) } - args := []string{"mkdir", "-p"} - for k := range dirs { - args = append(args, k) - } - cmd := exec.Command("sudo", args...) - if _, err := cr.RunCmd(cmd); err != nil { - return err + // Create directories that are not guaranteed to exist + if len(create) > 0 { + args := append([]string{"mkdir", "-p"}, create...) + if _, err := cr.RunCmd(exec.Command("sudo", args...)); err != nil { + return err + } } + // Copy the files into place for _, f := range fs { err := cr.Copy(f) if err != nil {