Commit b9c8a3d2 authored by Kamil Trzciński's avatar Kamil Trzciński

Merge branch 'fix/trace-patch-updated-at' into 'master'

Update the updated_at of a build while patching the trace

## What does this MR do?

Fixes build patching trace. It should update the `updated_at` field of a build to make sure it will not be removed by `stuck_ci_builds_worker`.

## Are there points in the code the reviewer needs to double check?

Construction of a test for the bug.

## Why was this MR needed?

Please read #22087 for a reference.

## Screenshots (if relevant)

## Does this MR meet the acceptance criteria?

- [x] [CHANGELOG](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG.md) entry added
- [ ] [Documentation created/updated](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/development/doc_styleguide.md)
- [ ] API support added
- Tests
  - [x] Added for this feature/bug
  - [ ] All builds are passing
- [ ] Conform by the [merge request performance guides](http://docs.gitlab.com/ce/development/merge_request_performance_guidelines.html)
- [ ] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [ ] Branch has no merge conflicts with `master` (if it does - rebase it please)
- [ ] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)

## What are the relevant issue numbers?

Fixes #22087

See merge request !7146
parents 4fd9a644 70048515
...@@ -271,6 +271,7 @@ module Ci ...@@ -271,6 +271,7 @@ module Ci
def append_trace(trace_part, offset) def append_trace(trace_part, offset)
recreate_trace_dir recreate_trace_dir
touch if needs_touch?
trace_part = hide_secrets(trace_part) trace_part = hide_secrets(trace_part)
...@@ -280,6 +281,10 @@ module Ci ...@@ -280,6 +281,10 @@ module Ci
end end
end end
def needs_touch?
Time.now - updated_at > 15.minutes.to_i
end
def trace_file_path def trace_file_path
if has_old_trace_file? if has_old_trace_file?
old_path_to_trace old_path_to_trace
......
---
title: Fix trace patching feature - update the updated_at value
merge_request: 7146
author:
...@@ -213,26 +213,102 @@ describe Ci::API::API do ...@@ -213,26 +213,102 @@ describe Ci::API::API do
let(:build) { create(:ci_build, :pending, :trace, runner_id: runner.id) } let(:build) { create(:ci_build, :pending, :trace, runner_id: runner.id) }
let(:headers) { { Ci::API::Helpers::BUILD_TOKEN_HEADER => build.token, 'Content-Type' => 'text/plain' } } let(:headers) { { Ci::API::Helpers::BUILD_TOKEN_HEADER => build.token, 'Content-Type' => 'text/plain' } }
let(:headers_with_range) { headers.merge({ 'Content-Range' => '11-20' }) } let(:headers_with_range) { headers.merge({ 'Content-Range' => '11-20' }) }
let(:update_interval) { 10.seconds.to_i }
def patch_the_trace(content = ' appended', request_headers = nil)
unless request_headers
offset = build.trace_length
limit = offset + content.length - 1
request_headers = headers.merge({ 'Content-Range' => "#{offset}-#{limit}" })
end
Timecop.travel(build.updated_at + update_interval) do
patch ci_api("/builds/#{build.id}/trace.txt"), content, request_headers
build.reload
end
end
def initial_patch_the_trace
patch_the_trace(' appended', headers_with_range)
end
def force_patch_the_trace
2.times { patch_the_trace('') }
end
before do before do
build.run! build.run!
patch ci_api("/builds/#{build.id}/trace.txt"), ' appended', headers_with_range initial_patch_the_trace
end end
context 'when request is valid' do context 'when request is valid' do
it 'gets correct response' do it 'gets correct response' do
expect(response.status).to eq 202 expect(response.status).to eq 202
expect(build.reload.trace).to eq 'BUILD TRACE appended'
expect(response.header).to have_key 'Range' expect(response.header).to have_key 'Range'
expect(response.header).to have_key 'Build-Status' expect(response.header).to have_key 'Build-Status'
end end
it { expect(build.reload.trace).to eq 'BUILD TRACE appended' } context 'when build has been updated recently' do
it { expect{ patch_the_trace }.not_to change { build.updated_at }}
it 'changes the build trace' do
patch_the_trace
expect(build.reload.trace).to eq 'BUILD TRACE appended appended'
end
context 'when Runner makes a force-patch' do
it { expect{ force_patch_the_trace }.not_to change { build.updated_at }}
it "doesn't change the build.trace" do
force_patch_the_trace
expect(build.reload.trace).to eq 'BUILD TRACE appended'
end
end
end
context 'when build was not updated recently' do
let(:update_interval) { 15.minutes.to_i }
it { expect { patch_the_trace }.to change { build.updated_at } }
it 'changes the build.trace' do
patch_the_trace
expect(build.reload.trace).to eq 'BUILD TRACE appended appended'
end
context 'when Runner makes a force-patch' do
it { expect { force_patch_the_trace }.to change { build.updated_at } }
it "doesn't change the build.trace" do
force_patch_the_trace
expect(build.reload.trace).to eq 'BUILD TRACE appended'
end
end
end
end
context 'when Runner makes a force-patch' do
before do
force_patch_the_trace
end
it 'gets correct response' do
expect(response.status).to eq 202
expect(build.reload.trace).to eq 'BUILD TRACE appended'
expect(response.header).to have_key 'Range'
expect(response.header).to have_key 'Build-Status'
end
end end
context 'when content-range start is too big' do context 'when content-range start is too big' do
let(:headers_with_range) { headers.merge({ 'Content-Range' => '15-20' }) } let(:headers_with_range) { headers.merge({ 'Content-Range' => '15-20' }) }
it 'gets correct response' do it 'gets 416 error response with range headers' do
expect(response.status).to eq 416 expect(response.status).to eq 416
expect(response.header).to have_key 'Range' expect(response.header).to have_key 'Range'
expect(response.header['Range']).to eq '0-11' expect(response.header['Range']).to eq '0-11'
...@@ -242,7 +318,7 @@ describe Ci::API::API do ...@@ -242,7 +318,7 @@ describe Ci::API::API do
context 'when content-range start is too small' do context 'when content-range start is too small' do
let(:headers_with_range) { headers.merge({ 'Content-Range' => '8-20' }) } let(:headers_with_range) { headers.merge({ 'Content-Range' => '8-20' }) }
it 'gets correct response' do it 'gets 416 error response with range headers' do
expect(response.status).to eq 416 expect(response.status).to eq 416
expect(response.header).to have_key 'Range' expect(response.header).to have_key 'Range'
expect(response.header['Range']).to eq '0-11' expect(response.header['Range']).to eq '0-11'
...@@ -250,7 +326,7 @@ describe Ci::API::API do ...@@ -250,7 +326,7 @@ describe Ci::API::API do
end end
context 'when Content-Range header is missing' do context 'when Content-Range header is missing' do
let(:headers_with_range) { headers.merge({}) } let(:headers_with_range) { headers }
it { expect(response.status).to eq 400 } it { expect(response.status).to eq 400 }
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