diff options
| -rw-r--r-- | app/finders/admin/projects_finder.rb | 4 | ||||
| -rw-r--r-- | app/models/concerns/routable.rb | 44 | ||||
| -rw-r--r-- | app/models/concerns/storage/legacy_namespace.rb | 2 | ||||
| -rw-r--r-- | app/models/namespace.rb | 1 | ||||
| -rw-r--r-- | app/models/project.rb | 3 | ||||
| -rw-r--r-- | app/serializers/pipeline_serializer.rb | 9 | ||||
| -rw-r--r-- | app/services/projects/transfer_service.rb | 1 | ||||
| -rw-r--r-- | app/views/admin/projects/_projects.html.haml | 2 | ||||
| -rw-r--r-- | changelogs/unreleased/stop-dynamic-routable-creation.yml | 5 | ||||
| -rw-r--r-- | db/migrate/20180702134423_generate_missing_routes.rb | 143 | ||||
| -rw-r--r-- | spec/controllers/projects/pipelines_controller_spec.rb | 14 | ||||
| -rw-r--r-- | spec/migrations/generate_missing_routes_spec.rb | 84 | ||||
| -rw-r--r-- | spec/models/concerns/routable_spec.rb | 33 | ||||
| -rw-r--r-- | spec/models/namespace_spec.rb | 10 | ||||
| -rw-r--r-- | spec/models/project_spec.rb | 4 | ||||
| -rw-r--r-- | spec/serializers/pipeline_serializer_spec.rb | 4 | ||||
| -rw-r--r-- | spec/services/projects/transfer_service_spec.rb | 6 |
17 files changed, 265 insertions, 104 deletions
diff --git a/app/finders/admin/projects_finder.rb b/app/finders/admin/projects_finder.rb index 53b77f5fed9..543bf1a1415 100644 --- a/app/finders/admin/projects_finder.rb +++ b/app/finders/admin/projects_finder.rb @@ -7,7 +7,7 @@ class Admin::ProjectsFinder end def execute - items = Project.without_deleted.with_statistics + items = Project.without_deleted.with_statistics.with_route items = by_namespace_id(items) items = by_visibilty_level(items) items = by_with_push(items) @@ -16,7 +16,7 @@ class Admin::ProjectsFinder items = by_archived(items) items = by_personal(items) items = by_name(items) - items = items.includes(namespace: [:owner]) + items = items.includes(namespace: [:owner, :route]) sort(items).page(params[:page]) end diff --git a/app/models/concerns/routable.rb b/app/models/concerns/routable.rb index 0176a12a131..cb91f8fbac8 100644 --- a/app/models/concerns/routable.rb +++ b/app/models/concerns/routable.rb @@ -90,34 +90,17 @@ module Routable end def full_name - if route && route.name.present? - @full_name ||= route.name # rubocop:disable Gitlab/ModuleWithInstanceVariables - else - update_route if persisted? - - build_full_name - end + route&.name || build_full_name end - # Every time `project.namespace.becomes(Namespace)` is called for polymorphic_path, - # a new instance is instantiated, and we end up duplicating the same query to retrieve - # the route. Caching this per request ensures that even if we have multiple instances, - # we will not have to duplicate work, avoiding N+1 queries in some cases. def full_path - return uncached_full_path unless RequestStore.active? && persisted? - - RequestStore[full_path_key] ||= uncached_full_path + route&.path || build_full_path end def full_path_components full_path.split('/') end - def expires_full_path_cache - RequestStore.delete(full_path_key) if RequestStore.active? - @full_path = nil # rubocop:disable Gitlab/ModuleWithInstanceVariables - end - def build_full_path if parent && path parent.full_path + '/' + path @@ -138,16 +121,6 @@ module Routable self.errors[:path].concat(route_path_errors) if route_path_errors end - def uncached_full_path - if route && route.path.present? - @full_path ||= route.path # rubocop:disable Gitlab/ModuleWithInstanceVariables - else - update_route if persisted? - - build_full_path - end - end - def full_name_changed? name_changed? || parent_changed? end @@ -156,10 +129,6 @@ module Routable path_changed? || parent_changed? end - def full_path_key - @full_path_key ||= "routable/full_path/#{self.class.name}/#{self.id}" - end - def build_full_name if parent && name parent.human_name + ' / ' + name @@ -168,18 +137,9 @@ module Routable end end - def update_route - return if Gitlab::Database.read_only? - - prepare_route - route.save - end - def prepare_route route || build_route(source: self) route.path = build_full_path route.name = build_full_name - @full_path = nil # rubocop:disable Gitlab/ModuleWithInstanceVariables - @full_name = nil # rubocop:disable Gitlab/ModuleWithInstanceVariables end end diff --git a/app/models/concerns/storage/legacy_namespace.rb b/app/models/concerns/storage/legacy_namespace.rb index f66bdd529f1..d61ad19b43d 100644 --- a/app/models/concerns/storage/legacy_namespace.rb +++ b/app/models/concerns/storage/legacy_namespace.rb @@ -11,8 +11,6 @@ module Storage Namespace.find(parent_id_was) # raise NotFound early if needed end - expires_full_path_cache - move_repositories if parent_changed? diff --git a/app/models/namespace.rb b/app/models/namespace.rb index 7034c633268..c1dc2f55346 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -304,7 +304,6 @@ class Namespace < ActiveRecord::Base def write_projects_repository_config all_projects.find_each do |project| - project.expires_full_path_cache # we need to clear cache to validate renames correctly project.write_repository_config end end diff --git a/app/models/project.rb b/app/models/project.rb index f880d728839..2034e752635 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1233,8 +1233,6 @@ class Project < ActiveRecord::Base return true if skip_disk_validation return false unless repository_storage - expires_full_path_cache # we need to clear cache to validate renames correctly - # Check if repository with same path already exists on disk we can # skip this for the hashed storage because the path does not change if legacy_storage? && repository_with_same_path_already_exists? @@ -1613,7 +1611,6 @@ class Project < ActiveRecord::Base # When we import a project overwriting the original project, there # is a move operation. In that case we don't want to send the instructions. send_move_instructions(full_path_was) unless import_started? - expires_full_path_cache self.old_path_with_namespace = full_path_was SystemHooksService.new.execute_hooks_for(self, :rename) diff --git a/app/serializers/pipeline_serializer.rb b/app/serializers/pipeline_serializer.rb index 4a33160afa1..3205578b83e 100644 --- a/app/serializers/pipeline_serializer.rb +++ b/app/serializers/pipeline_serializer.rb @@ -11,10 +11,15 @@ class PipelineSerializer < BaseSerializer :retryable_builds, :cancelable_statuses, :trigger_requests, - :project, :manual_actions, :artifacts, - { pending_builds: :project } + { + pending_builds: :project, + project: [:route, { namespace: :route }], + artifacts: { + project: [:route, { namespace: :route }] + } + } ]) end diff --git a/app/services/projects/transfer_service.rb b/app/services/projects/transfer_service.rb index a4a66330546..c2a0c5fa7f3 100644 --- a/app/services/projects/transfer_service.rb +++ b/app/services/projects/transfer_service.rb @@ -77,7 +77,6 @@ module Projects Gitlab::PagesTransfer.new.move_project(project.path, @old_namespace.full_path, @new_namespace.full_path) project.old_path_with_namespace = @old_path - project.expires_full_path_cache write_repository_config(@new_path) diff --git a/app/views/admin/projects/_projects.html.haml b/app/views/admin/projects/_projects.html.haml index 00933d726d9..fdaacc098e0 100644 --- a/app/views/admin/projects/_projects.html.haml +++ b/app/views/admin/projects/_projects.html.haml @@ -17,7 +17,7 @@ - if project.archived %span.badge.badge-warning archived .title - = link_to [:admin, project.namespace.becomes(Namespace), project] do + = link_to(admin_namespace_project_path(project.namespace, project)) do .dash-project-avatar .avatar-container.s40 = project_icon(project, alt: '', class: 'avatar project-avatar s40') diff --git a/changelogs/unreleased/stop-dynamic-routable-creation.yml b/changelogs/unreleased/stop-dynamic-routable-creation.yml new file mode 100644 index 00000000000..8bfcb5b2d11 --- /dev/null +++ b/changelogs/unreleased/stop-dynamic-routable-creation.yml @@ -0,0 +1,5 @@ +--- +title: Stop dynamically creating project and namespace routes +merge_request: 20313 +author: +type: performance diff --git a/db/migrate/20180702134423_generate_missing_routes.rb b/db/migrate/20180702134423_generate_missing_routes.rb new file mode 100644 index 00000000000..994725f9bd1 --- /dev/null +++ b/db/migrate/20180702134423_generate_missing_routes.rb @@ -0,0 +1,143 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +# This migration generates missing routes for any projects and namespaces that +# don't already have a route. +# +# On GitLab.com this would insert 611 project routes, and 0 namespace routes. +# The exact number could vary per instance, so we take care of both just in +# case. +class GenerateMissingRoutes < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + class User < ActiveRecord::Base + self.table_name = 'users' + end + + class Route < ActiveRecord::Base + self.table_name = 'routes' + end + + module Routable + def build_full_path + if parent && path + parent.build_full_path + '/' + path + else + path + end + end + + def build_full_name + if parent && name + parent.human_name + ' / ' + name + else + name + end + end + + def human_name + build_full_name + end + + def attributes_for_insert + time = Time.zone.now + + { + # We can't use "self.class.name" here as that would include the + # migration namespace. + source_type: source_type_for_route, + source_id: id, + created_at: time, + updated_at: time, + name: build_full_name, + + # The route path might already be taken. Instead of trying to generate a + # new unique name on every conflict, we just append the row ID to the + # route path. + path: "#{build_full_path}-#{id}" + } + end + end + + class Project < ActiveRecord::Base + self.table_name = 'projects' + + include EachBatch + include GenerateMissingRoutes::Routable + + belongs_to :namespace, class_name: 'GenerateMissingRoutes::Namespace' + + has_one :route, + as: :source, + inverse_of: :source, + class_name: 'GenerateMissingRoutes::Route' + + alias_method :parent, :namespace + alias_attribute :parent_id, :namespace_id + + def self.without_routes + where( + 'NOT EXISTS ( + SELECT 1 + FROM routes + WHERE source_type = ? + AND source_id = projects.id + )', + 'Project' + ) + end + + def source_type_for_route + 'Project' + end + end + + class Namespace < ActiveRecord::Base + self.table_name = 'namespaces' + + include EachBatch + include GenerateMissingRoutes::Routable + + belongs_to :parent, class_name: 'GenerateMissingRoutes::Namespace' + belongs_to :owner, class_name: 'GenerateMissingRoutes::User' + + has_one :route, + as: :source, + inverse_of: :source, + class_name: 'GenerateMissingRoutes::Route' + + def self.without_routes + where( + 'NOT EXISTS ( + SELECT 1 + FROM routes + WHERE source_type = ? + AND source_id = namespaces.id + )', + 'Namespace' + ) + end + + def source_type_for_route + 'Namespace' + end + end + + def up + [Namespace, Project].each do |model| + model.without_routes.each_batch(of: 100) do |batch| + rows = batch.map(&:attributes_for_insert) + + Gitlab::Database.bulk_insert(:routes, rows) + end + end + end + + def down + # Removing routes we previously generated makes no sense. + end +end diff --git a/spec/controllers/projects/pipelines_controller_spec.rb b/spec/controllers/projects/pipelines_controller_spec.rb index 290fcd4f8e6..d89716b1b50 100644 --- a/spec/controllers/projects/pipelines_controller_spec.rb +++ b/spec/controllers/projects/pipelines_controller_spec.rb @@ -55,10 +55,8 @@ describe Projects::PipelinesController do stub_feature_flags(ci_pipeline_persisted_stages: false) end - it 'returns JSON with serialized pipelines', :request_store do - queries = ActiveRecord::QueryRecorder.new do - get_pipelines_index_json - end + it 'returns JSON with serialized pipelines' do + get_pipelines_index_json expect(response).to have_gitlab_http_status(:ok) expect(response).to match_response_schema('pipeline') @@ -73,8 +71,14 @@ describe Projects::PipelinesController do json_response.dig('pipelines', 0, 'details', 'stages').tap do |stages| expect(stages.count).to eq 3 end + end + + it 'does not execute N+1 queries' do + queries = ActiveRecord::QueryRecorder.new do + get_pipelines_index_json + end - expect(queries.count).to be_within(5).of(30) + expect(queries.count).to be <= 36 end end diff --git a/spec/migrations/generate_missing_routes_spec.rb b/spec/migrations/generate_missing_routes_spec.rb new file mode 100644 index 00000000000..32515d353b0 --- /dev/null +++ b/spec/migrations/generate_missing_routes_spec.rb @@ -0,0 +1,84 @@ +require 'spec_helper' +require Rails.root.join('db', 'migrate', '20180702134423_generate_missing_routes.rb') + +describe GenerateMissingRoutes, :migration do + describe '#up' do + let(:namespaces) { table(:namespaces) } + let(:projects) { table(:projects) } + let(:routes) { table(:routes) } + + it 'creates routes for projects without a route' do + namespace = namespaces.create!(name: 'GitLab', path: 'gitlab') + + routes.create!( + path: 'gitlab', + source_type: 'Namespace', + source_id: namespace.id + ) + + project = projects.create!( + name: 'GitLab CE', + path: 'gitlab-ce', + namespace_id: namespace.id + ) + + described_class.new.up + + route = routes.where(source_type: 'Project').take + + expect(route.source_id).to eq(project.id) + expect(route.path).to eq("gitlab/gitlab-ce-#{project.id}") + end + + it 'creates routes for namespaces without a route' do + namespace = namespaces.create!(name: 'GitLab', path: 'gitlab') + + described_class.new.up + + route = routes.where(source_type: 'Namespace').take + + expect(route.source_id).to eq(namespace.id) + expect(route.path).to eq("gitlab-#{namespace.id}") + end + + it 'does not create routes for namespaces that already have a route' do + namespace = namespaces.create!(name: 'GitLab', path: 'gitlab') + + routes.create!( + path: 'gitlab', + source_type: 'Namespace', + source_id: namespace.id + ) + + described_class.new.up + + expect(routes.count).to eq(1) + end + + it 'does not create routes for projects that already have a route' do + namespace = namespaces.create!(name: 'GitLab', path: 'gitlab') + + routes.create!( + path: 'gitlab', + source_type: 'Namespace', + source_id: namespace.id + ) + + project = projects.create!( + name: 'GitLab CE', + path: 'gitlab-ce', + namespace_id: namespace.id + ) + + routes.create!( + path: 'gitlab/gitlab-ce', + source_type: 'Project', + source_id: project.id + ) + + described_class.new.up + + expect(routes.count).to eq(2) + end + end +end diff --git a/spec/models/concerns/routable_spec.rb b/spec/models/concerns/routable_spec.rb index ed3e28fbeca..565266321d3 100644 --- a/spec/models/concerns/routable_spec.rb +++ b/spec/models/concerns/routable_spec.rb @@ -12,16 +12,6 @@ describe Group, 'Routable' do it { is_expected.to have_many(:redirect_routes).dependent(:destroy) } end - describe 'GitLab read-only instance' do - it 'does not save route if route is not present' do - group.route.path = '' - allow(Gitlab::Database).to receive(:read_only?).and_return(true) - expect(group).to receive(:update_route).and_call_original - - expect { group.full_path }.to change { Route.count }.by(0) - end - end - describe 'Callbacks' do it 'creates route record on create' do expect(group.route.path).to eq(group.path) @@ -131,29 +121,6 @@ describe Group, 'Routable' do it { expect(group.full_path).to eq(group.path) } it { expect(nested_group.full_path).to eq("#{group.full_path}/#{nested_group.path}") } - - context 'with RequestStore active', :request_store do - it 'does not load the route table more than once' do - group.expires_full_path_cache - expect(group).to receive(:uncached_full_path).once.and_call_original - - 3.times { group.full_path } - expect(group.full_path).to eq(group.path) - end - end - end - - describe '#expires_full_path_cache' do - context 'with RequestStore active', :request_store do - it 'expires the full_path cache' do - expect(group.full_path).to eq('foo') - - group.route.update(path: 'bar', name: 'bar') - group.expires_full_path_cache - - expect(group.full_path).to eq('bar') - end - end end describe '#full_name' do diff --git a/spec/models/namespace_spec.rb b/spec/models/namespace_spec.rb index c1b385aaf76..f56b79119d3 100644 --- a/spec/models/namespace_spec.rb +++ b/spec/models/namespace_spec.rb @@ -295,6 +295,16 @@ describe Namespace do parent.update(path: 'mygroup_new') + # Routes are loaded when creating the projects, so we need to manually + # reload them for the below code to be aware of the above UPDATE. + [ + project_in_parent_group, + hashed_project_in_subgroup, + legacy_project_in_subgroup + ].each do |project| + project.route.reload + end + expect(project_rugged(project_in_parent_group).config['gitlab.fullpath']).to eq "mygroup_new/#{project_in_parent_group.path}" expect(project_rugged(hashed_project_in_subgroup).config['gitlab.fullpath']).to eq "mygroup_new/mysubgroup/#{hashed_project_in_subgroup.path}" expect(project_rugged(legacy_project_in_subgroup).config['gitlab.fullpath']).to eq "mygroup_new/mysubgroup/#{legacy_project_in_subgroup.path}" diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index b0ec725bf70..deaaaed424b 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -2942,8 +2942,6 @@ describe Project do expect(project).to receive(:expire_caches_before_rename) - expect(project).to receive(:expires_full_path_cache) - project.rename_repo end @@ -3103,8 +3101,6 @@ describe Project do expect(project).to receive(:expire_caches_before_rename) - expect(project).to receive(:expires_full_path_cache) - project.rename_repo end diff --git a/spec/serializers/pipeline_serializer_spec.rb b/spec/serializers/pipeline_serializer_spec.rb index eb4235e3ee6..cf57776346a 100644 --- a/spec/serializers/pipeline_serializer_spec.rb +++ b/spec/serializers/pipeline_serializer_spec.rb @@ -125,7 +125,7 @@ describe PipelineSerializer do it 'verifies number of queries', :request_store do recorded = ActiveRecord::QueryRecorder.new { subject } - expect(recorded.count).to be_within(2).of(27) + expect(recorded.count).to be_within(2).of(31) expect(recorded.cached_count).to eq(0) end end @@ -144,7 +144,7 @@ describe PipelineSerializer do # pipeline. With the same ref this check is cached but if refs are # different then there is an extra query per ref # https://gitlab.com/gitlab-org/gitlab-ce/issues/46368 - expect(recorded.count).to be_within(2).of(30) + expect(recorded.count).to be_within(2).of(34) expect(recorded.cached_count).to eq(0) end end diff --git a/spec/services/projects/transfer_service_spec.rb b/spec/services/projects/transfer_service_spec.rb index 7e85f599afb..1a85c52fc97 100644 --- a/spec/services/projects/transfer_service_spec.rb +++ b/spec/services/projects/transfer_service_spec.rb @@ -31,12 +31,6 @@ describe Projects::TransferService do transfer_project(project, user, group) end - it 'expires full_path cache' do - expect(project).to receive(:expires_full_path_cache) - - transfer_project(project, user, group) - end - it 'invalidates the user\'s personal_project_count cache' do expect(user).to receive(:invalidate_personal_projects_count) |