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

dry-run erroneously creates snapshots on all datasets #111

Open
kewiha opened this issue Nov 3, 2019 · 4 comments
Open

dry-run erroneously creates snapshots on all datasets #111

kewiha opened this issue Nov 3, 2019 · 4 comments

Comments

@kewiha
Copy link

kewiha commented Nov 3, 2019

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
Copy link
Author

kewiha commented Nov 3, 2019

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
Copy link

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
Copy link

hbh7 commented Jan 28, 2020

+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
Copy link

rlaager commented Jan 28, 2020

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:

  • systemd units where the timer is installed but not enabled
  • and optionally, a cron script (or series of scripts) that can be symlinked into /etc/cron.{daily,hourly,monthly,weekly} or a cron file that can be symlinked into /etc/cron.d

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.

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

No branches or pull requests

4 participants