Merge branch 'imskr/gitlab-skr-rescue-errors' into 'master'

Raise custom errors when backup creation fails

See merge request gitlab-org/gitlab!76754

lib/backup.rb

@@ -16,19 +16,6 @@ module Backup
     end
   end
 
-  class RepositoryBackupError < Backup::Error
-    attr_reader :container, :backup_repos_path
-
-    def initialize(container, backup_repos_path)
-      @container = container
-      @backup_repos_path = backup_repos_path
-    end
-
-    def message
-      "Failed to create compressed file '#{backup_repos_path}' when trying to backup the following paths: '#{container.disk_path}'"
-    end
-  end
-
   class DatabaseBackupError < Backup::Error
     attr_reader :config, :db_file_name
 

lib/backup/database.rb

@@ -61,7 +61,7 @@ module Backup
       report_success(success)
       progress.flush
 
-      raise Backup::Error, 'Backup failed' unless success
+      raise DatabaseBackupError.new(config, db_file_name) unless success
     end
 
     def restore

lib/backup/files.rb

@@ -37,7 +37,7 @@ module Backup
 
         unless status == 0
           puts output
-          raise Backup::Error, 'Backup failed'
+          raise_custom_error
         end
 
         tar_cmd = [tar, exclude_dirs(:tar), %W[-C #{@backup_files_dir} -cf - .]].flatten
@@ -49,7 +49,7 @@ module Backup
       end
 
       unless pipeline_succeeded?(tar_status: status_list[0], gzip_status: status_list[1], output: output)
-        raise Backup::Error, "Backup operation failed: #{output}"
+        raise_custom_error
       end
     end
 
@@ -143,5 +143,9 @@ module Backup
         end
       end
     end
+
+    def raise_custom_error
+      raise FileBackupError.new(app_files_dir, backup_tarball)
+    end
   end
 end

lib/tasks/gitlab/backup.rake

@@ -135,8 +135,12 @@ namespace :gitlab do
         if ENV["SKIP"] && ENV["SKIP"].include?("db")
           puts_time "[SKIPPED]".color(:cyan)
         else
+          begin
             Backup::Database.new(progress).dump
             puts_time "done".color(:green)
+          rescue Backup::DatabaseBackupError => e
+            progress.puts "#{e.message}"
+          end
         end
       end
 
@@ -168,8 +172,12 @@ namespace :gitlab do
         if ENV["SKIP"] && ENV["SKIP"].include?("builds")
           puts_time "[SKIPPED]".color(:cyan)
         else
+          begin
             Backup::Builds.new(progress).dump
             puts_time "done".color(:green)
+          rescue Backup::FileBackupError => e
+            progress.puts "#{e.message}"
+          end
         end
       end
 
@@ -187,8 +195,12 @@ namespace :gitlab do
         if ENV["SKIP"] && ENV["SKIP"].include?("uploads")
           puts_time "[SKIPPED]".color(:cyan)
         else
+          begin
             Backup::Uploads.new(progress).dump
             puts_time "done".color(:green)
+          rescue Backup::FileBackupError => e
+            progress.puts "#{e.message}"
+          end
         end
       end
 
@@ -206,8 +218,12 @@ namespace :gitlab do
         if ENV["SKIP"] && ENV["SKIP"].include?("artifacts")
           puts_time "[SKIPPED]".color(:cyan)
         else
+          begin
             Backup::Artifacts.new(progress).dump
             puts_time "done".color(:green)
+          rescue Backup::FileBackupError => e
+            progress.puts "#{e.message}"
+          end
         end
       end
 
@@ -225,8 +241,12 @@ namespace :gitlab do
         if ENV["SKIP"] && ENV["SKIP"].include?("pages")
           puts_time "[SKIPPED]".color(:cyan)
         else
+          begin
             Backup::Pages.new(progress).dump
             puts_time "done".color(:green)
+          rescue Backup::FileBackupError => e
+            progress.puts "#{e.message}"
+          end
         end
       end
 
@@ -244,8 +264,12 @@ namespace :gitlab do
         if ENV["SKIP"] && ENV["SKIP"].include?("lfs")
           puts_time "[SKIPPED]".color(:cyan)
         else
+          begin
             Backup::Lfs.new(progress).dump
             puts_time "done".color(:green)
+          rescue Backup::FileBackupError => e
+            progress.puts "#{e.message}"
+          end
         end
       end
 
@@ -283,8 +307,12 @@ namespace :gitlab do
           if ENV["SKIP"] && ENV["SKIP"].include?("registry")
             puts_time "[SKIPPED]".color(:cyan)
           else
+            begin
               Backup::Registry.new(progress).dump
               puts_time "done".color(:green)
+            rescue Backup::FileBackupError => e
+              progress.puts "#{e.message}"
+            end
           end
         else
           puts_time "[DISABLED]".color(:cyan)

spec/lib/backup/files_spec.rb

@@ -134,7 +134,7 @@ RSpec.describe Backup::Files do
 
       expect do
         subject.dump
-      end.to raise_error(/Backup operation failed:/)
+      end.to raise_error(/Failed to create compressed file/)
     end
 
     describe 'with STRATEGY=copy' do
@@ -170,7 +170,7 @@ RSpec.describe Backup::Files do
         expect do
           subject.dump
         end.to output(/rsync failed/).to_stdout
-           .and raise_error(/Backup failed/)
+           .and raise_error(/Failed to create compressed file/)
       end
     end
   end

spec/lib/backup/repository_backup_error_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe Backup::RepositoryBackupError do
-  let_it_be(:snippet) { create(:snippet, content: 'foo', file_name: 'foo') }
-  let_it_be(:project) { create(:project, :repository) }
-  let_it_be(:wiki) { ProjectWiki.new(project, nil ) }
-
-  let(:backup_repos_path) { '/tmp/backup/repositories' }
-
-  shared_examples 'includes backup path' do
-    it { is_expected.to respond_to :container }
-    it { is_expected.to respond_to :backup_repos_path }
-
-    it 'expects exception message to include repo backup path location' do
-      expect(subject.message).to include("#{subject.backup_repos_path}")
-    end
-
-    it 'expects exception message to include container being back-up' do
-      expect(subject.message).to include("#{subject.container.disk_path}")
-    end
-  end
-
-  context 'with snippet repository' do
-    subject { described_class.new(snippet, backup_repos_path) }
-
-    it_behaves_like 'includes backup path'
-  end
-
-  context 'with project repository' do
-    subject { described_class.new(project, backup_repos_path) }
-
-    it_behaves_like 'includes backup path'
-  end
-
-  context 'with wiki repository' do
-    subject { described_class.new(wiki, backup_repos_path) }
-
-    it_behaves_like 'includes backup path'
-  end
-end

spec/tasks/gitlab/backup_rake_spec.rb

@@ -222,6 +222,37 @@ RSpec.describe 'gitlab:app namespace rake task', :delete do
       end
     end
 
+    describe 'backup create fails' do
+      using RSpec::Parameterized::TableSyntax
+
+      file_backup_error = Backup::FileBackupError.new('/tmp', '/tmp/backup/uploads')
+      config = ActiveRecord::Base.configurations.find_db_config(Rails.env).configuration_hash
+      db_file_name = File.join(Gitlab.config.backup.path, 'db', 'database.sql.gz')
+      db_backup_error = Backup::DatabaseBackupError.new(config, db_file_name)
+
+      where(:backup_class, :rake_task, :error) do
+        Backup::Database     | 'gitlab:backup:db:create'        | db_backup_error
+        Backup::Builds       | 'gitlab:backup:builds:create'    | file_backup_error
+        Backup::Uploads      | 'gitlab:backup:uploads:create'   | file_backup_error
+        Backup::Artifacts    | 'gitlab:backup:artifacts:create' | file_backup_error
+        Backup::Pages        | 'gitlab:backup:pages:create'     | file_backup_error
+        Backup::Lfs          | 'gitlab:backup:lfs:create'       | file_backup_error
+        Backup::Registry     | 'gitlab:backup:registry:create'  | file_backup_error
+      end
+
+      with_them do
+        before do
+          expect_next_instance_of(backup_class) do |instance|
+            expect(instance).to receive(:dump).and_raise(error)
+          end
+        end
+
+        it "raises an error with message" do
+          expect { run_rake_task(rake_task) }.to output(Regexp.new(error.message)).to_stdout_from_any_process
+        end
+      end
+    end
+
     context 'tar creation' do
       context 'archive file permissions' do
         it 'sets correct permissions on the tar file' do