Implement destination inference for sync of dockerfile artifacts [2/3] by corneliusweig · Pull Request #2084 · GoogleContainerTools/skaffold

corneliusweig · 2019-05-07T20:10:09Z

The sync feature is getting refurbished, see design proposal #1844 .

This PR implements step 2 of the implementation plan for the sync improvements. Notable changes include:

adding a new method SyncMap to the builder interface. This new method should provide sync destinations for all relevant source files.
implementing SyncMap for docker artifacts.

Note that SyncMap is intentionally not wired up yet. The only way to test it is with skaffold diagnose. The speed of GetDependencies vs SyncMap (which do similar things) is almost the same. In a sample project with 4.7k files GetDependencies and SyncMap took ~30ms and ~32ms, respectively.

I tried to (reasonably) avoid code duplication. Yet, the directory walking code (pkg/skaffold/docker/dependencies.go and pkg/skaffold/docker/syncmap.go) is almost twice the same. Nevertheless, I think this is the right approach, because CustomArtifact also relies on docker.WalkWorkspace and this one is a little faster than docker.walkWorkspaceWithDestinations.

Possible issues

Overlapping copies to the same destination are now resolved correctly. This was not the case in the original PR WIP Sync: infer destination from Dockerfile #1812. For example:
```
COPY foo /foo
COPY bar/ /   # bar contains a file called "foo"
```
In that case /foo will have the content from bar/foo, as it is expected.
Transitive copies across stages of multi-stage dockerfiles are not resolved. For example:
```
FROM scratch
ADD foo .
FROM scratch
COPY --from=0 foo bar
```
The current implementation will resolve foo -> foo and not foo -> bar. As discussed in the design
proposal Design proposal for sync improvements #1844, this edge case will be ignored for now.

based-on #1847
Supersedes #1812
CC design shepherd @tejal29

corneliusweig · 2019-05-10T21:33:10Z

@balopat rebase is done

codecov-io · 2019-05-10T21:48:08Z

Codecov Report

Merging #2084 into master will decrease coverage by <.01%.
The diff coverage is 68.98%.

@@            Coverage Diff             @@
##           master    #2084      +/-   ##
==========================================
- Coverage    58.7%   58.69%   -0.01%     
==========================================
  Files         188      191       +3     
  Lines        7849     8012     +163     
==========================================
+ Hits         4608     4703      +95     
- Misses       2873     2900      +27     
- Partials      368      409      +41

Impacted Files	Coverage Δ
pkg/skaffold/runner/timings.go	`14% <ø> (ø)`	⬆️
pkg/skaffold/build/build.go	`0% <0%> (ø)`
pkg/skaffold/build/local/local.go	`36.76% <0%> (-0.55%)`	⬇️
pkg/skaffold/runner/diagnose.go	`0% <0%> (ø)`	⬆️
pkg/skaffold/build/gcb/types.go	`0% <0%> (ø)`	⬆️
pkg/skaffold/build/cluster/types.go	`0% <0%> (ø)`	⬆️
cmd/skaffold/app/cmd/deploy.go	`68.75% <0%> (+4.46%)`	⬆️
cmd/skaffold/app/cmd/run.go	`60% <0%> (+6.15%)`	⬆️
cmd/skaffold/app/cmd/delete.go	`66.66% <0%> (+11.11%)`	⬆️
cmd/skaffold/app/cmd/dev.go	`23.91% <0%> (+1.18%)`	⬆️
... and 53 more

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 9935f9e...5d95a01. Read the comment docs.

dgageot

I'm ok with the design.
Because some functions are both moved and modified, I find it hard to review the code changes.

Is there a way to break this PR in two pieces. One that moves the code and one that actually adds the new feature?

dgageot · 2019-05-16T11:11:23Z

pkg/skaffold/build/local/local.go

+	if err != nil {
+		// if the context was cancelled act as if all is well
+		// TODO(dgageot): this should be even higher in the call chain.
+		if ctx.Err() == context.Canceled {


Can we fix that first to simplify the code here?

I moved the checks to the outermost call site. Please check carefully :)

dgageot · 2019-05-16T11:13:15Z

pkg/skaffold/runner/runner_test.go

 func (t *TestBench) Cleanup(ctx context.Context, out io.Writer) error { return nil }
 func (t *TestBench) Prune(ctx context.Context, out io.Writer) error   { return nil }
+
+func (t *TestBench) SyncMap(ctx context.Context, artifact *latest.Artifact) (map[string][]string, error) {


This could be a oneliner

goimports won't let me. I can only remove the blank lines

dgageot · 2019-05-16T11:14:06Z

docs/design_proposals/sync-improvements.md


   // ADDED
-   SyncMap() (map[string][]string, error)
+   SyncMap(ctx context.Context, artifact *latest.Artifact) (map[string][]string, error)


should this return a syncMap? (Not sure it's better)

@balopat had the same idea in pkg/skaffold/sync/sync.go. I'm also very unsure what is better. My main issue is that a type alias binds the using sites tighter to the place where type SyncMap is defined (probably builder.go?). Besides, I don't particularly like that there will be two SyncMap symbols in builder.go: 1 the function, 2 its return type.

Overall me feeling is slightly towards keeping it as is. On the other hand, the local type alias in sync.go makes somewhat sense, even though it cannot substitute all usages of map[string][]string due to visibility.

dgageot · 2019-05-16T11:16:42Z

pkg/skaffold/docker/parse.go

 	return false
 }
+
+func changeDir(cur, to string) string {


It's hard to understand what this function is doing. It doesn't change the directory.

I renamed it to resolveDir and added some godoc.

dgageot · 2019-05-16T11:18:40Z

pkg/skaffold/docker/parse.go


-func processCopy(value *parser.Node, envs map[string]string) ([]string, error) {
-	var copied []string
+func retrieveWorkingDir(tagged string, insecureRegistries map[string]bool) (string, error) {


There's a copy of this code in sync.go

😓 I shamelessly copied from there. I think the best place for this will be in the docker package.

corneliusweig · 2019-05-16T20:27:48Z

@dgageot Thanks for taking this up so quickly after bringing it to your attention!

Because some functions are both moved and modified, I find it hard to review the code changes.

This is an artifact of a humongous rebase/squash of the original PR. Sorry for that 🙇‍♂️ . As suggested, I squeezed in a pure move commit. This should make reviewing possible.

nkubala · 2019-05-16T20:28:37Z

pkg/skaffold/build/local/local.go

+		return nil, build.ErrSyncMapNotSupported{}
+	}
+
+	syncMap, err := docker.SyncMap(ctx, a.Workspace, a.DockerArtifact.DockerfilePath, a.DockerArtifact.BuildArgs, b.insecureRegistries)


can we not just return docker.SyncMap(....) here? I think it's more idiomatic to check if err != nil in the caller and then conditionally use the returned syncMap.

oh yeah, sure! David made me pull other code out from there, but I missed that this can be further simplified..

nkubala · 2019-05-16T20:30:24Z

pkg/skaffold/docker/image_util.go

+		// No local Docker is available
+		cf, err = RetrieveRemoteConfig(tagged, insecureRegistries)
+	} else {
+		cf, err = localDocker.ConfigFile(context.Background(), tagged)


I realized after making this comment you didn't write this code, but going to leave it here in case anyone has opinions on it:

do we not want to try and retrieve the image config from remote if we were able to get a local docker client, but couldn't find the image in the local daemon?

good catch. Please have another look.

nkubala · 2019-05-16T20:33:35Z

pkg/skaffold/docker/dependencies.go

+)
+
+// NormalizeDockerfilePath returns the absolute path to the dockerfile.
+func NormalizeDockerfilePath(context, dockerfile string) (string, error) {


this was copied from pkg/skaffold/docker/parse.go right? can we not move it to a common location (pkg/skaffold/docker/util/util.go?) and reuse it?

@nkubala So the function was not copied but moved. It is therefore already reused.

I'm not so happy with moving it to util. Such packages/files quickly attract completely unrelated code. But if you feel strongly about it, I won't start to argue.

tejal29 · 2019-05-16T21:58:36Z

cmd/skaffold/app/cmd/cmd.go

 	return nil
 }
+
+func alwaysSucceedWhenCancelled(ctx context.Context, err error) error {


is this related to sync destination inference?

Only indirectly. @dgageot recommended to resolve an earlier todo item in order to simplify the code (#2084 (comment)).

Would you prefer if this change was submitted as a separate PR?

tejal29 · 2019-05-16T22:04:13Z

pkg/skaffold/runner/diagnose.go

 		fmt.Fprintln(out, " - Dependencies:", len(deps), "files")
 		fmt.Fprintf(out, " - Time to list dependencies: %v (2nd time: %v)\n", timeDeps1, timeDeps2)

+		timeSyncMap1, err := timeToConstructSyncMap(ctx, r.Builder, artifact)


why do we do this 2 times?
Also, is there a way to add tests for the L63 to L65?

I think it's run twice so that the OS can warm the inode cache. The second run should normally be more representative.

Since there was no test for diagnose at all, I didn't bother adding one. Since this is creating (random) timings, the best we can achieve here is a smoke test. Do you feel strongly about it?

i think the runtime object inspection could refactored and unit tested to make sure we dont' break this in future. But i dont feel strongly regarding it.

if _, isNotSupported := err.(build.ErrSyncMapNotSupported); !isNotSupported { return errors.Wrap(err, "construct artifact dependencies") }

@tejal29 Can you explain a little more how you would like that code to be refactored?

I opened #2157 for some smoke tests of the diagnose subcommand. I think this has advantages over unit testing here, because we would have to set up a lot of mock objects for unit tests.

corneliusweig · 2019-05-27T11:27:26Z

@dgageot At you convenience could you take another look?

dgageot · 2019-06-06T05:05:28Z

@corneliusweig Could you rebase this PR? I'll then take a look at it to make sure it's merged quickly

corneliusweig · 2019-06-06T05:41:54Z

@dgageot Thanks! Rebase is done.

dgageot · 2019-06-06T20:09:16Z

@corneliusweig sorry, I LGTMed but it needs rebase again