zfsonlinux / zfs-auto-snapshot

ZFS Automatic Snapshot Service for Linux
GNU General Public License v2.0
858 stars 248 forks source link

dry-run erroneously creates snapshots on all datasets #111

Open kewiha opened 5 years ago

kewiha commented 5 years ago

When I run the following command, snapshots are created on all datasets (not just the one I specified), but the --dry-run argument is given:

zfs-auto-snapshot -sq -k 120 -l "min" -r --dry-run h

Based on the --help text, I expect this code will not create any snapshots at all because --dry-run is specified. If --dry-run was not specified, I would expect it to create recursive snapshots of dataset "h" with the "min" label. Instead, it creates recursive snapshots on ALL datasets (not just "h") with the "frequent" label. I am running debian stretch using zfs-auto-snapshot 1.2.1-1+deb9u1 from the debian repo (installed with apt install zfs-auto-snapshot).

This is a bug and is a nuisance to clean up. I used the following code in the shell to clean up the mess it made:

zsnaps_extra="$(zfs list -t snapshot | grep "frequent" | awk '{print $1}')"
nsnaps_extra="$(echo "$zsnaps_extra" | grep -c "")"
for j in `seq 1 $nsnaps_extra` ; do
  snaptodel="$(printf '%s\n' "$zsnaps_extra" | head -n $j | tail -n 1 )"
  /sbin/zfs destroy -rpv "$snaptodel" 
done
kewiha commented 5 years ago

This appears to actually be the default additions to the various /etc/cron* directories in the debian stretch packages. I don't think this is a sensible default because certain situations (e.g. docker on zfs) need to destroy zfs datasets, but won't/can't if snapshots exist.

I suggest not including the files in /etc/cron* by default, or commenting out the lines that run zfs-auto-snapshot so users can enable them manually.

gdevenyi commented 4 years ago

I suggest not including the files in /etc/cron* by default, or commenting out the lines that run zfs-auto-snapshot so users can enable them manually.

This is a debian packaging issue then. Debian/Ubuntu take the position that when installing a package, the default configuration should be installed and enabled.

Once solution might be, as in other packages (smartmontools, apcupsd) to add an /etc/defaults/zfs-auto-snapshot that has an entry "enabled=0" and making the cron entries check that first, so it has to be manually flipped.

hbh7 commented 4 years ago

+1 to default disabled crontab entries in Debian/Ubuntu packages. I'm setting this up now and just noticed that it defaults to enabled. I want them to run on the hour rather than offset like cron.hourly says, so now I have to go disable them all and make new ones and hope that it doesn't get upset when an update comes along (although my understanding is that it will get upset).

rlaager commented 4 years ago

I strongly recommend against adding new enabled=0 type things to Debian. The new way to do scheduled jobs is systemd units, which can be enabled and disabled directly via first-class systemd features. If you have both a cron job (because you want to support non-systemd systems and/or for backwards-compatibility reasons) and a pair of systemd units plus an enabled=0 thing, it raises questions of whether that should control the systemd service. If you pick "yes", then you have two ways to enable/disable the systemd service, which is bad. If you pick "no", then you have a delta between how the cron version works and how the systemd version works, which may be confusing, especially to people converting from one to the other.

A better way, if you want the default to be off, is to ship:

In this way, the admin can enable the relevant task (systemd or cron) according to their system/preference. If they do not want the service, this avoids having jobs start up only to immediately exit (because enabled=0 or because it's the cron job checking that you're on a systemd system).

This wasn't my idea. See the recent discussion on debian-devel: https://lists.debian.org/debian-devel/2020/01/msg00154.html

If you want the default to be on, things are more complicated. The same idea would work, but I'm not sure if a package-installed symlink from /etc/cron. is Policy-compliant. I'm also not sure how much manual handling of that (e.g. in the postinst script) would be necessary to avoid stomping on user customizations (e.g. if they change the symlink to a file). It's also dangerous in that user may make edits not realizing it's a symlink; they'd be editing the file in /usr/share, which would mean their changes would be overwritten. It may be easier to use the "traditional" approach of just installing regular files into /etc/cron., and if you wish to also install system units, have the cron job check for systemd and exit.