Merge branch '99-support-configurable-sync-time' into 'master'

adds select field and sync time migration for mirror sync cron jobs

Closes #99

See merge request !1115

app/controllers/projects/mirrors_controller.rb

@@ -46,7 +46,8 @@ class Projects::MirrorsController < Projects::ApplicationController
   end
 
   def mirror_params
-    params.require(:project).permit(:mirror, :import_url, :mirror_user_id, :mirror_trigger_builds,
-      remote_mirrors_attributes: [:url, :id, :enabled])
+    params.require(:project).permit(:mirror, :import_url, :mirror_user_id,
+                                    :mirror_trigger_builds, :sync_time,
+                                    remote_mirrors_attributes: [:url, :id, :enabled, :sync_time])
   end
 end

app/models/project.rb

@@ -215,6 +215,10 @@ class Project < ActiveRecord::Base
   validates :repository_size_limit,
             numericality: { only_integer: true, greater_than_or_equal_to: 0, allow_nil: true }
 
+  validates :sync_time,
+    presence: true,
+    inclusion: { in: Gitlab::Mirror.sync_time_options.values }
+
   with_options if: :mirror? do |project|
     project.validates :import_url, presence: true
     project.validates :mirror_user, presence: true

app/models/remote_mirror.rb

@@ -12,6 +12,10 @@ class RemoteMirror < ActiveRecord::Base
   belongs_to :project, inverse_of: :remote_mirrors
 
   validates :url, presence: true, url: { protocols: %w(ssh git http https), allow_blank: true }
+  validates :sync_time,
+    presence: true,
+    inclusion: { in: Gitlab::Mirror.sync_time_options.values }
+
   validate  :url_availability, if: -> (mirror) { mirror.url_changed? || mirror.enabled? }
 
   after_save :refresh_remote, if: :mirror_url_changed?

app/views/projects/mirrors/show.html.haml

@@ -45,6 +45,9 @@
           They need to have at least master access to this project.
       - if @project.builds_enabled?
         = render "shared/mirror_trigger_builds_setting", f: f
+      .form-group
+        = f.label :sync_time, "Synchronization time", class: "label-light append-bottom-0"
+        = f.select :sync_time, options_for_select(Gitlab::Mirror.sync_time_options, @project.sync_time), {}, class: 'form-control'
     .col-sm-12
       %hr
     .col-lg-3
@@ -77,6 +80,9 @@
           = rm_form.label :url, "Git repository URL", class: "label-light"
           = rm_form.text_field :url, class: "form-control", placeholder: 'https://username:password@gitlab.company.com/group/project.git'
           = render "instructions"
+        .form-group
+          = rm_form.label :sync_time, "Synchronization time", class: "label-light append-bottom-0"
+          = rm_form.select :sync_time, options_for_select(Gitlab::Mirror.sync_time_options, @remote_mirror.sync_time), {}, class: 'form-control'
     .col-sm-12.text-center
       %hr
       = f.submit 'Save changes', class: 'btn btn-create', name: 'update_remote_mirror'

app/workers/update_all_mirrors_worker.rb

@@ -2,15 +2,15 @@ class UpdateAllMirrorsWorker
   include Sidekiq::Worker
   include CronjobQueue
 
-  LEASE_TIMEOUT = 3600
+  LEASE_TIMEOUT = 840
 
   def perform
     return unless try_obtain_lease
 
     fail_stuck_mirrors!
 
-    Project.mirror.find_each(batch_size: 200) do |project|
-      RepositoryUpdateMirrorDispatchWorker.perform_in(rand(30.minutes), project.id)
+    mirrors_to_sync.find_each(batch_size: 200) do |project|
+      RepositoryUpdateMirrorDispatchWorker.perform_in(rand((project.sync_time / 2).minutes), project.id)
     end
   end
 
@@ -26,8 +26,11 @@ class UpdateAllMirrorsWorker
 
   private
 
+  def mirrors_to_sync
+    Project.mirror.where(sync_time: Gitlab::Mirror.sync_times)
+  end
+
   def try_obtain_lease
-    # Using 30 minutes timeout based on the 95th percent of timings (currently max of 10 minutes)
     lease = ::Gitlab::ExclusiveLease.new("update_all_mirrors", timeout: LEASE_TIMEOUT)
     lease.try_obtain
   end

app/workers/update_all_remote_mirrors_worker.rb

@@ -5,7 +5,7 @@ class UpdateAllRemoteMirrorsWorker
   def perform
     fail_stuck_mirrors!
 
-    RemoteMirror.find_each(batch_size: 50).each(&:sync)
+    remote_mirrors_to_sync.find_each(batch_size: 50).each(&:sync)
   end
 
   def fail_stuck_mirrors!
@@ -13,4 +13,10 @@ class UpdateAllRemoteMirrorsWorker
       remote_mirror.mark_as_failed('The mirror update took too long to complete.')
     end
   end
+
+  private
+
+  def remote_mirrors_to_sync
+    RemoteMirror.where(sync_time: Gitlab::Mirror.sync_times)
+  end
 end

config/gitlab.yml.example

@@ -210,14 +210,6 @@ production: &base
     historical_data_worker:
       cron: "0 12 * * *"
 
-    # Update mirrored repositories
-    update_all_mirrors_worker:
-      cron: "0 * * * *"
-
-    # Update remote mirrors
-    update_all_remote_mirrors_worker:
-      cron: "30 * * * *"
-
     # In addition to refreshing users when they log in,
     # periodically refresh LDAP users membership.
     # NOTE: This will only take effect if LDAP is enabled

config/initializers/1_settings.rb

@@ -370,12 +370,6 @@ Settings.cron_jobs['repository_archive_cache_worker']['job_class'] = 'Repository
 Settings.cron_jobs['historical_data_worker'] ||= Settingslogic.new({})
 Settings.cron_jobs['historical_data_worker']['cron'] ||= '0 12 * * *'
 Settings.cron_jobs['historical_data_worker']['job_class'] = 'HistoricalDataWorker'
-Settings.cron_jobs['update_all_mirrors_worker'] ||= Settingslogic.new({})
-Settings.cron_jobs['update_all_mirrors_worker']['cron'] ||= '0 * * * *'
-Settings.cron_jobs['update_all_mirrors_worker']['job_class'] = 'UpdateAllMirrorsWorker'
-Settings.cron_jobs['update_all_remote_mirrors_worker'] ||= Settingslogic.new({})
-Settings.cron_jobs['update_all_remote_mirrors_worker']['cron'] ||= '30 * * * *'
-Settings.cron_jobs['update_all_remote_mirrors_worker']['job_class'] = 'UpdateAllRemoteMirrorsWorker'
 Settings.cron_jobs['ldap_sync_worker'] ||= Settingslogic.new({})
 Settings.cron_jobs['ldap_sync_worker']['cron'] ||= '30 1 * * *'
 Settings.cron_jobs['ldap_sync_worker']['job_class'] = 'LdapSyncWorker'

config/initializers/sidekiq.rb

@@ -34,6 +34,10 @@ Sidekiq.configure_server do |config|
   end
   Sidekiq::Cron::Job.load_from_hash! cron_jobs
 
+  # These jobs should not be allowed to be configured in gitlab.yml
+  Sidekiq::Cron::Job.create(name: 'update_all_remote_mirrors_worker', cron: '*/15 * * * *', class: 'UpdateAllRemoteMirrorsWorker')
+  Sidekiq::Cron::Job.create(name: 'update_all_mirrors_worker', cron: '*/15 * * * *', class: 'UpdateAllMirrorsWorker')
+
   # Gitlab Geo: enable bulk notify job only on primary node
   Gitlab::Geo.bulk_notify_job.disable! unless Gitlab::Geo.primary?
 

db/migrate/20170130113559_add_sync_schedule_to_projects_and_remote_projects.rb

+class AddSyncScheduleToProjectsAndRemoteProjects < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_column_with_default(:remote_mirrors, :sync_time, :integer, default: 60)
+    add_column_with_default(:projects, :sync_time, :integer, default: 60)
+  end
+
+  def down
+    remove_column :projects, :sync_time
+    remove_column :remote_mirrors, :sync_time
+  end
+end

db/schema.rb

@@ -1112,6 +1112,7 @@ ActiveRecord::Schema.define(version: 20170204181513) do
     t.text "description_html"
     t.boolean "only_allow_merge_if_all_discussions_are_resolved"
     t.integer "repository_size_limit", limit: 8
+    t.integer "sync_time", default: 60, null: false
   end
 
   add_index "projects", ["ci_id"], name: "index_projects_on_ci_id", using: :btree
@@ -1205,6 +1206,7 @@ ActiveRecord::Schema.define(version: 20170204181513) do
     t.string "encrypted_credentials_salt"
     t.datetime "created_at", null: false
     t.datetime "updated_at", null: false
+    t.integer "sync_time", default: 60, null: false
   end
 
   add_index "remote_mirrors", ["project_id"], name: "index_remote_mirrors_on_project_id", using: :btree

doc/administration/cron_jobs.md

-# Cron jobs
-
-## Adjusting synchronization times for repository mirroring
-
->**Notes:**
-- This is an [Enterprise Edition][ee] only feature.
-- For more information on the repository mirroring, see the
-  [user documentation](../workflow/repository_mirroring.md).
-
-You can manually configure the repository synchronization times by setting the
-following configuration values.
-
-Please note that `update_all_mirrors_worker_cron` refers to the worker used for
-pulling changes from a remote mirror while `update_all_remote_mirrors_worker_cron`
-refers to the worker used for pushing changes to the remote mirror.
-
->**Note:**
-These are cron formatted values. You can use a crontab generator to create these
-values, for example http://www.crontabgenerator.com/.
-
-**Omnibus installations**
-
-```
-gitlab_rails['update_all_mirrors_worker_cron'] = "0 * * * *"
-gitlab_rails['update_all_remote_mirrors_worker_cron'] = "30 * * * *"
-```
-
-**Source installations**
-
-```
-cron_jobs:
-  update_all_mirrors_worker_cron:
-    cron: "0 * * * *"
-  update_all_remote_mirrors_worker_cron:
-    cron: "30 * * * *"
-```
-
-[ee]: https://about.gitlab.com/products

doc/workflow/repository_mirroring.md

@@ -7,7 +7,7 @@ There are two kinds of repository mirroring features supported by GitLab:
 to another location, whereas the **pull** method mirrors an external repository
 in one in GitLab.
 
-Mirror repositories are updated every hour, and all new branches, tags, and
+By default mirror repositories are updated every hour, and all new branches, tags, and
 commits will be visible in the project's activity feed.
 
 Users with at least [developer access][perms] to the project can also force an
@@ -51,8 +51,8 @@ whether you want to trigger builds for mirror updates.
 Since the repository on GitLab functions as a mirror of the upstream repository,
 you are advised not to push commits directly to the repository on GitLab.
 Instead, any commits should be pushed to the upstream repository, and will end
-up in the GitLab repository automatically within an hour, or when a
-[forced update](#forcing-an-update) is initiated.
+up in the GitLab repository automatically within your project's configured
+synchronization time, or when a [forced update](#forcing-an-update) is initiated.
 
 If you do manually update a branch in the GitLab repository, the branch will
 become diverged from upstream, and GitLab will no longer automatically update
@@ -72,8 +72,8 @@ repository to push to. Hit **Save changes** for the changes to take effect.
 Similarly to the pull mirroring, since the upstream repository functions as a
 mirror to the repository in GitLab, you are advised not to push commits directly
 to the mirrored repository. Instead, any commits should be pushed to GitLab,
-and will end up in the mirrored repository automatically within an hour, or when
-a [forced update](#forcing-an-update) is initiated.
+and will end up in the mirrored repository automatically within the configured time,
+or when a [forced update](#forcing-an-update) is initiated.
 
 In case of a diverged branch, you will see an error indicated at the
 **Mirror repository** settings.
@@ -82,7 +82,7 @@ In case of a diverged branch, you will see an error indicated at the
 
 ## Forcing an update
 
-While mirrors update once an hour, you can force an update (either **push** or
+While mirrors update at a pre-configured time (hourly by default), you can always force an update (either **push** or
 **pull**) by using the **Update now** button which is exposed in various places:
 
 - in the commits page
@@ -92,22 +92,24 @@ While mirrors update once an hour, you can force an update (either **push** or
 
 ## Adjusting synchronization times
 
-You can adjust the synchronization times for the repository mirroring if you
-have access to the GitLab server. For more information, see
-[the administration documentation][sync-times].
+Your repository's default synchronization time is hourly.
+However, you can adjust it by visiting the **Mirror repository** page
+under the wheel icon in the upper right corner.
+Check the Synchronization time section where you can choose to have your mirror
+be updated once every fifteen minutes, hourly or daily and then hit **Save changes**
+at the bottom.
 
 ## Using both mirroring methods at the same time
 
 Currently there is no bidirectional support without conflicts. That means that
 if you configure a repository to both pull and push to a second one, there is
 no guarantee that it will update correctly on both remotes. You could
-[adjust the synchronization times][sync-times] to a very low value and hope
-that no conflicts occur during the pull/push window time, but that is not a
-solution to consider on a production environment. Another thing you could try
-is [configuring custom Git hooks][hooks] on the GitLab server.
+adjust the synchronization times on the mirror settings page
+to a very low value and hope that no conflicts occur during
+the pull/push window time, but that is not a solution to consider on a
+production environment. Another thing you could try is [configuring custom Git hooks][hooks] on the GitLab server.
 
 
 [ee-51]: https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/51
 [perms]: ../user/permissions.md
-[sync-times]: ../administration/cron_jobs.md#adjusting-synchronization-times-for-repository-mirroring
 [hooks]: https://docs.gitlab.com/ee/administration/custom_hooks.html

lib/gitlab/mirror.rb

+module Gitlab
+  module Mirror
+    FIFTEEN = 15
+    HOURLY  = 60
+    DAILY = 1440
+
+    INTERVAL_BEFORE_FIFTEEN = 14.minutes
+
+    class << self
+      def sync_time_options
+        {
+          "Update every 15 minutes" => FIFTEEN,
+          "Update hourly" => HOURLY,
+          "Update every day" => DAILY,
+        }
+      end
+
+      def sync_times
+        sync_times = [FIFTEEN]
+        sync_times << DAILY  if at_beginning_of_day?
+        sync_times << HOURLY if at_beginning_of_hour?
+
+        sync_times
+      end
+
+      def at_beginning_of_day?
+        start_at = DateTime.now.at_beginning_of_day
+        end_at = start_at + INTERVAL_BEFORE_FIFTEEN
+
+        DateTime.now.between?(start_at, end_at)
+      end
+
+      def at_beginning_of_hour?
+        start_at = DateTime.now.at_beginning_of_hour
+        end_at = start_at + INTERVAL_BEFORE_FIFTEEN
+
+        DateTime.now.between?(start_at, end_at)
+      end
+    end
+  end
+end

spec/controllers/projects/mirrors_controller_spec.rb

 require 'spec_helper'
 
 describe Projects::MirrorsController do
+  let(:sync_times) { Gitlab::Mirror.sync_time_options.values }
+
+  describe 'setting up a mirror' do
+    context 'when the current project is a mirror' do
+      before do
+        @project = create(:project, :mirror)
+        sign_in(@project.owner)
+      end
+
+      context 'sync_time update' do
+        it 'allows sync_time update with valid time' do
+          sync_times.each do |sync_time|
+            expect do
+              do_put(@project, sync_time: sync_time)
+            end.to change { Project.mirror.where(sync_time: sync_time).count }.by(1)
+          end
+        end
+
+        it 'fails to update sync_time with invalid time' do
+          expect do
+            do_put(@project, sync_time: 1000)
+          end.not_to change { @project.sync_time }
+        end
+      end
+    end
+  end
+
   describe 'setting up a remote mirror' do
     context 'when the current project is a mirror' do
       before do
@@ -14,6 +41,24 @@ describe Projects::MirrorsController do
         end.to change { RemoteMirror.count }.to(1)
       end
 
+      context 'sync_time update' do
+        it 'allows sync_time update with valid time' do
+          sync_times.each do |sync_time|
+            expect do
+              do_put(@project, remote_mirrors_attributes: { '0' => { 'enabled' => 1, 'url' => 'http://foo.com', 'sync_time' => sync_time } })
+            end.to change { RemoteMirror.where(sync_time: sync_time).count }.by(1)
+          end
+        end
+
+        it 'fails to update sync_time with invalid time' do
+          expect(@project.remote_mirrors.count).to eq(0)
+
+          expect do
+            do_put(@project, remote_mirrors_attributes: { '0' => { 'enabled' => 1, 'url' => 'http://foo.com', 'sync_time' => 1000 } })
+          end.not_to change { @project.remote_mirrors.count }
+        end
+      end
+
       context 'when remote mirror has the same URL' do
         it 'does not allow to create the remote mirror' do
           expect do

spec/factories/projects.rb

@@ -59,6 +59,18 @@ FactoryGirl.define do
       end
     end
 
+    trait :remote_mirror do
+      transient do
+        sync_time Gitlab::Mirror::HOURLY
+        url "http://foo.com"
+        enabled true
+      end
+
+      after(:create) do |project, evaluator|
+        project.remote_mirrors.create!(url: evaluator.url, enabled: evaluator.enabled, sync_time: evaluator.sync_time)
+      end
+    end
+
     trait :read_only_repository do
       repository_read_only true
     end

spec/workers/update_all_mirrors_worker_spec.rb

@@ -7,6 +7,16 @@ describe UpdateAllMirrorsWorker do
   end
 
   describe '#perform' do
+    project_count_with_time = { DateTime.now.beginning_of_hour + 15.minutes => 1,
+                                DateTime.now.beginning_of_hour => 2,
+                                DateTime.now.beginning_of_day => 3
+                              }
+
+    let!(:mirror1) { create(:empty_project, :mirror, sync_time: Gitlab::Mirror::FIFTEEN) }
+    let!(:mirror2) { create(:empty_project, :mirror, sync_time: Gitlab::Mirror::HOURLY) }
+    let!(:mirror3) { create(:empty_project, :mirror, sync_time: Gitlab::Mirror::DAILY) }
+    let(:mirrors) { Project.mirror.where(sync_time: Gitlab::Mirror.sync_times) }
+
     it 'fails stuck mirrors' do
       worker = described_class.new
 
@@ -15,17 +25,25 @@ describe UpdateAllMirrorsWorker do
       worker.perform
     end
 
-    it 'enqueue a job on all mirrored Projects' do
-      worker = described_class.new
+    project_count_with_time.each do |time, project_count|
+      describe "at #{time}" do
+        before do
+          allow(DateTime).to receive(:now).and_return(time)
+        end
 
-      mirror = create(:empty_project, :mirror)
-      create(:empty_project)
+        it 'enqueues a job on mirrored Projects' do
+          worker = described_class.new
 
-      expect(worker).to receive(:rand).with(30.minutes).and_return(10)
-      expect(RepositoryUpdateMirrorDispatchWorker).to receive(:perform_in).with(10, mirror.id)
+          expect(mirrors.count).to eq(project_count)
+          mirrors.each do |mirror|
+            expect(worker).to receive(:rand).with((mirror.sync_time / 2).minutes).and_return(mirror.sync_time / 2)
+            expect(RepositoryUpdateMirrorDispatchWorker).to receive(:perform_in).with(mirror.sync_time / 2, mirror.id)
+          end
 
           worker.perform
         end
+      end
+    end
 
     it 'does not execute if cannot get the lease' do
       allow_any_instance_of(Gitlab::ExclusiveLease)

spec/workers/update_all_remote_mirrors_worker.rb

+require 'rails_helper'
+
+describe UpdateAllRemoteMirrorsWorker do
+  describe "#perform" do
+    project_count_with_time = { DateTime.now.beginning_of_hour + 15.minutes => 1,
+                                DateTime.now.beginning_of_hour => 2,
+                                DateTime.now.beginning_of_day => 3
+                              }
+
+    let!(:mirror1) { create(:project, :remote_mirror, sync_time: Gitlab::Mirror::FIFTEEN) }
+    let!(:mirror2) { create(:project, :remote_mirror, sync_time: Gitlab::Mirror::HOURLY) }
+    let!(:mirror3) { create(:project, :remote_mirror, sync_time: Gitlab::Mirror::DAILY) }
+    let(:mirrors) { RemoteMirror.where(sync_time: Gitlab::Mirror.sync_times) }
+
+    it 'fails stuck mirrors' do
+      worker = described_class.new
+
+      expect(worker).to receive(:fail_stuck_mirrors!)
+
+      worker.perform
+    end
+
+    project_count_with_time.each do |time, project_count|
+      describe "at #{time}" do
+        before do
+          allow(DateTime).to receive(:now).and_return(time)
+        end
+
+        it 'enqueues a job on mirrored Projects' do
+          worker = described_class.new
+
+          expect(mirrors.count).to eq(project_count)
+          mirrors.each do |mirror|
+            expect(RepositoryUpdateRemoteMirrorWorker).to receive(:perform_async).with(mirror.id)
+          end
+
+          worker.perform
+        end
+      end
+    end
+  end
+end