Improve make -f docker.Makefile vendor #482

simonferquel · 2019-03-14T08:41:11Z

- What I did

Initial work from @ijc brought a containerized vendoring which was great
but had a few issues:

did not work on Windows: dependencies containing symlinks could not be
written by golang/dep on a cifs-mounted volume (wich is the case when
mounting a windows dir in Docker Desktop)
slow: 2 reasons for that: doing the vendoring accross a cifs or osxfs
bind mount was far from ideal due to FS latency and caching issues.
There was no cache preservation between 2 calls.

This PR fixes those issues.

- How I did it

Not using bind mounts: we resolve the vendoring directly in the
container RootFS, then copy back the vendor dir and the Gopkg.lock file
(which might have been updated) to the host
Mounting a volume in which golang/dep will keep repo info and git
clones in cache

Note that the dep-cache mount point and DEPCACHEDIR env var are
//dep-cache and not /dep-cache. From a Unix FS point of view both paths
are equivalent. But some windows shells (like the bash version shipped
with Git on Windows) tries to do clever conversions to win32 paths when detecting unix-style
paths in command line arguments, which makes docker trying to mount the
volume in "c:\Program Files\Git\bin..." which makes no sense in a Linux
container. Doubling the initial "/" disable this behavior.

- How to verify it

try make -f docker.Makefile on Windows

@ijc

Initial work from @ijc brought a containerized vendoring which was great but had a few issues: - did not work on Windows: dependencies containing symlinks could not be written by golang/dep on a cifs-mounted volume (wich is the case when mounting a windows dir in Docker Desktop) - slow: 2 reasons for that: doing the vendoring accross a cifs or osxfs bind mount was far from ideal due to FS latency and caching issues. There was no cache preservation between 2 calls. This fix all those issues by: - Not using bind mounts: we resolve the vendoring directly in the container RootFS, then copy back the vendor dir and the Gopkg.lock file (which might have been updated) to the host - Mounting a volume in which golang/dep will keep repo info and git clones in cache Note that the dep-cache mount point and DEPCACHEDIR env var are //dep-cache and not /dep-cache. From a Unix FS point of view both paths are equivalent. But some windows shells (like the bash version shipped with Git on Windows) tries to do clever conversions to win32 paths when detecting unix-style paths in command line arguments, which makes docker trying to mount the volume in "c:\Program Files\Git\bin\..." which makes no sense in a Linux container. Doubling the initial "/" disable this behavior. Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>

vdemeester

LGTM 🐯
The only "flaw" here is that we may end up with a container still running at the end if something fails

simonferquel · 2019-03-14T08:54:59Z

Yes, the exact reason why the target starts with an ugly docker rm -f docker-app-vendoring || echo ""
I suppose I should also add a target for cleaning the vendoring cache volume (in case of a dep crash / an untimely docker rm -f). I'll add a commit with that.

Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>

codecov · 2019-03-14T08:58:35Z

Codecov Report

Merging #482 into master will decrease coverage by 0.12%.
The diff coverage is 73.43%.

@@            Coverage Diff            @@
##           master    #482      +/-   ##
=========================================
- Coverage   69.53%   69.4%   -0.13%     
=========================================
  Files          51      50       -1     
  Lines        2629    2553      -76     
=========================================
- Hits         1828    1772      -56     
+ Misses        569     545      -24     
- Partials      232     236       +4

Impacted Files	Coverage Δ
internal/commands/inspect.go	`78.37% <100%> (ø)`	⬆️
internal/commands/uninstall.go	`65.78% <50%> (-2.79%)`	⬇️
internal/commands/status.go	`71.42% <50%> (-3.58%)`	⬇️
internal/commands/install.go	`63.63% <50%> (-1.45%)`	⬇️
internal/commands/upgrade.go	`63.46% <70%> (-1.85%)`	⬇️
internal/commands/cnab.go	`68.99% <80.48%> (-10.21%)`	⬇️
types/parameters/parameters.go	`92.06% <0%> (-4.77%)`	⬇️
internal/store/store.go	`71.42% <0%> (-0.58%)`	⬇️
internal/commands/dockerdesktop.go

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 622f5d9...2d70010. Read the comment docs.

docker.Makefile

ijc · 2019-03-14T10:55:37Z

docker.Makefile

-	docker run --rm -v "$(CURDIR)":/go/src/github.com/docker/app/ $(DEV_IMAGE_NAME) make vendor
+	rm -rf ./vendor
+	docker rm -f docker-app-vendoring || echo ""
+	docker create --name docker-app-vendoring -v docker-app-vendor-cache://dep-cache -e DEPCACHEDIR=//dep-cache $(DEV_IMAGE_NAME) sh -c "rm -rf ./vendor && make vendor"


why the double //?

mingw flavor of sh (which is used under the hood by make) tries to be clever and when calling a non-mingw executable (not sure how it does the distinction between vanilla win32 executable and those compiled in a mingw environment though), tries to expand any argument that looks like a unix path into the equivalent windows path. docker CLI is not a mingw executable, so it expands things like /dep-cache into c:\<path to mingw root\dep-cache. Using the double // disable this expansion mechanism.

😮, thanks for explaining!

Is the rm -rf ./vendor needed? I see the makefile already removes;

app/Makefile

Line 109 in 151e1c5

$(call rmdir,vendor)

@simonferquel would be good to add a comment about the double slashes.

I think setting MSYS_NO_PATHCONV=1 disables the behavior

see
moby/moby#12590 (comment) (and https://stackoverflow.com/a/34386471/1811501)

(and some other tickets https://github.com/moby/moby/search?q=MSYS_NO_PATHCONV&type=Issues)

MSYS_NO_PATHCONV=1 disables this behavior with git bash. It does not seem to do it on my cygwin environment.
The only thing that seems to uniformally work (without causing problems for mounting), is the double "//".

I will comment on that

the rm -rf ./vendor has to happen locally, so that we reset it with the equivalent coming from the container results (if we don't do it, we won't properly delete files that have been removed by the new vendoring)

It goes further than that:

$ touch vendor/github.com/docker/cli/cli/deleteme.go $ dep ensure -v # Bringing vendor into sync (1/1) Wrote github.com/docker/cli (from https://github.com/chris-crone/cli)@d6bfd7e5592dad85969516c131d33910fa5ebd58: hash of vendored tree didn't match digest in Gopkg.lock

so if the package exists in vendor/ it insists that it is exactly what it wrote there previously (rather than, say, fixing it like you probably wanted it to!). I don't see any flag which changes this behaviour.

docker.Makefile

ijc · 2019-03-14T10:57:11Z

docker.Makefile

+	docker create --name docker-app-vendoring -v docker-app-vendor-cache://dep-cache -e DEPCACHEDIR=//dep-cache $(DEV_IMAGE_NAME) sh -c "rm -rf ./vendor && make vendor"
+	docker start -i -a docker-app-vendoring
+	docker cp docker-app-vendoring:/go/src/github.com/docker/app/vendor .
+	docker cp docker-app-vendoring:/go/src/github.com/docker/app/Gopkg.lock .


Will it ever update the Gopkg.toml? I thought sometimes it did, I might be wrong though...

Not the toml no, only the lock

docker.Makefile

ijc · 2019-03-14T10:59:53Z

docker.Makefile

 	docker rm -f docker-app-vendoring || echo ""
 	docker create --name docker-app-vendoring -v docker-app-vendor-cache://dep-cache -e DEPCACHEDIR=//dep-cache $(DEV_IMAGE_NAME) sh -c "rm -rf ./vendor && make vendor"
 	docker start -i -a docker-app-vendoring
 	docker cp docker-app-vendoring:/go/src/github.com/docker/app/vendor .
 	docker cp docker-app-vendoring:/go/src/github.com/docker/app/Gopkg.lock .
 	docker rm -f docker-app-vendoring
+	git add Gopkg.lock vendor


The permissions changes on the vendored files (that I can't comment on directly so commenting here) are odd, I don't think they matter to us (we won't need to executre any of the scripts, and for one file the x bit looks spurious since it is a .go). My only concern is that it might be due to OS specific behaviour (i.e. Linux people might put these back), which only really matters because if the CI check (and I suppose we'll soon find out if that is a problem).

The problem here comes from git rmthat is slightly different that rm -rf.
On windows, when you modify an existing file, it will keep the mode of the original one and won't change it (as ntfs don't have a mode bitfield on files). git rm delete files and stage the deletion at the same time, which has the side effect of making a subsequent modification loose the previous version mode information.
I'll try to see if there is a flag for git rm that does not stage the deletion...

I reverted to rm -rf because I am worried at what can happen with symlinks as well.
I suggest we make a note about "you should use the make version that comes with mingw" for better script compatibility

WFM if it WFYou

ijc

In general I like the approach, I had a few questions/comments though...

ijc

I seem to have a mixture of direct and in-review comments, sorry about that. Here are the ones I thought I was posting just now...

ijc · 2019-03-14T11:02:33Z

Urk, I don't know wtf is going on with how those comments are presented (I did a per-commit review in different tags, but normally that DTRT, although I may have accidentally clicked "single comment" once or twice). Anyway, I think all of my comments have been captured in some form above.

Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>

thaJeztah · 2019-03-14T12:55:09Z

docker.Makefile

+	rm -rf ./vendor
+	docker rm -f docker-app-vendoring || true
+	docker create --name docker-app-vendoring -v docker-app-vendor-cache://dep-cache -e DEPCACHEDIR=//dep-cache $(DEV_IMAGE_NAME) sh -c "rm -rf ./vendor && make vendor"
+	docker start -i -a docker-app-vendoring


the container doesn't have to be running to use docker cp

Oh, nvm, this start triggers the actual vendoring, right?

An alternative would be to do the vendoring in docker build (in a build-stage). If you're using buildkit, you could even use a RUN --mount

See https://github.com/moby/buildkit/blob/master/hack/update-vendor for an example

I think we aren't quite ready (in CI at least, if not on contributors machines) to mandate buildkit.

Hm, yeah. And with the legacy builder, the caching would be written to the container's filesystem (so likely slower).

Still curious why docker create and docker start are done separately; could just docker run the container (and have it exit afterwards)

Yes, I removed the create and collapsed everything in start (in upcoming commit)

thaJeztah · 2019-03-14T14:05:10Z

docker.Makefile

-	docker run --rm -v "$(CURDIR)":/go/src/github.com/docker/app/ $(DEV_IMAGE_NAME) make vendor
+	rm -rf ./vendor
+	docker rm -f docker-app-vendoring || echo ""
+	docker create --name docker-app-vendoring -v docker-app-vendor-cache://dep-cache -e DEPCACHEDIR=//dep-cache $(DEV_IMAGE_NAME) sh -c "rm -rf ./vendor && make vendor"


Is the rm -rf ./vendor needed? I see the makefile already removes;

app/Makefile

Line 109 in 151e1c5

$(call rmdir,vendor)

thaJeztah · 2019-03-14T14:06:13Z

docker.Makefile

@@ -110,9 +110,19 @@ lint: ## run linter(s)

 vendor: build_dev_image
 	$(info Update Vendoring...)
-	docker run --rm -v "$(CURDIR)":/go/src/github.com/docker/app/ $(DEV_IMAGE_NAME) make vendor
+	rm -rf ./vendor


Should this be put after the container completes vendoring? (i.e. in case the container fails, your local vendor is gone)

thaJeztah · 2019-03-14T14:10:33Z

docker.Makefile

+	rm -rf ./vendor
+	docker rm -f docker-app-vendoring || true
+	docker create --name docker-app-vendoring -v docker-app-vendor-cache://dep-cache -e DEPCACHEDIR=//dep-cache $(DEV_IMAGE_NAME) sh -c "rm -rf ./vendor && make vendor"
+	docker start -i -a docker-app-vendoring


Hm, yeah. And with the legacy builder, the caching would be written to the container's filesystem (so likely slower).

Still curious why docker create and docker start are done separately; could just docker run the container (and have it exit afterwards)

Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>

simonferquel · 2019-03-15T15:30:39Z

@ijc @thaJeztah, just updated the PR. PTAL.
I kept using the double "//" as the MSYS_* env var does not apply to my current mingw make environment...
Imerged create/start into a single run, and I added the possibility to pass arguments (usefull for things like -update golang/x/sys etc.)

simonferquel requested review from chris-crone and silvin-lubecki as code owners March 14, 2019 08:41

GordonTheTurtle added the status/0-triage label Mar 14, 2019

simonferquel mentioned this pull request Mar 14, 2019

Rewrite Docker-Desktop contexts #479

Merged

vdemeester approved these changes Mar 14, 2019

View reviewed changes

add target for cleaning vendoring cache

0f5ce53

Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>

ijc reviewed Mar 14, 2019

View reviewed changes

nits

b14e5f3

Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>

simonferquel force-pushed the make-vendor-greater branch from 944d7f9 to b14e5f3 Compare March 14, 2019 12:31

thaJeztah suggested changes Mar 14, 2019

View reviewed changes

thaJeztah reviewed Mar 14, 2019

View reviewed changes

zappy-shu mentioned this pull request Mar 14, 2019

Don't run invocation image as root #478

Merged

Nits + custom DEP_ARGS

2d70010

Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>

ijc approved these changes Mar 15, 2019

View reviewed changes

ijc merged commit c22dc40 into docker:master Mar 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve make -f docker.Makefile vendor #482

Improve make -f docker.Makefile vendor #482

simonferquel commented Mar 14, 2019

vdemeester left a comment

simonferquel commented Mar 14, 2019

codecov bot commented Mar 14, 2019 •

edited

Loading

ijc Mar 14, 2019

simonferquel Mar 14, 2019

ijc Mar 14, 2019

thaJeztah Mar 14, 2019

chris-crone Mar 15, 2019

thaJeztah Mar 15, 2019

thaJeztah Mar 15, 2019

simonferquel Mar 15, 2019

simonferquel Mar 15, 2019

ijc Mar 15, 2019

ijc Mar 14, 2019

simonferquel Mar 14, 2019

ijc Mar 14, 2019

simonferquel Mar 14, 2019

simonferquel Mar 14, 2019

ijc Mar 14, 2019

ijc left a comment

ijc left a comment

ijc commented Mar 14, 2019

thaJeztah Mar 14, 2019

thaJeztah Mar 14, 2019

thaJeztah Mar 14, 2019

ijc Mar 14, 2019

thaJeztah Mar 14, 2019

simonferquel Mar 15, 2019

thaJeztah Mar 14, 2019

thaJeztah Mar 14, 2019

simonferquel Mar 15, 2019

thaJeztah Mar 14, 2019

simonferquel commented Mar 15, 2019

Improve make -f docker.Makefile vendor #482

Improve make -f docker.Makefile vendor #482

Conversation

simonferquel commented Mar 14, 2019

vdemeester left a comment

Choose a reason for hiding this comment

simonferquel commented Mar 14, 2019

codecov bot commented Mar 14, 2019 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ijc left a comment

Choose a reason for hiding this comment

ijc left a comment

Choose a reason for hiding this comment

ijc commented Mar 14, 2019

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

simonferquel commented Mar 15, 2019

codecov bot commented Mar 14, 2019 •

edited

Loading