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

S3 Persistent storage with Dockerfile nginx+uwsgi #13

Closed
wants to merge 72 commits into from
Closed

S3 Persistent storage with Dockerfile nginx+uwsgi #13

wants to merge 72 commits into from

Conversation

fluential
Copy link

@fluential fluential commented Jul 11, 2020

This implements

  • S3 persistent storage so its possible to safely start and stop container, it does restore on startup + backup after 30s delay, according to a schedule
  • uwsgi+nginx for much better performance

For yet better performance consider using ASGI frameworks instead of WSGI.
If you still need more +400% performance for this uwsgi+nginx flask app consider using this image https://github.com/tiangolo/meinheld-gunicorn-flask-docker 😎

@fluential
Copy link
Author

@brentvollebregt phew, I think this one is ready now, have a look

@fluential fluential mentioned this pull request Jul 11, 2020
@brentvollebregt
Copy link
Owner

I understand that there is currently an issue where the database will be lost when the container is removed however, I am not a huge fan of forcing everyone to use S3. I'm much more a fan of using something like a mount and users then build on top of that however they may need to.

A potential solution would be to make all database interactions use peewee which would then allow people to point at an external database. This method would also then not force people to use SQLite.

What's your opinion on this?

@fluential
Copy link
Author

fluential commented Jul 12, 2020

@brentvollebregt the S3 backup is optional in that scenario

  • you are not forcing anyone, if you don't specify the keys, backup would not do anything. If there is local db it would not overwrite from s3
  • there is a VOLUME option provided so you can just stick to it if that is your preference, check the README how to use it

So I would say its safe to merge?

The wasabi s3 compatible storage comes with 6 USD /month 2TB including, I use this a lot for running containers around the world with different providers but keep the state across.

My state is small enough that itso ok to pull it on startup. You can understand the difference between building web stack on a persistence docker volume storage of a platform/provider or just run emphemeral containers with out-of-bounds backup anywhere :)

With just using cron you could have a simple distributed sevice 😎

But to answer your question in using peewee, certainly somethign to consider but in my opinion you would need to have a solid caching layer to avoid remote db connections as much as possible and sync into remote state occasionally (same stands for SQLite). The other option is to consider something like jsonbox or similiar but same concepts stand really.

If you want to have fun I would suggest going for https://dqlite.io/ but otherwise peewee sounds good.

Dockerfile Outdated

COPY . /app
RUN pip install -r /var/www/requirements.txt
RUN chmod 755 /usr/local/bin/sq*
Copy link
Owner

Choose a reason for hiding this comment

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

Is this setting permissions for more than sqlitebackup.sh?

Dockerfile Outdated

EXPOSE 8080
RUN cd /usr/local/bin && wget https://raw.githubusercontent.com/fluential/docker-sqlite-to-s3/master/sqlite-to-s3.sh && chmod 755 sqlite*
Copy link
Owner

Choose a reason for hiding this comment

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

On README.md:106 I see you reference https://github.com/jacobtomlinson/docker-sqlite-to-s3 but then use a fork of it here? Since this fork is identical to the original repo, could we use the file from the repo referenced in the README?

Copy link
Author

Choose a reason for hiding this comment

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

Good catch, will fix

config.py Outdated
@@ -1,7 +1,7 @@
import os

# Location of database
DATABASE_FILE_PATH = os.path.abspath(os.getenv('DATABASE_FILE_PATH', 'data.db'))
DATABASE_FILE_PATH = os.path.abspath(os.getenv('DATABASE_FILE_PATH', 'data/data.db'))
Copy link
Owner

Choose a reason for hiding this comment

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

The default needs to be left as 'data.db' to not make any breaking changes. You can override the value using an environment variable.

Comment on lines 6 to 7
stdout_logfile=/dev/null
stderr_logfile=/dev/null
Copy link
Owner

Choose a reason for hiding this comment

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

I'm no expert on this, but wouldn't you want:

stderr_logfile=/dev/stderr
stdout_logfile=/dev/stdout

What's your reasoning for not keeping the output?

Copy link
Author

Choose a reason for hiding this comment

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

UWSGI server creates a lot of uneccesary noise which does not give any value, hence we tune that down

README.md Outdated
@@ -96,6 +96,93 @@ from server import app as application

> Alternatively these config values can be manually set in `config.py`.

## Persistent SQLite Storage to S3 & Docker
Copy link
Owner

Choose a reason for hiding this comment

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

Looks like some of the header numberings might be a bit off and it would make sense to put Persistent SQLite Storage to S3 & Docker under Docker maybe.

This file is getting pretty big, I might look at creating a /docs/ directory that we can relatively link to for more detailed documentation.

@brentvollebregt
Copy link
Owner

@brentvollebregt the S3 backup is optional in that scenario

  • you are not forcing anyone, if you don't specify the keys, backup would not do anything. If there is local db it would not overwrite from s3
  • there is a VOLUME option provided so you can just stick to it if that is your preference, check the README how to use it

I see now, in the README you have:

docker run --rm -ti --volumes-from hitcounter-data -p 80:80 -e AWS_PROFILE=wasabi -e AWS_ACCESS_KEY_ID="X" -e AWS_SECRET_ACCESS_KEY="Y" hitcounter

I was looking for something without AWS_PROFILE, AWS_ACCESS_KEY_ID or AWS_SECRET_ACCESS_KEY

fluential and others added 3 commits July 13, 2020 16:21
* Simplified Dockerfile with nginx+uwsgi

* Update docker info

* Also removed default supervisord.ini file as this comes by default from the original image https://github.com/tiangolo/uwsgi-nginx-docker/blob/master/docker-images/supervisord-alpine.ini and we don't need to modify it as I originally thought
@fluential
Copy link
Author

fluential commented Jul 13, 2020

@brentvollebregt I've cleaned up the docker info which perhaps was not the most obvious, plus some other various cleanups -- have a look

@fluential fluential closed this Jul 14, 2020
@fluential fluential deleted the dockerize branch July 14, 2020 10:18
@fluential fluential restored the dockerize branch July 14, 2020 10:18
@fluential fluential deleted the dockerize branch July 14, 2020 10:18
@fluential fluential restored the dockerize branch July 14, 2020 10:23
@fluential fluential deleted the dockerize branch July 14, 2020 10:24
@fluential fluential restored the dockerize branch July 14, 2020 10:24
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.

None yet

3 participants