diff options
30 files changed, 328 insertions, 208 deletions
diff --git a/.gitignore b/.gitignore index 234593b944e..ad7595dc7f2 100644 --- a/.gitignore +++ b/.gitignore @@ -78,6 +78,7 @@ eslint-report.html /test_results/ /deprecations/ /knapsack/ +/query_recorder/ /rspec_flaky/ /rspec/ /locale/**/LC_MESSAGES diff --git a/.gitlab/ci/package-and-test/main.gitlab-ci.yml b/.gitlab/ci/package-and-test/main.gitlab-ci.yml index a434518537a..34c95d7637d 100644 --- a/.gitlab/ci/package-and-test/main.gitlab-ci.yml +++ b/.gitlab/ci/package-and-test/main.gitlab-ci.yml @@ -577,7 +577,7 @@ ee:importers: extends: .qa variables: QA_SCENARIO: Test::Integration::Import - QA_ALLOW_LOCAL_REQUESTS: "true" + GITLAB_QA_OPTS: --set-feature-flags bulk_import_projects=enabled rules: - !reference [.rules:test:qa, rules] - if: $QA_SUITES =~ /Test::Integration::Import/ diff --git a/.gitlab/ci/rails/shared.gitlab-ci.yml b/.gitlab/ci/rails/shared.gitlab-ci.yml index d47bac5e433..ba4bde9d07b 100644 --- a/.gitlab/ci/rails/shared.gitlab-ci.yml +++ b/.gitlab/ci/rails/shared.gitlab-ci.yml @@ -68,6 +68,7 @@ include: - crystalball/ - deprecations/ - knapsack/ + - query_recorder/ - rspec/ - tmp/capybara/ - log/*.log diff --git a/doc/administration/logs/index.md b/doc/administration/logs/index.md index b0631c52a47..65b45639a41 100644 --- a/doc/administration/logs/index.md +++ b/doc/administration/logs/index.md @@ -161,10 +161,12 @@ seconds: - `gitaly_duration_s`: Total time by Gitaly calls - `gitaly_calls`: Total number of calls made to Gitaly - `redis_calls`: Total number of calls made to Redis +- `redis_cross_slot_calls`: Total number of cross-slot calls made to Redis - `redis_duration_s`: Total time to retrieve data from Redis - `redis_read_bytes`: Total bytes read from Redis - `redis_write_bytes`: Total bytes written to Redis - `redis_<instance>_calls`: Total number of calls made to a Redis instance +- `redis_<instance>_cross_slot_calls`: Total number of cross-slot calls made to a Redis instance - `redis_<instance>_duration_s`: Total time to retrieve data from a Redis instance - `redis_<instance>_read_bytes`: Total bytes read from a Redis instance - `redis_<instance>_write_bytes`: Total bytes written to a Redis instance diff --git a/doc/user/project/pages/public_folder.md b/doc/user/project/pages/public_folder.md index a19e296b954..8c9f1cbec86 100644 --- a/doc/user/project/pages/public_folder.md +++ b/doc/user/project/pages/public_folder.md @@ -2,40 +2,39 @@ description: 'Learn how to configure the build output folder for the most common static site generators' stage: Create -group: Incubation +group: Editor info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/product/ux/technical-writing/#assignments --- # Configure the public files folder **(FREE)** -GitLab Pages requires all files you intend to be available in the published website to -be in a root-level folder called `public`. This page describe how -to set this up for some common static site generators. +All the files that should be accessible by the browser must be in a root-level folder called `public`. -## Guide by framework +Follow these instructions to configure the `public` folder +for the following frameworks. -### Eleventy +## Eleventy -For Eleventy, you should either: +For Eleventy, you should do one of the following: -1. Add the `--output=public` flag in Eleventy's build commands, for example: +- Add the `--output=public` flag in Eleventy's build commands, for example: - `npx @11ty/eleventy --input=path/to/sourcefiles --output=public` + `npx @11ty/eleventy --input=path/to/sourcefiles --output=public` -1. Add the following to your `.eleventy.js` file: +- Add the following to your `.eleventy.js` file: - ```javascript - // .eleventy.js - module.exports = function(eleventyConfig) { - return { - dir: { - output: "public" - } - } - }; - ``` + ```javascript + // .eleventy.js + module.exports = function(eleventyConfig) { + return { + dir: { + output: "public" + } + } + }; + ``` -### Astro +## Astro By default, Astro uses the `public` folder to store static assets. For GitLab Pages, rename that folder to a collision-free alternative first: @@ -65,11 +64,11 @@ rename that folder to a collision-free alternative first: }); ``` -### SvelteKit +## SvelteKit NOTE: GitLab Pages supports only static sites. For SvelteKit, -we recommend using [`adapter-static`](https://kit.svelte.dev/docs/adapters#supported-environments-static-sites). +you can use [`adapter-static`](https://kit.svelte.dev/docs/adapters#supported-environments-static-sites). When using `adapter-static`, add the following to your `svelte.config.js`: @@ -86,11 +85,11 @@ export default { }; ``` -### Next.js +## Next.js NOTE: -GitLab Pages supports only static sites. For Next.js, we -recommend using Next's [Static HTML export functionality](https://nextjs.org/docs/advanced-features/static-html-export) +GitLab Pages supports only static sites. For Next.js, you can use +Next's [Static HTML export functionality](https://nextjs.org/docs/advanced-features/static-html-export). Use the `-o public` flag after `next export` as the build command, for example: @@ -118,7 +117,7 @@ GitLab Pages supports only static sites. 1. Configure your Nuxt.js application for [Static Site Generation](https://nuxtjs.org/docs/features/deployment-targets/#static-hosting). -### Vite +## Vite Update your `vite.config.js` to include the following: @@ -131,7 +130,7 @@ export default { } ``` -### Webpack +## Webpack Update your `webpack.config.js` to include the following: @@ -147,9 +146,9 @@ module.exports = { ## Should you commit the `public` folder? Not necessarily. However, when the GitLab Pages deploy pipeline runs, it looks -for an [artifact](../../../ci/pipelines/job_artifacts.md) of that name. So +for an [artifact](../../../ci/pipelines/job_artifacts.md) of that name. If you set up a job that creates the `public` folder before deploy, such as by running `npm run build`, committing the folder isn't required. If you prefer to build your site locally, you can commit the `public` folder and -omit the build step during the job, instead. +omit the build step during the job instead. diff --git a/lib/gitlab/database/query_analyzers/query_recorder.rb b/lib/gitlab/database/query_analyzers/query_recorder.rb index 88fe829c3d2..6fceaeea256 100644 --- a/lib/gitlab/database/query_analyzers/query_recorder.rb +++ b/lib/gitlab/database/query_analyzers/query_recorder.rb @@ -4,7 +4,7 @@ module Gitlab module Database module QueryAnalyzers class QueryRecorder < Base - LOG_FILE = 'rspec/query_recorder.ndjson' + LOG_PATH = 'query_recorder/' class << self def raw? @@ -24,11 +24,14 @@ module Gitlab log_query(payload) end + def log_file + Rails.root.join(LOG_PATH, "#{ENV.fetch('CI_JOB_NAME_SLUG', 'rspec')}.ndjson") + end + private def log_query(payload) - log_path = Rails.root.join(LOG_FILE) - log_dir = File.dirname(log_path) + log_dir = Rails.root.join(LOG_PATH) # Create log directory if it does not exist since it is only created # ahead of time by certain CI jobs @@ -36,7 +39,7 @@ module Gitlab log_line = "#{Gitlab::Json.dump(payload)}\n" - File.write(log_path, log_line, mode: 'a') + File.write(log_file, log_line, mode: 'a') end end end diff --git a/lib/gitlab/instrumentation/redis.rb b/lib/gitlab/instrumentation/redis.rb index a371930621d..da0061a4d99 100644 --- a/lib/gitlab/instrumentation/redis.rb +++ b/lib/gitlab/instrumentation/redis.rb @@ -39,7 +39,7 @@ module Gitlab end end - %i[get_request_count query_time read_bytes write_bytes].each do |method| + %i[get_request_count get_cross_slot_request_count query_time read_bytes write_bytes].each do |method| define_method method do STORAGES.sum(&method) end diff --git a/lib/gitlab/instrumentation/redis_base.rb b/lib/gitlab/instrumentation/redis_base.rb index 268c6cdf459..91ee5885ed2 100644 --- a/lib/gitlab/instrumentation/redis_base.rb +++ b/lib/gitlab/instrumentation/redis_base.rb @@ -45,6 +45,11 @@ module Gitlab ::RequestStore[write_bytes_key] += num_bytes end + def increment_cross_slot_request_count(amount = 1) + ::RequestStore[cross_slots_key] ||= 0 + ::RequestStore[cross_slots_key] += amount + end + def get_request_count ::RequestStore[request_count_key] || 0 end @@ -61,6 +66,10 @@ module Gitlab ::RequestStore[call_details_key] ||= [] end + def get_cross_slot_request_count + ::RequestStore[cross_slots_key] || 0 + end + def query_time query_time = ::RequestStore[call_duration_key] || 0 query_time.round(::Gitlab::InstrumentationHelper::DURATION_PRECISION) @@ -68,6 +77,11 @@ module Gitlab def redis_cluster_validate!(commands) ::Gitlab::Instrumentation::RedisClusterValidator.validate!(commands) if @redis_cluster_validation + true + rescue ::Gitlab::Instrumentation::RedisClusterValidator::CrossSlotError + raise if Rails.env.development? || Rails.env.test? # raise in test environments to catch violations + + false end def enable_redis_cluster_validation @@ -122,6 +136,10 @@ module Gitlab strong_memoize(:call_details_key) { build_key(:redis_call_details) } end + def cross_slots_key + strong_memoize(:cross_slots_key) { build_key(:redis_cross_slot_request_count) } + end + def build_key(namespace) "#{storage_key}_#{namespace}" end diff --git a/lib/gitlab/instrumentation/redis_cluster_validator.rb b/lib/gitlab/instrumentation/redis_cluster_validator.rb index 36d3e088956..a928d626f38 100644 --- a/lib/gitlab/instrumentation/redis_cluster_validator.rb +++ b/lib/gitlab/instrumentation/redis_cluster_validator.rb @@ -184,7 +184,6 @@ module Gitlab class << self def validate!(commands) - return unless Rails.env.development? || Rails.env.test? return if allow_cross_slot_commands? return if commands.empty? diff --git a/lib/gitlab/instrumentation/redis_interceptor.rb b/lib/gitlab/instrumentation/redis_interceptor.rb index f19279df2fe..1700f58ba13 100644 --- a/lib/gitlab/instrumentation/redis_interceptor.rb +++ b/lib/gitlab/instrumentation/redis_interceptor.rb @@ -33,7 +33,10 @@ module Gitlab def instrument_call(commands) start = Gitlab::Metrics::System.monotonic_time # must come first so that 'start' is always defined instrumentation_class.instance_count_request(commands.size) - instrumentation_class.redis_cluster_validate!(commands) + + if ::RequestStore.active? && !instrumentation_class.redis_cluster_validate!(commands) + instrumentation_class.increment_cross_slot_request_count + end yield rescue ::Redis::BaseError => ex diff --git a/lib/gitlab/instrumentation/redis_payload.rb b/lib/gitlab/instrumentation/redis_payload.rb index 86a6525c8d0..3a9835410af 100644 --- a/lib/gitlab/instrumentation/redis_payload.rb +++ b/lib/gitlab/instrumentation/redis_payload.rb @@ -20,6 +20,7 @@ module Gitlab { "#{key_prefix}_calls": -> { get_request_count }, + "#{key_prefix}_cross_slot_calls": -> { get_cross_slot_request_count }, "#{key_prefix}_duration_s": -> { query_time }, "#{key_prefix}_read_bytes": -> { read_bytes }, "#{key_prefix}_write_bytes": -> { write_bytes } diff --git a/qa/qa/page/profile/two_factor_auth.rb b/qa/qa/page/profile/two_factor_auth.rb index 0436b726911..68154ee1852 100644 --- a/qa/qa/page/profile/two_factor_auth.rb +++ b/qa/qa/page/profile/two_factor_auth.rb @@ -25,7 +25,7 @@ module QA def click_configure_it_later_button # TO DO: Investigate why button does not appear sometimes: # https://gitlab.com/gitlab-org/gitlab/-/issues/382698 - return unless has_element?(:configure_it_later_button, wait: 20) + return unless has_element?(:configure_it_later_button, wait: 40) click_element :configure_it_later_button wait_until(max_duration: 10, message: "Waiting for create a group page") do diff --git a/qa/qa/resource/merge_request.rb b/qa/qa/resource/merge_request.rb index fcfda106523..d1d99393ca2 100644 --- a/qa/qa/resource/merge_request.rb +++ b/qa/qa/resource/merge_request.rb @@ -199,6 +199,10 @@ module QA :source_project_id, :target_project_id, :merge_status, + # we consider mr to still be the same even if users changed + :author, + :reviewers, + :assignees, # these can differ depending on user fetching mr :user, :subscribed, diff --git a/qa/qa/specs/features/api/1_manage/migration/gitlab_migration_issue_spec.rb b/qa/qa/specs/features/api/1_manage/migration/gitlab_migration_issue_spec.rb index 887eeda51e3..027628c35f5 100644 --- a/qa/qa/specs/features/api/1_manage/migration/gitlab_migration_issue_spec.rb +++ b/qa/qa/specs/features/api/1_manage/migration/gitlab_migration_issue_spec.rb @@ -7,12 +7,18 @@ module QA let!(:source_issue) do Resource::Issue.fabricate_via_api! do |issue| - issue.api_client = api_client + issue.api_client = source_admin_api_client issue.project = source_project issue.labels = %w[label_one label_two] end end + let(:source_issue_comments) do + source_issue.comments.map do |note| + { **note.except(:id, :noteable_id), author: note[:author].except(:web_url) } + end + end + let(:imported_issues) { imported_projects.first.issues } let(:imported_issue) do @@ -24,6 +30,12 @@ module QA end end + let(:imported_issue_comments) do + imported_issue.comments.map do |note| + { **note.except(:id, :noteable_id), author: note[:author].except(:web_url) } + end + end + context 'with project issues' do let!(:source_comment) { source_issue.add_comment(body: 'This is a test comment!') } @@ -35,17 +47,16 @@ module QA ) do expect_import_finished expect(imported_issues.count).to eq(1) - - aggregate_failures do - expect(imported_issue).to eq(source_issue.reload!) - - expect(imported_comments.count).to eq(1) - expect(imported_comments.first&.fetch(:body)).to include(source_comment[:body]) - end + expect(imported_issue).to eq(source_issue.reload!) + expect(imported_issue_comments).to match_array(source_issue_comments) end end - context "with designs" do + # we can't fabricate things in source instance via UI + context "with designs", quarantine: { + type: :broken, + issue: 'https://gitlab.com/gitlab-org/gitlab/-/issues/366592' + } do let!(:source_design) do Flow::Login.sign_in(as: user) diff --git a/qa/qa/specs/features/api/1_manage/migration/gitlab_migration_members_spec.rb b/qa/qa/specs/features/api/1_manage/migration/gitlab_migration_members_spec.rb index 07e54ead9c8..51dd2098a22 100644 --- a/qa/qa/specs/features/api/1_manage/migration/gitlab_migration_members_spec.rb +++ b/qa/qa/specs/features/api/1_manage/migration/gitlab_migration_members_spec.rb @@ -5,32 +5,30 @@ module QA describe 'Gitlab migration', product_group: :import do include_context 'with gitlab project migration' - let(:member) do + let!(:source_member) do + Resource::User.fabricate_via_api! do |usr| + usr.api_client = source_admin_api_client + end.tap(&:set_public_email) + end + + let!(:target_member) do Resource::User.fabricate_via_api! do |usr| usr.api_client = admin_api_client - usr.hard_delete_on_api_removal = true - end + usr.email = source_member.email + end.tap(&:set_public_email) end let(:imported_group_member) do - imported_group.reload!.list_members.find { |usr| usr['username'] == member.username } + imported_group.reload!.list_members.find { |usr| usr['username'] == target_member.username } end let(:imported_project_member) do - imported_project.reload!.list_members.find { |usr| usr['username'] == member.username } - end - - before do - member.set_public_email - end - - after do - member.remove_via_api! + imported_project.reload!.list_members.find { |usr| usr['username'] == target_member.username } end context 'with group member' do before do - source_group.add_member(member, Resource::Members::AccessLevel::DEVELOPER) + source_group.add_member(source_member, Resource::Members::AccessLevel::DEVELOPER) end it( @@ -50,7 +48,7 @@ module QA context 'with project member' do before do - source_project.add_member(member, Resource::Members::AccessLevel::DEVELOPER) + source_project.add_member(source_member, Resource::Members::AccessLevel::DEVELOPER) end it( diff --git a/qa/qa/specs/features/api/1_manage/migration/gitlab_migration_mr_spec.rb b/qa/qa/specs/features/api/1_manage/migration/gitlab_migration_mr_spec.rb index f44786939dc..94ed58cfd72 100644 --- a/qa/qa/specs/features/api/1_manage/migration/gitlab_migration_mr_spec.rb +++ b/qa/qa/specs/features/api/1_manage/migration/gitlab_migration_mr_spec.rb @@ -7,57 +7,64 @@ module QA let!(:source_project_with_readme) { true } - # We create additional user so that object being migrated is not owned by the user doing migration - let!(:other_user) do - Resource::User - .fabricate_via_api! { |usr| usr.api_client = admin_api_client } - .tap do |usr| - usr.set_public_email - source_project.add_member(usr, Resource::Members::AccessLevel::MAINTAINER) - end + let!(:source_mr_reviewer) do + reviewer = Resource::User.fabricate_via_api! do |usr| + usr.api_client = source_admin_api_client + usr.username = "source-reviewer-#{SecureRandom.hex(6)}" + end + reviewer.tap do |usr| + usr.set_public_email + source_project.add_member(usr, Resource::Members::AccessLevel::MAINTAINER) + end end let!(:source_mr) do Resource::MergeRequest.fabricate_via_api! do |mr| mr.project = source_project - mr.api_client = Runtime::API::Client.new(user: other_user) - mr.reviewer_ids = [other_user.id] + mr.api_client = source_admin_api_client + mr.reviewer_ids = [source_mr_reviewer.id] end end - let!(:source_comment) { source_mr.add_comment(body: 'This is a test comment!') } + let!(:mr_reviewer) do + Resource::User.fabricate_via_api! do |usr| + usr.api_client = admin_api_client + usr.email = source_mr_reviewer.email + end.tap(&:set_public_email) + end - let(:imported_mrs) { imported_project.merge_requests } - let(:imported_mr_comments) { imported_mr.comments.map { |note| note.except(:id, :noteable_id) } } - let(:source_mr_comments) { source_mr.comments.map { |note| note.except(:id, :noteable_id) } } + let!(:source_mr_reviewers) { [source_mr_reviewer.email] } + let!(:source_mr_approvers) { [source_admin_user.email] } + let(:source_mr_comments) do + source_mr.comments.map do |note| + { **note.except(:id, :noteable_id), author: note[:author].except(:web_url) } + end + end + let(:imported_mrs) { imported_project.merge_requests } let(:imported_mr) do Resource::MergeRequest.init do |mr| mr.project = imported_project - mr.iid = imported_mrs.first[:iid] + mr.iid = imported_project.merge_requests.first[:iid] mr.api_client = api_client end end - let(:imported_mr_reviewers) { imported_mr.reviewers.map { |r| r.slice(:name, :username) } } - let(:source_mr_reviewers) { [{ name: other_user.name, username: other_user.username }] } + let(:imported_mr_comments) do + imported_mr.comments.map do |note| + { **note.except(:id, :noteable_id), author: note[:author].except(:web_url) } + end + end + let(:imported_mr_reviewers) { imported_mr.reviewers.map { |reviewer| reviewer[:username] } } let(:imported_mr_approvers) do - imported_mr.approval_configuration[:approved_by].map do |usr| - { username: usr.dig(:user, :username), name: usr.dig(:user, :name) } - end + imported_mr.approval_configuration[:approved_by].map { |usr| usr.dig(:user, :username) } end before do - source_project.update_approval_configuration( - merge_requests_author_approval: true, - approvals_before_merge: 1 - ) + source_project.update_approval_configuration(merge_requests_author_approval: true, approvals_before_merge: 1) source_mr.approve - end - - after do - other_user.remove_via_api! + source_mr.add_comment(body: 'This is a test comment!') end context 'with merge request' do @@ -72,8 +79,8 @@ module QA expect(imported_mr).to eq(source_mr.reload!) expect(imported_mr_comments).to match_array(source_mr_comments) - expect(imported_mr_reviewers).to eq(source_mr_reviewers) - expect(imported_mr_approvers).to eq([{ username: other_user.username, name: other_user.name }]) + expect(imported_mr_reviewers).to eq([mr_reviewer.username]) + expect(imported_mr_approvers).to eq([source_admin_user.username]) end end end diff --git a/qa/qa/specs/features/api/1_manage/migration/gitlab_migration_pipeline_spec.rb b/qa/qa/specs/features/api/1_manage/migration/gitlab_migration_pipeline_spec.rb index 7b79e6967c7..d8637a54a12 100644 --- a/qa/qa/specs/features/api/1_manage/migration/gitlab_migration_pipeline_spec.rb +++ b/qa/qa/specs/features/api/1_manage/migration/gitlab_migration_pipeline_spec.rb @@ -22,7 +22,7 @@ module QA before do Resource::Repository::Commit.fabricate_via_api! do |commit| - commit.api_client = api_client + commit.api_client = source_admin_api_client commit.project = source_project commit.commit_message = 'Add .gitlab-ci.yml' commit.add_files( diff --git a/qa/qa/specs/features/api/1_manage/migration/gitlab_migration_release_spec.rb b/qa/qa/specs/features/api/1_manage/migration/gitlab_migration_release_spec.rb index 36036a2321e..2f42b7a15e4 100644 --- a/qa/qa/specs/features/api/1_manage/migration/gitlab_migration_release_spec.rb +++ b/qa/qa/specs/features/api/1_manage/migration/gitlab_migration_release_spec.rb @@ -6,13 +6,13 @@ module QA include_context 'with gitlab project migration' context 'with release' do - let(:tag) { 'v0.0.1' } - let(:source_project_with_readme) { true } + let!(:tag) { 'v0.0.1' } + let!(:source_project_with_readme) { true } - let(:milestone) do + let!(:milestone) do Resource::ProjectMilestone.fabricate_via_api! do |resource| resource.project = source_project - resource.api_client = api_client + resource.api_client = source_admin_api_client end end diff --git a/qa/qa/specs/features/shared_contexts/import/gitlab_group_migration_common.rb b/qa/qa/specs/features/shared_contexts/import/gitlab_group_migration_common.rb index 9ec84e12ac0..8a8587c39a8 100644 --- a/qa/qa/specs/features/shared_contexts/import/gitlab_group_migration_common.rb +++ b/qa/qa/specs/features/shared_contexts/import/gitlab_group_migration_common.rb @@ -19,7 +19,13 @@ module QA is_new_session: false ) end - let!(:source_admin_user) { Resource::User.fabricate_via_api! { |usr| usr.api_client = source_admin_api_client } } + let!(:source_admin_user) do + Resource::User.fabricate_via_api! do |usr| + usr.api_client = source_admin_api_client + usr.username = "root" + usr.email = "admin@example.com" + end.tap(&:set_public_email) + end let!(:source_group) do Resource::Sandbox.fabricate_via_api! do |group| group.api_client = source_admin_api_client @@ -31,7 +37,19 @@ module QA # target instance objects # let!(:admin_api_client) { Runtime::API::Client.as_admin } - let!(:user) { Resource::User.fabricate_via_api! { |usr| usr.api_client = admin_api_client } } + let!(:admin_user) do + Resource::User.fabricate_via_api! do |usr| + usr.api_client = admin_api_client + usr.username = "root" + usr.email = "admin@example.com" + end.tap(&:set_public_email) + end + let!(:user) do + Resource::User.fabricate_via_api! do |usr| + usr.api_client = admin_api_client + usr.username = "target-user-#{SecureRandom.hex(6)}" + end + end let!(:api_client) { Runtime::API::Client.new(user: user) } let!(:target_sandbox) do Resource::Sandbox.fabricate_via_api! do |group| @@ -55,7 +73,6 @@ module QA before do target_sandbox.add_member(user, Resource::Members::AccessLevel::OWNER) - source_admin_user.set_public_email end after do |example| diff --git a/qa/qa/specs/features/shared_contexts/import/gitlab_project_migration_common.rb b/qa/qa/specs/features/shared_contexts/import/gitlab_project_migration_common.rb index 9c80c088917..e1189a0ea66 100644 --- a/qa/qa/specs/features/shared_contexts/import/gitlab_project_migration_common.rb +++ b/qa/qa/specs/features/shared_contexts/import/gitlab_project_migration_common.rb @@ -1,64 +1,21 @@ # frozen_string_literal: true module QA - RSpec.shared_context 'with gitlab project migration', requires_admin: 'creates a user via API', - quarantine: { - type: :flaky, - issue: 'https://gitlab.com/gitlab-org/gitlab/-/issues/364839' - }, - feature_flag: { - name: 'bulk_import_projects', - scope: :global - } do - let(:source_project_with_readme) { false } - let(:import_wait_duration) { { max_duration: 300, sleep_interval: 2 } } - let(:admin_api_client) { Runtime::API::Client.as_admin } - let(:user) do - Resource::User.fabricate_via_api! do |usr| - usr.api_client = admin_api_client - usr.hard_delete_on_api_removal = true - end - end - - let(:api_client) { Runtime::API::Client.new(user: user) } - - let(:sandbox) do - Resource::Sandbox.fabricate_via_api! do |group| - group.api_client = admin_api_client - end - end - - let(:destination_group) do - Resource::Group.fabricate_via_api! do |group| - group.api_client = api_client - group.sandbox = sandbox - group.path = "destination-group-for-import-#{SecureRandom.hex(4)}" - end - end + RSpec.shared_context 'with gitlab project migration' do + # gitlab project migration doesn't work on just the projects + # so all project migration tests will always require setup for gitlab group migration + include_context "with gitlab group migration" - let(:source_group) do - Resource::Group.fabricate_via_api! do |group| - group.api_client = api_client - group.path = "source-group-for-import-#{SecureRandom.hex(4)}" - end - end + let(:source_project_with_readme) { false } let(:source_project) do Resource::Project.fabricate_via_api! do |project| - project.api_client = api_client + project.api_client = source_admin_api_client project.group = source_group project.initialize_with_readme = source_project_with_readme end end - let(:imported_group) do - Resource::BulkImportGroup.fabricate_via_api! do |group| - group.api_client = api_client - group.sandbox = destination_group - group.source_group = source_group - end - end - let(:imported_projects) { imported_group.reload!.projects } let(:imported_project) { imported_projects.first } @@ -74,18 +31,8 @@ module QA end before do - Runtime::Feature.enable(:bulk_import_projects) - - sandbox.add_member(user, Resource::Members::AccessLevel::MAINTAINER) + Runtime::Feature.enable(:bulk_import_projects) unless Runtime::Feature.enabled?(:bulk_import_projects) source_project # fabricate source group and project end - - after do |example| - # Checking for failures in the test currently makes test very flaky due to catching unrelated failures - # Log failures for easier debugging - Runtime::Logger.warn("Import failures: #{import_failures}") if example.exception && !import_failures.empty? - ensure - user.remove_via_api! - end end end diff --git a/qa/qa/support/loglinking.rb b/qa/qa/support/loglinking.rb index f24577ff313..0bb6345709f 100644 --- a/qa/qa/support/loglinking.rb +++ b/qa/qa/support/loglinking.rb @@ -1,4 +1,7 @@ # frozen_string_literal: true + +require 'active_support/core_ext/integer/time' + module QA module Support module Loglinking @@ -22,28 +25,38 @@ module QA def self.failure_metadata(correlation_id) return if correlation_id.blank? - sentry_uri = sentry_url - kibana_uri = kibana_url + sentry_base_url = get_sentry_base_url + kibana_base_url = get_kibana_base_url errors = ["Correlation Id: #{correlation_id}"] - errors << "Sentry Url: #{sentry_uri}&query=correlation_id%3A%22#{correlation_id}%22" if sentry_uri - errors << "Kibana Url: #{kibana_uri}app/discover#/?_a=%28query%3A%28language%3Akuery%2Cquery%3A%27json.correlation_id%20%3A%20#{correlation_id}%27%29%29&_g=%28time%3A%28from%3Anow-24h%2Cto%3Anow%29%29" if kibana_uri + errors << "Sentry Url: #{get_sentry_url(sentry_base_url, correlation_id)}" if sentry_base_url + errors << "Kibana Url: #{get_kibana_url(kibana_base_url, correlation_id)}" if kibana_base_url errors.join("\n") end - def self.sentry_url + def self.get_sentry_base_url return unless logging_environment? SENTRY_ENVIRONMENTS[logging_environment] end - def self.kibana_url + def self.get_sentry_url(base_url, correlation_id) + "#{base_url}&query=correlation_id%3A%22#{correlation_id}%22" + end + + def self.get_kibana_base_url return unless logging_environment? KIBANA_ENVIRONMENTS[logging_environment] end + def self.get_kibana_url(base_url, correlation_id) + "#{base_url}app/discover#/?_a=%28query%3A%28language%3Akuery%2Cquery%3A" \ + "%27json.correlation_id%20%3A%20#{correlation_id}%27%29%29" \ + "&_g=%28time%3A%28from%3A%27#{start_time}%27%2Cto%3A%27#{end_time}%27%29%29" + end + def self.logging_environment address = QA::Runtime::Scenario.attributes[:gitlab_address] return if address.nil? @@ -65,6 +78,14 @@ module QA def self.logging_environment? !logging_environment.nil? end + + def self.start_time + (Time.now.utc - 24.hours).iso8601(3) + end + + def self.end_time + Time.now.utc.iso8601(3) + end end end end diff --git a/qa/spec/resource/api_fabricator_spec.rb b/qa/spec/resource/api_fabricator_spec.rb index 4cb6ef3c9b5..377af27e533 100644 --- a/qa/spec/resource/api_fabricator_spec.rb +++ b/qa/spec/resource/api_fabricator_spec.rb @@ -140,12 +140,14 @@ RSpec.describe QA::Resource::ApiFabricator do end end - it 'logs a sentry url from staging' do + it 'logs Sentry and Kibana URLs from staging' do response = double('Raw POST response', code: 400, body: post_response.to_json, headers: { x_request_id: 'foobar' }) cookies = [{ name: 'Foo', value: 'Bar' }, { name: 'gitlab_canary', value: 'true' }] + time = Time.new(2022, 11, 14, 0, 0, 0, '+00:00') allow(Capybara.current_session).to receive_message_chain(:driver, :browser, :manage, :all_cookies).and_return(cookies) allow(QA::Runtime::Scenario).to receive(:attributes).and_return({ gitlab_address: 'https://staging.gitlab.com' }) + allow(Time).to receive(:now).and_return(time) expect(api_request).to receive(:new).with(api_client_instance, subject.api_post_path).and_return(double(url: resource_web_url)) expect(subject).to receive(:post).with(resource_web_url, subject.api_post_body).and_return(response) @@ -156,7 +158,7 @@ RSpec.describe QA::Resource::ApiFabricator do Fabrication of FooBarResource using the API failed (400) with `#{raw_post}`. Correlation Id: foobar Sentry Url: https://sentry.gitlab.net/gitlab/staginggitlabcom/?environment=gstg&query=correlation_id%3A%22foobar%22 - Kibana Url: https://nonprod-log.gitlab.net/app/discover#/?_a=%28query%3A%28language%3Akuery%2Cquery%3A%27json.correlation_id%20%3A%20foobar%27%29%29&_g=%28time%3A%28from%3Anow-24h%2Cto%3Anow%29%29 + Kibana Url: https://nonprod-log.gitlab.net/app/discover#/?_a=%28query%3A%28language%3Akuery%2Cquery%3A%27json.correlation_id%20%3A%20foobar%27%29%29&_g=%28time%3A%28from%3A%272022-11-13T00:00:00.000Z%27%2Cto%3A%272022-11-14T00:00:00.000Z%27%29%29 ERROR end end diff --git a/qa/spec/support/loglinking_spec.rb b/qa/spec/support/loglinking_spec.rb index 10865068e3d..649829f096a 100644 --- a/qa/spec/support/loglinking_spec.rb +++ b/qa/spec/support/loglinking_spec.rb @@ -14,8 +14,8 @@ RSpec.describe QA::Support::Loglinking do context 'return error string' do it 'with sentry URL' do - allow(QA::Support::Loglinking).to receive(:sentry_url).and_return('https://sentry.address/?environment=bar') - allow(QA::Support::Loglinking).to receive(:kibana_url).and_return(nil) + allow(QA::Support::Loglinking).to receive(:get_sentry_base_url).and_return('https://sentry.address/?environment=bar') + allow(QA::Support::Loglinking).to receive(:get_kibana_base_url).and_return(nil) expect(QA::Support::Loglinking.failure_metadata('foo123')).to eql(<<~ERROR.chomp) Correlation Id: foo123 @@ -24,12 +24,15 @@ RSpec.describe QA::Support::Loglinking do end it 'with kibana URL' do - allow(QA::Support::Loglinking).to receive(:sentry_url).and_return(nil) - allow(QA::Support::Loglinking).to receive(:kibana_url).and_return('https://kibana.address/') + time = Time.new(2022, 11, 14, 0, 0, 0, '+00:00') + + allow(QA::Support::Loglinking).to receive(:get_sentry_base_url).and_return(nil) + allow(QA::Support::Loglinking).to receive(:get_kibana_base_url).and_return('https://kibana.address/') + allow(Time).to receive(:now).and_return(time) expect(QA::Support::Loglinking.failure_metadata('foo123')).to eql(<<~ERROR.chomp) Correlation Id: foo123 - Kibana Url: https://kibana.address/app/discover#/?_a=%28query%3A%28language%3Akuery%2Cquery%3A%27json.correlation_id%20%3A%20foo123%27%29%29&_g=%28time%3A%28from%3Anow-24h%2Cto%3Anow%29%29 + Kibana Url: https://kibana.address/app/discover#/?_a=%28query%3A%28language%3Akuery%2Cquery%3A%27json.correlation_id%20%3A%20foo123%27%29%29&_g=%28time%3A%28from%3A%272022-11-13T00:00:00.000Z%27%2Cto%3A%272022-11-14T00:00:00.000Z%27%29%29 ERROR end end @@ -51,7 +54,7 @@ RSpec.describe QA::Support::Loglinking do url_hash.each do |environment, url| allow(QA::Support::Loglinking).to receive(:logging_environment).and_return(environment) - expect(QA::Support::Loglinking.sentry_url).to eq(url) + expect(QA::Support::Loglinking.get_sentry_base_url).to eq(url) end end end @@ -71,7 +74,7 @@ RSpec.describe QA::Support::Loglinking do url_hash.each do |environment, url| allow(QA::Support::Loglinking).to receive(:logging_environment).and_return(environment) - expect(QA::Support::Loglinking.kibana_url).to eq(url) + expect(QA::Support::Loglinking.get_kibana_base_url).to eq(url) end end end diff --git a/spec/lib/gitlab/database/query_analyzers/query_recorder_spec.rb b/spec/lib/gitlab/database/query_analyzers/query_recorder_spec.rb index ec01ae623ae..65d00f420e6 100644 --- a/spec/lib/gitlab/database/query_analyzers/query_recorder_spec.rb +++ b/spec/lib/gitlab/database/query_analyzers/query_recorder_spec.rb @@ -12,7 +12,8 @@ RSpec.describe Gitlab::Database::QueryAnalyzers::QueryRecorder, query_analyzers: context 'when analyzer is enabled for tests' do let(:query) { 'SELECT 1 FROM projects' } - let(:log_path) { Rails.root.join(described_class::LOG_FILE) } + let(:log_path) { Rails.root.join(described_class::LOG_PATH) } + let(:log_file) { described_class.log_file } before do stub_env('CI', 'true') @@ -27,12 +28,39 @@ RSpec.describe Gitlab::Database::QueryAnalyzers::QueryRecorder, query_analyzers: it 'logs queries to a file' do allow(FileUtils).to receive(:mkdir_p) - .with(File.dirname(log_path)) + .with(log_path) expect(File).to receive(:write) - .with(log_path, /^{"sql":"#{query}/, mode: 'a') + .with(log_file, /^{"sql":"#{query}/, mode: 'a') expect(described_class).to receive(:analyze).with(/^#{query}/).and_call_original expect { ApplicationRecord.connection.execute(query) }.not_to raise_error end end + + describe '.log_file' do + let(:folder) { 'query_recorder' } + let(:extension) { 'ndjson' } + let(:default_name) { 'rspec' } + let(:job_name) { 'test-job-1' } + + subject { described_class.log_file.to_s } + + context 'when in CI' do + before do + stub_env('CI_JOB_NAME_SLUG', job_name) + end + + it { is_expected.to include("#{folder}/#{job_name}.#{extension}") } + it { is_expected.not_to include("#{folder}/#{default_name}.#{extension}") } + end + + context 'when not in CI' do + before do + stub_env('CI_JOB_NAME_SLUG', nil) + end + + it { is_expected.to include("#{folder}/#{default_name}.#{extension}") } + it { is_expected.not_to include("#{folder}/#{job_name}.#{extension}") } + end + end end diff --git a/spec/lib/gitlab/instrumentation/redis_base_spec.rb b/spec/lib/gitlab/instrumentation/redis_base_spec.rb index f9dd0c94c97..a84a26a8d92 100644 --- a/spec/lib/gitlab/instrumentation/redis_base_spec.rb +++ b/spec/lib/gitlab/instrumentation/redis_base_spec.rb @@ -1,8 +1,10 @@ # frozen_string_literal: true require 'spec_helper' +require 'rspec-parameterized' RSpec.describe Gitlab::Instrumentation::RedisBase, :request_store do + using RSpec::Parameterized::TableSyntax let(:instrumentation_class_a) do stub_const('InstanceA', Class.new(described_class)) end @@ -88,6 +90,25 @@ RSpec.describe Gitlab::Instrumentation::RedisBase, :request_store do end end + describe '.increment_cross_slot_request_count' do + context 'storage key overlapping' do + it 'keys do not overlap across storages' do + 3.times { instrumentation_class_a.increment_cross_slot_request_count } + 2.times { instrumentation_class_b.increment_cross_slot_request_count } + + expect(instrumentation_class_a.get_cross_slot_request_count).to eq(3) + expect(instrumentation_class_b.get_cross_slot_request_count).to eq(2) + end + + it 'increments by the given amount' do + instrumentation_class_a.increment_cross_slot_request_count(2) + instrumentation_class_a.increment_cross_slot_request_count(3) + + expect(instrumentation_class_a.get_cross_slot_request_count).to eq(5) + end + end + end + describe '.increment_read_bytes' do context 'storage key overlapping' do it 'keys do not overlap across storages' do @@ -130,4 +151,34 @@ RSpec.describe Gitlab::Instrumentation::RedisBase, :request_store do end end end + + describe '.redis_cluster_validate!' do + context 'Rails environments' do + where(:env, :should_raise) do + 'production' | false + 'staging' | false + 'development' | true + 'test' | true + end + + before do + instrumentation_class_a.enable_redis_cluster_validation + end + + with_them do + it do + stub_rails_env(env) + + args = [[:mget, 'foo', 'bar']] + + if should_raise + expect { instrumentation_class_a.redis_cluster_validate!(args) } + .to raise_error(::Gitlab::Instrumentation::RedisClusterValidator::CrossSlotError) + else + expect { instrumentation_class_a.redis_cluster_validate!(args) }.not_to raise_error + end + end + end + end + end end diff --git a/spec/lib/gitlab/instrumentation/redis_cluster_validator_spec.rb b/spec/lib/gitlab/instrumentation/redis_cluster_validator_spec.rb index 58c75bff9dd..c5462bd8545 100644 --- a/spec/lib/gitlab/instrumentation/redis_cluster_validator_spec.rb +++ b/spec/lib/gitlab/instrumentation/redis_cluster_validator_spec.rb @@ -10,30 +10,6 @@ RSpec.describe Gitlab::Instrumentation::RedisClusterValidator do describe '.validate!' do using RSpec::Parameterized::TableSyntax - context 'Rails environments' do - where(:env, :should_raise) do - 'production' | false - 'staging' | false - 'development' | true - 'test' | true - end - - with_them do - it do - stub_rails_env(env) - - args = [[:mget, 'foo', 'bar']] - - if should_raise - expect { described_class.validate!(args) } - .to raise_error(described_class::CrossSlotError) - else - expect { described_class.validate!(args) }.not_to raise_error - end - end - end - end - where(:command, :arguments, :should_raise) do :rename | %w(foo bar) | true :RENAME | %w(foo bar) | true diff --git a/spec/lib/gitlab/instrumentation/redis_interceptor_spec.rb b/spec/lib/gitlab/instrumentation/redis_interceptor_spec.rb index 02c5dfb7521..8b0f3e3c00a 100644 --- a/spec/lib/gitlab/instrumentation/redis_interceptor_spec.rb +++ b/spec/lib/gitlab/instrumentation/redis_interceptor_spec.rb @@ -2,6 +2,7 @@ require 'spec_helper' require 'rspec-parameterized' +require 'support/helpers/rails_helpers' RSpec.describe Gitlab::Instrumentation::RedisInterceptor, :clean_gitlab_redis_shared_state, :request_store do using RSpec::Parameterized::TableSyntax @@ -63,6 +64,12 @@ RSpec.describe Gitlab::Instrumentation::RedisInterceptor, :clean_gitlab_redis_sh end end + it 'skips count for non-cross-slot requests' do + expect(instrumentation_class).not_to receive(:increment_cross_slot_request_count).and_call_original + + Gitlab::Redis::SharedState.with { |redis| redis.call(:mget, '{foo}bar', '{foo}baz') } + end + it 'counts exceptions' do expect(instrumentation_class).to receive(:instance_count_exception) .with(instance_of(Redis::CommandError)).and_call_original @@ -74,6 +81,18 @@ RSpec.describe Gitlab::Instrumentation::RedisInterceptor, :clean_gitlab_redis_sh end end.to raise_exception(Redis::CommandError) end + + context 'in production env' do + before do + stub_rails_env('production') # to avoid raising CrossSlotError + end + + it 'counts cross-slot requests' do + expect(instrumentation_class).to receive(:increment_cross_slot_request_count).and_call_original + + Gitlab::Redis::SharedState.with { |redis| redis.call(:mget, 'foo', 'bar') } + end + end end describe 'latency' do diff --git a/spec/lib/gitlab/instrumentation/redis_spec.rb b/spec/lib/gitlab/instrumentation/redis_spec.rb index c01d06c97b0..7b6f31c1a08 100644 --- a/spec/lib/gitlab/instrumentation/redis_spec.rb +++ b/spec/lib/gitlab/instrumentation/redis_spec.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require 'spec_helper' +require 'support/helpers/rails_helpers' RSpec.describe Gitlab::Instrumentation::Redis do def stub_storages(method, value) @@ -22,6 +23,7 @@ RSpec.describe Gitlab::Instrumentation::Redis do end it_behaves_like 'aggregation of redis storage data', :get_request_count + it_behaves_like 'aggregation of redis storage data', :get_cross_slot_request_count it_behaves_like 'aggregation of redis storage data', :query_time it_behaves_like 'aggregation of redis storage data', :read_bytes it_behaves_like 'aggregation of redis storage data', :write_bytes @@ -35,7 +37,8 @@ RSpec.describe Gitlab::Instrumentation::Redis do Gitlab::Redis::Cache.with { |redis| redis.info } RequestStore.clear! - Gitlab::Redis::Cache.with { |redis| redis.set('cache-test', 321) } + stub_rails_env('staging') # to avoid raising CrossSlotError + Gitlab::Redis::Cache.with { |redis| redis.mset('cache-test', 321, 'cache-test-2', 321) } Gitlab::Redis::SharedState.with { |redis| redis.set('shared-state-test', 123) } end @@ -43,12 +46,14 @@ RSpec.describe Gitlab::Instrumentation::Redis do expected_payload = { # Aggregated results redis_calls: 2, + redis_cross_slot_calls: 1, redis_duration_s: be >= 0, redis_read_bytes: be >= 0, redis_write_bytes: be >= 0, # Cache results redis_cache_calls: 1, + redis_cache_cross_slot_calls: 1, redis_cache_duration_s: be >= 0, redis_cache_read_bytes: be >= 0, redis_cache_write_bytes: be >= 0, diff --git a/spec/lib/gitlab/instrumentation_helper_spec.rb b/spec/lib/gitlab/instrumentation_helper_spec.rb index d5ff39767c4..38607ce2752 100644 --- a/spec/lib/gitlab/instrumentation_helper_spec.rb +++ b/spec/lib/gitlab/instrumentation_helper_spec.rb @@ -2,6 +2,7 @@ require 'spec_helper' require 'rspec-parameterized' +require 'support/helpers/rails_helpers' RSpec.describe Gitlab::InstrumentationHelper do using RSpec::Parameterized::TableSyntax @@ -38,18 +39,20 @@ RSpec.describe Gitlab::InstrumentationHelper do context 'when Redis calls are made' do it 'adds Redis data and omits Gitaly data' do - Gitlab::Redis::Cache.with { |redis| redis.set('test-cache', 123) } + stub_rails_env('staging') # to avoid raising CrossSlotError + Gitlab::Redis::Cache.with { |redis| redis.mset('test-cache', 123, 'test-cache2', 123) } Gitlab::Redis::Queues.with { |redis| redis.set('test-queues', 321) } subject # Aggregated payload expect(payload[:redis_calls]).to eq(2) + expect(payload[:redis_cross_slot_calls]).to eq(1) expect(payload[:redis_duration_s]).to be >= 0 expect(payload[:redis_read_bytes]).to be >= 0 expect(payload[:redis_write_bytes]).to be >= 0 - # Shared state payload + # Queue payload expect(payload[:redis_queues_calls]).to eq(1) expect(payload[:redis_queues_duration_s]).to be >= 0 expect(payload[:redis_queues_read_bytes]).to be >= 0 @@ -57,6 +60,7 @@ RSpec.describe Gitlab::InstrumentationHelper do # Cache payload expect(payload[:redis_cache_calls]).to eq(1) + expect(payload[:redis_cache_cross_slot_calls]).to eq(1) expect(payload[:redis_cache_duration_s]).to be >= 0 expect(payload[:redis_cache_read_bytes]).to be >= 0 expect(payload[:redis_cache_write_bytes]).to be >= 0 diff --git a/spec/support/database/query_recorder.rb b/spec/support/database/query_recorder.rb index 1050120e528..3480430a0da 100644 --- a/spec/support/database/query_recorder.rb +++ b/spec/support/database/query_recorder.rb @@ -3,7 +3,7 @@ RSpec.configure do |config| # Truncate the query_recorder log file before starting the suite config.before(:suite) do - log_path = Rails.root.join(Gitlab::Database::QueryAnalyzers::QueryRecorder::LOG_FILE) - File.write(log_path, '') if File.exist?(log_path) + log_file = Rails.root.join(Gitlab::Database::QueryAnalyzers::QueryRecorder.log_file) + File.write(log_file, '') if File.exist?(log_file) end end |