Bug 38554 - Limit number of backups in /var/univention-backup
Limit number of backups in /var/univention-backup
Status: CLOSED FIXED
Product: UCS
Classification: Unclassified
Component: General
UCS 4.0
Other Linux
: P5 enhancement (vote)
: UCS 4.2-2-errata
Assigned To: Lukas Oyen
Arvid Requate
:
Depends on:
Blocks: 45408
  Show dependency treegraph
 
Reported: 2015-05-18 15:50 CEST by Tim Petersen
Modified: 2017-09-18 10:58 CEST (History)
7 users (show)

See Also:
What kind of report is it?: ---
What type of bug is this?: ---
Who will be affected by this bug?: ---
How will those affected feel about the bug?: ---
User Pain:
Enterprise Customer affected?: Yes
School Customer affected?:
ISV affected?:
Waiting Support:
Flags outvoted (downgraded) after PO Review:
Ticket number: 2015110221000417, 2016041321000837, 2016072721000191
Bug group (optional): External feedback
Max CVSS v3 score:
oyen: Patch_Available+


Attachments
Suggested patch for /usr/sbin/univention-ldap-backup (3.06 KB, patch)
2016-09-12 11:42 CEST, Julius Hinrichs
Details | Diff
/usr/share/ucs-test/10_ldap/101_test_univention_ldap_backup_limit (2.44 KB, text/plain)
2016-12-07 13:15 CET, Julius Hinrichs
Details
38554-backup-cleanup-420.patch (8.68 KB, patch)
2017-05-31 17:22 CEST, Lukas Oyen
Details | Diff
quoting.patch (734 bytes, patch)
2017-09-06 19:29 CEST, Arvid Requate
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Petersen univentionstaff 2015-05-18 15:50:21 CEST
It would be nice to be able to limit the max amount of days for the nightly backup scripts - for example with the help of UCR.
Otherwise you have tons of unqiue backup variants underneath /var/univention-backup for the last 100xxx days.
Comment 1 Stephan Hendl 2015-10-06 15:15:55 CEST
Are there any updates? Maybe upcoming UCS-4.1 is a good place in case it is not possible within 4.0-x?
Comment 2 Tim Petersen univentionstaff 2015-11-02 14:58:09 CET
Requested again at 2015110221000417
Comment 3 Michael Grandjean univentionstaff 2015-11-05 20:50:39 CET
Requested during workshop.
Comment 4 Michael Grandjean univentionstaff 2016-04-13 15:54:38 CEST
2016041321000837
Comment 5 Nico Stöckigt univentionstaff 2016-07-27 14:20:26 CEST
dearly requested at Ticket#2016072721000191
Comment 6 Julius Hinrichs univentionstaff 2016-09-12 11:42:39 CEST
Created attachment 7995 [details]
Suggested patch for /usr/sbin/univention-ldap-backup

This is a suggested patch for /usr/sbin/univention-ldap-backup

A maximum number of backups can be set like this:
ucr set ucr/univention-ldap-backup/limit='n'

After a new backup has been created, the oldest backups will be deleted until only the newest n remain.
Comment 7 Julius Hinrichs univentionstaff 2016-12-07 13:15:56 CET
Created attachment 8291 [details]
/usr/share/ucs-test/10_ldap/101_test_univention_ldap_backup_limit

This is a test case for the attachment above.
Comment 8 Stephan Hendl 2017-05-29 09:13:19 CEST
Are there any new investigations on this?
Comment 9 Lukas Oyen univentionstaff 2017-05-31 17:22:38 CEST
Created attachment 8891 [details]
38554-backup-cleanup-420.patch

The attached patches implement `clean_old_backups` in univention-lib. This
functions expects a pattern and an optional `max_age` and deletes files in
/var/univention-backup older than max_age.

Two new UCR variables control this function: backup/clean/max_age and
backup/clean/min_backups. If the former is unset no cleanup is performed. The
latter acts as a safeguard of a minimal amount of backups to keep, even if older
than `max_age` (10 is the default if unset).

This changes the behaviour of univention-samba4-backup, as per default no
cleanup of samba4-backups is performed (u-s4-backup deleted backups after 356
days). univention-samba4-backup overrides backup/clean/max_age locally if called
with `--days`. This is possible via the UCR variable samba4/backup/cron/options.

This is a more general solution, in contrast to Julius' cleanup of LDAP-backups.

We should bump the dependencies on `shell-univention-lib` on the following
packages if this is commited: univention-base-files, univention-ldap,
univention-samba4
Comment 10 Florian Best univentionstaff 2017-06-28 14:52:56 CEST
There is a Customer ID set so I set the flag "Enterprise Customer affected".
Comment 11 Arvid Requate univentionstaff 2017-08-01 12:12:08 CEST
Ok, please keep the syntax /bin/sh compatible if possible, so the library can be used by dash scripts too. Also doesn't it make sense to keep the base path out of the function to make it more usable? like

clean_old_backups '/var/univention-backup/samba/\(samba4_\|sysvol\).*.bz2' \
                  "$DAYS"
Comment 12 Lukas Oyen univentionstaff 2017-08-01 17:35:46 CEST
(In reply to Arvid Requate from comment #11)
> Also doesn't it make sense to keep the base
> path out of the function to make it more usable?

Yes, but such a general solution would also allow the user to pass `(/var/univention-backup/)|(/home/peter/backup)` as the pattern. This makes it difficult to extract a base-dir to pass to find.

So fixing the directory to /var/univention-backup keeps the implementation simple and spares us from running `find / -regex ...` which would be a performance problem.

A possible solution would be an optional third parameter which defaults to /var/univention-backup.
Comment 13 Arvid Requate univentionstaff 2017-08-01 18:19:29 CEST
Ok, I understand, then just commit it. AFAIK dash doesn't support assigning a value to a variable during the "local" declaration.
Comment 14 Lukas Oyen univentionstaff 2017-08-02 18:24:35 CEST
implementation in univention-lib: r81720-r81722, advisory r81723
changes in univention-base-files: r81726, advisory r81727
changes in univention-samba4: r81728, advisory r81729
changes in univention-ldap: r81730, advisory r81731
Comment 15 Stephan Hendl 2017-08-14 12:22:04 CEST
I'd appreciate an (erratum) package :-) We need this feature to avoid filesystem messages from Nagios/Icinga!
Comment 16 Arvid Requate univentionstaff 2017-08-28 19:38:37 CEST
> The latter acts as a safeguard of a minimal amount of backups to keep, even if older than `max_age` (10 is the default if unset).


From reading the code I think it would remove all files aged over max_age if at least backup/clean/min/backups files are found. So assuming I have 9 files with age > max_age and then I add one new backup, I think the code would just keep the one new file. Please also declare the shell variables in the function as local.
Comment 17 Lukas Oyen univentionstaff 2017-09-04 11:17:09 CEST
(In reply to Arvid Requate from comment #16)
> From reading the code I think it would remove all files aged over max_age if
> at least backup/clean/min/backups files are found. So assuming I have 9
> files with age > max_age and then I add one new backup, I think the code
> would just keep the one new file.

No, if you have 9 old files and one new files, `count` would still be 9 and no files will be removed. Or I don't understand you correctly.

But the second `find` incorrectly uses `backup_clean_max_age` instead of `max_age`.

> Please also declare the shell variables in the function as local.

This contradicts comment 11, after which I removed the `local` for variables.
Comment 18 Lukas Oyen univentionstaff 2017-09-04 13:16:54 CEST
Added `local` back in and fixed the `max_age` typo. 

4.2-1: r82597/r82598, YAML: r82601
4.2-2: r82599/r82600, YAML: r82607
Comment 19 Arvid Requate univentionstaff 2017-09-06 19:09:03 CEST
Ok, works.
Comment 20 Arvid Requate univentionstaff 2017-09-06 19:29:06 CEST
Created attachment 9178 [details]
quoting.patch

Just a minor quoting patch, simplifies debugging.
Comment 21 Lukas Oyen univentionstaff 2017-09-11 15:03:45 CEST
Applied in 04a60cdb, YAML 9520d7.
Comment 22 Arvid Requate univentionstaff 2017-09-11 17:29:21 CEST
Ok, works.