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

Better Docker support #252

Merged
merged 5 commits into from
Mar 21, 2019
Merged

Conversation

AnalogJ
Copy link
Collaborator

@AnalogJ AnalogJ commented Mar 17, 2019

Hey,

First of all, this project is awesome, and I appreciate all the hard work you've all put into it. Its been incredibly valuable for me.

I make heavy use of docker for managing applications on my server, though I've read in other issues that Docker is not how your team deploys Klaxon in production.

I've made a couple of tweaks to your docker-compose.yml & Dockerfile that greatly simplifies the startup process for any of your users using Docker. It's just docker-compose upnow.

Here's a list of changes I made:

  • Added a docker-centric service manager (s6)
  • Created a cron & klaxon service
  • cron service does bundle exec rake check:all every 15 minutes
  • Modified the docker-compose file such that it uses Docker healthchecks to ensure that the postgres DB is ready before starting the app container.
  • Fixed some issues with the "production" config when running without SSL behind a firewall.

I'd love to see some (or all) of these changes merged into mainline master, so I can delete my version.

In addition, I'd love to see an official automated build setup on Docker hub (I can help with that if you're not familiar).

…ce runner (cron + app in same container). Added healthcheck to docker-compose file, simplifying docker instructions. Moved database into docker-compose from overrides, since its required for application startup.
@AnalogJ
Copy link
Collaborator Author

AnalogJ commented Mar 20, 2019

Pinging recent committers: @tommeagher @GabeIsman

@GabeIsman
Copy link
Member

Hey @AnalogJ,

Sorry, missed this when you first submitted it. Thanks so much for taking a crack at improving our Docker support! As you have correctly surmised we don't use Docker and we're not well-equipped to support it. The Docker support that does exist is the result of enterprising contributors like yourself. So I'm inclined to just trust that you've thought about how this should work and merge it. From what I understand about Docker, it looks great!

My main question is this: If someone were already deploying Klaxon with Docker, would this change mess them up in some way? Or would the upgrade be smooth? Basically, how much do we need to communicate that the Docker setup is changing?

I'd be super happy to have a Klaxon image on Docker hub, and even happier if you'd be interesting in helping us get that set up!

Thanks so much for contributing!
Gabe

@AnalogJ
Copy link
Collaborator Author

AnalogJ commented Mar 21, 2019

Thanks for taking a look @GabeIsman !

Looking at how everything was configured prior to my PR, I don't really see any real conflicts for existing users of Docker. The only thing I'd be concerned about would be the rake check:all command. Since that wasn't built into the previous Docker container, existing users would have to come up with their own workaround to trigger the check. While it wont cause any catastrophic failures, the default check I've added to the image is every 15 mins, while other users might want it more/less frequently.


Adding an official docker build is pretty easy, the Docker hub docs are pretty good but they make it more complicated than it is, and they don't have any screenshots.

Basically I would just do the following:

  1. Go to https://hub.docker.com/
  2. Create a Docker account/Sign into a Docker account
  3. Create a new Organization called themarshallproject by clicking on Organizations at the top.
  4. Add any other contributors to the Organization.
  5. Click the Repository tab/heading, then Create Repository button.
  6. Under Build Settings click on the Github logo and allow Docker to integrate with Github (specifically ensure that themarshallproject repos are allowed)
  7. Once Github is associated with Docker, you can click the logo and select themarshallproject as the owner, then search for klaxon as the repo.
  8. Finally click the Create & Build button (the default values under advanced will work for this repo`.

If you want, you can add me as member of themarshallproject org and I can set this all up for you, however you'll need to make sure that I have permissions to add webhooks to the repo (thats how Docker hub knows that it needs to create a new image on every commit to the repo).
You can boot me out of the docker hub and Github orgs after, and it should all keep working.

@GabeIsman
Copy link
Member

Hey @AnalogJ,

If you wouldn't mind setting that up for us, that would be amazing. I've added you as a collaborator on the repo. Let me know if you need any more specific access. Thanks for much for pitching in to make Klaxon better!

I'm going to merge this PR now and hopefully cut a release later today!

Gabe

@GabeIsman GabeIsman merged commit cfc2e47 into themarshallproject:develop Mar 21, 2019
@AnalogJ
Copy link
Collaborator Author

AnalogJ commented Mar 21, 2019

Hey @GabeIsman
Thanks for adding me to the repo, but unfortunately that doesn't look like enough.

  1. I attempted to add yourself and @tommeagher to the Docker Hub organization I created, but neither of you have Docker accounts. You'll need to do that do become an Admin.

  2. The Docker hub integration seems to require Github Organization level access before I can even select the Klaxon repo.

Screen Shot 2019-03-21 at 9 08 03 AM

We have two options here:

  1. Add me to the organization, without adding me to your private repos
  2. After you create a Docker Hub account, we can associate you (or your Service Account) with the Docker Hub TheMarshallProject organization. (Settings -> themarshallproject org dropdown -> Linked Accounts)
    • Then I can use those credentials to configure a new repo.

Sorry, about that.

@GabeIsman
Copy link
Member

No worries, thanks for sorting through this with me. Turns out I did have a docker hub account (gabeisman is the username). Feel free to add me to the organization and I'll make sure the github org is linked to the docker hub org.

Thanks!

@AnalogJ
Copy link
Collaborator Author

AnalogJ commented Mar 21, 2019

@GabeIsman done, you're now an owner.

You'll need to go to https://cloud.docker.com/u/themarshallproject/settings and scroll down to the Linked Accounts section, then click Connect on the Github row.

Screen Shot 2019-03-21 at 2 32 48 PM

Once that's done, I should be able to continue wiring everything up.

@GabeIsman
Copy link
Member

Hey @AnalogJ ,

Sorry it took me a while to get back to this. I believe I've set everything up in Docker hub. Want to take a look and make sure it looks OK?

Thanks again for helping with this!

Gabe

@AnalogJ
Copy link
Collaborator Author

AnalogJ commented Mar 25, 2019

Looks good @GabeIsman !

master is mapped to the latest tag on Docker, I also added a mapping from the develop branch to the develop tag for the docker image.

Looks like the master branch is broken, but that's expected.

I triggered a build of the develop branch, which should finish successfully.

@rdmurphy
Copy link
Contributor

rdmurphy commented Apr 2, 2019

@AnalogJ @GabeIsman I only now saw this, but re: whether this would bust Docker installs if you were already doing it — it's not ideal for us. 😬 We already had a way to run the cron job. (We're using this within Kubernetes.) This means we no longer have control over the frequency, making this a deal breaker for depending on the Dockerfile here. 😞

It's at odds with Docker to have the cronjob running within the container — scheduling is typically the job of something outside the system (cron on the instance you're using Docker, or within a orchestration system, etc.) so a single container isn't responsible for everything. This now means that if you're in a scenario where you spin up multiple instances of Klaxon (which we were) you're also spinning up multiple cron jobs (one for each instance).

What's done is done, but I think it may have been better to handle this by documenting how to schedule tasks taking advantage of Docker instead of pushing it inside the container. We were already maintaining a fork and will continue to do so (and will just pull this stuff out), but I do think it's not ideal and kind of a surprise.

@GabeIsman
Copy link
Member

Hey @rdmurphy, sorry to hear this is at odds with your setup. I've never deployed a production system with Docker, so I'm totally unfamiliar with the best practices. If you feel like we've gone awry with this setup, I'm happy to reconsider, I just don't have the expertise myself. So I'm depending on all you Docker users to guide the Klaxon Docker support.

What are your thoughts @AnalogJ? Does it make sense to just document ways to schedule the task and remove the cron job? Maybe an environment variable could be added to disable the cron as a compromise?

I do think that having the cron job run automatically is probably friendlier for first time users. But the Heroku setup is geared to ease of use for first timers, so I don't necessarily think the Docker system needs to maintain the same goals. If someone prefers Docker they are probably much more prepared to schedule their own tasks.

@rdmurphy
Copy link
Contributor

rdmurphy commented Apr 2, 2019

I'll make it work either way! I totally understand the desire/need to try and make it as smooth for all kinds of users. 😅 I think @AnalogJ did a great job.

Some thoughts on how to manage it in a Docker world:

I think it's safe to not assume everyone is gonna run Klaxon in a massive managed system like Kubernetes, so whatever is the answer needs to help with the simpler use cases. Typically when I had access to the ability to run docker-compose in a box, I also had the ability to run cron jobs in it too. So what I'd recommend is documenting what cron job you'd need to set up in a box to pass commands to docker-compose or through docker run itself and lean into using the system. That way Docker is responsible for spinning up and down a container to just run the rake task. Then the image can remain lean and focused on just running the app. Could perhaps even include a shell script for folks to run to set it up in the common use case?

I think a flag would be acceptable and an okay compromise, but I'd probably want them for the migrate and create_admin calls too. I think what's most difficult about the addition of all the scripts to the Docker container is that it makes a lot of assumptions about your environment. For example, in the current state you cannot run the container without it attempting to run a migration. You may not always want that!

@AnalogJ
Copy link
Collaborator Author

AnalogJ commented Apr 3, 2019

While I don't completely agree with the premise that "Docker must run one process, and only one process", I do understand the concern about controlling the cron schedule.

It should be pretty easy to make the cron service optional. Having said that, if you don't want to manage cron outside of docker, but you want to change the frequency of the cron task, the /config is a Volume, and it contains a file /config/klaxon-crontab that can be modified and will not be overridden.

While my goal with this PR was to significantly decrease the friction for new users launching Klaxon, I knew that existing users were managing the cron task somehow, so some turbulence was expected.
I'm configuring Klaxon for my users via a Portainer template, so instructions for how to add a cron job for docker-compose doesn't quite cover my use-case.

I'll work on a PR to add individual environmental flags for the rake tasks and the cron service.

@rdmurphy
Copy link
Contributor

rdmurphy commented Apr 3, 2019

I'm not super strict about the "one process" thing either, but IMO this should have been done in a fork for the Portainer use-case, or as a separate image that comes with "all-in-one" support instead of changing the primary image to better suit a specific deployment platform and its limitations. It could even extend the primary image and happily co-exist with it. The flags are gonna add even more complexity here (and the current additions/escape hatches aren't documented at all).

Perhaps having the base image and an "all-in-one" image is something we could consider?

@chriszs
Copy link
Contributor

chriszs commented Apr 3, 2019

Hey there, our set up looks similar to Ryan's. We're running Klaxon on Kubernetes using this Helm chart and an external cron job definition. Running the check in a separate container preserves our ability to scale and improves reliability. We've got a forked image we've been building, this'll probably keep us on that. Though we'd love to use the official image in the future.

@AnalogJ AnalogJ mentioned this pull request Apr 8, 2019
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.

4 participants