File Sync for skaffold dev #1039

r2d4 · 2018-09-26T18:28:55Z

This PR introduces file sync for skaffold dev. Each artifact config now has an optional sync map. If the changes for the dev cycle are contained entirely in the sync map, then a rebuild and redeploy will be skipped, and those files will be synced to the running container. If only some of the changes are contained in the sync map, a full rebuild and redeploy will happen as normal.

That can optionally take glob patterns in the "key" part of the map (if the "key" is a pattern, the "value" destination must be a directory).

This PR changes the watcher slightly to compute WatchEvents, which are files that are added, modified, deleted, rather than an aggregate yes/no to whether things have changed. We were already computing this information and throwing it away, so it shouldn't be any slower.

It also adds a "webserver" example

Caveats: Files are copied with kubectl cp and kubectl exec rm. kubectl cp requires the tar and cp binaries to be present in the container. kubectl exec rm will require rm to be in the container. This feature will not work for scratch containers.

Future work for another PR will directly infer the "destination" from the Dockerfile if it not present.

codecov-io · 2018-09-26T18:54:47Z

Codecov Report

Merging #1039 into master will increase coverage by 0.83%.
The diff coverage is 70.9%.

@@            Coverage Diff             @@
##           master    #1039      +/-   ##
==========================================
+ Coverage   42.32%   43.16%   +0.83%     
==========================================
  Files          72       74       +2     
  Lines        3303     3408     +105     
==========================================
+ Hits         1398     1471      +73     
- Misses       1771     1799      +28     
- Partials      134      138       +4

Impacted Files	Coverage Δ
cmd/skaffold/app/cmd/fix.go	`0% <ø> (ø)`	⬆️
pkg/skaffold/util/util.go	`45.09% <0%> (-2.33%)`	⬇️
pkg/skaffold/runner/runner.go	`51.68% <42.1%> (-1.48%)`	⬇️
pkg/skaffold/kubernetes/sync.go	`68.42% <68.42%> (ø)`
pkg/skaffold/runner/changes.go	`83.33% <75%> (-16.67%)`	⬇️
pkg/skaffold/sync/sync.go	`95% <95%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 616f9f7...9b11bc4. Read the comment docs.

nkubala · 2018-09-26T19:59:23Z

pkg/skaffold/kubernetes/sync.go

+	return perform(image, syncMap, deleteFileFn)
+}
+
+// TODO(r2d4): kubectl exec doesn't seem to take a namespace flag?


it's not documented, but you can pass --namespace to kubectl exec

➜ ~ kubectl exec getting-started --namespace=default -- "ls" app bin ... ➜ ~ kubectl exec getting-started --namespace=asdf -- "ls" Error from server (NotFound): pods "getting-started" not found

Good catch, I'll update

Do you mind opening up an issue on kubectl to document that? That might be a good contribution to upstream kubectl.

nkubala · 2018-09-26T20:00:25Z

pkg/skaffold/kubernetes/sync.go

+	if err != nil {
+		return errors.Wrap(err, "getting k8s client")
+	}
+	pods, err := client.CoreV1().Pods("").List(meta_v1.ListOptions{})


I think we want to plumb through the namespace to these functions and pass it in here. As per my comment above this should work with kubectl exec

I don't think we actually know what namespace we're looking for? We don't actually parse the namespace anywhere, and it can be different for kubectl and helm.

nkubala · 2018-09-26T20:06:27Z

pkg/skaffold/util/util.go

@@ -106,6 +106,13 @@ func ExpandPathsGlob(workingDir string, paths []string) ([]string, error) {
 	return ret, nil
 }

+// HasMeta reports whether path contains any of the magic characters
+// recognized by filepath.Match.
+// This is a copy of filepath/match.go's hasMeta


the original implementation also includes the filepath separator. why not just use that?

https://golang.org/pkg/path/filepath/#Match?

nkubala · 2018-09-26T20:10:05Z

pkg/skaffold/watch/watch_test.go

@@ -108,8 +108,9 @@ func newCallback() *callback {
 	}
 }

-func (c *callback) call() {
+func (c *callback) call(e Events) error {


what's this change for?

callback.call() has to implement ChangeFn which is func(Events) error

nkubala · 2018-09-26T20:11:15Z

pkg/skaffold/kubernetes/sync.go

+		for _, c := range p.Spec.Containers {
+			if strings.HasPrefix(c.Image, image) {
+				for src, dst := range files {
+					cmd := cmdFn(p, c, src, dst)


could we call these in parallel?

Good idea, I'll make them run in parallel

nkubala · 2018-09-26T20:13:20Z

pkg/skaffold/runner/runner.go

+				if !sync {
+					changed.Add(artifact)
+				} else {
+					color.Default.Fprintln(out, "Synced:", "copied", append(e.Added, e.Modified...), "deleted", e.Deleted)


I think this is the case, but just to be sure: if we get here, is it always true that we've performed a sync?

Yep, shouldSync should probably be renamed, since it actually syncs and returns true or doesn't sync and returns false

dgageot · 2018-09-27T07:01:58Z

@r2d4 when I tried, it would sync files when I don't have a sync on the artifacts and it would stop syncing as soon as I had one...

dgageot · 2018-09-27T07:03:46Z

@r2d4 to make the review easier, could you create a PR that introduces watch events?

dgageot · 2018-09-27T07:11:47Z

pkg/skaffold/runner/runner.go

+	return true, nil
+}
+
+func intersect(syncMap map[string]string, files []string) (map[string]string, error) {


This doesn't work if the artifact is not in . but in a sub-folder. sync destinations are relative to the workspace but file paths are relative to the root of the project

Also if I want to copy all static/*.html files to /html, I end up copying them to /html/static/. It should not work that way.

intersect function would benefit from a good test coverage

I rewrote it to fix those issues and added another suite of tests to cover it. I still might rewrite both these functions after I split out watch, they're doing a little too much.

r2d4 · 2018-09-27T16:15:09Z

I’ll split out the watch changes into a separate PR

ajbouh · 2018-09-28T16:08:49Z

@r2d4 it's possible that you could use similar logic to keep the build context up to date to make container builds on knative really fast

r2d4 · 2018-09-28T17:31:11Z

Yes @ajbouh we can use the same information from the watcher to only update the file deltas on remote contexts 👍

dgageot · 2018-10-02T16:28:21Z

I added a wip label to make sure we merge #1064 first and rebase this one on top of it

dgageot · 2018-10-02T17:58:18Z

ping @r2d4, #1064 is merged.

dgageot

I'll test it tomorrow. Just one question

dgageot · 2018-10-02T20:01:18Z

pkg/skaffold/kubernetes/sync.go

+	if err != nil {
+		return errors.Wrap(err, "getting k8s client")
+	}
+	pods, err := client.CoreV1().Pods("").List(meta_v1.ListOptions{


Should you provide the ns here?

I don't think we have the namespace. In port-forward and log, we have to watch all pods.

"" is all namespaces

We do have a namespace sometimes but it's ok, your proposal of filtering on label is equally good.

dgageot · 2018-10-02T20:03:27Z

Sorry, @r2d4 you need to rebase once again...

dgageot · 2018-10-03T13:12:26Z

pkg/skaffold/kubernetes/sync.go

+					})
+				}
+				if err := e.Wait(); err != nil {
+					return errors.Wrap(err, "syncing files:")


Trailing :

dgageot · 2018-10-03T13:13:37Z

pkg/skaffold/runner/runner.go

+		case len(changed.needsResync) > 0:
+			for _, s := range changed.needsResync {
+				if err := r.Syncer.Sync(s); err != nil {
+					logrus.Warnln("Skipping build and deploy due to sync error:", err)


When the sync fails I don't see the warning.

In fact, sometimes I see it...

dgageot · 2018-10-03T13:22:28Z

pkg/skaffold/kubernetes/sync.go

+}
+
+func deleteFileFn(pod v1.Pod, container v1.Container, src, dst string) *exec.Cmd {
+	return exec.Command("kubectl", "exec", pod.Name, "--namespace", pod.Namespace, "-c", container.Name, "--", "rm", "-rf", dst)


A debug log would be useful here.

dgageot · 2018-10-03T13:22:31Z

pkg/skaffold/kubernetes/sync.go

+
+func copyFileFn(pod v1.Pod, container v1.Container, src, dst string) *exec.Cmd {
+	return exec.Command("kubectl", "cp", src, fmt.Sprintf("%s/%s:%s", pod.Namespace, pod.Name, dst), "-c", container.Name)
+}


A debug log would be useful here.

We get a debug log with util.RunCmd. Does that work?

yes should be good

dgageot · 2018-10-03T13:25:58Z

pkg/skaffold/runner/runner.go

+					logrus.Warnln("Skipping build and deploy due to sync error:", err)
+					return nil
+				}
+				color.Default.Fprintf(out, "Synced files for %s...\nCopied: %s\nDeleted: %s\n", s.Image, s.Copy, s.Delete)


Maybe print the number of files in info and the names in debug

Should there be any message in stdout?

stdout: ?
info: # of files synced
debug: filenames

I like info: # of files synced
debug: filenames

These run every second, so its difficult to read other debug statements

r2d4 · 2018-10-05T01:44:54Z

final rebase :)

thomas-riccardi · 2018-10-18T12:17:31Z

integration/examples/hot-reload/skaffold.yaml

+  - image: gcr.io/k8s-skaffold/node-example
+    context: node
+    sync:
+      '*.js': .


Shouldn't we avoid user data as keys ?

Why is that?

I should have justified my comment indeed.
Coming from k8s it is unusual to have user data as keys; it surprised me.

Digging into k8s design doc, we have:
https://github.com/kubernetes/community/blob/master/contributors/devel/api-conventions.md#lists-of-named-subobjects-preferred-over-maps
There are whole discussions linked from there, notably about

readability (mostly in case of subobjects though)

merging definitions, which may or may not be of concert for skaffold

It lists exceptions to this though: "pure maps [...] as opposed to sets of subobjects".

I suppose if we later want to enrich the format with subobjects it would make things harder with the current format.

I am not (yet) familiar with all the skaffold format, I don't know if there are other cases like that, I couln't find any.

So this is just to open the discussion. Should I move that into an issue?

Thanks for the link. That actually makes a lot of sense. Please open an issue! I think it’s a good suggestion

here we go: #1180

r2d4 requested review from balopat, dgageot and nkubala as code owners September 26, 2018 18:28

nkubala reviewed Sep 26, 2018

View reviewed changes

r2d4 force-pushed the file-sync branch from 3672939 to 0064ec7 Compare September 26, 2018 21:22

dgageot suggested changes Sep 27, 2018

View reviewed changes

r2d4 mentioned this pull request Sep 28, 2018

Watch returns filemap #1064

Merged

r2d4 force-pushed the file-sync branch 5 times, most recently from 90d2f35 to 0a206c2 Compare October 1, 2018 16:32

dgageot added the wip label Oct 2, 2018

r2d4 force-pushed the file-sync branch from 0a206c2 to 5efabf6 Compare October 2, 2018 17:51

r2d4 force-pushed the file-sync branch from 5efabf6 to 032559e Compare October 2, 2018 18:01

r2d4 removed the wip label Oct 2, 2018

dgageot suggested changes Oct 2, 2018

View reviewed changes

dgageot suggested changes Oct 3, 2018

View reviewed changes

r2d4 force-pushed the file-sync branch 2 times, most recently from d344b05 to 5da3ccd Compare October 3, 2018 14:33

bhack mentioned this pull request Oct 3, 2018

Skaffold REPL #913

Closed

r2d4 and others added 23 commits October 4, 2018 20:23

[kubernetes] add kubectl syncer

350f0b0

[runner] add sync step to skaffold dev

9a1d4d0

[examples] add readme to webserver example

64af3ab

[linter] fixes

884e6b3

review feedback

16415be

[vendor] add x/sync/errgroup

98b2219

[linter] add boilerplate to sync_test.go

f0ed4cd

File changes should be relative to artifact context

c511693

rename webserver example to hot reload

88f3d02

[sync] move to sync package, move logic to watch main loop

6edae2e

[examples] add flask hot-reload example

81bd94c

add boilerplate

f7b8a44

linter fixes

799d630

rebase on v1alpha4

d565e05

fix windows test

931b5d8

[parse] remove loud debug statements

35ef51c

These run every second, so its difficult to read other debug statements

[sync] query only skaffold pods, use exact tag

3f8ba1e

rebase on v1alpha4 config

c7b0fd5

delete out of date readme

10d5d31

add legacy watch for nodemon example

8f9ac9a

review feedback

8a22fd8

rebase changes

be4cc6c

debug and info messages only for sync

9b11bc4

r2d4 force-pushed the file-sync branch from a88893f to 9b11bc4 Compare October 5, 2018 01:24

dgageot merged commit d65ab29 into GoogleContainerTools:master Oct 5, 2018

r2d4 deleted the file-sync branch October 5, 2018 14:16

balopat mentioned this pull request Oct 8, 2018

Support static files/interpreted language containers #477

Closed

thomas-riccardi reviewed Oct 18, 2018

View reviewed changes

thomas-riccardi mentioned this pull request Oct 19, 2018

File Sync format specification: maybe don't have user data in the key part #1180

Closed

File Sync for skaffold dev #1039

File Sync for skaffold dev #1039

Conversation

r2d4 commented Sep 26, 2018 • edited Loading

codecov-io commented Sep 26, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgageot commented Sep 27, 2018

dgageot commented Sep 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r2d4 commented Sep 27, 2018

ajbouh commented Sep 28, 2018

r2d4 commented Sep 28, 2018

dgageot commented Oct 2, 2018

dgageot commented Oct 2, 2018

dgageot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgageot commented Oct 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r2d4 commented Oct 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r2d4 commented Sep 26, 2018 •

edited

Loading

codecov-io commented Sep 26, 2018 •

edited

Loading