Pass backup ID to each backup task when creating a backup

Eventually we will pass this backup ID to gitaly-backup so that it can
maintain its own persistent storage and still be associated with the
correct backup.

ee/spec/lib/ee/backup/repositories_spec.rb

@@ -6,6 +6,7 @@ RSpec.describe Backup::Repositories do
   let(:progress) { spy(:stdout) }
   let(:strategy) { spy(:strategy) }
   let(:destination) { 'repositories' }
+  let(:backup_id) { 'backup_id' }
 
   subject { described_class.new(progress, strategy: strategy) }
 
@@ -14,7 +15,7 @@ RSpec.describe Backup::Repositories do
     let_it_be(:groups) { create_list(:group, 5, :wiki_repo) }
 
     it 'calls enqueue for each repository type', :aggregate_failures do
-      subject.dump(destination)
+      subject.dump(destination, backup_id)
 
       expect(strategy).to have_received(:start).with(:create, destination)
       expect(strategy).to have_received(:enqueue).with(project, Gitlab::GlRepository::PROJECT)
@@ -28,25 +29,25 @@ RSpec.describe Backup::Repositories do
       it 'enqueue_group raises an error' do
         allow(strategy).to receive(:enqueue).with(anything, Gitlab::GlRepository::WIKI).and_raise(IOError)
 
-        expect { subject.dump(destination) }.to raise_error(IOError)
+        expect { subject.dump(destination, backup_id) }.to raise_error(IOError)
       end
 
       it 'group query raises an error' do
         allow(Group).to receive_message_chain(:includes, :find_each).and_raise(ActiveRecord::StatementTimeout)
 
-        expect { subject.dump(destination) }.to raise_error(ActiveRecord::StatementTimeout)
+        expect { subject.dump(destination, backup_id) }.to raise_error(ActiveRecord::StatementTimeout)
       end
     end
 
     it 'avoids N+1 database queries' do
       control_count = ActiveRecord::QueryRecorder.new do
-        subject.dump(destination)
+        subject.dump(destination, backup_id)
       end.count
 
       create_list(:group, 2, :wiki_repo)
 
       expect do
-        subject.dump(destination)
+        subject.dump(destination, backup_id)
       end.not_to exceed_query_limit(control_count)
     end
   end

lib/backup/database.rb

@@ -25,7 +25,7 @@ module Backup
     end
 
     override :dump
-    def dump(db_file_name)
+    def dump(db_file_name, backup_id)
       FileUtils.mkdir_p(File.dirname(db_file_name))
       FileUtils.rm_f(db_file_name)
       compress_rd, compress_wr = IO.pipe

lib/backup/files.rb

@@ -21,7 +21,7 @@ module Backup
 
     # Copy files from public/files to backup/files
     override :dump
-    def dump(backup_tarball)
+    def dump(backup_tarball, backup_id)
       FileUtils.mkdir_p(Gitlab.config.backup.path)
       FileUtils.rm_f(backup_tarball)
 

lib/backup/manager.rb

@@ -113,7 +113,7 @@ module Backup
       end
 
       puts_time "Dumping #{definition.task.human_name} ... ".color(:blue)
-      definition.task.dump(File.join(Gitlab.config.backup.path, definition.destination_path))
+      definition.task.dump(File.join(Gitlab.config.backup.path, definition.destination_path), backup_id)
       puts_time "Dumping #{definition.task.human_name} ... ".color(:blue) + "done".color(:green)
 
     rescue Backup::DatabaseBackupError, Backup::FileBackupError => e

lib/backup/repositories.rb

@@ -13,7 +13,7 @@ module Backup
     end
 
     override :dump
-    def dump(path)
+    def dump(path, backup_id)
       strategy.start(:create, path)
       enqueue_consecutive
 

lib/backup/task.rb

@@ -12,7 +12,10 @@ module Backup
     end
 
     # dump task backup to `path`
-    def dump(path)
+    #
+    # @param [String] path fully qualified backup task destination
+    # @param [String] backup_id unique identifier for the backup
+    def dump(path, backup_id)
       raise NotImplementedError
     end
 

spec/lib/backup/artifacts_spec.rb

@@ -18,7 +18,7 @@ RSpec.describe Backup::Artifacts 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], ''])
       expect(backup).to receive(:pipeline_succeeded?).and_return(true)
-      backup.dump('artifacts.tar.gz')
+      backup.dump('artifacts.tar.gz', 'backup_id')
     end
   end
 end

spec/lib/backup/files_spec.rb

@@ -118,14 +118,14 @@ RSpec.describe Backup::Files do
     end
 
     it 'raises no errors' do
-      expect { subject.dump('registry.tar.gz') }.not_to raise_error
+      expect { subject.dump('registry.tar.gz', 'backup_id') }.not_to raise_error
     end
 
     it 'excludes tmp dirs from archive' do
       expect(subject).to receive(:tar).and_return('blabla-tar')
 
       expect(subject).to receive(:run_pipeline!).with([%w(blabla-tar --exclude=lost+found --exclude=./@pages.tmp -C /var/gitlab-pages -cf - .), 'gzip -c -1'], any_args)
-      subject.dump('registry.tar.gz')
+      subject.dump('registry.tar.gz', 'backup_id')
     end
 
     it 'raises an error on failure' do
@@ -133,7 +133,7 @@ RSpec.describe Backup::Files do
       expect(subject).to receive(:pipeline_succeeded?).and_return(false)
 
       expect do
-        subject.dump('registry.tar.gz')
+        subject.dump('registry.tar.gz', 'backup_id')
       end.to raise_error(/Failed to create compressed file/)
     end
 
@@ -149,7 +149,7 @@ RSpec.describe Backup::Files do
           .with(%w(rsync -a --delete --exclude=lost+found --exclude=/gitlab-pages/@pages.tmp /var/gitlab-pages /var/gitlab-backup))
           .and_return(['', 0])
 
-        subject.dump('registry.tar.gz')
+        subject.dump('registry.tar.gz', 'backup_id')
       end
 
       it 'retries if rsync fails due to vanishing files' do
@@ -158,7 +158,7 @@ RSpec.describe Backup::Files do
           .and_return(['rsync failed', 24], ['', 0])
 
         expect do
-          subject.dump('registry.tar.gz')
+          subject.dump('registry.tar.gz', 'backup_id')
         end.to output(/files vanished during rsync, retrying/).to_stdout
       end
 
@@ -168,7 +168,7 @@ RSpec.describe Backup::Files do
           .and_return(['rsync failed', 1])
 
         expect do
-          subject.dump('registry.tar.gz')
+          subject.dump('registry.tar.gz', 'backup_id')
         end.to output(/rsync failed/).to_stdout
            .and raise_error(/Failed to create compressed file/)
       end

spec/lib/backup/lfs_spec.rb

@@ -20,7 +20,7 @@ RSpec.describe Backup::Lfs do
       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)
 
-      backup.dump('lfs.tar.gz')
+      backup.dump('lfs.tar.gz', 'backup_id')
     end
   end
 end

spec/lib/backup/manager_spec.rb

@@ -147,7 +147,8 @@ RSpec.describe Backup::Manager do
   describe '#create' do
     let(:incremental_env) { 'false' }
     let(:expected_backup_contents) { %w{backup_information.yml task1.tar.gz task2.tar.gz} }
-    let(:tar_file) { '1546300800_2019_01_01_12.3_gitlab_backup.tar' }
+    let(:backup_id) { '1546300800_2019_01_01_12.3' }
+    let(:tar_file) { "#{backup_id}_gitlab_backup.tar" }
     let(:tar_system_options) { { out: [tar_file, 'w', Gitlab.config.backup.archive_permissions] } }
     let(:tar_cmdline) { ['tar', '-cf', '-', *expected_backup_contents, tar_system_options] }
     let(:backup_information) do
@@ -176,8 +177,8 @@ RSpec.describe Backup::Manager do
         .and_return(backup_information)
 
       allow(subject).to receive(:backup_information).and_return(backup_information)
-      allow(task1).to receive(:dump).with(File.join(Gitlab.config.backup.path, 'task1.tar.gz'))
-      allow(task2).to receive(:dump).with(File.join(Gitlab.config.backup.path, 'task2.tar.gz'))
+      allow(task1).to receive(:dump).with(File.join(Gitlab.config.backup.path, 'task1.tar.gz'), backup_id)
+      allow(task2).to receive(:dump).with(File.join(Gitlab.config.backup.path, 'task2.tar.gz'), backup_id)
     end
 
     it 'executes tar' do
@@ -201,7 +202,7 @@ RSpec.describe Backup::Manager do
     end
 
     context 'when BACKUP is set' do
-      let(:tar_file) { 'custom_gitlab_backup.tar' }
+      let(:backup_id) { 'custom' }
 
       it 'uses the given value as tar file name' do
         stub_env('BACKUP', '/ignored/path/custom')
@@ -581,7 +582,8 @@ RSpec.describe Backup::Manager do
     context 'incremental' do
       let(:incremental_env) { 'true' }
       let(:gitlab_version) { Gitlab::VERSION }
-      let(:tar_file) { "1546300800_2019_01_01_#{gitlab_version}_gitlab_backup.tar" }
+      let(:backup_id) { "1546300800_2019_01_01_#{gitlab_version}" }
+      let(:tar_file) { "#{backup_id}_gitlab_backup.tar" }
       let(:backup_information) do
         {
           backup_created_at: Time.zone.parse('2019-01-01'),
@@ -645,6 +647,7 @@ RSpec.describe Backup::Manager do
       end
 
       context 'when BACKUP variable is set to a correct file' do
+        let(:backup_id) { '1451606400_2016_01_01_1.2.3' }
         let(:tar_cmdline) { %w{tar -xf 1451606400_2016_01_01_1.2.3_gitlab_backup.tar} }
 
         before do

spec/lib/backup/object_backup_spec.rb

@@ -21,7 +21,7 @@ RSpec.shared_examples 'backup object' do |setting|
       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)
 
-      backup.dump('backup_object.tar.gz')
+      backup.dump('backup_object.tar.gz', 'backup_id')
     end
   end
 end

spec/lib/backup/pages_spec.rb

@@ -19,7 +19,7 @@ RSpec.describe Backup::Pages do
       expect(subject).to receive(:tar).and_return('blabla-tar')
       expect(subject).to receive(:run_pipeline!).with([%w(blabla-tar --exclude=lost+found --exclude=./@pages.tmp -C /var/gitlab-pages -cf - .), 'gzip -c -1'], any_args).and_return([[true, true], ''])
       expect(subject).to receive(:pipeline_succeeded?).and_return(true)
-      subject.dump('pages.tar.gz')
+      subject.dump('pages.tar.gz', 'backup_id')
     end
   end
 end

spec/lib/backup/repositories_spec.rb

@@ -6,6 +6,7 @@ RSpec.describe Backup::Repositories do
   let(:progress) { spy(:stdout) }
   let(:strategy) { spy(:strategy) }
   let(:destination) { 'repositories' }
+  let(:backup_id) { 'backup_id' }
 
   subject do
     described_class.new(
@@ -22,7 +23,7 @@ RSpec.describe Backup::Repositories do
         project_snippet = create(:project_snippet, :repository, project: project)
         personal_snippet = create(:personal_snippet, :repository, author: project.first_owner)
 
-        subject.dump(destination)
+        subject.dump(destination, backup_id)
 
         expect(strategy).to have_received(:start).with(:create, destination)
         expect(strategy).to have_received(:enqueue).with(project, Gitlab::GlRepository::PROJECT)
@@ -50,25 +51,25 @@ RSpec.describe Backup::Repositories do
       it 'enqueue_project raises an error' do
         allow(strategy).to receive(:enqueue).with(anything, Gitlab::GlRepository::PROJECT).and_raise(IOError)
 
-        expect { subject.dump(destination) }.to raise_error(IOError)
+        expect { subject.dump(destination, backup_id) }.to raise_error(IOError)
       end
 
       it 'project query raises an error' do
         allow(Project).to receive_message_chain(:includes, :find_each).and_raise(ActiveRecord::StatementTimeout)
 
-        expect { subject.dump(destination) }.to raise_error(ActiveRecord::StatementTimeout)
+        expect { subject.dump(destination, backup_id) }.to raise_error(ActiveRecord::StatementTimeout)
       end
     end
 
     it 'avoids N+1 database queries' do
       control_count = ActiveRecord::QueryRecorder.new do
-        subject.dump(destination)
+        subject.dump(destination, backup_id)
       end.count
 
       create_list(:project, 2, :repository)
 
       expect do
-        subject.dump(destination)
+        subject.dump(destination, backup_id)
       end.not_to exceed_query_limit(control_count)
     end
   end

spec/lib/backup/task_spec.rb

@@ -15,7 +15,7 @@ RSpec.describe Backup::Task do
 
   describe '#dump' do
     it 'must be implemented by the subclass' do
-      expect { subject.dump('some/path') }.to raise_error(NotImplementedError)
+      expect { subject.dump('some/path', 'backup_id') }.to raise_error(NotImplementedError)
     end
   end
 

spec/lib/backup/uploads_spec.rb

@@ -19,7 +19,7 @@ RSpec.describe Backup::Uploads 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], ''])
       expect(backup).to receive(:pipeline_succeeded?).and_return(true)
-      backup.dump('uploads.tar.gz')
+      backup.dump('uploads.tar.gz', 'backup_id')
     end
   end
 end