Commit cd4538bb authored by Will Chandler's avatar Will Chandler

Fix exclude path for backup rsync command

https://gitlab.com/gitlab-org/gitlab/-/merge_requests/41706 attempted to
prevent `rsync` from including tmp directories when preparing backups
with `STRATEGY=copy`, but used an incorrect path that caused `rsync`
to continue to include these directories.

The `--exclude` argument was passed with the tmp directory name with a
leading slash, but this is not the correct format when no trailing slash
is used on the source path. `rsync` will `chdir` to the parent directory
in this scenario, e.g. /var/opt/gitlab/gitlab-rails/shared for pages,
treating that location as `/` in the `--exclude` path.

We need to include full path from the source directory to the tmp
directory in `--exclude`, which means adding the basename of
`app_files_dir`.

Current command:
  rsync -a --delete --exclude=lost+found --exclude=/@pages.tmp \
    /var/opt/gitlab/gitlab-rails/shared/pages /var/opt/gitlab/backups

Correct command:
  rsync -a --delete --exclude=lost+found --exclude=/pages/@pages.tmp \
    /var/opt/gitlab/gitlab-rails/shared/pages /var/opt/gitlab/backups
parent f4081477
---
title: Fix exclude path for backup rsync command
merge_request: 52503
author:
type: fixed
......@@ -137,7 +137,7 @@ module Backup
if s == DEFAULT_EXCLUDE
'--exclude=' + s
elsif fmt == :rsync
'--exclude=/' + s
'--exclude=/' + File.join(File.basename(app_files_dir), s)
elsif fmt == :tar
'--exclude=./' + s
end
......
......@@ -150,7 +150,7 @@ RSpec.describe Backup::Files do
it 'excludes tmp dirs from rsync' do
expect(Gitlab::Popen).to receive(:popen)
.with(%w(rsync -a --delete --exclude=lost+found --exclude=/@pages.tmp /var/gitlab-pages /var/gitlab-backup))
.with(%w(rsync -a --delete --exclude=lost+found --exclude=/gitlab-pages/@pages.tmp /var/gitlab-pages /var/gitlab-backup))
.and_return(['', 0])
subject.dump
......@@ -158,7 +158,7 @@ RSpec.describe Backup::Files do
it 'retries if rsync fails due to vanishing files' do
expect(Gitlab::Popen).to receive(:popen)
.with(%w(rsync -a --delete --exclude=lost+found --exclude=/@pages.tmp /var/gitlab-pages /var/gitlab-backup))
.with(%w(rsync -a --delete --exclude=lost+found --exclude=/gitlab-pages/@pages.tmp /var/gitlab-pages /var/gitlab-backup))
.and_return(['rsync failed', 24], ['', 0])
expect do
......@@ -168,7 +168,7 @@ RSpec.describe Backup::Files do
it 'raises an error and outputs an error message if rsync failed' do
allow(Gitlab::Popen).to receive(:popen)
.with(%w(rsync -a --delete --exclude=lost+found --exclude=/@pages.tmp /var/gitlab-pages /var/gitlab-backup))
.with(%w(rsync -a --delete --exclude=lost+found --exclude=/gitlab-pages/@pages.tmp /var/gitlab-pages /var/gitlab-backup))
.and_return(['rsync failed', 1])
expect do
......@@ -186,8 +186,8 @@ RSpec.describe Backup::Files do
expect(subject.exclude_dirs(:tar)).to eq(['--exclude=lost+found', '--exclude=./@pages.tmp'])
end
it 'prepends a leading slash to rsync excludes' do
expect(subject.exclude_dirs(:rsync)).to eq(['--exclude=lost+found', '--exclude=/@pages.tmp'])
it 'prepends a leading slash and app_files_dir basename to rsync excludes' do
expect(subject.exclude_dirs(:rsync)).to eq(['--exclude=lost+found', '--exclude=/gitlab-pages/@pages.tmp'])
end
end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment