Merge branch '224466-deprecate-remove-trace-parameter-from-put-api-jobs-id' into 'master'

Remove `trace` parameter from `PUT /api/jobs/:id` [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!62758

app/services/ci/update_build_state_service.rb

@@ -19,8 +19,6 @@ module Ci
     end
 
     def execute
-      overwrite_trace! if has_trace?
-
       unless accept_available?
         return update_build_state!
       end
@@ -34,12 +32,6 @@ module Ci
 
     private
 
-    def overwrite_trace!
-      metrics.increment_trace_operation(operation: :overwrite)
-
-      build.trace.set(params[:trace]) if Gitlab::Ci::Features.trace_overwrite?
-    end
-
     def ensure_pending_state!
       pending_state.created_at
     end
@@ -151,10 +143,6 @@ module Ci
       params.dig(:state).to_s
     end
 
-    def has_trace?
-      params.dig(:trace).present?
-    end
-
     def has_checksum?
       trace_checksum.present?
     end

config/feature_flags/ops/ci_trace_overwrite.yml

----
-name: ci_trace_overwrite
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/41304
-rollout_issue_url: 
-milestone: '13.4'
-type: ops
-group: group::continuous integration
-default_enabled: false

doc/administration/job_logs.md

@@ -20,9 +20,8 @@ In the following table you can see the phases a log goes through:
 | Phase          | State        | Condition               | Data flow                                | Stored path |
 | -------------- | ------------ | ----------------------- | -----------------------------------------| ----------- |
 | 1: patching    | log          | When a job is running   | Runner => Puma => file storage | `#{ROOT_PATH}/gitlab-ci/builds/#{YYYY_mm}/#{project_id}/#{job_id}.log` |
-| 2: overwriting | log          | When a job is finished  | Runner => Puma => file storage | `#{ROOT_PATH}/gitlab-ci/builds/#{YYYY_mm}/#{project_id}/#{job_id}.log` |
-| 3: archiving   | archived log | After a job is finished | Sidekiq moves log to artifacts folder    | `#{ROOT_PATH}/gitlab-rails/shared/artifacts/#{disk_hash}/#{YYYY_mm_dd}/#{job_id}/#{job_artifact_id}/job.log` |
-| 4: uploading   | archived log | After a log is archived | Sidekiq moves archived log to [object storage](#uploading-logs-to-object-storage) (if configured) | `#{bucket_name}/#{disk_hash}/#{YYYY_mm_dd}/#{job_id}/#{job_artifact_id}/job.log` |
+| 2: archiving   | archived log | After a job is finished | Sidekiq moves log to artifacts folder    | `#{ROOT_PATH}/gitlab-rails/shared/artifacts/#{disk_hash}/#{YYYY_mm_dd}/#{job_id}/#{job_artifact_id}/job.log` |
+| 3: uploading   | archived log | After a log is archived | Sidekiq moves archived log to [object storage](#uploading-logs-to-object-storage) (if configured) | `#{bucket_name}/#{disk_hash}/#{YYYY_mm_dd}/#{job_id}/#{job_artifact_id}/job.log` |
 
 The `ROOT_PATH` varies per environment. For Omnibus GitLab it
 would be `/var/opt/gitlab`, and for installations from source

lib/api/ci/runner.rb

@@ -168,7 +168,6 @@ module API
         params do
           requires :token, type: String, desc: %q(Runners's authentication token)
           requires :id, type: Integer, desc: %q(Job's ID)
-          optional :trace, type: String, desc: %q(Job's full trace)
           optional :state, type: String, desc: %q(Job's status: success, failed)
           optional :checksum, type: String, desc: %q(Job's trace CRC32 checksum)
           optional :failure_reason, type: String, desc: %q(Job's failure_reason)

lib/gitlab/ci/features.rb

@@ -25,10 +25,6 @@ module Gitlab
         ::Feature.enabled?(:ci_disallow_to_create_merge_request_pipelines_in_target_project, target_project)
       end
 
-      def self.trace_overwrite?
-        ::Feature.enabled?(:ci_trace_overwrite, type: :ops, default_enabled: false)
-      end
-
       def self.accept_trace?(project)
         ::Feature.enabled?(:ci_enable_live_trace, project) &&
           ::Feature.enabled?(:ci_accept_trace, project, type: :ops, default_enabled: true)

lib/gitlab/ci/trace/metrics.rb

@@ -11,7 +11,6 @@ module Gitlab
           :streamed,  # new trace data has been sent by a runner
           :chunked,   # new trace chunk has been created
           :mutated,   # trace has been mutated when removing secrets
-          :overwrite, # runner requested overwritting a build trace
           :accepted,  # scheduled chunks for migration and responded with 202
           :finalized, # all live build trace chunks have been persisted
           :discarded, # failed to persist live chunks before timeout

spec/requests/api/ci/runner/jobs_put_spec.rb

@@ -17,18 +17,14 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do
   end
 
   describe '/api/v4/jobs' do
-    let(:group) { create(:group, :nested) }
-    let(:project) { create(:project, namespace: group, shared_runners_enabled: false) }
-    let(:pipeline) { create(:ci_pipeline, project: project, ref: 'master') }
-    let(:runner) { create(:ci_runner, :project, projects: [project]) }
-    let(:user) { create(:user) }
-    let(:job) do
-      create(:ci_build, :artifacts, :extended_options,
-             pipeline: pipeline, name: 'spinach', stage: 'test', stage_idx: 0)
-    end
+    let_it_be(:group) { create(:group, :nested) }
+    let_it_be(:project) { create(:project, namespace: group, shared_runners_enabled: false) }
+    let_it_be(:pipeline) { create(:ci_pipeline, project: project, ref: 'master') }
+    let_it_be(:runner) { create(:ci_runner, :project, projects: [project]) }
+    let_it_be(:user) { create(:user) }
 
     describe 'PUT /api/v4/jobs/:id' do
-      let(:job) do
+      let_it_be_with_reload(:job) do
         create(:ci_build, :pending, :trace_live, pipeline: pipeline, project: project, user: user, runner_id: runner.id)
       end
 
@@ -204,53 +200,6 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do
         end
       end
 
-      context 'when trace is given' do
-        it 'creates a trace artifact' do
-          allow(BuildFinishedWorker).to receive(:perform_async).with(job.id) do
-            ArchiveTraceWorker.new.perform(job.id)
-          end
-
-          update_job(state: 'success', trace: 'BUILD TRACE UPDATED')
-
-          job.reload
-          expect(response).to have_gitlab_http_status(:ok)
-          expect(job.trace.raw).to eq 'BUILD TRACE UPDATED'
-          expect(job.job_artifacts_trace.open.read).to eq 'BUILD TRACE UPDATED'
-        end
-
-        context 'when concurrent update of trace is happening' do
-          before do
-            job.trace.write('wb') do
-              update_job(state: 'success', trace: 'BUILD TRACE UPDATED')
-            end
-          end
-
-          it 'returns that operation conflicts' do
-            expect(response).to have_gitlab_http_status(:conflict)
-          end
-        end
-      end
-
-      context 'when no trace is given' do
-        it 'does not override trace information' do
-          update_job
-
-          expect(job.reload.trace.raw).to eq 'BUILD TRACE'
-        end
-
-        context 'when running state is sent' do
-          it 'updates update_at value' do
-            expect { update_job_after_time }.to change { job.reload.updated_at }
-          end
-        end
-
-        context 'when other state is sent' do
-          it "doesn't update update_at value" do
-            expect { update_job_after_time(20.minutes, state: 'success') }.not_to change { job.reload.updated_at }
-          end
-        end
-      end
-
       context 'when job has been erased' do
         let(:job) { create(:ci_build, runner_id: runner.id, erased_at: Time.now) }
 
@@ -267,20 +216,19 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do
           job.drop!(:script_failure)
         end
 
-        it 'does not update job status and job trace' do
-          update_job(state: 'success', trace: 'BUILD TRACE UPDATED')
+        it 'does not update job status' do
+          update_job(state: 'success')
 
           job.reload
           expect(response).to have_gitlab_http_status(:forbidden)
           expect(response.header['Job-Status']).to eq 'failed'
-          expect(job.trace.raw).to eq 'Job failed'
           expect(job).to be_failed
         end
       end
 
       context 'when job does not exist anymore' do
         it 'returns 403 Forbidden' do
-          update_job(non_existing_record_id, state: 'success', trace: 'BUILD TRACE UPDATED')
+          update_job(non_existing_record_id, state: 'success')
 
           expect(response).to have_gitlab_http_status(:forbidden)
         end

spec/services/ci/update_build_state_service_spec.rb

@@ -3,8 +3,9 @@
 require 'spec_helper'
 
 RSpec.describe Ci::UpdateBuildStateService do
-  let(:project) { create(:project) }
-  let(:pipeline) { create(:ci_pipeline, project: project) }
+  let_it_be(:project) { create(:project) }
+  let_it_be(:pipeline) { create(:ci_pipeline, project: project) }
+
   let(:build) { create(:ci_build, :running, pipeline: pipeline) }
   let(:metrics) { spy('metrics') }
 
@@ -47,25 +48,6 @@ RSpec.describe Ci::UpdateBuildStateService do
       end
     end
 
-    context 'when request payload carries a trace' do
-      let(:params) { { state: 'success', trace: 'overwritten' } }
-
-      it 'overwrites a trace' do
-        result = subject.execute
-
-        expect(build.trace.raw).to eq 'overwritten'
-        expect(result.status).to eq 200
-      end
-
-      it 'updates overwrite operation metric' do
-        execute_with_stubbed_metrics!
-
-        expect(metrics)
-          .to have_received(:increment_trace_operation)
-          .with(operation: :overwrite)
-      end
-    end
-
     context 'when state is unknown' do
       let(:params) { { state: 'unknown' } }