Merge dd244de945 into a7e6c2db68

[sanoid] implemented dataset cache and fix race conditions

sanoid

@@ -35,17 +35,6 @@ if (keys %args < 4) {
 	$args{'verbose'} = 1;
 }
 
-
-my $cacheTTL = 900; # 15 minutes
-
-# Allow a much older snapshot cache file than default if _only_ "--monitor-*" action commands are given
-# (ignore "--verbose", "--configdir" etc)
-if (($args{'monitor-snapshots'} || $args{'monitor-health'} || $args{'monitor-capacity'}) && ! ($args{'cron'} || $args{'force-update'} || $args{'take-snapshots'} || $args{'prune-snapshots'} || $args{'force-prune'})) {
-	# The command combination above must not assert true for any command that takes or prunes snapshots
-	$cacheTTL = 18000; # 5 hours
-	if ($args{'debug'}) { print "DEBUG: command combo means that the cache file (provided it exists) will be allowed to be older than default.\n"; }
-}
-
 # for compatibility reasons, older versions used hardcoded command paths
 $ENV{'PATH'} = $ENV{'PATH'} . ":/bin:/sbin";
 
@@ -57,25 +46,70 @@ my $zpool = 'zpool';
 my $conf_file = "$args{'configdir'}/sanoid.conf";
 my $default_conf_file = "$args{'configdir'}/sanoid.defaults.conf";
 
-# parse config file
-my %config = init($conf_file,$default_conf_file);
-
 my $cache_dir = $args{'cache-dir'};
 my $run_dir = $args{'run-dir'};
 
 make_path($cache_dir);
 make_path($run_dir);
 
-# if we call getsnaps(%config,1) it will forcibly update the cache, TTL or no TTL
-my $forcecacheupdate = 0;
+my $cacheTTL = 1200; # 20 minutes
+
+# Allow a much older snapshot cache file than default if _only_ "--monitor-*" action commands are given
+# (ignore "--verbose", "--configdir" etc)
+if (
+	(
+		$args{'monitor-snapshots'}
+		|| $args{'monitor-health'}
+		|| $args{'monitor-capacity'}
+	) && ! (
+		$args{'cron'}
+		|| $args{'force-update'}
+		|| $args{'take-snapshots'}
+		|| $args{'prune-snapshots'}
+		|| $args{'force-prune'}
+	)
+) {
+	# The command combination above must not assert true for any command that takes or prunes snapshots
+	$cacheTTL = 18000; # 5 hours
+	if ($args{'debug'}) { print "DEBUG: command combo means that the cache file (provided it exists) will be allowed to be older than default.\n"; }
+}
+
+# snapshot cache
 my $cache = "$cache_dir/snapshots.txt";
-my %snaps = getsnaps( \%config, $cacheTTL, $forcecacheupdate );
+
+# configured dataset cache
+my $cachedatasetspath = "$cache_dir/datasets.txt";
+my @cachedatasets;
+
+# parse config file
+my %config = init($conf_file,$default_conf_file);
+
 my %pruned;
 my %capacitycache;
 
-my %snapsbytype = getsnapsbytype( \%config, \%snaps );
+my %snaps;
+my %snapsbytype;
+my %snapsbypath;
 
-my %snapsbypath = getsnapsbypath( \%config, \%snaps );
+# get snapshot list only if needed
+if ($args{'monitor-snapshots'}
+	|| $args{'monitor-health'}
+	|| $args{'cron'}
+	|| $args{'take-snapshots'}
+	|| $args{'prune-snapshots'}
+	|| $args{'force-update'}
+	|| $args{'debug'}
+) {
+	my $forcecacheupdate = 0;
+	if ($args{'force-update'}) 	{
+		$forcecacheupdate = 1;
+	}
+
+	%snaps = getsnaps( \%config, $cacheTTL, $forcecacheupdate);
+
+	%snapsbytype = getsnapsbytype( \%config, \%snaps );
+	%snapsbypath = getsnapsbypath( \%config, \%snaps );
+}
 
 # let's make it a little easier to be consistent passing these hashes in the same order to each sub
 my @params = ( \%config, \%snaps, \%snapsbytype, \%snapsbypath );
@@ -84,7 +118,6 @@ if ($args{'debug'})		{ $args{'verbose'}=1; blabber (@params); }
 if ($args{'monitor-snapshots'}) { monitor_snapshots(@params); }
 if ($args{'monitor-health'}) 	{ monitor_health(@params); }
 if ($args{'monitor-capacity'})  { monitor_capacity(@params); }
-if ($args{'force-update'}) 	{ my $snaps = getsnaps( \%config, $cacheTTL, 1 ); }
 
 if ($args{'cron'}) {
 	if ($args{'quiet'}) { $args{'verbose'} = 0; }
@@ -275,7 +308,6 @@ sub prune_snapshots {
 	my ($config, $snaps, $snapsbytype, $snapsbypath) = @_;
 
 	my %datestamp = get_date();
-	my $forcecacheupdate = 0;
 
 	foreach my $section (keys %config) {
 		if ($section =~ /^template/) { next; }
@@ -826,7 +858,7 @@ sub getsnaps {
 		if (checklock('sanoid_cacheupdate')) {
 			writelock('sanoid_cacheupdate');
 			if ($args{'verbose'}) {
-				if ($args{'force-update'}) {
+				if ($forcecacheupdate) {
 					print "INFO: cache forcibly expired - updating from zfs list.\n";
 				} else {
 					print "INFO: cache expired - updating from zfs list.\n";
@@ -836,9 +868,10 @@ sub getsnaps {
 			@rawsnaps = <FH>;
 			close FH;
 
-			open FH, "> $cache" or die 'Could not write to $cache!\n';
+			open FH, "> $cache.tmp" or die 'Could not write to $cache.tmp!\n';
 			print FH @rawsnaps;
 			close FH;
+			rename("$cache.tmp", "$cache") or die 'Could not rename to $cache!\n';
 			removelock('sanoid_cacheupdate');
 		} else {
 			if ($args{'verbose'}) { print "INFO: deferring cache update - valid cache update lock held by another sanoid process.\n"; }
@@ -901,6 +934,20 @@ sub init {
 		die "FATAL: you're using sanoid.defaults.conf v$defaults_version, this version of sanoid requires a minimum sanoid.defaults.conf v$MINIMUM_DEFAULTS_VERSION";
 	}
 
+	my @updatedatasets;
+
+	# load dataset cache if valid
+	if (!$args{'force-update'} && -f $cachedatasetspath) {
+		my ($dev, $ino, $mode, $nlink, $uid, $gid, $rdev, $size, $atime, $mtime, $ctime, $blksize, $blocks) = stat($cachedatasetspath);
+
+		if ((time() - $mtime) <= $cacheTTL) {
+			if ($args{'debug'}) { print "DEBUG: dataset cache not expired (" . (time() - $mtime) . " seconds old with TTL of $cacheTTL): pulling dataset list from cache.\n"; }
+			open FH, "< $cachedatasetspath";
+			@cachedatasets = <FH>;
+			close FH;
+		}
+	}
+
 	foreach my $section (keys %ini) {
 
 		# first up - die with honor if unknown parameters are set in any modules or templates by the user.
@@ -990,6 +1037,10 @@ sub init {
 			$config{$section}{'path'} = $section;
 		}
 
+		if (! @cachedatasets) {
+			push (@updatedatasets, "$config{$section}{'path'}\n");
+		}
+
 		# how 'bout some recursion? =)
 		if ($config{$section}{'zfs_recursion'} && $config{$section}{'zfs_recursion'} == 1 && $config{$section}{'autosnap'} == 1) {
 			warn "ignored autosnap configuration for '$section' because it's part of a zfs recursion.\n";
@@ -1007,6 +1058,10 @@ sub init {
 
 			@datasets = getchilddatasets($config{$section}{'path'});
 			DATASETS: foreach my $dataset(@datasets) {
+				if (! @cachedatasets) {
+					push (@updatedatasets, $dataset);
+				}
+
 				chomp $dataset;
 
 				if ($zfsRecursive) {
@@ -1038,9 +1093,27 @@ sub init {
 				$config{$dataset}{'initialized'} = 1;
 			}
 		}
+	}
 
-
-
+	# update dataset cache if it was unused
+	if (! @cachedatasets) {
+		if (checklock('sanoid_cachedatasetupdate')) {
+			writelock('sanoid_cachedatasetupdate');
+			if ($args{'verbose'}) {
+				if ($args{'force-update'}) {
+					print "INFO: dataset cache forcibly expired - updating from zfs list.\n";
+				} else {
+					print "INFO: dataset cache expired - updating from zfs list.\n";
+				}
+			}
+			open FH, "> $cachedatasetspath.tmp" or die 'Could not write to $cachedatasetspath.tmp!\n';
+			print FH @updatedatasets;
+			close FH;
+			rename("$cachedatasetspath.tmp", "$cachedatasetspath") or die 'Could not rename to $cachedatasetspath!\n';
+			removelock('sanoid_cachedatasetupdate');
+		} else {
+			if ($args{'verbose'}) { print "INFO: deferring dataset cache update - valid cache update lock held by another sanoid process.\n"; }
+		}
 	}
 
 	return %config;
@@ -1590,6 +1663,30 @@ sub getchilddatasets {
 	my $fs = shift;
 	my $mysudocmd = '';
 
+	# use dataset cache if available
+	if (@cachedatasets) {
+		my $foundparent = 0;
+		my @cachechildren = ();
+		foreach my $dataset (@cachedatasets) {
+			chomp $dataset;
+			my $ret = rindex $dataset, "${fs}/", 0;
+			if ($ret == 0) {
+				push (@cachechildren, $dataset);
+			} else {
+				if ($dataset eq $fs) {
+					$foundparent = 1;
+				}
+			}
+		}
+
+		# sanity check
+		if ($foundparent) {
+			return @cachechildren;
+		}
+
+		# fallback if cache misses items for whatever reason
+	}
+
 	my $getchildrencmd = "$mysudocmd $zfs list -o name -t filesystem,volume -Hr $fs |";
 	 if ($args{'debug'}) { print "DEBUG: getting list of child datasets on $fs using $getchildrencmd...\n"; }
 	open FH, $getchildrencmd;
@@ -1636,16 +1733,17 @@ sub removecachedsnapshots {
 	my @rawsnaps = <FH>;
 	close FH;
 
-	open FH, "> $cache" or die 'Could not write to $cache!\n';
+	open FH, "> $cache.tmp" or die 'Could not write to $cache.tmp!\n';
 	foreach my $snapline ( @rawsnaps ) {
 		my @columns = split("\t", $snapline);
 		my $snap = $columns[0];
 		print FH $snapline unless ( exists($pruned{$snap}) );
 	}
 	close FH;
+	rename("$cache.tmp", "$cache") or die 'Could not rename to $cache!\n';
 
 	removelock('sanoid_cacheupdate');
-	%snaps = getsnaps(\%config,$cacheTTL,$forcecacheupdate);
+	%snaps = getsnaps(\%config,$cacheTTL,0);
 
 	# clear hash
 	undef %pruned;

syncoid

@@ -828,15 +828,16 @@ sub syncdataset {
 	}
 	if (defined $args{'no-sync-snap'}) {
 		if (defined $args{'create-bookmark'}) {
-			my $ret = createbookmark($sourcehost, $sourcefs, $newsyncsnap, $newsyncsnap);
+			my %existingbookmarks = getbookmarks($sourcehost,$sourcefs,$sourceisroot);
+			my $guid = $snaps{'source'}{$newsyncsnap}{'guid'};
+			my $ret = createbookmark($sourcehost, $sourcefs, $newsyncsnap, $newsyncsnap, $guid, %existingbookmarks);
 			$ret == 0 or do {
 				# fallback: assume naming conflict and try again with guid based suffix
-				my $guid = $snaps{'source'}{$newsyncsnap}{'guid'};
-				$guid = substr($guid, 0, 6);
+				my $suffix = substr($guid, 0, 6);
 
-				writelog('INFO', "bookmark creation failed, retrying with guid based suffix ($guid)...");
+				writelog('INFO', "bookmark creation failed, retrying with guid based suffix ($suffix)...");
 
-				my $ret = createbookmark($sourcehost, $sourcefs, $newsyncsnap, "$newsyncsnap$guid");
+				my $ret = createbookmark($sourcehost, $sourcefs, $newsyncsnap, "$newsyncsnap$suffix", $guid, %existingbookmarks);
 				$ret == 0 or do {
 					if ($exitcode < 2) { $exitcode = 2; }
 					return 0;
@@ -1099,7 +1100,12 @@ sub syncbookmark {
 } # end syncbookmark
 
 sub createbookmark {
-	my ($sourcehost, $sourcefs, $snapname, $bookmark) = @_;
+	my ($sourcehost, $sourcefs, $snapname, $bookmark, $guid, %existingbookmarks) = @_;
+
+	if (defined $existingbookmarks{$guid} && $existingbookmarks{$guid}{'name'} eq $bookmark) {
+		writelog('INFO', "bookmark already exists, skipping creation");
+		return 0;
+	}
 
 	my $sourcefsescaped = escapeshellparam($sourcefs);
 	my $bookmarkescaped = escapeshellparam($bookmark);

tests/syncoid/12_bookmark_check_guid_on_existing/run.sh

@@ -0,0 +1,93 @@
+#!/bin/bash
+
+# test if guid of existing bookmark matches new guid
+
+set -x
+set -e
+
+. ../../common/lib.sh
+
+POOL_IMAGE="/tmp/syncoid-test-12.zpool"
+MOUNT_TARGET="/tmp/syncoid-test-12.mount"
+POOL_SIZE="1000M"
+POOL_NAME="syncoid-test-12"
+
+truncate -s "${POOL_SIZE}" "${POOL_IMAGE}"
+
+zpool create -m "${MOUNT_TARGET}" -f "${POOL_NAME}" "${POOL_IMAGE}"
+
+function cleanUp {
+  zpool export "${POOL_NAME}"
+}
+
+function getGuid {
+  zfs get -H guid "$1" | awk '{print $3}'
+}
+
+# export pool in any case
+trap cleanUp EXIT
+
+zfs create "${POOL_NAME}/a"
+zfs snapshot "${POOL_NAME}/a@s0"
+
+# This fully replicates a to b
+../../../syncoid --debug --no-sync-snap --no-rollback --create-bookmark "${POOL_NAME}"/a "${POOL_NAME}"/b
+
+# This fully replicates a to c
+../../../syncoid --debug --no-sync-snap --no-rollback --create-bookmark "${POOL_NAME}"/a "${POOL_NAME}"/c
+
+bookmark_guid=$(getGuid "${POOL_NAME}/a#s0")
+snap_a_guid=$(getGuid "${POOL_NAME}/a@s0")
+snap_b_guid=$(getGuid "${POOL_NAME}/b@s0")
+snap_c_guid=$(getGuid "${POOL_NAME}/c@s0")
+
+# Bookmark guid should equal guid of all snapshots
+if [ "${bookmark_guid}" != "${snap_a_guid}" ] || \
+   [ "${bookmark_guid}" != "${snap_b_guid}" ] || \
+   [ "${bookmark_guid}" != "${snap_c_guid}" ]; then
+  exit 1
+fi
+
+bookmark_suffix="${bookmark_guid:0:6}"
+fallback_bookmark="${POOL_NAME}/a#s0${bookmark_suffix}"
+
+# Fallback bookmark should not exist
+if zfs get guid "${fallback_bookmark}"; then
+  exit 1
+fi
+
+zfs snapshot "${POOL_NAME}/a@s1"
+
+# Create bookmark so syncoid is forced to create fallback bookmark
+zfs bookmark "${POOL_NAME}/a@s0" "${POOL_NAME}/a#s1"
+
+# This incrementally replicates from a@s0 to a@s1 and should create a
+# bookmark with fallback suffix
+../../../syncoid --debug --no-sync-snap --no-rollback --create-bookmark "${POOL_NAME}"/a "${POOL_NAME}"/b
+
+snap_guid=$(getGuid "${POOL_NAME}/a@s1")
+bookmark_suffix="${snap_guid:0:6}"
+fallback_bookmark="${POOL_NAME}/a#s1${bookmark_suffix}"
+
+# Fallback bookmark guid should equal guid of snapshot
+if [ "$(getGuid "${fallback_bookmark}")" != "${snap_guid}" ]; then
+  exit 1
+fi
+
+zfs snapshot "${POOL_NAME}/a@s2"
+
+snap_guid=$(getGuid "${POOL_NAME}/a@s2")
+bookmark_suffix="${snap_guid:0:6}"
+fallback_bookmark="${POOL_NAME}/a#s2${bookmark_suffix}"
+
+# Create bookmark and fallback bookmark so syncoid should fail
+zfs bookmark "${POOL_NAME}/a@s0" "${POOL_NAME}/a#s2"
+zfs bookmark "${POOL_NAME}/a@s0" "${fallback_bookmark}"
+
+# This incrementally replicates from a@s1 to a@s2 and should fail to create a
+# bookmark with fallback suffix
+if ../../../syncoid --debug --no-sync-snap --no-rollback --create-bookmark "${POOL_NAME}"/a "${POOL_NAME}"/b; then
+  exit 1
+fi
+
+exit 0