Convert to Docker Official Images #152

lag-linaro · 2018-11-19T13:56:44Z

When this project last attempted to become part of Docker Official Images, it was heavily reviewed.

This PR attempts to address each of the issues raised by @tianon.

For reference, the last review took place here:

LINK: docker-library/official-images#3724 (comment)

repeatedly · 2018-11-21T05:15:38Z

Currently, many users use onbuild images to use 3rd party plugins and own configuration.
So we need the announcement for changes and need feedback from users.

lag-linaro · 2018-11-21T08:55:49Z

Okay, understood.

How about the remainder of the changes?

tianon · 2018-11-21T14:28:40Z

If you know your users are successfully using ”ONBUILD" variants and are fine with their limitations (and you're fine maintaining them), we're willing to make exceptions there (see "sentry" for example). 👍 (I haven't had a chance to look at the rest of these changes yet, but I do plan to pre-review this before it hits our queue to try and help out here.)

lag-linaro · 2018-11-22T07:27:29Z

Thanks @tianon. A review from you would be good.

tianon

Had a bit of spare time so I took a first pass. 👍

v0.12/alpine/Dockerfile

tianon · 2018-11-22T18:40:35Z

v0.12/alpine/entrypoint.sh

+
+# If user does not supply config file or plugins, use the default
+if [ "$1" == "fluentd" ]; then
+    if ! echo $@ | grep '\-c' ; then


This grep is a little bit hacky -- if given multiple -c options, does fluentd not simply prefer the last one specified? It appears to:

$ docker run --rm fluent/fluentd:v1.2.3-debian fluentd -c /dev/null -c /fluentd/etc/fluent.conf 2018-11-22 18:38:07 +0000 [info]: parsing config file is succeeded path="/fluentd/etc/fluent.conf" ...

So these default flags could instead be specified like this:

if [ "$1" = 'fluentd' ]; then shift # remove 'fluentd' so we can prepend easier set -- fluentd -c /fluentd/etc/fluent.conf -p /fluentd/plugins "$@" # set defaults for "-c" and "-p" (which are then overrideable via the container command) fi

This seems even hackier than my solution. Don't you think this will confuse users?

By "hacky", I mean "fragile". With the current solution, if there's the string -c anywhere in the supplied arguments, it will not apply the default configuration, which is more likely to break.

For the purposes of the official images, I'm much more concerned with the user's usage patterns than what the final generated command ends up being (multiple -c parameters would likely be slightly more confusing for users, but that can be trivially mitigated here by a comment that explains that the behavior of fluentd is to prefer the last-specified-flag for each option given, which is why these get pre-pended to the front of the command string instead of appended to the end). The end goal is to have a reliable means of supplying a default configuration file and a trivial way for users to override it, which the solution I've proposed will provide in all cases I've tested, which I can't say about echo ... | grep '\-c'. The echo solution would be slightly better with something like haveConfig=; for arg; do if [ "$arg" = '-c' ]; then haveConfig=1; break; fi; done; if [ -z "$haveConfig" ]; then set -- "$@" -c ...; fi, but is significantly more verbose.

I think this is likely another place to solicit preference from the maintainer (@repeatedly). 👍

The latter solution won't catch instances of -c/fluentd/etc/fluent.conf which is still valid.

How about I expand my solution to if ! echo $@ | grep '<SPACE>\-c' ; then, since <SPACE>-[a-zA-Z0-9] should always (?) be treated as a flagged argument, there shouldn't be any false positives and should therefore be less fragile.

v0.12/debian/Dockerfile

tianon · 2018-11-22T19:34:54Z

v0.12/debian/entrypoint.sh

    set -o allexport
    source $DEFAULT
    set +o allexport
 fi


Is this sourcing code just to set (the now-removed) FLUENTD_CONF and FLUENTD_OPT variables or does it have other uses?

No idea. I guess this is one for @repeatedly.

does it have other uses?

Fluentd users sometimes set own environment variable via this mechanizm.
So this image keeps same mechaznim.

tianon · 2018-11-22T19:37:15Z

v0.12/debian/entrypoint.sh

+    fi
+fi

 exec gosu fluent "$@"


Would it be worthwhile to update this for allowing arbitrary --user values? ie something like:

if [ "$(id -u)" = '0' ]; then exec gosu fluent "$@" else exec "$@" fi

Not sure this is required for this project.

Best ask @repeatedly.

Is it important for official images?
We can add this change after become official, right?

Judging by the language @tianon uses here, I would say that it is not a blocker for Official Images.

Thus adding this change as a subsequent PR sounds reasonable.

Dockerfile.template.erb

entrypoint.sh.erb

lag-linaro · 2018-11-23T08:34:49Z

Thanks for your review @tianon. I will have a look at fixing (mostly adding really) your comments.

@tianon

Note that we can't convert Alpine ARMHF images, as they are not pushed to Docker Hub. This issue was highlighted by @tianon LINK: fluent#152 (comment) This is part of an effort to make FluentD part of Docker's Official Images. Signed-off-by: Lee Jones <[email protected]>

lag-linaro · 2018-11-23T15:43:17Z

Phew! Okay @tianon should be all done.

I didn't change the semantics where we search for the -c flag and insert the default one if absent, since I thought having more than one configuration/plugin argument on the command line would be more confusing and is more of a hack than the current implementation.

Everything else has been fixed up though.

I'd really appreciate a review from both @tianon and @repeatedly so we can have this merged. 😃

edsiper · 2018-11-23T15:54:24Z

question:

is it time to get rid of Alpine images ?, basically without Jemalloc there is a lot of memory fragmentation on high load environments, I don't see how that is healthy in a containerized environment.

tianon · 2018-11-23T16:41:48Z

is it time to get rid of Alpine images ?, basically without Jemalloc there is a lot of memory fragmentation on high load environments, I don't see how that is healthy in a containerized environment.

For the official images, the backwards compatibility concern with removing isn't a large concern (so this would be fine from our perspective), but the real call there is likely up to @repeatedly (we don't have a strong preference either way). 👍

lag-linaro · 2018-11-29T08:22:16Z

@repeatedly could you please review the PR?

edsiper · 2018-11-29T17:36:20Z

@lag-linaro

I did a quick build test and found the following problem in one of the arm images:

docker build   -t fluent/fluentd:v1.3.0-debian-armhf v1.3/armhf/debian --build-arg VERSION=v1.3.0-debian-armhf
Sending build context to Docker daemon  9.728kB
Step 1/21 : FROM resin/armv7hf-debian:stretch
 ---> b23e2470b550
Step 2/21 : ARG VERSION=1.1
 ---> Using cache
 ---> 251d8834050f
Step 3/21 : LABEL maintainer "TAGOMORI Satoshi <[email protected]>"
 ---> Using cache
 ---> ea48e88ac954
Step 4/21 : LABEL Description="Fluentd docker image" Vendor="Fluent Organization" Version=${VERSION}
 ---> Using cache
 ---> 59058334f972
Step 5/21 : ARG CROSS_BUILD_START="cross-build-start"
 ---> Using cache
 ---> a1833a170101
Step 6/21 : ARG CROSS_BUILD_END="cross-build-end"
 ---> Using cache
 ---> 62443ed896c8
Step 7/21 : RUN [ ${CROSS_BUILD_START} ]
 ---> Using cache
 ---> 8c90c07e36f5
Step 8/21 : ENV TINI_VERSION=0.18.0
 ---> Using cache
 ---> 5e9d869d34b3
Step 9/21 : ENV GOSU_VERSION=1.10-1+b2
 ---> Using cache
 ---> e50b7f9f140e
Step 10/21 : RUN apt-get update  && apt-get install -y --no-install-recommends             ca-certificates             ruby             tini             gosu=${GOSU_VERSION}  && buildDeps="       make gcc g++ libc-dev       ruby-dev       wget bzip2 gnupg dirmngr     "  && apt-get install -y --no-install-recommends $buildDeps  && echo 'gem: --no-document' >> /etc/gemrc  && gem install oj -v 3.3.10  && gem install json -v 2.1.0  && gem install fluentd -v 1.3.0  && gosu nobody true  && wget -O /tmp/jemalloc-4.5.0.tar.bz2 https://github.com/jemalloc/jemalloc/releases/download/4.5.0/jemalloc-4.5.0.tar.bz2  && cd /tmp && tar -xjf jemalloc-4.5.0.tar.bz2 && cd jemalloc-4.5.0/  && ./configure && make  && mv lib/libjemalloc.so.2 /usr/lib  && apt-get purge -y --auto-remove                   -o APT::AutoRemove::RecommendsImportant=false                   $buildDeps  && rm -rf /var/lib/apt/lists/*  && rm -rf /tmp/* /var/tmp/* /usr/lib/ruby/gems/*/cache/*.gem
 ---> Running in dccd4e6822a9
Ign:1 http://deb.debian.org/debian stretch InRelease
Get:2 http://security.debian.org/debian-security stretch/updates InRelease [94.3 kB]
Get:3 http://deb.debian.org/debian stretch-updates InRelease [91.0 kB]
Get:4 http://deb.debian.org/debian stretch Release [118 kB]
Get:5 http://deb.debian.org/debian stretch Release.gpg [2434 B]
Get:6 http://security.debian.org/debian-security stretch/updates/main armhf Packages [445 kB]
Get:7 http://deb.debian.org/debian stretch-updates/main armhf Packages [5108 B]
Get:8 http://deb.debian.org/debian stretch/main armhf Packages [6919 kB]
Fetched 7675 kB in 10s (699 kB/s)
Reading package lists...
Reading package lists...
Building dependency tree...
Reading state information...
E: Unable to locate package tini
The command '/bin/sh -c apt-get update  && apt-get install -y --no-install-recommends             ca-certificates             ruby             tini             gosu=${GOSU_VERSION}  && buildDeps="       make gcc g++ libc-dev       ruby-dev       wget bzip2 gnupg dirmngr     "  && apt-get install -y --no-install-recommends $buildDeps  && echo 'gem: --no-document' >> /etc/gemrc  && gem install oj -v 3.3.10  && gem install json -v 2.1.0  && gem install fluentd -v 1.3.0  && gosu nobody true  && wget -O /tmp/jemalloc-4.5.0.tar.bz2 https://github.com/jemalloc/jemalloc/releases/download/4.5.0/jemalloc-4.5.0.tar.bz2  && cd /tmp && tar -xjf jemalloc-4.5.0.tar.bz2 && cd jemalloc-4.5.0/  && ./configure && make  && mv lib/libjemalloc.so.2 /usr/lib  && apt-get purge -y --auto-remove                   -o APT::AutoRemove::RecommendsImportant=false                   $buildDeps  && rm -rf /var/lib/apt/lists/*  && rm -rf /tmp/* /var/tmp/* /usr/lib/ruby/gems/*/cache/*.gem' returned a non-zero code: 100
make[1]: *** [Makefile:60: image] Error 100

looks like related to the missing dependency:

E: Unable to locate package tini

lag-linaro · 2018-11-29T18:58:47Z

Thank you for testing this @edsiper.

Ah, I didn't test the armhf build since it won't be part of the Official Images suite. I will do so in future.

I can fix this, although what I'd really like to do is remove the cross-complied armhf build entirely.

The main/default Dockerfile should support armhf (and all the other architectures) without issue.

@tianon

Note that we can't convert Alpine ARMHF images, as they are not pushed to Docker Hub. This issue was highlighted by @tianon LINK: fluent#152 (comment) This is part of an effort to make FluentD part of Docker's Official Images. Signed-off-by: Lee Jones <[email protected]>

lag-linaro · 2018-11-30T09:15:46Z

This pull-request has been rebased, so no more merge conflicts.

The Travis failures are due to GPG issues (which aren't anything to do with this PR) and on-build issues where the images are missing. The latter will be automatically rectified once we're in Official Images.

runningman84 · 2018-12-05T19:26:26Z

@lag-linaro do you have some build already published somewhere? I have a mixed k8s env with amd64/arm64 and would be happy to test if I can deploy it with the same daemonset...

repeatedly

By this patch, we remove FLIENT_UID for #78
We should document proper way for id mapping.

If we break backward compatibility, we should add version tag to new images?
fluent/fluentd-kubernetes-daemonset#213

repeatedly · 2018-12-05T20:29:58Z

v0.12/debian/entrypoint.sh

    set -o allexport
    source $DEFAULT
    set +o allexport
 fi


does it have other uses?

Fluentd users sometimes set own environment variable via this mechanizm.
So this image keeps same mechaznim.

repeatedly · 2018-12-05T20:31:21Z

v0.12/debian/entrypoint.sh

+    fi
+fi

 exec gosu fluent "$@"


Is it important for official images?
We can add this change after become official, right?

lag-linaro · 2018-12-12T07:18:08Z

@lag-linaro do you have some build already published somewhere? I have a mixed k8s env with amd64/arm64 and would be happy to test if I can deploy it with the same daemonset...

I can push a build for you if it helps?

lag-linaro · 2018-12-12T07:18:43Z

@repeatedly @edsiper @tianon: what's blocking this currently?

runningman84 · 2018-12-12T17:59:56Z

@lag-linaro do you have some build already published somewhere? I have a mixed k8s env with amd64/arm64 and would be happy to test if I can deploy it with the same daemonset...

I can push a build for you if it helps?

yes a multiarch image in docker hub would help...

repeatedly · 2018-12-12T19:26:27Z

No blocker for patch itself.
We need to change tag format before because it may break existing environment and it shouldn't.

#159

lag-linaro · 2018-12-13T09:56:35Z

@lag-linaro do you have some build already published somewhere? I have a mixed k8s env with amd64/arm64 and would be happy to test if I can deploy it with the same daemonset...

I can push a build for you if it helps?

yes a multiarch image in docker hub would help...

It's not multi-arch (no images are), but here is an aarch64 (arm64, armv8) one for you:

https://hub.docker.com/r/laglinaro/fluentd-v1.3-aarch64/

To pull it on your AArch64 based platform use:

docker pull laglinaro/fluentd-v1.3-aarch64

lag-linaro · 2018-12-13T09:56:58Z

No blocker for patch itself.
We need to change tag format before because it may break existing environment and it shouldn't.

#159

Understood. Do you know when this might be resolved?

repeatedly · 2018-12-19T03:51:52Z

I just merged #161
So ready to merge this patch.

lag-linaro · 2018-12-19T07:30:11Z

Wonderful. Do you know when it will be done?

repeatedly · 2018-12-19T10:37:08Z

@lag-linaro Could you resolve the conflict?
master's fluentd version is v1.3.2 so need to merge master change.

lag-linaro · 2018-12-19T11:37:31Z

@lag-linaro Could you resolve the conflict?
master's fluentd version is v1.3.2 so need to merge master change.

Yes, I'll do that right away.

@tianon

Note that we can't convert Alpine ARMHF images, as they are not pushed to Docker Hub. This issue was highlighted by @tianon LINK: fluent#152 (comment) This is part of an effort to make FluentD part of Docker's Official Images. Signed-off-by: Lee Jones <[email protected]>

@tianon

As suggested by @tianon LINK: docker-library/official-images#3724 (comment) This is part of an effort to make FluentD part of Docker's Official Images. Signed-off-by: Lee Jones <[email protected]>

@tianon

As suggested by @tianon LINK: docker-library/official-images#3724 (comment) This is part of an effort to make FluentD part of Docker's Official Images. Also see LINK: https://docs.docker.com/engine/userguide/eng-image/dockerfile_best-practices/#apt-get Signed-off-by: Lee Jones <[email protected]>

@tianon

As suggested by @tianon LINK: docker-library/official-images#3724 (comment) This is part of an effort to make FluentD part of Docker's Official Images. Also see LINK: https://docs.docker.com/engine/userguide/eng-image/dockerfile_best-practices/#apt-get Signed-off-by: Lee Jones <[email protected]>

@tianon

As suggested by @tianon LINK: docker-library/official-images#3724 (comment) This is part of an effort to make FluentD part of Docker's Official Images. Signed-off-by: Lee Jones <[email protected]>

@tianon

The FluentD package does not provide a 'fluent' user. Until now hacks exist in the entrypoint.sh which provide one manually at run-time. Instead of doing that, let's create the user when the container is created. This issue was highlighted by @tianon LINK: docker-library/official-images#3724 (comment) Signed-off-by: Lee Jones <[email protected]>

@tianon

@tianon

This allows us to remove the `FLUENTD_CONF` and `FLUENTD_OPT` envs. As suggested by @tianon LINK: docker-library/official-images#3724 (comment) This is part of an effort to make FluentD part of Docker's Official Images. Signed-off-by: Lee Jones <[email protected]>

Tiny is much more cross-platform friendly than dumb-init. Signed-off-by: Lee Jones <[email protected]>

Signed-off-by: Lee Jones <[email protected]>

LINK: https://bugs.debian.org/913614 LINK: docker-library/busybox#55 Signed-off-by: Lee Jones <[email protected]>

lag-linaro · 2018-12-20T14:34:42Z

Awesome.

Will you be submitting the change to Docker's Official Images repo?

An updated version of this PR: docker-library/official-images#3724

lag-linaro mentioned this pull request Nov 19, 2018

Remove *-onbuild images. #151

Closed

tianon reviewed Nov 22, 2018

View reviewed changes

lag-linaro force-pushed the convert-to-docker-official-images branch from 4ff7f21 to f38a9dc Compare November 23, 2018 15:24

lag-linaro force-pushed the convert-to-docker-official-images branch 2 times, most recently from d6aaa84 to da77e14 Compare November 30, 2018 09:04

repeatedly reviewed Dec 5, 2018

View reviewed changes

repeatedly mentioned this pull request Dec 18, 2018

Add version to tag #161

Merged

Lee Jones added 8 commits December 19, 2018 11:46

Remove DEBIAN_FRONTEND=noninteractive

3333994

As suggested by @tianon LINK: docker-library/official-images#3724 (comment) This is part of an effort to make FluentD part of Docker's Official Images. Signed-off-by: Lee Jones <[email protected]>

Do not manually run update-ca-certificates

749047a

As suggested by @tianon LINK: docker-library/official-images#3724 (comment) This is part of an effort to make FluentD part of Docker's Official Images. Signed-off-by: Lee Jones <[email protected]>

lag-linaro force-pushed the convert-to-docker-official-images branch from da77e14 to 223e5fa Compare December 19, 2018 11:56

Lee Jones added 5 commits December 19, 2018 12:14

Replace dumb-init with tini

75f3fa2

Tiny is much more cross-platform friendly than dumb-init. Signed-off-by: Lee Jones <[email protected]>

Make entrypoint.sh executable on creation

536d949

Signed-off-by: Lee Jones <[email protected]>

Use Alpine's (Busybox's) built-in version of wget

7ec057f

Signed-off-by: Lee Jones <[email protected]>

Use --no-cache instead of manually removing /var/cache/apk/*

3ff0df6

Signed-off-by: Lee Jones <[email protected]>

Use GPG '--batch' option and remove '--no-tty'

5896b1e

LINK: https://bugs.debian.org/913614 LINK: docker-library/busybox#55 Signed-off-by: Lee Jones <[email protected]>

lag-linaro force-pushed the convert-to-docker-official-images branch from 223e5fa to 5896b1e Compare December 19, 2018 12:14

repeatedly merged commit d5b7782 into fluent:master Dec 20, 2018

lag-linaro deleted the convert-to-docker-official-images branch December 20, 2018 14:34

This was referenced Dec 20, 2018

Use tagged version for onbuild #163

Merged

Create a new official image for fluentd docker-library/official-images#3724

Closed

tianon mentioned this pull request Apr 26, 2019

Add fluentd images docker-library/official-images#5303

Merged

9 tasks

Convert to Docker Official Images #152

Convert to Docker Official Images #152

Uh oh!

Conversation

lag-linaro commented Nov 19, 2018

Uh oh!

repeatedly commented Nov 21, 2018

Uh oh!

lag-linaro commented Nov 21, 2018

Uh oh!

tianon commented Nov 21, 2018 via email

Uh oh!

lag-linaro commented Nov 22, 2018

Uh oh!

tianon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lag-linaro Nov 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lag-linaro commented Nov 23, 2018

Uh oh!

lag-linaro commented Nov 23, 2018

Uh oh!

edsiper commented Nov 23, 2018

Uh oh!

tianon commented Nov 23, 2018

Uh oh!

lag-linaro commented Nov 29, 2018

Uh oh!

edsiper commented Nov 29, 2018

Uh oh!

lag-linaro commented Nov 29, 2018

Uh oh!

lag-linaro commented Nov 30, 2018

Uh oh!

runningman84 commented Dec 5, 2018

Uh oh!

repeatedly left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lag-linaro commented Dec 12, 2018

Uh oh!

lag-linaro commented Dec 12, 2018

Uh oh!

runningman84 commented Dec 12, 2018

Uh oh!

lag-linaro Nov 26, 2018 •

edited

Loading

repeatedly left a comment •

edited

Loading