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

Anything worth pulling in from my fork? #1

Closed
wezm opened this issue Oct 2, 2021 · 5 comments
Closed

Anything worth pulling in from my fork? #1

wezm opened this issue Oct 2, 2021 · 5 comments
Assignees
Labels
enhancement New feature or request

Comments

@wezm
Copy link
Contributor

wezm commented Oct 2, 2021

Hi, I've made a bunch of tweaks in my fork of your project. I understand that I kinda went to town on the code so my changes might not align with your original goals. Nonetheless I tried to make the commits self-contained so if any of these are of interest to you let me know and I can raise one or more PRs.

(oldest first):

  • 5af40ff Remove extern crates declarations (not needed in Rust 2018)
  • 9b26b73 Address cargo warning
  • 30c0a2a Remove unnecessary to_string call
  • a107dca Make RetentionPolicy::parse a FromStr impl
  • e6565cb Remove unnecessary copy of snapshots in check_age
  • 9c8e655 Use lines and is_empty in _call_read
  • a1e14a1 Construct age check result at end of gc_find
  • 7b63abf Avoid copying SnapshotMetadata in gc_find
  • fe9a103 Add tests for RetentionPolicy::from_str
  • f1722bd Drop regex and lazy_static dependencies
  • 5fdf101 Drop anyhow dependency
  • 1896827 Extract _parse_snapshots function and add tests
  • e9da9d6 Replace unwraps and panics with errors
  • 7bc4201 Tidy up check_age
  • 58c14c7 Split library code out of the binary
  • 2c4e878 Make check_age a method of RetentionPolicy
  • 1f53496 Tidy up main function
  • b0a83ad Add CI
  • f01c0257 Implement cargo clippy suggestions
@rollcat
Copy link
Owner

rollcat commented Nov 4, 2021

Hey! Sorry, my email client ate the notification. This is a lot of really excellent work - thank you a million. I consider the tool feature complete, but this is exactly the kind of review, cleanup & polish it needed.

I've cherry-picked the commits I liked (almost everything) and reverted only two lines - I find continue next_rule; easier to follow ;)

Are you currently using zfs-autosnap in production? Have you had any issues, found any rough edges?

@rollcat rollcat self-assigned this Nov 4, 2021
@rollcat rollcat added the enhancement New feature or request label Nov 4, 2021
@wezm
Copy link
Contributor Author

wezm commented Nov 4, 2021

Are you currently using zfs-autosnap in production? Have you had any issues, found any rough edges?

Only on my desktop. I run Linux (Arch) and my home directory is on a ZRAID pool of 3 SSDs. I started running my fork of zfs-autosnap on it a month ago, scheduled with the systemd timers in my repo and it has been working fine. I needed to use a snapshot to restore a file merely days after setting it up too.

@Ramalama2
Copy link

Hey boys, im using your versions, actually @wezm one in production, instead of the official autosnap tools.
Thought that wezms one is a bit newer simply.
So far everything works great 👍

I would only suggest only 1 improvement:

  1. You can limit hourly snapshots to let's say 6 per day.
    With systemd service or a cronjob, you can simply change the interval to every 4 hours, for example with systemd "OnCalendar=00/4:00"
    So you get spreaded daily snapshots, 6 of them every 4 hours.
    Would be actually great that someone is able to spread the same way daily snapshots.
    Let's say, you do 5 weekly (every day for the last 5 days), and 14 monthly: but that 14 will get spreaded over the month (like with hourlys above), because you have already a weekly variable.

Now, actually this is a bit confusing: let's say i do: 7/week and 14/month, so you get of the last week 7 snapshots + 14daly snapshots per month.

Doesn't that mean that i get simply the last 14 daily snapshots of the month? And the weekly variable can be ignored?

That's what im finding a little confusing and where i think that the documentation needs a little improvement.
I mean about the weekly variable.

Im running the utility about 4 days now, so i actually need to wait a bit to see what the actual behavior is with the weekly ones.
But when i change the variable to experiment what happens, ill need to wait forever 😂

So i thought to write here!
Thanks btw for the tool, i do like it a lot, because you can set different routines for datasets in a very nice and visible way, instead of fu*king with 6 different variables per dataset😂

@rollcat
Copy link
Owner

rollcat commented Dec 19, 2022

Hi @Ramalama2, you should probably open a new issue for questions. I will close this one as after merging @wezm's changes the forks are pretty much identical (and he's kind enough to provide automated builds).

Trying to answer your questions below:

You can limit hourly snapshots to let's say 6 per day.
With systemd service or a cronjob, you can simply change the interval to every 4 hours, for example with systemd "OnCalendar=00/4:00"
So you get spreaded daily snapshots, 6 of them every 4 hours.
Would be actually great that someone is able to spread the same way daily snapshots.

The whole point of this utility is it does not differentiate, which snapshot is supposed to be "hourly" or "daily" - it just takes a snapshot whenever instructed to (e.g. by cron, anacron, systemd timer, manually, while sleep $RANDOM; do zfs-autosnap snap; done, however you choose to run it). That's it - you can take snapshots as (in)frequently as you wish to, on any schedule you like, it's all 100% up to you, and how you configure your schedules. You don't even need to take the snapshots with this utility - as long as they inherit the at.rollc.at:snapkeep property (which happens by default even on 100% manual snapshots), they will be considered for automatic GC.

This is also why you do zfs-autosnap gc in a separate step, and decide on your own retention policy. The retention policy will just decide which snapshots to keep:

Now, actually this is a bit confusing: let's say i do: 7/week and 14/month, so you get of the last week 7 snapshots + 14daly snapshots per month.

Doesn't that mean that i get simply the last 14 daily snapshots of the month? And the weekly variable can be ignored?

The "weekly" retention policy does not mean "X daily snapshots in the last week", it means "consider the ISO week number and the the year as the bucket, limit the number of buckets to 7, put each snapshot into it's week's bucket, and keep 1 freshest snapshot from each bucket" - so with your policy (7w14m), you will retain 7 snapshots (one from each Sunday: Dec 18th, Dec 11th, Dec 4th, etc), plus 14 more snapshots (one from each last day of the month: Oct 30th, Nov 31st, Sep 30th...), not just the last 14 days (that would be a 14d policy).

I suggest that you turn off GC on your machine for a couple of days, weeks or months (as space allows), and run zfs-autosnap status at different points in time, to see what it WOULD do. Also browse the source for the function check_age, it's written to be as readable as I could make it, and includes a fair bit of commentary that explains what's going on at each step.

That's what im finding a little confusing and where i think that the documentation needs a little improvement.

Yes, as you may have noticed from the README, it's a quick & dirty rewrite of a quick&dirty old script that just happens to have caused exactly 0 data loss problems (and prevented MANY) in the 6+ years that I've been running it ;) I might eventually get to properly documenting and explaining this, but you are more than welcome to give it a try yourself.

Closing the issue, if you'd like to follow up please open a new one (or feel free to email me).

@rollcat rollcat closed this as completed Dec 19, 2022
@Ramalama2
Copy link

Thank you very much for explaining!
I do what you said, with turning off gc and wait a month or so, to check with status what gc would delete/keep.
That's very clever, thx!

Otherwise i do like the utility, it's great, thanks for the work and sorry for posting here 🙈

I can do the documentation pull request myself, in a month or so.
Thx & Cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants