diff options
author | Tiago Botelho <tiagonbotelho@hotmail.com> | 2017-09-04 20:55:04 +0300 |
---|---|---|
committer | Tiago Botelho <tiagonbotelho@hotmail.com> | 2017-09-07 20:52:04 +0300 |
commit | 39298575a819ade6ad4f9e37a7f22592a05d21f8 (patch) | |
tree | 43319ff3c1bb533e20c5185571aa7341be984f31 | |
parent | 21935d85382989e38dd4cc12de55966e0c9b6eba (diff) |
Adds exclusive lease to Git garbage collect worker.
-rw-r--r-- | app/workers/git_garbage_collect_worker.rb | 34 | ||||
-rw-r--r-- | lib/gitlab/exclusive_lease.rb | 10 | ||||
-rw-r--r-- | spec/lib/gitlab/exclusive_lease_spec.rb | 12 | ||||
-rw-r--r-- | spec/services/projects/housekeeping_service_spec.rb | 1 | ||||
-rw-r--r-- | spec/workers/git_garbage_collect_worker_spec.rb | 121 |
5 files changed, 154 insertions, 24 deletions
diff --git a/app/workers/git_garbage_collect_worker.rb b/app/workers/git_garbage_collect_worker.rb index c95497dfaba..ec65d3ff65e 100644 --- a/app/workers/git_garbage_collect_worker.rb +++ b/app/workers/git_garbage_collect_worker.rb @@ -5,6 +5,9 @@ class GitGarbageCollectWorker sidekiq_options retry: false + # Timeout set to 24h + LEASE_TIMEOUT = 86400 + GITALY_MIGRATED_TASKS = { gc: :garbage_collect, full_repack: :repack_full, @@ -13,8 +16,19 @@ class GitGarbageCollectWorker def perform(project_id, task = :gc, lease_key = nil, lease_uuid = nil) project = Project.find(project_id) - task = task.to_sym + active_uuid = get_lease_uuid(lease_key) + + if active_uuid + return unless active_uuid == lease_uuid + + renew_lease(lease_key, active_uuid) + else + lease_uuid = try_obtain_lease(lease_key) + + return unless lease_uuid + end + task = task.to_sym cmd = command(task) repo_path = project.repository.path_to_repo description = "'#{cmd.join(' ')}' in #{repo_path}" @@ -33,11 +47,27 @@ class GitGarbageCollectWorker # Refresh the branch cache in case garbage collection caused a ref lookup to fail flush_ref_caches(project) if task == :gc ensure - Gitlab::ExclusiveLease.cancel(lease_key, lease_uuid) if lease_key.present? && lease_uuid.present? + cancel_lease(lease_key, lease_uuid) if lease_key.present? && lease_uuid.present? end private + def try_obtain_lease(key) + ::Gitlab::ExclusiveLease.new(key, timeout: LEASE_TIMEOUT).try_obtain + end + + def renew_lease(key, uuid) + ::Gitlab::ExclusiveLease.new(key, uuid: uuid, timeout: LEASE_TIMEOUT).renew + end + + def cancel_lease(key, uuid) + ::Gitlab::ExclusiveLease.cancel(key, uuid) + end + + def get_lease_uuid(key) + ::Gitlab::ExclusiveLease.get_uuid(key) + end + ## `repository` has to be a Gitlab::Git::Repository def gitaly_call(task, repository) client = Gitlab::GitalyClient::RepositoryService.new(repository) diff --git a/lib/gitlab/exclusive_lease.rb b/lib/gitlab/exclusive_lease.rb index 3784f6c4947..3f7b42456af 100644 --- a/lib/gitlab/exclusive_lease.rb +++ b/lib/gitlab/exclusive_lease.rb @@ -25,6 +25,12 @@ module Gitlab end EOS + def self.get_uuid(key) + Gitlab::Redis::SharedState.with do |redis| + redis.get(redis_shared_state_key(key)) || false + end + end + def self.cancel(key, uuid) Gitlab::Redis::SharedState.with do |redis| redis.eval(LUA_CANCEL_SCRIPT, keys: [redis_shared_state_key(key)], argv: [uuid]) @@ -35,10 +41,10 @@ module Gitlab "gitlab:exclusive_lease:#{key}" end - def initialize(key, timeout:) + def initialize(key, uuid: nil, timeout:) @redis_shared_state_key = self.class.redis_shared_state_key(key) @timeout = timeout - @uuid = SecureRandom.uuid + @uuid = uuid || SecureRandom.uuid end # Try to obtain the lease. Return lease UUID on success, diff --git a/spec/lib/gitlab/exclusive_lease_spec.rb b/spec/lib/gitlab/exclusive_lease_spec.rb index c1ed47cf64a..7322a326b01 100644 --- a/spec/lib/gitlab/exclusive_lease_spec.rb +++ b/spec/lib/gitlab/exclusive_lease_spec.rb @@ -47,6 +47,18 @@ describe Gitlab::ExclusiveLease, :clean_gitlab_redis_shared_state do end end + describe '.get_uuid' do + it 'gets the uuid if lease with the key associated exists' do + uuid = described_class.new(unique_key, timeout: 3600).try_obtain + + expect(described_class.get_uuid(unique_key)).to eq(uuid) + end + + it 'returns false if the lease does not exist' do + expect(described_class.get_uuid(unique_key)).to be false + end + end + describe '.cancel' do it 'can cancel a lease' do uuid = new_lease(unique_key) diff --git a/spec/services/projects/housekeeping_service_spec.rb b/spec/services/projects/housekeeping_service_spec.rb index 9386c110385..437c009e7fa 100644 --- a/spec/services/projects/housekeeping_service_spec.rb +++ b/spec/services/projects/housekeeping_service_spec.rb @@ -20,6 +20,7 @@ describe Projects::HousekeepingService do expect(GitGarbageCollectWorker).to receive(:perform_async).with(project.id, :the_task, :the_lease_key, :the_uuid) subject.execute + expect(project.reload.pushes_since_gc).to eq(0) end diff --git a/spec/workers/git_garbage_collect_worker_spec.rb b/spec/workers/git_garbage_collect_worker_spec.rb index c4979792194..6f9ddb6c63c 100644 --- a/spec/workers/git_garbage_collect_worker_spec.rb +++ b/spec/workers/git_garbage_collect_worker_spec.rb @@ -5,28 +5,100 @@ require 'spec_helper' describe GitGarbageCollectWorker do let(:project) { create(:project, :repository) } let(:shell) { Gitlab::Shell.new } + let!(:lease_uuid) { SecureRandom.uuid } + let!(:lease_key) { "project_housekeeping:#{project.id}" } subject { described_class.new } describe "#perform" do shared_examples 'flushing ref caches' do |gitaly| - it "flushes ref caches when the task if 'gc'" do - expect(subject).to receive(:command).with(:gc).and_return([:the, :command]) - - if gitaly - expect_any_instance_of(Gitlab::GitalyClient::RepositoryService).to receive(:garbage_collect) - .and_return(nil) - else - expect(Gitlab::Popen).to receive(:popen) - .with([:the, :command], project.repository.path_to_repo).and_return(["", 0]) + context 'with active lease_uuid' do + before do + allow(subject).to receive(:get_lease_uuid).and_return(lease_uuid) end - expect_any_instance_of(Repository).to receive(:after_create_branch).and_call_original - expect_any_instance_of(Repository).to receive(:branch_names).and_call_original - expect_any_instance_of(Gitlab::Git::Repository).to receive(:branch_count).and_call_original - expect_any_instance_of(Gitlab::Git::Repository).to receive(:has_visible_content?).and_call_original + it "flushes ref caches when the task if 'gc'" do + expect(subject).to receive(:renew_lease).with(lease_key, lease_uuid).and_call_original + expect(subject).to receive(:command).with(:gc).and_return([:the, :command]) - subject.perform(project.id) + if gitaly + expect_any_instance_of(Gitlab::GitalyClient::RepositoryService).to receive(:garbage_collect) + .and_return(nil) + else + expect(Gitlab::Popen).to receive(:popen) + .with([:the, :command], project.repository.path_to_repo).and_return(["", 0]) + end + + expect_any_instance_of(Repository).to receive(:after_create_branch).and_call_original + expect_any_instance_of(Repository).to receive(:branch_names).and_call_original + expect_any_instance_of(Gitlab::Git::Repository).to receive(:branch_count).and_call_original + expect_any_instance_of(Gitlab::Git::Repository).to receive(:has_visible_content?).and_call_original + + subject.perform(project.id, :gc, lease_key, lease_uuid) + end + end + + context 'with different lease than the active one' do + before do + allow(subject).to receive(:get_lease_uuid).and_return(SecureRandom.uuid) + end + + it 'returns silently' do + expect(subject).not_to receive(:command) + expect_any_instance_of(Repository).not_to receive(:after_create_branch).and_call_original + expect_any_instance_of(Repository).not_to receive(:branch_names).and_call_original + expect_any_instance_of(Repository).not_to receive(:branch_count).and_call_original + expect_any_instance_of(Repository).not_to receive(:has_visible_content?).and_call_original + + subject.perform(project.id, :gc, lease_key, lease_uuid) + end + end + + context 'with no active lease' do + before do + allow(subject).to receive(:get_lease_uuid).and_return(false) + end + + context 'when is able to get the lease' do + before do + allow(subject).to receive(:try_obtain_lease).and_return(SecureRandom.uuid) + end + + it "flushes ref caches when the task if 'gc'" do + expect(subject).to receive(:command).with(:gc).and_return([:the, :command]) + + if gitaly + expect_any_instance_of(Gitlab::GitalyClient::RepositoryService).to receive(:garbage_collect) + .and_return(nil) + else + expect(Gitlab::Popen).to receive(:popen) + .with([:the, :command], project.repository.path_to_repo).and_return(["", 0]) + end + + expect_any_instance_of(Repository).to receive(:after_create_branch).and_call_original + expect_any_instance_of(Repository).to receive(:branch_names).and_call_original + expect_any_instance_of(Gitlab::Git::Repository).to receive(:branch_count).and_call_original + expect_any_instance_of(Gitlab::Git::Repository).to receive(:has_visible_content?).and_call_original + + subject.perform(project.id) + end + end + + context 'when no lease can be obtained' do + before do + expect(subject).to receive(:try_obtain_lease).and_return(false) + end + + it 'returns silently' do + expect(subject).not_to receive(:command) + expect_any_instance_of(Repository).not_to receive(:after_create_branch).and_call_original + expect_any_instance_of(Repository).not_to receive(:branch_names).and_call_original + expect_any_instance_of(Repository).not_to receive(:branch_count).and_call_original + expect_any_instance_of(Repository).not_to receive(:has_visible_content?).and_call_original + + subject.perform(project.id) + end + end end end @@ -39,25 +111,34 @@ describe GitGarbageCollectWorker do end context "repack_full" do + before do + expect(subject).to receive(:get_lease_uuid).and_return(lease_uuid) + end + it "calls Gitaly" do expect_any_instance_of(Gitlab::GitalyClient::RepositoryService).to receive(:repack_full) .and_return(nil) - subject.perform(project.id, :full_repack) + subject.perform(project.id, :full_repack, lease_key, lease_uuid) end end context "repack_incremental" do + before do + expect(subject).to receive(:get_lease_uuid).and_return(lease_uuid) + end + it "calls Gitaly" do expect_any_instance_of(Gitlab::GitalyClient::RepositoryService).to receive(:repack_incremental) .and_return(nil) - subject.perform(project.id, :incremental_repack) + subject.perform(project.id, :incremental_repack, lease_key, lease_uuid) end end shared_examples 'gc tasks' do before do + allow(subject).to receive(:get_lease_uuid).and_return(lease_uuid) allow(subject).to receive(:bitmaps_enabled?).and_return(bitmaps_enabled) end @@ -67,7 +148,7 @@ describe GitGarbageCollectWorker do expect(before_packs.count).to be >= 1 - subject.perform(project.id, 'incremental_repack') + subject.perform(project.id, 'incremental_repack', lease_key, lease_uuid) after_packs = packs(project) # Exactly one new pack should have been created @@ -79,12 +160,12 @@ describe GitGarbageCollectWorker do it 'full repack consolidates into 1 packfile' do create_objects(project) - subject.perform(project.id, 'incremental_repack') + subject.perform(project.id, 'incremental_repack', lease_key, lease_uuid) before_packs = packs(project) expect(before_packs.count).to be >= 2 - subject.perform(project.id, 'full_repack') + subject.perform(project.id, 'full_repack', lease_key, lease_uuid) after_packs = packs(project) expect(after_packs.count).to eq(1) @@ -102,7 +183,7 @@ describe GitGarbageCollectWorker do expect(before_packs.count).to be >= 1 - subject.perform(project.id, 'gc') + subject.perform(project.id, 'gc', lease_key, lease_uuid) after_packed_refs = packed_refs(project) after_packs = packs(project) |