Refix COPY file . after WORKDIR (now always created) #28514
Conversation
|
#28505 was merged, so this can be rebased |
|
Rebased |
|
@jhowardmsft grmbl, needs another one already 😢 |
|
Rebased again... |
|
@jhowardmsft I'm not sure I fully understand the issue so let me ask this... right now someone can do |
|
@duglin The performance issue is that with the old fix, every single docker run (which is really a create then a start) calls an extra mount/unmount regardless to setup the working directory in the create. While on Linux, that is a relatively cheap operation (very cheap); on Windows, to mount/unmount the scratchspace and initialize it added (I can't recall the exact figure without going back to email) something IRO 100+ms. That was an unacceptable performance regression in their opinion when everyone is working extremely hard to do everything possible to reduce container start times on Windows. Hence why this alternate fix only performs the mount/unmount in the WORKDIR builder call itself, rather than relying on it being done in a subsequent operation. Does that make sense? |
|
ok I think I get it - let me just confirm.... the mount/unmount you're referring to only happens when the "working dir" isn't there in the image. So, in my previous comment when I talked about how specifying If so, then this looks ok. One question I have, and its less for you and more for @tiborvass, would it make sense to have the build's |
|
@duglin - yes, you've got it 👍 |
|
I suspect @jhowardmsft wants this ASAP so I'll: |
|
@tiborvass Any chance of looking at this. Thanks 😄 |
| @@ -279,12 +280,31 @@ func workdir(b *Builder, args []string, attributes map[string]bool, original str | |||
| return err | |||
| } | |||
|
|
|||
| // NOTE: You won't find the "mkdir" for the directory in here. Rather we | |||
| comment := fmt.Sprintf("WORKDIR %v", b.runConfig.WorkingDir) | |||
There was a problem hiding this comment.
I know this was this way before, but should probably just be comment := "WORKDIR " + b.runConfig.WorkingDir
There was a problem hiding this comment.
Fixed in the forthcoming push with the Linux updates.
| return err | ||
| } | ||
| defer daemon.Unmount(container) | ||
| if err := container.SetupWorkingDirectory(0, 0); err != nil { |
There was a problem hiding this comment.
return container.SetupWorkingDirectory(0, 0)
There was a problem hiding this comment.
Will be fixed in the forthcoming update. But I need some more work here to support Linux anyway for the UID/GID mapping calls for UserNS
|
@jhowardmsft these are just small nits, otherwise LGTM |
|
@tiborvass any thoughts on this comment: #28514 (comment) ? |
|
@duglin From a code perspective it's better to mutualize as much as possible between windows and linux, so I would say yes I'm in favor. However is there anything that could break existing users? I couldn't think of anything. |
|
I can't think of anything either and I like the consistency of it. |
|
OK, I'll make the Linux change too. |
|
@jhowardmsft thanks! |
lowenna
changed the title
|
@duglin @tiborvass Updated to matching behaviour on Linux and Windows, and fixed the nits from the previous review. |
|
LGTM if janky is happy |
|
Hmmm. TestCommitChange fails (doesn't run on Windows). Investigating.... |
|
@duglin. Fixed. The update to the previous commit is to not call back into the daemon if |
| @@ -256,27 +256,6 @@ func TestOnbuild(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func TestWorkdir(t *testing.T) { | |||
There was a problem hiding this comment.
In the previous commit, I skipped the test on Windows as the new path sent us calling through the backend into the daemon which wasn't setup in unit tests. Of course, with the latest fix described above, now that we only callback if the disableCommit flag isn't set, the test can be fully re-instated. So it'll be back in for the next update coming shortly.
|
looks good, just one question |
Signed-off-by: John Howard <jhoward@microsoft.com>
|
Updated to re-instate the unit test after comment from @duglin |
|
@duglin Still looks good? Could you give a final LGTM if so. Thanks. @tiborvass You OK with this too? |
|
yup - still LGTM ! |
|
LGTM |
Signed-off-by: John Howard jhoward@microsoft.com
Fixes #27545 (again). @duglin @thaJeztah @vieux @cpuguy83
The fix for the above issue was in #27884, but reverted in #28505 as doing a mount/unmount for each container create is prohibitively expensive in terms of performance (as discovered by our performance team). Instead, this extends the interface from the builder back into the daemon so that the mount/unmount in container creation is done (on Windows) in the WORKDIR processing itself. The overall solution should still be in 1.13.
Edit: From comments below. The Linux behaviour has been updated to mirror the new Windows behaviour - WORKDIR in the builder always calls back into the daemon to create the directory rather than deferring it to the next statement.
@swernli @jstarks FYI.