Merge branch 'sh-fix-geo-secondary-check' into 'master'

Geo: Ensure database is connected before attempting to check for secondary status

Closes #3074

See merge request gitlab-org/gitlab-ee!3112

changelogs/unreleased-ee/sh-fix-geo-secondary-check.yml

+---
+title: 'Geo: Ensure database is connected before attempting to check for secondary
+  status'
+merge_request:
+author:
+type: fixed

config/initializers/geo.rb

@@ -5,7 +5,7 @@ if File.exist?(Rails.root.join('config/database_geo.yml'))
 end
 
 begin
-  if Gitlab::Geo.primary?
+  if Gitlab::Geo.connected? && Gitlab::Geo.primary?
     Gitlab::Geo.current_node&.update_clone_url!
   end
 rescue => e

lib/gitlab/geo.rb

@@ -32,13 +32,20 @@ module Gitlab
       self.cache_value(:geo_secondary_nodes) { GeoNode.where(primary: false) }
     end
 
+    def self.connected?
+      Gitlab::Database.postgresql? && GeoNode.connected? && GeoNode.table_exists?
+    end
+
     def self.enabled?
-      GeoNode.connected? && self.cache_value(:geo_node_enabled) { GeoNode.exists? }
-    rescue => e
-      # We can't use the actual classes in rescue because we load only one of them based on database supported
-      raise e unless %w(PG::UndefinedTable Mysql2::Error).include? e.class.name
+      cache_value(:geo_node_enabled) { GeoNode.exists? }
+    end
 
-      false
+    def self.primary?
+      self.enabled? && self.current_node&.primary?
+    end
+
+    def self.secondary?
+      self.enabled? && self.current_node&.secondary?
     end
 
     def self.current_node_enabled?
@@ -64,14 +71,6 @@ module Gitlab
       ::License.feature_available?(:geo)
     end
 
-    def self.primary?
-      self.cache_value(:geo_node_primary) { self.enabled? && self.current_node && self.current_node.primary? }
-    end
-
-    def self.secondary?
-      self.cache_value(:geo_node_secondary) { self.enabled? && self.current_node && self.current_node.secondary? }
-    end
-
     def self.geo_node?(host:, port:)
       GeoNode.where(host: host, port: port).exists?
     end

spec/lib/gitlab/geo_spec.rb

@@ -101,31 +101,35 @@ describe Gitlab::Geo, :geo do
       end
     end
 
+    context 'with RequestStore enabled', :request_store do
+      it 'return false when no GeoNode exists' do
+        GeoNode.delete_all
+
+        expect(GeoNode).to receive(:exists?).once.and_call_original
+
+        2.times { expect(described_class.enabled?).to be_falsey }
+      end
+    end
+  end
+
+  describe 'connected?' do
     context 'when there is a database issue' do
       it 'returns false when database connection is down' do
         allow(GeoNode).to receive(:connected?) { false }
 
-        expect(described_class.enabled?).to be_falsey
+        expect(described_class.connected?).to be_falsey
       end
 
-      it 'returns false when database schema does not contain required tables' do
-        if Gitlab::Database.mysql?
-          allow(GeoNode).to receive(:exists?).and_raise(Mysql2::Error, "Table 'gitlabhq_test.geo_nodes' doesn't exist: SHOW FULL FIELDS FROM `geo_nodes`")
-        else
-          allow(GeoNode).to receive(:exists?).and_raise(PG::UndefinedTable)
-        end
+      it 'returns false when the table does not exist' do
+        allow(GeoNode).to receive(:table_exists?) { false }
 
-        expect(described_class.enabled?).to be_falsey
+        expect(described_class.connected?).to be_falsey
       end
-    end
-
-    context 'with RequestStore enabled', :request_store do
-      it 'return false when no GeoNode exists' do
-        GeoNode.delete_all
 
-        expect(GeoNode).to receive(:exists?).once.and_call_original
+      it 'returns false when MySQL is in use' do
+        allow(Gitlab::Database).to receive(:postgresql?) { false }
 
-        2.times { expect(described_class.enabled?).to be_falsey }
+        expect(described_class.connected?).to be_falsey
       end
     end
   end
@@ -139,7 +143,7 @@ describe Gitlab::Geo, :geo do
     end
 
     context 'current node is primary' do
-      it 'returns false ' do
+      it 'returns false' do
         expect(described_class.secondary?).to be_falsey
       end
     end