Merge branch '3569-geo-backfill-unconditional-wiki' into 'master'

Geo: Don't sync disabled project wikis

Closes #3569

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

app/models/geo/project_registry.rb

@@ -20,19 +20,29 @@ class Geo::ProjectRegistry < Geo::BaseRegistry
       .where(resync_repository: false, resync_wiki: false)
   end
 
-  def resync_repository?
-    resync_repository || last_repository_successful_sync_at.nil?
+  def repository_sync_due?(scheduled_time)
+    never_synced_repository? || repository_sync_needed?(scheduled_time)
   end
 
-  def resync_wiki?
-    resync_wiki || last_wiki_successful_sync_at.nil?
+  def wiki_sync_due?(scheduled_time)
+    project.wiki_enabled? && (never_synced_wiki? || wiki_sync_needed?(scheduled_time))
   end
 
-  def repository_synced_since?(timestamp)
-    last_repository_synced_at && last_repository_synced_at > timestamp
+  private
+
+  def never_synced_repository?
+    last_repository_successful_sync_at.nil?
+  end
+
+  def never_synced_wiki?
+    last_wiki_successful_sync_at.nil?
+  end
+
+  def repository_sync_needed?(timestamp)
+    resync_repository? && (last_repository_synced_at.nil? || timestamp > last_repository_synced_at)
   end
 
-  def wiki_synced_since?(timestamp)
-    last_wiki_synced_at && last_wiki_synced_at > timestamp
+  def wiki_sync_needed?(timestamp)
+    resync_wiki? && (last_wiki_synced_at.nil? || timestamp > last_wiki_synced_at)
   end
 end

app/services/geo/repository_created_event_store.rb

@@ -10,7 +10,7 @@ module Geo
         repository_storage_name: project.repository.storage,
         repository_storage_path: project.repository_storage_path,
         repo_path: project.disk_path,
-        wiki_path: "#{project.disk_path}.wiki",
+        wiki_path: ("#{project.disk_path}.wiki" if project.wiki_enabled?),
         project_name: project.name
       )
     end

app/workers/geo/project_sync_worker.rb

@@ -11,30 +11,16 @@ module Geo
     end
 
     def perform(project_id, scheduled_time)
-      project  = Project.find(project_id)
       registry = Geo::ProjectRegistry.find_or_initialize_by(project_id: project_id)
+      project = registry.project
 
-      Geo::RepositorySyncService.new(project).execute if sync_repository?(registry, scheduled_time)
-      Geo::WikiSyncService.new(project).execute if sync_wiki?(registry, scheduled_time)
-    rescue ActiveRecord::RecordNotFound => e
-      Gitlab::Geo::Logger.error(
-        class: self.class.name,
-        message: "Couldn't find project, skipping syncing",
-        project_id: project_id,
-        error: e
-      )
-    end
-
-    private
-
-    def sync_repository?(registry, scheduled_time)
-      !registry.repository_synced_since?(scheduled_time) &&
-        registry.resync_repository?
-    end
+      if project.nil?
+        Gitlab::Geo::Logger.error(class: self.class.name, message: "Couldn't find project, skipping syncing", project_id: project_id)
+        return
+      end
 
-    def sync_wiki?(registry, scheduled_time)
-      !registry.wiki_synced_since?(scheduled_time) &&
-        registry.resync_wiki?
+      Geo::RepositorySyncService.new(project).execute if registry.repository_sync_due?(scheduled_time)
+      Geo::WikiSyncService.new(project).execute if registry.wiki_sync_due?(scheduled_time)
     end
   end
 end

changelogs/unreleased-ee/3569-geo-backfill-unconditional-wiki.yml

+---
+title: 'Geo: Don''t sync disabled project wikis'
+merge_request: 3109
+author:
+type: fixed

lib/gitlab/geo/log_cursor/daemon.rb

@@ -19,9 +19,7 @@ module Gitlab
           full_scan! if options[:full_scan]
 
           until exit?
-            Events.fetch_in_batches do |batch|
-              handle_events(batch)
-            end
+            run_once!
 
             return if exit?
 
@@ -30,6 +28,10 @@ module Gitlab
           end
         end
 
+        def run_once!
+          Events.fetch_in_batches { |batch| handle_events(batch) }
+        end
+
         # Execute routines to verify the required initial data is available
         # and mark non-replicated data as requiring replication.
         def full_scan!

spec/factories/geo/event_log.rb

@@ -24,7 +24,7 @@ FactoryGirl.define do
     repository_storage_path { project.repository_storage_path }
     add_attribute(:repo_path) { project.disk_path }
     project_name { project.name }
-    wiki_path { "{project.disk_path}.wiki" }
+    wiki_path { "#{project.disk_path}.wiki" }
   end
 
   factory :geo_repository_updated_event, class: Geo::RepositoryUpdatedEvent do

spec/models/geo/project_registry_spec.rb

 require 'spec_helper'
 
 describe Geo::ProjectRegistry do
-  subject { create(:geo_project_registry) }
+  using RSpec::Parameterized::TableSyntax
+
+  set(:project) { create(:project) }
+  set(:registry) { create(:geo_project_registry, project_id: project.id) }
+
+  subject { registry }
 
   describe 'relationships' do
     it { is_expected.to belong_to(:project) }
@@ -33,85 +38,79 @@ describe Geo::ProjectRegistry do
     end
   end
 
-  describe '#resync_repository?' do
-    it 'returns true when resync_repository is true' do
-      subject.resync_repository = true
-
-      expect(subject.resync_repository).to be true
-    end
-
-    it 'returns true when last_repository_successful_sync_at is nil' do
-      subject.last_repository_successful_sync_at = nil
-
-      expect(subject.resync_repository).to be true
-    end
-
-    it 'returns false when resync_repository is false and last_repository_successful_sync_at is present' do
-      subject.resync_repository = false
-      subject.last_repository_successful_sync_at = Time.now
-
-      expect(subject.resync_repository).to be false
-    end
-  end
-
-  describe '#resync_wiki?' do
-    it 'returns true when resync_wiki is true' do
-      subject.resync_wiki = true
-
-      expect(subject.resync_wiki).to be true
+  describe '#repository_sync_due?' do
+    where(:resync_repository, :last_successful_sync, :last_sync, :expected) do
+      now = Time.now
+      past = now - 1.year
+      future = now + 1.year
+
+      true  | nil | nil | true
+      true  | now | nil | true
+      false | nil | nil | true
+      false | now | nil | false
+
+      true  | nil | past | true
+      true  | now | past | true
+      false | nil | past | true
+      false | now | past | false
+
+      true  | nil | future | true
+      true  | now | future | false
+      false | nil | future | true
+      false | now | future | false
     end
 
-    it 'returns true when last_wiki_successful_sync_at is nil' do
-      subject.last_wiki_successful_sync_at = nil
+    with_them do
+      before do
+        registry.update!(resync_repository: resync_repository, last_repository_successful_sync_at: last_successful_sync, last_repository_synced_at: last_sync)
+      end
 
-      expect(subject.resync_wiki).to be true
-    end
-
-    it 'returns false when resync_wiki is false and last_wiki_successful_sync_at is present' do
-      subject.resync_wiki = false
-      subject.last_wiki_successful_sync_at = Time.now
+      subject { registry.repository_sync_due?(Time.now) }
 
-      expect(subject.resync_wiki).to be false
+      it { is_expected.to eq(expected) }
     end
   end
 
-  describe '#repository_synced_since?' do
-    it 'returns false when last_repository_synced_at is nil' do
-      subject.last_repository_synced_at = nil
-
-      expect(subject.repository_synced_since?(Time.now)).to be_nil
+  describe '#wiki_sync_due?' do
+    where(:resync_wiki, :last_successful_sync, :last_sync, :expected) do
+      now = Time.now
+      past = now - 1.year
+      future = now + 1.year
+
+      true  | nil | nil | true
+      true  | now | nil | true
+      false | nil | nil | true
+      false | now | nil | false
+
+      true  | nil | past | true
+      true  | now | past | true
+      false | nil | past | true
+      false | now | past | false
+
+      true  | nil | future | true
+      true  | now | future | false
+      false | nil | future | true
+      false | now | future | false
     end
 
-    it 'returns false when last_repository_synced_at before timestamp' do
-      subject.last_repository_synced_at = Time.now - 2.hours
+    with_them do
+      before do
+        registry.update!(resync_wiki: resync_wiki, last_wiki_successful_sync_at: last_successful_sync, last_wiki_synced_at: last_sync)
+      end
 
-      expect(subject.repository_synced_since?(Time.now)).to be false
-    end
+      subject { registry.wiki_sync_due?(Time.now) }
 
-    it 'returns true when last_repository_synced_at after timestamp' do
-      subject.last_repository_synced_at = Time.now + 2.hours
-
-      expect(subject.repository_synced_since?(Time.now)).to be true
-    end
-  end
-
-  describe '#wiki_synced_since?' do
-    it 'returns false when last_wiki_synced_at is nil' do
-      subject.last_wiki_synced_at = nil
-
-      expect(subject.wiki_synced_since?(Time.now)).to be_nil
-    end
-
-    it 'returns false when last_wiki_synced_at before timestamp' do
-      subject.last_wiki_synced_at = Time.now - 2.hours
-
-      expect(subject.wiki_synced_since?(Time.now)).to be false
-    end
+      context 'wiki enabled' do
+        it { is_expected.to eq(expected) }
+      end
 
-    it 'returns true when last_wiki_synced_at after timestamp' do
-      subject.last_wiki_synced_at = Time.now + 2.hours
+      context 'wiki disabled' do
+        before do
+          project.update!(wiki_enabled: false)
+        end
 
-      expect(subject.wiki_synced_since?(Time.now)).to be true
+        it { is_expected.to be_falsy }
+      end
     end
   end
 end

spec/services/geo/repository_created_event_store_spec.rb

 require 'spec_helper'
 
 describe Geo::RepositoryCreatedEventStore do
-  let(:project) { create(:project) }
+  set(:project) { create(:project) }
 
-  subject(:event) { described_class.new(project) }
+  subject(:create!) { described_class.new(project).create }
 
   describe '#create' do
     it 'does not create an event when not running on a primary node' do
       allow(Gitlab::Geo).to receive(:primary?) { false }
 
-      expect { event.create }.not_to change(Geo::RepositoryCreatedEvent, :count)
+      expect { create! }.not_to change(Geo::RepositoryCreatedEvent, :count)
     end
 
-    context 'when running on a primary node' do
+    context 'running on a primary node' do
       before do
         allow(Gitlab::Geo).to receive(:primary?) { true }
       end
 
       it 'creates a created event' do
-        expect { event.create }.to change(Geo::RepositoryCreatedEvent, :count).by(1)
+        expect { create! }.to change(Geo::RepositoryCreatedEvent, :count).by(1)
       end
 
       it 'tracks information for the created project' do
-        event.create
+        create!
 
         event = Geo::RepositoryCreatedEvent.last
 
@@ -35,6 +35,15 @@ describe Geo::RepositoryCreatedEventStore do
           repository_storage_path: project.repository_storage_path
         )
       end
+
+      it 'does not set a wiki path if the wiki is disabled' do
+        project.update!(wiki_enabled: false)
+
+        create!
+
+        event = Geo::RepositoryCreatedEvent.last
+        expect(event.wiki_path).to be_nil
+      end
     end
   end
 end

spec/workers/geo/project_sync_worker_spec.rb

@@ -102,6 +102,23 @@ RSpec.describe Geo::ProjectSyncWorker do
       end
     end
 
+    context 'wiki is not enabled for project' do
+      let!(:registry) { create(:geo_project_registry, resync_repository: true, resync_wiki: true, project: project) }
+
+      before do
+        project.update!(wiki_enabled: false)
+        subject.perform(project.id, Time.now)
+      end
+
+      it 'syncs the project repository' do
+        expect(repository_sync_service).to have_received(:execute)
+      end
+
+      it 'does not sync the project wiki' do
+        expect(wiki_sync_service).not_to have_received(:execute)
+      end
+    end
+
     context 'when project repository was synced after the time the job was scheduled in' do
       it 'does not perform Geo::RepositorySyncService for the given project' do
         create(:geo_project_registry, :synced, :repository_dirty, project: project, last_repository_synced_at: Time.now)