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

Dockerfile and Base Path changes #102

Merged
merged 9 commits into from
May 1, 2021
Merged

Dockerfile and Base Path changes #102

merged 9 commits into from
May 1, 2021

Conversation

jgregmac
Copy link
Contributor

Thanks for creating this fork! Our org is dependent on ElastAlert at least until Kibana Alerts mature quite a bit more.

I am afraid I can't quite follow your contribution guidelines. The link to https://github.com/jertel/elastalert2/releases/new seems to be broken, and I can't figure out how to execute the workflows that you are requesting. There also is not an "alt" branch available at the present time. Sorry!

The purposes of this PR are twofold:

  1. To change the Dockerfile base image to python/slim-buster in the interest of speeding builds (alpine has no wheels!). I also reduced the number of image layers, removed tmp files and dev packages from the final image, and converted the image to run as a non-root user.
  2. To set the Dockerfile/Helm chart base directory to /opt/elastalert, and the config file to /opt/elastalert/config.yaml. This is in the interest of following path conventions from the original ElastAlert project. Doing so allows elastalert binaries to run without the need to specify the --config parameter each time you execute them. It is really helpful when testing/debugging new rules from within a running pod!

@jertel
Copy link
Owner

jertel commented Apr 29, 2021

Thanks for your contribution. Can you change the destination branch to master? Once it's pointing to master it will then automatically kick off the automated CI routines for you, and should email you if something fails.

As far as the contributing guidelines, I don't see any references to an "alt" branch. I thought I had cleaned all those up last week. Can you paste a link? Also, the release link is only accessible to maintainers. Contributors don't need to deal with releases.

@ferozsalam
Copy link
Collaborator

@jgregmac Could you also update the base image change to a breaking change in the Changelog please? This will affect anyone downstream who has built their own images using the image in this repo as a base. I imagine some will need to refactor their images before taking the release.

@jgregmac jgregmac changed the base branch from fix/jinja_deepcopy to master April 30, 2021 00:10
jgregmac and others added 2 commits April 29, 2021 20:15
- Dockerfile base image marked as breaking change.
@jgregmac
Copy link
Contributor Author

Okay, I have retargeted this PR to the master branch, and I moved the Dockerfile base image change from "Other changes" to "Breaking changes".

The alt branch reference was not in the CONTRIBUTING.md, but rather here, in the repository discussions:
#11

@jertel
Copy link
Owner

jertel commented Apr 30, 2021

Okay, I have retargeted this PR to the master branch, and I moved the Dockerfile base image change from "Other changes" to "Breaking changes".

Thanks for updating that. Will you please revert the version bumps? This change would likely require a minor version bump instead of a patch version increment. I will need some time to review these changes more in-depth because this will affect a lot of people, as well as other projects I maintain. I need to compare image size, and other factors compared to the existing release. When it comes time to releasing the next version we'll take care of choosing an appropriate next version label.

The alt branch reference was not in the CONTRIBUTING.md, but rather here, in the repository discussions:
#11

Ok, I see it and have updated it to point to the new CONTRIBUTING.md file. Thanks for pointing it out.

@jgregmac
Copy link
Contributor Author

Okay, I have reverted the version bumps and marked this as release 2.x.x in the CHANGELOG.md

@jertel
Copy link
Owner

jertel commented Apr 30, 2021

I ran some comparison tests against the existing Dockerfile and the new one:

Existing:
Size ~ 450MB
Time ~ 3m 50s

New:
Size ~ 700MB
Time ~ 1m 20s

The build speed certainly improved but the size grew by almost 250MB. The size increase will be a problem for one of my projects so I'd like to figure out some options for getting this image size back down. I tried apt-get clean and removal of the /var/lib/apt/lists/ contents but that was negligible.

@jgregmac
Copy link
Contributor Author

We already have package cache cleanup in the Dockerfile:

apt-get -y autoremove &&\
rm -rf /var/lib/apt/lists/* &&\

The bulk of the additional image size is from the system packages that were installed, mirroring equivalent commands ported over from the alpine-based build. The "cargo" package is especially large, and I cannot see why it should be present.

I tried running the image without these packages, and Elastalert functions as expected in my environment. However, I only a using email webhook alerts, so I am not testing all possible scenarios. However, I don't really see why these packages were needed. The only Python package in the project that seems to have a system dependency is "tzlocal", and the "tzdata" package that it needs is part of the base image!

Removing these packages reduces the image size to a reasonable 244Mb.

If you want curl and jq present for diagnostics, it would be possible to add a third build stage "as debug", relabeling the current final state "as prod". You then could run separate "docker build" jobs for production and debug versions of the container image.

@jertel
Copy link
Owner

jertel commented Apr 30, 2021

Nice work! The cargo was needed only for alpine due to some recent Python changes. I don't recall why libmagic was needed. I don't see any use of the python-libmagic import but maybe it's used in some other capacity for mime-type detection of ES documents? The jq and curl were added by another user in a PR, and they only add 5MB so I don't mind keeping those in for that small increase, and avoid the extra docker image.

For those following along, now the numbers look like this:

Existing:
Size ~ 450MB
Time ~ 3m 50s

New:
Size ~ 250MB
Time ~ 45s

@jertel jertel merged commit 02b8393 into jertel:master May 1, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants