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

Add Bonita BPM Official Docker image #852

Merged
merged 11 commits into from Oct 14, 2015

Conversation

JeremJR
Copy link
Contributor

@JeremJR JeremJR commented Jun 29, 2015

Hello,

I add a bit of context here, my name is Jérémy and I work as a Sysadmin for Bonitasoft.
Bonitasoft is a software editor which supports the development of Bonita BPM, an open-source (available on GitHub) business process management and workflow suite.

We are really interested by Docker and would like to let our users benefit from this technology with an Official image.

Kind regards,
Jérémy

@psftw
Copy link
Contributor

psftw commented Jul 9, 2015

Sorry for the delay getting back to you on this. Overall this look good and it is clear you've done a thorough job preparing it. I do have a few nitpicks, though: The container doesn't shutdown gracefully with docker stop as I would expect. This may be due to using "sudo" to run as the "bonita" user. There are a lot of variables in the Dockerfile and files/config.sh that could be removed/simplified: BASE_URL and BONITA_ARCHIVE_DIR/BONITA_ARCHIVE_FILE come to mind. The use of ADD in the Dockerfile to download remote files should be changed to curl/wget. The DEBIAN_FRONTEND environment variable is likely not needed, and if it is, it could be added strategically to RUN command with apt-get. The MySQL and Postgresql downloads could be verified, particularly the MYSQL connector which is transferred without encryption. Whenever I see chown -R I get suspicious -- is there a reason this needs to be done in the startup script each time?

[less nitpick and more meta]: Is there a good reason to base on ubuntu:14.04 and ship your bundle with an older version of tomcat vs. building on the latest tomcat (or maybe java) image? I'm agnostic on the approach so long as the image works well, but I'm curious as to the reasoning. In general, the files/config.sh script is alarming to me in that it's doing all kinds of preparation work, perhaps some of which could be done once during initial startup maybe? I haven't dug into the details on that one, so just speculating based on my gut. I'll add some comments to the docs PR tomorrow.

…nd PostgreSQL downloads. Fix container shutdown
@JeremJR
Copy link
Contributor Author

JeremJR commented Jul 16, 2015

Thanks a lot for your remarks. I will try to respond to all of your points.

I've fixed the shutdown by using exec so the script executed using sudo receive well the signal now.

I have tried to reduce the number of variables.

The last ADD is replaced by a wget. MySQL and Postgresql downloads are now verified.

DEBIAN_FRONTEND is here to avoid some debconf red messages (debconf: unable to initialize frontend: Dialog...) that could be disturbing for some users. I've followed your advise and moved it inside the RUN command with apt-get.

I have changed a little the way to manage ownership but I still have issues with data volume owned by root. I hope it's ok for you. I also noticed that MySQL and PostgreSQL containers use a chown -R. If it's not sufficient, I will be glad to see some examples of best practices regarding that.

One reason to base the image on ubuntu:14.04 and use our Tomcat Bundle is that it's in our Supported Environments Matrix (and currently it's our recommended stack). Another point is that an image based on java:7-jre is heavier (483.8 MB) than an image based on ubuntu:14.04 (391 MB).

I come back to you ASAP regarding the remaining points on the doc PR.

@psftw
Copy link
Contributor

psftw commented Jul 17, 2015

Thanks for the improvements. You'll want to make this change to take out the extra chmod -R that was added:

 # create user to launch Bonita BPM as non-root
  RUN groupadd -r bonita -g 1000 \
-  && useradd -u 1000 -r -g bonita -d /opt/bonita/ -s /sbin/nologin -c "Bonita User" bonita \
-  && chown -R bonita:bonita /opt/files /opt/templates
+  && useradd -u 1000 -r -g bonita -d /opt/bonita/ -s /sbin/nologin -c "Bonita User" bonita

Due to the way that the builder works, it will duplicate modified files into a new layer, so this winds up adding a bunch of useless data to the layer created by that RUN command.

$ docker images bonita
REPOSITORY          TAG                  IMAGE ID            CREATED             VIRTUAL SIZE
bonita              latest-revertchmod   7935ca6b93c6        43 minutes ago      390.9 MB
bonita              7.0.0                f49a6b53b234        About an hour ago   463.2 MB
bonita              latest               f49a6b53b234        About an hour ago   463.2 MB
$ docker history bonita:latest | grep groupadd
a4b63c30cb91        About an hour ago   /bin/sh -c groupadd -r bonita -g 1000   && us   72.66 MB            
$ docker history bonita:latest-revertchmod | grep groupadd
48d89e73fbd2        40 minutes ago      /bin/sh -c groupadd -r bonita -g 1000   && us   329.3 kB            

Outside of the chmod change, there is also now a 7.0.1 release, so it needs a version bump. These are the only two remaining barriers in my mind.

@JeremJR
Copy link
Contributor Author

JeremJR commented Jul 17, 2015

Thank you, I've removed the chmod on both 7.0.0 and 7.0.1.

@psftw
Copy link
Contributor

psftw commented Jul 29, 2015

I don't think 7.0.0 should be included since 7.0.1 deprecates it, unless you plan to follow-up with a PR to remove 7.0.0 and only backport it for historical reasons. Typically, only the latest releases in each supported series should be included in the library definition file.

@psftw
Copy link
Contributor

psftw commented Jul 29, 2015

I noticed that your repo has been reorganized a bit. Be careful to use "git mv" as much as possible to retain history when you do that kind of thing. I compared the previous commit I looked at to the latest one out of band and saw a few other subtle improvements which look good. Let's make sure that the docs discuss the new /opt/custom-init.d/ functionality :-)

@md5
Copy link
Contributor

md5 commented Jul 29, 2015

@psftw There's nothing special about git mv. Moving a file with a normal mv, followed by the appropriate git rm and git add will result in the exact same history when git commit is called. Move tracking in Git (and on GitHub) is based solely on file similarity (cf. http://git-scm.com/docs/gitdiffcore#_diffcore_rename_for_detection_renames_and_copies).

@md5
Copy link
Contributor

md5 commented Jul 29, 2015

@JeremJR
Copy link
Contributor Author

JeremJR commented Jul 29, 2015

@psftw Yes, if it's possible, I would like to push 7.0.0 also to keep it in the historic then I will create another PR to remove it from the list. Using that, I think that it can help to track and reproduce issues on all releases and find out which one introduce it.
Regarding the new directory '/opt/custom-init.d/' I've already added a section "How to extend this image" which discuss about it :) but let me know if it's not enough.

@psftw
Copy link
Contributor

psftw commented Jul 29, 2015

@md5 yup, sorry to introduce noise on that non-issue! @JeremJR I'm happy with where the image is at and will take another look at the docs tomorrow. Thanks!

@JeremJR
Copy link
Contributor Author

JeremJR commented Jul 30, 2015

To be more coherent with existing official images (folders named MAJOR.MINOR), I've done another move into a folder "7.0".
I will create in the future the folders 7.1, 7.2 ...
In the mean time, as in the Jenkins official image, I have tagged "7.0.0" and "7.0.1". But I still use git-commit-id into the library definition file. It was a bit messy at the beginning but I think it will be easier to follow up now.

@JeremJR
Copy link
Contributor Author

JeremJR commented Aug 18, 2015

Hello,

Any news for me regarding this PR?
Is there anything that needs fixing?

Kind regards,
Jérémy

@tianon
Copy link
Member

tianon commented Aug 19, 2015

I'm a bit confused/worried about the size of the scripts in scripts (and with how much boilerplate is in templates). Why isn't more of this boilerplate in Bonita itself? Would a user deploying Bonita on their own server need to set up all this same boilerplate too?

@JeremJR
Copy link
Contributor Author

JeremJR commented Aug 20, 2015

Thank you for taking a look on this PR.

If you are referring to the many lines commented into templates/bonita-platform-community-custom.properties and templates/bonita-tenant-community-custom.properties
it's due to the fact that I made the templates directly from the original files, from the Bonita Tomcat bundle.
I thought that it would be more clear, so a user can see the default values of all parameters that can be customized by extending this image.
But if it's counterproductive, and you think it preferable that I update the image to create files with only the values managed by this container, I can do that.
Regarding what a user deploying Bonita should do, if they want to install their own server and configure a different type of database, and secure Bonita, they will have to perform the same kind of modifications.
FYI, this is the official documentation to follow if a user wants to do the same:
http://documentation.bonitasoft.com/tomcat-bundle-2
http://documentation.bonitasoft.com/database-configuration-2
http://documentation.bonitasoft.com/first-steps-after-setup-1
http://documentation.bonitasoft.com/rest-api-authorization-0
My aim with this image is to improve the user experience by providing an easier way to deploy Bonita with MySQL or PostgreSQL.
I tried to keep it as simple as possible by managing only the parameters related to database configuration and security. Then as recommended by psftw, I provide a way to extend this easily using a directory to store some custom scripts.

@JeremJR
Copy link
Contributor Author

JeremJR commented Sep 7, 2015

I hope that we can find a way to meet Docker requirements in order that bonita image could integrate the Docker library. So we could ensure to follow the best practices to satisfy both Docker, and our community.
In the mean time I have published the Bonita BPM image on the registry through https://hub.docker.com/r/bonitasoft/bonita/
Because we think that we need to answer our community at this state. It will also permit to have some feedbacks to improve it.

@tianon
Copy link
Member

tianon commented Sep 9, 2015

it's due to the fact that I made the templates directly from the original files, from the Bonita Tomcat bundle

Yeah, this is definitely what I was referring to. My largest concerns about that are two-fold. The first part is that the onus will now be on you to ensure that you double-check that the default configuration of Bonita hasn't changed each release (and incorporate the necessary changes each time), which makes your Docker maintenance burden slightly larger, as opposed to only including what's necessary to be changed for Bonita to work within Docker (if anything?) and thus making version bumps fairly trivial. The second is that the changes to these files are also then included in our review, which means slightly more content for us to wade through in order to review image updates. Does that make more sense?

Users who want to customize Bonita ought to read Bonita's documentation instead of trying to understand it all just by reading the Dockerization source code, right?

@JeremJR
Copy link
Contributor Author

JeremJR commented Sep 10, 2015

Ok, you're right, I've made this simplification.

@yosifkit
Copy link
Member

I'm still a little worried that there is a lot hidden in startup.sh, config.sh, and functions.sh, but it looks to be mostly configuration to connect to a linked database. One thing to note, the exec sudo has issues when trying to send signals to the running process since the sudo process stays resident, an alternative is to use gosu.

I would recommend moving a few lines in your Dockerfile around to utiliize caching better:

  • swapping lines 22-23 and lines 42-46, COPY sometimes has caching issues, so it is better to move it to as late as possible
    • will probably need to also mkdir -p /opt/files/ since the COPY won't be there to automatically create it
  • move lines 5-10 below the apt-get lines and even below the moved useradd
    • this will help share layers between versions of bonita
  • DEBIAN_FRONTEND=noninteractive is not necessary since users will not see the build output

@yosifkit
Copy link
Member

yosifkit commented Oct 9, 2015

What do you think of my suggestions, @JeremJR? (sorry this has been such a long PR) @tianon do you have anything else that would hold this back from being merged?

@JeremJR
Copy link
Contributor Author

JeremJR commented Oct 13, 2015

Sorry for the delay.
Regarding the scripts yes, the main purpose is the configuration to connect to a linked database.
About using sudo I didn't notice any issue yet, could you please give me a problem example?
I've updated the Dockerfile to follow your recommendations.

@tianon
Copy link
Member

tianon commented Oct 13, 2015

https://docs.docker.com/articles/dockerfile_best-practices/#user has a bit about the evils of sudo:

You should avoid installing or using sudo since it has unpredictable TTY and signal-forwarding behavior that can cause more problems than it solves. If you absolutely need functionality similar to sudo (e.g., initializing the daemon as root but running it as non-root), you may be able to use “gosu”.

@JeremJR
Copy link
Contributor Author

JeremJR commented Oct 14, 2015

Ok thanks. I've replaced sudo by gosu.

@yosifkit
Copy link
Member

LGTM, Build test of #852; cc1eded (bonita):

$ url="https://raw.githubusercontent.com/docker-library/official-images/cc1eded2de6c500afa56a82056fcd3e5749f2d33/library/bonita"
$ bashbrew build "$url"
Cloning bonita (git://github.com/Bonitasoft-Community/docker_bonita) ...
Processing bonita:7.0.0 ...
Processing bonita:7.0.1 ...
Processing bonita:7.0.2 ...
Processing bonita:7.0.3 ...
Processing bonita:latest ...
$ bashbrew list --uniq "$url" | xargs test/run.sh
testing bonita:7.0.0
    'utc' [1/4]...passed
    'cve-2014--shellshock' [2/4]...passed
    'no-hard-coded-passwords' [3/4]...passed
    'override-cmd' [4/4]...passed
testing bonita:7.0.1
    'utc' [1/4]...passed
    'cve-2014--shellshock' [2/4]...passed
    'no-hard-coded-passwords' [3/4]...passed
    'override-cmd' [4/4]...passed
testing bonita:7.0.2
    'utc' [1/4]...passed
    'cve-2014--shellshock' [2/4]...passed
    'no-hard-coded-passwords' [3/4]...passed
    'override-cmd' [4/4]...passed
testing bonita:7.0.3
    'utc' [1/4]...passed
    'cve-2014--shellshock' [2/4]...passed
    'no-hard-coded-passwords' [3/4]...passed
    'override-cmd' [4/4]...passed
$ docker images bonita
REPOSITORY          TAG              IMAGE ID            CREATED             VIRTUAL SIZE
bonita              7.0.3            62e706735eae        5 minutes ago       393.8 MB
bonita              latest           62e706735eae        5 minutes ago       393.8 MB
bonita              7.0.2            acf509191a3f        6 minutes ago       393.8 MB
bonita              7.0.1            de54d8d3f30b        7 minutes ago       393.8 MB
bonita              7.0.0            ebf422a94f62        8 minutes ago       393.7 MB

$ # not the final image ids, but useful to see the size

@tianon
Copy link
Member

tianon commented Oct 14, 2015

LGTM

tianon added a commit that referenced this pull request Oct 14, 2015
Add Bonita BPM Official Docker image
@tianon tianon merged commit e9e0133 into docker-library:master Oct 14, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants