4 files changed, 208 insertions, 15 deletions

diff --git a/spec/lib/backup/artifacts_spec.rb b/spec/lib/backup/artifacts_spec.rb
index 2a3f1949ba5..5a965030b01 100644
--- a/spec/lib/backup/artifacts_spec.rb
+++ b/spec/lib/backup/artifacts_spec.rb
@@ -30,7 +30,8 @@ RSpec.describe Backup::Artifacts do
 
     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)
+      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
     end
   end
diff --git a/spec/lib/backup/files_spec.rb b/spec/lib/backup/files_spec.rb
index 45cc73974d6..dbc04704fba 100644
--- a/spec/lib/backup/files_spec.rb
+++ b/spec/lib/backup/files_spec.rb
@@ -6,6 +6,10 @@ RSpec.describe Backup::Files do
   let(:progress) { StringIO.new }
   let!(:project) { create(:project) }
 
+  let(:status_0) { double('exit 0', success?: true, exitstatus: 0) }
+  let(:status_1) { double('exit 1', success?: false, exitstatus: 1) }
+  let(:status_2) { double('exit 2', success?: false, exitstatus: 2) }
+
   before do
     allow(progress).to receive(:puts)
     allow(progress).to receive(:print)
@@ -24,6 +28,20 @@ RSpec.describe Backup::Files do
     allow_any_instance_of(described_class).to receive(:progress).and_return(progress)
   end
 
+  RSpec::Matchers.define :eq_statuslist do |expected|
+    match do |actual|
+      actual.map(&:exitstatus) == expected.map(&:exitstatus)
+    end
+
+    description do
+      'be an Array of Process::Status with equal exitstatus against expected'
+    end
+
+    failure_message do |actual|
+      "expected #{actual} exitstatuses list to be equal #{expected} exitstatuses list"
+    end
+  end
+
   describe '#restore' do
     subject { described_class.new('registry', '/var/gitlab-registry') }
 
@@ -35,8 +53,9 @@ RSpec.describe Backup::Files do
 
     describe 'folders with permission' do
       before do
-        allow(subject).to receive(:run_pipeline!).and_return(true)
+        allow(subject).to receive(:run_pipeline!).and_return([[true, true], ''])
         allow(subject).to receive(:backup_existing_files).and_return(true)
+        allow(subject).to receive(:pipeline_succeeded?).and_return(true)
         allow(Dir).to receive(:glob).with("/var/gitlab-registry/*", File::FNM_DOTMATCH).and_return(["/var/gitlab-registry/.", "/var/gitlab-registry/..", "/var/gitlab-registry/sample1"])
       end
 
@@ -54,14 +73,22 @@ RSpec.describe Backup::Files do
         expect(subject).to receive(:tar).and_return('blabla-tar')
 
         expect(subject).to receive(:run_pipeline!).with([%w(gzip -cd), %w(blabla-tar --unlink-first --recursive-unlink -C /var/gitlab-registry -xf -)], any_args)
+        expect(subject).to receive(:pipeline_succeeded?).and_return(true)
         subject.restore
       end
+
+      it 'raises an error on failure' do
+        expect(subject).to receive(:pipeline_succeeded?).and_return(false)
+
+        expect { subject.restore }.to raise_error(/Restore operation failed:/)
+      end
     end
 
     describe 'folders without permissions' do
       before do
         allow(FileUtils).to receive(:mv).and_raise(Errno::EACCES)
-        allow(subject).to receive(:run_pipeline!).and_return(true)
+        allow(subject).to receive(:run_pipeline!).and_return([[true, true], ''])
+        allow(subject).to receive(:pipeline_succeeded?).and_return(true)
       end
 
       it 'shows error message' do
@@ -73,7 +100,8 @@ RSpec.describe Backup::Files do
     describe 'folders that are a mountpoint' do
       before do
         allow(FileUtils).to receive(:mv).and_raise(Errno::EBUSY)
-        allow(subject).to receive(:run_pipeline!).and_return(true)
+        allow(subject).to receive(:run_pipeline!).and_return([[true, true], ''])
+        allow(subject).to receive(:pipeline_succeeded?).and_return(true)
       end
 
       it 'shows error message' do
@@ -89,7 +117,8 @@ RSpec.describe Backup::Files do
     subject { described_class.new('pages', '/var/gitlab-pages', excludes: ['@pages.tmp']) }
 
     before do
-      allow(subject).to receive(:run_pipeline!).and_return(true)
+      allow(subject).to receive(:run_pipeline!).and_return([[true, true], ''])
+      allow(subject).to receive(:pipeline_succeeded?).and_return(true)
     end
 
     it 'raises no errors' do
@@ -103,29 +132,190 @@ RSpec.describe Backup::Files do
       subject.dump
     end
 
+    it 'raises an error on failure' do
+      allow(subject).to receive(:run_pipeline!).and_return([[true, true], ''])
+      expect(subject).to receive(:pipeline_succeeded?).and_return(false)
+
+      expect do
+        subject.dump
+      end.to raise_error(/Backup operation failed:/)
+    end
+
     describe 'with STRATEGY=copy' do
       before do
         stub_env('STRATEGY', 'copy')
-      end
-
-      it 'excludes tmp dirs from rsync' do
         allow(Gitlab.config.backup).to receive(:path) { '/var/gitlab-backup' }
         allow(File).to receive(:realpath).with("/var/gitlab-backup").and_return("/var/gitlab-backup")
+      end
 
+      it 'excludes tmp dirs from rsync' do
         expect(Gitlab::Popen).to receive(:popen).with(%w(rsync -a --exclude=lost+found --exclude=/@pages.tmp /var/gitlab-pages /var/gitlab-backup)).and_return(['', 0])
 
         subject.dump
       end
+
+      it 'raises an error and outputs an error message if rsync failed' do
+        allow(Gitlab::Popen).to receive(:popen).with(%w(rsync -a --exclude=lost+found --exclude=/@pages.tmp /var/gitlab-pages /var/gitlab-backup)).and_return(['rsync failed', 1])
+
+        expect do
+          subject.dump
+        end.to output(/rsync failed/).to_stdout
+           .and raise_error(/Backup failed/)
+      end
+    end
+  end
+
+  describe '#exclude_dirs' do
+    subject { described_class.new('pages', '/var/gitlab-pages', excludes: ['@pages.tmp']) }
+
+    it 'prepends a leading dot slash to tar excludes' 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'])
+    end
+  end
+
+  describe '#run_pipeline!' do
+    subject { described_class.new('registry', '/var/gitlab-registry') }
+
+    it 'executes an Open3.pipeline for cmd_list' do
+      expect(Open3).to receive(:pipeline).with(%w[whew command], %w[another cmd], any_args)
+
+      subject.run_pipeline!([%w[whew command], %w[another cmd]])
+    end
+
+    it 'returns an empty output on success pipeline' do
+      expect(subject.run_pipeline!(%w[true true])[1]).to eq('')
+    end
+
+    it 'returns the stderr for failed pipeline' do
+      expect(
+        subject.run_pipeline!(['echo OMG: failed command present 1>&2; false', 'true'])[1]
+      ).to match(/OMG: failed/)
+    end
+
+    it 'returns the success status list on success pipeline' do
+      expect(
+        subject.run_pipeline!(%w[true true])[0]
+      ).to eq_statuslist([status_0, status_0])
+    end
+
+    it 'returns the failed status in status list for failed commands in pipeline' do
+      expect(subject.run_pipeline!(%w[false true true])[0]).to eq_statuslist([status_1, status_0, status_0])
+      expect(subject.run_pipeline!(%w[true false true])[0]).to eq_statuslist([status_0, status_1, status_0])
+      expect(subject.run_pipeline!(%w[false false true])[0]).to eq_statuslist([status_1, status_1, status_0])
+      expect(subject.run_pipeline!(%w[false true false])[0]).to eq_statuslist([status_1, status_0, status_1])
+      expect(subject.run_pipeline!(%w[false false false])[0]).to eq_statuslist([status_1, status_1, status_1])
+    end
+  end
+
+  describe '#pipeline_succeeded?' do
+    subject { described_class.new('registry', '/var/gitlab-registry') }
+
+    it 'returns true if both tar and gzip succeeeded' do
+      expect(
+        subject.pipeline_succeeded?(tar_status: status_0, gzip_status: status_0, output: 'any_output')
+      ).to be_truthy
+    end
+
+    it 'returns false if gzip failed' do
+      expect(
+        subject.pipeline_succeeded?(tar_status: status_1, gzip_status: status_1, output: 'any_output')
+      ).to be_falsey
+    end
+
+    context 'if gzip succeeded and tar failed non-critically' do
+      before do
+        allow(subject).to receive(:tar_ignore_non_success?).and_return(true)
+      end
+
+      it 'returns true' do
+        expect(
+          subject.pipeline_succeeded?(tar_status: status_1, gzip_status: status_0, output: 'any_output')
+        ).to be_truthy
+      end
+    end
+
+    context 'if gzip succeeded and tar failed in other cases' do
+      before do
+        allow(subject).to receive(:tar_ignore_non_success?).and_return(false)
+      end
+
+      it 'returns false' do
+        expect(
+          subject.pipeline_succeeded?(tar_status: status_1, gzip_status: status_0, output: 'any_output')
+        ).to be_falsey
+      end
+    end
+  end
+
+  describe '#tar_ignore_non_success?' do
+    subject { described_class.new('registry', '/var/gitlab-registry') }
+
+    context 'if `tar` command exits with 1 exitstatus' do
+      it 'returns true' do
+        expect(
+          subject.tar_ignore_non_success?(1, 'any_output')
+        ).to be_truthy
+      end
+
+      it 'outputs a warning' do
+        expect do
+          subject.tar_ignore_non_success?(1, 'any_output')
+        end.to output(/Ignoring tar exit status 1/).to_stdout
+      end
+    end
+
+    context 'if `tar` command exits with 2 exitstatus with non-critical warning' do
+      before do
+        allow(subject).to receive(:noncritical_warning?).and_return(true)
+      end
+
+      it 'returns true' do
+        expect(
+          subject.tar_ignore_non_success?(2, 'any_output')
+        ).to be_truthy
+      end
+
+      it 'outputs a warning' do
+        expect do
+          subject.tar_ignore_non_success?(2, 'any_output')
+        end.to output(/Ignoring non-success exit status/).to_stdout
+      end
     end
 
-    describe '#exclude_dirs' do
-      it 'prepends a leading dot slash to tar excludes' do
-        expect(subject.exclude_dirs(:tar)).to eq(['--exclude=lost+found', '--exclude=./@pages.tmp'])
+    context 'if `tar` command exits with any other unlisted error' do
+      before do
+        allow(subject).to receive(:noncritical_warning?).and_return(false)
       end
 
-      it 'prepends a leading slash to rsync excludes' do
-        expect(subject.exclude_dirs(:rsync)).to eq(['--exclude=lost+found', '--exclude=/@pages.tmp'])
+      it 'returns false' do
+        expect(
+          subject.tar_ignore_non_success?(2, 'any_output')
+        ).to be_falsey
       end
     end
   end
+
+  describe '#noncritical_warning?' do
+    subject { described_class.new('registry', '/var/gitlab-registry') }
+
+    it 'returns true if given text matches noncritical warnings list' do
+      expect(
+        subject.noncritical_warning?('tar: .: Cannot mkdir: No such file or directory')
+      ).to be_truthy
+
+      expect(
+        subject.noncritical_warning?('gtar: .: Cannot mkdir: No such file or directory')
+      ).to be_truthy
+    end
+
+    it 'returns false otherwize' do
+      expect(
+        subject.noncritical_warning?('unknown message')
+      ).to be_falsey
+    end
+  end
 end
diff --git a/spec/lib/backup/pages_spec.rb b/spec/lib/backup/pages_spec.rb
index 59df4d1adf7..551d2df8f30 100644
--- a/spec/lib/backup/pages_spec.rb
+++ b/spec/lib/backup/pages_spec.rb
@@ -23,7 +23,8 @@ RSpec.describe Backup::Pages do
       allow(Gitlab.config.pages).to receive(:path) { '/var/gitlab-pages' }
 
       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)
+      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
     end
   end
diff --git a/spec/lib/backup/uploads_spec.rb b/spec/lib/backup/uploads_spec.rb
index 678b670db34..a82cb764f4d 100644
--- a/spec/lib/backup/uploads_spec.rb
+++ b/spec/lib/backup/uploads_spec.rb
@@ -32,7 +32,8 @@ RSpec.describe Backup::Uploads do
 
     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)
+      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
     end
   end