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

29 Oct 2017 - Daylight Saving Time Ended In Italy, sanoid left 60 snapshot per ZFS file system #155

Closed
gbonazzoli opened this issue Oct 29, 2017 · 22 comments · Fixed by #226

Comments

@gbonazzoli
Copy link

commented Oct 29, 2017

Sunday, 29 October 2017, 03:00:00 clocks were turned backward 1 hour to
Sunday, 29 October 2017, 02:00:00 local standard time instead.

This was the zfs snapshot result the day after.
That hour was played twice on the known computer italian universe and I supposed that sanoid was tricked.

vgraid/kvm/images/srv-progress@autosnap_2017-10-26_23:59:02_daily    130M      -  36.3G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-27_23:59:01_daily    107M      -  36.3G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-28_18:00:01_hourly   878K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-28_19:00:01_hourly   700K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-28_20:00:02_hourly   685K      -  36.4G  -
vgraid/kvm/images/srv-progress@syncoid_kvm01_2017-10-28:20:30:30     182K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-28_21:00:01_hourly   214K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-28_22:00:01_hourly   720K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-28_23:00:01_hourly   716K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-28_23:59:01_daily   61.5K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:00:01_hourly      0      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:00:01_daily       0      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:01:01_daily   91.5K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:02:01_daily    110K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:03:01_daily   60.5K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:04:01_daily     61K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:05:01_daily     61K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:06:01_daily     61K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:07:01_daily   60.5K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:08:01_daily   60.5K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:09:01_daily     60K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:10:01_daily     60K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:11:01_daily     60K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:12:01_daily   59.5K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:13:01_daily   59.5K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:14:01_daily   59.5K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:15:01_daily     59K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:16:01_daily     59K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:17:01_daily     59K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:18:01_daily     91K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:19:01_daily     59K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:20:01_daily     59K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:21:01_daily     59K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:22:01_daily   58.5K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:23:01_daily   58.5K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:24:01_daily   58.5K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:25:01_daily     58K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:26:01_daily     58K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:27:01_daily     58K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:28:01_daily   57.5K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:29:01_daily   57.5K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:30:01_daily   57.5K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:31:01_daily     57K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:32:01_daily     57K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:33:01_daily     57K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:34:01_daily   56.5K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:35:01_daily   56.5K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:36:01_daily   56.5K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:37:01_daily   56.5K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:38:01_daily     60K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:39:01_daily   59.5K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:40:01_daily     59K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:41:01_daily     59K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:42:01_daily     59K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:43:01_daily   58.5K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:44:01_daily     57K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:45:01_daily   57.5K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:46:01_daily   57.5K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:47:01_daily   57.5K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:48:01_daily     59K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:49:01_daily   59.5K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:50:01_daily   59.5K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:51:01_daily     59K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:52:01_daily     59K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:53:01_daily   59.5K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:54:01_daily   59.5K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:55:01_daily   58.5K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:56:01_daily   58.5K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:57:01_daily   58.5K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:58:02_daily   91.5K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_00:59:01_daily     59K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_01:00:01_hourly  90.5K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_02:00:01_hourly   696K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_03:00:01_hourly   690K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_04:00:01_hourly   691K      -  36.4G  -
vgraid/kvm/images/srv-progress@autosnap_2017-10-29_05:00:01_hourly   693K      -  36.4G  -
vgraid/kvm/images/srv-rdp@prima_installazione_progress_11           2.02G      -  37.1G  -

I will not manually prune the snapshots in order to see if the system will fill the "prune gap" by himself.

Regards

@DHowett

This comment has been minimized.

Copy link
Contributor

commented Nov 5, 2017

This happened with the US DST transition on 2017-11-05 as well.

@UltraPhil

This comment has been minimized.

Copy link

commented Nov 10, 2017

I've got the same problem (Montreal/DST) -- what's the best approach to deal with this?

I guess I could always remove them manually with the % wildcard?

Also, I just observed that my only machine that doesn't exhibit the problem is the one that doesn't run sanoid every minute.

*/5 * * * * /opt/sanoid/sanoid --cron --quiet

@jimsalterjrs

This comment has been minimized.

Copy link
Owner

commented Nov 10, 2017

You can always prune them manually, or wait until Sanoid prunes them automatically (when they fall outside your normal hourly retention window).

The creation of them is definitely a bug; there are a few corner cases where you end up with one Sanoid process creating a snapshot, then another one creating it again before the cache can be updated. This sounds like one of them. I'm considering ways to mitigate that, but TBH it's a fairly low-priority issue for me - due to the nature of snapshots, the extras really don't take any significant space, and they all go away after the retention window anyway, so it's not too big a deal.

I would like to fix the issue, don't get me wrong - but it requires a really cautious approach, since what we're talking about doing is figuring out a way to make Sanoid not take snapshots, and I've designed it very deliberately to "fail safe" when it fails. What I really don't want to happen is for Sanoid to stop taking snapshots that it should.

@phreaker0

This comment has been minimized.

Copy link
Collaborator

commented Nov 12, 2017

An easy fix would be to check if the modification time of the cache file is in the future and if so force an update. This would resolve this problem.
But the issue with skipping snapshots would remain because of the time shift. It's would be best to circumvent those corner cases instead of handling them. For example by using UTC for the snapshot scheduling logic (no daylight saving times), for the snapshot names the local time could still be used. The downside is that all configured settings (daily_hour, ...) are now UTC instead of the local time.

@UltraPhil

This comment has been minimized.

Copy link

commented Nov 12, 2017

@phreaker0: isn't it already UTC? I believe that perl's timestamp used in the sanoid cache file are from UTC epoch. But, it couldn't hurt to have the human readable DTs in ISO 8601 format, though, since you could potentially have snapshots names clash when the DST rewinds.

@jimsalterjrs: For now I simply changed my cron's entry from:
* * * * * /opt/sanoid/sanoid --cron --quiet
to
*/10 * * * * /opt/sanoid/sanoid --cron --quiet
Since my servers running sanoid every 10s didn't exhibit the issue, contrary to the ones running it every minute.
Do you think there's any downside doing this? And as a matter of fact, shouldn't we rather run Sanoid hourly, since it's the finest granularity snapshots are going to be taken, anyway?

@phreaker0

This comment has been minimized.

Copy link
Collaborator

commented Nov 12, 2017

@UltraPhil you are right, the time() call and the cache stat() command already provide UTC timestamps so I don't understand the problem with the old snapshot cache file not being updated in this case.

@phreaker0

This comment has been minimized.

Copy link
Collaborator

commented Nov 14, 2017

I think i know now what is happening, the preferredtime for the daily snapshot is 23:59 on the same day. On the 29 October this time is without DST, but the invocation time time (for example 00:00) is still with DST. The preferredtime is converted from the timezone specific format to UTC via the timelocal() function, which doesn't take the DST into account. This will lead to a negative maxAge calculation for the daily snapshot for the whole hour and therefore a daily snapshot is taken on each invocation. I reproduced this in a vm (i am in the same timezone as @gbonazzoli):

root@benchmarkVm:/opt/sanoid# date --utc --set @1509227990; date; ./sanoid --cron --verbose
Sat Oct 28 21:59:50 UTC 2017
Sat Oct 28 23:59:50 CEST 2017
INFO: taking snapshots...
taking snapshot test2@autosnap_2017-10-28_23:59:50_frequently
taking snapshot test2@autosnap_2017-10-28_23:59:50_hourly
taking snapshot test2@autosnap_2017-10-28_23:59:50_daily
taking snapshot test2@autosnap_2017-10-28_23:59:50_monthly
INFO: cache expired - updating from zfs list.
INFO: pruning snapshots...
root@benchmarkVm:/opt/sanoid# date --utc --set @1509228000; date; ./sanoid --cron --verbose
Sat Oct 28 22:00:00 UTC 2017
Sun Oct 29 00:00:00 CEST 2017
INFO: taking snapshots...
we should have had a daily snapshot of test2 -3540 seconds ago; most recent is 4 seconds old.
we should have had a hourly snapshot of test2 0 seconds ago; most recent is 5 seconds old.
taking snapshot test2@autosnap_2017-10-29_00:00:00_daily
taking snapshot test2@autosnap_2017-10-29_00:00:00_hourly
INFO: cache expired - updating from zfs list.
INFO: pruning snapshots...
root@benchmarkVm:/opt/sanoid# date --utc --set @1509228010; date; ./sanoid --cron --verbose
Sat Oct 28 22:00:10 UTC 2017
Sun Oct 29 00:00:10 CEST 2017
INFO: taking snapshots...
we should have had a daily snapshot of test2 -3530 seconds ago; most recent is 8 seconds old.
taking snapshot test2@autosnap_2017-10-29_00:00:10_daily
INFO: cache expired - updating from zfs list.

I fixed it locally with a check if the DST of the current time and the preferred time is different and if so,
adapt the UTC time by an hour. Works so far in the vm but this still leads to a duplicate daily snapshot because the day is now 25h long due the DST change and daily snapshot ist also generated if the last one is more than 24h old.

@UltraPhil There has to be something else why your servers with 10s interval didn't hit the issue, as it should create the daily snapshot on each invocation in that special hour. Easily reproducible with:

POOLNAME="test2"
zfs destroy "${POOLNAME}"@%; rm /var/cache/sanoidsnapshots.txt
date --utc --set @1509227990; date; ./sanoid --cron --verbose | grep "taking snapshot "
date --utc --set @1509228000; date; ./sanoid --cron --verbose | grep "taking snapshot "
date --utc --set @1509228010; date; ./sanoid --cron --verbose | grep "taking snapshot "
date --utc --set @1509228020; date; ./sanoid --cron --verbose | grep "taking snapshot "
date --utc --set @1509228030; date; ./sanoid --cron --verbose | grep "taking snapshot "
date --utc --set @1509228040; date; ./sanoid --cron --verbose | grep "taking snapshot "
date --utc --set @1509228050; date; ./sanoid --cron --verbose | grep "taking snapshot "
date --utc --set @1509228060; date; ./sanoid --cron --verbose | grep "taking snapshot "
date --utc --set @1509228070; date; ./sanoid --cron --verbose | grep "taking snapshot "
date --utc --set @1509228080; date; ./sanoid --cron --verbose | grep "taking snapshot "
date --utc --set @1509228090; date; ./sanoid --cron --verbose | grep "taking snapshot "
date --utc --set @1509228100; date; ./sanoid --cron --verbose | grep "taking snapshot "
date --utc --set @1509228110; date; ./sanoid --cron --verbose | grep "taking snapshot "
date --utc --set @1509228120; date; ./sanoid --cron --verbose | grep "taking snapshot "
date --utc --set @1509228130; date; ./sanoid --cron --verbose | grep "taking snapshot "
date --utc --set @1509228140; date; ./sanoid --cron --verbose | grep "taking snapshot "
date --utc --set @1509228150; date; ./sanoid --cron --verbose | grep "taking snapshot "
date --utc --set @1509228160; date; ./sanoid --cron --verbose | grep "taking snapshot "
date --utc --set @1509228170; date; ./sanoid --cron --verbose | grep "taking snapshot "
date --utc --set @1509231600; date; ./sanoid --cron --verbose | grep "taking snapshot "
date --utc --set @1509231600; date; ./sanoid --cron --verbose | grep "taking snapshot "
date --utc --set @1509314300; date; ./sanoid --cron --verbose | grep "taking snapshot "
date --utc --set @1509317940; date; ./sanoid --cron --verbose | grep "taking snapshot "
date --utc --set @1509400740; date; ./sanoid --cron --verbose | grep "taking snapshot "
date --utc --set @1509404340; date; ./sanoid --cron --verbose | grep "taking snapshot "

on each invocation a daily snapshot is generated

@UltraPhil

This comment has been minimized.

Copy link

commented Nov 15, 2017

@phreaker0 I'll check the config and timezone on each machines and try to spot other differences.

phreaker0 added a commit to phreaker0/sanoid that referenced this issue Dec 6, 2017
…shots, fixes jimsalterjrs#155
@phreaker0 phreaker0 referenced this issue Dec 6, 2017
@shodanshok

This comment has been minimized.

Copy link
Contributor

commented Jan 16, 2019

@phreaker0 @jimsalterjrs can you confirm that, apart from the multiple taken daily snapshot, using local time does not pose any additional problems? I am asking because, while I have no problem using UTC, this would be "problematic" for some customers.
Thanks.

EDIT: I just noticed that #175 should have solved the problem, by discovering the DST offset and using a "_y" hourly suffix. Am I right? Or do you suggest to use UTC anyway?

@jimsalterjrs

This comment has been minimized.

Copy link
Owner

commented Jan 17, 2019

@phreaker0

This comment has been minimized.

Copy link
Collaborator

commented Jan 17, 2019

@shodanshok, @jimsalterjrs since the dst patch was merged, one should't get extra snapshots while using local time. The only downside is that the snapshots on the DST border day are suffixed with "_y" as with local time there are two hourly snapshots with the same name/time.

@shodanshok

This comment has been minimized.

Copy link
Contributor

commented Jan 17, 2019

@shodanshok, @jimsalterjrs since the dst patch was merged, one should't get extra snapshots while using local time. The only downside is that the snapshots on the DST border day are suffixed with "_y" as with local time there are two hourly snapshots with the same name/time.

@phreaker0 @jimsalterjrs I noticed the new behavior just after posting (see edit). What I see is that any hourly snapshot in the DST "border day" will be marked with the "_y" suffix. Is this the intended behavior? On the other hand, for other way around (ie: noDST -> DST) no change should be needed/expected, right?

@shodanshok

This comment has been minimized.

Copy link
Contributor

commented Jan 17, 2019

@phreaker0 @jimsalterjrs OK, I familiarized with the problem by reading the source code and doing some tests. While I really appreciate the work done by @phreaker0 I do not like to much the "_y" suffix and the complexity of the new dstOffset code in #175. Can I suggest an alternative approach?

From my understanding, maxage should never be a negative number. This means a simple check as the one below will fill the bill:

...
# reconstruct our human-formatted most recent preferred snapshot time into an epoch time, to compare with the epoch of our most recent snapshot
my $maxage = time()-$lastpreferred;
if ($type eq 'daily' and $maxage < 0) {
    $maxage = 60*60; # adjust for DST
}
...

I did some test and it seems to work well, with upside of its extreme simplicity. The only downside is that, on the effective switch between DST -> noDST, an hourly snapshot is lost (it is not taken). That said, it happen on the middle of the night, and so should not pose any real problem. Anyone really wanting that hourly snapshot should run with UTC time instead.

Am I missing something? Does any possibility exist to revert #175 ?

@phreaker0

This comment has been minimized.

Copy link
Collaborator

commented Jan 18, 2019

@shodanshok It was not really intended for all snapshots of the day to have the suffix, but this was the version I could easily come up with. I don't like the suffix either, in the perfect case only the affected snapshot should have a suffix like "_dst". Making the code simpler would be awesome but I'm not a fan of skipping an hourly snapshot, as user this would be unexpected to me, at least this should be documented pretty good. Have you tested your solution, for example with the "tests/2_dst_handling/" script in an VM?

@shodanshok

This comment has been minimized.

Copy link
Contributor

commented Jan 18, 2019

@phreaker0 @jimsalterjrs I understand your concerns about skipping an hourly snapshot. That said, this will only happen on a single night each year. I think that clearly communication it should be enough; something similar to:

IMPORTANT NOTE: using a local timezone will result in a single hourly snapshot to be skipped during daylight->nodaylight transition. To avoid that, using UTC as timezone is recommend whenever possible.

Back to code: I run your test using the latest stable release not featuring the dstOffset patch (v1.4.18: https://github.com/jimsalterjrs/sanoid/releases/tag/v1.4.18), patching it as the above:

...
# reconstruct our human-formatted most recent preferred snapshot time into an epoch time, to compare with the epoch of our most recent snapshot
my $maxage = time()-$lastpreferred;
if ($type eq 'daily' and $maxage < 0) {
    $maxage = 60*60; # adjust for DST
}
...

I copied your test suite from the lastest master version and I issued cd 2_dst_handling; ./run.sh. The output from /tmp/sanoid_test_result seems to confirm no extra snapshots were taken. Full output is below:

sanoid-test-2@autosnap_2017-10-28_00:00:00_daily
sanoid-test-2@autosnap_2017-10-28_00:00:00_hourly
sanoid-test-2@autosnap_2017-10-28_00:00:00_monthly
sanoid-test-2@autosnap_2017-10-28_01:00:00_hourly
sanoid-test-2@autosnap_2017-10-28_02:00:00_hourly
sanoid-test-2@autosnap_2017-10-28_03:00:00_hourly
sanoid-test-2@autosnap_2017-10-28_04:00:00_hourly
sanoid-test-2@autosnap_2017-10-28_05:00:00_hourly
sanoid-test-2@autosnap_2017-10-28_06:00:00_hourly
sanoid-test-2@autosnap_2017-10-28_07:00:00_hourly
sanoid-test-2@autosnap_2017-10-28_08:00:00_hourly
sanoid-test-2@autosnap_2017-10-28_09:00:00_hourly
sanoid-test-2@autosnap_2017-10-28_10:00:00_hourly
sanoid-test-2@autosnap_2017-10-28_11:00:00_hourly
sanoid-test-2@autosnap_2017-10-28_12:00:00_hourly
sanoid-test-2@autosnap_2017-10-28_13:00:00_hourly
sanoid-test-2@autosnap_2017-10-28_14:00:00_hourly
sanoid-test-2@autosnap_2017-10-28_15:00:00_hourly
sanoid-test-2@autosnap_2017-10-28_16:00:00_hourly
sanoid-test-2@autosnap_2017-10-28_17:00:00_hourly
sanoid-test-2@autosnap_2017-10-28_18:00:00_hourly
sanoid-test-2@autosnap_2017-10-28_19:00:00_hourly
sanoid-test-2@autosnap_2017-10-28_20:00:00_hourly
sanoid-test-2@autosnap_2017-10-28_21:00:00_hourly
sanoid-test-2@autosnap_2017-10-28_22:00:00_hourly
sanoid-test-2@autosnap_2017-10-28_23:00:00_hourly
sanoid-test-2@autosnap_2017-10-29_00:00:00_daily
sanoid-test-2@autosnap_2017-10-29_00:00:00_hourly
sanoid-test-2@autosnap_2017-10-29_01:00:00_daily
sanoid-test-2@autosnap_2017-10-29_01:00:00_hourly
sanoid-test-2@autosnap_2017-10-29_02:00:00_hourly
sanoid-test-2@autosnap_2017-10-29_03:00:00_hourly
sanoid-test-2@autosnap_2017-10-29_04:00:00_hourly
sanoid-test-2@autosnap_2017-10-29_05:00:00_hourly
sanoid-test-2@autosnap_2017-10-29_06:00:00_hourly
sanoid-test-2@autosnap_2017-10-29_07:00:00_hourly
sanoid-test-2@autosnap_2017-10-29_08:00:00_hourly
sanoid-test-2@autosnap_2017-10-29_09:00:00_hourly
sanoid-test-2@autosnap_2017-10-29_10:00:00_hourly
sanoid-test-2@autosnap_2017-10-29_11:00:00_hourly
sanoid-test-2@autosnap_2017-10-29_12:00:00_hourly
sanoid-test-2@autosnap_2017-10-29_13:00:00_hourly
sanoid-test-2@autosnap_2017-10-29_14:00:00_hourly
sanoid-test-2@autosnap_2017-10-29_15:00:00_hourly
sanoid-test-2@autosnap_2017-10-29_16:00:00_hourly
sanoid-test-2@autosnap_2017-10-29_17:00:00_hourly
sanoid-test-2@autosnap_2017-10-29_18:00:00_hourly
sanoid-test-2@autosnap_2017-10-29_19:00:00_hourly
sanoid-test-2@autosnap_2017-10-29_20:00:00_hourly
sanoid-test-2@autosnap_2017-10-29_21:00:00_hourly
sanoid-test-2@autosnap_2017-10-29_22:00:00_hourly
sanoid-test-2@autosnap_2017-10-29_23:00:00_hourly
sanoid-test-2@autosnap_2017-10-30_00:00:00_daily
sanoid-test-2@autosnap_2017-10-30_00:00:00_hourly
sanoid-test-2@autosnap_2017-10-30_01:00:00_hourly
sanoid-test-2@autosnap_2017-10-30_02:00:00_hourly
sanoid-test-2@autosnap_2017-10-30_03:00:00_hourly
sanoid-test-2@autosnap_2017-10-30_04:00:00_hourly
sanoid-test-2@autosnap_2017-10-30_05:00:00_hourly
sanoid-test-2@autosnap_2017-10-30_06:00:00_hourly
sanoid-test-2@autosnap_2017-10-30_07:00:00_hourly
sanoid-test-2@autosnap_2017-10-30_08:00:00_hourly
sanoid-test-2@autosnap_2017-10-30_09:00:00_hourly
sanoid-test-2@autosnap_2017-10-30_10:00:00_hourly
sanoid-test-2@autosnap_2017-10-30_11:00:00_hourly
sanoid-test-2@autosnap_2017-10-30_12:00:00_hourly
sanoid-test-2@autosnap_2017-10-30_13:00:00_hourly
sanoid-test-2@autosnap_2017-10-30_14:00:00_hourly
sanoid-test-2@autosnap_2017-10-30_15:00:00_hourly
sanoid-test-2@autosnap_2017-10-30_16:00:00_hourly
sanoid-test-2@autosnap_2017-10-30_17:00:00_hourly
sanoid-test-2@autosnap_2017-10-30_18:00:00_hourly
sanoid-test-2@autosnap_2017-10-30_19:00:00_hourly
sanoid-test-2@autosnap_2017-10-30_20:00:00_hourly
sanoid-test-2@autosnap_2017-10-30_21:00:00_hourly
sanoid-test-2@autosnap_2017-10-30_22:00:00_hourly
sanoid-test-2@autosnap_2017-10-30_23:00:00_hourly

From how I understand the code (correct me if I am wrong!), maxage is never suppose to be negative (being time()-$lastpreferred). So, it should pose no problems to set it to 1 hour ($maxage = 60*60;) in case of a negative value, especially when only adjusted if a daily snapshot was requested.

Any comment/thought is welcomed!

@phreaker0

This comment has been minimized.

Copy link
Collaborator

commented Jan 18, 2019

@shodanshok
seems like you overlooked the duplicate daily snapshot in the output:

sanoid-test-2@autosnap_2017-10-29_00:00:00_daily
sanoid-test-2@autosnap_2017-10-29_00:00:00_hourly
sanoid-test-2@autosnap_2017-10-29_01:00:00_daily
@shodanshok

This comment has been minimized.

Copy link
Contributor

commented Jan 18, 2019

@phreaker0 @jimsalterjrs Good catch, I missed it! Still a good improvement over the original 60 additional snapshot, but clearly and imperfect solution.

Let's try to do what seems the right thing to me: using perl's buildin time function, rather than playing with a variable-length (!) day. I reverted the maxage hack, applying this patch instead:

[root@left scripts]# diff sanoid-1.4.18/sanoid sanoid/sanoid
311c311,314
<                                       if ($lastpreferred > time()) { $lastpreferred -= 60*60*24; } # preferred time is later today - so look at yesterday's
---
>                                       if ($lastpreferred > time()) {
>                                               $preferredtime[3] -= 1; # preferred time is later today - so look at yesterday's
>                                               $lastpreferred = timelocal(@preferredtime);
>                                       }

cd 2_dst_handling; ./run.sh returns the following output:

sanoid-test-2@autosnap_2017-10-28_00:00:00_daily
sanoid-test-2@autosnap_2017-10-28_00:00:00_hourly
sanoid-test-2@autosnap_2017-10-28_00:00:00_monthly
sanoid-test-2@autosnap_2017-10-28_01:00:00_hourly
sanoid-test-2@autosnap_2017-10-28_02:00:00_hourly
sanoid-test-2@autosnap_2017-10-28_03:00:00_hourly
sanoid-test-2@autosnap_2017-10-28_04:00:00_hourly
sanoid-test-2@autosnap_2017-10-28_05:00:00_hourly
sanoid-test-2@autosnap_2017-10-28_06:00:00_hourly
sanoid-test-2@autosnap_2017-10-28_07:00:00_hourly
sanoid-test-2@autosnap_2017-10-28_08:00:00_hourly
sanoid-test-2@autosnap_2017-10-28_09:00:00_hourly
sanoid-test-2@autosnap_2017-10-28_10:00:00_hourly
sanoid-test-2@autosnap_2017-10-28_11:00:00_hourly
sanoid-test-2@autosnap_2017-10-28_12:00:00_hourly
sanoid-test-2@autosnap_2017-10-28_13:00:00_hourly
sanoid-test-2@autosnap_2017-10-28_14:00:00_hourly
sanoid-test-2@autosnap_2017-10-28_15:00:00_hourly
sanoid-test-2@autosnap_2017-10-28_16:00:00_hourly
sanoid-test-2@autosnap_2017-10-28_17:00:00_hourly
sanoid-test-2@autosnap_2017-10-28_18:00:00_hourly
sanoid-test-2@autosnap_2017-10-28_19:00:00_hourly
sanoid-test-2@autosnap_2017-10-28_20:00:00_hourly
sanoid-test-2@autosnap_2017-10-28_21:00:00_hourly
sanoid-test-2@autosnap_2017-10-28_22:00:00_hourly
sanoid-test-2@autosnap_2017-10-28_23:00:00_hourly
sanoid-test-2@autosnap_2017-10-29_00:00:00_daily
sanoid-test-2@autosnap_2017-10-29_00:00:00_hourly
sanoid-test-2@autosnap_2017-10-29_01:00:00_hourly
sanoid-test-2@autosnap_2017-10-29_02:00:00_hourly
sanoid-test-2@autosnap_2017-10-29_03:00:00_hourly
sanoid-test-2@autosnap_2017-10-29_04:00:00_hourly
sanoid-test-2@autosnap_2017-10-29_05:00:00_hourly
sanoid-test-2@autosnap_2017-10-29_06:00:00_hourly
sanoid-test-2@autosnap_2017-10-29_07:00:00_hourly
sanoid-test-2@autosnap_2017-10-29_08:00:00_hourly
sanoid-test-2@autosnap_2017-10-29_09:00:00_hourly
sanoid-test-2@autosnap_2017-10-29_10:00:00_hourly
sanoid-test-2@autosnap_2017-10-29_11:00:00_hourly
sanoid-test-2@autosnap_2017-10-29_12:00:00_hourly
sanoid-test-2@autosnap_2017-10-29_13:00:00_hourly
sanoid-test-2@autosnap_2017-10-29_14:00:00_hourly
sanoid-test-2@autosnap_2017-10-29_15:00:00_hourly
sanoid-test-2@autosnap_2017-10-29_16:00:00_hourly
sanoid-test-2@autosnap_2017-10-29_17:00:00_hourly
sanoid-test-2@autosnap_2017-10-29_18:00:00_hourly
sanoid-test-2@autosnap_2017-10-29_19:00:00_hourly
sanoid-test-2@autosnap_2017-10-29_20:00:00_hourly
sanoid-test-2@autosnap_2017-10-29_21:00:00_hourly
sanoid-test-2@autosnap_2017-10-29_22:00:00_hourly
sanoid-test-2@autosnap_2017-10-29_23:00:00_hourly
sanoid-test-2@autosnap_2017-10-30_00:00:00_daily
sanoid-test-2@autosnap_2017-10-30_00:00:00_hourly
sanoid-test-2@autosnap_2017-10-30_01:00:00_hourly
sanoid-test-2@autosnap_2017-10-30_02:00:00_hourly
sanoid-test-2@autosnap_2017-10-30_03:00:00_hourly
sanoid-test-2@autosnap_2017-10-30_04:00:00_hourly
sanoid-test-2@autosnap_2017-10-30_05:00:00_hourly
sanoid-test-2@autosnap_2017-10-30_06:00:00_hourly
sanoid-test-2@autosnap_2017-10-30_07:00:00_hourly
sanoid-test-2@autosnap_2017-10-30_08:00:00_hourly
sanoid-test-2@autosnap_2017-10-30_09:00:00_hourly
sanoid-test-2@autosnap_2017-10-30_10:00:00_hourly
sanoid-test-2@autosnap_2017-10-30_11:00:00_hourly
sanoid-test-2@autosnap_2017-10-30_12:00:00_hourly
sanoid-test-2@autosnap_2017-10-30_13:00:00_hourly
sanoid-test-2@autosnap_2017-10-30_14:00:00_hourly
sanoid-test-2@autosnap_2017-10-30_15:00:00_hourly
sanoid-test-2@autosnap_2017-10-30_16:00:00_hourly
sanoid-test-2@autosnap_2017-10-30_17:00:00_hourly
sanoid-test-2@autosnap_2017-10-30_18:00:00_hourly
sanoid-test-2@autosnap_2017-10-30_19:00:00_hourly
sanoid-test-2@autosnap_2017-10-30_20:00:00_hourly
sanoid-test-2@autosnap_2017-10-30_21:00:00_hourly
sanoid-test-2@autosnap_2017-10-30_22:00:00_hourly
sanoid-test-2@autosnap_2017-10-30_23:00:00_hourly

No more duplicate daily snapshot. The only thing missing a single hourly snapshot (with duplicate 02:00:00 time).

@shodanshok

This comment has been minimized.

Copy link
Contributor

commented Jan 21, 2019

@phreaker0 @jimsalterjrs Hi all, any chances to review the patch above? Thanks.

@phreaker0

This comment has been minimized.

Copy link
Collaborator

commented Jan 25, 2019

@jimsalterjrs your call. I'm happy if this get's merged as long as it's clearly documented that an hour will be missed on DST border.

@shodanshok

This comment has been minimized.

Copy link
Contributor

commented Feb 4, 2019

@jimsalterjrs any chances to review the proposed patch?

@shodanshok

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2019

@jimsalter @jimsalterjrs @phreaker0 sorry for the bump ... any possibilities to check the proposed patch? Should I open a new PR? Thanks.

@shodanshok

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2019

@jimsalter @jimsalterjrs @phreaker0 I've opened a PR, give it a look if/when you want. Thanks.

phreaker0 added a commit to phreaker0/sanoid that referenced this issue Jun 14, 2019
…#155)"

This reverts commit cf496dc.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.