Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
May 5, 2024

Conversation

skillcoder
Copy link

@skillcoder skillcoder commented Apr 12, 2024

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.

@skillcoder skillcoder changed the title use atomic mv to create backup fix(backup): use atomic mv to create backup Apr 12, 2024
@brokenpip3
Copy link
Collaborator

This is a great contribution, ty! 🚀

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
Copy link
Collaborator

This is great, ty!

@brokenpip3 brokenpip3 enabled auto-merge (squash) May 5, 2024 14:52
@brokenpip3 brokenpip3 merged commit 25b329a into jenkinsci:master May 5, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants