Merge branch '2896-error-out-clone-url-prefix-nil' into 'master'

Geo: improve error message when clone_url_prefix is nil

Closes #2896

See merge request !2410

app/services/geo/base_sync_service.rb

 module Geo
+  # The clone_url_prefix is used to build URLs for the Geo synchronization
+  # If this is missing from the primary node we raise this exception
+  EmptyCloneUrlPrefixError = Class.new(StandardError)
+
   class BaseSyncService
     class << self
       attr_accessor :type
@@ -25,8 +29,18 @@ module Geo
       @lease_key ||= "#{LEASE_KEY_PREFIX}:#{type}:#{project.id}"
     end
 
+    def primary_ssh_path_prefix
+      @primary_ssh_path_prefix ||= Gitlab::Geo.primary_node.clone_url_prefix.tap do |prefix|
+        raise EmptyCloneUrlPrefixError, 'Missing clone_url_prefix in the primary node' unless prefix.present?
+      end
+    end
+
     private
 
+    def sync_repository
+      raise NotImplementedError, 'This class should implement sync_repository method'
+    end
+
     def registry
       @registry ||= Geo::ProjectRegistry.find_or_initialize_by(project_id: project.id)
     end
@@ -70,10 +84,6 @@ module Geo
       self.class.type
     end
 
-    def primary_ssh_path_prefix
-      @primary_ssh_path_prefix ||= Gitlab::Geo.primary_node.clone_url_prefix
-    end
-
     def log(message)
       Rails.logger.info("#{self.class.name}: #{message} for project #{project.path_with_namespace} (#{project.id})")
     end

app/services/geo/repository_sync_service.rb

@@ -18,7 +18,7 @@ module Geo
         project.repository.fetch_geo_mirror(ssh_url_to_repo)
 
         update_registry(:repository, finished_at: DateTime.now)
-      rescue Gitlab::Shell::Error => e
+      rescue Gitlab::Shell::Error, Geo::EmptyCloneUrlPrefixError => e
         Rails.logger.error("#{self.class.name}: Error syncing repository for project #{project.path_with_namespace}: #{e}")
       rescue Gitlab::Git::Repository::NoRepository => e
         Rails.logger.error("#{self.class.name}: Error invalid repository for project #{project.path_with_namespace}: #{e}")

app/services/geo/wiki_sync_service.rb

@@ -17,7 +17,10 @@ module Geo
         project.wiki.repository.fetch_geo_mirror(ssh_url_to_wiki)
 
         update_registry(:wiki, finished_at: DateTime.now)
-      rescue Gitlab::Git::Repository::NoRepository, Gitlab::Shell::Error, ProjectWiki::CouldNotCreateWikiError => e
+      rescue Gitlab::Git::Repository::NoRepository,
+             Gitlab::Shell::Error,
+             ProjectWiki::CouldNotCreateWikiError,
+             Geo::EmptyCloneUrlPrefixError => e
         Rails.logger.error("#{self.class.name}: Error syncing wiki repository for project #{project.path_with_namespace}: #{e}")
       end
     end

lib/gitlab/geo.rb

@@ -8,7 +8,6 @@ module Gitlab
       geo_node_enabled
       geo_node_primary
       geo_node_secondary
-      geo_primary_ssh_path_prefix
       geo_oauth_application
     ).freeze
 

spec/services/geo/base_sync_service_spec.rb

+require 'spec_helper'
+
+describe Geo::BaseSyncService, services: true do
+  let(:project) { build('project')}
+  subject { described_class.new(project) }
+
+  it_behaves_like 'geo base sync execution'
+
+  describe '#lease_key' do
+    it 'returns a key in the correct pattern' do
+      described_class.type = :test
+      allow(project).to receive(:id) { 999 }
+
+      expect(subject.lease_key).to eq('geo_sync_service:test:999')
+    end
+  end
+
+  describe '#primary_ssh_path_prefix' do
+    let!(:primary_node) { create(:geo_node, :primary, host: 'primary-geo-node') }
+
+    it 'raises exception when clone_url_prefix is nil' do
+      allow_any_instance_of(GeoNode).to receive(:clone_url_prefix) { nil }
+
+      expect { subject.primary_ssh_path_prefix }.to raise_error Geo::EmptyCloneUrlPrefixError
+    end
+
+    it 'returns the prefix defined in the primary node' do
+      expect(subject.primary_ssh_path_prefix).to eq('git@localhost:')
+    end
+  end
+end

spec/services/geo/repository_sync_service_spec.rb

@@ -6,6 +6,13 @@ RSpec.describe Geo::RepositorySyncService, services: true do
 
   subject { described_class.new(project) }
 
+  it_behaves_like 'geo base sync execution'
+
+  describe '#execute' do
+    let(:project) { create(:project_empty_repo) }
+    let(:repository) { project.repository }
+    let(:url_to_repo) { "#{primary.clone_url_prefix}#{project.path_with_namespace}.git" }
+
     before do
       allow(Gitlab::ExclusiveLease).to receive(:new)
         .with(subject.lease_key, anything)
@@ -15,11 +22,6 @@ RSpec.describe Geo::RepositorySyncService, services: true do
         .and_return(true)
     end
 
-  describe '#execute' do
-    let(:project) { create(:project_empty_repo) }
-    let(:repository) { project.repository }
-    let(:url_to_repo) { "#{primary.clone_url_prefix}#{project.path_with_namespace}.git" }
-
     it 'fetches project repository' do
       expect(repository).to receive(:fetch_geo_mirror).with(url_to_repo).once
 

spec/services/geo/wiki_sync_service_spec.rb

@@ -6,6 +6,13 @@ RSpec.describe Geo::WikiSyncService, services: true do
 
   subject { described_class.new(project) }
 
+  it_behaves_like 'geo base sync execution'
+
+  describe '#execute' do
+    let(:project) { create(:project_empty_repo) }
+    let(:repository) { project.wiki.repository }
+    let(:url_to_repo) { "#{primary.clone_url_prefix}#{project.path_with_namespace}.wiki.git" }
+
     before do
       allow(Gitlab::ExclusiveLease).to receive(:new)
         .with(subject.lease_key, anything)
@@ -15,11 +22,6 @@ RSpec.describe Geo::WikiSyncService, services: true do
         .and_return(true)
     end
 
-  describe '#execute' do
-    let(:project) { create(:project_empty_repo) }
-    let(:repository) { project.wiki.repository }
-    let(:url_to_repo) { "#{primary.clone_url_prefix}#{project.path_with_namespace}.wiki.git" }
-
     it 'fetches wiki repository' do
       expect(repository).to receive(:fetch_geo_mirror).with(url_to_repo).once
 

spec/support/services/base_sync_service_shared_examples.rb

+shared_examples 'geo base sync execution' do
+  describe '#execute' do
+    let(:project) { build('project')}
+
+    context 'when can acquire exclusive lease' do
+      before do
+        allow_any_instance_of(Gitlab::ExclusiveLease).to receive(:try_obtain) { 12345 }
+      end
+
+      it 'executes the synchronization' do
+        expect(subject).to receive(:sync_repository)
+
+        subject.execute
+      end
+    end
+
+    context 'when exclusive lease is not acquired' do
+      before do
+        allow_any_instance_of(Gitlab::ExclusiveLease).to receive(:try_obtain) { nil }
+      end
+
+      it 'is does not execute synchronization' do
+        expect(subject).not_to receive(:sync_repository)
+
+        subject.execute
+      end
+    end
+  end
+end