Replace LDAP group sync exclusive lease with state machine

CHANGELOG-EE

@@ -21,6 +21,7 @@ v 8.10.0
   - Rename Git Hooks to Push Rules
   - Fix EE keys fingerprint add index migration if came from CE
   - Add todos for MR approvers !547
+  - Replace LDAP group sync exclusive lease with state machine
   - Prevent the author of an MR from being on the approvers list
   - Isolate EE LDAP library code in EE module (Part 1) !511
   - Make Elasticsearch indexer run as an async task

app/models/ee/group.rb

+# Group EE mixin
+#
+# This module is intended to encapsulate EE-specific model logic
+# and be included in the `Group` model
+module EE
+  module Group
+    extend ActiveSupport::Concern
+
+    included do
+      state_machine :ldap_sync_status, namespace: :ldap_sync, initial: :ready do
+        state :ready
+        state :started
+        state :failed
+
+        event :start do
+          transition [:ready, :failed] => :started
+        end
+
+        event :finish do
+          transition started: :ready
+        end
+
+        event :fail do
+          transition started: :failed
+        end
+
+        after_transition ready: :started do |group, _|
+          group.ldap_sync_last_sync_at = DateTime.now
+          group.save
+        end
+
+        after_transition started: :ready do |group, _|
+          current_time = DateTime.now
+          group.ldap_sync_last_update_at = current_time
+          group.ldap_sync_last_successful_update_at = current_time
+          group.ldap_sync_error = nil
+          group.save
+        end
+
+        after_transition started: :failed do |group, _|
+          group.ldap_sync_last_update_at = DateTime.now
+          group.save
+        end
+      end
+    end
+
+    def mark_ldap_sync_as_failed(error_message)
+      return false unless ldap_sync_started?
+
+      fail_ldap_sync
+      update_column(:ldap_sync_error, ::Gitlab::UrlSanitizer.sanitize(error_message))
+    end
+  end
+end

app/models/group.rb

 require 'carrierwave/orm/activerecord'
 
+# Contains methods common to both GitLab CE and EE.
+# All EE methods should be in `EE::Group` only.
 class Group < Namespace
+  include EE::Group
   include Gitlab::ConfigHelper
   include Gitlab::VisibilityLevel
   include AccessRequestable

db/migrate/20160718210912_add_ldap_sync_state_to_groups.rb

+# Migration type: online without errors (works on previous version and new one)
+class AddLdapSyncStateToGroups < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+  disable_ddl_transaction!
+
+  DOWNTIME = false
+
+  def up
+    add_column_with_default :namespaces, :ldap_sync_status, :string, default: 'ready'
+    add_column :namespaces, :ldap_sync_error, :string
+    add_column :namespaces, :ldap_sync_last_update_at, :datetime
+    add_column :namespaces, :ldap_sync_last_successful_update_at, :datetime
+    add_column :namespaces, :ldap_sync_last_sync_at, :datetime
+  end
+
+  def down
+    remove_column :namespaces, :ldap_sync_status
+    remove_column :namespaces, :ldap_sync_error
+    remove_column :namespaces, :ldap_sync_last_update_at
+    remove_column :namespaces, :ldap_sync_last_successful_update_at
+    remove_column :namespaces, :ldap_sync_last_sync_at
+  end
+end

db/migrate/20160718210939_add_ldap_sync_state_indices_to_groups.rb

+# Migration type: online without errors (works on previous version and new one)
+class AddLdapSyncStateIndicesToGroups < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+  disable_ddl_transaction!
+
+  DOWNTIME = false
+
+  def up
+    add_concurrent_index :namespaces, :ldap_sync_last_update_at
+    add_concurrent_index :namespaces, :ldap_sync_last_successful_update_at
+  end
+
+  def down
+    remove_index :namespaces, column: :ldap_sync_last_update_at if index_exists?(:namespaces, :ldap_sync_last_update_at)
+    remove_index :namespaces, column: :ldap_sync_last_successful_update_at if index_exists?(:namespaces, :ldap_sync_last_successful_update_at)
+  end
+end

db/migrate/20160718211006_remove_last_ldap_sync_status_index_from_groups.rb

+# Migration type: online without errors (works on previous version and new one)
+class RemoveLastLdapSyncStatusIndexFromGroups < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+  disable_ddl_transaction!
+
+  DOWNTIME = false
+
+  def up
+    remove_index :namespaces, column: :last_ldap_sync_at if index_exists?(:namespaces, :last_ldap_sync_at)
+  end
+
+  def down
+    add_concurrent_index :namespaces, :last_ldap_sync_at
+  end
+end

db/migrate/20160718211059_remove_last_ldap_sync_status_from_groups.rb

+# Migration type: online without errors (works on previous version and new one)
+class RemoveLastLdapSyncStatusFromGroups < ActiveRecord::Migration
+
+  DOWNTIME = false
+
+  def change
+    remove_column :namespaces, :last_ldap_sync_at, :datetime
+  end
+end

db/schema.rb

@@ -756,12 +756,17 @@ ActiveRecord::Schema.define(version: 20160722221922) do
     t.boolean  "membership_lock",       default: false
     t.boolean  "share_with_group_lock", default: false
     t.integer  "visibility_level",      default: 20,    null: false
-    t.datetime "last_ldap_sync_at"
     t.boolean  "request_access_enabled", default: true,  null: false
+    t.string   "ldap_sync_status",                    default: "ready", null: false
+    t.string   "ldap_sync_error"
+    t.datetime "ldap_sync_last_update_at"
+    t.datetime "ldap_sync_last_successful_update_at"
+    t.datetime "ldap_sync_last_sync_at"
   end
 
   add_index "namespaces", ["created_at", "id"], name: "index_namespaces_on_created_at_and_id", using: :btree
-  add_index "namespaces", ["last_ldap_sync_at"], name: "index_namespaces_on_last_ldap_sync_at", using: :btree
+  add_index "namespaces", ["ldap_sync_last_successful_update_at"], name: "index_namespaces_on_ldap_sync_last_successful_update_at", using: :btree
+  add_index "namespaces", ["ldap_sync_last_update_at"], name: "index_namespaces_on_ldap_sync_last_update_at", using: :btree
   add_index "namespaces", ["name"], name: "index_namespaces_on_name", unique: true, using: :btree
   add_index "namespaces", ["name"], name: "index_namespaces_on_name_trigram", using: :gin, opclasses: {"name"=>"gin_trgm_ops"}
   add_index "namespaces", ["owner_id"], name: "index_namespaces_on_owner_id", using: :btree

lib/ee/gitlab/ldap/sync/group.rb

@@ -16,11 +16,13 @@ module EE
           end
 
           def update_permissions
-            lease = ::Gitlab::ExclusiveLease.new(
-              "ldap_group_sync:#{provider}:#{group.id}",
-              timeout: 3600
-            )
-            return unless lease.try_obtain
+            fail_stuck_group(group)
+
+            if group.ldap_sync_started?
+              logger.debug { "Group '#{group.name}' is not ready for LDAP sync. Skipping" }
+              return
+            end
+            group.start_ldap_sync
 
             logger.debug { "Syncing '#{group.name}' group" }
 
@@ -38,7 +40,7 @@ module EE
             update_existing_group_membership(group, access_levels)
             add_new_members(group, access_levels)
 
-            group.update(last_ldap_sync_at: Time.now)
+            group.finish_ldap_sync
 
             logger.debug { "Finished syncing '#{group.name}' group" }
           end
@@ -143,6 +145,14 @@ module EE
               .with_identity_provider(provider).preload(:user)
           end
 
+          def fail_stuck_group(group)
+            return false unless group.ldap_sync_started?
+
+            if group.ldap_sync_last_sync_at < 1.hour.ago
+              group.mark_ldap_sync_as_failed('The sync took too long to complete.')
+            end
+          end
+
           def logger
             Rails.logger
           end

lib/ee/gitlab/ldap/sync/groups.rb

@@ -73,7 +73,7 @@ module EE
           def groups_where_group_links_with_provider_ordered
             ::Group.where_group_links_with_provider(provider)
               .preload(:ldap_group_links)
-              .reorder('last_ldap_sync_at ASC, namespaces.id ASC')
+              .reorder('ldap_sync_last_successful_update_at ASC, namespaces.id ASC')
               .distinct
           end
 

spec/lib/ee/gitlab/ldap/sync/group_spec.rb

@@ -7,9 +7,6 @@ describe EE::Gitlab::LDAP::Sync::Group, lib: true do
   let(:user) { create(:user) }
 
   before do
-    allow_any_instance_of(::Gitlab::ExclusiveLease)
-      .to receive(:try_obtain).and_return(true)
-
     create(:identity, user: user, extern_uid: user_dn(user.username))
 
     stub_ldap_config(active_directory: false)
@@ -27,6 +24,35 @@ describe EE::Gitlab::LDAP::Sync::Group, lib: true do
       context 'with basic add/update actions' do
         let(:ldap_group1) { ldap_group_entry(user_dn(user.username)) }
 
+        it 'fails a stuck group older than 1 hour' do
+          group.start_ldap_sync
+          group.update_column(:ldap_sync_last_sync_at, 61.minutes.ago)
+
+          expect(group).to receive(:mark_ldap_sync_as_failed)
+
+          sync_group.update_permissions
+        end
+
+        context 'when the group ldap sync is already started' do
+          it 'logs a debug message' do
+            group.start_ldap_sync
+
+            expect(Rails.logger).to receive(:debug) do |&block|
+              expect(block.call).to match /^Group '\w*' is not ready for LDAP sync. Skipping/
+            end.at_least(1).times
+
+            sync_group.update_permissions
+          end
+
+          it 'does not add new members' do
+            group.start_ldap_sync
+
+            sync_group.update_permissions
+
+            expect(group.members).not_to include(user)
+          end
+        end
+
         it 'adds new members' do
           sync_group.update_permissions
 
@@ -54,6 +80,13 @@ describe EE::Gitlab::LDAP::Sync::Group, lib: true do
           expect(group.members.find_by(user_id: user.id).access_level)
             .to eq(::Gitlab::Access::DEVELOPER)
         end
+
+        it 'uses the ldap sync state machine' do
+          expect(group).to receive(:start_ldap_sync)
+          expect(group).to receive(:finish_ldap_sync)
+
+          sync_group.update_permissions
+        end
       end
 
       context 'when existing user is no longer in LDAP group' do

spec/models/ee/group_spec.rb

+require 'spec_helper'
+
+describe Group, models: true do
+  let(:group) { create(:group) }
+
+  it { is_expected.to include_module(EE::Group) }
+
+  describe 'states' do
+    it { is_expected.to be_ldap_sync_ready }
+
+    context 'after the start transition' do
+      it 'sets the last sync timestamp' do
+        expect { group.start_ldap_sync }.to change { group.ldap_sync_last_sync_at }
+      end
+    end
+
+    context 'after the finish transition' do
+      it 'sets the state to started' do
+        group.start_ldap_sync
+
+        expect(group).to be_ldap_sync_started
+
+        group.finish_ldap_sync
+      end
+
+      it 'sets last update and last successful update to the same timestamp' do
+        group.start_ldap_sync
+
+        group.finish_ldap_sync
+
+        expect(group.ldap_sync_last_update_at)
+          .to eq(group.ldap_sync_last_successful_update_at)
+      end
+
+      it 'clears previous error message on success' do
+        group.start_ldap_sync
+        group.mark_ldap_sync_as_failed('Error')
+        group.start_ldap_sync
+
+        group.finish_ldap_sync
+
+        expect(group.ldap_sync_error).to be_nil
+      end
+    end
+
+    context 'after the fail transition' do
+      it 'sets the state to failed' do
+        group.start_ldap_sync
+
+        group.fail_ldap_sync
+
+        expect(group).to be_ldap_sync_failed
+      end
+
+      it 'sets last update timestamp but not last successful update timestamp' do
+        group.start_ldap_sync
+
+        group.fail_ldap_sync
+
+        expect(group.ldap_sync_last_update_at)
+          .not_to eq(group.ldap_sync_last_successful_update_at)
+      end
+    end
+  end
+
+  describe '#mark_ldap_sync_as_failed' do
+    it 'sets the state to failed' do
+      group.start_ldap_sync
+
+      group.mark_ldap_sync_as_failed('Error')
+
+      expect(group).to be_ldap_sync_failed
+    end
+
+    it 'sets the error message' do
+      group.start_ldap_sync
+
+      group.mark_ldap_sync_as_failed('Something went wrong')
+
+      expect(group.ldap_sync_error).to eq('Something went wrong')
+    end
+
+    it 'is graceful when current state is not valid for the fail transition' do
+      expect(group).to be_ldap_sync_ready
+      expect { group.mark_ldap_sync_as_failed('Error') }.not_to raise_error
+    end
+  end
+end