Skip to content
This repository has been archived by the owner on Mar 27, 2023. It is now read-only.

Wait for database if run on slow machine #10

Merged
merged 6 commits into from
Jun 13, 2020

Conversation

Baschdl
Copy link
Contributor

@Baschdl Baschdl commented May 30, 2020

Depends on #7

@kevihiiin It currently prints the error from the docker command we use to check the status. I tried to prevent the output but wasn't very successful. Do you have an idea for this?

@Baschdl Baschdl requested a review from kevihiiin May 30, 2020 22:03
@kevihiiin
Copy link
Member

kevihiiin commented May 31, 2020

I would also have tried to check whether we can establish a connection to the database container...

Did we try healtchecks in the docker-compose before?
And then use a condition in the depends_on on the backend side?
https://stackoverflow.com/questions/35069027/docker-wait-for-postgresql-to-be-running


Edit:
Conditions inside depends_on is not supported in version 3+
This is their official workaround:
https://docs.docker.com/compose/startup-order/

So basically we can do this

@Baschdl
Copy link
Contributor Author

Baschdl commented Jun 5, 2020

I just learned recently about health checks but from my current understanding an unhealthy container on startup wouldn't trigger that depends on container would wait for a healthy state

@Baschdl
Copy link
Contributor Author

Baschdl commented Jun 5, 2020

Oh, I didn't read all your comments. Our current solution is basically equivalent to https://stackoverflow.com/a/61248971/3342058

@Baschdl Baschdl changed the base branch from create-whole-new-experience-yeah to staging June 5, 2020 20:16
@Baschdl
Copy link
Contributor Author

Baschdl commented Jun 5, 2020

@kevihiiin's changes look good 😄

Copy link
Member

@kevihiiin kevihiiin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I approve 😄

Copy link
Contributor

@maltezacharias maltezacharias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quite elegant solution. I think we should remove the build process from the container image itself, as we should do that anyway for javascript as it makes no sense to have node available in the main image. Will create another issue for that

@maltezacharias maltezacharias merged commit 88edaa8 into staging Jun 13, 2020
@maltezacharias maltezacharias deleted the mercy-with-slow-machines branch June 13, 2020 19:29
@Baschdl
Copy link
Contributor Author

Baschdl commented Jun 14, 2020

@maltezacharias What is the build process for you? We're basically just installing ubuntu and python dependencies

@maltezacharias
Copy link
Contributor

For me that would also include the following:

django-admin makemessages --no-location 
backend django-admin compilemessages
python3 manage.py collectstatic --no-input

Is something that only needs to be run when a container is build and never again after.

@Baschdl
Copy link
Contributor Author

Baschdl commented Jun 14, 2020

The build process for me is everything which happens when I run docker build. These commands are part of the deployment (deploy.sh) and in my understanding have their place there. We have to run them every time when there is new code but the container is not necessarily built each time. We could include them in the docker build process (in the Dockerfile) but I don't think that's your idea with "remove the build process from the container image itself".

@maltezacharias
Copy link
Contributor

Yes, I meant exactly that, I would move

django-admin makemessages --no-location 
backend django-admin compilemessages
python3 manage.py collectstatic --no-input

to the Dockerfile.

The end-result would be that deploying would be completed by docker-compose -f docker-compose.dev.yml -f docker-compose.prod.yml up -d --build. So far we always run deploy.sh so it would make sense to have this be part of the container start-up. OTOH in a load-balancing scenario this would introduce other problems when multiple containers try to migrate at the same time.

We have to run them every time when there is new code but the container is not necessarily built each time.
I think for easier usability we should be moving towards an approach that facilitates running the app using docker pull on the server which would probably not include a bind mount for the application code. And so this code really only would be needed during a build. Obviously we should still document this, if someone decides to tinker around with the code on a production system but IMHO we should streamline a m4e deployment towards

  1. create env files
  2. create a configuration file for initial configuration (if not solved with a web ui)
  3. create a docker-compose file
  4. run docker-compose up

I would also suggest to modify ENTRYPOINT to the remaining deploy.sh script, so that it would automatically be invoked when we run the container.

@Baschdl
Copy link
Contributor Author

Baschdl commented Jun 15, 2020

The ENTRYPOINT is a good idea. The remaining steps would only be the migration and running the checks.

The idea behind the bind mount is that we can develop with the dev setup. Would you remove this option?

@maltezacharias
Copy link
Contributor

No, I think that's very clever and we should keep that possibility. I've been experimenting a bit and I think our best bet is a combination of docker-compose.yml + docker-compose.override.yml + docker-compose.prod.yml. (The crux is when running docker-compose -f 1 -f 2 you can only add to but not remove definitions from 1. But when an override file is present, this will automatically extend docker-compose.yml on docker-compose up/build/...

That way we can add bind mounts to the override file, so they would work in dev and replace them with volumes in prod. I had a chat with @kevihiiin today as well and we also think that we should keep some of the bind mounts like logs and backup, but let's talk about that next weekend maybe?

I will prepare an example during the week on the new repo and then we can discuss it.

@Baschdl
Copy link
Contributor Author

Baschdl commented Jun 15, 2020

A first idea with the entrypoint is here: #21

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