Merge branch '1463-improve-repo-sync' into 'master'

Improve Repository Sync (with new SystemHook)

Closes #1463 and #1493

See merge request !1789

app/controllers/admin/hooks_controller.rb

@@ -60,6 +60,7 @@ class Admin::HooksController < Admin::ApplicationController
       :enable_ssl_verification,
       :push_events,
       :tag_push_events,
+      :repository_update_events,
       :token,
       :url
     )

app/models/geo_node.rb

@@ -159,8 +159,9 @@ class GeoNode < ActiveRecord::Base
     self.build_system_hook if system_hook.nil?
     self.system_hook.token = SecureRandom.hex(20) unless self.system_hook.token.present?
     self.system_hook.url = geo_events_url if uri.present?
-    self.system_hook.push_events = true
-    self.system_hook.tag_push_events = true
+    self.system_hook.push_events = false
+    self.system_hook.tag_push_events = false
+    self.system_hook.repository_update_events = true
   end
 
   def expire_cache!

app/models/hooks/system_hook.rb

 class SystemHook < WebHook
+  scope :repository_update_hooks, ->  { where(repository_update_events: true) }
+
+  default_value_for :push_events, false
+  default_value_for :repository_update_events, true
+
   def async_execute(data, hook_name)
     Sidekiq::Client.enqueue(SystemHookWorker, id, data, hook_name)
   end
-
-  def self.repository_update_hooks
-    GeoNode.where(primary: false).map(&:system_hook)
-  end
 end

app/models/hooks/web_hook.rb

@@ -10,6 +10,7 @@ class WebHook < ActiveRecord::Base
   default_value_for :tag_push_events, false
   default_value_for :build_events, false
   default_value_for :pipeline_events, false
+  default_value_for :repository_update_events, false
   default_value_for :enable_ssl_verification, true
 
   scope :push_hooks, -> { where(push_events: true) }

app/services/geo/repository_update_service.rb

+module Geo
+  class RepositoryUpdateService
+    attr_reader :project, :clone_url, :logger
+
+    LEASE_TIMEOUT = 1.hour.freeze
+    LEASE_KEY_PREFIX = 'geo_repository_fetch'.freeze
+
+    def initialize(project, clone_url, logger = Rails.logger)
+      @project = project
+      @clone_url = clone_url
+      @logger = logger
+    end
+
+    def execute
+      try_obtain_lease do
+        project.create_repository unless project.repository_exists?
+        project.repository.after_create if project.empty_repo?
+        project.repository.fetch_geo_mirror(clone_url)
+        project.repository.expire_all_method_caches
+        project.repository.expire_branch_cache
+        project.repository.expire_content_cache
+      end
+    rescue Gitlab::Shell::Error => e
+      logger.error "Error fetching repository for project #{project.path_with_namespace}: #{e}"
+    end
+
+    private
+
+    def try_obtain_lease
+      log('Trying to obtain lease to sync repository')
+
+      repository_lease = Gitlab::ExclusiveLease.new(lease_key, timeout: LEASE_TIMEOUT).try_obtain
+      unless repository_lease.present?
+        log('Could not obtain lease to sync repository')
+
+        return
+      end
+
+      begin
+        yield
+      ensure
+        log('Releasing leases to sync repository')
+        Gitlab::ExclusiveLease.cancel(lease_key, repository_lease)
+      end
+    end
+
+    def lease_key
+      @lease_key ||= "#{LEASE_KEY_PREFIX}:#{project.id}"
+    end
+
+    def log(message)
+      logger.info("#{self.class.name}: #{message} for project #{project.path_with_namespace} (#{project.id})")
+    end
+  end
+end

app/services/geo/schedule_repo_fetch_service.rb

@@ -2,7 +2,7 @@ module Geo
   class ScheduleRepoFetchService
     def initialize(params)
       @project_id = params[:project_id]
-      @remote_url = params[:remote_url]
+      @remote_url = params[:project][:git_ssh_url]
     end
 
     def execute

app/views/admin/hooks/_form.html.haml

@@ -18,19 +18,26 @@
       or adding ssh key. But you can also enable extra triggers like Push events.
 
     .prepend-top-default
+      = form.check_box :repository_update_events, class: 'pull-left'
+      .prepend-left-20
+        = form.label :repository_update_events, class: 'list-label' do
+          %strong Repository update events
+        %p.light
+          This URL will be triggered when repository is updated
+    %div
       = form.check_box :push_events, class: 'pull-left'
       .prepend-left-20
         = form.label :push_events, class: 'list-label' do
           %strong Push events
         %p.light
-          This url will be triggered by a push to the repository
+          This URL will be triggered for each branch updated to the repository
     %div
       = form.check_box :tag_push_events, class: 'pull-left'
       .prepend-left-20
         = form.label :tag_push_events, class: 'list-label' do
           %strong Tag push events
         %p.light
-          This url will be triggered when a new tag is pushed to the repository
+          This URL will be triggered when a new tag is pushed to the repository
 .form-group
   = form.label :enable_ssl_verification, 'SSL verification', class: 'control-label checkbox'
   .col-sm-10

app/views/admin/hooks/index.html.haml

@@ -27,7 +27,7 @@
             = link_to 'Remove', admin_hook_path(hook), data: { confirm: 'Are you sure?' }, method: :delete, class: 'btn btn-remove btn-sm'
           .monospace= hook.url
           %div
-            - %w(push_events tag_push_events issues_events note_events merge_requests_events build_events).each do |trigger|
+            - %w(repository_update_events push_events tag_push_events issues_events note_events merge_requests_events build_events).each do |trigger|
               - if hook.send(trigger)
                 %span.label.label-gray= trigger.titleize
             %span.label.label-gray SSL Verification: #{hook.enable_ssl_verification ? 'enabled' : 'disabled'}

app/workers/geo_repository_fetch_worker.rb

@@ -8,14 +8,6 @@ class GeoRepositoryFetchWorker
 
   def perform(project_id, clone_url)
     project = Project.find(project_id)
-
-    project.create_repository unless project.repository_exists?
-    project.repository.after_create if project.empty_repo?
-    project.repository.fetch_geo_mirror(clone_url)
-    project.repository.expire_all_method_caches
-    project.repository.expire_branch_cache
-    project.repository.expire_content_cache
-  rescue Gitlab::Shell::Error => e
-    logger.error "Error fetching repository for project #{project.path_with_namespace}: #{e}"
+    Geo::RepositoryUpdateService.new(project, clone_url, logger).execute
   end
 end

app/workers/post_receive.rb

@@ -23,26 +23,33 @@ class PostReceive
       # Triggers repository update on secondary nodes when Geo is enabled
       Gitlab::Geo.notify_wiki_update(post_received.project) if Gitlab::Geo.enabled?
     else
-      # TODO: gitlab-org/gitlab-ce#26325. Remove this.
-      if Gitlab::Geo.enabled?
-        hook_data = {
-          event_name: 'repository_update',
-          project_id: post_received.project.id,
-          project: post_received.project.hook_attrs,
-          remote_url: post_received.project.ssh_url_to_repo
-        }
-
-        SystemHooksService.new.execute_hooks(hook_data, :repository_update_hooks)
+      process_project_changes(post_received)
+      process_repository_update(post_received)
+    end
+  end
+
+  def process_repository_update(post_received)
+    changes = []
+    refs = Set.new
+
+    post_received.changes_refs do |oldrev, newrev, ref|
+      @user ||= post_received.identify(newrev)
+
+      unless @user
+        log("Triggered hook for non-existing user \"#{post_received.identifier}\"")
+        return false
       end
 
-      process_project_changes(post_received)
+      changes << Gitlab::DataBuilder::Repository.single_change(oldrev, newrev, ref)
+      refs << ref
     end
+
+    hook_data = Gitlab::DataBuilder::Repository.update(post_received.project, @user, changes, refs.to_a)
+    SystemHooksService.new.execute_hooks(hook_data, :repository_update_hooks)
   end
 
   def process_project_changes(post_received)
-    post_received.changes.each do |change|
-      oldrev, newrev, ref = change.strip.split(' ')
-
+    post_received.changes_refs do |oldrev, newrev, ref|
       @user ||= post_received.identify(newrev)
 
       unless @user

changelogs/unreleased-ee/1463-improve-repo-sync.yml

+---
+title: Geo: Improve Repository Sync (with new SystemHook)
+merge_request: 1789
+author:

db/migrate/20170503023315_add_repository_update_events_to_web_hooks.rb

+class AddRepositoryUpdateEventsToWebHooks < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_column_with_default :web_hooks, :repository_update_events, :boolean, default: false, allow_null: false
+  end
+
+  def down
+    remove_column :web_hooks, :repository_update_events
+  end
+end

db/migrate/20170505133904_update_geo_nodes_system_hooks.rb

+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class UpdateGeoNodesSystemHooks < ActiveRecord::Migration
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+
+  def up
+    attrs = {
+      push_events: false,
+      tag_push_events: false,
+      repository_update_events: true
+    }
+    SystemHook.joins('INNER JOIN geo_nodes ON geo_nodes.system_hook_id = web_hooks.id').update_all(attrs)
+  end
+
+  def down
+    attrs = {
+      push_events: true,
+      tag_push_events: true,
+      repository_update_events: false
+    }
+    SystemHook.joins('INNER JOIN geo_nodes ON geo_nodes.system_hook_id = web_hooks.id').update_all(attrs)
+  end
+end

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: 20170504102911) do
+ActiveRecord::Schema.define(version: 20170505133904) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -1606,6 +1606,7 @@ ActiveRecord::Schema.define(version: 20170504102911) do
     t.string "token"
     t.boolean "pipeline_events", default: false, null: false
     t.boolean "confidential_issues_events", default: false, null: false
+    t.boolean "repository_update_events", default: false, null: false
   end
 
   add_index "web_hooks", ["project_id"], name: "index_web_hooks_on_project_id", using: :btree

lib/api/entities.rb

@@ -50,7 +50,7 @@ module API
     end
 
     class Hook < Grape::Entity
-      expose :id, :url, :created_at, :push_events, :tag_push_events
+      expose :id, :url, :created_at, :push_events, :tag_push_events, :repository_update_events
       expose :enable_ssl_verification
     end
 

lib/api/geo.rb

@@ -66,7 +66,7 @@ module API
           required_attributes! %w(key id)
           ::Geo::ScheduleKeyChangeService.new(params).execute
         when 'repository_update'
-          required_attributes! %w(event_name project_id remote_url)
+          required_attributes! %w(event_name project_id project)
           ::Geo::ScheduleRepoFetchService.new(params).execute
         when 'push'
           required_attributes! %w(event_name project_id project)

lib/gitlab/data_builder/repository.rb

+module Gitlab
+  module DataBuilder
+    module Repository
+      extend self
+
+      # Produce a hash of post-receive data
+      def update(project, user, changes, refs)
+        {
+          event_name: 'repository_update',
+
+          user_id: user.id,
+          user_name: user.name,
+          user_email: user.email,
+          user_avatar: user.avatar_url,
+
+          project_id: project.id,
+          project: project.hook_attrs,
+
+          changes: changes,
+
+          refs: refs
+        }
+      end
+
+      # Produce a hash of partial data for a single change
+      def single_change(oldrev, newrev, ref)
+        {
+          before: oldrev,
+          after: newrev,
+          ref: ref
+        }
+      end
+    end
+  end
+end

lib/gitlab/git_post_receive.rb

@@ -13,6 +13,16 @@ module Gitlab
       super(identifier, project, revision)
     end
 
+    def changes_refs
+      return enum_for(:changes_refs) unless block_given?
+
+      changes.each do |change|
+        oldrev, newrev, ref = change.strip.split(' ')
+
+        yield oldrev, newrev, ref
+      end
+    end
+
     private
 
     def deserialize_changes(changes)

spec/controllers/admin/hooks_controller_spec.rb

+require 'spec_helper'
+
+describe Admin::HooksController do
+  let(:admin) { create(:admin) }
+
+  before do
+    sign_in(admin)
+  end
+
+  describe 'POST #create' do
+    it 'sets all parameters' do
+      hook_params = {
+        enable_ssl_verification: true,
+        push_events: true,
+        tag_push_events: true,
+        repository_update_events: true,
+        token: "TEST TOKEN",
+        url: "http://example.com",
+      }
+
+      post :create, hook: hook_params
+
+      expect(response).to have_http_status(302)
+      expect(SystemHook.all.size).to eq(1)
+      expect(SystemHook.first).to have_attributes(hook_params)
+    end
+  end
+end

spec/lib/gitlab/import_export/safe_model_attributes.yml

@@ -317,6 +317,7 @@ ProjectHook:
 - token
 - group_id
 - confidential_issues_events
+- repository_update_events
 ProtectedBranch:
 - id
 - project_id

spec/models/geo_node_spec.rb

@@ -89,8 +89,9 @@ describe GeoNode, type: :model do
         expect(node.system_hook.url).to be_present
         expect(node.system_hook.url).to eq(node.geo_events_url)
         expect(node.system_hook.token).to be_present
-        expect(node.system_hook.push_events).to be_truthy
-        expect(node.system_hook.tag_push_events).to be_truthy
+        expect(node.system_hook.push_events).to be_falsey
+        expect(node.system_hook.tag_push_events).to be_falsey
+        expect(node.system_hook.repository_update_events).to be_truthy
       end
     end
   end

spec/models/hooks/system_hook_spec.rb

 require "spec_helper"
 
 describe SystemHook, models: true do
+  context 'default attributes' do
+    let(:system_hook) { build(:system_hook) }
+
+    it 'sets defined default parameters' do
+      attrs = {
+        push_events: false,
+        repository_update_events: true,
+        enable_ssl_verification: true
+      }
+      expect(system_hook).to have_attributes(attrs)
+    end
+  end
+
   describe "execute" do
     let(:system_hook) { create(:system_hook) }
     let(:user)        { create(:user) }
@@ -105,4 +118,12 @@ describe SystemHook, models: true do
       ).once
     end
   end
+
+  describe '.repository_update_hooks' do
+    it 'returns hooks for repository update events only' do
+      hook = create(:system_hook, repository_update_events: true)
+      create(:system_hook, repository_update_events: false)
+      expect(SystemHook.repository_update_hooks).to eq([hook])
+    end
+  end
 end

spec/requests/api/system_hooks_spec.rb

@@ -32,8 +32,9 @@ describe API::SystemHooks do
         expect(response).to include_pagination_headers
         expect(json_response).to be_an Array
         expect(json_response.first['url']).to eq(hook.url)
-        expect(json_response.first['push_events']).to be true
+        expect(json_response.first['push_events']).to be false
         expect(json_response.first['tag_push_events']).to be false
+        expect(json_response.first['repository_update_events']).to be true
       end
     end
   end

spec/requests/api/v3/system_hooks_spec.rb

@@ -31,8 +31,9 @@ describe API::V3::SystemHooks do
         expect(response).to have_http_status(200)
         expect(json_response).to be_an Array
         expect(json_response.first['url']).to eq(hook.url)
-        expect(json_response.first['push_events']).to be true
+        expect(json_response.first['push_events']).to be false
         expect(json_response.first['tag_push_events']).to be false
+        expect(json_response.first['repository_update_events']).to be true
       end
     end
   end

spec/services/geo/repository_update_service_spec.rb

+require 'spec_helper'
+
+describe Geo::RepositoryUpdateService, services: true do
+  let(:project) { create(:empty_project) }
+  let(:clone_url) { project.ssh_url_to_repo }
+
+  subject { described_class.new(project, clone_url) }
+
+  describe '#execute' do
+    before do
+      allow_any_instance_of(Gitlab::Geo).to receive_messages(secondary?: true)
+      allow(project.repository).to receive(:fetch_geo_mirror).and_return(true)
+      allow(project).to receive(:repository_exists?) { false }
+      allow(project).to receive(:empty_repo?) { true }
+
+      allow(project.repository).to receive(:expire_all_method_caches)
+      allow(project.repository).to receive(:expire_branch_cache)
+      allow(project.repository).to receive(:expire_content_cache)
+    end
+
+    it 'releases the lease' do
+      expect(Gitlab::ExclusiveLease).to receive(:cancel).once.and_call_original
+
+      subject.execute
+    end
+
+    it 'creates a new repository' do
+      expect(project).to receive(:create_repository)
+
+      subject.execute
+    end
+
+    it 'executes after_create hook' do
+      expect(project.repository).to receive(:after_create).at_least(:once)
+
+      subject.execute
+    end
+
+    it 'fetches the Geo mirror' do
+      expect(project.repository).to receive(:fetch_geo_mirror)
+
+      subject.execute
+    end
+
+    it 'expires repository caches' do
+      expect(project.repository).to receive(:expire_all_method_caches)
+      expect(project.repository).to receive(:expire_branch_cache)
+      expect(project.repository).to receive(:expire_content_cache)
+
+      subject.execute
+    end
+
+    it 'does not raise exception when git failures occurs' do
+      expect(project.repository).to receive(:fetch_geo_mirror).and_raise(Gitlab::Shell::Error)
+
+      expect { subject.execute }.not_to raise_error
+    end
+  end
+end

spec/workers/geo_repository_fetch_worker_spec.rb

@@ -4,48 +4,11 @@ describe GeoRepositoryFetchWorker do
   describe '#perform' do
     let(:project) { create(:empty_project) }
 
-    before do
-      allow_any_instance_of(Gitlab::Geo).to receive_messages(secondary?: true)
-      allow_any_instance_of(Repository).to receive(:fetch_geo_mirror).and_return(true)
-      allow_any_instance_of(Project).to receive(:repository_exists?) { false }
-      allow_any_instance_of(Project).to receive(:empty_repo?) { true }
-
-      allow_any_instance_of(Repository).to receive(:expire_all_method_caches)
-      allow_any_instance_of(Repository).to receive(:expire_branch_cache)
-      allow_any_instance_of(Repository).to receive(:expire_content_cache)
-    end
-
-    it 'creates a new repository' do
-      expect_any_instance_of(Project).to receive(:create_repository)
-
-      perform
-    end
-
-    it 'executes after_create hook' do
-      expect_any_instance_of(Repository).to receive(:after_create).at_least(:once)
-
-      perform
-    end
-
-    it 'fetches the Geo mirror' do
-      expect_any_instance_of(Repository).to receive(:fetch_geo_mirror)
+    it 'delegates to Geo::RepositoryUpdateService' do
+      expect_any_instance_of(Geo::RepositoryUpdateService).to receive(:execute)
 
       perform
     end
-
-    it 'expires repository caches' do
-      expect_any_instance_of(Repository).to receive(:expire_all_method_caches)
-      expect_any_instance_of(Repository).to receive(:expire_branch_cache)
-      expect_any_instance_of(Repository).to receive(:expire_content_cache)
-
-      perform
-    end
-
-    it 'does not raise exception when git failures occurs' do
-      expect_any_instance_of(Repository).to receive(:fetch_geo_mirror).and_raise(Gitlab::Shell::Error)
-
-      expect { perform }.not_to raise_error
-    end
   end
 
   def perform

spec/workers/post_receive_spec.rb

@@ -9,7 +9,7 @@ describe PostReceive do
   let(:key) { create(:key, user: project.owner) }
   let(:key_id) { key.shell_id }
 
-  context "as a resque worker" do
+  context "as a sidekiq worker" do
     it "reponds to #perform" do
       expect(described_class.new).to respond_to(:perform)
     end
@@ -93,6 +93,27 @@ describe PostReceive do
     end
   end
 
+  describe '#process_repository_update' do
+    let(:changes) {'123456 789012 refs/heads/tést'}
+    let(:fake_hook_data) do
+      { event_name: 'repository_update' }
+    end
+
+    before do
+      allow_any_instance_of(Gitlab::GitPostReceive).to receive(:identify).and_return(project.owner)
+      allow_any_instance_of(Gitlab::DataBuilder::Repository).to receive(:update).and_return(fake_hook_data)
+      # silence hooks so we can isolate
+      allow_any_instance_of(Key).to receive(:post_create_hook).and_return(true)
+      allow(subject).to receive(:process_project_changes).and_return(true)
+    end
+
+    it 'calls SystemHooksService' do
+      expect_any_instance_of(SystemHooksService).to receive(:execute_hooks).with(fake_hook_data, :repository_update_hooks).and_return(true)
+
+      subject.perform(pwd(project), key_id, base64_changes)
+    end
+  end
+
   context "webhook" do
     it "fetches the correct project" do
       expect(Project).to receive(:find_by).with(id: project.id.to_s)