From a3f4f27f481fba924ec7db49c40732a255fef3eb Mon Sep 17 00:00:00 2001 From: Priya Wadhwa Date: Wed, 10 Oct 2018 11:33:20 -0700 Subject: [PATCH 1/2] Move sync code to pkg/skaffold/sync/kubectl This is the correct place for this code. Might require some reorganization if we add other syncers, like rsync, but that shouldn't be difficult. --- pkg/skaffold/runner/runner.go | 3 ++- .../{kubernetes/sync.go => sync/kubectl/kubectl.go} | 5 +++-- .../sync_test.go => sync/kubectl/kubectl_test.go} | 7 ++++--- 3 files changed, 9 insertions(+), 6 deletions(-) rename pkg/skaffold/{kubernetes/sync.go => sync/kubectl/kubectl.go} (95%) rename pkg/skaffold/{kubernetes/sync_test.go => sync/kubectl/kubectl_test.go} (92%) diff --git a/pkg/skaffold/runner/runner.go b/pkg/skaffold/runner/runner.go index 0313adaa8f9..844383bd315 100644 --- a/pkg/skaffold/runner/runner.go +++ b/pkg/skaffold/runner/runner.go @@ -39,6 +39,7 @@ import ( kubectx "github.com/GoogleContainerTools/skaffold/pkg/skaffold/kubernetes/context" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/sync" + "github.com/GoogleContainerTools/skaffold/pkg/skaffold/sync/kubectl" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/test" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/watch" @@ -108,7 +109,7 @@ func NewForConfig(opts *config.SkaffoldOptions, cfg *latest.SkaffoldPipeline) (* Deployer: deployer, Tagger: tagger, Trigger: trigger, - Syncer: &kubernetes.KubectlSyncer{}, + Syncer: &kubectl.KubectlSyncer{}, opts: opts, watchFactory: watch.NewWatcher, }, nil diff --git a/pkg/skaffold/kubernetes/sync.go b/pkg/skaffold/sync/kubectl/kubectl.go similarity index 95% rename from pkg/skaffold/kubernetes/sync.go rename to pkg/skaffold/sync/kubectl/kubectl.go index 4e442c62921..3136835c3c7 100644 --- a/pkg/skaffold/kubernetes/sync.go +++ b/pkg/skaffold/sync/kubectl/kubectl.go @@ -14,13 +14,14 @@ See the License for the specific language governing permissions and limitations under the License. */ -package kubernetes +package kubectl import ( "context" "fmt" "os/exec" + "github.com/GoogleContainerTools/skaffold/pkg/skaffold/kubernetes" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/sync" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/util" @@ -61,7 +62,7 @@ func perform(ctx context.Context, image string, files map[string]string, cmdFn f return nil } - client, err := Client() + client, err := kubernetes.Client() if err != nil { return errors.Wrap(err, "getting k8s client") } diff --git a/pkg/skaffold/kubernetes/sync_test.go b/pkg/skaffold/sync/kubectl/kubectl_test.go similarity index 92% rename from pkg/skaffold/kubernetes/sync_test.go rename to pkg/skaffold/sync/kubectl/kubectl_test.go index 36910ee3755..74fa5f0dd36 100644 --- a/pkg/skaffold/kubernetes/sync_test.go +++ b/pkg/skaffold/sync/kubectl/kubectl_test.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package kubernetes +package kubectl import ( "context" @@ -23,6 +23,7 @@ import ( "strings" "testing" + pkgkubernetes "github.com/GoogleContainerTools/skaffold/pkg/skaffold/kubernetes" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes/fake" @@ -121,8 +122,8 @@ func TestPerform(t *testing.T) { defer func(c util.Command) { util.DefaultExecCommand = c }(util.DefaultExecCommand) util.DefaultExecCommand = cmdRecord - defer func(c func() (kubernetes.Interface, error)) { Client = c }(GetClientset) - Client = func() (kubernetes.Interface, error) { + defer func(c func() (kubernetes.Interface, error)) { pkgkubernetes.Client = c }(pkgkubernetes.GetClientset) + pkgkubernetes.Client = func() (kubernetes.Interface, error) { return fake.NewSimpleClientset(pod), test.clientErr } From 971c26215048c19173a52e339a7beb997c01c3d0 Mon Sep 17 00:00:00 2001 From: Priya Wadhwa Date: Wed, 10 Oct 2018 15:26:02 -0700 Subject: [PATCH 2/2] Move perform into sync.go --- pkg/skaffold/runner/runner.go | 2 +- pkg/skaffold/sync/kubectl/kubectl.go | 52 +------- pkg/skaffold/sync/kubectl/kubectl_test.go | 137 ---------------------- pkg/skaffold/sync/sync.go | 45 +++++++ pkg/skaffold/sync/sync_test.go | 113 ++++++++++++++++++ 5 files changed, 163 insertions(+), 186 deletions(-) delete mode 100644 pkg/skaffold/sync/kubectl/kubectl_test.go diff --git a/pkg/skaffold/runner/runner.go b/pkg/skaffold/runner/runner.go index 844383bd315..5002b9b8340 100644 --- a/pkg/skaffold/runner/runner.go +++ b/pkg/skaffold/runner/runner.go @@ -109,7 +109,7 @@ func NewForConfig(opts *config.SkaffoldOptions, cfg *latest.SkaffoldPipeline) (* Deployer: deployer, Tagger: tagger, Trigger: trigger, - Syncer: &kubectl.KubectlSyncer{}, + Syncer: &kubectl.Syncer{}, opts: opts, watchFactory: watch.NewWatcher, }, nil diff --git a/pkg/skaffold/sync/kubectl/kubectl.go b/pkg/skaffold/sync/kubectl/kubectl.go index 3136835c3c7..6fbccf9fe71 100644 --- a/pkg/skaffold/sync/kubectl/kubectl.go +++ b/pkg/skaffold/sync/kubectl/kubectl.go @@ -21,28 +21,25 @@ import ( "fmt" "os/exec" - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/kubernetes" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/sync" - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/util" "github.com/pkg/errors" "github.com/sirupsen/logrus" "k8s.io/api/core/v1" - meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) -type KubectlSyncer struct{} +type Syncer struct{} -func (k *KubectlSyncer) Sync(ctx context.Context, s *sync.Item) error { +func (k *Syncer) Sync(ctx context.Context, s *sync.Item) error { logrus.Infoln("Copying files:", s.Copy, "to", s.Image) - if err := perform(ctx, s.Image, s.Copy, copyFileFn); err != nil { + if err := sync.Perform(ctx, s.Image, s.Copy, copyFileFn); err != nil { return errors.Wrap(err, "copying files") } logrus.Infoln("Deleting files:", s.Delete, "from", s.Image) - if err := perform(ctx, s.Image, s.Delete, deleteFileFn); err != nil { + if err := sync.Perform(ctx, s.Image, s.Delete, deleteFileFn); err != nil { return errors.Wrap(err, "deleting files") } @@ -56,44 +53,3 @@ func deleteFileFn(ctx context.Context, pod v1.Pod, container v1.Container, src, func copyFileFn(ctx context.Context, pod v1.Pod, container v1.Container, src, dst string) *exec.Cmd { return exec.CommandContext(ctx, "kubectl", "cp", src, fmt.Sprintf("%s/%s:%s", pod.Namespace, pod.Name, dst), "-c", container.Name) } - -func perform(ctx context.Context, image string, files map[string]string, cmdFn func(context.Context, v1.Pod, v1.Container, string, string) *exec.Cmd) error { - if len(files) == 0 { - return nil - } - - client, err := kubernetes.Client() - if err != nil { - return errors.Wrap(err, "getting k8s client") - } - - pods, err := client.CoreV1().Pods("").List(meta_v1.ListOptions{}) - if err != nil { - return errors.Wrap(err, "getting pods") - } - - synced := map[string]bool{} - - for _, p := range pods.Items { - for _, c := range p.Spec.Containers { - if c.Image != image { - continue - } - - for src, dst := range files { - cmd := cmdFn(ctx, p, c, src, dst) - if err := util.RunCmd(cmd); err != nil { - return err - } - - synced[src] = true - } - } - } - - if len(synced) != len(files) { - return errors.New("couldn't sync all the files") - } - - return nil -} diff --git a/pkg/skaffold/sync/kubectl/kubectl_test.go b/pkg/skaffold/sync/kubectl/kubectl_test.go deleted file mode 100644 index 74fa5f0dd36..00000000000 --- a/pkg/skaffold/sync/kubectl/kubectl_test.go +++ /dev/null @@ -1,137 +0,0 @@ -/* -Copyright 2018 The Skaffold Authors - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package kubectl - -import ( - "context" - "fmt" - "os/exec" - "strings" - "testing" - - pkgkubernetes "github.com/GoogleContainerTools/skaffold/pkg/skaffold/kubernetes" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/client-go/kubernetes/fake" - - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/constants" - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/util" - "github.com/GoogleContainerTools/skaffold/testutil" - v1 "k8s.io/api/core/v1" - "k8s.io/client-go/kubernetes" -) - -type TestCmdRecorder struct { - cmds []string - err error -} - -func (t *TestCmdRecorder) RunCmd(cmd *exec.Cmd) error { - if t.err != nil { - return t.err - } - t.cmds = append(t.cmds, strings.Join(cmd.Args, " ")) - return nil -} - -func (t *TestCmdRecorder) RunCmdOut(cmd *exec.Cmd) ([]byte, error) { - return nil, t.RunCmd(cmd) -} - -func fakeCmd(ctx context.Context, p v1.Pod, c v1.Container, src, dst string) *exec.Cmd { - return exec.CommandContext(ctx, "copy", src, dst) -} - -var pod = &v1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: "podname", - Labels: constants.Labels.DefaultLabels, - }, - Status: v1.PodStatus{ - Phase: v1.PodRunning, - }, - Spec: v1.PodSpec{ - Containers: []v1.Container{ - { - Name: "container_name", - Image: "gcr.io/k8s-skaffold:123", - }, - }, - }, -} - -func TestPerform(t *testing.T) { - var tests = []struct { - description string - image string - files map[string]string - cmdFn func(context.Context, v1.Pod, v1.Container, string, string) *exec.Cmd - cmdErr error - clientErr error - expected []string - shouldErr bool - }{ - { - description: "no error", - image: "gcr.io/k8s-skaffold:123", - files: map[string]string{"test.go": "/test.go"}, - cmdFn: fakeCmd, - expected: []string{"copy test.go /test.go"}, - }, - { - description: "cmd error", - image: "gcr.io/k8s-skaffold:123", - files: map[string]string{"test.go": "/test.go"}, - cmdFn: fakeCmd, - cmdErr: fmt.Errorf(""), - shouldErr: true, - }, - { - description: "client error", - image: "gcr.io/k8s-skaffold:123", - files: map[string]string{"test.go": "/test.go"}, - cmdFn: fakeCmd, - clientErr: fmt.Errorf(""), - shouldErr: true, - }, - { - description: "no copy", - image: "gcr.io/different-pod:123", - files: map[string]string{"test.go": "/test.go"}, - cmdFn: fakeCmd, - shouldErr: true, - }, - } - - for _, test := range tests { - t.Run(test.description, func(t *testing.T) { - cmdRecord := &TestCmdRecorder{err: test.cmdErr} - defer func(c util.Command) { util.DefaultExecCommand = c }(util.DefaultExecCommand) - util.DefaultExecCommand = cmdRecord - - defer func(c func() (kubernetes.Interface, error)) { pkgkubernetes.Client = c }(pkgkubernetes.GetClientset) - pkgkubernetes.Client = func() (kubernetes.Interface, error) { - return fake.NewSimpleClientset(pod), test.clientErr - } - - util.DefaultExecCommand = cmdRecord - - err := perform(context.Background(), test.image, test.files, test.cmdFn) - - testutil.CheckErrorAndDeepEqual(t, test.shouldErr, err, test.expected, cmdRecord.cmds) - }) - } -} diff --git a/pkg/skaffold/sync/sync.go b/pkg/skaffold/sync/sync.go index d5849480a79..5b2efdf5bb8 100644 --- a/pkg/skaffold/sync/sync.go +++ b/pkg/skaffold/sync/sync.go @@ -19,14 +19,18 @@ package sync import ( "context" "fmt" + "os/exec" "path" "path/filepath" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build" + "github.com/GoogleContainerTools/skaffold/pkg/skaffold/kubernetes" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/util" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/watch" "github.com/pkg/errors" + "k8s.io/api/core/v1" + meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) type Syncer interface { @@ -108,3 +112,44 @@ func intersect(context string, syncMap map[string]string, files []string) (map[s } return ret, nil } + +func Perform(ctx context.Context, image string, files map[string]string, cmdFn func(context.Context, v1.Pod, v1.Container, string, string) *exec.Cmd) error { + if len(files) == 0 { + return nil + } + + client, err := kubernetes.Client() + if err != nil { + return errors.Wrap(err, "getting k8s client") + } + + pods, err := client.CoreV1().Pods("").List(meta_v1.ListOptions{}) + if err != nil { + return errors.Wrap(err, "getting pods") + } + + synced := map[string]bool{} + + for _, p := range pods.Items { + for _, c := range p.Spec.Containers { + if c.Image != image { + continue + } + + for src, dst := range files { + cmd := cmdFn(ctx, p, c, src, dst) + if err := util.RunCmd(cmd); err != nil { + return err + } + + synced[src] = true + } + } + } + + if len(synced) != len(files) { + return errors.New("couldn't sync all the files") + } + + return nil +} diff --git a/pkg/skaffold/sync/sync_test.go b/pkg/skaffold/sync/sync_test.go index 175c36a5a02..cdfde8594aa 100644 --- a/pkg/skaffold/sync/sync_test.go +++ b/pkg/skaffold/sync/sync_test.go @@ -16,13 +16,24 @@ limitations under the License. package sync import ( + "context" + "fmt" + "os/exec" "path/filepath" + "strings" "testing" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build" + "github.com/GoogleContainerTools/skaffold/pkg/skaffold/constants" + pkgkubernetes "github.com/GoogleContainerTools/skaffold/pkg/skaffold/kubernetes" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest" + "github.com/GoogleContainerTools/skaffold/pkg/skaffold/util" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/watch" "github.com/GoogleContainerTools/skaffold/testutil" + "k8s.io/api/core/v1" + meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes" + "k8s.io/client-go/kubernetes/fake" ) func TestNewSyncItem(t *testing.T) { @@ -230,3 +241,105 @@ func TestIntersect(t *testing.T) { }) } } + +type TestCmdRecorder struct { + cmds []string + err error +} + +func (t *TestCmdRecorder) RunCmd(cmd *exec.Cmd) error { + if t.err != nil { + return t.err + } + t.cmds = append(t.cmds, strings.Join(cmd.Args, " ")) + return nil +} + +func (t *TestCmdRecorder) RunCmdOut(cmd *exec.Cmd) ([]byte, error) { + return nil, t.RunCmd(cmd) +} + +func fakeCmd(ctx context.Context, p v1.Pod, c v1.Container, src, dst string) *exec.Cmd { + return exec.CommandContext(ctx, "copy", src, dst) +} + +var pod = &v1.Pod{ + ObjectMeta: meta_v1.ObjectMeta{ + Name: "podname", + Labels: constants.Labels.DefaultLabels, + }, + Status: v1.PodStatus{ + Phase: v1.PodRunning, + }, + Spec: v1.PodSpec{ + Containers: []v1.Container{ + { + Name: "container_name", + Image: "gcr.io/k8s-skaffold:123", + }, + }, + }, +} + +func TestPerform(t *testing.T) { + var tests = []struct { + description string + image string + files map[string]string + cmdFn func(context.Context, v1.Pod, v1.Container, string, string) *exec.Cmd + cmdErr error + clientErr error + expected []string + shouldErr bool + }{ + { + description: "no error", + image: "gcr.io/k8s-skaffold:123", + files: map[string]string{"test.go": "/test.go"}, + cmdFn: fakeCmd, + expected: []string{"copy test.go /test.go"}, + }, + { + description: "cmd error", + image: "gcr.io/k8s-skaffold:123", + files: map[string]string{"test.go": "/test.go"}, + cmdFn: fakeCmd, + cmdErr: fmt.Errorf(""), + shouldErr: true, + }, + { + description: "client error", + image: "gcr.io/k8s-skaffold:123", + files: map[string]string{"test.go": "/test.go"}, + cmdFn: fakeCmd, + clientErr: fmt.Errorf(""), + shouldErr: true, + }, + { + description: "no copy", + image: "gcr.io/different-pod:123", + files: map[string]string{"test.go": "/test.go"}, + cmdFn: fakeCmd, + shouldErr: true, + }, + } + + for _, test := range tests { + t.Run(test.description, func(t *testing.T) { + cmdRecord := &TestCmdRecorder{err: test.cmdErr} + defer func(c util.Command) { util.DefaultExecCommand = c }(util.DefaultExecCommand) + util.DefaultExecCommand = cmdRecord + + defer func(c func() (kubernetes.Interface, error)) { pkgkubernetes.Client = c }(pkgkubernetes.GetClientset) + pkgkubernetes.Client = func() (kubernetes.Interface, error) { + return fake.NewSimpleClientset(pod), test.clientErr + } + + util.DefaultExecCommand = cmdRecord + + err := Perform(context.Background(), test.image, test.files, test.cmdFn) + + testutil.CheckErrorAndDeepEqual(t, test.shouldErr, err, test.expected, cmdRecord.cmds) + }) + } +}