Skip to content
Snippets Groups Projects
Commit 20d47111 authored by Vasilii Iakliushin's avatar Vasilii Iakliushin :two:
Browse files

Verify correctness of ProtectedBranch cache

* Contributes to #366724
* Follow-up for:
!92922
* Feature flag: #368279

**Problem**

Previous MR added a new implementation of ProtectedBranch cache. We want
to verify if it works correctly.

**Solution**

* Add `dry_run` option to ProtectedBranch cache
* Add feature flag to control cache flow

New cache implementation with `dry_run: true` will create new cache
structures but it will still return non-cached values to the user. If we
have an cache inconsistency then we will log an error message.
parent 26f76df7
No related branches found
No related tags found
No related merge requests found
......@@ -4,6 +4,7 @@ class ProtectedBranch < ApplicationRecord
include ProtectedRef
include Gitlab::SQL::Pattern
# Deprecated: https://gitlab.com/gitlab-org/gitlab/-/issues/368279
CACHE_EXPIRE_IN = 1.hour
scope :requiring_code_owner_approval,
......@@ -31,11 +32,25 @@ def self.protected?(project, ref_name)
return true if project.empty_repo? && project.default_branch_protected?
return false if ref_name.blank?
new_cache(project, ref_name) || deprecated_cache(project, ref_name)
end
def self.new_cache(project, ref_name)
if Feature.enabled?(:hash_based_cache_for_protected_branches, project)
ProtectedBranches::CacheService.new(project).fetch(ref_name, dry_run: true) do # rubocop: disable CodeReuse/ServiceClass
self.matching(ref_name, protected_refs: protected_refs(project)).present?
end
end
end
# Deprecated: https://gitlab.com/gitlab-org/gitlab/-/issues/368279
def self.deprecated_cache(project, ref_name)
Rails.cache.fetch(protected_ref_cache_key(project, ref_name), expires_in: CACHE_EXPIRE_IN) do
self.matching(ref_name, protected_refs: protected_refs(project)).present?
end
end
# Deprecated: https://gitlab.com/gitlab-org/gitlab/-/issues/368279
def self.protected_ref_cache_key(project, ref_name)
"protected_ref-#{project.cache_key}-#{Digest::SHA1.hexdigest(ref_name)}"
end
......
......@@ -7,16 +7,29 @@ class CacheService < ProtectedBranches::BaseService
CACHE_EXPIRE_IN = 1.day
CACHE_LIMIT = 1000
def fetch(ref_name)
def fetch(ref_name, dry_run: false)
record = OpenSSL::Digest::SHA256.hexdigest(ref_name)
Gitlab::Redis::Cache.with do |redis|
cached_result = redis.hget(redis_key, record)
break Gitlab::Redis::Boolean.decode(cached_result) unless cached_result.nil?
decoded_result = Gitlab::Redis::Boolean.decode(cached_result) unless cached_result.nil?
break decoded_result unless dry_run || decoded_result.nil?
value = yield
if dry_run && !decoded_result.nil? && decoded_result != value
encoded_ref_name = Gitlab::EncodingHelper.encode_utf8_with_replacement_character(ref_name)
log_error(
'class' => self.class.name,
'message' => "Cache mismatch '#{encoded_ref_name}': cached value: #{decoded_result}, real value: #{value}",
'project_id' => @project.id,
'project_path' => @project.full_path
)
end
redis.hset(redis_key, record, Gitlab::Redis::Boolean.encode(value))
# We don't want to extend cache expiration time
......
---
name: hash_based_cache_for_protected_branches
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/92934
rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/368279
milestone: '15.3'
type: development
group: group::source code
default_enabled: false
......@@ -171,7 +171,10 @@
let_it_be(:project) { create(:project, :repository) }
let_it_be(:protected_branch) { create(:protected_branch, project: project, name: "“jawn”") }
let(:feature_flag) { true }
before do
stub_feature_flags(hash_based_cache_for_protected_branches: feature_flag)
allow(described_class).to receive(:matching).with(protected_branch.name, protected_refs: anything).once.and_call_original
# the original call works and warms the cache
......@@ -179,24 +182,80 @@
end
it 'correctly invalidates a cache' do
expect(described_class).to receive(:matching).with(protected_branch.name, protected_refs: anything).once.and_call_original
expect(described_class).to receive(:matching).with(protected_branch.name, protected_refs: anything).exactly(3).times.and_call_original
create(:protected_branch, project: project, name: "bar")
# the cache is invalidated because the project has been "updated"
expect(described_class.protected?(project, protected_branch.name)).to eq(true)
branch = ProtectedBranches::CreateService.new(project, project.owner, name: 'bar').execute
described_class.protected?(project, protected_branch.name)
ProtectedBranches::UpdateService.new(project, project.owner, name: 'ber').execute(branch)
described_class.protected?(project, protected_branch.name)
ProtectedBranches::DestroyService.new(project, project.owner).execute(branch)
described_class.protected?(project, protected_branch.name)
end
it 'correctly uses the cached version' do
context 'when project is updated' do
# Expected to fail, because of fetch(dry_run: true)
xit 'does not invalidate a cache' do
expect(described_class).not_to receive(:matching).with(protected_branch.name, protected_refs: anything)
project.touch
described_class.protected?(project, protected_branch.name)
end
end
context 'when other project protected branch is updated' do
# Expected to fail, because of fetch(dry_run: true)
xit 'does not invalidate the current project cache' do
expect(described_class).not_to receive(:matching).with(protected_branch.name, protected_refs: anything)
another_project = create(:project)
ProtectedBranches::CreateService.new(another_project, another_project.owner, name: 'bar').execute
described_class.protected?(project, protected_branch.name)
end
end
# Expected to fail, because of fetch(dry_run: true)
xit 'correctly uses the cached version' do
expect(described_class).not_to receive(:matching)
expect(described_class.protected?(project, protected_branch.name)).to eq(true)
end
it 'sets expires_in for a cache key' do
cache_key = described_class.protected_ref_cache_key(project, protected_branch.name)
context 'when feature flag is off' do
let(:feature_flag) { false }
expect(Rails.cache).to receive(:fetch).with(cache_key, expires_in: 1.hour)
it 'correctly invalidates a cache' do
expect(described_class).to receive(:matching).with(protected_branch.name, protected_refs: anything).once.and_call_original
described_class.protected?(project, protected_branch.name)
create(:protected_branch, project: project, name: "bar")
# the cache is invalidated because the project has been "updated"
expect(described_class.protected?(project, protected_branch.name)).to eq(true)
end
it 'sets expires_in for a cache key' do
cache_key = described_class.protected_ref_cache_key(project, protected_branch.name)
expect(Rails.cache).to receive(:fetch).with(cache_key, expires_in: 1.hour)
described_class.protected?(project, protected_branch.name)
end
context 'when project is updated' do
it 'also invalidates a cache' do
expect(described_class).to receive(:matching).with(protected_branch.name, protected_refs: anything).once.and_call_original
project.touch
described_class.protected?(project, protected_branch.name)
end
end
it 'correctly uses the cached version' do
expect(described_class).not_to receive(:matching)
expect(described_class.protected?(project, protected_branch.name)).to eq(true)
end
end
end
end
......
......@@ -63,6 +63,38 @@
expect(service.fetch('not-found') { true }).to eq(true)
end
end
context 'when dry_run is on' do
it 'does not use cached value' do
expect(service.fetch('main', dry_run: true) { true }).to eq(true)
expect(service.fetch('main', dry_run: true) { false }).to eq(false)
end
context 'when cache mismatch' do
it 'logs an error' do
expect(service.fetch('main', dry_run: true) { true }).to eq(true)
expect(Gitlab::AppLogger).to receive(:error).with(
'class' => described_class.name,
'message' => /Cache mismatch/,
'project_id' => project.id,
'project_path' => project.full_path
)
expect(service.fetch('main', dry_run: true) { false }).to eq(false)
end
end
context 'when cache matches' do
it 'does not log an error' do
expect(service.fetch('main', dry_run: true) { true }).to eq(true)
expect(Gitlab::AppLogger).not_to receive(:error)
expect(service.fetch('main', dry_run: true) { true }).to eq(true)
end
end
end
end
describe '#refresh' do
......
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment