Skip to content
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

docker: switch from musl to glibc, and simplify stuff #4219

Merged
merged 1 commit into from
Sep 14, 2017

Conversation

ghost
Copy link

@ghost ghost commented Sep 8, 2017

The fs-repo-migrations releases on dist.ipfs.io never worked with the go-ipfs docker image because it's based on musl. That means binaries linked against glibc don't work, and I manually built and patched in an additional musl-based tarball for each fs-repo-migrations release. This madness has to stop.

The Dockerfile now has two stages: build and assembly.
This allows for a full-fledged debian build container,
while still resulting in a super-thin glibc-based busybox image.

License: MIT
Signed-off-by: Lars Gierth larsg@systemli.org

Copy link
Member

@whyrusleeping whyrusleeping left a comment

Choose a reason for hiding this comment

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

After a quick overview, this LGTM. Though I definitely want someone else to look over it. cc @Kubuxu

@whyrusleeping whyrusleeping added this to the Ipfs 0.4.11 milestone Sep 11, 2017
RUN apt-get install -y ca-certificates

# Now comes the actual target image, which aims to be as small as possible.
FROM busybox:1-glibc
Copy link
Member

Choose a reason for hiding this comment

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

I heard some info that multiple FROM directives might be discontinued but as they are in stable Docker and AFAIK the Dockerfile format is still in change-lock state it should be safe to use.

@Kubuxu
Copy link
Member

Kubuxu commented Sep 11, 2017

@lgierth docker sharness tests are failing. Can you take a look at this?

EDIT: @lgierth don't worry about this, I will try to handle it.

repo="$IPFS_PATH"

if [ `id -u` -eq 0 ]; then
# ensure folder is writable
su-exec "$user" test -w "$repo" || chown -R -- "$user" "$repo"
Copy link
Contributor

Choose a reason for hiding this comment

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

This is needed to fix permissions (#3744) if you run docker run -v /does/not/exist/yet:/data/ipfs go-ipfs, otherwise go-ipfs can't write to it's home and crashes. This is also the reason why there's no USER ipfs in the dockerfile itself, as the permissions need to be preserved and then dropped in the entrypoint. :)

Dockerfile Outdated

# This installs a very simple program acting as the init process.
# Makes sure signals are properly passed to the ipfs daemon process.
ENV TINI_VERSION v0.16.1
Copy link
Contributor

Choose a reason for hiding this comment

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

On debian, I'd recommend dumb-init. :)

Dockerfile.fast Outdated
RUN mkdir -p $IPFS_PATH \
&& useradd -s /usr/sbin/nologin -d $IPFS_PATH -u 1000 -g 100 ipfs \
&& chown 1000:100 $IPFS_PATH
USER ipfs
Copy link
Contributor

Choose a reason for hiding this comment

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

USER ipfs needs to be removed as well so the entrypoint has the permissions to correct the ownership. :)

Dockerfile Outdated
ENV PATH /go/bin:$PATH
ENV SRC_PATH /go/src/github.com/ipfs/go-ipfs
RUN mkdir -p $IPFS_PATH && adduser -D -h $IPFS_PATH -u 1000 -g 100 ipfs
USER ipfs
Copy link
Contributor

Choose a reason for hiding this comment

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

here as well :)

@Kubuxu
Copy link
Member

Kubuxu commented Sep 12, 2017

Hmm, it goes beyond my head right now. I would either have to dig deep into how that Dockerfile works or IDK.

@kpcyrd
Copy link
Contributor

kpcyrd commented Sep 12, 2017

@Kubuxu the entrypoint is started as root, then the permissions are adjusted here:
https://github.com/ipfs/go-ipfs/blob/c14a995fab81f894371ed06ec989c32bdd628e3d/bin/container_daemon#L7-L8
After that, root permissions aren't needed anymore so the entrypoint is restarted with the ipfs user:
https://github.com/ipfs/go-ipfs/blob/c14a995fab81f894371ed06ec989c32bdd628e3d/bin/container_daemon#L9-L10
Afterwards, this part is executed with the ipfs user:
https://github.com/ipfs/go-ipfs/blob/c14a995fab81f894371ed06ec989c32bdd628e3d/bin/container_daemon#L13-L42

The most complicated part is the deprecation of the implicit daemon which was added about half a year ago so people start invoking the daemon with the new format. :)

@ghost ghost force-pushed the feat/docker-rework branch 3 times, most recently from 94e0205 to 5d5e605 Compare September 13, 2017 01:22
The Dockerfile now has two stages: build and assembly.
This allows for a full-fledged debian build container,
while still resulting in a super-thin busybox image.

License: MIT
Signed-off-by: Lars Gierth <larsg@systemli.org>
@ghost
Copy link
Author

ghost commented Sep 13, 2017

Okay, removed that new test for now -- it wasn't able to clean up after itself :(

@ghost
Copy link
Author

ghost commented Sep 13, 2017

That OSX failure on travis looks unrelated, it's t0180-p2p (cc @magik6k https://api.travis-ci.org/jobs/275047939/log.txt?deansi=true)

@magik6k
Copy link
Member

magik6k commented Sep 13, 2017

Looks like 500ms is not enough for travis to launch a program and write a pidfile:

  ma-pipe-unidir --listen --pidFile=listener.pid send /ip4/127.0.0.1/tcp/10101 < test0.bin &

  go-sleep 500ms &&
  kill -0 $(cat listener.pid) &&

  ipfsi 1 p2p stream dial $PEERID_0 p2p-test /ip4/127.0.0.1/tcp/10102 2>&1 > dialer-stdouterr.log &&
  ma-pipe-unidir recv /ip4/127.0.0.1/tcp/10102 > client.out &&
  test ! -f listener.pid

cat: listener.pid: No such file or directory

kill: usage: kill [-s sigspec | -n signum | -sigspec] pid | jobspec ... or kill -l [sigspec]

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

Successfully merging this pull request may close these issues.

4 participants