Merge branch '7314-backup-files-noncritical-warnings' into 'master'

Fix backup error due to file changed warning

See merge request gitlab-org/gitlab!42197

changelogs/unreleased/7314-backup-files-noncritical-warnings.yml

+---
+title: Show tar warning message when file/folder changed during backup instead of
+  failing whole backup operation
+merge_request: 42197
+author:
+type: fixed

lib/backup/files.rb

@@ -26,7 +26,7 @@ module Backup
       FileUtils.rm_f(backup_tarball)
 
       if ENV['STRATEGY'] == 'copy'
-        cmd = [%w(rsync -a), exclude_dirs(:rsync), %W(#{app_files_dir} #{Gitlab.config.backup.path})].flatten
+        cmd = [%w[rsync -a], exclude_dirs(:rsync), %W[#{app_files_dir} #{Gitlab.config.backup.path}]].flatten
         output, status = Gitlab::Popen.popen(cmd)
 
         unless status == 0
@@ -34,19 +34,27 @@ module Backup
           raise Backup::Error, 'Backup failed'
         end
 
-        tar_cmd = [tar, exclude_dirs(:tar), %W(-C #{@backup_files_dir} -cf - .)].flatten
-        run_pipeline!([tar_cmd, gzip_cmd], out: [backup_tarball, 'w', 0600])
+        tar_cmd = [tar, exclude_dirs(:tar), %W[-C #{@backup_files_dir} -cf - .]].flatten
+        status_list, output = run_pipeline!([tar_cmd, gzip_cmd], out: [backup_tarball, 'w', 0600])
         FileUtils.rm_rf(@backup_files_dir)
       else
-        tar_cmd = [tar, exclude_dirs(:tar), %W(-C #{app_files_dir} -cf - .)].flatten
-        run_pipeline!([tar_cmd, gzip_cmd], out: [backup_tarball, 'w', 0600])
+        tar_cmd = [tar, exclude_dirs(:tar), %W[-C #{app_files_dir} -cf - .]].flatten
+        status_list, output = run_pipeline!([tar_cmd, gzip_cmd], out: [backup_tarball, 'w', 0600])
+      end
+
+      unless pipeline_succeeded?(tar_status: status_list[0], gzip_status: status_list[1], output: output)
+        raise Backup::Error, "Backup operation failed: #{output}"
       end
     end
 
     def restore
       backup_existing_files_dir
 
-      run_pipeline!([%w(gzip -cd), %W(#{tar} --unlink-first --recursive-unlink -C #{app_files_dir} -xf -)], in: backup_tarball)
+      cmd_list = [%w[gzip -cd], %W[#{tar} --unlink-first --recursive-unlink -C #{app_files_dir} -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}"
+      end
     end
 
     def tar
@@ -78,13 +86,44 @@ module Backup
     def run_pipeline!(cmd_list, options = {})
       err_r, err_w = IO.pipe
       options[:err] = err_w
-      status = Open3.pipeline(*cmd_list, options)
+      status_list = Open3.pipeline(*cmd_list, options)
       err_w.close
-      return if status.compact.all?(&:success?)
 
-      regex = /^g?tar: \.: Cannot mkdir: No such file or directory$/
-      error = err_r.read
-      raise Backup::Error, "Backup failed. #{error}" unless error =~ regex
+      [status_list, err_r.read]
+    end
+
+    def noncritical_warning?(warning)
+      noncritical_warnings = [
+        /^g?tar: \.: Cannot mkdir: No such file or directory$/
+      ]
+
+      noncritical_warnings.map { |w| warning =~ w }.any?
+    end
+
+    def pipeline_succeeded?(tar_status:, gzip_status:, output:)
+      return false unless gzip_status&.success?
+
+      tar_status&.success? || tar_ignore_non_success?(tar_status.exitstatus, output)
+    end
+
+    def tar_ignore_non_success?(exitstatus, output)
+      # tar can exit with nonzero code:
+      #  1 - if some files changed (i.e. a CI job is currently writes to log)
+      #  2 - if it cannot create `.` directory (see issue https://gitlab.com/gitlab-org/gitlab/-/issues/22442)
+      #  http://www.gnu.org/software/tar/manual/html_section/tar_19.html#Synopsis
+      #  so check tar status 1 or stderr output against some non-critical warnings
+      if exitstatus == 1
+        $stdout.puts "Ignoring tar exit status 1 'Some files differ': #{output}"
+        return true
+      end
+
+      # allow tar to fail with other non-success status if output contain non-critical warning
+      if noncritical_warning?(output)
+        $stdout.puts "Ignoring non-success exit status #{exitstatus} due to output of non-critical warning(s): #{output}"
+        return true
+      end
+
+      false
     end
 
     def exclude_dirs(fmt)

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

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,22 +132,42 @@ 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
@@ -127,5 +176,146 @@ RSpec.describe Backup::Files 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
+
+    context 'if `tar` command exits with any other unlisted error' do
+      before do
+        allow(subject).to receive(:noncritical_warning?).and_return(false)
+      end
+
+      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

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

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