Pick Various improvements to repository checks into EE

Pick the changes from gitlab-org/gitlab-ce!18484 into EE.

app/views/admin/application_settings/_repository_check.html.haml

@@ -12,7 +12,7 @@
               Enable Repository Checks
             .help-block
               GitLab will periodically run
-              %a{ href: 'https://www.kernel.org/pub/software/scm/git/docs/git-fsck.html', target: 'blank' } 'git fsck'
+              %a{ href: 'https://git-scm.com/docs/git-fsck', target: 'blank' } 'git fsck'
               in all project and wiki repositories to look for silent disk corruption issues.
       .form-group
         .col-sm-offset-2.col-sm-10

app/workers/admin_email_worker.rb

@@ -3,6 +3,12 @@ class AdminEmailWorker
   include CronjobQueue
 
   def perform
+    send_repository_check_mail if Gitlab::CurrentSettings.repository_checks_enabled
+  end
+
+  private
+
+  def send_repository_check_mail
     repository_check_failed_count = Project.where(last_repository_check_failed: true).count
     return if repository_check_failed_count.zero?
 

app/workers/repository_check/batch_worker.rb

@@ -6,6 +6,8 @@ module RepositoryCheck
     RUN_TIME = 3600
 
     def perform
+      break unless Gitlab::CurrentSettings.repository_checks_enabled
+
       start = Time.now
 
       # This loop will break after a little more than one hour ('a little
@@ -15,7 +17,6 @@ module RepositoryCheck
       # check, only one (or two) will be checked at a time.
       project_ids.each do |project_id|
         break if Time.now - start >= RUN_TIME
-        break unless current_settings.repository_checks_enabled
 
         next unless try_obtain_lease(project_id)
 
@@ -47,16 +48,5 @@ module RepositoryCheck
         timeout: 24.hours
       ).try_obtain
     end
-
-    def current_settings
-      # No caching of the settings! If we cache them and an admin disables
-      # this feature, an active RepositoryCheckWorker would keep going for up
-      # to 1 hour after the feature was disabled.
-      if Rails.env.test?
-        Gitlab::CurrentSettings.fake_application_settings
-      else
-        ApplicationSetting.current
-      end
-    end
   end
 end

app/workers/repository_check/single_repository_worker.rb

@@ -5,27 +5,38 @@ module RepositoryCheck
 
     def perform(project_id)
       project = Project.find(project_id)
+
+      save_result(project, !check)
+    end
+
+    private
+
+    def save_result(project, failure)
       project.update_columns(
-        last_repository_check_failed: !check(project),
+        last_repository_check_failed: failure,
         last_repository_check_at: Time.now
       )
     end
 
-    private
-
     def check(project)
-      if has_pushes?(project) && !git_fsck(project.repository)
-        false
-      elsif project.wiki_enabled?
-        # Historically some projects never had their wiki repos initialized;
-        # this happens on project creation now. Let's initialize an empty repo
-        # if it is not already there.
-        project.create_wiki
-
-        git_fsck(project.wiki.repository)
-      else
-        true
-      end
+      check_repo(project) && check_wiki_repo(project)
+    end
+
+    def check_repo(project)
+      return true if project.empty_repo?
+
+      git_fsck(project.repository)
+    end
+
+    def check_wiki_repo(project)
+      return true unless project.wiki_enabled?
+
+      # Historically some projects never had their wiki repos initialized;
+      # this happens on project creation now. Let's initialize an empty repo
+      # if it is not already there.
+      project.create_wiki
+
+      git_fsck(project.wiki.repository)
     end
 
     def git_fsck(repository)

doc/administration/repository_checks.md

@@ -13,12 +13,12 @@ checks failed you can see their output on the admin log page under
 
 ## Periodic checks
 
-When enabled, GitLab periodically runs a repository check on all project 
-repositories and wiki repositories in order to detect data corruption problems. 
+When enabled, GitLab periodically runs a repository check on all project
+repositories and wiki repositories in order to detect data corruption problems.
 A project will be checked no more than once per month. If any projects
 fail their repository checks all GitLab administrators will receive an email
 notification of the situation. This notification is sent out once a week on
-Sunday, by default. 
+Sunday, by default.
 
 ## Disabling periodic checks
 
@@ -28,16 +28,18 @@ panel.
 ## What to do if a check failed
 
 If the repository check fails for some repository you should look up the error
-in repocheck.log (in the admin panel or on disk; see
-`/var/log/gitlab/gitlab-rails` for Omnibus installations or
-`/home/git/gitlab/log` for installations from source). Once you have
-resolved the issue use the admin panel to trigger a new repository check on
-the project. This will clear the 'check failed' state.
+in `repocheck.log`:
+
+- in the [admin panel](logs.md#repocheck.log)
+- or on disk, see:
+    - `/var/log/gitlab/gitlab-rails` for Omnibus installations
+    - `/home/git/gitlab/log` for installations from source
 
 If for some reason the periodic repository check caused a lot of false
-alarms you can choose to clear ALL repository check states from the
-'Settings' page of the admin panel.
+alarms you can choose to clear *all* repository check states by
+clicking "Clear all repository checks" on the 'Settings' page of the
+admin panel.
 
 ---
 [ce-3232]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/3232 "Auto git fsck"
-[git-fsck]: https://www.kernel.org/pub/software/scm/git/docs/git-fsck.html "git fsck documentation"
+[git-fsck]: https://git-scm.com/docs/git-fsck "git fsck documentation"

spec/workers/admin_email_worker_spec.rb

+require 'spec_helper'
+
+describe AdminEmailWorker do
+  subject(:worker) { described_class.new }
+
+  describe '.perform' do
+    it 'does not attempt to send repository check mail when they are disabled' do
+      stub_application_setting(repository_checks_enabled: false)
+
+      expect(worker).not_to receive(:send_repository_check_mail)
+
+      worker.perform
+    end
+
+    context 'repository_checks enabled' do
+      before do
+        stub_application_setting(repository_checks_enabled: true)
+      end
+
+      it 'checks if repository check mail should be sent' do
+        expect(worker).to receive(:send_repository_check_mail)
+
+        worker.perform
+      end
+
+      it 'does not send mail when there are no failed repos' do
+        expect(RepositoryCheckMailer).not_to receive(:notify)
+
+        worker.perform
+      end
+
+      it 'send mail when there is a failed repo' do
+        create(:project, last_repository_check_failed: true, last_repository_check_at: Date.yesterday)
+
+        expect(RepositoryCheckMailer).to receive(:notify).and_return(spy)
+
+        worker.perform
+      end
+    end
+  end
+end

spec/workers/repository_check/batch_worker_spec.rb

@@ -31,8 +31,8 @@ describe RepositoryCheck::BatchWorker do
 
   it 'does nothing when repository checks are disabled' do
     create(:project, created_at: 1.week.ago)
-    current_settings = double('settings', repository_checks_enabled: false)
-    expect(subject).to receive(:current_settings) { current_settings }
+
+    stub_application_setting(repository_checks_enabled: false)
 
     expect(subject.perform).to eq(nil)
   end

spec/workers/repository_check/single_repository_worker_spec.rb

@@ -2,44 +2,55 @@ require 'spec_helper'
 require 'fileutils'
 
 describe RepositoryCheck::SingleRepositoryWorker do
-  subject { described_class.new }
+  subject(:worker) { described_class.new }
 
-  it 'passes when the project has no push events' do
-    project = create(:project_empty_repo, :wiki_disabled)
-    project.events.destroy_all
-    break_repo(project)
+  it 'skips when the project repo is empty' do
+    project = create(:project, :wiki_disabled)
 
-    subject.perform(project.id)
+    expect(worker).not_to receive(:git_fsck)
+
+    worker.perform(project.id)
 
     expect(project.reload.last_repository_check_failed).to eq(false)
   end
 
-  it 'fails when the project has push events and a broken repository' do
-    project = create(:project_empty_repo)
-    create_push_event(project)
-    break_repo(project)
+  it 'succeeds when the project repo is valid' do
+    project = create(:project, :repository, :wiki_disabled)
 
-    subject.perform(project.id)
+    expect(worker).to receive(:git_fsck).and_call_original
+
+    expect do
+      worker.perform(project.id)
+    end.to change { project.reload.last_repository_check_at }
+
+    expect(project.reload.last_repository_check_failed).to eq(false)
+  end
+
+  it 'fails when the project is not empty and a broken repository' do
+    project = create(:project, :repository)
+    break_project(project)
+
+    worker.perform(project.id)
 
     expect(project.reload.last_repository_check_failed).to eq(true)
   end
 
   it 'fails if the wiki repository is broken' do
-    project = create(:project_empty_repo, :wiki_enabled)
+    project = create(:project, :wiki_enabled)
     project.create_wiki
 
     # Test sanity: everything should be fine before the wiki repo is broken
-    subject.perform(project.id)
+    worker.perform(project.id)
     expect(project.reload.last_repository_check_failed).to eq(false)
 
     break_wiki(project)
-    subject.perform(project.id)
+    worker.perform(project.id)
 
     expect(project.reload.last_repository_check_failed).to eq(true)
   end
 
   it 'skips wikis when disabled' do
-    project = create(:project_empty_repo, :wiki_disabled)
+    project = create(:project, :wiki_disabled)
     # Make sure the test would fail if the wiki repo was checked
     break_wiki(project)
 
@@ -49,8 +60,8 @@ describe RepositoryCheck::SingleRepositoryWorker do
   end
 
   it 'creates missing wikis' do
-    project = create(:project_empty_repo, :wiki_enabled)
-    FileUtils.rm_rf(wiki_path(project))
+    project = create(:project, :wiki_enabled)
+    Gitlab::Shell.new.rm_directory(project.repository_storage_path, project.wiki.path)
 
     subject.perform(project.id)
 
@@ -58,34 +69,35 @@ describe RepositoryCheck::SingleRepositoryWorker do
   end
 
   it 'does not create a wiki if the main repo does not exist at all' do
-    project = create(:project_empty_repo)
-    create_push_event(project)
-    FileUtils.rm_rf(project.repository.path_to_repo)
-    FileUtils.rm_rf(wiki_path(project))
+    project = create(:project, :repository)
+    Gitlab::Shell.new.rm_directory(project.repository_storage_path, project.path)
+    Gitlab::Shell.new.rm_directory(project.repository_storage_path, project.wiki.path)
 
     subject.perform(project.id)
 
-    expect(File.exist?(wiki_path(project))).to eq(false)
+    expect(Gitlab::Shell.new.exists?(project.repository_storage_path, project.wiki.path)).to eq(false)
   end
 
   def break_wiki(project)
-    objects_dir = wiki_path(project) + '/objects'
-
-    # Replace the /objects directory with a file so that the repo is
-    # invalid, _and_ 'git init' cannot fix it.
-    FileUtils.rm_rf(objects_dir)
-    FileUtils.touch(objects_dir) if File.directory?(wiki_path(project))
+    break_repo(wiki_path(project))
   end
 
   def wiki_path(project)
     project.wiki.repository.path_to_repo
   end
 
-  def create_push_event(project)
-    project.events.create(action: Event::PUSHED, author_id: create(:user).id)
+  def break_project(project)
+    break_repo(project.repository.path_to_repo)
   end
 
-  def break_repo(project)
-    FileUtils.rm_rf(File.join(project.repository.path_to_repo, 'objects'))
+  def break_repo(repo)
+    # Create or replace blob ffffffffffffffffffffffffffffffffffffffff with an empty file
+    # This will make the repo invalid, _and_ 'git init' cannot fix it.
+    path = File.join(repo, 'objects', 'ff')
+    file = File.join(path, 'ffffffffffffffffffffffffffffffffffffff')
+
+    FileUtils.mkdir_p(path)
+    FileUtils.rm_f(file)
+    FileUtils.touch(file)
   end
 end