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

entrypoint: fix broken automatic backups #281

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Roosted7
Copy link

The scheduled database backups, that run by default using a cron timer, have not worked for a while. This seemed to be for 2 reasons:

  • Firstly the date command was not used properly, since both the '+' for the format was missing and %D should be %F (since the slashes produced by %D create new folders due to the usage in the script).
  • Secondly, the password was not passed on to pg_dump. This resulted in it not being able to run from the cron timer.

This PR fixes both those things :)

I have seen plans to drop this backup system in favor of the integrated one, but for the time being it would make sense to at least have this backup function operational?

@timabbott
Copy link
Sponsor Member

Sorry for the slow reply, and thanks for the PR!

@hackerkid can you take a look at this?

@hackerkid
Copy link
Member

@Roosted7 Do you know when was the last successful backup?

@Roosted7
Copy link
Author

@hackerkid Nope, our installation is quite recent and backups never worked. Only while implementing a custom (internal) backup solution, I noticed this broken built-in functionality and fixed that instead.

@hackerkid
Copy link
Member

@Roosted7 Okay. Just to verify, with this fix the corn job is correctly creating the backup right?

One thing I am a bit puzzled about is this.

root@de940ecbcfd4:~# more /etc/cron.d/autobackup 
MAILTO=""
30 3 * * * cd /;/entrypoint.sh app:backup
root@de940ecbcfd4:~#  cd /;/entrypoint.sh app:backup

The cronjob try to execute /entrypoint.sh but entrypoint.sh is present in /sbin/ directory. Shouldn't it be cd /;entrypoint.sh app:backup or /sbin/entrypoint.sh app:backup instead?

@timabbott
Copy link
Sponsor Member

Oh, I think we should actually just be removing this pg_dump logic entirely in favor of running manage.py backup, which is more carefully implemented and also designed to be portable to a non-docker-zulip system.

@maxxer
Copy link
Contributor

maxxer commented Nov 25, 2021

The cron entry looks wrong as well. Being on the filesystem it should include the username. The resulting schedule should be

30 3 * * * root cd /usr/sbin/;./entrypoint.sh app:backup

@maxxer
Copy link
Contributor

maxxer commented Nov 26, 2021

#262

@klardotsh
Copy link
Member

@alexmv you cool with me closing this in favor of #262? We then have a forking path to decide between taking the PGPASSWORD bits here and fixing (and probably extensively testing, given the discussion above) the cron job, or to migrate to manage.py backup outright, which I think is probably best suited as an Issues ticket for now until we know we have time to take it on (or an interested party to do so).

@alexmv
Copy link
Contributor

alexmv commented Nov 29, 2022

Fixing the cron entry is probably useful regardless of how the backup runs. I don't know that doing any of the PGPASSWORD work is useful, given #262.

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

6 participants