Skip clearing repositories for exclusion-based backups

Anton Smithrequested to merge
fix/skip-repositories-path-deletes-on-restore into master

What does this MR do and why?

Backup::Targets::Repositories#remove_all_repositories decided whether to clear existing repositories before a restore based only on whether REPOSITORIES_PATHS was set:

return if paths.present?

A backup created with SKIP_REPOSITORIES_PATHS alone leaves paths empty, so the restore passed -remove-all-repositories to gitaly-backup and cleared every repository in the configured storages first. Because the excluded repositories are also filtered out of the restore itself, they were deleted and never restored, with no way to recover them from that backup.

This also made it impossible to restore several SKIP_REPOSITORIES_PATHS backups in sequence, since each restore destroyed the repositories left by the previous one. The guard on REPOSITORIES_PATHS exists to support exactly that workflow.

This MR treats a backup as partial when either option is set. A full restore, with neither set, still clears stale repositories.

Why this was missed

SKIP_REPOSITORIES_PATHS was added in !121865 (merged). That MR threaded skip_paths through project_relation, snippet_relation, and a new skipped_path_relation, but left remove_all_repositories unchanged. It appears as untouched context in that diff, immediately below the attr_reader line the MR did modify. The for skip_paths examples added in the same MR asserted the resulting behaviour, so this MR updates those two assertions along with the fix.

The behaviour also stayed hidden because the documented usage passes both options together. paths.present? short-circuits in that case, so the SKIP_REPOSITORIES_PATHS-only path was never exercised.

Points for reviewer attention

  • Two existing assertions are inverted. The for skip_paths examples under #restore previously expected remove_all_repositories: %w[default]. Both now expect nil. See "Why this was missed" above.
  • Presence is treated as intent. A non-matching value, for example a typo'd path, now suppresses clearing even though nothing is actually excluded. This matches how the existing paths guard already behaves.
  • REPOSITORIES_STORAGES combined with SKIP_REPOSITORIES_PATHS previously cleared the named storage and now does not. Covered by a new spec.
  • Restore-time environment variables. Backup::Options#update_from_backup_information! merges manifest values with +=, so SKIP_REPOSITORIES_PATHS passed at restore time also suppresses clearing, even for a backup that recorded no skip paths. This seems reasonable, since the restore is partial, but it is new behaviour for skip_paths.

References

Screenshots or screen recordings

Note

The screen recording has been edited to skip over waiting for commands.

How to set up and validate locally

Warning

Run this on a disposable GDK. Step 3 permanently destroys the flightjs repositories, and they are only recoverable by completing step 4.

Steps 3 and 5 restore the same backup artifact, so the only variable is the code.

Prerequisites

Run everything from <gdk-root>/gitlab, starting on master:

git checkout master

Before starting, note which repositories the flightjs projects currently have, so you can compare after each restore. Checking in the UI is enough.

1. On master, create a full backup to use as a restore point

bundle exec rake gitlab:backup:create BACKUP=full-backup \
  SKIP=db,uploads,builds,artifacts,lfs,terraform_state,registry,packages,ci_secure_files,external_diffs,pages,agent_plan_content

This deliberately omits SKIP_REPOSITORIES_PATHS, so the backup contains every repository and step 4 can restore flightjs.

2. Create a partial backup that excludes flightjs

bundle exec rake gitlab:backup:create BACKUP=skip-test \
  SKIP_REPOSITORIES_PATHS=flightjs \
  SKIP=db,uploads,builds,artifacts,lfs,terraform_state,registry,packages,ci_secure_files,external_diffs,pages,agent_plan_content

Confirm the manifest recorded the exclusion:

tar -xOf tmp/backups/skip-test_gitlab_backup.tar backup_information.yml \
  | grep -E ':backup_id|:skip_repositories_paths'

3. Restore skip-test and observe the data loss

rm -rf tmp/backups/db tmp/backups/repositories tmp/backups/backup_information.yml
tar -xf tmp/backups/skip-test_gitlab_backup.tar -C tmp/backups
bundle exec rake gitlab:backup:repo:restore BACKUP=skip-test

The flightjs repositories are now gone, while every other repository is present.

4. Reset back to the starting state

rm -rf tmp/backups/db tmp/backups/repositories tmp/backups/backup_information.yml
tar -xf tmp/backups/full-backup_gitlab_backup.tar -C tmp/backups
bundle exec rake gitlab:backup:repo:restore BACKUP=full-backup

The flightjs repositories are present again.

5. On the fix/skip-repositories-path-deletes-on-restore branch, restore the same backup

git checkout fix/skip-repositories-path-deletes-on-restore
gdk restart rails-web vite
rm -rf tmp/backups/db tmp/backups/repositories tmp/backups/backup_information.yml
tar -xf tmp/backups/skip-test_gitlab_backup.tar -C tmp/backups
bundle exec rake gitlab:backup:repo:restore BACKUP=skip-test

The flightjs repositories are still present and unchanged, because the clearing step was skipped.

6. Still on this branch, confirm the documented restore-time argument

The restore documentation shows SKIP_REPOSITORIES_PATHS passed directly to a restore. That route reaches the clearing decision differently, through extract_from_env! in the Backup::Manager constructor, so verify it too using the full backup from step 1:

rm -rf tmp/backups/db tmp/backups/repositories tmp/backups/backup_information.yml
tar -xf tmp/backups/full-backup_gitlab_backup.tar -C tmp/backups
bundle exec rake gitlab:backup:repo:restore BACKUP=full-backup \
  SKIP_REPOSITORIES_PATHS=flightjs

The flightjs repositories survive, and the restore log shows only the flightjs group wiki being restored. On master, the same command removes them.

Notes on the steps

  • gitlab:backup:repo:restore restores only repositories, leaving the database and gitlab:shell:setup alone.
  • The rm -rf and tar -xf before each restore matter: restore_task does not call run_unpack, so it reads the manifest and staging directory from disk. Without clearing first, a restore can run against a stale manifest from an earlier step and give a misleading result.
  • SKIP filters tasks, whereas SKIP_REPOSITORIES_PATHS filters repositories. agent_plan_content is in the SKIP list only because that storage path is not provisioned in a default GDK.
  • A full gitlab:backup:create is used rather than gitlab:backup:repo:create because only the full task list writes the manifest that carries skip_repositories_paths through to the restore.

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Anton Smith

Merge request reports