Bug 30627 - koha-run-backups delete the backup files after finished its job without caring days option
Summary: koha-run-backups delete the backup files after finished its job without carin...
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Andreas Jonsson
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 36425 36435
  Show dependency treegraph
 
Reported: 2022-04-27 11:06 UTC by Mengu Yazicioglu
Modified: 2024-03-27 02:52 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00,23.11.03,23.05.09


Attachments
Bug 30627: Verify --days parameter and use find command to select old backups for deletion (3.16 KB, patch)
2023-11-06 14:34 UTC, Andreas Jonsson
Details | Diff | Splinter Review
Bug 30627: Verify --days parameter and use find command to select old backups for deletion (3.22 KB, patch)
2023-12-17 22:10 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 30627: Verify --days parameter and use find command to select old backups for deletion (3.31 KB, patch)
2024-01-26 08:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Mengu Yazicioglu 2022-04-27 11:06:49 UTC
I've a doubt about the koha-run-backups script that it deletes its backup files if the backup destination directory is empty.
Comment 1 Mengu Yazicioglu 2022-04-27 11:51:00 UTC
First findings:

and a correction, if there are some other files in the backup destination directory, script also counts them and this makes the problem.

In the remove part of script, sed "s:^$name ... part removes the instance name and expects only related backup files, but if somehow there are some other files, backups are being deleted

It could be better to find older files with find command maybe.
Comment 2 jabiertxof 2023-01-12 15:39:54 UTC
I also have this issue, backups are fixed to 2 days even harrcoding days variable from 2 to 7
Comment 3 Andreas Jonsson 2023-11-06 14:32:53 UTC
I do not understand why it is validated that a complete pair of sql.gz and tar.gz exists before deleting the pair.  As if it would be especially desirable to preserve backups that may have failed before being completed indefinitely.  I suggest that we don't do this.

We can select files for deletion with the `find' command.  We can use the modification time instead of the date in the filename.  These should normally be the same.

We can also use a stricter pattern for matching files to avoid deleting files that have not been created by the backup script.  For instance:

find $backupdir -maxdepth 1 \( -mtime +$days -or -mtime $days \) \
                -name $name-'[1-9][0-9][0-9][0-9]-[0-9][0-9]-[0-9][0-9].???'.gz \
                \( -name \*.sql.gz -or -name \*.tar.gz \) -print0 | xargs -0 -r rm
Comment 4 Andreas Jonsson 2023-11-06 14:34:59 UTC
Created attachment 158533 [details] [review]
Bug 30627: Verify --days parameter and use find command to select old backups for deletion

Test plan

* Create some old fake backups:

backuproot=/var/spool/koha
instance=kohadev

backupdir="$backuproot"/"$instance"

for i in 1 2 3 4 ; do
   for j in sql tar xxx ; do
      file="$backupdir"/"$instance"-$(date -I -d "- $i day").${j}.gz
      if ! test -e "$file" ; then
         touch -t "$(date +%Y%m%d%H%M -d "- $i day")" "$file"
      fi
   done
done

* Verify that --days parameter is validated

sudo koha-run-backups --days 0
sudo koha-run-backups --days foo

* Run backup

sudo koha-run-backups --days 3

* Verify that backups from 3 days have been preserved and older backups have been deleted
* Verify that filenames that do not match the pattern (the .xxx.gz files) are preserved
Comment 5 David Nind 2023-11-30 18:34:14 UTC
Added assignee.
Comment 6 Chris Cormack 2023-12-17 22:10:04 UTC
Created attachment 159931 [details] [review]
Bug 30627: Verify --days parameter and use find command to select old backups for deletion

Test plan

* Create some old fake backups:

backuproot=/var/spool/koha
instance=kohadev

backupdir="$backuproot"/"$instance"

for i in 1 2 3 4 ; do
   for j in sql tar xxx ; do
      file="$backupdir"/"$instance"-$(date -I -d "- $i day").${j}.gz
      if ! test -e "$file" ; then
         touch -t "$(date +%Y%m%d%H%M -d "- $i day")" "$file"
      fi
   done
done

* Verify that --days parameter is validated

sudo koha-run-backups --days 0
sudo koha-run-backups --days foo

* Run backup

sudo koha-run-backups --days 3

* Verify that backups from 3 days have been preserved and older backups have been deleted
* Verify that filenames that do not match the pattern (the .xxx.gz files) are preserved

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Comment 7 Victor Grousset/tuxayo 2024-01-12 19:54:11 UTC
I don't understand the code enough to fully QA this but here are some findings.

why is there `\( -name \*.sql.gz -or -name \*.tar.gz \)` for the pattern? Doesn't that negates the use of the preceding more strict pattern? Wait, is that just to constrain the ??? to be either sql or tar? (just found that there is an implicit AND (-a) when there is no operator between tests)

-----

About switching to using file metadata instead of file name to get the date.
There should be at least a mention in the documentation of --days and a mention in the release notes ("Text to go in the release notes"). To lessen the chances of mistakes due to eventual divergences there. So if there is one, we have less to worry about and keep the proposed approach to use metadata.

Speaking of divergences, is file metadata something we can count on? I don't know enough about managing servers and backups to reliably say if that's reliable or not. Here are some thoughts
- if another backup system backups koha backup files, then we can expect date to be preserved
- if the files are moved via ssh or something else to a new machine to restore a koha backup (hardware upgrade for a koha server), then at worse date should be reset to current date, so backups would be deleted later, that doesn't look like much of an issue
- zip archives preserve the date
- copying on a FAT32 USB drive also
- if the place where the koha backup files are mounted is via some network protocol, is that possible (and not that rare enough) that it doesn't support date metadata? I recall with a phone using MTP (not a network protocol and MTP has tons of very bad implementation) that all the files I copied to my PC got their date set to 1970...
So that's the only real worry. Does date being reset to 1970 is a thing that can happen enough to compromise the use of date metadata vs name?
Comment 8 Marcel de Rooy 2024-01-26 08:32:51 UTC
(In reply to Victor Grousset/tuxayo from comment #7)
> So that's the only real worry. Does date being reset to 1970 is a thing that
> can happen enough to compromise the use of date metadata vs name?

LOL It happens all the time :)
Comment 9 Marcel de Rooy 2024-01-26 08:51:06 UTC
Created attachment 161494 [details] [review]
Bug 30627: Verify --days parameter and use find command to select old backups for deletion

Test plan

* Create some old fake backups:

backuproot=/var/spool/koha
instance=kohadev

backupdir="$backuproot"/"$instance"

for i in 1 2 3 4 ; do
   for j in sql tar xxx ; do
      file="$backupdir"/"$instance"-$(date -I -d "- $i day").${j}.gz
      if ! test -e "$file" ; then
         touch -t "$(date +%Y%m%d%H%M -d "- $i day")" "$file"
      fi
   done
done

* Verify that --days parameter is validated

sudo koha-run-backups --days 0
sudo koha-run-backups --days foo

* Run backup

sudo koha-run-backups --days 3

* Verify that backups from 3 days have been preserved and older backups have been deleted
* Verify that filenames that do not match the pattern (the .xxx.gz files) are preserved

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 10 Marcel de Rooy 2024-01-26 08:54:43 UTC
Great simplification and fix, Andreas.

Victor: Can understand your concerns, but should not really be an issue imo in daily operation.
Comment 11 Katrin Fischer 2024-01-26 14:17:16 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 12 Fridolin Somers 2024-01-31 13:05:13 UTC
Pushed to 23.11.x for 23.11.03
Comment 13 Victor Grousset/tuxayo 2024-02-02 05:14:07 UTC
(In reply to Marcel de Rooy from comment #10)
> Victor: Can understand your concerns, but should not really be an issue imo
> in daily operation.

Oh great :D I though we were doomed when you said "LOL It happens all the time :)"
T_T

But no!

Thanks for your insight that this shouldn't be a problem in practice ^^
Comment 14 Lucas Gass 2024-02-08 18:57:02 UTC
Backported to 23.05.x for upcoming 23.05.09