Stop backup files from requiring directories to exist when skipped

Calls to File.realpath where the directory does not exist will raise an
exception. Since all backup tasks are initialized no matter if they are
skipped or not, we need to stop calling realpath within the initializer.
Instead here we are lazily initializing realpath as required.

Removed assertions for the now private realpath accessors. These paths
are still correctly asserted by checking the exact tar commandline
arguments.

Changelog: fixed

lib/backup/files.rb

@@ -9,13 +9,11 @@ module Backup
 
     DEFAULT_EXCLUDE = 'lost+found'
 
-    attr_reader :name, :app_files_dir, :backup_tarball, :excludes, :files_parent_dir
+    attr_reader :name, :backup_tarball, :excludes
 
     def initialize(name, app_files_dir, excludes: [])
       @name = name
-      @app_files_dir = File.realpath(app_files_dir)
-      @files_parent_dir = File.realpath(File.join(@app_files_dir, '..'))
-      @backup_files_dir = File.join(Gitlab.config.backup.path, File.basename(@app_files_dir) )
+      @app_files_dir = app_files_dir
       @backup_tarball = File.join(Gitlab.config.backup.path, name + '.tar.gz')
       @excludes = [DEFAULT_EXCLUDE].concat(excludes)
     end
@@ -26,7 +24,7 @@ module Backup
       FileUtils.rm_f(backup_tarball)
 
       if ENV['STRATEGY'] == 'copy'
-        cmd = [%w[rsync -a --delete], exclude_dirs(:rsync), %W[#{app_files_dir} #{Gitlab.config.backup.path}]].flatten
+        cmd = [%w[rsync -a --delete], exclude_dirs(:rsync), %W[#{app_files_realpath} #{Gitlab.config.backup.path}]].flatten
         output, status = Gitlab::Popen.popen(cmd)
 
         # Retry if rsync source files vanish
@@ -40,11 +38,11 @@ module Backup
           raise_custom_error
         end
 
-        tar_cmd = [tar, exclude_dirs(:tar), %W[-C #{@backup_files_dir} -cf - .]].flatten
+        tar_cmd = [tar, exclude_dirs(:tar), %W[-C #{backup_files_realpath} -cf - .]].flatten
         status_list, output = run_pipeline!([tar_cmd, gzip_cmd], out: [backup_tarball, 'w', 0600])
-        FileUtils.rm_rf(@backup_files_dir)
+        FileUtils.rm_rf(backup_files_realpath)
       else
-        tar_cmd = [tar, exclude_dirs(:tar), %W[-C #{app_files_dir} -cf - .]].flatten
+        tar_cmd = [tar, exclude_dirs(:tar), %W[-C #{app_files_realpath} -cf - .]].flatten
         status_list, output = run_pipeline!([tar_cmd, gzip_cmd], out: [backup_tarball, 'w', 0600])
       end
 
@@ -56,7 +54,7 @@ module Backup
     def restore
       backup_existing_files_dir
 
-      cmd_list = [%w[gzip -cd], %W[#{tar} --unlink-first --recursive-unlink -C #{app_files_dir} -xf -]]
+      cmd_list = [%w[gzip -cd], %W[#{tar} --unlink-first --recursive-unlink -C #{app_files_realpath} -xf -]]
       status_list, output = run_pipeline!(cmd_list, in: backup_tarball)
       unless pipeline_succeeded?(gzip_status: status_list[0], tar_status: status_list[1], output: output)
         raise Backup::Error, "Restore operation failed: #{output}"
@@ -78,17 +76,17 @@ module Backup
 
     def backup_existing_files_dir
       timestamped_files_path = File.join(Gitlab.config.backup.path, "tmp", "#{name}.#{Time.now.to_i}")
-      if File.exist?(app_files_dir)
+      if File.exist?(app_files_realpath)
         # Move all files in the existing repos directory except . and .. to
         # repositories.old.<timestamp> directory
         FileUtils.mkdir_p(timestamped_files_path, mode: 0700)
-        files = Dir.glob(File.join(app_files_dir, "*"), File::FNM_DOTMATCH) - [File.join(app_files_dir, "."), File.join(app_files_dir, "..")]
+        files = Dir.glob(File.join(app_files_realpath, "*"), File::FNM_DOTMATCH) - [File.join(app_files_realpath, "."), File.join(app_files_realpath, "..")]
         begin
           FileUtils.mv(files, timestamped_files_path)
         rescue Errno::EACCES
-          access_denied_error(app_files_dir)
+          access_denied_error(app_files_realpath)
         rescue Errno::EBUSY
-          resource_busy_error(app_files_dir)
+          resource_busy_error(app_files_realpath)
         end
       end
     end
@@ -141,7 +139,7 @@ module Backup
         if s == DEFAULT_EXCLUDE
           '--exclude=' + s
         elsif fmt == :rsync
-          '--exclude=/' + File.join(File.basename(app_files_dir), s)
+          '--exclude=/' + File.join(File.basename(app_files_realpath), s)
         elsif fmt == :tar
           '--exclude=./' + s
         end
@@ -149,7 +147,17 @@ module Backup
     end
 
     def raise_custom_error
-      raise FileBackupError.new(app_files_dir, backup_tarball)
+      raise FileBackupError.new(app_files_realpath, backup_tarball)
+    end
+
+    private
+
+    def app_files_realpath
+      @app_files_realpath ||= File.realpath(@app_files_dir)
+    end
+
+    def backup_files_realpath
+      @backup_files_realpath ||= File.join(Gitlab.config.backup.path, File.basename(@app_files_dir) )
     end
   end
 end

spec/lib/backup/artifacts_spec.rb

@@ -7,16 +7,6 @@ RSpec.describe Backup::Artifacts do
 
   subject(:backup) { described_class.new(progress) }
 
-  describe '#initialize' do
-    it 'uses the correct upload dir' do
-      Dir.mktmpdir do |tmpdir|
-        allow(JobArtifactUploader).to receive(:root) { "#{tmpdir}" }
-
-        expect(backup.app_files_dir).to eq("#{File.realpath(tmpdir)}")
-      end
-    end
-  end
-
   describe '#dump' do
     before do
       allow(File).to receive(:realpath).with('/var/gitlab-artifacts').and_return('/var/gitlab-artifacts')
@@ -24,10 +14,6 @@ RSpec.describe Backup::Artifacts do
       allow(JobArtifactUploader).to receive(:root) { '/var/gitlab-artifacts' }
     end
 
-    it 'uses the correct artifact dir' do
-      expect(backup.app_files_dir).to eq('/var/gitlab-artifacts')
-    end
-
     it 'excludes tmp from backup tar' do
       expect(backup).to receive(:tar).and_return('blabla-tar')
       expect(backup).to receive(:run_pipeline!).with([%w(blabla-tar --exclude=lost+found --exclude=./tmp -C /var/gitlab-artifacts -cf - .), 'gzip -c -1'], any_args).and_return([[true, true], ''])

spec/lib/backup/lfs_spec.rb

@@ -16,7 +16,6 @@ RSpec.describe Backup::Lfs do
     end
 
     it 'uses the correct lfs dir in tar command', :aggregate_failures do
-      expect(backup.app_files_dir).to eq('/var/lfs-objects')
       expect(backup).to receive(:tar).and_return('blabla-tar')
       expect(backup).to receive(:run_pipeline!).with([%w(blabla-tar --exclude=lost+found -C /var/lfs-objects -cf - .), 'gzip -c -1'], any_args).and_return([[true, true], ''])
       expect(backup).to receive(:pipeline_succeeded?).and_return(true)

spec/lib/backup/manager_spec.rb

@@ -12,11 +12,6 @@ RSpec.describe Backup::Manager do
   before do
     allow(progress).to receive(:puts)
     allow(progress).to receive(:print)
-    FileUtils.mkdir_p('tmp/tests/public/uploads')
-  end
-
-  after do
-    FileUtils.rm_rf('tmp/tests/public/uploads', secure: true)
   end
 
   describe '#pack' do

spec/lib/backup/object_backup_spec.rb

@@ -17,7 +17,6 @@ RSpec.shared_examples 'backup object' do |setting|
     end
 
     it 'uses the correct storage dir in tar command and excludes tmp', :aggregate_failures do
-      expect(backup.app_files_dir).to eq(backup_path)
       expect(backup).to receive(:tar).and_return('blabla-tar')
       expect(backup).to receive(:run_pipeline!).with([%W(blabla-tar --exclude=lost+found --exclude=./tmp -C #{backup_path} -cf - .), 'gzip -c -1'], any_args).and_return([[true, true], ''])
       expect(backup).to receive(:pipeline_succeeded?).and_return(true)

spec/lib/backup/pages_spec.rb

@@ -13,12 +13,6 @@ RSpec.describe Backup::Pages do
   end
 
   describe '#dump' do
-    it 'uses the correct pages dir' do
-      allow(Gitlab.config.pages).to receive(:path) { '/var/gitlab-pages' }
-
-      expect(subject.app_files_dir).to eq('/var/gitlab-pages')
-    end
-
     it 'excludes tmp from backup tar' do
       allow(Gitlab.config.pages).to receive(:path) { '/var/gitlab-pages' }
 

spec/lib/backup/uploads_spec.rb

@@ -7,18 +7,6 @@ RSpec.describe Backup::Uploads do
 
   subject(:backup) { described_class.new(progress) }
 
-  describe '#initialize' do
-    it 'uses the correct upload dir' do
-      Dir.mktmpdir do |tmpdir|
-        FileUtils.mkdir_p("#{tmpdir}/uploads")
-
-        allow(Gitlab.config.uploads).to receive(:storage_path) { tmpdir }
-
-        expect(backup.app_files_dir).to eq("#{File.realpath(tmpdir)}/uploads")
-      end
-    end
-  end
-
   describe '#dump' do
     before do
       allow(File).to receive(:realpath).and_call_original
@@ -27,10 +15,6 @@ RSpec.describe Backup::Uploads do
       allow(Gitlab.config.uploads).to receive(:storage_path) { '/var' }
     end
 
-    it 'uses the correct upload dir' do
-      expect(backup.app_files_dir).to eq('/var/uploads')
-    end
-
     it 'excludes tmp from backup tar' do
       expect(backup).to receive(:tar).and_return('blabla-tar')
       expect(backup).to receive(:run_pipeline!).with([%w(blabla-tar --exclude=lost+found --exclude=./tmp -C /var/uploads -cf - .), 'gzip -c -1'], any_args).and_return([[true, true], ''])