Commit 5d9b8b23 authored by Reuben Pereira's avatar Reuben Pereira Committed by Thong Kuah

Add exclusive guard to ClusterUpdateAppWorker

Due to https://gitlab.com/gitlab-org/gitlab/issues/196243, the
ClusterUpdateAppWorker should never be run multiple times concurrently
for the same application.
parent 6ae01f77
---
title: Fix race condition bug in Prometheus managed app update process
merge_request: 24228
author:
type: fixed
...@@ -6,11 +6,24 @@ class ClusterUpdateAppWorker ...@@ -6,11 +6,24 @@ class ClusterUpdateAppWorker
include ApplicationWorker include ApplicationWorker
include ClusterQueue include ClusterQueue
include ClusterApplications include ClusterApplications
include ExclusiveLeaseGuard
sidekiq_options retry: 3, dead: false sidekiq_options retry: 3, dead: false
# rubocop: disable CodeReuse/ActiveRecord LEASE_TIMEOUT = 10.minutes.to_i
def perform(app_name, app_id, project_id, scheduled_time) def perform(app_name, app_id, project_id, scheduled_time)
@app_id = app_id
try_obtain_lease do
execute(app_name, app_id, project_id, scheduled_time)
end
end
private
# rubocop: disable CodeReuse/ActiveRecord
def execute(app_name, app_id, project_id, scheduled_time)
project = Project.find_by(id: project_id) project = Project.find_by(id: project_id)
return unless project return unless project
...@@ -22,4 +35,12 @@ class ClusterUpdateAppWorker ...@@ -22,4 +35,12 @@ class ClusterUpdateAppWorker
end end
end end
# rubocop: enable CodeReuse/ActiveRecord # rubocop: enable CodeReuse/ActiveRecord
def lease_key
@lease_key ||= "#{self.class.name.underscore}-#{@app_id}"
end
def lease_timeout
LEASE_TIMEOUT
end
end end
...@@ -3,7 +3,10 @@ ...@@ -3,7 +3,10 @@
require 'spec_helper' require 'spec_helper'
describe ClusterUpdateAppWorker do describe ClusterUpdateAppWorker do
let(:project) { create(:project) } include ExclusiveLeaseHelpers
let_it_be(:project) { create(:project) }
let(:prometheus_update_service) { spy } let(:prometheus_update_service) { spy }
subject { described_class.new } subject { described_class.new }
...@@ -42,5 +45,54 @@ describe ClusterUpdateAppWorker do ...@@ -42,5 +45,54 @@ describe ClusterUpdateAppWorker do
subject.perform(application.name, application.id, project.id, Time.now) subject.perform(application.name, application.id, project.id, Time.now)
end end
context 'with exclusive lease' do
let(:application) { create(:clusters_applications_prometheus, :installed) }
let(:lease_key) { "#{described_class.name.underscore}-#{application.id}" }
before do
allow(Gitlab::ExclusiveLease).to receive(:new)
stub_exclusive_lease_taken(lease_key)
end
it 'does not allow same app to be updated concurrently by same project' do
expect(Clusters::Applications::PrometheusUpdateService).not_to receive(:new)
subject.perform(application.name, application.id, project.id, Time.now)
end
it 'does not allow same app to be updated concurrently by different project' do
project1 = create(:project)
expect(Clusters::Applications::PrometheusUpdateService).not_to receive(:new)
subject.perform(application.name, application.id, project1.id, Time.now)
end
it 'allows different app to be updated concurrently by same project' do
application2 = create(:clusters_applications_prometheus, :installed)
lease_key2 = "#{described_class.name.underscore}-#{application2.id}"
stub_exclusive_lease(lease_key2)
expect(Clusters::Applications::PrometheusUpdateService).to receive(:new)
.with(application2, project)
subject.perform(application2.name, application2.id, project.id, Time.now)
end
it 'allows different app to be updated by different project' do
application2 = create(:clusters_applications_prometheus, :installed)
lease_key2 = "#{described_class.name.underscore}-#{application2.id}"
project2 = create(:project)
stub_exclusive_lease(lease_key2)
expect(Clusters::Applications::PrometheusUpdateService).to receive(:new)
.with(application2, project2)
subject.perform(application2.name, application2.id, project2.id, Time.now)
end
end
end end
end 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