Remove hack to copy systemd libraries from host in fluentd-gcp container. #101
Conversation
| fi | ||
|
|
||
| /usr/local/bin/fluentd $@ | ||
| exec /usr/local/bin/fluentd "$@" |
There was a problem hiding this comment.
Can we delete this script now?
There was a problem hiding this comment.
As I pointed out in the PR/commit description, I suggest keeping it around just in case someone is still running it with something like:
docker run gcr.io/google-containers/fluentd-gcp:latest /run.sh $FLUENTD_ARGSI don't think it's that detrimental to keep it around, since the space it takes is really minimal...
So I think I'd rather not delete it, or at least not delete it right away. I could be swayed the other way.
There was a problem hiding this comment.
Oh sorry, I missed that. I suggest adding this to the script, so we can clean it up later (and it's clear why it's still here):
echo >&2 "DEPRECATED: use /usr/local/bin/fluentd directly"
| fi | ||
|
|
||
| /usr/local/bin/fluentd $@ | ||
| exec /usr/local/bin/fluentd "$@" |
There was a problem hiding this comment.
Oh sorry, I missed that. I suggest adding this to the script, so we can clean it up later (and it's clear why it's still here):
echo >&2 "DEPRECATED: use /usr/local/bin/fluentd directly"
Done! PTAL |
|
/lgtm |
|
@tallclair thanks a lot for reviewing! @filbranden could you please squash commits? |
@crassirostris I actually intended to keep them as separate commits, since they make sense on their own. If you really prefer them squashed, you can do so at merge time with the "Squash and merge" option in the drop down next to the merge button on GitHub (see here for details and a screenshot.) |
|
I just saw a small snag here... In creation of /var/log/journal (in /run.sh or Dockerfile)... Because the way this container is used, the host's /var/log is mounted into the container's /var/log, so the mkdir from /run.sh actually creates that directory in the host's /var/log if it doesn't exist there... Should we remove some intermediate commits to remove that change (and related ones) from that PR? In particular, that means that /run.sh continues to be used (so we're dropping 3 commits, the mkdir, the change to CMD and the deprecation warning added...) @crassirostris Let me know what you'd like to do about this one! |
There was a problem hiding this comment.
If you really prefer them squashed, you can do so at merge time with the "Squash and merge" option in the drop down next to the merge button on GitHub (see here for details and a screenshot.)
Manually squashing them will make the resulting commit more concise and meaningful
fluentd-gcp-image/Dockerfile
Outdated
| @@ -45,6 +45,9 @@ RUN BUILD_DEPS="make gcc g++ libc6-dev ruby-dev libffi-dev" \ | |||
| /var/log/* \ | |||
| /var/tmp/* | |||
|
|
|||
| # For systems without journald. | |||
| RUN mkdir -p /var/log/journal | |||
There was a problem hiding this comment.
Oh, that actually won't work, because /var/log is mounted in runtime and this directory is dropped
There was a problem hiding this comment.
Yes I seem to have noticed the same thing :-)
I'll rework that and send a single squashed commit. Hang on.
This hack doesn't work on e.g. CentOS 7 where systemd links to versions of libgcrypt etc. that are not available in the container. This hack was used to ensure the systemd libraries in the container could access the journald files from the host, which could use either XZ or LZ4 compression, and the fluentd-gcp-image only had support for XZ. This is no longer the case, since the base image debian-base:0.3 is now based on Debian Stretch (and not Debian Jessie) which supports both XZ and LZ4, so this hack is no longer required. Also: Properly quote "$@" in fluentd command-line. That is the correct usage of $@. And exec fluentd binary, so the shell is no longer in the way. There's really no reason to keep a shell running inside the container... Bump version of fluentd-gcp container to 2.0.16 in order to release these changes as a new version.
|
@filbranden What you proposed (dropping some parts of the changes) sounds reasonable, thanks! |
|
Done. And squashed. PTAL. |
|
LGTM, thanks! |
|
Thanks @crassirostris ! Do you have plans to release a new 2.0.16 version to GCR? Let me know when that's available, then I'll push changes to Kubernetes to start using it and stop trying to mount the host's lib directory that's no longer used... Cheers! |
|
/cc @bmoyles0117 |
|
I pushed 2.0.16 to gcr.io and will modify kubernetes/kubernetes#59808 to use it in Kubernetes. Currently this PR bumps the version to 2.0.15 which is broken anyway. |
This mapping is no longer needed since fluentd-gcp v2.0.16, in which it started using a container image based on Debian Stretch, in which the systemd libraries already include support for all the supported compression algorithms. The /run.sh in the image no longer accesses /host/lib anyways, so let's stop mapping it here. Related changes: - fluentd-gcp on GoogleCloudPlatform/k8s-stackdriver#101 - fluentd-es on GoogleCloudPlatform/google-fluentd#80
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Remove mapping to /host/lib from fluentd-gcp container. **What this PR does / why we need it**: This mapping is no longer needed since fluentd-gcp v2.0.16, in which it started using a container image based on Debian Stretch, in which the systemd libraries already include support for all the supported compression algorithms. The `/run.sh` in the image no longer accesses `/host/lib` anyways, so let's stop mapping it here. Related changes: - fluentd-gcp on GoogleCloudPlatform/k8s-stackdriver#101 - fluentd-es on GoogleCloudPlatform/google-fluentd#80 /assign @timstclair /cc @crassirostris @bmoyles0117 **Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*: N/A **Special notes for your reviewer**: N/A **Release note**: ```release-note NONE ```
This mapping is no longer needed since fluentd-gcp v2.0.16, in which it started using a container image based on Debian Stretch, in which the systemd libraries already include support for all the supported compression algorithms. The /run.sh in the image no longer accesses /host/lib anyways, so let's stop mapping it here. Related changes: - fluentd-gcp on GoogleCloudPlatform/k8s-stackdriver#101 - fluentd-es on GoogleCloudPlatform/google-fluentd#80
Remove hack to copy systemd libraries from host in fluentd-gcp container. That hack was present to make fluentd-gcp be able to access journal files from COS, which are created using LZ4 compression. But debian-base:0.3 (on which this is now based) already supports both +XZ and +LZ4 compression, so this hack is no longer needed.
That hack breaks on, say, CentOS 7, where the systemd libraries link to other versions of libraries that are not available in the fluentd-gcp.
For more context, see some discussion here.
Also clean up the /run.sh shell script and effectively stop using it altogether (though still ship it, for compatibility.) See individual commits for the individual cleanup steps.
Bump version to 2.0.16 to incorporate these changes.
cc @crassirostris who introduced the /host/lib hack to this container.
cc @adityakali to help test this in COS.
cc @x13n who pushed latest changes to this container. (Can you help us build official versions with these commits?)
cc @tallclair who bumped debian-base from 0.1 to 0.3 in this container.