Commit 9f6ae914 authored by Kamil Trzciński's avatar Kamil Trzciński

Merge branch '282464-introduce-exclusive-lease-for-pages-deployments' into 'master'

Introduce exclusive lease for pages deployments

See merge request gitlab-org/gitlab!48349
parents 3f803928 20dbc7cf
......@@ -21,7 +21,7 @@ module ExclusiveLeaseGuard
lease = exclusive_lease.try_obtain
unless lease
log_error("Cannot obtain an exclusive lease for #{self.class.name}. There must be another instance already in execution.")
log_error("Cannot obtain an exclusive lease for #{lease_key}. There must be another instance already in execution.")
return
end
......
# frozen_string_literal: true
module Pages
module LegacyStorageLease
extend ActiveSupport::Concern
include ::ExclusiveLeaseGuard
LEASE_TIMEOUT = 1.hour
# override method from exclusive lease guard to guard it by feature flag
# TODO: just remove this method after testing this in production
# https://gitlab.com/gitlab-org/gitlab/-/issues/282464
def try_obtain_lease
return yield unless Feature.enabled?(:pages_use_legacy_storage_lease, project)
super
end
def lease_key
"pages_legacy_storage:#{project.id}"
end
def lease_timeout
LEASE_TIMEOUT
end
end
end
......@@ -4,6 +4,9 @@ module Projects
class UpdatePagesService < BaseService
InvalidStateError = Class.new(StandardError)
FailedToExtractError = Class.new(StandardError)
ExclusiveLeaseTaken = Class.new(StandardError)
include ::Pages::LegacyStorageLease
BLOCK_SIZE = 32.kilobytes
PUBLIC_DIR = 'public'
......@@ -109,6 +112,17 @@ module Projects
end
def deploy_page!(archive_public_path)
deployed = try_obtain_lease do
deploy_page_unsafe!(archive_public_path)
true
end
unless deployed
raise ExclusiveLeaseTaken, "Failed to deploy pages - other deployment is in progress"
end
end
def deploy_page_unsafe!(archive_public_path)
# Do atomic move of pages
# Move and removal may not be atomic, but they are significantly faster then extracting and removal
# 1. We move deployed public to previous public path (file removal is slow)
......
---
name: pages_use_legacy_storage_lease
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/48349
rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/282464
milestone: '13.7'
type: development
group: group::release
default_enabled: false
......@@ -51,7 +51,7 @@ RSpec.describe ExclusiveLeaseGuard, :clean_gitlab_redis_shared_state do
it 'does not call internal_method but logs error', :aggregate_failures do
expect(subject).not_to receive(:internal_method)
expect(Gitlab::AppLogger).to receive(:error).with("Cannot obtain an exclusive lease for #{subject.class.name}. There must be another instance already in execution.")
expect(Gitlab::AppLogger).to receive(:error).with("Cannot obtain an exclusive lease for #{subject.lease_key}. There must be another instance already in execution.")
subject.call
end
......
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe ::Pages::LegacyStorageLease do
let(:project) { create(:project) }
let(:implementation) do
Class.new do
include ::Pages::LegacyStorageLease
attr_reader :project
def initialize(project)
@project = project
end
def execute
try_obtain_lease do
execute_unsafe
end
end
def execute_unsafe
true
end
end
end
let(:service) { implementation.new(project) }
it 'allows method to be executed' do
expect(service).to receive(:execute_unsafe).and_call_original
expect(service.execute).to eq(true)
end
context 'when another service holds the lease for the same project' do
around do |example|
implementation.new(project).try_obtain_lease do
example.run
end
end
it 'does not run guarded method' do
expect(service).not_to receive(:execute_unsafe)
expect(service.execute).to eq(nil)
end
it 'runs guarded method if feature flag is disabled' do
stub_feature_flags(pages_use_legacy_storage_lease: false)
expect(service).to receive(:execute_unsafe).and_call_original
expect(service.execute).to eq(true)
end
end
context 'when another service holds the lease for the different project' do
around do |example|
implementation.new(create(:project)).try_obtain_lease do
example.run
end
end
it 'allows method to be executed' do
expect(service).to receive(:execute_unsafe).and_call_original
expect(service.execute).to eq(true)
end
end
end
......@@ -139,7 +139,7 @@ RSpec.describe Projects::GitDeduplicationService do
end
it 'fails when a lease is already out' do
expect(service).to receive(:log_error).with("Cannot obtain an exclusive lease for #{service.class.name}. There must be another instance already in execution.")
expect(service).to receive(:log_error).with("Cannot obtain an exclusive lease for #{lease_key}. There must be another instance already in execution.")
service.execute
end
......
......@@ -69,6 +69,16 @@ RSpec.describe Projects::UpdatePagesService do
expect(project.pages_metadatum.reload.pages_deployment_id).to eq(deployment.id)
end
it 'fails if another deployment is in progress' do
subject.try_obtain_lease do
expect do
execute
end.to raise_error("Failed to deploy pages - other deployment is in progress")
expect(GenericCommitStatus.last.description).to eq("Failed to deploy pages - other deployment is in progress")
end
end
it 'does not fail if pages_metadata is absent' do
project.pages_metadatum.destroy!
project.reload
......
......@@ -32,7 +32,7 @@ RSpec.describe RepositoryRemoveRemoteWorker do
expect(subject)
.to receive(:log_error)
.with("Cannot obtain an exclusive lease for #{subject.class.name}. There must be another instance already in execution.")
.with("Cannot obtain an exclusive lease for #{lease_key}. There must be another instance already in execution.")
subject.perform(project.id, remote_name)
end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment