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

Enhance the production compose file #200

Merged
merged 11 commits into from
Feb 23, 2022
Merged

Enhance the production compose file #200

merged 11 commits into from
Feb 23, 2022

Conversation

TheFrenchGhosty
Copy link
Member

@TheFrenchGhosty TheFrenchGhosty commented Feb 22, 2022

Follow up to #160 (comment)

Related to iv-org/invidious#2917 (wait for both to be ready - so that we can merge them at the same time)

Massively enhance the production compose:

  • Make the production and development compose use the same (sane and clean) bases
  • Change the restart policy for unless-stopped since always is bad practice
  • Remove the network (that's completely useless)
  • Remove the resources limiters (that's not really useful, and users should only use it if they need to)
  • Remove the willfarrell/autoheal container: we can't vouch for it's usefulness, and its security (it has access to the docker socket, which is a massive security risk)
  • Bump the postgres version to 14 since it's the latest and 10 EOL in 8 months and move to an Alpine-based Postgres image
  • Layout change (service then DB)
  • Enforce a container_name Edit: Enhance the production compose file #200 (comment)

It has been tested, and it's working

Installation.md Show resolved Hide resolved
Installation.md Outdated Show resolved Hide resolved
Installation.md Outdated Show resolved Hide resolved
Installation.md Outdated Show resolved Hide resolved
@TheFrenchGhosty TheFrenchGhosty requested review from Perflyst and unixfox and removed request for unixfox February 22, 2022 14:40
@unixfox
Copy link
Member

unixfox commented Feb 23, 2022

  • Don't advertise postgresql with alpine, it is known to have some issues. I can't find the github issue again but I remember it was better to use the more wide spread debian alternative for postgres docker image.
  • willfarrell/autoheal is still useful for automatically restarting invidious in case of errors. I don't think we should remove it until we have a solution/replacement.

Also maybe we could add a comment for the ARM64 image like this:

invidious:
    #image: quay.io/invidious/invidious:latest-arm64 # for ARM - raspberry pi
    image: quay.io/invidious/invidious:latest

@TheFrenchGhosty
Copy link
Member Author

TheFrenchGhosty commented Feb 23, 2022

@unixfox

Don't advertise postgresql with alpine, it is known to have some issues. I can't find the github issue again but I remember it was better to use the more wide spread debian alternative for postgres docker image.

It's what Nextcloud recommends: https://github.com/nextcloud/docker/blob/master/.examples/docker-compose/insecure/postgres/apache/docker-compose.yml#L5

Also, after reading https://hub.docker.com/_/postgres/ they just warn that postgres on alpine has less features... that we don't use. It that really a problem? It saves a good amount of space, and doesn't break anything.

willfarrell/autoheal is still useful for automatically restarting invidious in case of errors. I don't think we should remove it until we have a solution/replacement.

As I said: "we can't vouch for it's usefulness, and its security (it has access to the docker socket, which is a massive security risk)", users can use it, but we shouldn't "enforce" or endorse it in any way.

@unixfox
Copy link
Member

unixfox commented Feb 23, 2022

@unixfox

Don't advertise postgresql with alpine, it is known to have some issues. I can't find the github issue again but I remember it was better to use the more wide spread debian alternative for postgres docker image.

It's what Nextcloud recommends: https://github.com/nextcloud/docker/blob/master/.examples/docker-compose/insecure/postgres/apache/docker-compose.yml#L5

Also, after reading hub.docker.com/_/postgres they just warn that postgres on alpine has less features... that we don't use. It that really a problem? It saves a good amount of space, and doesn't break anything.

I can't find the issue again on invidious but I remember that it was causing some issues. I'll search again.

willfarrell/autoheal is still useful for automatically restarting invidious in case of errors. I don't think we should remove it until we have a solution/replacement.

As I said: "we can't vouch for it's usefulness, and its security (it has access to the docker socket, which is a massive security risk)", users can use it, but we shouldn't "enforce" or endorse it in any way.

Comment it then? I don't exactly know how to recommend but not having it by default.

@TheFrenchGhosty
Copy link
Member Author

TheFrenchGhosty commented Feb 23, 2022

Comment it then? I don't exactly know how to recommend but not having it by default.

Does it actual do anything compared to just the healthcheck though? Does it provides anything?

@unixfox
Copy link
Member

unixfox commented Feb 23, 2022

Comment it then? I don't exactly know how to recommend but not having it by default.

Does it actual do anything compared to just the healthcheck though? Does it provides anything?

It restarts the container if the healthcheck fails.

@TheFrenchGhosty
Copy link
Member Author

TheFrenchGhosty commented Feb 23, 2022

@unixfox

I can't find the issue again on invidious but I remember that it was causing some issues. I'll search again.

Did you find anything? What should I do? Stay on an Alpine image, or not risk anything and go back to the Debian one?


About willfarrell/autoheal:

After this PR is merged, I want to do some more minor changes to the document, so I'll mention that it can be used (this is out of scope for this PR)

@SamantazFox
Copy link
Member

SamantazFox commented Feb 23, 2022

Did you find anything? What should I do? Stay on an Alpine image, or not risk anything and go back to the Debian one?

I think we should stay on debian for the moment (and maybe use postgres 13 rather than 14, as 13 is what's currently available on debian stable).

Also, I'm not sure that the ~130M image will do much against the gigabyte or two that our DB can take. And that's a compose file aimed at "noobs" anyway (more experienced people will edit it as they see fit).

@TheFrenchGhosty
Copy link
Member Author

TheFrenchGhosty commented Feb 23, 2022

@SamantazFox

maybe use postgres 13 rather than 14, as 13 is what's currently available on debian stable

It really doesn't matters what ships on a distro for docker, 14 is the latest, so we should definitely go for it.

Also, I'm not sure that the ~130M image will do much against the gigabyte or two that our DB can take. And that's a compose file aimed at "noobs" anyway (more experienced people will edit it as they see fit).

Fair, let's go back to a Debian one then.

@unixfox
Copy link
Member

unixfox commented Feb 23, 2022

I can't find the issue, but I just remembered, basically it was something like that:
If you use the docker postgresql alpine you won't be able to convert it straight into a classic postgresql on debian/ubuntu by copying the postgres folder due to the incompatibilities with musl and glibc. And the same issue occurs if you want to convert a classic postgresql from debian/ubuntu to docker postgresql alpine.

Obviously, you can do a pg_dump from the source database then import it back from the .sql but sometimes just copying the entire postgresql data folder is faster and you keep everything including the permissions.

Also related to #201, if we create a tutorial to upgrade postgresql on docker, I'm not sure if we can easily upgrade a docker postgres 10 with debian to a docker postgres 14 with alpine.
The "PoC" made by one of the maintainer of the postgres docker image is only for debian it seems like: https://github.com/tianon/docker-postgres-upgrade

@TheFrenchGhosty
Copy link
Member Author

TheFrenchGhosty commented Feb 23, 2022

@unixfox That's a good point indeed!

PRs ready to merge then, unless you have any objection.

@TheFrenchGhosty TheFrenchGhosty merged commit 687ae29 into master Feb 23, 2022
@TheFrenchGhosty TheFrenchGhosty deleted the better-compose branch February 23, 2022 20:17
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