From c3ad57034cc1cbd6d0ad02de7ac57f6004440c83 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Mon, 13 Jan 2020 09:08:03 +0000 Subject: [PATCH] Add latest changes from gitlab-org/gitlab@master --- app/controllers/projects/jobs_controller.rb | 2 + .../projects/uploads_controller.rb | 10 +++ app/views/layouts/_flash.html.haml | 2 + ...-reducing-build-trace-update-frequency.yml | 5 ++ changelogs/unreleased/icons-to-alerts.yml | 5 ++ .../sh-disable-formats-in-uploads-routes.yml | 5 ++ config/routes/group.rb | 2 +- config/routes/project.rb | 2 +- config/routes/uploads.rb | 6 +- lib/api/runner.rb | 4 ++ lib/gitlab/ci/trace.rb | 24 +++++++ .../groups/uploads_controller_spec.rb | 16 +++++ .../projects/jobs_controller_spec.rb | 6 ++ .../projects/uploads_controller_spec.rb | 15 +++++ spec/lib/gitlab/ci/trace_spec.rb | 62 +++++++++++++++++++ spec/requests/api/runner_spec.rb | 36 +++++++++++ spec/routing/uploads_routing_spec.rb | 8 +++ 17 files changed, 206 insertions(+), 4 deletions(-) create mode 100644 changelogs/unreleased/33658-reducing-build-trace-update-frequency.yml create mode 100644 changelogs/unreleased/icons-to-alerts.yml create mode 100644 changelogs/unreleased/sh-disable-formats-in-uploads-routes.yml diff --git a/app/controllers/projects/jobs_controller.rb b/app/controllers/projects/jobs_controller.rb index 796f3ff603f..cb473d6ee96 100644 --- a/app/controllers/projects/jobs_controller.rb +++ b/app/controllers/projects/jobs_controller.rb @@ -51,6 +51,8 @@ class Projects::JobsController < Projects::ApplicationController build.trace.read do |stream| respond_to do |format| format.json do + build.trace.being_watched! + # TODO: when the feature flag is removed we should not pass # content_format to serialize method. content_format = Feature.enabled?(:job_log_json, @project, default_enabled: true) ? :json : :html diff --git a/app/controllers/projects/uploads_controller.rb b/app/controllers/projects/uploads_controller.rb index 3e5a1cfc74d..72251988b5e 100644 --- a/app/controllers/projects/uploads_controller.rb +++ b/app/controllers/projects/uploads_controller.rb @@ -29,4 +29,14 @@ class Projects::UploadsController < Projects::ApplicationController Project.find_by_full_path("#{namespace}/#{id}") end + + # Overrides ApplicationController#build_canonical_path since there are + # multiple routes that match project uploads: + # https://gitlab.com/gitlab-org/gitlab/issues/196396 + def build_canonical_path(project) + return super unless action_name == 'show' + return super unless params[:secret] && params[:filename] + + show_namespace_project_uploads_url(project.namespace.to_param, project.to_param, params[:secret], params[:filename]) + end end diff --git a/app/views/layouts/_flash.html.haml b/app/views/layouts/_flash.html.haml index de1caeaa50f..07c271be2f0 100644 --- a/app/views/layouts/_flash.html.haml +++ b/app/views/layouts/_flash.html.haml @@ -1,10 +1,12 @@ -# We currently only support `alert`, `notice`, `success`, 'toast' +- icons = {'alert' => 'error', 'notice' => 'information-o', 'success' => 'check-circle'}; .flash-container.flash-container-page.sticky - flash.each do |key, value| - if key == 'toast' && value .js-toast-message{ data: { message: value } } - elsif value %div{ class: "flash-#{key} mb-2" } + = sprite_icon(icons[key], size: 16, css_class: 'align-middle mr-1') unless icons[key].nil? %span= value %div{ class: "close-icon-wrapper js-close-icon" } = sprite_icon('close', size: 16, css_class: 'close-icon') diff --git a/changelogs/unreleased/33658-reducing-build-trace-update-frequency.yml b/changelogs/unreleased/33658-reducing-build-trace-update-frequency.yml new file mode 100644 index 00000000000..aff8ec5c918 --- /dev/null +++ b/changelogs/unreleased/33658-reducing-build-trace-update-frequency.yml @@ -0,0 +1,5 @@ +--- +title: Request less frequent updates from Runner when job log is not being watched +merge_request: 20841 +author: +type: performance diff --git a/changelogs/unreleased/icons-to-alerts.yml b/changelogs/unreleased/icons-to-alerts.yml new file mode 100644 index 00000000000..a7e8b713f6d --- /dev/null +++ b/changelogs/unreleased/icons-to-alerts.yml @@ -0,0 +1,5 @@ +--- +title: Fix aligment for icons on alerts +merge_request: 22760 +author: Rajendra Kadam +type: added diff --git a/changelogs/unreleased/sh-disable-formats-in-uploads-routes.yml b/changelogs/unreleased/sh-disable-formats-in-uploads-routes.yml new file mode 100644 index 00000000000..4d0189749ac --- /dev/null +++ b/changelogs/unreleased/sh-disable-formats-in-uploads-routes.yml @@ -0,0 +1,5 @@ +--- +title: Fix upload redirections when project has moved +merge_request: 22822 +author: +type: fixed diff --git a/config/routes/group.rb b/config/routes/group.rb index 30671d4e0a1..24957d5ecef 100644 --- a/config/routes/group.rb +++ b/config/routes/group.rb @@ -68,7 +68,7 @@ constraints(::Constraints::GroupUrlConstrainer.new) do resources :uploads, only: [:create] do collection do - get ":secret/:filename", action: :show, as: :show, constraints: { filename: %r{[^/]+} } + get ":secret/:filename", action: :show, as: :show, constraints: { filename: %r{[^/]+} }, format: false, defaults: { format: nil } post :authorize end end diff --git a/config/routes/project.rb b/config/routes/project.rb index b420f07c7a1..3bc38ef3349 100644 --- a/config/routes/project.rb +++ b/config/routes/project.rb @@ -452,7 +452,7 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do resources :uploads, only: [:create] do collection do - get ":secret/:filename", action: :show, as: :show, constraints: { filename: %r{[^/]+} } + get ":secret/:filename", action: :show, as: :show, constraints: { filename: %r{[^/]+} }, format: false, defaults: { format: nil } post :authorize end end diff --git a/config/routes/uploads.rb b/config/routes/uploads.rb index 096ef146e07..fb8af76397c 100644 --- a/config/routes/uploads.rb +++ b/config/routes/uploads.rb @@ -21,9 +21,11 @@ scope path: :uploads do as: 'appearance_upload' # Project markdown uploads + # DEPRECATED: Remove this in GitLab 13.0 because this is redundant to show_namespace_project_uploads + # https://gitlab.com/gitlab-org/gitlab/issues/196396 get ":namespace_id/:project_id/:secret/:filename", - to: "projects/uploads#show", - constraints: { namespace_id: /[a-zA-Z.0-9_\-]+/, project_id: /[a-zA-Z.0-9_\-]+/, filename: %r{[^/]+} } + to: redirect("%{namespace_id}/%{project_id}/uploads/%{secret}/%{filename}"), + constraints: { namespace_id: /[a-zA-Z.0-9_\-]+/, project_id: /[a-zA-Z.0-9_\-]+/, filename: %r{[^/]+} }, format: false, defaults: { format: nil } # create uploads for models, snippets (notes) available for now post ':model', diff --git a/lib/api/runner.rb b/lib/api/runner.rb index f383c541f8a..60cf9bf2c9c 100644 --- a/lib/api/runner.rb +++ b/lib/api/runner.rb @@ -200,6 +200,10 @@ module API status 202 header 'Job-Status', job.status header 'Range', "0-#{stream_size}" + + if Feature.enabled?(:runner_job_trace_update_interval_header, default_enabled: true) + header 'X-GitLab-Trace-Update-Interval', job.trace.update_interval.to_s + end end desc 'Authorize artifacts uploading for job' do diff --git a/lib/gitlab/ci/trace.rb b/lib/gitlab/ci/trace.rb index 941f7178dac..4e83826b249 100644 --- a/lib/gitlab/ci/trace.rb +++ b/lib/gitlab/ci/trace.rb @@ -9,6 +9,10 @@ module Gitlab LOCK_TTL = 10.minutes LOCK_RETRIES = 2 LOCK_SLEEP = 0.001.seconds + WATCH_FLAG_TTL = 10.seconds + + UPDATE_FREQUENCY_DEFAULT = 30.seconds + UPDATE_FREQUENCY_WHEN_BEING_WATCHED = 3.seconds ArchiveError = Class.new(StandardError) AlreadyArchivedError = Class.new(StandardError) @@ -119,6 +123,22 @@ module Gitlab end end + def update_interval + being_watched? ? UPDATE_FREQUENCY_WHEN_BEING_WATCHED : UPDATE_FREQUENCY_DEFAULT + end + + def being_watched! + Gitlab::Redis::SharedState.with do |redis| + redis.set(being_watched_cache_key, true, ex: WATCH_FLAG_TTL) + end + end + + def being_watched? + Gitlab::Redis::SharedState.with do |redis| + redis.exists(being_watched_cache_key) + end + end + private def unsafe_write!(mode, &blk) @@ -236,6 +256,10 @@ module Gitlab def trace_artifact job.job_artifacts_trace end + + def being_watched_cache_key + "gitlab:ci:trace:#{job.id}:watched" + end end end end diff --git a/spec/controllers/groups/uploads_controller_spec.rb b/spec/controllers/groups/uploads_controller_spec.rb index 60342bf8e3d..8abebd04e8b 100644 --- a/spec/controllers/groups/uploads_controller_spec.rb +++ b/spec/controllers/groups/uploads_controller_spec.rb @@ -19,6 +19,22 @@ describe Groups::UploadsController do let(:uploader_class) { NamespaceFileUploader } end + context 'with a moved group' do + let!(:upload) { create(:upload, :issuable_upload, :with_file, model: model) } + let(:group) { model } + let(:old_path) { group.to_param + 'old' } + let!(:redirect_route) { model.redirect_routes.create(path: old_path) } + let(:upload_path) { File.basename(upload.path) } + + it 'redirects to a file with the proper extension' do + get :show, params: { group_id: old_path, filename: upload_path, secret: upload.secret } + + expect(response.location).to eq(show_group_uploads_url(group, upload.secret, upload_path)) + expect(response.location).to end_with(upload.path) + expect(response).to have_gitlab_http_status(:redirect) + end + end + def post_authorize(verified: true) request.headers.merge!(workhorse_internal_api_request_header) if verified diff --git a/spec/controllers/projects/jobs_controller_spec.rb b/spec/controllers/projects/jobs_controller_spec.rb index edef24f6595..53c40683a5b 100644 --- a/spec/controllers/projects/jobs_controller_spec.rb +++ b/spec/controllers/projects/jobs_controller_spec.rb @@ -556,6 +556,12 @@ describe Projects::JobsController, :clean_gitlab_redis_shared_state do expect(json_response['status']).to eq job.status expect(json_response['lines']).to eq [{ 'content' => [{ 'text' => 'BUILD TRACE' }], 'offset' => 0 }] end + + it 'sets being-watched flag for the job' do + expect(response).to have_gitlab_http_status(:ok) + + expect(job.trace.being_watched?).to be(true) + end end context 'when job has no traces' do diff --git a/spec/controllers/projects/uploads_controller_spec.rb b/spec/controllers/projects/uploads_controller_spec.rb index cd6a9886f72..a70669e86a6 100644 --- a/spec/controllers/projects/uploads_controller_spec.rb +++ b/spec/controllers/projects/uploads_controller_spec.rb @@ -25,6 +25,21 @@ describe Projects::UploadsController do end end + context 'with a moved project' do + let!(:upload) { create(:upload, :issuable_upload, :with_file, model: model) } + let(:project) { model } + let(:upload_path) { File.basename(upload.path) } + let!(:redirect_route) { project.redirect_routes.create(path: project.full_path + 'old') } + + it 'redirects to a file with the proper extension' do + get :show, params: { namespace_id: project.namespace, project_id: project.to_param + 'old', filename: File.basename(upload.path), secret: upload.secret } + + expect(response.location).to eq(show_project_uploads_url(project, upload.secret, upload_path)) + expect(response.location).to end_with(upload.path) + expect(response).to have_gitlab_http_status(:redirect) + end + end + context "when exception occurs" do before do allow(FileUploader).to receive(:workhorse_authorize).and_raise(SocketError.new) diff --git a/spec/lib/gitlab/ci/trace_spec.rb b/spec/lib/gitlab/ci/trace_spec.rb index f7bc5686b68..574c2b73722 100644 --- a/spec/lib/gitlab/ci/trace_spec.rb +++ b/spec/lib/gitlab/ci/trace_spec.rb @@ -26,4 +26,66 @@ describe Gitlab::Ci::Trace, :clean_gitlab_redis_shared_state do it_behaves_like 'trace with enabled live trace feature' end + + describe '#update_interval' do + context 'it is not being watched' do + it 'returns 30 seconds' do + expect(trace.update_interval).to eq(30.seconds) + end + end + + context 'it is being watched' do + before do + trace.being_watched! + end + + it 'returns 3 seconds' do + expect(trace.update_interval).to eq(3.seconds) + end + end + end + + describe '#being_watched!' do + let(:cache_key) { "gitlab:ci:trace:#{build.id}:watched" } + + it 'sets gitlab:ci:trace::watched in redis' do + trace.being_watched! + + result = Gitlab::Redis::SharedState.with do |redis| + redis.exists(cache_key) + end + + expect(result).to eq(true) + end + + it 'updates the expiry of gitlab:ci:trace::watched in redis', :clean_gitlab_redis_shared_state do + Gitlab::Redis::SharedState.with do |redis| + redis.set(cache_key, true, ex: 4.seconds) + end + + expect do + trace.being_watched! + end.to change { Gitlab::Redis::SharedState.with { |redis| redis.pttl(cache_key) } } + end + end + + describe '#being_watched?' do + context 'gitlab:ci:trace::watched in redis is set', :clean_gitlab_redis_shared_state do + before do + Gitlab::Redis::SharedState.with do |redis| + redis.set("gitlab:ci:trace:#{build.id}:watched", true) + end + end + + it 'returns true' do + expect(trace.being_watched?).to be(true) + end + end + + context 'gitlab:ci:trace::watched in redis is not set' do + it 'returns false' do + expect(trace.being_watched?).to be(false) + end + end + end end diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb index cc6cadb190a..93b35401706 100644 --- a/spec/requests/api/runner_spec.rb +++ b/spec/requests/api/runner_spec.rb @@ -1154,6 +1154,7 @@ describe API::Runner, :clean_gitlab_redis_shared_state do expect(job.reload.trace.raw).to eq 'BUILD TRACE appended' expect(response.header).to have_key 'Range' expect(response.header).to have_key 'Job-Status' + expect(response.header).to have_key 'X-GitLab-Trace-Update-Interval' end context 'when job has been updated recently' do @@ -1291,6 +1292,41 @@ describe API::Runner, :clean_gitlab_redis_shared_state do expect(response.header['Job-Status']).to eq 'canceled' end end + + context 'when build trace is being watched' do + before do + job.trace.being_watched! + end + + it 'returns X-GitLab-Trace-Update-Interval as 3' do + patch_the_trace + + expect(response.status).to eq 202 + expect(response.header['X-GitLab-Trace-Update-Interval']).to eq('3') + end + end + + context 'when build trace is not being watched' do + it 'returns X-GitLab-Trace-Update-Interval as 30' do + patch_the_trace + + expect(response.status).to eq 202 + expect(response.header['X-GitLab-Trace-Update-Interval']).to eq('30') + end + end + + context 'when feature flag runner_job_trace_update_interval_header is disabled' do + before do + stub_feature_flags(runner_job_trace_update_interval_header: { enabled: false }) + end + + it 'does not return X-GitLab-Trace-Update-Interval header' do + patch_the_trace + + expect(response.status).to eq 202 + expect(response.header).not_to have_key 'X-GitLab-Trace-Update-Interval' + end + end end context 'when Runner makes a force-patch' do diff --git a/spec/routing/uploads_routing_spec.rb b/spec/routing/uploads_routing_spec.rb index 42e84774088..f94ae81eeb5 100644 --- a/spec/routing/uploads_routing_spec.rb +++ b/spec/routing/uploads_routing_spec.rb @@ -28,4 +28,12 @@ describe 'Uploads', 'routing' do expect(post("/uploads/#{model}?id=1")).not_to be_routable end end + + describe 'legacy paths' do + include RSpec::Rails::RequestExampleGroup + + it 'redirects project uploads to canonical path under project namespace' do + expect(get('/uploads/namespace/project/12345/test.png')).to redirect_to('/namespace/project/uploads/12345/test.png') + end + end end