From b9fe665d2539f53fb86771383e18f9045db52414 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Mon, 14 Mar 2022 19:45:29 +0000 Subject: Add latest changes from gitlab-org/gitlab@14-8-stable-ee --- app/models/user.rb | 2 +- app/services/users/destroy_service.rb | 2 +- .../users/migrate_to_ghost_user_service.rb | 13 +++++++++-- config.ru | 10 -------- config/initializers/7_prometheus_metrics.rb | 26 ++++++++++++++------- config/initializers/zz_metrics.rb | 4 ++++ doc/user/project/description_templates.md | 10 ++++---- doc/user/project/settings/index.md | 6 ++++- lib/gitlab/cleanup/orphan_job_artifact_files.rb | 3 +++ lib/gitlab/cluster/lifecycle_events.rb | 19 +++------------ package.json | 2 +- .../cleanup/orphan_job_artifact_files_spec.rb | 27 ++++++++++++++++++++-- spec/services/users/destroy_service_spec.rb | 14 ++++++----- .../users/migrate_to_ghost_user_service_spec.rb | 7 +++--- ...igrate_to_ghost_user_service_shared_examples.rb | 12 ++++++++++ yarn.lock | 18 +++++++-------- 16 files changed, 111 insertions(+), 64 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index 74832bff9ac..9cd238904ff 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -390,7 +390,7 @@ class User < ApplicationRecord # rubocop: disable CodeReuse/ServiceClass # Ideally we should not call a service object here but user.block - # is also bcalled by Users::MigrateToGhostUserService which references + # is also called by Users::MigrateToGhostUserService which references # this state transition object in order to do a rollback. # For this reason the tradeoff is to disable this cop. after_transition any => :blocked do |user| diff --git a/app/services/users/destroy_service.rb b/app/services/users/destroy_service.rb index 4ec875098fa..46eec082125 100644 --- a/app/services/users/destroy_service.rb +++ b/app/services/users/destroy_service.rb @@ -54,7 +54,7 @@ module Users yield(user) if block_given? - MigrateToGhostUserService.new(user).execute unless options[:hard_delete] + MigrateToGhostUserService.new(user).execute(hard_delete: options[:hard_delete]) response = Snippets::BulkDestroyService.new(current_user, user.snippets).execute(options) raise DestroyError, response.message if response.error? diff --git a/app/services/users/migrate_to_ghost_user_service.rb b/app/services/users/migrate_to_ghost_user_service.rb index 515d7821416..575614e8743 100644 --- a/app/services/users/migrate_to_ghost_user_service.rb +++ b/app/services/users/migrate_to_ghost_user_service.rb @@ -10,14 +10,21 @@ module Users class MigrateToGhostUserService extend ActiveSupport::Concern - attr_reader :ghost_user, :user + attr_reader :ghost_user, :user, :hard_delete def initialize(user) @user = user @ghost_user = User.ghost end - def execute + # If an admin attempts to hard delete a user, in some cases associated + # records may have a NOT NULL constraint on the user ID that prevent that record + # from being destroyed. In such situations we must assign the record to the ghost user. + # Passing in `hard_delete: true` will ensure these records get assigned to + # the ghost user before the user is destroyed. Other associated records will be destroyed. + # letting the other associated records be destroyed. + def execute(hard_delete: false) + @hard_delete = hard_delete transition = user.block_transition # Block the user before moving records to prevent a data race. @@ -46,6 +53,8 @@ module Users private def migrate_records + return if hard_delete + migrate_issues migrate_merge_requests migrate_notes diff --git a/config.ru b/config.ru index c74a49cd0e2..f07c25f503a 100644 --- a/config.ru +++ b/config.ru @@ -4,17 +4,7 @@ require ::File.expand_path('../config/environment', __FILE__) -def master_process? - Prometheus::PidProvider.worker_id == 'puma_master' -end - warmup do |app| - # The following is necessary to ensure stale Prometheus metrics don't accumulate over time. - # It needs to be done as early as here to ensure metrics files aren't deleted. - # After we hit our app in `warmup`, first metrics and corresponding files already being created, - # for example in `lib/gitlab/metrics/requests_rack_middleware.rb`. - Prometheus::CleanupMultiprocDirService.new.execute if master_process? - client = Rack::MockRequest.new(app) client.get('/') end diff --git a/config/initializers/7_prometheus_metrics.rb b/config/initializers/7_prometheus_metrics.rb index 17ce2a30d66..c55b074f9c8 100644 --- a/config/initializers/7_prometheus_metrics.rb +++ b/config/initializers/7_prometheus_metrics.rb @@ -13,6 +13,24 @@ def prometheus_default_multiproc_dir end end +def puma_metrics_server_process? + Prometheus::PidProvider.worker_id == 'puma_master' +end + +def sidekiq_metrics_server_process? + Gitlab::Runtime.sidekiq? && (!ENV['SIDEKIQ_WORKER_ID'] || ENV['SIDEKIQ_WORKER_ID'] == '0') +end + +if puma_metrics_server_process? || sidekiq_metrics_server_process? + # The following is necessary to ensure stale Prometheus metrics don't accumulate over time. + # It needs to be done as early as here to ensure metrics files aren't deleted. + # After we hit our app in `warmup`, first metrics and corresponding files already being created, + # for example in `lib/gitlab/metrics/requests_rack_middleware.rb`. + Prometheus::CleanupMultiprocDirService.new.execute + + ::Prometheus::Client.reinitialize_on_pid_change(force: true) +end + ::Prometheus::Client.configure do |config| config.logger = Gitlab::AppLogger @@ -47,21 +65,13 @@ if Gitlab::Runtime.sidekiq? && (!ENV['SIDEKIQ_WORKER_ID'] || ENV['SIDEKIQ_WORKER end if !Rails.env.test? && Gitlab::Metrics.prometheus_metrics_enabled? - # When running Puma in a Single mode, `on_master_start` and `on_worker_start` are the same. - # Thus, we order these events to run `reinitialize_on_pid_change` with `force: true` first. Gitlab::Cluster::LifecycleEvents.on_master_start do - ::Prometheus::Client.reinitialize_on_pid_change(force: true) - if Gitlab::Runtime.puma? Gitlab::Metrics::Samplers::PumaSampler.instance.start end Gitlab::Metrics.gauge(:deployments, 'GitLab Version', {}, :max).set({ version: Gitlab::VERSION, revision: Gitlab.revision }, 1) - if Gitlab::Runtime.web_server? - Gitlab::Metrics::RequestsRackMiddleware.initialize_metrics - end - Gitlab::Ci::Parsers.instrument! rescue IOError => e Gitlab::ErrorTracking.track_exception(e) diff --git a/config/initializers/zz_metrics.rb b/config/initializers/zz_metrics.rb index 7fa71225aae..88469d2cdef 100644 --- a/config/initializers/zz_metrics.rb +++ b/config/initializers/zz_metrics.rb @@ -36,6 +36,10 @@ if Gitlab::Metrics.enabled? && !Rails.env.test? && !(Rails.env.development? && d config.middleware.use(Gitlab::Metrics::ElasticsearchRackMiddleware) end + if Gitlab::Runtime.puma? + Gitlab::Metrics::RequestsRackMiddleware.initialize_metrics + end + GC::Profiler.enable module TrackNewRedisConnections diff --git a/doc/user/project/description_templates.md b/doc/user/project/description_templates.md index 539f5230063..4f8cfad1444 100644 --- a/doc/user/project/description_templates.md +++ b/doc/user/project/description_templates.md @@ -129,8 +129,9 @@ Prerequisites: To set a default description template for merge requests, either: -- [Create a merge request template](#create-a-merge-request-template) named `Default.md` and save it in `.gitlab/merge_request_templates/`. - This will not overwrite the default template if one has been set in the project settings. +- [Create a merge request template](#create-a-merge-request-template) named `Default.md` or `default.md` + and save it in `.gitlab/merge_request_templates/`. + This doesn't overwrite the default template if one has been set in the project settings. - Users on GitLab Premium and higher: set the default template in project settings: 1. On the top bar, select **Menu > Projects** and find your project. @@ -141,8 +142,9 @@ To set a default description template for merge requests, either: To set a default description template for issues, either: -- [Create an issue template](#create-an-issue-template) named `Default.md` and save it in `.gitlab/issue_templates/`. - This will not overwrite the default template if one has been set in the project settings. +- [Create an issue template](#create-an-issue-template) named `Default.md` or `default.md` + and save it in `.gitlab/issue_templates/`. + This doesn't overwrite the default template if one has been set in the project settings. - Users on GitLab Premium and higher: set the default template in project settings: 1. On the top bar, select **Menu > Projects** and find your project. diff --git a/doc/user/project/settings/index.md b/doc/user/project/settings/index.md index 4c6be3511f2..12d75551aac 100644 --- a/doc/user/project/settings/index.md +++ b/doc/user/project/settings/index.md @@ -217,7 +217,11 @@ cannot change them: This ensures that your job uses the settings you intend and that they are not overridden by project-level pipelines. -##### Avoid parent and child pipelines +##### Avoid parent and child pipelines in GitLab 14.7 and earlier + +NOTE: +This advice does not apply to GitLab 14.8 and later because [a fix](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/78878) added +compatibility for combining compliance pipelines, and parent and child pipelines. Compliance pipelines start on the run of _every_ pipeline in a relevant project. This means that if a pipeline in the relevant project triggers a child pipeline, the compliance pipeline runs first. This can trigger the parent pipeline, instead of the child pipeline. diff --git a/lib/gitlab/cleanup/orphan_job_artifact_files.rb b/lib/gitlab/cleanup/orphan_job_artifact_files.rb index 05dfdcd4486..90123b9d000 100644 --- a/lib/gitlab/cleanup/orphan_job_artifact_files.rb +++ b/lib/gitlab/cleanup/orphan_job_artifact_files.rb @@ -99,6 +99,9 @@ module Gitlab # ^--+--+- components of hashed storage project path cmd += %w[-mindepth 6 -maxdepth 6] + # Intentionally exclude pipeline artifacts which match the same path + cmd += %w[-not -path */pipelines/*] + # Artifact directories are named on their ID cmd += %w[-type d] diff --git a/lib/gitlab/cluster/lifecycle_events.rb b/lib/gitlab/cluster/lifecycle_events.rb index 6159fb0a811..e423d1f17da 100644 --- a/lib/gitlab/cluster/lifecycle_events.rb +++ b/lib/gitlab/cluster/lifecycle_events.rb @@ -70,7 +70,7 @@ module Gitlab # Hook registration methods (called from initializers) # def on_worker_start(&block) - if in_clustered_environment? + if in_clustered_puma? # Defer block execution (@worker_start_hooks ||= []) << block else @@ -101,7 +101,7 @@ module Gitlab end def on_master_start(&block) - if in_clustered_environment? + if in_clustered_puma? on_before_fork(&block) else on_worker_start(&block) @@ -158,21 +158,8 @@ module Gitlab end end - def in_clustered_environment? - # Sidekiq doesn't fork - return false if Gitlab::Runtime.sidekiq? - - # Puma sometimes forks - return true if in_clustered_puma? - - # Default assumption is that we don't fork - false - end - def in_clustered_puma? - return false unless Gitlab::Runtime.puma? - - @puma_options && @puma_options[:workers] && @puma_options[:workers] > 0 + Gitlab::Runtime.puma? && @puma_options && @puma_options[:workers] && @puma_options[:workers] > 0 end end end diff --git a/package.json b/package.json index 582d86454b8..ddaf4f60af7 100644 --- a/package.json +++ b/package.json @@ -155,6 +155,7 @@ "pikaday": "^1.8.0", "popper.js": "^1.16.1", "portal-vue": "^2.1.7", + "postcss": "8.4.5", "prismjs": "^1.21.0", "prosemirror-markdown": "1.6.0", "prosemirror-model": "^1.16.1", @@ -241,7 +242,6 @@ "miragejs": "^0.1.40", "mock-apollo-client": "1.2.0", "nodemon": "^2.0.4", - "postcss": "^8.4.5", "prettier": "2.2.1", "prosemirror-schema-basic": "^1.1.2", "prosemirror-schema-list": "^1.1.6", diff --git a/spec/lib/gitlab/cleanup/orphan_job_artifact_files_spec.rb b/spec/lib/gitlab/cleanup/orphan_job_artifact_files_spec.rb index b5adb603dab..e6ef2d8a541 100644 --- a/spec/lib/gitlab/cleanup/orphan_job_artifact_files_spec.rb +++ b/spec/lib/gitlab/cleanup/orphan_job_artifact_files_spec.rb @@ -34,10 +34,33 @@ RSpec.describe Gitlab::Cleanup::OrphanJobArtifactFiles do cleanup.run! end - it 'finds artifacts on disk' do + it 'finds job artifacts on disk' do artifact = create(:ci_job_artifact, :archive) + artifact_directory = artifact.file.relative_path.to_s.split('/')[0...6].join('/') + + cleaned = [] + + expect(cleanup).to receive(:find_artifacts).and_wrap_original do |original_method, *args, &block| + original_method.call(*args) { |dir| cleaned << dir } + end + + cleanup.run! + + expect(cleaned).to include(/#{artifact_directory}/) + end + + it 'does not find pipeline artifacts on disk' do + artifact = create(:ci_pipeline_artifact, :with_coverage_report) + # using 0...6 to match the -min/maxdepth 6 strictly, since this is one directory + # deeper than job artifacts, and .dirname would not match + artifact_directory = artifact.file.relative_path.to_s.split('/')[0...6].join('/') + + expect(cleanup).to receive(:find_artifacts).and_wrap_original do |original_method, *args, &block| + # this can either _not_ yield at all, or yield with any other file + # except the one that we're explicitly excluding + original_method.call(*args) { |path| expect(path).not_to match(artifact_directory) } + end - expect(cleanup).to receive(:find_artifacts).and_yield(artifact.file.path) cleanup.run! end diff --git a/spec/services/users/destroy_service_spec.rb b/spec/services/users/destroy_service_spec.rb index 76b84e3b4ab..602db66dba1 100644 --- a/spec/services/users/destroy_service_spec.rb +++ b/spec/services/users/destroy_service_spec.rb @@ -215,8 +215,8 @@ RSpec.describe Users::DestroyService do end end - context "migrating associated records" do - let!(:issue) { create(:issue, author: user) } + context 'migrating associated records' do + let!(:issue) { create(:issue, author: user) } it 'delegates to the `MigrateToGhostUser` service to move associated records to the ghost user' do expect_any_instance_of(Users::MigrateToGhostUserService).to receive(:execute).once.and_call_original @@ -226,12 +226,14 @@ RSpec.describe Users::DestroyService do expect(issue.reload.author).to be_ghost end - it 'does not run `MigrateToGhostUser` if hard_delete option is given' do - expect_any_instance_of(Users::MigrateToGhostUserService).not_to receive(:execute) + context 'when hard_delete option is given' do + it 'will not ghost certain records' do + expect_any_instance_of(Users::MigrateToGhostUserService).to receive(:execute).once.and_call_original - service.execute(user, hard_delete: true) + service.execute(user, hard_delete: true) - expect(Issue.exists?(issue.id)).to be_falsy + expect(Issue.exists?(issue.id)).to be_falsy + end end end diff --git a/spec/services/users/migrate_to_ghost_user_service_spec.rb b/spec/services/users/migrate_to_ghost_user_service_spec.rb index c36889f20ec..073ebaae5b0 100644 --- a/spec/services/users/migrate_to_ghost_user_service_spec.rb +++ b/spec/services/users/migrate_to_ghost_user_service_spec.rb @@ -3,9 +3,10 @@ require 'spec_helper' RSpec.describe Users::MigrateToGhostUserService do - let!(:user) { create(:user) } - let!(:project) { create(:project, :repository) } - let(:service) { described_class.new(user) } + let!(:user) { create(:user) } + let!(:project) { create(:project, :repository) } + let(:service) { described_class.new(user) } + let(:always_ghost) { false } context "migrating a user's associated records to the ghost user" do context 'issues' do diff --git a/spec/support/services/migrate_to_ghost_user_service_shared_examples.rb b/spec/support/services/migrate_to_ghost_user_service_shared_examples.rb index 2fbc01a9195..1e291a90163 100644 --- a/spec/support/services/migrate_to_ghost_user_service_shared_examples.rb +++ b/spec/support/services/migrate_to_ghost_user_service_shared_examples.rb @@ -42,6 +42,18 @@ RSpec.shared_examples "migrating a deleted user's associated records to the ghos end end + it 'will only migrate specific records during a hard_delete' do + service.execute(hard_delete: true) + + migrated_record = record_class.find_by_id(record.id) + + check_user = always_ghost ? User.ghost : user + + migrated_fields.each do |field| + expect(migrated_record.public_send(field)).to eq(check_user) + end + end + context "race conditions" do context "when #{record_class_name} migration fails and is rolled back" do before do diff --git a/yarn.lock b/yarn.lock index dc2080f9bfb..b05b46afdc5 100644 --- a/yarn.lock +++ b/yarn.lock @@ -9470,15 +9470,7 @@ postcss-value-parser@^4.1.0, postcss-value-parser@^4.2.0: resolved "https://registry.yarnpkg.com/postcss-value-parser/-/postcss-value-parser-4.2.0.tgz#723c09920836ba6d3e5af019f92bc0971c02e514" integrity sha512-1NNCs6uurfkVbeXG4S8JFT9t19m45ICnif8zWLd5oPSZ50QnwMfK+H3jv408d4jw/7Bttv5axS5IiHoLaVNHeQ== -postcss@^7.0.14, postcss@^7.0.5, postcss@^7.0.6: - version "7.0.39" - resolved "https://registry.yarnpkg.com/postcss/-/postcss-7.0.39.tgz#9624375d965630e2e1f2c02a935c82a59cb48309" - integrity sha512-yioayjNbHn6z1/Bywyb2Y4s3yvDAeXGOyxqD+LnVOinq6Mdmd++SW2wUNVzavyyHxd6+DxzWGIuosg6P1Rj8uA== - dependencies: - picocolors "^0.2.1" - source-map "^0.6.1" - -postcss@^8.2.1, postcss@^8.4.5: +postcss@8.4.5, postcss@^8.2.1, postcss@^8.4.5: version "8.4.5" resolved "https://registry.yarnpkg.com/postcss/-/postcss-8.4.5.tgz#bae665764dfd4c6fcc24dc0fdf7e7aa00cc77f95" integrity sha512-jBDboWM8qpaqwkMwItqTQTiFikhs/67OYVvblFFTM7MrZjt6yMKd6r2kgXizEbTTljacm4NldIlZnhbjr84QYg== @@ -9487,6 +9479,14 @@ postcss@^8.2.1, postcss@^8.4.5: picocolors "^1.0.0" source-map-js "^1.0.1" +postcss@^7.0.14, postcss@^7.0.5, postcss@^7.0.6: + version "7.0.39" + resolved "https://registry.yarnpkg.com/postcss/-/postcss-7.0.39.tgz#9624375d965630e2e1f2c02a935c82a59cb48309" + integrity sha512-yioayjNbHn6z1/Bywyb2Y4s3yvDAeXGOyxqD+LnVOinq6Mdmd++SW2wUNVzavyyHxd6+DxzWGIuosg6P1Rj8uA== + dependencies: + picocolors "^0.2.1" + source-map "^0.6.1" + prelude-ls@^1.2.1: version "1.2.1" resolved "https://registry.yarnpkg.com/prelude-ls/-/prelude-ls-1.2.1.tgz#debc6489d7a6e6b0e7611888cec880337d316396" -- cgit v1.2.3