Skip to content

LCOW: Ensure platform is populated on COPY/ADD#37563

Merged
yongtang merged 1 commit into
moby:masterfrom
microsoft:jjh/fix-vso17531561v2
Aug 17, 2018
Merged

LCOW: Ensure platform is populated on COPY/ADD#37563
yongtang merged 1 commit into
moby:masterfrom
microsoft:jjh/fix-vso17531561v2

Conversation

@lowenna
Copy link
Copy Markdown
Member

@lowenna lowenna commented Jul 30, 2018 •
edited
Loading

Signed-off-by: John Howard [email protected]

Fixes #37561, and internal VSO bug 17531561.

The copier object constructor is only using the API/dockerfile requested platform (OS) and ignoring the implicit operating system in the builder state. Hence, on LCOW (see linked issue this fixes) where no platform is passed in the API, a docker build goes down the route of assuming it's Windows paths.

@tonistiigi, @johnstep PTAL. @jterry75 FYI

Note there is an alternate fix I came up with initially. Both fixes work, but I think this is cleaner having the correct OS set at the start of the COPY command as it comes in from the dispatcher. Alternate can be found at 3bf17fc.

Using the same dockerfile from the linked issue, results with this fix (it succeeds and /foo/bar/dockerfile is shown in step 4).

PS E:\docker\build\17531561> docker build -t 17531561 --no-cache .
Sending build context to Docker daemon  2.048kB
Step 1/4 : FROM alpine
 ---> 11cd0b38bc3c
Step 2/4 : ADD . /foo/bar
 ---> 5ff4b6738a94
Step 3/4 : RUN ls -l /
 ---> Running in 602c3ce3695a
total 52
drwxr-xr-x    2 root     root          4096 Jul  5 14:47 bin
drwxr-xr-x    5 root     root           340 Jul 30 18:34 dev
drwxr-xr-x    1 root     root            60 Jul 30 18:34 etc
drwxr-xr-x    3 root     root          4096 Jul 30 18:34 foo
drwxr-xr-x    2 root     root          4096 Jul  5 14:47 home
drwxr-xr-x    5 root     root          4096 Jul  5 14:47 lib
drwxr-xr-x    5 root     root          4096 Jul  5 14:47 media
drwxr-xr-x    2 root     root          4096 Jul  5 14:47 mnt
dr-xr-xr-x   72 root     root             0 Jul 30 18:34 proc
drwx------    2 root     root          4096 Jul  5 14:47 root
drwxr-xr-x    2 root     root          4096 Jul  5 14:47 run
drwxr-xr-x    2 root     root          4096 Jul  5 14:47 sbin
drwxr-xr-x    2 root     root          4096 Jul  5 14:47 srv
dr-xr-xr-x   13 root     root             0 Jul 30 18:34 sys
drwxrwxrwt    2 root     root          4096 Jul  5 14:47 tmp
drwxr-xr-x    7 root     root          4096 Jul  5 14:47 usr
drwxr-xr-x   11 root     root          4096 Jul  5 14:47 var
Removing intermediate container 602c3ce3695a
 ---> 704ea4b1141d
Step 4/4 : RUN ls -lR /foo
 ---> Running in ba0ec5a90cf1
/foo:
total 4
drwxr-xr-x    2 root     root          4096 Jul 30 18:34 bar

/foo/bar:
total 4
-rwxr-xr-x    1 root     root           228 Jul 30 16:51 dockerfile
Removing intermediate container ba0ec5a90cf1
 ---> 4a62ac6e4668
Successfully built 4a62ac6e4668
Successfully tagged 17531561:latest
PS E:\docker\build\17531561>

@lowenna lowenna requested a review from tonistiigi as a code owner July 30, 2018 19:56
@lowenna lowenna requested a review from johnstep July 30, 2018 19:57
@codecov
Copy link
Copy Markdown

codecov Bot commented Jul 30, 2018 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@73d73ba). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master   #37563   +/-   ##
=========================================
  Coverage          ?   35.62%           
=========================================
  Files             ?      610           
  Lines             ?    44903           
  Branches          ?        0           
=========================================
  Hits              ?    15996           
  Misses            ?    26705           
  Partials          ?     2202

thaJeztah
thaJeztah requested changes Aug 1, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a comment; I'm not super-familiar with this code, so my comment is just from digging a bit

Comment thread builder/dockerfile/copy.go
download: download,
imageSource: imageSource,
platform: req.builder.platform,
platform: platform,
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah Aug 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I notice that after the copier is created, builder.performCopy() is called;

moby/builder/dockerfile/dispatchers.go

Lines 123 to 131 in 492545e

copier := copierFromDispatchRequest(d, errOnSourceDownload, im)
defer copier.Cleanup()
copyInstruction, err := copier.createCopyInstruction(c.SourcesAndDest, "COPY")
if err != nil {
return err
}
copyInstruction.chownStr = c.Chown
return d.builder.performCopy(d, copyInstruction)

Looking at that function, that seems to use req.builder.platform, not the value set on the copier.

moby/builder/dockerfile/internals.go

Line 172 in f0585d0

imageMount, err := b.imageSources.Get(state.imageID, true, req.builder.platform)

The value is actually used, but only for path separators, and not persisted in the copy instruction itself;

moby/builder/dockerfile/copy.go

Lines 98 to 106 in b711437

// Work in platform-specific filepath semantics
// TODO: This OS switch for paths is NOT correct and should not be supported.
// Maintained for backwards compatibility
pathOS := runtime.GOOS
if o.platform != nil {
pathOS = o.platform.OS
}
inst.dest = fromSlash(args[last], pathOS)
separator := string(separator(pathOS))

Should a property beaded to copyInstruction, and set to this value?

Copy link
Copy Markdown
Member Author

@lowenna lowenna Aug 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thaJeztah I'll defer to @tonistiigi here. I don't think it matters, but I don't own the builder code. Just fixing a bug...

Copy link
Copy Markdown
Member

@tonistiigi tonistiigi Aug 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The platform in

moby/builder/dockerfile/internals.go

Line 172 in f0585d0

imageMount, err := b.imageSources.Get(state.imageID, true, req.builder.platform)
is for the target image of --from not for the current instruction so may be different.

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Aug 1, 2018

ping @simonferquel PTAL as well

thaJeztah
thaJeztah approved these changes Aug 2, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@yongtang yongtang merged commit 14d5569 into moby:master Aug 17, 2018
@thaJeztah thaJeztah added the area/lcow Issues and PR's related to the experimental LCOW feature label Oct 30, 2018
This was referenced Nov 15, 2018
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thaJeztah thaJeztah thaJeztah approved these changes

@tonistiigi tonistiigi Awaiting requested review from tonistiigi tonistiigi is a code owner

+1 more reviewer

@johnstep johnstep johnstep approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@duglin duglin

Labels

area/builder Build area/lcow Issues and PR's related to the experimental LCOW feature status/4-merge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

LCOW: Add . /foo/bar is created in container as /foobar (no separator)

7 participants

@lowenna @thaJeztah @tonistiigi @johnstep @duglin @yongtang @GordonTheTurtle