Merge branch...

Merge branch '5699-enabling-disabling-features-does-not-propagate-correctly-to-geo-secondaries' into 'master'

Geo - Send a cache invalidation event via the log cursor whenever features are changed on the primary

Closes #5699

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

db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20180930171532) do
+ActiveRecord::Schema.define(version: 20181001172651) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -1076,6 +1076,10 @@ ActiveRecord::Schema.define(version: 20180930171532) do
 
   add_index "gcp_clusters", ["project_id"], name: "index_gcp_clusters_on_project_id", unique: true, using: :btree
 
+  create_table "geo_cache_invalidation_events", id: :bigserial, force: :cascade do |t|
+    t.string "key", null: false
+  end
+
   create_table "geo_event_log", id: :bigserial, force: :cascade do |t|
     t.datetime "created_at", null: false
     t.integer "repository_updated_event_id", limit: 8
@@ -1089,8 +1093,10 @@ ActiveRecord::Schema.define(version: 20180930171532) do
     t.integer "job_artifact_deleted_event_id", limit: 8
     t.integer "upload_deleted_event_id", limit: 8
     t.integer "reset_checksum_event_id", limit: 8
+    t.integer "cache_invalidation_event_id", limit: 8
   end
 
+  add_index "geo_event_log", ["cache_invalidation_event_id"], name: "index_geo_event_log_on_cache_invalidation_event_id", using: :btree
   add_index "geo_event_log", ["repositories_changed_event_id"], name: "index_geo_event_log_on_repositories_changed_event_id", using: :btree
   add_index "geo_event_log", ["repository_created_event_id"], name: "index_geo_event_log_on_repository_created_event_id", using: :btree
   add_index "geo_event_log", ["repository_deleted_event_id"], name: "index_geo_event_log_on_repository_deleted_event_id", using: :btree
@@ -3171,6 +3177,7 @@ ActiveRecord::Schema.define(version: 20180930171532) do
   add_foreign_key "gcp_clusters", "projects", on_delete: :cascade
   add_foreign_key "gcp_clusters", "services", on_delete: :nullify
   add_foreign_key "gcp_clusters", "users", on_delete: :nullify
+  add_foreign_key "geo_event_log", "geo_cache_invalidation_events", column: "cache_invalidation_event_id", name: "fk_42c3b54bed", on_delete: :cascade
   add_foreign_key "geo_event_log", "geo_hashed_storage_migrated_events", column: "hashed_storage_migrated_event_id", name: "fk_27548c6db3", on_delete: :cascade
   add_foreign_key "geo_event_log", "geo_job_artifact_deleted_events", column: "job_artifact_deleted_event_id", name: "fk_176d3fbb5d", on_delete: :cascade
   add_foreign_key "geo_event_log", "geo_lfs_object_deleted_events", column: "lfs_object_deleted_event_id", name: "fk_d5af95fcd9", on_delete: :cascade

doc/administration/geo/disaster_recovery/background_verification.md

@@ -41,7 +41,7 @@ sudo -u git -H bin/rails console RAILS_ENV=production
 **To check if automatic background verification is enabled:**
 
 ```ruby
-Feature.enabled?('geo_repository_verification')
+Gitlab::Geo.repository_verification_enabled?
 ```
 
 **To disable automatic background verification:**
@@ -56,11 +56,6 @@ Feature.disable('geo_repository_verification')
 Feature.enable('geo_repository_verification')
 ```
 
-NOTE: **Note:**
-Until [issue #5699][ee-5699] is completed, we need to reset the cache for this
-feature flag on each **secondary**, to do this run
-`sudo gitlab-rails runner 'Rails.cache.expire('flipper/v1/feature/geo_repository_verification', 0)'`.
-
 # Repository verification
 
 Navigate to the **Admin Area > Geo** dashboard on the **primary** node and expand
@@ -131,11 +126,7 @@ on both nodes, and comparing the output between them.
 Data in object storage is **not verified**, as the object store is responsible
 for ensuring the integrity of the data.
 
-[disaster-recovery]: index.md
 [feature-flag]: background_verification.md#enabling-or-disabling-the-automatic-background-verification
 [reset-verification]: background_verification.md#reset-verification-for-projects-where-verification-has-failed
 [foreground-verification]: ../../raketasks/check.md
 [ee-5064]: https://gitlab.com/gitlab-org/gitlab-ee/issues/5064
-[ee-5699]: https://gitlab.com/gitlab-org/gitlab-ee/issues/5699
-[ee-5195]: https://gitlab.com/gitlab-org/gitlab-ee/issues/5195
-[ee-5196]: https://gitlab.com/gitlab-org/gitlab-ee/issues/5196

ee/app/controllers/groups/pipeline_quota_controller.rb

@@ -11,7 +11,7 @@ class Groups::PipelineQuotaController < Groups::ApplicationController
   private
 
   def all_projects
-    if Feature.enabled?(:shared_runner_minutes_on_root_namespace)
+    if ::Feature.enabled?(:shared_runner_minutes_on_root_namespace)
       @group.all_projects
     else
       @group.projects

ee/app/models/ee/namespace.rb

@@ -119,7 +119,7 @@ module EE
 
     def shared_runner_minutes_supported?
       if has_parent?
-        !Feature.enabled?(:shared_runner_minutes_on_root_namespace)
+        !::Feature.enabled?(:shared_runner_minutes_on_root_namespace)
       else
         true
       end
@@ -142,7 +142,7 @@ module EE
     end
 
     def shared_runners_enabled?
-      if Feature.enabled?(:shared_runner_minutes_on_root_namespace)
+      if ::Feature.enabled?(:shared_runner_minutes_on_root_namespace)
         all_projects.with_shared_runners.any?
       else
         projects.with_shared_runners.any?

ee/app/models/ee/project.rb

@@ -128,7 +128,7 @@ module EE
     end
 
     def shared_runners_limit_namespace
-      if Feature.enabled?(:shared_runner_minutes_on_root_namespace)
+      if ::Feature.enabled?(:shared_runner_minutes_on_root_namespace)
         root_namespace
       else
         namespace

ee/app/models/geo/cache_invalidation_event.rb

+# frozen_string_literal: true
+
+module Geo
+  class CacheInvalidationEvent < ActiveRecord::Base
+    include Geo::Model
+    include Geo::Eventable
+
+    validates :key, presence: true
+  end
+end

ee/app/models/geo/event_log.rb

@@ -3,7 +3,8 @@ module Geo
     include Geo::Model
     include ::EachBatch
 
-    EVENT_CLASSES = %w[Geo::RepositoryCreatedEvent
+    EVENT_CLASSES = %w[Geo::CacheInvalidationEvent
+                       Geo::RepositoryCreatedEvent
                        Geo::RepositoryUpdatedEvent
                        Geo::RepositoryDeletedEvent
                        Geo::RepositoryRenamedEvent
@@ -15,6 +16,10 @@ module Geo
                        Geo::JobArtifactDeletedEvent
                        Geo::UploadDeletedEvent].freeze
 
+    belongs_to :cache_invalidation_event,
+      class_name: 'Geo::CacheInvalidationEvent',
+      foreign_key: :cache_invalidation_event_id
+
     belongs_to :repository_created_event,
       class_name: 'Geo::RepositoryCreatedEvent',
       foreign_key: :repository_created_event_id
@@ -82,7 +87,8 @@ module Geo
         lfs_object_deleted_event ||
         job_artifact_deleted_event ||
         upload_deleted_event ||
-        reset_checksum_event
+        reset_checksum_event ||
+        cache_invalidation_event
     end
 
     def project_id

ee/app/services/ee/ci/register_job_service.rb

@@ -49,7 +49,7 @@ module EE
       def all_namespaces
         namespaces = ::Namespace.reorder(nil).where('namespaces.id = projects.namespace_id')
 
-        if Feature.enabled?(:shared_runner_minutes_on_root_namespace)
+        if ::Feature.enabled?(:shared_runner_minutes_on_root_namespace)
           namespaces = ::Gitlab::GroupHierarchy.new(namespaces).roots
         end
 

ee/app/services/geo/cache_invalidation_event_store.rb

+# frozen_string_literal: true
+
+module Geo
+  class CacheInvalidationEventStore < EventStore
+    self.event_type = :cache_invalidation_event
+
+    attr_reader :key
+
+    def initialize(key)
+      @key = key
+    end
+
+    private
+
+    def build_event
+      Geo::CacheInvalidationEvent.new(key: key)
+    end
+
+    # This is called by ProjectLogHelpers to build json log with context info
+    #
+    # @see ::Gitlab::Geo::ProjectLogHelpers
+    def base_log_data(message)
+      {
+        class: self.class.name,
+        cache_key: key.to_s,
+        job_id: get_sidekiq_job_id,
+        message: message
+      }.compact
+    end
+  end
+end

ee/changelogs/unreleased/5699-enabling-disabling-features-does-not-propagate-correctly-to-geo-secondaries.yml

+---
+title: Geo - Send a cache invalidation event via the log cursor whenever features
+  are changed on the primary
+merge_request: 7738
+author:
+type: fixed

ee/db/migrate/20181001172126_create_geo_cache_invalidation_events.rb

+# frozen_string_literal: true
+
+class CreateGeoCacheInvalidationEvents < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def change
+    create_table :geo_cache_invalidation_events, id: :bigserial do |t|
+      t.string :key, null: false
+    end
+
+    add_column :geo_event_log, :cache_invalidation_event_id, :integer, limit: 8
+  end
+end

ee/db/migrate/20181001172651_add_geo_cache_invalidation_events_foreign_key.rb

+# frozen_string_literal: true
+
+class AddGeoCacheInvalidationEventsForeignKey < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_foreign_key :geo_event_log, :geo_cache_invalidation_events, column: :cache_invalidation_event_id, on_delete: :cascade
+    add_concurrent_index :geo_event_log, :cache_invalidation_event_id
+  end
+
+  def down
+    remove_foreign_key :geo_event_log, column: :cache_invalidation_event_id
+    remove_concurrent_index :geo_event_log, :cache_invalidation_event_id
+  end
+end

ee/lib/ee/feature.rb

+# frozen_string_literal: true
+
+module EE
+  module Feature
+    module ClassMethods
+      extend ::Gitlab::Utils::Override
+
+      override :enable
+      def enable(key, thing = true)
+        super
+        log_geo_event(key)
+      end
+
+      override :disable
+      def disable(key, thing = false)
+        super
+        log_geo_event(key)
+      end
+
+      override :enable_group
+      def enable_group(key, group)
+        super
+        log_geo_event(key)
+      end
+
+      override :disable_group
+      def disable_group(key, group)
+        super
+        log_geo_event(key)
+      end
+
+      private
+
+      def log_geo_event(key)
+        Geo::CacheInvalidationEventStore.new(cache_store.key_for(key)).create!
+      end
+
+      def cache_store
+        Flipper::Adapters::ActiveSupportCacheStore
+      end
+    end
+
+    def self.prepended(base)
+      base.singleton_class.prepend ClassMethods
+    end
+  end
+end

ee/lib/gitlab/geo.rb

@@ -111,10 +111,10 @@ module Gitlab
     end
 
     def self.repository_verification_enabled?
-      feature = Feature.get('geo_repository_verification')
+      feature = ::Feature.get('geo_repository_verification')
 
       # If the feature has been set, always evaluate
-      if Feature.persisted?(feature)
+      if ::Feature.persisted?(feature)
         return feature.enabled?
       else
         true

ee/lib/gitlab/geo/log_cursor/events/cache_invalidation_event.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Geo
+    module LogCursor
+      module Events
+        class CacheInvalidationEvent
+          include BaseEvent
+
+          def process
+            result = expire_cache_for_event_key
+            log_cache_invalidation_event(result)
+          end
+
+          private
+
+          def expire_cache_for_event_key
+            Rails.cache.delete(event.key)
+          end
+
+          def log_cache_invalidation_event(expired)
+            logger.event_info(
+              created_at,
+              'Cache invalidation',
+              cache_key: event.key,
+              cache_expired: expired,
+              skippable: skippable?
+            )
+          end
+
+          def skippable?
+            false
+          end
+        end
+      end
+    end
+  end
+end

ee/spec/factories/geo/event_log.rb

@@ -39,6 +39,10 @@ FactoryBot.define do
     trait :reset_checksum_event do
       reset_checksum_event factory: :geo_reset_checksum_event
     end
+
+    trait :cache_invalidation_event do
+      cache_invalidation_event factory: :geo_cache_invalidation_event
+    end
   end
 
   factory :geo_repository_created_event, class: Geo::RepositoryCreatedEvent do
@@ -146,4 +150,8 @@ FactoryBot.define do
   factory :geo_reset_checksum_event, class: Geo::ResetChecksumEvent do
     project
   end
+
+  factory :geo_cache_invalidation_event, class: Geo::CacheInvalidationEvent do
+    sequence(:key) { |n| "cache-key-#{n}" }
+  end
 end

ee/spec/lib/ee/feature_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Feature do
+  include EE::GeoHelpers
+
+  describe '.enable' do
+    context 'when running on a Geo primary node' do
+      before do
+        stub_primary_node
+      end
+
+      it 'does not create a Geo::CacheInvalidationEvent if there are no Geo secondary nodes' do
+        allow(Gitlab::Geo).to receive(:secondary_nodes) { [] }
+
+        expect { described_class.enable(:foo) }.not_to change(Geo::CacheInvalidationEvent, :count)
+      end
+
+      it 'creates a Geo::CacheInvalidationEvent' do
+        allow(Gitlab::Geo).to receive(:secondary_nodes) { [double] }
+
+        expect { described_class.enable(:foo) }.to change(Geo::CacheInvalidationEvent, :count).by(1)
+      end
+    end
+  end
+
+  describe '.disable' do
+    context 'when running on a Geo primary node' do
+      before do
+        stub_primary_node
+      end
+
+      it 'does not create a Geo::CacheInvalidationEvent if there are no Geo secondary nodes' do
+        allow(Gitlab::Geo).to receive(:secondary_nodes) { [] }
+
+        expect { described_class.disable(:foo) }.not_to change(Geo::CacheInvalidationEvent, :count)
+      end
+
+      it 'creates a Geo::CacheInvalidationEvent' do
+        allow(Gitlab::Geo).to receive(:secondary_nodes) { [double] }
+
+        expect { described_class.disable(:foo) }.to change(Geo::CacheInvalidationEvent, :count).by(1)
+      end
+    end
+  end
+
+  describe '.enable_group' do
+    context 'when running on a Geo primary node' do
+      before do
+        stub_primary_node
+      end
+
+      it 'does not create a Geo::CacheInvalidationEvent if there are no Geo secondary nodes' do
+        allow(Gitlab::Geo).to receive(:secondary_nodes) { [] }
+
+        expect { described_class.enable_group(:foo, :bar) }.not_to change(Geo::CacheInvalidationEvent, :count)
+      end
+
+      it 'creates a Geo::CacheInvalidationEvent' do
+        allow(Gitlab::Geo).to receive(:secondary_nodes) { [double] }
+
+        expect { described_class.enable_group(:foo, :bar) }.to change(Geo::CacheInvalidationEvent, :count).by(1)
+      end
+    end
+  end
+
+  describe '.disable_group' do
+    context 'when running on a Geo primary node' do
+      before do
+        stub_primary_node
+      end
+
+      it 'does not create a Geo::CacheInvalidationEvent if there are no Geo secondary nodes' do
+        allow(Gitlab::Geo).to receive(:secondary_nodes) { [] }
+
+        expect { described_class.disable_group(:foo, :bar) }.not_to change(Geo::CacheInvalidationEvent, :count)
+      end
+
+      it 'creates a Geo::CacheInvalidationEvent' do
+        allow(Gitlab::Geo).to receive(:secondary_nodes) { [double] }
+
+        expect { described_class.disable_group(:foo, :bar) }.to change(Geo::CacheInvalidationEvent, :count).by(1)
+      end
+    end
+  end
+end

ee/spec/lib/gitlab/geo/log_cursor/events/cache_invalidation_event_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::Geo::LogCursor::Events::CacheInvalidationEvent, :postgresql, :clean_gitlab_redis_shared_state do
+  let(:logger) { Gitlab::Geo::LogCursor::Logger.new(described_class, Logger::INFO) }
+  let(:event_log) { create(:geo_event_log, :cache_invalidation_event) }
+  let!(:event_log_state) { create(:geo_event_log_state, event_id: event_log.id - 1) }
+  let(:cache_invalidation_event) { event_log.cache_invalidation_event }
+  let(:cache_key) { cache_invalidation_event.key }
+
+  subject { described_class.new(cache_invalidation_event, Time.now, logger) }
+
+  around do |example|
+    Sidekiq::Testing.fake! { example.run }
+  end
+
+  describe '#process' do
+    it 'expires the cache of the given key' do
+      expect(Rails.cache).to receive(:delete).with(cache_key).and_call_original
+
+      subject.process
+    end
+
+    it 'logs an info event' do
+      data = {
+        class: described_class.name,
+        message: 'Cache invalidation',
+        cache_key: cache_key,
+        cache_expired: false,
+        skippable: false
+      }
+
+      expect(::Gitlab::Logger)
+        .to receive(:info)
+        .with(hash_including(data))
+
+      subject.process
+    end
+  end
+end

ee/spec/models/geo/cache_invalidation_event_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Geo::CacheInvalidationEvent, type: :model do
+  describe 'validations' do
+    it { is_expected.to validate_presence_of(:key) }
+  end
+end

ee/spec/models/geo/event_log_spec.rb

@@ -2,6 +2,7 @@ require 'spec_helper'
 
 RSpec.describe Geo::EventLog, type: :model do
   describe 'relationships' do
+    it { is_expected.to belong_to(:cache_invalidation_event).class_name('Geo::CacheInvalidationEvent').with_foreign_key('cache_invalidation_event_id') }
     it { is_expected.to belong_to(:repositories_changed_event).class_name('Geo::RepositoriesChangedEvent').with_foreign_key('repositories_changed_event_id') }
     it { is_expected.to belong_to(:repository_created_event).class_name('Geo::RepositoryCreatedEvent').with_foreign_key('repository_created_event_id') }
     it { is_expected.to belong_to(:repository_deleted_event).class_name('Geo::RepositoryDeletedEvent').with_foreign_key('repository_deleted_event_id') }
@@ -103,6 +104,13 @@ RSpec.describe Geo::EventLog, type: :model do
 
       expect(subject.event).to eq reset_checksum_event
     end
+
+    it 'returns cache_invalidation_event when set' do
+      cache_invalidation_event = build(:geo_cache_invalidation_event)
+      subject.cache_invalidation_event = cache_invalidation_event
+
+      expect(subject.event).to eq cache_invalidation_event
+    end
   end
 
   describe '#project_id' do

ee/spec/services/geo/cache_invalidation_event_store_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Geo::CacheInvalidationEventStore do
+  include EE::GeoHelpers
+
+  set(:secondary_node) { create(:geo_node) }
+
+  let(:cache_key) { 'cache-key' }
+
+  subject { described_class.new(cache_key) }
+
+  describe '#create' do
+    it_behaves_like 'a Geo event store', Geo::CacheInvalidationEvent
+
+    context 'when running on a primary node' do
+      before do
+        stub_primary_node
+      end
+
+      it 'tracks the cache key that should be invalidated' do
+        subject.create!
+
+        expect(Geo::CacheInvalidationEvent.last).to have_attributes(key: cache_key)
+      end
+
+      it 'logs an error message when event creation fail' do
+        subject = described_class.new(nil)
+
+        expected_message = {
+          class: described_class.name,
+          cache_key: '',
+          message: 'Cache invalidation event could not be created',
+          error: "Validation failed: Key can't be blank"
+        }
+
+        expect(Gitlab::Geo::Logger).to receive(:error)
+          .with(expected_message).and_call_original
+
+        subject.create!
+      end
+    end
+  end
+end

lib/feature.rb

@@ -2,6 +2,8 @@ require 'flipper/adapters/active_record'
 require 'flipper/adapters/active_support_cache_store'
 
 class Feature
+  prepend EE::Feature
+
   # Classes to override flipper table names
   class FlipperFeature < Flipper::Adapters::ActiveRecord::Feature
     # Using `self.table_name` won't work. ActiveRecord bug?
@@ -72,7 +74,11 @@ class Feature
     end
 
     def flipper
-      @flipper ||= (Gitlab::SafeRequestStore[:flipper] ||= build_flipper_instance)
+      if Gitlab::SafeRequestStore.active?
+        Gitlab::SafeRequestStore[:flipper] ||= build_flipper_instance
+      else
+        @flipper ||= build_flipper_instance
+      end
     end
 
     def build_flipper_instance