feat: expose last on retain options for restic mover

[onedr0p]

Describe what this PR does

Allows users to set last as forget option in the ReplicationDestination CR.

Is there anything that requires special attention?

I believe I hit all the right spots in the code as the functionality was mainly there just not exposed as an option.

Related issues:

#557

[openshift-ci]

Hi @onedr0p. Thanks for your PR.

I'm waiting for a backube member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[tesshuflower]

@JohnStrunk what do you think about allowing restic keep-last? Let me know if you're ok with this one and I could help add a unit test as well.

[codecov]

Codecov Report

Merging #797 (fe0b546) into main (7dfdeda) will decrease coverage by 0.1%.
The diff coverage is 0.0%.

❗ Current head fe0b546 differs from pull request most recent head 00cec82. Consider uploading reports for the commit 00cec82 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##            main    #797     +/-   ##
=======================================
- Coverage   66.8%   66.8%   -0.1%     
=======================================
  Files         55      55             
  Lines       7055    7058      +3     
=======================================
  Hits        4716    4716             
- Misses      2059    2061      +2     
- Partials     280     281      +1     

Impacted Files	Coverage Δ
controllers/mover/restic/mover.go	81.5% <0.0%> (-0.6%)	⬇️

[JohnStrunk]

My concern with this is if the backup portion succeeds, but something else (like the forget) fails such that the operator thinks the container failed. This can cause the operator to loop, retrying backups. The result could end up being a large number of backups in rapid succession.
"keep last" would cause the older backups to get purged, leaving the user with all backups from approximately the same time.
This is why I didn't enable it originally.

Having said that, the current structure of the restic script makes that fairly unlikely since all it does is a forget right after the backup.

Honestly, I'm on the fence w/ whether to take this because it's dangerous for the user, in a way that isn't obvious.

[JohnStrunk]

This also needs a make generate added to the commit to fix the failed test

[tesshuflower]

fixed the make generate, but I do think @JohnStrunk your concerns are valid about potentially losing older snapshots unintentionally.

[JohnStrunk]

/ok-to-test

[onedr0p]

This can cause the operator to loop, retrying backups. The result could end up being a large number of backups in rapid succession.

I've seen then happen on my end once or twice but maybe it's worth putting in the docs to use last only with another type like daily so that at least if the backup loop happens at least daily will be retained?

retain:
  last: 10
  daily: 7

[onedr0p]

@JohnStrunk with that said if last is supported and the backup loop happens maybe at least it won't flood the restic repo with hundreds or more snapshots? I've seen that happen a few times on my instance and it was due to lock errors on the restic repo. I had to manually purge old snapshots a few times on a repo due to that.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[JohnStrunk]

/approve
/lgtm

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JohnStrunk, onedr0p

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [JohnStrunk]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment