Automatic merge of gitlab-org/gitlab master

GITALY_SERVER_VERSION

-cb5c003584e180415fe33d1712e0d5aee2a22b0c
+a5a5d83630f13c3eb3e1650a24423fc5e9bc47d2

app/helpers/notifications_helper.rb

@@ -106,7 +106,8 @@ module NotificationsHelper
     when :success_pipeline
       s_('NotificationEvent|Successful pipeline')
     else
-      s_(event.to_s.humanize)
+      event_name = "NotificationEvent|#{event.to_s.humanize}"
+      s_(event_name)
     end
   end
 

app/models/audit_event.rb

@@ -13,6 +13,8 @@ class AuditEvent < ApplicationRecord
     :target_id
   ].freeze
 
+  self.primary_key = :id
+
   serialize :details, Hash # rubocop:disable Cop/ActiveRecordSerialize
 
   belongs_to :user, foreign_key: :author_id

app/models/notification_setting.rb

@@ -30,6 +30,7 @@ class NotificationSetting < ApplicationRecord
 
   scope :preload_source_route, -> { preload(source: [:route]) }
 
+  # NOTE: Applicable unfound_translations.rb also needs to be updated when below events are changed.
   EMAIL_EVENTS = [
     :new_release,
     :new_note,
@@ -51,7 +52,6 @@ class NotificationSetting < ApplicationRecord
     :moved_project
   ].freeze
 
-  # Update unfound_translations.rb when events are changed
   def self.email_events(source = nil)
     EMAIL_EVENTS
   end

app/services/concerns/exclusive_lease_guard.rb

@@ -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
 

app/services/pages/legacy_storage_lease.rb

+# 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

app/services/projects/update_pages_service.rb

@@ -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)

config/feature_flags/development/pages_use_legacy_storage_lease.yml

+---
+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

locale/gitlab.pot

@@ -18859,9 +18859,15 @@ msgstr ""
 msgid "NotificationEvent|Fixed pipeline"
 msgstr ""
 
+msgid "NotificationEvent|Issue due"
+msgstr ""
+
 msgid "NotificationEvent|Merge merge request"
 msgstr ""
 
+msgid "NotificationEvent|Moved project"
+msgstr ""
+
 msgid "NotificationEvent|New epic"
 msgstr ""
 
@@ -18877,6 +18883,9 @@ msgstr ""
 msgid "NotificationEvent|New release"
 msgstr ""
 
+msgid "NotificationEvent|Push to merge request"
+msgstr ""
+
 msgid "NotificationEvent|Reassign issue"
 msgstr ""
 
@@ -18886,6 +18895,9 @@ msgstr ""
 msgid "NotificationEvent|Reopen issue"
 msgstr ""
 
+msgid "NotificationEvent|Reopen merge request"
+msgstr ""
+
 msgid "NotificationEvent|Successful pipeline"
 msgstr ""
 

locale/unfound_translations.rb

@@ -4,16 +4,20 @@
 # https://github.com/grosser/gettext_i18n_rails#unfound-translations-with-rake-gettextfind
 
 # NotificationSetting.email_events
+N_('NotificationEvent|New release')
 N_('NotificationEvent|New note')
 N_('NotificationEvent|New issue')
 N_('NotificationEvent|Reopen issue')
 N_('NotificationEvent|Close issue')
 N_('NotificationEvent|Reassign issue')
+N_('NotificationEvent|Issue due')
 N_('NotificationEvent|New merge request')
+N_('NotificationEvent|Push to merge request')
+N_('NotificationEvent|Reopen merge request')
 N_('NotificationEvent|Close merge request')
 N_('NotificationEvent|Reassign merge request')
+N_('NotificationEvent|Change reviewer merge request')
 N_('NotificationEvent|Merge merge request')
 N_('NotificationEvent|Failed pipeline')
 N_('NotificationEvent|Fixed pipeline')
-N_('NotificationEvent|New release')
-N_('NotificationEvent|Change reviewer merge request')
+N_('NotificationEvent|Moved project')

spec/helpers/notifications_helper_spec.rb

@@ -20,10 +20,19 @@ RSpec.describe NotificationsHelper do
   end
 
   describe '#notification_event_name' do
-    it { expect(notification_event_name(:success_pipeline)).to match('Successful pipeline') }
-    it { expect(notification_event_name(:failed_pipeline)).to match('Failed pipeline') }
-    it { expect(notification_event_name(:fixed_pipeline)).to match('Fixed pipeline') }
-    it { expect(notification_event_name(:moved_project)).to match('Moved project') }
+    context 'for success_pipeline' do
+      it 'returns the custom name' do
+        expect(FastGettext).to receive(:cached_find).with('NotificationEvent|Successful pipeline')
+        expect(notification_event_name(:success_pipeline)).to eq('Successful pipeline')
+      end
+    end
+
+    context 'for everything else' do
+      it 'returns a humanized name' do
+        expect(FastGettext).to receive(:cached_find).with('NotificationEvent|Failed pipeline')
+        expect(notification_event_name(:failed_pipeline)).to eq('Failed pipeline')
+      end
+    end
   end
 
   describe '#notification_icon_level' do

spec/services/concerns/exclusive_lease_guard_spec.rb

@@ -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

spec/services/pages/legacy_storage_lease_spec.rb

+# 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

spec/services/projects/git_deduplication_service_spec.rb

@@ -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

spec/services/projects/update_pages_service_spec.rb

@@ -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

spec/workers/repository_remove_remote_worker_spec.rb

@@ -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