Merge branch 'feature/gb/replication-lag-resilient-build-traces' into 'master'

Make cloud native build logs resilient to replication lag

See merge request gitlab-org/gitlab!46976

app/models/ci/build_trace_chunk.rb

@@ -76,6 +76,22 @@ module Ci
         end
       end
 
+      ##
+      # Sometime we need to ensure that the first read goes to a primary
+      # database, what is especially important in EE. This method does not
+      # change the behavior in CE.
+      #
+      def with_read_consistency(build, &block)
+        return yield unless consistent_reads_enabled?(build)
+
+        ::Gitlab::Database::Consistency
+          .with_read_consistency(&block)
+      end
+
+      def consistent_reads_enabled?(build)
+        Feature.enabled?(:gitlab_ci_trace_read_consistency, build.project, type: :development, default_enabled: false)
+      end
+
       ##
       # Sometimes we do not want to read raw data. This method makes it easier
       # to find attributes that are just metadata excluding raw data.
@@ -154,8 +170,8 @@ module Ci
       in_lock(lock_key, **lock_params) do # exclusive Redis lock is acquired first
         raise FailedToPersistDataError, 'Modifed build trace chunk detected' if has_changes_to_save?
 
-        self.reset.then do |chunk|     # we ensure having latest lock_version
-          chunk.unsafe_persist_data!   # we migrate the data and update data store
+        self.class.with_read_consistency(build) do
+          self.reset.then { |chunk| chunk.unsafe_persist_data! }
         end
       end
     rescue FailedToObtainLockError

config/feature_flags/development/gitlab_ci_trace_read_consistency.yml

+---
+name: gitlab_ci_trace_read_consistency
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/46976
+rollout_issue_url: 
+milestone: '13.9'
+type: development
+group: group::continuous integration
+default_enabled: false

ee/lib/ee/gitlab/database/consistency.rb

+# frozen_string_literal: true
+
+module EE
+  module Gitlab
+    module Database
+      module Consistency
+        extend ::Gitlab::Utils::Override
+        ##
+        # In EE we are disabling the database load balancing for calls that
+        # require read consistency after recent writes.
+        #
+        override :with_read_consistency
+        def with_read_consistency(&block)
+          ::Gitlab::Database::LoadBalancing::Session
+            .current.use_primary(&block)
+        end
+      end
+    end
+  end
+end

ee/lib/gitlab/database/load_balancing/session.rb

@@ -28,6 +28,8 @@ module Gitlab
           @use_primary
         end
 
+        alias_method :using_primary?, :use_primary?
+
         def use_primary!
           @use_primary = true
         end

ee/spec/lib/gitlab/database/consistency_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Database::Consistency do
+  let(:session) do
+    Gitlab::Database::LoadBalancing::Session.current
+  end
+
+  describe '.with_read_consistency' do
+    it 'sticks to primary database' do
+      expect(session).not_to be_using_primary
+
+      block = -> (&control) do
+        described_class.with_read_consistency do
+          expect(session).to be_using_primary
+
+          control.call
+        end
+      end
+
+      expect { |probe| block.call(&probe) }.to yield_control
+    end
+  end
+end

lib/gitlab/ci/trace/checksum.rb

@@ -30,7 +30,11 @@ module Gitlab
         end
 
         def state_crc32
-          strong_memoize(:state_crc32) { build.pending_state&.crc32 }
+          strong_memoize(:state_crc32) do
+            ::Gitlab::Database::Consistency.with_read_consistency do
+              build.pending_state&.crc32
+            end
+          end
         end
 
         def chunks_crc32
@@ -59,8 +63,10 @@ module Gitlab
         #
         def trace_chunks
           strong_memoize(:trace_chunks) do
-            build.trace_chunks.persisted
-              .select(::Ci::BuildTraceChunk.metadata_attributes)
+            ::Ci::BuildTraceChunk.with_read_consistency(build) do
+              build.trace_chunks.persisted
+                .select(::Ci::BuildTraceChunk.metadata_attributes)
+            end
           end
         end
 

lib/gitlab/ci/trace/chunked_io.rb

@@ -227,12 +227,20 @@ module Gitlab
         end
         # rubocop: enable CodeReuse/ActiveRecord
 
-        def build_chunk
-          @chunks_cache[chunk_index] = ::Ci::BuildTraceChunk.new(build: build, chunk_index: chunk_index)
+        def next_chunk
+          @chunks_cache[chunk_index] = begin
+            if ::Ci::BuildTraceChunk.consistent_reads_enabled?(build)
+              ::Ci::BuildTraceChunk
+                .safe_find_or_create_by(build: build, chunk_index: chunk_index)
+            else
+              ::Ci::BuildTraceChunk
+                .new(build: build, chunk_index: chunk_index)
+            end
+          end
         end
 
         def ensure_chunk
-          current_chunk || build_chunk
+          current_chunk || next_chunk || current_chunk
         end
 
         # rubocop: disable CodeReuse/ActiveRecord

lib/gitlab/database/consistency.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Database
+    ##
+    # This class is used to make it possible to ensure read consistency in
+    # GitLab EE without the need of overriding a lot of methods / classes /
+    # classs.
+    #
+    # This is a CE class that does nothing in CE, because database load
+    # balancing is EE-only feature, but you can still use it in CE. It will
+    # start ensuring read consistency once it is overridden in EE.
+    #
+    # Using this class in CE helps to avoid creeping discrepancy between CE /
+    # EE only to force usage of the primary database in EE.
+    #
+    class Consistency
+      ##
+      # In CE there is no database load balancing, so all reads are expected to
+      # be consistent by the ACID guarantees of a single PostgreSQL instance.
+      #
+      # This method is overridden in EE.
+      #
+      def self.with_read_consistency(&block)
+        yield
+      end
+    end
+  end
+end
+
+::Gitlab::Database::Consistency.singleton_class.prepend_if_ee('EE::Gitlab::Database::Consistency')

spec/lib/gitlab/ci/trace/chunked_io_spec.rb

@@ -9,7 +9,7 @@ RSpec.describe Gitlab::Ci::Trace::ChunkedIO, :clean_gitlab_redis_cache do
   let(:chunked_io) { described_class.new(build) }
 
   before do
-    stub_feature_flags(ci_enable_live_trace: true)
+    stub_feature_flags(ci_enable_live_trace: true, gitlab_ci_trace_read_consistency: true)
   end
 
   describe "#initialize" do

spec/models/ci/build_trace_chunk_spec.rb

@@ -17,7 +17,7 @@ RSpec.describe Ci::BuildTraceChunk, :clean_gitlab_redis_shared_state do
   it_behaves_like 'having unique enum values'
 
   before do
-    stub_feature_flags(ci_enable_live_trace: true)
+    stub_feature_flags(ci_enable_live_trace: true, gitlab_ci_trace_read_consistency: true)
     stub_artifacts_object_storage
   end