Automatic merge of gitlab-org/gitlab master

app/models/remote_mirror.rb

@@ -84,13 +84,7 @@ class RemoteMirror < ApplicationRecord
     end
 
     after_transition started: :failed do |remote_mirror|
-      Gitlab::Metrics.add_event(:remote_mirrors_failed)
-
-      remote_mirror.update(last_update_at: Time.current)
-
-      remote_mirror.run_after_commit do
-        RemoteMirrorNotificationWorker.perform_async(remote_mirror.id)
-      end
+      remote_mirror.send_failure_notifications
     end
   end
 
@@ -188,6 +182,24 @@ class RemoteMirror < ApplicationRecord
     update_fail!
   end
 
+  # Force the mrror into the retry state
+  def hard_retry!(error_message)
+    update_error_message(error_message)
+    self.update_status = :to_retry
+
+    save!(validate: false)
+  end
+
+  # Force the mirror into the failed state
+  def hard_fail!(error_message)
+    update_error_message(error_message)
+    self.update_status = :failed
+
+    save!(validate: false)
+
+    send_failure_notifications
+  end
+
   def url=(value)
     super(value) && return unless Gitlab::UrlSanitizer.valid?(value)
 
@@ -239,6 +251,17 @@ class RemoteMirror < ApplicationRecord
     last_update_at.present? ? MAX_INCREMENTAL_RUNTIME : MAX_FIRST_RUNTIME
   end
 
+  def send_failure_notifications
+    Gitlab::Metrics.add_event(:remote_mirrors_failed)
+
+    run_after_commit do
+      RemoteMirrorNotificationWorker.perform_async(id)
+    end
+
+    self.last_update_at = Time.current
+    save!(validate: false)
+  end
+
   private
 
   def store_credentials

app/services/packages/debian/extract_changes_metadata_service.rb

@@ -42,9 +42,9 @@ module Packages
       def files
         strong_memoize(:files) do
           raise ExtractionError.new("is not a changes file") unless file_type == :changes
-          raise ExtractionError.new("Files field is missing") if fields[:Files].blank?
-          raise ExtractionError.new("Checksums-Sha1 field is missing") if fields[:'Checksums-Sha1'].blank?
-          raise ExtractionError.new("Checksums-Sha256 field is missing") if fields[:'Checksums-Sha256'].blank?
+          raise ExtractionError.new("Files field is missing") if fields['Files'].blank?
+          raise ExtractionError.new("Checksums-Sha1 field is missing") if fields['Checksums-Sha1'].blank?
+          raise ExtractionError.new("Checksums-Sha256 field is missing") if fields['Checksums-Sha256'].blank?
 
           init_entries_from_files
           entries_from_checksums_sha1
@@ -56,7 +56,7 @@ module Packages
       end
 
       def init_entries_from_files
-        each_lines_for(:Files) do |line|
+        each_lines_for('Files') do |line|
           md5sum, size, section, priority, filename = line.split
           entry = FileEntry.new(
             filename: filename,
@@ -70,7 +70,7 @@ module Packages
       end
 
       def entries_from_checksums_sha1
-        each_lines_for(:'Checksums-Sha1') do |line|
+        each_lines_for('Checksums-Sha1') do |line|
           sha1sum, size, filename = line.split
           entry = @entries[filename]
           raise ExtractionError.new("#{filename} is listed in Checksums-Sha1 but not in Files") unless entry
@@ -81,7 +81,7 @@ module Packages
       end
 
       def entries_from_checksums_sha256
-        each_lines_for(:'Checksums-Sha256') do |line|
+        each_lines_for('Checksums-Sha256') do |line|
           sha256sum, size, filename = line.split
           entry = @entries[filename]
           raise ExtractionError.new("#{filename} is listed in Checksums-Sha256 but not in Files") unless entry

app/services/packages/debian/extract_metadata_service.rb

@@ -72,7 +72,7 @@ module Packages
 
       def extract_metadata
         fields = extracted_fields
-        architecture = fields.delete(:Architecture) if file_type_debian?
+        architecture = fields.delete('Architecture') if file_type_debian?
 
         {
           file_type: file_type,

app/services/packages/debian/parse_debian822_service.rb

@@ -26,7 +26,7 @@ module Packages
               section[field] += line[1..] unless paragraph_separator?(line)
             elsif match = match_section_line(line)
               section_name = match[:name] if section_name.nil?
-              field = match[:field].to_sym
+              field = match[:field]
 
               raise InvalidDebian822Error, "Duplicate field '#{field}' in section '#{section_name}'" if section.include?(field)
 

app/services/projects/update_remote_mirror_service.rb

@@ -9,8 +9,10 @@ module Projects
     def execute(remote_mirror, tries)
       return success unless remote_mirror.enabled?
 
+      # Blocked URLs are a hard failure, no need to attempt to retry
       if Gitlab::UrlBlocker.blocked_url?(normalized_url(remote_mirror.url))
-        return error("The remote mirror URL is invalid.")
+        hard_retry_or_fail(remote_mirror, _('The remote mirror URL is invalid.'), tries)
+        return error(remote_mirror.last_error)
       end
 
       update_mirror(remote_mirror)
@@ -19,11 +21,11 @@ module Projects
     rescue Gitlab::Git::CommandError => e
       # This happens if one of the gitaly calls above fail, for example when
       # branches have diverged, or the pre-receive hook fails.
-      retry_or_fail(remote_mirror, e.message, tries)
+      hard_retry_or_fail(remote_mirror, e.message, tries)
 
       error(e.message)
     rescue => e
-      remote_mirror.mark_as_failed!(e.message)
+      remote_mirror.hard_fail!(e.message)
       raise e
     end
 
@@ -70,15 +72,15 @@ module Projects
       ).execute
     end
 
-    def retry_or_fail(mirror, message, tries)
+    def hard_retry_or_fail(mirror, message, tries)
       if tries < MAX_TRIES
-        mirror.mark_for_retry!(message)
+        mirror.hard_retry!(message)
       else
         # It's not likely we'll be able to recover from this ourselves, so we'll
         # notify the users of the problem, and don't trigger any sidekiq retries
         # Instead, we'll wait for the next change to try the push again, or until
         # a user manually retries.
-        mirror.mark_as_failed!(message)
+        mirror.hard_fail!(message)
       end
     end
   end

changelogs/unreleased/300884-fix-push-mirror-failures.yml

+---
+title: A blocked URL for a push mirror is a hard failure
+merge_request: 57392
+author:
+type: fixed

lib/gitlab/uuid.rb

@@ -9,7 +9,7 @@ module Gitlab
       production: "58dc0f06-936c-43b3-93bb-71693f1b6570"
     }.freeze
 
-    UUID_V5_PATTERN = /\h{8}-\h{4}-5\h{3}-\h{4}-\h{4}\h{8}/.freeze
+    UUID_V5_PATTERN = /\h{8}-\h{4}-5\h{3}-\h{4}-\h{12}/.freeze
     NAMESPACE_REGEX = /(\h{8})-(\h{4})-(\h{4})-(\h{4})-(\h{4})(\h{8})/.freeze
     PACK_PATTERN = "NnnnnN"
 

locale/gitlab.pot

@@ -30450,6 +30450,9 @@ msgstr ""
 msgid "The regular expression used to find test coverage output in the job log. For example, use %{regex} for Simplecov (Ruby). Leave blank to disable."
 msgstr ""
 
+msgid "The remote mirror URL is invalid."
+msgstr ""
+
 msgid "The remote mirror took to long to complete."
 msgstr ""
 

spec/models/remote_mirror_spec.rb

@@ -263,6 +263,30 @@ RSpec.describe RemoteMirror, :mailer do
     end
   end
 
+  describe '#hard_retry!' do
+    let(:remote_mirror) { create(:remote_mirror).tap {|mirror| mirror.update_column(:url, 'invalid') } }
+
+    it 'transitions an invalid mirror to the to_retry state' do
+      remote_mirror.hard_retry!('Invalid')
+
+      expect(remote_mirror.update_status).to eq('to_retry')
+      expect(remote_mirror.last_error).to eq('Invalid')
+    end
+  end
+
+  describe '#hard_fail!' do
+    let(:remote_mirror) { create(:remote_mirror).tap {|mirror| mirror.update_column(:url, 'invalid') } }
+
+    it 'transitions an invalid mirror to the failed state' do
+      remote_mirror.hard_fail!('Invalid')
+
+      expect(remote_mirror.update_status).to eq('failed')
+      expect(remote_mirror.last_error).to eq('Invalid')
+      expect(remote_mirror.last_update_at).not_to be_nil
+      expect(RemoteMirrorNotificationWorker.jobs).not_to be_empty
+    end
+  end
+
   context 'when remote mirror gets destroyed' do
     it 'removes remote' do
       mirror = create_mirror(url: 'http://foo:bar@test.com')

spec/services/packages/debian/extract_changes_metadata_service_spec.rb

@@ -13,7 +13,7 @@ RSpec.describe Packages::Debian::ExtractChangesMetadataService do
 
     context 'with valid package file' do
       it 'extract metadata', :aggregate_failures do
-        expected_fields = { 'Architecture': 'source amd64', 'Binary': 'libsample0 sample-dev sample-udeb' }
+        expected_fields = { 'Architecture' => 'source amd64', 'Binary' => 'libsample0 sample-dev sample-udeb' }
 
         expect(subject[:file_type]).to eq(:changes)
         expect(subject[:architecture]).to be_nil
@@ -40,7 +40,7 @@ RSpec.describe Packages::Debian::ExtractChangesMetadataService do
       let(:sha256_dsc) { '844f79825b7e8aaa191e514b58a81f9ac1e58e2180134b0c9512fa66d896d7ba 671 sample_1.2.3~alpha2.dsc' }
       let(:sha256_source) { 'b5a599e88e7cbdda3bde808160a21ba1dd1ec76b2ec8d4912aae769648d68362 864 sample_1.2.3~alpha2.tar.xz' }
       let(:sha256s) { "#{sha256_dsc}\n#{sha256_source}" }
-      let(:fields) { { Files: md5s, 'Checksums-Sha1': sha1s, 'Checksums-Sha256': sha256s } }
+      let(:fields) { { 'Files' => md5s, 'Checksums-Sha1' => sha1s, 'Checksums-Sha256' => sha256s } }
       let(:metadata) { { file_type: :changes, architecture: 'amd64', fields: fields } }
 
       before do

spec/services/packages/debian/extract_deb_metadata_service_spec.rb

@@ -10,17 +10,17 @@ RSpec.describe Packages::Debian::ExtractDebMetadataService do
   context 'with correct file' do
     it 'return as expected' do
       expected = {
-        'Package': 'libsample0',
-        'Source': 'sample',
-        'Version': '1.2.3~alpha2',
-        'Architecture': 'amd64',
-        'Maintainer': 'John Doe <john.doe@example.com>',
-        'Installed-Size': '7',
-        'Section': 'libs',
-        'Priority': 'optional',
-        'Multi-Arch': 'same',
-        'Homepage': 'https://gitlab.com/',
-        'Description': "Some mostly empty lib\nUsed in GitLab tests.\n\nTesting another paragraph."
+        'Package' => 'libsample0',
+        'Source' => 'sample',
+        'Version' => '1.2.3~alpha2',
+        'Architecture' => 'amd64',
+        'Maintainer' => 'John Doe <john.doe@example.com>',
+        'Installed-Size' => '7',
+        'Section' => 'libs',
+        'Priority' => 'optional',
+        'Multi-Arch' => 'same',
+        'Homepage' => 'https://gitlab.com/',
+        'Description' => "Some mostly empty lib\nUsed in GitLab tests.\n\nTesting another paragraph."
       }
 
       expect(subject.execute).to eq expected

spec/services/packages/debian/extract_metadata_service_spec.rb

@@ -33,11 +33,11 @@ RSpec.describe Packages::Debian::ExtractMetadataService do
   where(:case_name, :trait, :expected_file_type, :expected_architecture, :expected_fields) do
     'with invalid'    | :invalid   | :unknown   | nil     | nil
     'with source'     | :source    | :source    | nil     | nil
-    'with dsc'        | :dsc       | :dsc       | nil     | { 'Binary': 'sample-dev, libsample0, sample-udeb' }
-    'with deb'        | :deb       | :deb       | 'amd64' | { 'Multi-Arch': 'same' }
-    'with udeb'       | :udeb      | :udeb      | 'amd64' | { 'Package': 'sample-udeb' }
-    'with buildinfo'  | :buildinfo | :buildinfo | nil     | { 'Architecture': 'amd64 source', 'Build-Architecture': 'amd64' }
-    'with changes'    | :changes   | :changes   | nil     | { 'Architecture': 'source amd64', 'Binary': 'libsample0 sample-dev sample-udeb' }
+    'with dsc'        | :dsc       | :dsc       | nil     | { 'Binary' => 'sample-dev, libsample0, sample-udeb' }
+    'with deb'        | :deb       | :deb       | 'amd64' | { 'Multi-Arch' => 'same' }
+    'with udeb'       | :udeb      | :udeb      | 'amd64' | { 'Package' => 'sample-udeb' }
+    'with buildinfo'  | :buildinfo | :buildinfo | nil     | { 'Architecture' => 'amd64 source', 'Build-Architecture' => 'amd64' }
+    'with changes'    | :changes   | :changes   | nil     | { 'Architecture' => 'source amd64', 'Binary' => 'libsample0 sample-dev sample-udeb' }
   end
 
   with_them do

spec/services/packages/debian/parse_debian822_service_spec.rb

@@ -27,17 +27,17 @@ RSpec.describe Packages::Debian::ParseDebian822Service do
     it 'return as expected, preserving order' do
       expected = {
         'Package: libsample0' => {
-          'Package': 'libsample0',
-          'Source': 'sample',
-          'Version': '1.2.3~alpha2',
-          'Architecture': 'amd64',
-          'Maintainer': 'John Doe <john.doe@example.com>',
-          'Installed-Size': '9',
-          'Section': 'libs',
-          'Priority': 'optional',
-          'Multi-Arch': 'same',
-          'Homepage': 'https://gitlab.com/',
-          'Description': "Some mostly empty lib\nUsed in GitLab tests.\n\nTesting another paragraph."
+          'Package' => 'libsample0',
+          'Source' => 'sample',
+          'Version' => '1.2.3~alpha2',
+          'Architecture' => 'amd64',
+          'Maintainer' => 'John Doe <john.doe@example.com>',
+          'Installed-Size' => '9',
+          'Section' => 'libs',
+          'Priority' => 'optional',
+          'Multi-Arch' => 'same',
+          'Homepage' => 'https://gitlab.com/',
+          'Description' => "Some mostly empty lib\nUsed in GitLab tests.\n\nTesting another paragraph."
         }
       }
 
@@ -51,38 +51,38 @@ RSpec.describe Packages::Debian::ParseDebian822Service do
     it 'return as expected, preserving order' do
       expected = {
         'Source: sample' => {
-          'Source': 'sample',
-          'Priority': 'optional',
-          'Maintainer': 'John Doe <john.doe@example.com>',
-          'Build-Depends': 'debhelper-compat (= 13)',
-          'Standards-Version': '4.5.0',
-          'Section': 'libs',
-          'Homepage': 'https://gitlab.com/',
-          # 'Vcs-Browser': 'https://salsa.debian.org/debian/sample-1.2.3',
-          # '#Vcs-Git': 'https://salsa.debian.org/debian/sample-1.2.3.git',
-          'Rules-Requires-Root': 'no'
+          'Source' => 'sample',
+          'Priority' => 'optional',
+          'Maintainer' => 'John Doe <john.doe@example.com>',
+          'Build-Depends' => 'debhelper-compat (= 13)',
+          'Standards-Version' => '4.5.0',
+          'Section' => 'libs',
+          'Homepage' => 'https://gitlab.com/',
+          # 'Vcs-Browser' => 'https://salsa.debian.org/debian/sample-1.2.3',
+          # '#Vcs-Git' => 'https://salsa.debian.org/debian/sample-1.2.3.git',
+          'Rules-Requires-Root' => 'no'
         },
         'Package: sample-dev' => {
-          'Package': 'sample-dev',
-          'Section': 'libdevel',
-          'Architecture': 'any',
-          'Multi-Arch': 'same',
-          'Depends': 'libsample0 (= ${binary:Version}), ${misc:Depends}',
-          'Description': "Some mostly empty developpement files\nUsed in GitLab tests.\n\nTesting another paragraph."
+          'Package' => 'sample-dev',
+          'Section' => 'libdevel',
+          'Architecture' => 'any',
+          'Multi-Arch' => 'same',
+          'Depends' => 'libsample0 (= ${binary:Version}), ${misc:Depends}',
+          'Description' => "Some mostly empty developpement files\nUsed in GitLab tests.\n\nTesting another paragraph."
         },
         'Package: libsample0' => {
-          'Package': 'libsample0',
-          'Architecture': 'any',
-          'Multi-Arch': 'same',
-          'Depends': '${shlibs:Depends}, ${misc:Depends}',
-          'Description': "Some mostly empty lib\nUsed in GitLab tests.\n\nTesting another paragraph."
+          'Package' => 'libsample0',
+          'Architecture' => 'any',
+          'Multi-Arch' => 'same',
+          'Depends' => '${shlibs:Depends}, ${misc:Depends}',
+          'Description' => "Some mostly empty lib\nUsed in GitLab tests.\n\nTesting another paragraph."
          },
          'Package: sample-udeb' => {
-           'Package': 'sample-udeb',
-           'Package-Type': 'udeb',
-           'Architecture': 'any',
-           'Depends': 'installed-base',
-           'Description': 'Some mostly empty udeb'
+           'Package' => 'sample-udeb',
+           'Package-Type' => 'udeb',
+           'Architecture' => 'any',
+           'Depends' => 'installed-base',
+           'Description' => 'Some mostly empty udeb'
          }
       }
 

spec/services/projects/update_remote_mirror_service_spec.rb

@@ -12,7 +12,9 @@ RSpec.describe Projects::UpdateRemoteMirrorService do
   subject(:service) { described_class.new(project, project.creator) }
 
   describe '#execute' do
-    subject(:execute!) { service.execute(remote_mirror, 0) }
+    let(:retries) { 0 }
+
+    subject(:execute!) { service.execute(remote_mirror, retries) }
 
     before do
       project.repository.add_branch(project.owner, 'existing-branch', 'master')
@@ -62,8 +64,18 @@ RSpec.describe Projects::UpdateRemoteMirrorService do
         allow(Gitlab::UrlBlocker).to receive(:blocked_url?).and_return(true)
       end
 
-      it 'fails and returns error status' do
+      it 'hard retries and returns error status' do
         expect(execute!).to eq(status: :error, message: 'The remote mirror URL is invalid.')
+        expect(remote_mirror).to be_to_retry
+      end
+
+      context 'when retries are exceeded' do
+        let(:retries) { 4 }
+
+        it 'hard fails and returns error status' do
+          expect(execute!).to eq(status: :error, message: 'The remote mirror URL is invalid.')
+          expect(remote_mirror).to be_failed
+        end
       end
     end