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

Smaller baseimage with Alpine #66

Merged
merged 21 commits into from
Nov 11, 2018
Merged

Smaller baseimage with Alpine #66

merged 21 commits into from
Nov 11, 2018

Conversation

bebehei
Copy link
Contributor

@bebehei bebehei commented Nov 6, 2018

It builds fine, but I'm currently testing it on my machine. But I'm not considering it mergable yet.


This gets rid of the phusion image and uses Alpine as the base. You don't have to care about security updates anymore, as these are included in Alpine.

Also it's possible to build it on aarch64 (and probably on all other docker supported archs, too). Also it builds much faster on my Odroid C2 (about 1/3 of the time).

This reduces the image size dramatically from ~530MB to 188MB.

Fixes #55


I've got a question: Why did you install NodeJS 6 in the image? Is it because statsd's latest release is from 2016 and NodeJS 6 then was the latest version? I don't find any notice in the commits.

We don't need git, pip and the dev libraries for rdd in the production
image.
The timezone argument can get seeded alternatively via the TZ env
variable or the special GRAPHITE_TIME_ZONE env.
@piotr1212
Copy link
Member

I've got a question: Why did you install NodeJS 6 in the image? Is it because statsd's latest release is from 2016 and NodeJS 6 then was the latest version? I don't find any notice in the commits.

Guess so, latest statsd 'release' doesn't work with newest nodejs. There are some patches in master which fix compatibility. I'm using https://github.com/etsy/statsd/archive/8d5363cb109cc6363661a1d5813e0b96787c4411.tar.gz#/statsd-8d5363c.tar.gz in the Fedora packages.

@deniszh
Copy link
Member

deniszh commented Nov 6, 2018

Yes, what @piotr1212 said. We can use newer NodeJS with a patch then.

@deniszh
Copy link
Member

deniszh commented Nov 6, 2018

But much appreciated, @bebehei , I had this idea to do the same, but I'm a bit busy recently. Thanks!

@bebehei
Copy link
Contributor Author

bebehei commented Nov 7, 2018

Ok, statsd seems to be running now properly with the given NodeJS version. Thanks @piotr1212

This script shall only run during initialisation
This improves build time, as the base image already contains most of the
packages for the dev installation
Even though statsd didn't get updated for a long time, the patches
on current master should suffice to make it run on a current NodeJS.
This is absolutely not necessary. If someone really wants to edit and
save the config, mount the file.
HERE documents have got the ability to remove leading tabs, when used
with a trailing dash (<<-). When using spaces, the indentation wouldn't
be possible.

http://pubs.opengroup.org/onlinepubs/009695399/utilities/xcu_chap02.html#tag_02_07_04
This allows faster startup time and uses paralell initialisation.
@bebehei
Copy link
Contributor Author

bebehei commented Nov 8, 2018

Well, graphite web and graphite do work properly.

Redis, statsd and collectd do start, too. But I'm not sure, if they're still properly configured. I don't use them, so I can't judge, if they're still running properly.

But for the rest, it seems to work flawlessly.

@bebehei bebehei changed the title [WIP] Smaller baseimage with Alpine Smaller baseimage with Alpine Nov 8, 2018
@bebehei
Copy link
Contributor Author

bebehei commented Nov 8, 2018

@deniszh I'd love to get a review for this code. I know it's a lot. Take your time. But at the current point, I myself consider my work on this PR as "finished" (of course except for fixes of upcoming comments).

@deniszh
Copy link
Member

deniszh commented Nov 8, 2018

Well, I already did one comment - could you please check?

@bebehei
Copy link
Contributor Author

bebehei commented Nov 8, 2018

@deniszh Where? I don't see anything. May it be, you started a review, but didn't submit it fully?

@deniszh
Copy link
Member

deniszh commented Nov 8, 2018

Ah, nevermind, just found out that's not relevant. Plz give me some time to properly review it, especially in corner cases. Thanks a lot!

@deniszh
Copy link
Member

deniszh commented Nov 11, 2018

OK, change looks good, thanks, @bebehei !
Although, I'm hesitating to build 1.1.4 image with it because it's not really compatible with previous release.
So, I'm going to merge it to master branch for now and will release 1.1.5 version basing on this PR.

@deniszh deniszh merged commit bbb036e into graphite-project:master Nov 11, 2018
@bebehei bebehei mentioned this pull request Nov 11, 2018
@bebehei bebehei deleted the smaller-baseimage branch April 12, 2020 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants