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

revert to old dst handling and make it nice #406

Merged
merged 2 commits into from Sep 25, 2019

Conversation

@phreaker0
Copy link
Collaborator

commented Jun 14, 2019

This PR is a follow up to #175 and makes it basically complete by only suffixing the duplicate snapshot name caused by the DST hour rollback. It's an alternative to the proposed #362, #395. This one seems to work correctly (based on the long running tests and it's currently used in the last release) without any duplicate or missing snapshots but of course the dst handling leads to code which is more difficult to read and understand.

The snapshots surrounding an DST change by rolling back one hour looks like this after this patch:

...
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_02:00:00dst_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
...

Fixes #394
Closes #395

phreaker0 added 2 commits Jun 13, 2019
This reverts commit cf496dc.
… DST change
@jimsalterjrs

This comment has been minimized.

Copy link
Owner

commented Jun 14, 2019

Should I merge #395 first? Or should this be merged and #395 rejected?

@gdevenyi

This comment has been minimized.

Copy link

commented Jun 14, 2019

I wrote up a small proposal at #409 about using the DateTime module to handle comparing snapshots using standardized comparions and reproducible checks. Please take a look.

@phreaker0

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 17, 2019

@jimsalterjrs Thats your decision to make (both @shodanshok and me are biased), either this or #395 should be merged but not both. This PR solves the problems correctly for the user (no duplicate snapshots and no lost hourly snapshots for local timezone users) in contrast to @shodanshok solution which is simpler but an hourly snapshot is losts and there is at least one duplicate daily snapshot.

@phreaker0

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 25, 2019

@jimsalterjrs there was the idea from @gdevenyi in #410 to just include the hour shift information to the snapshot name if local time is used. This would eliminate the complete dst handling code entirely and it would become a lot easier. Furthermore no suffixes are needed because there can't be a duplicate snapshot, as the hour shift changes on DST boundary. Would this be acceptable to you? To sum it up:

If UTC is used, nothing would change:
2019-06-19T16:43:47
But if local time is used it would look like:
2019-06-19T12:43:47-04:00

@shodanshok

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2019

@phreaker0 @gdevenyi The idea is indeed very good. However it will change the snapshot name format, which can have a non-trivial impact on the old-snapshot-trimmig code. Maybe this can be queued for a new sanoid point release, eg: 2.1.x or 3.x.x, keeping a less invasive change for 2.0.x? @jimsalterjrs any thoughts?

@phreaker0

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 25, 2019

The old snapshot trimming code should still handle the old format as if is would be in utc format, this way no snapshots are left behind and the schedule error is only up to 12/24? hours big for pruning.

@skrenes

This comment has been minimized.

Copy link

commented Sep 1, 2019

Can we please get this or the other PR merged? It's been months with this serious problem.
Edit: I definitely prefer this approach as it allows one to realistically use local time instead of UTC, which makes management much easier (I hate having to subtract hours to figure out local time, especially living in a DST timezone where that number changes twice a year).

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