-
Notifications
You must be signed in to change notification settings - Fork 72
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
Improve the dockerfile #45
Conversation
Signed-off-by: Bilal.M.K <[email protected]>
Signed-off-by: Bilal.M.K <[email protected]>
Signed-off-by: Bilal.M.K <[email protected]>
@ksatchit Could you please check and approve the pull request |
@bilal-mk , the litmus tests with updated docker images are in progress. @shashank855 will approve the PR once done! |
Just looking at the sysbench documentation, looks like building w/ the --without-mysql flag causes database tests to not work, which incidentally is needed for load generation tests in litmus (we use oltp.lua). @shashank855 , can you confirm ? |
I think mongo-client does not require mysql support. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bilal-mk If no database drivers are available database-related scripts will not work with --without-mysql.
mongo-client/Dockerfile
Outdated
|
||
RUN cp /usr/include/libbson-1.0/* /usr/include/ \ | ||
&& cp /usr/include/libmongoc-1.0/* /usr/include/ | ||
|
||
ADD sysbench-mongo/sysbench /sysbench | ||
WORKDIR /sysbench | ||
RUN ./autogen.sh && ./configure && make | ||
RUN ./autogen.sh && ./configure --without-mysql && make |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bilal-mk Can you remove --without-mysql
becouse its throwing error.
|
Signed-off-by: Bilal.M.K <[email protected]>
FROM alpine | ||
|
||
LABEL maintainer="OpenEBS" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bilal-mk In our liveness script we used bash commands and Alpine docker image doesn't have bash installed by default. You will need to add following commands to get bash:
RUN apk update && apk add bash
, Otherwise it will throw an error.
Signed-off-by: Bilal.M.K <[email protected]>
mongo-client/Dockerfile
Outdated
libssl-dev \ | ||
libmongoc-dev \ | ||
libbson-dev | ||
RUN apt-get update && apt-get -y --force-yes install --no-install-recommends \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bilal-mk We figued out that --no-install-recommends does not allows to install some other dependency related to sysbench.
mongo-client/Dockerfile
Outdated
libmongoc-dev \ | ||
libbson-dev | ||
RUN apt-get update && apt-get -y --force-yes install --no-install-recommends \ | ||
python3 \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bilal-mk As we are making this docker file optmised pyhon 3 & --upgrade setup tools may not be need. Your changes on these commented lines will highy apreciated.
mongo-client/Dockerfile
Outdated
python3 \ | ||
python \ | ||
python-pip | ||
|
||
RUN pip install --upgrade setuptools |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bilal-mk Can we bring these seperate RUN command into a single RUN ?
Signed-off-by: Bilal.M.K <[email protected]>
Signed-off-by: Bilal.M.K <[email protected]>
Signed-off-by: Bilal.M.K <[email protected]>
@@ -1,35 +1,29 @@ | |||
FROM ubuntu:16.04 | |||
|
|||
RUN apt-get clean \ | |||
RUN apt-get update && apt-get -y --force-yes install \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes @bilal-mk. This changes seems to be good but we tested this Docker file in our own environment and it doesn't work for the purpose. Can you kindly push the changes with the following Docker spec:
FROM ubuntu:16.04
RUN apt-get update -y || true
RUN apt-get install -y --force-yes --no-install-recommends \
python \
python-pip \
make \
automake \
libmysqlclient-dev \
libtool \
libsasl2-dev \
libssl-dev \
libmongoc-dev \
libbson-dev
RUN apt-get clean \
&& rm -rf /var/lib/apt/lists/* \
&& cp /usr/include/libbson-1.0/* /usr/include/ \
&& cp /usr/include/libmongoc-1.0/* /usr/include/
# && pip install --upgrade pip
RUN pip install --upgrade setuptools
RUN python -m pip install pystrich \
&& python -m pip install pymongo
ADD sysbench-mongo/sysbench /sysbench
WORKDIR /sysbench
RUN ./autogen.sh && ./configure && make
# components for liveness script
ADD liveness/server.py ./
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same is this pull request with reduced the few-layer as part of the Dockerfile best practice. Please share how to properly test this or add those test as part of travis integration test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed @bilal-mk. Have raised #49 & #50 to track this.
Currently travis performs a build check alone for all the images and doesn't look for existence of packages/artifacts specified in the Dockerfile (saw a few commits pushed as "fixes" for those that broke the build), which is what has necessitated certain manual tests. One way of testing this is to look at the python/bash scripts being pushed into the image & verifying the presence of the binaries &/or auxiliary files used in it in the locally built image. Or maybe simpler, compare the existing docker image layers w/ the one built with changes (docker diff, docker export - or even simply entering the fs)
Amongst the multiple docker images maintained in this repo & litmus - hacktoberfest has helped us refactor the images & its CI to the "extent possible", i.e., w/o breaking prod deployments where they are being used as K8s resources. Of course, further optimizations are possible by refactoring the scripts used in them (which came about before containerizing them!) - but these will be taken up based on priority.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dockerfile actually looks good. However, In this particular case, the following seem to sporadically cause a skipped install of some packages (though this shouldn't ideally impact us) :
- the order of execution of apt-get clean/removal of repo refs in apt/lists
- combining installation of python pip & pip-install of packages into same command.
- setuptools dependency (typically this is needed only if easy_install is used)
Having said this, it may also depend on the build environment where the build is happening (its n/w connections, proxy settings etc.,).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The travis build has passed(https://api.travis-ci.org/v3/job/443697398/log.txt) & I have tested this on a GCP compute instance (you can guarantee a clean build env here!). So, we should be good to go.
We will revisit this for further optimizations
mysql-client/Dockerfile
Outdated
|
||
LABEL maintainer="OpenEBS" | ||
|
||
RUN apk add --no-cache mysql-client && apk add --no-cache jq && apk add --no-cache bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bilal-mk I think that adding --no-cache bash will not support all the bash command. What we can do is either installing moreutils package or we can use the base image as ubuntu only. Please accomodate this change it will highy appreciated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added moreutils package
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shashank855 moerutils is necessitated on account of usage of ts
in the script. --no-cache shouldn't matter IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few comments.
PTAL
Signed-off-by: Bilal.M.K <[email protected]>
Thanks for your tremendous patience with this @bilal-mk ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Signed-off-by: Bilal.M.K [email protected]
Fixes #41
Fixes #42