diff --git a/app/models/ci/build_trace_chunk.rb b/app/models/ci/build_trace_chunk.rb index 9e7ebf41ee8..a8f43fd549f 100644 --- a/app/models/ci/build_trace_chunk.rb +++ b/app/models/ci/build_trace_chunk.rb @@ -24,7 +24,7 @@ module Ci ## # Data is memoized for optimizing #size and #end_offset def data - @data ||= get_data + @data ||= get_data.to_s end def truncate(offset = 0) @@ -32,11 +32,10 @@ module Ci end def append(new_data, offset) - current_data = self.data.to_s - raise ArgumentError, 'Offset is out of bound' if offset > current_data.bytesize || offset < 0 - raise ArgumentError, 'Outside of chunk size' if CHUNK_SIZE < offset + new_data.bytesize + raise ArgumentError, 'Offset is out of range' if offset > data.bytesize || offset < 0 + raise ArgumentError, 'Chunk size overflow' if CHUNK_SIZE < (offset + new_data.bytesize) - set_data(current_data.byteslice(0, offset) + new_data) + set_data(data.byteslice(0, offset) + new_data) end def size diff --git a/lib/gitlab/ci/trace/stream.rb b/lib/gitlab/ci/trace/stream.rb index 89e36ecbb15..a71040e5e56 100644 --- a/lib/gitlab/ci/trace/stream.rb +++ b/lib/gitlab/ci/trace/stream.rb @@ -52,6 +52,7 @@ module Gitlab stream.seek(0, IO::SEEK_SET) stream.write(data) + stream.truncate(data.bytesize) stream.flush() end diff --git a/spec/factories/ci/build_trace_chunks.rb b/spec/factories/ci/build_trace_chunks.rb index be13a84a47c..c0b9a25bfe8 100644 --- a/spec/factories/ci/build_trace_chunks.rb +++ b/spec/factories/ci/build_trace_chunks.rb @@ -1,6 +1,6 @@ FactoryBot.define do factory :ci_build_trace_chunk, class: Ci::BuildTraceChunk do - job factory: :ci_build + build factory: :ci_build chunk_index 0 data_store :redis end diff --git a/spec/models/ci/build_trace_chunk_spec.rb b/spec/models/ci/build_trace_chunk_spec.rb index 81e58eca3ed..a122ee84b3c 100644 --- a/spec/models/ci/build_trace_chunk_spec.rb +++ b/spec/models/ci/build_trace_chunk_spec.rb @@ -46,7 +46,7 @@ describe Ci::BuildTraceChunk, :clean_gitlab_redis_shared_state do end describe '#set_data' do - subject { build_trace_chunk.set_data(value) } + subject { build_trace_chunk.send(:set_data, value) } let(:value) { 'Sample data' } @@ -131,13 +131,17 @@ describe Ci::BuildTraceChunk, :clean_gitlab_redis_shared_state do context 'when offset is negative' do let(:offset) { -1 } - it { expect { subject }.to raise_error('Offset is out of bound') } + it { expect { subject }.to raise_error('Offset is out of range') } end context 'when offset is bigger than data size' do let(:offset) { data.bytesize + 1 } - it { expect { subject }.to raise_error('Offset is out of bound') } + it do + expect_any_instance_of(described_class).not_to receive(:append) { } + + subject + end end context 'when offset is 10' do @@ -182,19 +186,19 @@ describe Ci::BuildTraceChunk, :clean_gitlab_redis_shared_state do context 'when offset is negative' do let(:offset) { -1 } - it { expect { subject }.to raise_error('Offset is out of bound') } + it { expect { subject }.to raise_error('Offset is out of range') } end context 'when offset is bigger than data size' do let(:offset) { data.bytesize + 1 } - it { expect { subject }.to raise_error('Offset is out of bound') } + it { expect { subject }.to raise_error('Offset is out of range') } end context 'when offset is bigger than data size' do let(:new_data) { 'a' * (described_class::CHUNK_SIZE + 1) } - it { expect { subject }.to raise_error('Outside of chunk size') } + it { expect { subject }.to raise_error('Chunk size overflow') } end context 'when offset is EOF' do @@ -320,7 +324,7 @@ describe Ci::BuildTraceChunk, :clean_gitlab_redis_shared_state do describe 'ExclusiveLock' do before do allow_any_instance_of(Gitlab::ExclusiveLease).to receive(:try_obtain) { nil } - stub_const('Ci::BuildTraceChunk::LOCK_RETRY', 1) + stub_const('Ci::BuildTraceChunk::WRITE_LOCK_RETRY', 1) end it 'raise an error' do @@ -333,30 +337,31 @@ describe Ci::BuildTraceChunk, :clean_gitlab_redis_shared_state do before do pipeline = create(:ci_pipeline, project: project) - create(:ci_build, :running, :trace_live, pipeline: pipeline, project: project) - create(:ci_build, :running, :trace_live, pipeline: pipeline, project: project) - create(:ci_build, :running, :trace_live, pipeline: pipeline, project: project) + @build_ids = [] + @build_ids << create(:ci_build, :running, :trace_live, pipeline: pipeline, project: project).id + @build_ids << create(:ci_build, :running, :trace_live, pipeline: pipeline, project: project).id + @build_ids << create(:ci_build, :running, :trace_live, pipeline: pipeline, project: project).id end shared_examples_for 'deletes all build_trace_chunk and data in redis' do it do - project.builds.each do |build| + @build_ids.each do |build_id| Gitlab::Redis::SharedState.with do |redis| - redis.scan_each(match: "gitlab:ci:trace:#{build.id}:chunks:?") do |key| + redis.scan_each(match: "gitlab:ci:trace:#{build_id}:chunks:?") do |key| expect(redis.exists(key)).to be_truthy end end end - expect(described_class.count).not_to eq(0) + expect(described_class.count).to eq(3) subject expect(described_class.count).to eq(0) - project.builds.each do |build| + @build_ids.each do |build_id| Gitlab::Redis::SharedState.with do |redis| - redis.scan_each(match: "gitlab:ci:trace:#{build.id}:chunks:?") do |key| + redis.scan_each(match: "gitlab:ci:trace:#{build_id}:chunks:?") do |key| expect(redis.exists(key)).to be_falsey end end @@ -364,14 +369,6 @@ describe Ci::BuildTraceChunk, :clean_gitlab_redis_shared_state do end end - context 'when build_trace_chunk is destroyed' do - let(:subject) do - project.builds.each { |build| build.chunks.destroy_all } - end - - it_behaves_like 'deletes all build_trace_chunk and data in redis' - end - context 'when build is destroyed' do let(:subject) do project.builds.destroy_all diff --git a/spec/services/ci/retry_build_service_spec.rb b/spec/services/ci/retry_build_service_spec.rb index 955894444eb..cc200f6fc0a 100644 --- a/spec/services/ci/retry_build_service_spec.rb +++ b/spec/services/ci/retry_build_service_spec.rb @@ -30,7 +30,7 @@ describe Ci::RetryBuildService do runner_id tag_taggings taggings tags trigger_request_id user_id auto_canceled_by_id retried failure_reason artifacts_file_store artifacts_metadata_store - metadata chunks].freeze + metadata trace_chunks].freeze shared_examples 'build duplication' do let(:another_pipeline) { create(:ci_empty_pipeline, project: project) }