Don't fail backup deletion if downloading tarball fails

[zubron]

Previously, we would always attempt to download the tarball for a backup
for processing DeleteItemAction plugins, even if there weren't any.
This caused an issue for some users in the case where the backup tarball
had been deleted from object storage as the backup deletion would fail.

Now, we only attempt to download the tarball in the case where there are
DeleteItemAction plugins. If downloading that tarball fails, we log
the error, skip the processing of the DeleteItemAction plugins and
proceed with the rest of the deletion.

Fixes #2980

Signed-off-by: Bridget McErlean [email protected]

[ashish-amarnath]

I think the correct fix is to not delete the backup from the object store if there were any errors in the process of deleting the backup.

refer #3094 (comment)

[zubron]

@ashish-amarnath #3094 is related but I think this PR and the issue it is addressing are different enough that they should be considered separately. This PR will only deal with the case where there are DeleteItemAction plugins to run and the backup tarball doesn't exist. The fact that a backup deletion fails due to missing cloud resources is a change in behaviour from 1.4 so this PR was to restore the previous behaviour. For example, if someone manually removes the backup tarball from their object storage or the backup tarball was never written in the first place, then attempting to delete those backups through velero will always result in an error and the deletion will fail.

In #3094, the issue is that Velero should not have deleted any backup resources in the first place if there was an error. They are related, in that the error described in #2980 will appear on following deletion attempts, but I still think they are distinct issues. I think the issue of orphaned resources described in #3094 would have existed prior to the introduction of DeleteItemActions.

All that said, I think we need both this change and the one you mentioned in #3094 😄

[nrb]

Should this defer be in the else block? I don't think the backupFile variable exists here, unless I'm reading something wrong.

[zubron]

It's defined on 307. I put the defer here thinking that we shouldn't attempt to close and remove it unless it was successfully created, but I should check the implementation of both functions. downloadToTempFile might create the file and not write to it, or closeAndRemoveFile might be able to handle the case where it doesn't exist and in that case it would be safe to move it to the outer scope where backupFile was defined.

[zubron]

downloadToTempFile returns a nil file pointer in the case where there was an error. This means that closeAndRemoveFile will panic as it was passed nil. I can leave the defer in the else block or update closeAndRemoveFile to handle the case where the file is nil. I'm leaning towards the latter option but let me know what you think!

[nrb]

Got it, thanks for the detailed reply. I was actually confusing the ifs and thought the else was part of the len(actions) > 0 condition.

I think updating closeAndRemoveFile to handle a nil file is a good idea. That way, we can move the defer right after the downloadToTempFile call, which is more idiomatic Go, where the defer is put right after the creation or fetching of the resource.

The other option is a short comment above the defer statement where it's at now to explain why, but I think updating closeAndRemoveFile is the better choice.

[nrb]

I think we need both fixes as well. If nothing else, this is an optimization that we won't try to download the backup tarball when we don't need to act on it.

[nrb]

Thanks for your patience @zubron :)

@ashish-amarnath Could you please revisit your review?

[ashish-amarnath]

🚀

[haslersn]

When will this fix be released?