search

DiUS / pact_broker-docker

'Dockerised' pact broker
http://pact.io
MIT License
76 stars 102 forks source link

New Dockerfile with Alpine, Puma and User-mode #90

Closed tavogel closed 5 years ago

tavogel commented 5 years ago

Extension of https://github.com/DiUS/pact_broker-docker/pull/87, uses Puma and Alpine instead of Passenger and Ubuntu.

Image is 246mb now, down from 507MB.

bethesque commented 5 years ago

I did some googling on puma, and came across this interesting thread which is relevant to us because we're using Sequel and Postgres https://github.com/puma/puma/issues/1254

Because the broker is rarely under high load (it's usually a drip feed from CI) I don't think it's going to be an issue, but I thought I'd leave it here for reference in case it ever became relevant.

bethesque commented 5 years ago

@k-ong I'm fine with this. If you wouldn't mind just giving it a quick sanity test, I'm ok to accept it.

k-ong commented 5 years ago

looks great @tavogel !! only comment I have would be on the "/pact_broker" directory permissions. Having the COPY set the ownership to the "ruby" user will be good; so that everything under the directory is owned by the "ruby" user.

tavogel commented 5 years ago

Hey @k-ong, I intentionally do not do that, so that the code cannot be modified at runtime (e.g. by a malicious script injection). The important part is that everything in the folder is readable to the root group... the ruby user is part of the root group and can read what it needs. Likewise in platforms like OpenShift that will randomize the UID, they will still be able to access the folder via GID=0.

k-ong commented 5 years ago

Oh, nice to know the intention behind that. :) One thing that we should probably do down the track is to support read-only filesystem on docker run. At the moment, puma creates the log directory on start which will error out and fail. Not urgent now, just a nice to have.

great work on this PR @tavogel !

tavogel commented 5 years ago

The /pact_broker folder should be group writeable so it can create the logging folder. But all logs should go to stdout. Maybe there is a way to not create this folder at all...

bethesque commented 5 years ago

Ok, I've fixed the loggers so they don't try to create files. All goes to stdout now.

We've also solved the backwards compatibility problem by moving the new image to https://hub.docker.com/r/pactfoundation/pact-broker/tags with the rest of the pactfoundation images.

It looks like this PR is not merged, but it's because it's merged in the new repository here: https://github.com/pact-foundation/pact-broker-docker

I've released it with the tag edge.

@YOU54F you might be interested in this too.

bethesque commented 5 years ago

Once you've run it for bit with no issues @tavogel, we'll release latest

YOU54F commented 5 years ago

Fab, thanks for all the hard work @tavogel and @bethesque

Will start trialling this out and pass back any feedback!

bethesque commented 5 years ago

+1 thanks for your work @tavogel!

tavogel commented 5 years ago

@bethesque @mefellows FYI we have been running the new pact-foundation edge image in thousands of pipelines for the last month. Everything works perfectly! As a consumer of this new package, I would be in favour of moving forward with the latest tag, and deprecating the DiUS image.

In Kubernetes, we use the following security context:

          securityContext:
            runAsNonRoot: true
            runAsUser: 1234 # any number will work

We did try to use the image with readOnlyRootFilesystem: true, but there were a couple of issues with that... the application still needs to write to /tmp (which can be resolved with an emptyDir volume), but also /pact-broker (which cannot) at runtime. It would be interesting to fine tune this, but I don't think it is a deal breaker. For sure having the user-mode access is by far more important.

bethesque commented 5 years ago

That's great. I'll put out an official tag then. I should be able to work out the read only file system if it becomes an issue.

bethesque commented 5 years ago

Done! There's a latest and a 2.32.0-1 tag. Thanks so much for you work on this Tom. We really appreciate it.

bethesque commented 5 years ago

@tavogel I'm reading this article https://github.com/phusion/passenger/wiki/Puma-vs-Phusion-Passenger#jungle-sysv-init-version-vs-phusion-passenger

Phusion Passenger restarts all crashed processes. Phusion Passenger even restarts itself if it crashes, thanks to its watchdog architecture.

There is no such equivalent for Puma. If it crashes, you have to go and restart it.

I will be adding this section to the readme of the new image:

Which one should I use?

Please read https://github.com/phusion/passenger/wiki/Puma-vs-Phusion-Passenger for information on which server will suit your needs best. The tl;dr is that if you want to run the docker image in a managed architecture which will make your application highly available (eg. ECS, Kubernetes) then use the pactfoundation/pact-broker. Puma will not restart itself if it crashes, so you will need external monitoring to ensure the Pact Broker stays available.

If you want to run the container as a standalone instance, then the dius/pact-broker image which uses Phusion Passenger will server you better, as Passenger will restart any crashed processes.

Thoughts? We're not using Jungle currently, but perhaps we should add it.

mefellows commented 5 years ago

My 2c.

Philosophically, I prefer the Puma model (a crash-based architecture). The more layers of indirection a system has the harder it is to reason about, operate, monitor etc.

Consider running an application like Phusion on Docker on an EC2 instance within an ECS/Kube/... cluster (I bet you can 😆).

You now need to monitor the multiple processes within the container as well as those outside of it. But how to behave under all conditions? Phusion is now meddling around to try and keep things stable. Meanwhile, you have the container runtime inspecting the image for issues and reporting that to the orchestration tool (e.g. ECS/Kube). How do you then decide that the container is behaving badly and if so, how should you respond? How do you know when we need to scale out if Phusion is trying to do this also.

In a crash based architecture, the container is immediately unavailable and the orchestration tool knows what to do - schedule another one. Simpler strategies can be used to scale out - memory and CPU etc. You still have redundancy via more containers, and can use much simpler health checks to determine if a container is misbehaving.

bethesque commented 5 years ago

Yeah, I'm cool with the Puma model too, but I feel we need to give the users the information to make the best decision for what they're wanting to run. There are places with very little technical know-how who won't be comfortable setting up a highly available cluster with external monitoring. Getting a single container running on a hand made instance might be all they can manage, in which case, the phusion image would probably be better for them.

mefellows commented 5 years ago

Yes, sorry didn't mean to ignore that question - all valid points!

tavogel commented 5 years ago

Even if all you can muster is running a single container on a hand provisioned box, docker has the --restart option: https://docs.docker.com/engine/reference/run/#restart-policies---restart

Their opinion really only makes sense for running Ruby directly on a VM. But in the context of containers or "12 factor apps", what they are doing is counterintuitive; in the name of "simplicity" they actually make the problem far more complex (as @mefellows pointed out).