New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make `FROM scratch` a special cased 'no-base' spec #8827

Merged
merged 1 commit into from Dec 18, 2014

Conversation

Projects
None yet
10 participants
@jlhawn
Copy link
Contributor

jlhawn commented Oct 28, 2014

Make FROM scratch a special cased 'no-base' spec

There has been a lot of discussion (issues 4242 and 5262) about making
FROM scratch either a special case or making FROM optional, implying
starting from an empty file system.

This patch makes the build command FROM scratch special cased from now on
and if used does not pull/set the the initial layer of the build to the ancient
image ID (511136ea..) but instead marks the build as having no base image. The
next command in the dockerfile will create an image with a parent image ID of "".
This means every image ever can now use one fewer layer!

This also makes the image name scratch a reserved name by the TagStore. You
will not be able to tag an image with this name from now on. If any users
currently have an image tagged as scratch, they will still be able to use that
image, but will not be able to tag a new image with that name.

Goodbye '511136ea3c5a64f264b78b5433614aec563103b4d4702f3ba7d4d2698e22c158',
it was nice knowing you.

Fixes #4242

@jlhawn

This comment has been minimized.

Copy link
Contributor Author

jlhawn commented Oct 28, 2014

This modifies more files than necessary only because I renamed Container.Image to Container.ImageID.

@thaJeztah

This comment has been minimized.

Copy link
Member

thaJeztah commented Oct 28, 2014

I think this may affect a lot of beginning docker users if they forget to add a FROM? Not against it per se, but will make it harder to detect if someone left out the FROM deliberately or by accident.

Will require doc changes as well

@erikh

This comment has been minimized.

Copy link
Contributor

erikh commented Oct 28, 2014

@jlhawn is this necessary? We might re-introduce this requirement with another proposal I'm working on for Dockerfile v2.

@erikh

This comment has been minimized.

Copy link
Contributor

erikh commented Oct 28, 2014

to clarify: I like scratch being "special" but I don't think the implicit FROM is going to work out in the long run.

@erikh erikh self-assigned this Oct 28, 2014

@jlhawn

This comment has been minimized.

Copy link
Contributor Author

jlhawn commented Oct 28, 2014

@erikh @thaJeztah There was discussion about this in #4242 @shykes, @vieux and a few others seemed to be in agreement that making FROM optional was the more elegant solution over special casing the name. Feel free to have more discussion on the issue over in that thread.

I'll update this with a change to the tests and documentation.

@erikh

This comment has been minimized.

Copy link
Contributor

erikh commented Oct 28, 2014

Like I said, we're going to be fixating the FROM requirement in Dockerfile v2. It is not going to be healthy for our users to have this changed twice.

@@ -575,7 +575,7 @@ func (daemon *Daemon) newContainer(name string, config *runconfig.Config, img *i
Args: args, //FIXME: de-duplicate from config
Config: config,
hostConfig: &runconfig.HostConfig{},
Image: img.ID, // Always use the resolved image id
ImageID: imgID,

This comment has been minimized.

@erikh

erikh Oct 29, 2014

Contributor

what happens here if the imgID is an empty string?

This comment has been minimized.

@jlhawn

jlhawn Oct 29, 2014

Author Contributor

Nothing happens at all :)

The container create logic just passes the image id string to the storage graph driver which knows how to handle it when making the container init layer.

It doesn't affect users at all (You can't do docker run "" for example) but if you do docker ps you get something like this (I ran docker build -t busybox --rm=false --no-cache .):

CONTAINER ID        IMAGE               COMMAND                CREATED             STATUS              PORTS               NAMES
ce55b20a0180        8a3051d7cf1a        "/bin/sh -c '#(nop)    28 hours ago                                                suspicious_pike     
8e1877d9055b        bbab8f8ff236        "/bin/sh -c '#(nop)    28 hours ago                                                furious_bartik      
907893df670c                            "/bin/sh -c '#(nop)    28 hours ago                                                silly_ptolemy

and docker inspect 907893df670c shows the container's image as "" also.

This comment has been minimized.

@erikh

erikh Oct 29, 2014

Contributor

Hmm. Can you adjust ps to say “scratch” or “” or something similar? That might be nice to clarify what’s going on here.

On Oct 29, 2014, at 10:51 AM, Josh Hawn notifications@github.com wrote:

In daemon/daemon.go:

@@ -575,7 +575,7 @@ func (daemon *Daemon) newContainer(name string, config *runconfig.Config, img *i
Args: args, //FIXME: de-duplicate from config
Config: config,
hostConfig: &runconfig.HostConfig{},

  •   Image:           img.ID, // Always use the resolved image id
    
  •   ImageID:         imgID,
    
    Nothing happens at all :)

The container container create logic just passes the image id string to the storage graph driver which knows how to handle it when making the container init layer.

It doesn't affect users at all (You can't do docker run "" for example) but if you do docker ps you get something like this (I ran docker build -t busybox --rm=false --no-cache .):

CONTAINER ID IMAGE COMMAND CREATED STATUS PORTS NAMES
ce55b20a0180 8a3051d7cf1a "/bin/sh -c '#(nop) 28 hours ago suspicious_pike
8e1877d9055b bbab8f8ff236 "/bin/sh -c '#(nop) 28 hours ago furious_bartik
907893df670c "/bin/sh -c '#(nop) 28 hours ago silly_ptolemy
and docker inspect 907893df670c shows the container's image as "" also.


Reply to this email directly or view it on GitHub https://github.com/docker/docker/pull/8827/files#r19557572.

This comment has been minimized.

@jlhawn

jlhawn Oct 29, 2014

Author Contributor

Here's the context of the docker build -t busybox ... from above. I used Jerome's busybox repo: https://github.com/jpetazzo/docker-busybox

but modified the Dockerfile to remove FROM scratch:

MAINTAINER Jérôme Petazzoni <jerome@docker.com>
ADD rootfs.tar /
CMD ["/bin/sh"]

This comment has been minimized.

@jlhawn

jlhawn Oct 29, 2014

Author Contributor

Hmm. Can you adjust ps to say “scratch” or “” or something similar? That might be nice to clarify what’s going on here.

I thought leaving it empty makes it pretty clear. It also follows what the other fields do when there's nothing to display, for example, thePORTS columns don't say <no ports>, it just leaves it empty.

Also, that field typically only shows an image ID or tagged repository name. I don't want to introduce a placeholder name like scratch since it would cause an issue if the user actually had a legitimate image named scratch. I'm agreeable to a placeholder like <no image> if other people are okay with it as long as it's on the client side and isn't something the daemon sets in the api response. I think the API responses for GET /containers/json and GET /containers/(id)/json should still return something with {"image": ""} in this case.

This comment has been minimized.

@thaJeztah

thaJeztah Oct 29, 2014

Member

+1 on <no image> or simply <none>; it's just a bit clearer that the image column is deliberately empty (maybe the same should be applied for other columns?

If people want to grep for this, <no image> is probably more helpful than <none>

This comment has been minimized.

@jlhawn

jlhawn Oct 29, 2014

Author Contributor

@thaJeztah @erikh I've added <no image> to what the client will now output on docker ps with the latest update.

@erikh

This comment has been minimized.

Copy link
Contributor

erikh commented Oct 29, 2014

Once my comments are addressed, LGTM

@tiborvass

This comment has been minimized.

Copy link
Collaborator

tiborvass commented Oct 29, 2014

I hope this is not a huge blocker for you @jlhawn but I'd prefer to wait a little on this one, as we're rethinking Dockerfiles and versioning of them and this might actually be a nuisance in a scenario where we want to take advantage of the fact that all current Dockerfiles have a FROM statement in the beginning. Will get back to this PR as shortly as I can :)

@jlhawn jlhawn force-pushed the jlhawn:build_implied_from_scratch branch from 30910e6 to 549ddae Oct 29, 2014

@jlhawn jlhawn changed the title Make `FROM` optional for base image Dockerfiles Make `FROM scratch` a special cased 'no-base' spec Oct 29, 2014

@jlhawn

This comment has been minimized.

Copy link
Contributor Author

jlhawn commented Oct 29, 2014

With the latest change, we're now back to requiring FROM <ARG> at the top of the Dockerfile. scratch is a reserved image name going forward (you cannot tag an image as scratch) but you can continue to use any currently existing image which happens to be named scratch. FROM scratch is a special build directive which sets the build to have no base image, causing the subsequent build command to produce an image with no parent.

@erikh

This comment has been minimized.

Copy link
Contributor

erikh commented Oct 29, 2014

ok. I’ll review this tonight, thanks!

On Oct 29, 2014, at 3:00 PM, Josh Hawn notifications@github.com wrote:

With the latest change, we're now back to requiring FROM at the top of the Dockerfile. scratch is a reserved image name going forward (you cannot tag an image as scratch) but you can continue to use any currently existing image which happens to be named scratch. FROM scratch is a special build directive which sets the build to have no base image, causing the proceeding build command to produce an image with no parent.


Reply to this email directly or view it on GitHub #8827 (comment).

@jlhawn jlhawn force-pushed the jlhawn:build_implied_from_scratch branch 2 times, most recently from 85d76cf to eb9ddfd Oct 29, 2014

@thaJeztah

This comment has been minimized.

Copy link
Member

thaJeztah commented Oct 29, 2014

Integration tests are failing on drone?

Update; nvm, see you updated the PR

@jlhawn

This comment has been minimized.

Copy link
Contributor Author

jlhawn commented Oct 29, 2014

one of the integration tests try to tag something as scratch which my latest change doesn't allow. I fixed that, then found out there are two other tests which assume an image exists named scratch, but I changed it to docker_scratch. I just pushed to trigger tests on drone since it's faster than my dev VM at building and running the suite.

@thaJeztah

This comment has been minimized.

Copy link
Member

thaJeztah commented Oct 29, 2014

Alright then, didn't know if you would notice it before Erik would start reviewing :)

@jlhawn

This comment has been minimized.

Copy link
Contributor Author

jlhawn commented Oct 29, 2014

meh, I'll just work these out locally. don't mind the red X for now, it's just minor issues.

@jlhawn jlhawn force-pushed the jlhawn:build_implied_from_scratch branch from eb9ddfd to 965339a Oct 29, 2014

@erikh

This comment has been minimized.

Copy link
Contributor

erikh commented Oct 30, 2014

I think it needs a quick docs pass to ensure "scratch" is explained well, but LGTM from me.

@jlhawn

This comment has been minimized.

Copy link
Contributor Author

jlhawn commented Oct 30, 2014

Will do, but before I start, I'd like a definitive 'yes, we're going with this' from the core maintainers.

@tiborvass

This comment has been minimized.

Copy link
Collaborator

tiborvass commented Oct 30, 2014

@jlhawn we'll have to do a design review on this with @shykes but you have my +1.

@tianon

This comment has been minimized.

Copy link
Member

tianon commented Oct 31, 2014

As I mentioned on the issue, I'm very strongly -1 on this because of the
errors it could obscure, and find an explicit "please make this image start
from nothing" to be much more transparent. If you add this together with
skipping unknown instructions, then we could have someone misspell FROM
and inadvertently end up with an empty broken image, as a concrete example
of one place this could cause issues that get unnecessarily harder to then
diagnose.

@jessfraz

This comment has been minimized.

Copy link
Contributor

jessfraz commented Oct 31, 2014

I honestly don't know about this one, I am uneasy as it might be confusing for
new users who forget the FROM directive. Plus the comments made by tianon above.

@thaJeztah

This comment has been minimized.

Copy link
Member

thaJeztah commented Oct 31, 2014

@jfrazelle @tianon I think this PR was changed after starting; initially it made the FROM statement optional, but after that it was changed to make FROM scratch a "special case" and start with an empty image in stead of pulling the "scratch" image from the registry.

Yup, it's confusing 😄

@jessfraz

This comment has been minimized.

Copy link
Contributor

jessfraz commented Oct 31, 2014

oooo ok wow, now caught up

@jessfraz

This comment has been minimized.

Copy link
Contributor

jessfraz commented Oct 31, 2014

@jlhawn can you confirm the above is true, and its not removing the FROM line

@tiborvass

This comment has been minimized.

Copy link
Collaborator

tiborvass commented Nov 21, 2014

@crosbymichael @jlhawn I summed it all up in #4242 (comment)

@jlhawn Let us know when you want us to take another look at this PR.

@jlhawn jlhawn force-pushed the jlhawn:build_implied_from_scratch branch from b681520 to 411eaa6 Dec 16, 2014

@jlhawn

This comment has been minimized.

Copy link
Contributor Author

jlhawn commented Dec 16, 2014

@tiborvass Let's see what these tests say

@jlhawn

This comment has been minimized.

Copy link
Contributor Author

jlhawn commented Dec 16, 2014

@tiborvass it's back to being merge-able. can you and @erikh, and anyone else you need, please review

@erikh

This comment has been minimized.

Copy link
Contributor

erikh commented Dec 16, 2014

Will do tonight

On Dec 16, 2014, at 1:18 PM, Josh Hawn notifications@github.com wrote:

@tiborvass https://github.com/tiborvass it's back to being merge-able. can you and @erikh https://github.com/erikh, and anyone else you need, please review


Reply to this email directly or view it on GitHub #8827 (comment).

@erikh

This comment has been minimized.

Copy link
Contributor

erikh commented Dec 17, 2014

LGTM

@jlhawn

This comment has been minimized.

Copy link
Contributor Author

jlhawn commented Dec 17, 2014

thanks @erikh!

what do you think @tiborvass, @tianon ?

@@ -230,9 +230,9 @@ func TestEventsRedirectStdout(t *testing.T) {

func TestEventsImagePull(t *testing.T) {
since := time.Now().Unix()
pullCmd := exec.Command(dockerBinary, "pull", "scratch")
pullCmd := exec.Command(dockerBinary, "pull", "busybox")

This comment has been minimized.

@tianon

tianon Dec 17, 2014

Member

If you're just looking for something tiny, the hello-world image is even smaller than busybox. 😉

This comment has been minimized.

@unclejack

unclejack Dec 17, 2014

Contributor

hello-world would make test execution faster.

This comment has been minimized.

@jlhawn

jlhawn Dec 17, 2014

Author Contributor

Oh I missed this one. I meant to switch it to docker_scratch too. We can't use scratch anymore because the tagStore will refuse to install it with that name. Do you recommend that I switch this to hello-world in the other location (https://github.com/docker/docker/pull/8827/files#r22012185) too?

@@ -9,11 +9,11 @@ import (

// pulling an image from the central registry should work
func TestPullImageFromCentralRegistry(t *testing.T) {
pullCmd := exec.Command(dockerBinary, "pull", "scratch")
pullCmd := exec.Command(dockerBinary, "pull", "docker_scratch")

This comment has been minimized.

@tianon

tianon Dec 17, 2014

Member

Another special top-level image just for Docker's test suite? 😞

@crosbymichael crosbymichael added this to the 1.5.0 milestone Dec 18, 2014

@jlhawn jlhawn force-pushed the jlhawn:build_implied_from_scratch branch from 411eaa6 to e2dd604 Dec 18, 2014

@jlhawn

This comment has been minimized.

Copy link
Contributor Author

jlhawn commented Dec 18, 2014

I've rebased again. I also modified some test cases to pull the small hello-world image instead of busybox or docker_scratch

@tianon

This comment has been minimized.

Copy link
Member

tianon commented Dec 18, 2014

The new public top-level image docker_scratch still makes me sick to my stomach, but otherwise the feature LGTM.

@jlhawn

This comment has been minimized.

Copy link
Contributor Author

jlhawn commented Dec 18, 2014

@tianon It should be safe to remove it now since the tests shouldn't be pulling it any more.

Make `FROM scratch` a special cased 'no-base' spec
There has been a lot of discussion (issues 4242 and 5262) about making
`FROM scratch` either a special case or making `FROM` optional, implying
starting from an empty file system.

This patch makes the build command `FROM scratch` special cased from now on
and if used does not pull/set the the initial layer of the build to the ancient
image ID (511136ea..) but instead marks the build as having no base image. The
next command in the dockerfile will create an image with a parent image ID of "".
This means every image ever can now use one fewer layer!

This also makes the image name `scratch` a reserved name by the TagStore. You
will not be able to tag an image with this name from now on. If any users
currently have an image tagged as `scratch`, they will still be able to use that
image, but will not be able to tag a new image with that name.

Goodbye '511136ea3c5a64f264b78b5433614aec563103b4d4702f3ba7d4d2698e22c158',
it was nice knowing you.

Fixes #4242

Docker-DCO-1.1-Signed-off-by: Josh Hawn <josh.hawn@docker.com> (github: jlhawn)

@jlhawn jlhawn force-pushed the jlhawn:build_implied_from_scratch branch from e2dd604 to 8936789 Dec 18, 2014

@jlhawn

This comment has been minimized.

Copy link
Contributor Author

jlhawn commented Dec 18, 2014

@tianon I've updated the tests to work with an image it creates called emptyfs and removed the docker_scratch image I created from the official registry.

@unclejack once the current test/build passes this should be okay for you to review again.

@tianon

This comment has been minimized.

Copy link
Member

tianon commented Dec 18, 2014

🎉 👍 ❤️

@unclejack

This comment has been minimized.

Copy link
Contributor

unclejack commented Dec 18, 2014

LGTM

@jlhawn

This comment has been minimized.

Copy link
Contributor Author

jlhawn commented Dec 18, 2014

IT'S HAPPENING

@tiborvass

This comment has been minimized.

Copy link
Collaborator

tiborvass commented Dec 18, 2014

LGTM

tiborvass added a commit that referenced this pull request Dec 18, 2014

Merge pull request #8827 from jlhawn/build_implied_from_scratch
Make `FROM scratch` a special cased 'no-base' spec

@tiborvass tiborvass merged commit 610842f into moby:master Dec 18, 2014

1 check passed

default The build succeeded on drone.io
Details
@tiborvass

This comment has been minimized.

Copy link
Collaborator

tiborvass commented Dec 18, 2014

@jlhawn Thank you for your patience and for flying DockAir :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment