Commit 5f84509c authored by Douwe Maan's avatar Douwe Maan

Merge branch 'stop-dynamic-routable-creation' into 'master'

Stop building Route rows on the fly

See merge request gitlab-org/gitlab-ce!20313
parents ed81ee9b 7a233b37
...@@ -7,7 +7,7 @@ class Admin::ProjectsFinder ...@@ -7,7 +7,7 @@ class Admin::ProjectsFinder
end end
def execute def execute
items = Project.without_deleted.with_statistics items = Project.without_deleted.with_statistics.with_route
items = by_namespace_id(items) items = by_namespace_id(items)
items = by_visibilty_level(items) items = by_visibilty_level(items)
items = by_with_push(items) items = by_with_push(items)
...@@ -16,7 +16,7 @@ class Admin::ProjectsFinder ...@@ -16,7 +16,7 @@ class Admin::ProjectsFinder
items = by_archived(items) items = by_archived(items)
items = by_personal(items) items = by_personal(items)
items = by_name(items) items = by_name(items)
items = items.includes(namespace: [:owner]) items = items.includes(namespace: [:owner, :route])
sort(items).page(params[:page]) sort(items).page(params[:page])
end end
......
...@@ -90,34 +90,17 @@ module Routable ...@@ -90,34 +90,17 @@ module Routable
end end
def full_name def full_name
if route && route.name.present? route&.name || build_full_name
@full_name ||= route.name # rubocop:disable Gitlab/ModuleWithInstanceVariables
else
update_route if persisted?
build_full_name
end
end 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 def full_path
return uncached_full_path unless RequestStore.active? && persisted? route&.path || build_full_path
RequestStore[full_path_key] ||= uncached_full_path
end end
def full_path_components def full_path_components
full_path.split('/') full_path.split('/')
end 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 def build_full_path
if parent && path if parent && path
parent.full_path + '/' + path parent.full_path + '/' + path
...@@ -138,16 +121,6 @@ module Routable ...@@ -138,16 +121,6 @@ module Routable
self.errors[:path].concat(route_path_errors) if route_path_errors self.errors[:path].concat(route_path_errors) if route_path_errors
end 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? def full_name_changed?
name_changed? || parent_changed? name_changed? || parent_changed?
end end
...@@ -156,10 +129,6 @@ module Routable ...@@ -156,10 +129,6 @@ module Routable
path_changed? || parent_changed? path_changed? || parent_changed?
end end
def full_path_key
@full_path_key ||= "routable/full_path/#{self.class.name}/#{self.id}"
end
def build_full_name def build_full_name
if parent && name if parent && name
parent.human_name + ' / ' + name parent.human_name + ' / ' + name
...@@ -168,18 +137,9 @@ module Routable ...@@ -168,18 +137,9 @@ module Routable
end end
end end
def update_route
return if Gitlab::Database.read_only?
prepare_route
route.save
end
def prepare_route def prepare_route
route || build_route(source: self) route || build_route(source: self)
route.path = build_full_path route.path = build_full_path
route.name = build_full_name route.name = build_full_name
@full_path = nil # rubocop:disable Gitlab/ModuleWithInstanceVariables
@full_name = nil # rubocop:disable Gitlab/ModuleWithInstanceVariables
end end
end end
...@@ -11,8 +11,6 @@ module Storage ...@@ -11,8 +11,6 @@ module Storage
Namespace.find(parent_id_was) # raise NotFound early if needed Namespace.find(parent_id_was) # raise NotFound early if needed
end end
expires_full_path_cache
move_repositories move_repositories
if parent_changed? if parent_changed?
......
...@@ -304,7 +304,6 @@ class Namespace < ActiveRecord::Base ...@@ -304,7 +304,6 @@ class Namespace < ActiveRecord::Base
def write_projects_repository_config def write_projects_repository_config
all_projects.find_each do |project| all_projects.find_each do |project|
project.expires_full_path_cache # we need to clear cache to validate renames correctly
project.write_repository_config project.write_repository_config
end end
end end
......
...@@ -1238,8 +1238,6 @@ class Project < ActiveRecord::Base ...@@ -1238,8 +1238,6 @@ class Project < ActiveRecord::Base
return true if skip_disk_validation return true if skip_disk_validation
return false unless repository_storage 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 # Check if repository with same path already exists on disk we can
# skip this for the hashed storage because the path does not change # skip this for the hashed storage because the path does not change
if legacy_storage? && repository_with_same_path_already_exists? if legacy_storage? && repository_with_same_path_already_exists?
...@@ -1618,7 +1616,6 @@ class Project < ActiveRecord::Base ...@@ -1618,7 +1616,6 @@ class Project < ActiveRecord::Base
# When we import a project overwriting the original project, there # 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. # is a move operation. In that case we don't want to send the instructions.
send_move_instructions(full_path_was) unless import_started? send_move_instructions(full_path_was) unless import_started?
expires_full_path_cache
self.old_path_with_namespace = full_path_was self.old_path_with_namespace = full_path_was
SystemHooksService.new.execute_hooks_for(self, :rename) SystemHooksService.new.execute_hooks_for(self, :rename)
......
...@@ -11,10 +11,15 @@ class PipelineSerializer < BaseSerializer ...@@ -11,10 +11,15 @@ class PipelineSerializer < BaseSerializer
:retryable_builds, :retryable_builds,
:cancelable_statuses, :cancelable_statuses,
:trigger_requests, :trigger_requests,
:project,
:manual_actions, :manual_actions,
:artifacts, :artifacts,
{ pending_builds: :project } {
pending_builds: :project,
project: [:route, { namespace: :route }],
artifacts: {
project: [:route, { namespace: :route }]
}
}
]) ])
end end
......
...@@ -77,7 +77,6 @@ module Projects ...@@ -77,7 +77,6 @@ module Projects
Gitlab::PagesTransfer.new.move_project(project.path, @old_namespace.full_path, @new_namespace.full_path) Gitlab::PagesTransfer.new.move_project(project.path, @old_namespace.full_path, @new_namespace.full_path)
project.old_path_with_namespace = @old_path project.old_path_with_namespace = @old_path
project.expires_full_path_cache
write_repository_config(@new_path) write_repository_config(@new_path)
......
...@@ -17,7 +17,7 @@ ...@@ -17,7 +17,7 @@
- if project.archived - if project.archived
%span.badge.badge-warning archived %span.badge.badge-warning archived
.title .title
= link_to [:admin, project.namespace.becomes(Namespace), project] do = link_to(admin_namespace_project_path(project.namespace, project)) do
.dash-project-avatar .dash-project-avatar
.avatar-container.s40 .avatar-container.s40
= project_icon(project, alt: '', class: 'avatar project-avatar s40') = project_icon(project, alt: '', class: 'avatar project-avatar s40')
......
---
title: Stop dynamically creating project and namespace routes
merge_request: 20313
author:
type: performance
# See http://doc.gitlab.com/ce/development/migration_style_guide.html
# for more information on how to write migrations for GitLab.
class RemoveOrphanedRoutes < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
disable_ddl_transaction!
class Route < ActiveRecord::Base
self.table_name = 'routes'
include EachBatch
def self.orphaned_namespace_routes
where(source_type: 'Namespace')
.where('NOT EXISTS ( SELECT 1 FROM namespaces WHERE namespaces.id = routes.source_id )')
end
def self.orphaned_project_routes
where(source_type: 'Project')
.where('NOT EXISTS ( SELECT 1 FROM projects WHERE projects.id = routes.source_id )')
end
end
def up
# Some of these queries can take up to 10 seconds to run on GitLab.com,
# which is pretty close to our 15 second statement timeout. To ensure a
# smooth deployment procedure we disable the statement timeouts for this
# migration, just in case.
disable_statement_timeout
# On GitLab.com there are around 4000 orphaned project routes, and around
# 150 orphaned namespace routes.
[
Route.orphaned_project_routes,
Route.orphaned_namespace_routes
].each do |relation|
relation.each_batch(of: 1_000) do |batch|
batch.delete_all
end
end
end
def down
# There is no way to restore orphaned routes, and this doesn't make any
# sense anyway.
end
end
# 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
...@@ -55,10 +55,8 @@ describe Projects::PipelinesController do ...@@ -55,10 +55,8 @@ describe Projects::PipelinesController do
stub_feature_flags(ci_pipeline_persisted_stages: false) stub_feature_flags(ci_pipeline_persisted_stages: false)
end end
it 'returns JSON with serialized pipelines', :request_store do it 'returns JSON with serialized pipelines' do
queries = ActiveRecord::QueryRecorder.new do
get_pipelines_index_json get_pipelines_index_json
end
expect(response).to have_gitlab_http_status(:ok) expect(response).to have_gitlab_http_status(:ok)
expect(response).to match_response_schema('pipeline') expect(response).to match_response_schema('pipeline')
...@@ -73,8 +71,14 @@ describe Projects::PipelinesController do ...@@ -73,8 +71,14 @@ describe Projects::PipelinesController do
json_response.dig('pipelines', 0, 'details', 'stages').tap do |stages| json_response.dig('pipelines', 0, 'details', 'stages').tap do |stages|
expect(stages.count).to eq 3 expect(stages.count).to eq 3
end 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
end end
......
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
...@@ -12,16 +12,6 @@ describe Group, 'Routable' do ...@@ -12,16 +12,6 @@ describe Group, 'Routable' do
it { is_expected.to have_many(:redirect_routes).dependent(:destroy) } it { is_expected.to have_many(:redirect_routes).dependent(:destroy) }
end 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 describe 'Callbacks' do
it 'creates route record on create' do it 'creates route record on create' do
expect(group.route.path).to eq(group.path) expect(group.route.path).to eq(group.path)
...@@ -131,29 +121,6 @@ describe Group, 'Routable' do ...@@ -131,29 +121,6 @@ describe Group, 'Routable' do
it { expect(group.full_path).to eq(group.path) } it { expect(group.full_path).to eq(group.path) }
it { expect(nested_group.full_path).to eq("#{group.full_path}/#{nested_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 end
describe '#full_name' do describe '#full_name' do
......
...@@ -323,6 +323,16 @@ describe Namespace do ...@@ -323,6 +323,16 @@ describe Namespace do
parent.update(path: 'mygroup_new') 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(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(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}" expect(project_rugged(legacy_project_in_subgroup).config['gitlab.fullpath']).to eq "mygroup_new/mysubgroup/#{legacy_project_in_subgroup.path}"
......
...@@ -2958,8 +2958,6 @@ describe Project do ...@@ -2958,8 +2958,6 @@ describe Project do
expect(project).to receive(:expire_caches_before_rename) expect(project).to receive(:expire_caches_before_rename)
expect(project).to receive(:expires_full_path_cache)
project.rename_repo project.rename_repo
end end
...@@ -3119,8 +3117,6 @@ describe Project do ...@@ -3119,8 +3117,6 @@ describe Project do
expect(project).to receive(:expire_caches_before_rename) expect(project).to receive(:expire_caches_before_rename)
expect(project).to receive(:expires_full_path_cache)
project.rename_repo project.rename_repo
end end
......
...@@ -125,7 +125,7 @@ describe PipelineSerializer do ...@@ -125,7 +125,7 @@ describe PipelineSerializer do
it 'verifies number of queries', :request_store do it 'verifies number of queries', :request_store do
recorded = ActiveRecord::QueryRecorder.new { subject } 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) expect(recorded.cached_count).to eq(0)
end end
end end
...@@ -144,7 +144,7 @@ describe PipelineSerializer do ...@@ -144,7 +144,7 @@ describe PipelineSerializer do
# pipeline. With the same ref this check is cached but if refs are # pipeline. With the same ref this check is cached but if refs are
# different then there is an extra query per ref # different then there is an extra query per ref
# https://gitlab.com/gitlab-org/gitlab-ce/issues/46368 # 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) expect(recorded.cached_count).to eq(0)
end end
end end
......
...@@ -31,12 +31,6 @@ describe Projects::TransferService do ...@@ -31,12 +31,6 @@ describe Projects::TransferService do
transfer_project(project, user, group) transfer_project(project, user, group)
end 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 it 'invalidates the user\'s personal_project_count cache' do
expect(user).to receive(:invalidate_personal_projects_count) expect(user).to receive(:invalidate_personal_projects_count)
......
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