[bug] Week retention not working

[vejnar]

Thanks for writing and maintaining this. I am testing it with success so far, before using it in production. But I have a problem with retention as it seems not to behave as I would except, if I understand the help correctly. In particular, the "week" retention.

For example, these snapshots are tagged/not-tagged using 4 leafs, 3 days, 4 weeks, 12 months and 5 years:

2021/06/26/152400 not tagged
2021/06/26/142400 not tagged
2021/06/24/152400 not tagged
2021/06/24/142400 not tagged
2021/06/23/152400 not tagged
2021/06/22/152400 tagged
2021/06/21/152400 tagged
2021/06/21/142400 tagged
2021/06/20/152400 tagged
2021/06/20/142400 tagged
2021/06/19/152400 tagged
2021/06/19/142400 tagged
2021/06/14/152400 tagged
2021/06/13/152400 tagged
2021/06/12/152400 tagged

I would except (13 and 20 are Sunday):

2021/06/26/152400 not tagged
2021/06/26/142400 not tagged
2021/06/24/152400 not tagged
2021/06/24/142400 not tagged
2021/06/23/152400 not tagged
2021/06/22/152400 tagged
2021/06/21/152400 tagged
2021/06/21/142400 tagged
2021/06/20/152400 not tagged
2021/06/20/142400 tagged
2021/06/19/152400 tagged
2021/06/19/142400 tagged
2021/06/14/152400 tagged
2021/06/13/152400 not tagged
2021/06/12/152400 tagged

To obtain this result, I replaced:

[[ -n "$lw" ]] && (( $(date -d "$lw UTC" +%s) - $(date -d "$y/$m/$d UTC" +%s) / (60 * 60 * 24) > 6 )) \
&& (( tag &= retention[weeks]-- < 1 ))
lw=$(date -d "$y/$m/$d -$(date -d $y/$m/$d +%u) days +$weekstart day" +'%F')

by

cw=$(date -d "$y/$m/$d" +'%Y%V')
(( $lw != $cw && (lw = cw, tag &= retention[weeks]-- < 1) ))

• I replace your calculation with a variable keeping the last week (lw) saved and using date to get the current week number (cw)
• Replacing +'%Y%V' by +'%Y%U' would use Sunday as first day of the week instead of Monday
• I defined lw='000000' before the loop

To test my code, I added this code at the end of the backup script:

echo """/2021/06/26/152400
/2021/06/26/142400
/2021/06/24/152400
/2021/06/24/142400
/2021/06/23/152400
/2021/06/22/152400
/2021/06/21/152400
/2021/06/21/142400
/2021/06/20/152400
/2021/06/20/142400
/2021/06/19/152400
/2021/06/19/142400
/2021/06/14/152400
/2021/06/13/152400
/2021/06/12/152400""" | sort -dr | tokenize_path | check_retention --all | rotate_test

Does this make sense to you (if yes I can submit a PR)? Am I missing something?

[3coma3]

Hi @vejnar, thanks a lot for spotting this. I'm about to attempt confirmation, but on first sight it does look like a bug.

Moreover I think your code is simpler and more effective:

1. I avoided using %V / %U, turning instead to factor in a number between 0 and 7 with manual calculation of timestamps. I likely did this in order "to be more flexible", but that yielded arguably the ugliest part of the retention checking code. For nothing really, because there is no country in the world where the first day of the week isn't either a Sunday or a Monday :-)

2. I formatted the dates for the manual calculation with UTC. I have the feeling that should cause problems too, when computing for snapshots that were made in any other timezone but UTC (unconfirmed).

The replacement is perfect, I would only like to see a way to still have it plugged to a config variable to opt between Sunday and Monday. There's an idiom I've been using in this code for this kind of simple ternaries, I think we could have something like:

cw=$(date -d "$y/$m/$d" +"%Y$(cond "$weekstart = monday" %V %U)")

I'll be testing something like this, and would also like to find out what's the root problem with the timestamp based code, but because of the second point above this is really a secondary exercise, I think the code is just flawed.

Also, the way you tested for this highlights a shortcoming with cmd_test(): lack of a way to test regular schedules for backups. This is already in queue (see #24) to be added when I find a little time.

[3coma3]

I pushed a preliminary patch in 06caa4f (fix-week-retention branch). Could you please test / review?

[vejnar]

Hi @3coma3. Thanks for fixing this so fast! Problem I was having is solved.

My review for the patch:

• I understand keeping 1 and 7 for compatibility but I wouldn't add so many more names (s, sun etc) for the config. I think sunday and monday are enough.
• Add a comment to indicate this section of code checks the week number.

[3coma3]

Hi @3coma3. Thanks for fixing this so fast! Problem I was having is solved.

My review for the patch:

• I understand keeping 1 and 7 for compatibility but I wouldn't add so many more names (s, sun etc) for the config. I think sunday and monday are enough.
• Add a comment to indicate this section of code checks the week number.

Done! I like (as a user) to have more options, but let's go with "less is more" (which is good advice btw)

Thanks a lot for the help and reporting!