Skip to content
This repository was archived by the owner on May 6, 2020. It is now read-only.

ref(Dockerfile): use base 0.3.4, smaller packages and better cleanup#1088

Merged
helgi merged 1 commit intodeis:masterfrom
helgi:update_image
Sep 27, 2016
Merged

ref(Dockerfile): use base 0.3.4, smaller packages and better cleanup#1088
helgi merged 1 commit intodeis:masterfrom
helgi:update_image

Conversation

@helgi
Copy link
Copy Markdown
Contributor

@helgi helgi commented Sep 25, 2016

Use python3-pip instead of using non-package management approach as this provides us with the proper pip version anyway, and better cleanup

Old vs New image size:

REPOSITORY                TAG                 IMAGE ID             SIZE
quay.io/deis/controller   v2.5.1              8e926c780d4d         214.4 MB
quay.io/helgi/controller   canary              a1017a79f8a2       204.8 MB

@helgi helgi added this to the v2.6 milestone Sep 25, 2016
@helgi helgi self-assigned this Sep 25, 2016
@deis-bot
Copy link
Copy Markdown

@mboersma, @krancour and @bacongobbler are potential reviewers of this pull request based on my analysis of git blame information. Thanks @helgi!

@codecov-io
Copy link
Copy Markdown

codecov-io commented Sep 26, 2016

Current coverage is 87.87% (diff: 100%)

Merging #1088 into master will increase coverage by 0.08%

@@             master      #1088   diff @@
==========================================
  Files            42         42          
  Lines          3603       3603          
  Methods           0          0          
  Messages          0          0          
  Branches        610        610          
==========================================
+ Hits           3163       3166     +3   
+ Misses          290        288     -2   
+ Partials        150        149     -1   

Powered by Codecov. Last update 260b1e4...6658336

Comment thread rootfs/Dockerfile Outdated
/lib/udev \
/usr/lib/x86_64-linux-gnu/gconv/IBM* \
/usr/lib/x86_64-linux-gnu/gconv/EBC* && \
mkdir -p /usr/share/man/man{1..8}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

can you wrap this with bash -c? See deis/docker-base#15

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Can you or @mboersma explain the rationale? We'd been using mkdir directly for a release or two already without issues I thought?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

deis/docker-base#14 should clarify.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Dockerfile commands run under /bin/sh. mkdir foo{1..8} is bash syntax. Notice the difference between these two commands:

$ docker run ubuntu /bin/sh -c 'mkdir -p /usr/share/man/man{1..8} && ls /usr/share/man'
cs
da
de
es
fi
fr
hu
id
it
ja
ko
man1
man3
man5
man7
man8
man{1..8}
nl
pl
pt
pt_BR
ru
sl
sv
tr
zh_CN
zh_TW
$ docker run ubuntu /bin/bash -c 'mkdir -p /usr/share/man/man{1..8} && ls /usr/share/man'
cs
da
de
es
fi
fr
hu
id
it
ja
ko
man1
man2
man3
man4
man5
man6
man7
man8
nl
pl
pt
pt_BR
ru
sl
sv
tr
zh_CN
zh_TW

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hmmm - that's fair. annoying tho since it made it all the way up to ubuntu-slim :| guess it wasn't really tested the first time around :(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It was originally tested, but it was broken in deis/docker-base#13. Before then we were wrapping it with bash -c: https://github.com/deis/docker-base/blob/cb24562c729d9ad87abd1d74f0786cd0f925eb6f/rootfs/Dockerfile#L16

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Didn't even see that change in that PR... trippy

@helgi helgi force-pushed the update_image branch 2 times, most recently from 4a3d91f to cc0f507 Compare September 26, 2016 16:57
@helgi helgi changed the title ref(Dockerfile): use base 0.3.2, smaller packages and better cleanup ref(Dockerfile): use base 0.3.3, smaller packages and better cleanup Sep 26, 2016
@helgi helgi changed the title ref(Dockerfile): use base 0.3.3, smaller packages and better cleanup ref(Dockerfile): use base 0.3.4, smaller packages and better cleanup Sep 26, 2016
@jchauncey
Copy link
Copy Markdown
Member

this caused the controller to crashloop

@helgi
Copy link
Copy Markdown
Contributor Author

helgi commented Sep 26, 2016

@jchauncey thanks, already looking into it - doesn't happen on my persona cluster so I'm having to look at other places right now

Comment thread rootfs/Dockerfile Outdated
/lib/udev \
/usr/lib/x86_64-linux-gnu/gconv/IBM* \
/usr/lib/x86_64-linux-gnu/gconv/EBC* && \
mkdir -p /usr/share/man/man{1..8}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This mkdir command is redundant with the same one in deis/base, so it shouldn't be necessary. However, see deis/docker-base#14.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is not redundant - like lines above you can see /usr/share/man being included in the rm -rf statement

Comment thread rootfs/Dockerfile Outdated
/lib/udev \
/usr/lib/x86_64-linux-gnu/gconv/IBM* \
/usr/lib/x86_64-linux-gnu/gconv/EBC* && \
mkdir -p /usr/share/man/man{1..8}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

deis/docker-base#14 should clarify.

Comment thread rootfs/Dockerfile
RUN buildDeps='gcc git libffi-dev libpq-dev python3-dev'; \
RUN buildDeps='gcc git libffi-dev libpq-dev python3-dev python3-pip python3-wheel python3-setuptools'; \
apt-get update && \
apt-get install -y --no-install-recommends \
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you prefix this with DEBIAN_FRONTEND=noninteractive? The warnings from apt-get are distracting.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Can you point me to where it is warning you off that?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't have logs handy, so just ignore me. It's a pervasive issue that isn't strictly in scope for this PR, so let's fix it later.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I asked because I haven't found it in the e2e building phase but I have seen it in chef and such before. Depends if the env in ubuntu-slim isn't doing its job

@mboersma mboersma added the LGTM1 label Sep 26, 2016
@kmala kmala added the LGTM2 label Sep 27, 2016
@helgi helgi merged commit 13c1711 into deis:master Sep 27, 2016
@helgi helgi deleted the update_image branch September 27, 2016 00:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants