Merge branch 'release-tools/update-gitaly' into 'master'

Update Gitaly version

See merge request gitlab-org/gitlab!48984

.gitlab/ci/rails.gitlab-ci.yml

@@ -22,7 +22,7 @@
     RUBY_GC_MALLOC_LIMIT_MAX: 134217728
     CRYSTALBALL: "true"
     RECORD_DEPRECATIONS: "true"
-  needs: ["setup-test-env", "retrieve-tests-metadata", "compile-test-assets"]
+  needs: ["setup-test-env", "retrieve-tests-metadata", "compile-test-assets", "detect-tests"]
   script:
     - *base-script
     - rspec_paralellized_job "--tag ~quarantine --tag ~geo --tag ~level:migration"
@@ -64,7 +64,7 @@
     - .rspec-base
     - .as-if-foss
     - .use-pg11
-  needs: ["setup-test-env", "retrieve-tests-metadata", "compile-test-assets as-if-foss"]
+  needs: ["setup-test-env", "retrieve-tests-metadata", "compile-test-assets as-if-foss", "detect-tests"]
 
 .rspec-ee-base-pg11:
   extends:

.gitlab/ci/setup.gitlab-ci.yml

@@ -61,14 +61,16 @@ verify-tests-yml:
     - scripts/verify-tff-mapping
 
 .detect-test-base:
-  image: ruby:2.7-alpine
+  image: ruby:2.7
   needs: []
   stage: prepare
   script:
-    - source scripts/utils.sh
+    - source ./scripts/utils.sh
+    - source ./scripts/rspec_helpers.sh
     - install_gitlab_gem
     - install_tff_gem
-    - tooling/bin/find_foss_tests ${MATCHED_TESTS_FILE}
+    - retrieve_tests_mapping
+    - tooling/bin/find_tests ${MATCHED_TESTS_FILE}
     - 'echo "test files affected: $(cat $MATCHED_TESTS_FILE)"'
   artifacts:
     expire_in: 7d

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

scripts/api/get_job_id

@@ -20,6 +20,7 @@ class JobFinder
     @job_query = options.delete(:job_query)
     @pipeline_id = options.delete(:pipeline_id)
     @job_name = options.delete(:job_name)
+    @artifact_path = options.delete(:artifact_path)
 
     # Force the token to be a string so that if api_token is nil, it's set to '', allowing unauthenticated requests (for forks).
     api_token = options.delete(:api_token).to_s
@@ -33,19 +34,31 @@ class JobFinder
   end
 
   def execute
-    find_job_with_filtered_pipelines || find_job_in_pipeline
+    find_job_with_artifact || find_job_with_filtered_pipelines || find_job_in_pipeline
   end
 
   private
 
-  attr_reader :project, :pipeline_query, :job_query, :pipeline_id, :job_name
+  attr_reader :project, :pipeline_query, :job_query, :pipeline_id, :job_name, :artifact_path
+
+  def find_job_with_artifact
+    return if artifact_path.nil?
+
+    Gitlab.pipelines(project, pipeline_query_params).auto_paginate do |pipeline|
+      Gitlab.pipeline_jobs(project, pipeline.id, job_query_params).auto_paginate do |job|
+        return job if found_job_with_artifact?(job) # rubocop:disable Cop/AvoidReturnFromBlocks
+      end
+    end
+
+    raise 'Job not found!'
+  end
 
   def find_job_with_filtered_pipelines
     return if pipeline_query.empty?
 
     Gitlab.pipelines(project, pipeline_query_params).auto_paginate do |pipeline|
       Gitlab.pipeline_jobs(project, pipeline.id, job_query_params).auto_paginate do |job|
-        return job if job.name == job_name # rubocop:disable Cop/AvoidReturnFromBlocks
+        return job if found_job_by_name?(job) # rubocop:disable Cop/AvoidReturnFromBlocks
       end
     end
 
@@ -56,12 +69,22 @@ class JobFinder
     return unless pipeline_id
 
     Gitlab.pipeline_jobs(project, pipeline_id, job_query_params).auto_paginate do |job|
-      return job if job.name == job_name # rubocop:disable Cop/AvoidReturnFromBlocks
+      return job if found_job_by_name?(job) # rubocop:disable Cop/AvoidReturnFromBlocks
     end
 
     raise 'Job not found!'
   end
 
+  def found_job_with_artifact?(job)
+    artifact_url = "https://gitlab.com/api/v4/projects/#{CGI.escape(project)}/jobs/#{job.id}/artifacts/#{artifact_path}"
+    response = HTTParty.head(artifact_url) # rubocop:disable Gitlab/HTTParty
+    response.success?
+  end
+
+  def found_job_by_name?(job)
+    job.name == job_name
+  end
+
   def pipeline_query_params
     @pipeline_query_params ||= { per_page: 100, **pipeline_query }
   end
@@ -95,6 +118,10 @@ if $0 == __FILE__
       options[:job_name] = value
     end
 
+    opts.on("-a", "--artifact-path ARTIFACT_PATH", String, "A valid artifact path") do |value|
+      options[:artifact_path] = value
+    end
+
     opts.on("-t", "--api-token API_TOKEN", String, "A value API token with the `read_api` scope") do |value|
       options[:api_token] = value
     end

scripts/rspec_helpers.sh

 #!/usr/bin/env bash
 
 function retrieve_tests_metadata() {
-  mkdir -p crystalball/ knapsack/ rspec_flaky/ rspec_profiling/
+  mkdir -p knapsack/ rspec_flaky/ rspec_profiling/
 
   local project_path="gitlab-org/gitlab"
   local test_metadata_job_id
@@ -16,13 +16,6 @@ function retrieve_tests_metadata() {
   if [[ ! -f "${FLAKY_RSPEC_SUITE_REPORT_PATH}" ]]; then
     scripts/api/download_job_artifact --project "${project_path}" --job-id "${test_metadata_job_id}" --artifact-path "${FLAKY_RSPEC_SUITE_REPORT_PATH}" || echo "{}" > "${FLAKY_RSPEC_SUITE_REPORT_PATH}"
   fi
-
-  # FIXME: We will need to find a pipeline where the $RSPEC_PACKED_TESTS_MAPPING_PATH.gz actually exists (Crystalball only runs every two-hours, but the `update-tests-metadata` runs for all `master` pipelines...).
-  # if [[ ! -f "${RSPEC_PACKED_TESTS_MAPPING_PATH}" ]]; then
-  #   (scripts/api/download_job_artifact --project "${project_path}" --job-id "${test_metadata_job_id}" --artifact-path "${RSPEC_PACKED_TESTS_MAPPING_PATH}.gz" && gzip -d "${RSPEC_PACKED_TESTS_MAPPING_PATH}.gz") || echo "{}" > "${RSPEC_PACKED_TESTS_MAPPING_PATH}"
-  # fi
-  #
-  # scripts/unpack-test-mapping "${RSPEC_PACKED_TESTS_MAPPING_PATH}" "${RSPEC_TESTS_MAPPING_PATH}"
 }
 
 function update_tests_metadata() {
@@ -43,6 +36,21 @@ function update_tests_metadata() {
   fi
 }
 
+function retrieve_tests_mapping() {
+  mkdir -p crystalball/
+
+  local project_path="gitlab-org/gitlab"
+  local test_metadata_with_mapping_job_id
+
+  test_metadata_with_mapping_job_id=$(scripts/api/get_job_id --project "${project_path}" -q "status=success" -q "ref=master" -q "username=gitlab-bot" -Q "scope=success" --job-name "update-tests-metadata" --artifact-path "${RSPEC_PACKED_TESTS_MAPPING_PATH}.gz")
+
+  if [[ ! -f "${RSPEC_PACKED_TESTS_MAPPING_PATH}" ]]; then
+   (scripts/api/download_job_artifact --project "${project_path}" --job-id "${test_metadata_with_mapping_job_id}" --artifact-path "${RSPEC_PACKED_TESTS_MAPPING_PATH}.gz" && gzip -d "${RSPEC_PACKED_TESTS_MAPPING_PATH}.gz") || echo "{}" > "${RSPEC_PACKED_TESTS_MAPPING_PATH}"
+  fi
+
+  scripts/unpack-test-mapping "${RSPEC_PACKED_TESTS_MAPPING_PATH}" "${RSPEC_TESTS_MAPPING_PATH}"
+}
+
 function update_tests_mapping() {
   if ! crystalball_rspec_data_exists; then
     echo "No crystalball rspec data found."
@@ -119,8 +127,8 @@ function rspec_paralellized_job() {
 
   local rspec_args="-Ispec -rspec_helper --color --format documentation --format RspecJunitFormatter --out junit_rspec.xml ${rspec_opts}"
 
-  if [[ -n $RSPEC_MATCHING_TESTS_ENABLED ]]; then
-    tooling/bin/parallel_rspec --rspec_args "${rspec_args}" --filter tmp/matching_tests.txt
+  if [[ -n $RSPEC_TESTS_MAPPING_ENABLED ]]; then
+    tooling/bin/parallel_rspec --rspec_args "${rspec_args}" --filter "tmp/matching_tests.txt"
   else
     tooling/bin/parallel_rspec --rspec_args "${rspec_args}"
   fi

scripts/utils.sh

@@ -36,7 +36,7 @@ function install_gitlab_gem() {
 }
 
 function install_tff_gem() {
-  gem install test_file_finder --version 0.1.0
+  gem install test_file_finder --version 0.1.1
 }
 
 function run_timed_command() {

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

tooling/bin/find_foss_tests → tooling/bin/find_tests

@@ -19,7 +19,12 @@ mr_iid = ENV.fetch('CI_MERGE_REQUEST_IID')
 mr_changes = Gitlab.merge_request_changes(mr_project_path, mr_iid)
 changed_files = mr_changes.changes.map { |change| change['new_path'] }
 
-mapping = TestFileFinder::Mapping.load('tests.yml')
-test_files = TestFileFinder::FileFinder.new(paths: changed_files, mapping: mapping).test_files
+tff = TestFileFinder::FileFinder.new(paths: changed_files).tap do |file_finder|
+  file_finder.use TestFileFinder::MappingStrategies::PatternMatching.load('tests.yml')
 
-File.write(output_file, test_files.uniq.join(' '))
+  if ENV['RSPEC_TESTS_MAPPING_ENABLED']
+    file_finder.use TestFileFinder::MappingStrategies::DirectMatching.load_json(ENV['RSPEC_TESTS_MAPPING_PATH'])
+  end
+end
+
+File.write(output_file, tff.test_files.uniq.join(' '))