Failed backups cleanup

[mantissahz]

Clean up failed backups when there is a backup in state Error or Unknown.

longhorn/longhorn#3898 longhorn/longhorn#4334

Signed-off-by: James Lu [email protected]

[innobead]

In general, LGTM. Have some feedback, please resolve others' comments as well before the merge.

P.S. this needs longhorn/longhorn change as well because a new setting will be introduced.

[codecov[bot]]

Codecov Report

Merging #1477 (83a7cbb) into master (feb8809) will decrease coverage by 0.00%. The diff coverage is 0.00%.

:exclamation: Current head 83a7cbb differs from pull request most recent head 0d720cb. Consider uploading reports for the commit 0d720cb to get more accurate results

@@            Coverage Diff             @@
##           master    #1477      +/-   ##
==========================================
- Coverage   15.28%   15.27%   -0.01%     
==========================================
  Files          50       50              
  Lines       18514    18525      +11     
==========================================
  Hits         2829     2829              
- Misses      15223    15234      +11     
  Partials      462      462              

Impacted Files	Coverage Δ
controller/backup_volume_controller.go	0.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[mantissahz]

Not sure why you prefer to delete the latter part... I think we should retain it but change the log level to warning instead.

@shuo-wu did you mean to keep this part "and it will skip the auto-deletion for the failed backups?"

[shuo-wu]

@shuo-wu did you mean to keep this part "and it will skip the auto-deletion for the failed backups?"

Right. This part is what I suggest in the previous comment. But remember to modify the log level.

cc @derekbit What's the reason for removing this part?

[derekbit]

@shuo-wu did you mean to keep this part "and it will skip the auto-deletion for the failed backups?"

Right. This part is what I suggest in the previous comment. But remember to modify the log level.

cc @derekbit What's the reason for removing this part?

Ah, sorry. I was not aware of that is your suggestion. Just thought the simple error message is enough for debugging. But yes, we can keep it ("and it will skip the auto-deletion for the failed backups") here.

[shuo-wu]

Ah, sorry. I was not aware of that is your suggestion. Just thought the simple error message is enough for debugging. But yes, we can keep it ("and it will skip the auto-deletion for the failed backups") here.

Never mind. I think it can help avoid a potential misunderstanding that this error will fail the reconciliation

[innobead]

@mantissahz Please backport this to 1.3.x and 1.2.x.