Design proposal for sync improvements

[corneliusweig]

As discussed in #1812

[corneliusweig]

@tejal29 I can't finish this right now, but I'll try to find time later today. The section about config change might already be interesting.

[codecov-io]

Codecov Report

Merging #1844 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1844   +/-   ##
=======================================
  Coverage   52.07%   52.07%           
=======================================
  Files         179      179           
  Lines        7923     7923           
=======================================
  Hits         4126     4126           
  Misses       3415     3415           
  Partials      382      382

---

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 533378c...0d742bd. Read the comment docs.

[tejal29]

This looks amazing.
I have 2 open questions which we need to address before taking it out to

1. How will Syncer use the new proposed DependenciesForArtifact source, destination map
2. How will Syncer use the SyncMap map?

What if the DependenciesForArtifact contain sources which need to be rebuilt.

[tejal29]

Bail out with "not yet supported error"
It would be great if we can bail out at the start but checking if "SyncMap" is implemented and the Builder who don't implement it throw an error.
Something like that.

[corneliusweig]

If we want to throw an error, we must do it upfront during the Skaffold pipeline config validation. Otherwise we cannot differentiate between dependencies that are not copied into the container (e.g. Dockerfile) and builders that can not provide destinations paths.

But I also prefer Option 1, because it does not surprise users.

[briandealwis]

This situation seem similar to a situation when the destination cannot be inferred by the builder.

FROM scratch
  ADD foo baz
  RUN mv baz bar
FROM scratch
  COPY --from=0 bar bar

[balopat]

+1 on error - preferably at schema validation time to fail fast but could be just inference time too

[googlebot]

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

[corneliusweig]

@tejal29 I'm done for now. At your convenience, please have a second look.

[tejal29]

@corneliusweig This looks good.

The next skaffold bi-weekly meeting is on 3rd April 9:30 am to 10:am
I put this DD on the agenda, Will you be able to join?

Please join https://groups.google.com/forum/#!forum/skaffold-users to get meeting invite.

[tejal29]

great! this is ready to merge once kokoro build goes green

[corneliusweig]

Thanks again to everyone who shaped this design proposal! In particular @tejal29 who initiated the whole design proposal process. Doing these revisions on the PR has helped a lot!

[corneliusweig]

@tejal29 Do you think this is ready to be merged?

[balopat]

@dgageot can you please have a look - it would be great to have your input before we jump in implementing this.

[balopat]

Let's merge this in. @dgageot can comment on the actual PR later, and we'll adjust accordingly.

[briandealwis]

Let me throw in an additional wrinkle that may be worth thinking about. I glossed over some details previously when I described how Jib copies from src/main/resources to /app/resources. Maven and Gradle actually copy and (optionally) transform static file into somewhere under their build results (target/ for Maven, build/ for Gradle), and then Jib copies those file copies into the destination container. For example, Maven allows treating files as a template and interpolates them using variables from the build context.

For Skaffold to handle this properly, Skaffold would need to know if a builder requires a rebuild prior to performing a sync. It would require separating SkaffoldRunner.buildTestDeploy() to separate rebuilding from redeploying. The rebuild could continue to push the resulting image to a registry — that is, Skaffold can push, but it shouldn't shove :-)

[corneliusweig]

@briandealwis Just to clarify, I think "build" is used in two different contexts:

1. The container build
2. The java build, aka compilation

So I guess,

if a builder requires a rebuild prior to performing a sync

could be rephrased to

if a builder requires a recompile prior to performing a sync

Correct?

That kind of change probably needs to be picked up by the JIB team. However, the whole thing will only work, if the container allows some hot-swapping or hot-reloading of updated files. Otherwise, the main process in the container needs to be restarted, which kind of defeats the whole point of sync. Does JIB support a hot-swapping mode?

[briandealwis]

You're right @corneliusweig; I just used "rebuild" as a shorthand as for Jib-based projects, as a container build performs a recompile. It might be advantageous for the builder interface to support compile and build (to build the container image), but it seems easy enough to just use build since generating a container image is cheap.

It's not a real stretch to imagine Skaffold being able to sync files from a built container image: it's just a matter of trawling through the layers.

The hot-swapping/reloading is something that needs to be supported by the language runtime. The JVM does support it via helper libraries like Spring Boot Dev Tools.

[corneliusweig]

@loosebazooka (I hope this is the correct github handle :) We talked yesterday about the magic/smart sync mode.
What I had in mind is https://github.com/GoogleContainerTools/skaffold/pull/1844/files#diff-9f76156c150848a57c51506d8c4354d1R55 and https://github.com/GoogleContainerTools/skaffold/pull/1844\#issuecomment-487944328 . But I think your ideas are already much further evolved.

[loosebazooka]

@corneliusweig, yeah this is what I was looking for. smart or auto is maybe what we want for jib?