jenkinsci / kubernetes-operator

Kubernetes native Jenkins Operator
https://jenkinsci.github.io/kubernetes-operator
Other
591 stars 231 forks source link

fix(backup): use atomic mv to create backup #1000

Closed skillcoder closed 2 months ago

skillcoder commented 2 months ago

Changes

use atomic mv to create backup to avoid broken unrestorable backups

Details

% k exec jenkins-jenkins -c backup -- df -hT | grep -E "overlay|ext4"
overlay        overlay  110G   23G   88G  21% /
/dev/nvme2n1   ext4      64G   23G   42G  36% /backup

https://github.com/jenkinsci/kubernetes-operator/blob/cf49a4a28fbb1af5250031ce89b0a5267d81aca1/backup/pvc/bin/backup.sh#L8C1-L8C15

BACKUP_TMP_DIR=$(mktemp -d)

https://www.gnu.org/software/autogen/mktemp.html

      --tmpdir[=DIR]  interpret TEMPLATE relative to DIR.  If DIR is not
                        specified, use $TMPDIR if set, else /tmp.  With
                        this option, TEMPLATE must not be an absolute name.
                        Unlike with -t, TEMPLATE may contain slashes, but
                        mktemp creates only the final component

https://www.gnu.org/software/coreutils/manual/html_node/mv-invocation.html#mv-invocation

To move a file, mv ordinarily simply renames it. However, if renaming does not work because the destination's file system differs, mv falls back on copying as if by cp -a, then (assuming the copy succeeded) it removes the original.

To fix need just change this line to something like this

BACKUP_TMP_DIR=$(mktemp -d --tmpdir=${BACKUP_DIR})

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you review them:

See the contribution guide for more details.

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS.

Release Notes

- backup creating on the same filesystem as BACKUP_DIR, to avoid broken backups during copy.
brokenpip3 commented 2 months ago

This is a great contribution, ty! :rocket:

However this change will impact on the backup pvc size since it will need to also contains the temporary tar.gz, can you please also add line about this here in doc?

After that I will immediately merge this PR, Thanks!

brokenpip3 commented 2 months ago

This is great, ty!