Merge branch 'backup_create_id' into 'master'

Pass backup ID to gitaly-backup

See merge request gitlab-org/gitlab!83798

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,9 +15,9 @@ 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(:start).with(:create, destination, backup_id: backup_id)
       expect(strategy).to have_received(:enqueue).with(project, Gitlab::GlRepository::PROJECT)
       groups.each do |group|
         expect(strategy).to have_received(:enqueue).with(group, Gitlab::GlRepository::WIKI)
@@ -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/gitaly_backup.rb

@@ -9,16 +9,14 @@ module Backup
     # @param [StringIO] progress IO interface to output progress
     # @param [Integer] max_parallelism max parallelism when running backups
     # @param [Integer] storage_parallelism max parallelism per storage (is affected by max_parallelism)
-    # @param [String] backup_id unique identifier for the backup
     def initialize(progress, max_parallelism: nil, storage_parallelism: nil, incremental: false, backup_id: nil)
       @progress = progress
       @max_parallelism = max_parallelism
       @storage_parallelism = storage_parallelism
       @incremental = incremental
-      @backup_id = backup_id
     end
 
-    def start(type, backup_repos_path)
+    def start(type, backup_repos_path, backup_id: nil)
       raise Error, 'already started' if started?
 
       command = case type
@@ -37,7 +35,7 @@ module Backup
         args += ['-layout', 'pointer']
         if type == :create
           args += ['-incremental'] if @incremental
-          args += ['-id', @backup_id] if @backup_id
+          args += ['-id', backup_id] if backup_id
         end
       end
 

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,8 +13,8 @@ module Backup
     end
 
     override :dump
-    def dump(path)
-      strategy.start(:create, path)
+    def dump(path, backup_id)
+      strategy.start(:create, path, backup_id: backup_id)
       enqueue_consecutive
 
     ensure

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/gitaly_backup_spec.rb

@@ -25,11 +25,11 @@ RSpec.describe Backup::GitalyBackup do
     progress.close
   end
 
-  subject { described_class.new(progress, max_parallelism: max_parallelism, storage_parallelism: storage_parallelism, backup_id: backup_id) }
+  subject { described_class.new(progress, max_parallelism: max_parallelism, storage_parallelism: storage_parallelism) }
 
   context 'unknown' do
     it 'fails to start unknown' do
-      expect { subject.start(:unknown, destination) }.to raise_error(::Backup::Error, 'unknown backup type: unknown')
+      expect { subject.start(:unknown, destination, backup_id: backup_id) }.to raise_error(::Backup::Error, 'unknown backup type: unknown')
     end
   end
 
@@ -44,7 +44,7 @@ RSpec.describe Backup::GitalyBackup do
 
         expect(Open3).to receive(:popen2).with(expected_env, anything, 'create', '-path', anything, '-layout', 'pointer', '-id', backup_id).and_call_original
 
-        subject.start(:create, destination)
+        subject.start(:create, destination, backup_id: backup_id)
         subject.enqueue(project, Gitlab::GlRepository::PROJECT)
         subject.enqueue(project, Gitlab::GlRepository::WIKI)
         subject.enqueue(project, Gitlab::GlRepository::DESIGN)
@@ -65,7 +65,7 @@ RSpec.describe Backup::GitalyBackup do
         it 'passes parallel option through' do
           expect(Open3).to receive(:popen2).with(expected_env, anything, 'create', '-path', anything, '-parallel', '3', '-layout', 'pointer', '-id', backup_id).and_call_original
 
-          subject.start(:create, destination)
+          subject.start(:create, destination, backup_id: backup_id)
           subject.finish!
         end
       end
@@ -76,7 +76,7 @@ RSpec.describe Backup::GitalyBackup do
         it 'passes parallel option through' do
           expect(Open3).to receive(:popen2).with(expected_env, anything, 'create', '-path', anything, '-parallel-storage', '3', '-layout', 'pointer', '-id', backup_id).and_call_original
 
-          subject.start(:create, destination)
+          subject.start(:create, destination, backup_id: backup_id)
           subject.finish!
         end
       end
@@ -84,14 +84,14 @@ RSpec.describe Backup::GitalyBackup do
       it 'raises when the exit code not zero' do
         expect(subject).to receive(:bin_path).and_return(Gitlab::Utils.which('false'))
 
-        subject.start(:create, destination)
+        subject.start(:create, destination, backup_id: backup_id)
         expect { subject.finish! }.to raise_error(::Backup::Error, 'gitaly-backup exit status 1')
       end
 
       it 'raises when gitaly_backup_path is not set' do
         stub_backup_setting(gitaly_backup_path: nil)
 
-        expect { subject.start(:create, destination) }.to raise_error(::Backup::Error, 'gitaly-backup binary not found and gitaly_backup_path is not configured')
+        expect { subject.start(:create, destination, backup_id: backup_id) }.to raise_error(::Backup::Error, 'gitaly-backup binary not found and gitaly_backup_path is not configured')
       end
 
       context 'feature flag incremental_repository_backup disabled' do
@@ -108,7 +108,7 @@ RSpec.describe Backup::GitalyBackup do
 
           expect(Open3).to receive(:popen2).with(expected_env, anything, 'create', '-path', anything).and_call_original
 
-          subject.start(:create, destination)
+          subject.start(:create, destination, backup_id: backup_id)
           subject.enqueue(project, Gitlab::GlRepository::PROJECT)
           subject.enqueue(project, Gitlab::GlRepository::WIKI)
           subject.enqueue(project, Gitlab::GlRepository::DESIGN)
@@ -152,7 +152,7 @@ RSpec.describe Backup::GitalyBackup do
       it 'passes through SSL envs' do
         expect(Open3).to receive(:popen2).with(ssl_env, anything, 'create', '-path', anything, '-layout', 'pointer', '-id', backup_id).and_call_original
 
-        subject.start(:create, destination)
+        subject.start(:create, destination, backup_id: backup_id)
         subject.finish!
       end
     end
@@ -177,7 +177,7 @@ RSpec.describe Backup::GitalyBackup do
 
       expect(Open3).to receive(:popen2).with(expected_env, anything, 'restore', '-path', anything, '-layout', 'pointer').and_call_original
 
-      subject.start(:restore, destination)
+      subject.start(:restore, destination, backup_id: backup_id)
       subject.enqueue(project, Gitlab::GlRepository::PROJECT)
       subject.enqueue(project, Gitlab::GlRepository::WIKI)
       subject.enqueue(project, Gitlab::GlRepository::DESIGN)
@@ -200,7 +200,7 @@ RSpec.describe Backup::GitalyBackup do
       it 'passes parallel option through' do
         expect(Open3).to receive(:popen2).with(expected_env, anything, 'restore', '-path', anything, '-parallel', '3', '-layout', 'pointer').and_call_original
 
-        subject.start(:restore, destination)
+        subject.start(:restore, destination, backup_id: backup_id)
         subject.finish!
       end
     end
@@ -211,7 +211,7 @@ RSpec.describe Backup::GitalyBackup do
       it 'passes parallel option through' do
         expect(Open3).to receive(:popen2).with(expected_env, anything, 'restore', '-path', anything, '-parallel-storage', '3', '-layout', 'pointer').and_call_original
 
-        subject.start(:restore, destination)
+        subject.start(:restore, destination, backup_id: backup_id)
         subject.finish!
       end
     end
@@ -230,7 +230,7 @@ RSpec.describe Backup::GitalyBackup do
 
         expect(Open3).to receive(:popen2).with(expected_env, anything, 'restore', '-path', anything).and_call_original
 
-        subject.start(:restore, destination)
+        subject.start(:restore, destination, backup_id: backup_id)
         subject.enqueue(project, Gitlab::GlRepository::PROJECT)
         subject.enqueue(project, Gitlab::GlRepository::WIKI)
         subject.enqueue(project, Gitlab::GlRepository::DESIGN)
@@ -251,14 +251,14 @@ RSpec.describe Backup::GitalyBackup do
     it 'raises when the exit code not zero' do
       expect(subject).to receive(:bin_path).and_return(Gitlab::Utils.which('false'))
 
-      subject.start(:restore, destination)
+      subject.start(:restore, destination, backup_id: backup_id)
       expect { subject.finish! }.to raise_error(::Backup::Error, 'gitaly-backup exit status 1')
     end
 
     it 'raises when gitaly_backup_path is not set' do
       stub_backup_setting(gitaly_backup_path: nil)
 
-      expect { subject.start(:restore, destination) }.to raise_error(::Backup::Error, 'gitaly-backup binary not found and gitaly_backup_path is not configured')
+      expect { subject.start(:restore, destination, backup_id: backup_id) }.to raise_error(::Backup::Error, 'gitaly-backup binary not found and gitaly_backup_path is not configured')
     end
   end
 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,9 +23,9 @@ 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(:start).with(:create, destination, backup_id: backup_id)
         expect(strategy).to have_received(:enqueue).with(project, Gitlab::GlRepository::PROJECT)
         expect(strategy).to have_received(:enqueue).with(project, Gitlab::GlRepository::WIKI)
         expect(strategy).to have_received(:enqueue).with(project, Gitlab::GlRepository::DESIGN)
@@ -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