Merge branch '6097-turn-gitlab-geo-fdw-into-a-class' into 'master'

Geo - Turn Gitlab::Geo::Fdw into a class

Closes #6097

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

ee/app/models/geo/fdw/ci/job_artifact.rb

@@ -8,7 +8,7 @@ module Geo
 
         STORE_COLUMN = :file_store
 
-        self.table_name = Gitlab::Geo::Fdw.table('ci_job_artifacts')
+        self.table_name = Gitlab::Geo::Fdw.foreign_table_name('ci_job_artifacts')
 
         scope :not_expired, -> { where('expire_at IS NULL OR expire_at > ?', Time.current) }
         scope :geo_syncable, -> { with_files_stored_locally.not_expired }

ee/app/models/geo/fdw/lfs_object.rb

@@ -7,7 +7,7 @@ module Geo
 
       STORE_COLUMN = :file_store
 
-      self.table_name = Gitlab::Geo::Fdw.table('lfs_objects')
+      self.table_name = Gitlab::Geo::Fdw.foreign_table_name('lfs_objects')
 
       scope :geo_syncable, -> { with_files_stored_locally }
     end

ee/app/models/geo/fdw/project.rb

@@ -5,7 +5,7 @@ module Geo
     class Project < ::Geo::BaseFdw
       include Gitlab::SQL::Pattern
 
-      self.table_name = Gitlab::Geo::Fdw.table('projects')
+      self.table_name = Gitlab::Geo::Fdw.foreign_table_name('projects')
 
       class << self
         # Searches for a list of projects based on the query given in `query`.

ee/app/models/geo/fdw/project_feature.rb

@@ -3,7 +3,7 @@
 module Geo
   module Fdw
     class ProjectFeature < ::Geo::BaseFdw
-      self.table_name = Gitlab::Geo::Fdw.table('project_features')
+      self.table_name = Gitlab::Geo::Fdw.foreign_table_name('project_features')
     end
   end
 end

ee/app/models/geo/fdw/project_repository_state.rb

@@ -3,7 +3,7 @@
 module Geo
   module Fdw
     class ProjectRepositoryState < ::Geo::BaseFdw
-      self.table_name = Gitlab::Geo::Fdw.table('project_repository_states')
+      self.table_name = Gitlab::Geo::Fdw.foreign_table_name('project_repository_states')
     end
   end
 end

ee/app/models/geo/fdw/upload.rb

@@ -7,7 +7,7 @@ module Geo
 
       STORE_COLUMN = :store
 
-      self.table_name = Gitlab::Geo::Fdw.table('uploads')
+      self.table_name = Gitlab::Geo::Fdw.foreign_table_name('uploads')
 
       scope :geo_syncable, -> { with_files_stored_locally }
     end

ee/lib/gitlab/geo/fdw.rb

@@ -2,21 +2,16 @@
 
 module Gitlab
   module Geo
-    module Fdw
-      DEFAULT_SCHEMA = 'public'.freeze
-      FDW_SCHEMA = 'gitlab_secondary'.freeze
-
-      # Return full table name with FDW schema
-      #
-      # @param [String] table_name
-      def self.table(table_name)
-        FDW_SCHEMA + ".#{table_name}"
-      end
+    class Fdw
+      DEFAULT_SCHEMA = 'public'
+      FOREIGN_SERVER = 'gitlab_secondary'
+      FOREIGN_SCHEMA = 'gitlab_secondary'
 
+      class << self
         # Return if FDW is enabled for this instance
         #
         # @return [Boolean] whether FDW is enabled
-      def self.enabled?
+        def enabled?
           return false unless fdw_capable?
 
           # FDW is enabled by default, disable it by setting `fdw: false` in config/database_geo.yml
@@ -24,80 +19,86 @@ module Gitlab
           value.nil? ? true : value
         end
 
-      def self.fdw_capable?
-        has_foreign_schema? && connection_exist? && count_tables.positive?
+        # Return full table name with foreign schema
+        #
+        # @param [String] table_name
+        def foreign_table_name(table_name)
+          FOREIGN_SCHEMA + ".#{table_name}"
         end
 
-      def self.fdw_up_to_date?
+        def foreign_tables_up_to_date?
           has_foreign_schema? && foreign_schema_tables_match?
         end
 
-      def self.has_foreign_schema?
-        Gitlab::Geo.cache_value(:geo_fdw_schema_exist) do
+        # Number of existing tables
+        #
+        # @return [Integer] number of tables
+        def foreign_schema_tables_count
+          Gitlab::Geo.cache_value(:geo_fdw_count_tables) do
             sql = <<~SQL
-            SELECT 1
-              FROM information_schema.schemata
-             WHERE schema_name='#{FDW_SCHEMA}'
+              SELECT COUNT(*)
+                FROM information_schema.tables
+               WHERE table_schema = '#{FOREIGN_SCHEMA}'
+                 AND table_type = 'FOREIGN TABLE'
+                 AND table_name NOT LIKE 'pg_%'
             SQL
 
-          ::Geo::TrackingBase.connection.execute(sql).count.positive?
+            ::Geo::TrackingBase.connection.execute(sql).first.fetch('count').to_i
+          end
+        end
+
+        def gitlab_schema_tables_count
+          ActiveRecord::Schema.tables.reject { |table| table.start_with?('pg_') }.count
         end
+
+        private
+
+        def fdw_capable?
+          has_foreign_server? && has_foreign_schema? && foreign_schema_tables_count.positive?
         end
 
-      # Check if there is at least one FDW connection configured
+        # Check if there is at least one foreign server configured
         #
-      # @return [Boolean] whether any FDW connection exists
-      def self.connection_exist?
+        # @return [Boolean] whether any foreign server exists
+        def has_foreign_server?
           ::Geo::TrackingBase.connection.execute(
             "SELECT 1 FROM pg_foreign_server"
           ).count.positive?
         end
 
-      # Number of existing tables
-      #
-      # @return [Integer] number of tables
-      def self.count_tables
-        Gitlab::Geo.cache_value(:geo_fdw_count_tables) do
+        def has_foreign_schema?
+          Gitlab::Geo.cache_value(:geo_FOREIGN_SCHEMA_exist) do
             sql = <<~SQL
-            SELECT COUNT(*)
-              FROM information_schema.tables
-             WHERE table_schema = '#{FDW_SCHEMA}'
-               AND table_type = 'FOREIGN TABLE'
-               AND table_name NOT LIKE 'pg_%'
+              SELECT 1
+                FROM information_schema.schemata
+               WHERE schema_name='#{FOREIGN_SCHEMA}'
             SQL
 
-          ::Geo::TrackingBase.connection.execute(sql).first.fetch('count').to_i
+            ::Geo::TrackingBase.connection.execute(sql).count.positive?
           end
         end
 
         # Check if foreign schema has exact the same tables and fields defined on secondary database
         #
         # @return [Boolean] whether schemas match and are not empty
-      def self.foreign_schema_tables_match?
-        Gitlab::Geo.cache_value(:geo_fdw_schema_tables_match) do
-          schema = gitlab_schema
+        def foreign_schema_tables_match?
+          Gitlab::Geo.cache_value(:geo_foreign_schema_tables_match) do
+            gitlab_schema_tables = retrieve_gitlab_schema_tables.to_set
+            foreign_schema_tables = retrieve_foreign_schema_tables.to_set
 
-          schema.present? && (schema.to_set == fdw_schema.to_set)
+            gitlab_schema_tables.present? && (gitlab_schema_tables == foreign_schema_tables)
           end
         end
 
-      def self.count_tables_match?
-        gitlab_tables.count == count_tables
+        def retrieve_foreign_schema_tables
+          retrieve_schema_tables(::Geo::TrackingBase, Rails.configuration.geo_database['database'], FOREIGN_SCHEMA).to_a
         end
 
-      def self.gitlab_tables
-        ActiveRecord::Schema.tables.reject { |table| table.start_with?('pg_') }
-      end
-
-      def self.gitlab_schema
+        def retrieve_gitlab_schema_tables
           retrieve_schema_tables(ActiveRecord::Base, ActiveRecord::Base.connection_config[:database], DEFAULT_SCHEMA).to_a
         end
 
-      def self.fdw_schema
-        retrieve_schema_tables(::Geo::TrackingBase, Rails.configuration.geo_database['database'], FDW_SCHEMA).to_a
-      end
-
-      def self.retrieve_schema_tables(adapter, database, schema)
+        def retrieve_schema_tables(adapter, database, schema)
           sql = <<~SQL
               SELECT table_name, column_name, data_type
                 FROM information_schema.columns
@@ -109,7 +110,7 @@ module Gitlab
 
           adapter.connection.select_all(sql)
         end
-      private_class_method :retrieve_schema_tables
+      end
     end
   end
 end

ee/lib/gitlab/geo/geo_tasks.rb

@@ -55,7 +55,7 @@ module Gitlab
         sql = <<~SQL
           SELECT count(1)
             FROM pg_foreign_server
-           WHERE srvname = '#{Gitlab::Geo::Fdw::FDW_SCHEMA}';
+           WHERE srvname = '#{Gitlab::Geo::Fdw::FOREIGN_SERVER}';
         SQL
 
         Gitlab::Geo::DatabaseTasks.with_geo_db do

ee/lib/gitlab/geo/health_check.rb

@@ -21,11 +21,14 @@ module Gitlab
 
         return 'The Geo database is not configured to use Foreign Data Wrapper.' unless Gitlab::Geo::Fdw.enabled?
 
-        unless Gitlab::Geo::Fdw.fdw_up_to_date?
+        unless Gitlab::Geo::Fdw.foreign_tables_up_to_date?
           output = "The Geo database has an outdated FDW remote schema."
 
-          unless Gitlab::Geo::Fdw.count_tables_match?
-            output = "#{output} It contains #{Gitlab::Geo::Fdw.count_tables} of #{Gitlab::Geo::Fdw.gitlab_tables.count} expected tables."
+          foreign_schema_tables_count = Gitlab::Geo::Fdw.foreign_schema_tables_count
+          gitlab_schema_tables_count = Gitlab::Geo::Fdw.gitlab_schema_tables_count
+
+          unless gitlab_schema_tables_count == foreign_schema_tables_count
+            output = "#{output} It contains #{foreign_schema_tables_count} of #{gitlab_schema_tables_count} expected tables."
           end
 
           return output

ee/lib/system_check/geo/fdw_schema_up_to_date_check.rb

@@ -25,7 +25,7 @@ module SystemCheck
       end
 
       def check?
-        Gitlab::Geo::Fdw.fdw_up_to_date?
+        Gitlab::Geo::Fdw.foreign_tables_up_to_date?
       end
 
       def show_error

ee/lib/tasks/geo.rake

@@ -58,7 +58,7 @@ namespace :geo do
     desc 'Refresh Foreign Tables definition in Geo Secondary node'
     task refresh_foreign_tables: [:environment] do
       if Gitlab::Geo::GeoTasks.foreign_server_configured?
-        print "\nRefreshing foreign tables for FDW: #{Gitlab::Geo::Fdw::FDW_SCHEMA} ... "
+        print "\nRefreshing foreign tables for FDW: #{Gitlab::Geo::Fdw::FOREIGN_SCHEMA} ... "
         Gitlab::Geo::GeoTasks.refresh_foreign_tables!
         puts 'Done!'
       else

ee/spec/lib/gitlab/geo/fdw_spec.rb

 require 'spec_helper'
 
 describe Gitlab::Geo::Fdw, :geo do
-  include ::EE::GeoHelpers
+  describe '.enabled?' do
+    it 'returns false when foreign server does not exist' do
+      drop_foreign_server
 
-  describe 'enabled?' do
-    it 'returns false when PostgreSQL FDW is not enabled' do
-      expect(described_class).to receive(:count_tables).and_return(0)
-      allow(Rails.configuration).to receive(:geo_database).and_return('fdw' => true)
-
-      expect(described_class.enabled?).to be_falsey
+      expect(described_class.enabled?).to eq false
     end
 
-    context 'with fdw capable' do
-      before do
-        allow(described_class).to receive(:fdw_capable?).and_return(true)
+    it 'returns false when foreign server exists but foreign schema does not exist' do
+      drop_foreign_schema
+
+      expect(described_class.enabled?).to eq false
     end
 
-      it 'returns true by default' do
-        allow(Rails.configuration).to receive(:geo_database).and_return('fdw' => nil)
+    it 'returns false when foreign server and schema exists but foreign tables are empty' do
+      drop_foreign_schema
+      create_foreign_schema
 
-        expect(described_class.enabled?).to be_truthy
+      expect(described_class.enabled?).to eq false
     end
 
-      it 'returns false if configured in `config/database_geo.yml`' do
+    it 'returns false when fdw is disabled in `config/database_geo.yml`' do
       allow(Rails.configuration).to receive(:geo_database).and_return('fdw' => false)
 
       expect(described_class.enabled?).to be_falsey
     end
 
-      it 'returns true if configured in `config/database_geo.yml`' do
+    it 'returns true when fdw is set in `config/database_geo.yml`' do
       allow(Rails.configuration).to receive(:geo_database).and_return('fdw' => true)
 
       expect(described_class.enabled?).to be_truthy
     end
-    end
-  end
-
-  describe '.gitlab_tables' do
-    it 'excludes pg_ tables' do
-      tables = described_class.gitlab_tables
-
-      ActiveRecord::Base.connection.create_table(:pg_gitlab_test)
 
-      expect(described_class.gitlab_tables).to eq(tables)
+    it 'returns true when fdw is nil in `config/database_geo.yml`' do
+      allow(Rails.configuration).to receive(:geo_database).and_return('fdw' => nil)
 
-      ActiveRecord::Base.connection.drop_table(:pg_gitlab_test)
-    end
+      expect(described_class.enabled?).to be_truthy
     end
 
-  describe 'fdw_capable?' do
-    context 'with mocked FDW environment' do
-      it 'returns true when PostgreSQL FDW is enabled' do
-        expect(described_class).to receive(:has_foreign_schema?).and_return(true)
-        expect(described_class).to receive(:count_tables).and_return(1)
-
-        expect(described_class.fdw_capable?).to be_truthy
+    it 'returns true with a functional fdw environment' do
+      expect(described_class.enabled?).to be_truthy
     end
-
-      it 'returns false when PostgreSQL FDW is not enabled' do
-        expect(described_class).to receive(:has_foreign_schema?).and_return(false)
-
-        expect(described_class.fdw_capable?).to be_falsey
   end
 
-      it 'returns false when PostgreSQL FDW is enabled but remote tables are empty' do
-        expect(described_class).to receive(:has_foreign_schema?).and_return(true)
-        expect(described_class).to receive(:count_tables).and_return(0)
+  describe '.foreign_tables_up_to_date?' do
+    it 'returns false when foreign schema does not exist' do
+      drop_foreign_schema
 
-        expect(described_class.fdw_capable?).to be_falsey
+      expect(described_class.foreign_tables_up_to_date?).to eq false
     end
 
-      it 'returns false when PostgreSQL FDW is enabled but no remote connection is defined' do
-        expect(described_class).to receive(:has_foreign_schema?).and_return(true)
-        expect(described_class).to receive(:connection_exist?).and_return(false)
+    it 'returns false when foreign schema exists but tables in schema doesnt match' do
+      create_foreign_table(:gitlab_test)
 
-        expect(described_class.fdw_capable?).to be_falsey
-      end
+      expect(described_class.foreign_tables_up_to_date?).to eq false
     end
 
-    context 'with functional FDW environment' do
-      it 'returns true' do
-        expect(described_class.fdw_capable?).to be_truthy
+    it 'returns true when foreign schema exists and foreign schema has same tables as secondary database' do
+      expect(described_class.foreign_tables_up_to_date?).to eq true
+    end
   end
 
-      context 'with a pg_ table' do
+  describe '.foreign_schema_tables_count' do
     before do
-          ActiveRecord::Base.connection.create_table(:pg_gitlab_test)
+      drop_foreign_schema
+      create_foreign_schema
     end
 
-        after do
-          ActiveRecord::Base.connection.drop_table(:pg_gitlab_test)
-        end
+    it 'returns the number of tables in the foreign schema' do
+      create_foreign_table(:gitlab_test)
 
-        it 'returns true' do
-          expect(described_class.fdw_capable?).to be_truthy
-        end
-      end
-    end
+      expect(described_class.foreign_schema_tables_count).to eq(1)
     end
 
-  describe 'fdw_up_to_date?' do
-    context 'with mocked FDW environment' do
-      it 'returns true when FDW is enabled and foreign schema has same tables as secondary database' do
-        expect(described_class).to receive(:has_foreign_schema?).and_return(true)
-        expect(described_class).to receive(:foreign_schema_tables_match?).and_return(true)
+    it 'excludes tables that start with `pg_`' do
+      create_foreign_table(:pg_gitlab_test)
 
-        expect(described_class.fdw_up_to_date?).to be_truthy
+      expect(described_class.foreign_schema_tables_count).to eq(0)
     end
-
-      it 'returns false when FDW is enabled but tables in schema doesnt match' do
-        expect(described_class).to receive(:has_foreign_schema?).and_return(true)
-        expect(described_class).to receive(:foreign_schema_tables_match?).and_return(false)
-
-        expect(described_class.fdw_up_to_date?).to be_falsey
   end
 
-      it 'returns false when FDW is disabled' do
-        expect(described_class).to receive(:has_foreign_schema?).and_return(false)
-
-        expect(described_class.fdw_up_to_date?).to be_falsey
-      end
+  describe '.gitlab_schema_tables_count' do
+    it 'returns the same number of tables as defined in the database' do
+      expect(described_class.gitlab_schema_tables_count).to eq(ActiveRecord::Schema.tables.count)
     end
 
-    context 'with functional FDW environment' do
-      it 'returns true' do
-        expect(described_class.fdw_up_to_date?).to be_truthy
-      end
-    end
-  end
+    it 'excludes tables that start with `pg_`' do
+      ActiveRecord::Base.connection.create_table(:pg_gitlab_test)
 
-  describe 'has_foreign_schema?' do
-    context 'with functional FDW environment' do
-      it 'returns true' do
-        # When testing it locally, make sure you have FDW set up correctly.
-        # If you are using GDK, you can run, from GDK root folder:
-        #
-        # make postgresql/geo-fdw/test
-        expect(described_class.has_foreign_schema?).to be_truthy
-      end
-    end
-  end
+      expect(described_class.gitlab_schema_tables_count).to eq(ActiveRecord::Schema.tables.count - 1)
 
-  describe 'count_tables' do
-    context 'with functional FDW environment' do
-      it 'returns same amount as defined in schema migration' do
-        # When testing it locally, you may need to refresh FDW with:
-        #
-        # rake geo:db:test:refresh_foreign_tables
-        expect(described_class.count_tables).to eq(ActiveRecord::Schema.tables.count)
-      end
+      ActiveRecord::Base.connection.drop_table(:pg_gitlab_test)
     end
   end
 
-  describe 'connection_exist?' do
-    context 'with functional FDW environment' do
-      it 'returns true' do
-        # When testing it locally, make sure you have FDW set up correctly.
-        # If you are using GDK, you can run, from GDK root folder:
-        #
-        # make postgresql/geo-fdw/test
-        expect(described_class.connection_exist?).to be_truthy
-      end
-    end
+  def with_foreign_connection
+    Geo::TrackingBase.connection
   end
 
-  describe 'foreign_schema_tables_match?' do
-    context 'with functional FDW environment' do
-      it 'returns true' do
-        # When testing it locally, you may need to refresh FDW with:
-        #
-        # rake geo:db:test:refresh_foreign_tables
-        expect(described_class.foreign_schema_tables_match?).to be_truthy
+  def drop_foreign_server
+    with_foreign_connection.execute <<-SQL
+      DROP SERVER IF EXISTS #{described_class::FOREIGN_SERVER} CASCADE
+    SQL
   end
 
-      it 'returns true if order is different' do
-        one_schema = [
-          { "table_name" => "events", "column_name" => "target_type", "data_type" => "character varying" },
-          { "table_name" => "ci_job_artifacts", "column_name" => "id", "data_type" => "integer" }
-        ]
-        second_schema = one_schema.reverse
+  def drop_foreign_schema
+    with_foreign_connection.execute <<-SQL
+      DROP SCHEMA IF EXISTS #{described_class::FOREIGN_SCHEMA} CASCADE
+    SQL
+  end
 
-        allow(described_class).to receive(:gitlab_schema).and_return(one_schema)
-        allow(described_class).to receive(:fdw_schema).and_return(second_schema)
+  def create_foreign_schema
+    with_foreign_connection.execute <<-SQL
+      CREATE SCHEMA IF NOT EXISTS #{described_class::FOREIGN_SCHEMA}
+    SQL
 
-        expect(described_class.foreign_schema_tables_match?).to be_truthy
-      end
+    with_foreign_connection.execute <<-SQL
+      GRANT USAGE ON FOREIGN SERVER #{described_class::FOREIGN_SERVER} TO current_user
+    SQL
   end
+
+  def create_foreign_table(table_name)
+    with_foreign_connection.execute <<-SQL
+      CREATE FOREIGN TABLE IF NOT EXISTS #{described_class::FOREIGN_SCHEMA}.#{table_name} (
+        id int
+      ) SERVER #{described_class::FOREIGN_SERVER}
+    SQL
   end
 end

ee/spec/lib/gitlab/geo/health_check_spec.rb

@@ -96,8 +96,9 @@ describe Gitlab::Geo::HealthCheck, :geo do
       allow(described_class).to receive(:database_secondary?) { true }
       allow(described_class).to receive(:streaming_active?) { true }
 
-      allow(Gitlab::Geo::Fdw).to receive(:fdw_up_to_date?) { false }
-      allow(Gitlab::Geo::Fdw).to receive(:count_tables_match?) { true }
+      allow(Gitlab::Geo::Fdw).to receive(:foreign_tables_up_to_date?) { false }
+      allow(Gitlab::Geo::Fdw).to receive(:foreign_schema_tables_count) { 1 }
+      allow(Gitlab::Geo::Fdw).to receive(:gitlab_schema_tables_count) { 1 }
 
       expect(subject.perform_checks).to match(/The Geo database has an outdated FDW remote schema\./)
     end
@@ -106,8 +107,9 @@ describe Gitlab::Geo::HealthCheck, :geo do
       allow(described_class).to receive(:database_secondary?) { true }
       allow(described_class).to receive(:streaming_active?) { true }
 
-      allow(Gitlab::Geo::Fdw).to receive(:fdw_up_to_date?) { false }
-      allow(Gitlab::Geo::Fdw).to receive(:count_tables_match?) { false }
+      allow(Gitlab::Geo::Fdw).to receive(:foreign_tables_up_to_date?) { false }
+      allow(Gitlab::Geo::Fdw).to receive(:foreign_schema_tables_count) { 1 }
+      allow(Gitlab::Geo::Fdw).to receive(:gitlab_schema_tables_count) { 2 }
 
       expect(subject.perform_checks).to match(/The Geo database has an outdated FDW remote schema\. It contains [0-9]+ of [0-9]+ expected tables/)
     end