diff options
13 files changed, 268 insertions, 9 deletions
diff --git a/.rubocop.yml b/.rubocop.yml index b2c1d3ac947..80628bb5523 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -416,6 +416,11 @@ Migration/BatchMigrationsPostOnly: - 'db/migrate/*.rb' - 'db/post_migrate/*.rb' +BackgroundMigration/FeatureCategory: + Enabled: true + Include: + - 'lib/gitlab/background_migration/*.rb' + # See https://gitlab.com/gitlab-org/gitlab/-/issues/373194 Gitlab/RSpec/AvoidSetup: Enabled: true diff --git a/.rubocop_todo/background_migration/feature_category.yml b/.rubocop_todo/background_migration/feature_category.yml new file mode 100644 index 00000000000..cae07260406 --- /dev/null +++ b/.rubocop_todo/background_migration/feature_category.yml @@ -0,0 +1,61 @@ +--- +BackgroundMigration/FeatureCategory: + Details: grace period + Exclude: + - 'lib/gitlab/background_migration/backfill_cluster_agents_has_vulnerabilities.rb' + - 'lib/gitlab/background_migration/backfill_environment_tiers.rb' + - 'lib/gitlab/background_migration/backfill_epic_cache_counts.rb' + - 'lib/gitlab/background_migration/backfill_group_features.rb' + - 'lib/gitlab/background_migration/backfill_imported_issue_search_data.rb' + - 'lib/gitlab/background_migration/backfill_internal_on_notes.rb' + - 'lib/gitlab/background_migration/backfill_namespace_details.rb' + - 'lib/gitlab/background_migration/backfill_namespace_id_of_vulnerability_reads.rb' + - 'lib/gitlab/background_migration/backfill_project_feature_package_registry_access_level.rb' + - 'lib/gitlab/background_migration/backfill_project_import_level.rb' + - 'lib/gitlab/background_migration/backfill_project_member_namespace_id.rb' + - 'lib/gitlab/background_migration/backfill_project_namespace_details.rb' + - 'lib/gitlab/background_migration/backfill_project_namespace_on_issues.rb' + - 'lib/gitlab/background_migration/backfill_project_statistics_container_repository_size.rb' + - 'lib/gitlab/background_migration/backfill_project_statistics_storage_size_without_uploads_size.rb' + - 'lib/gitlab/background_migration/backfill_user_details_fields.rb' + - 'lib/gitlab/background_migration/backfill_vulnerability_reads_cluster_agent.rb' + - 'lib/gitlab/background_migration/backfill_work_item_type_id_for_issues.rb' + - 'lib/gitlab/background_migration/cleanup_orphaned_routes.rb' + - 'lib/gitlab/background_migration/copy_column_using_background_migration_job.rb' + - 'lib/gitlab/background_migration/delete_approval_rules_with_vulnerability.rb' + - 'lib/gitlab/background_migration/delete_invalid_epic_issues.rb' + - 'lib/gitlab/background_migration/delete_orphaned_operational_vulnerabilities.rb' + - 'lib/gitlab/background_migration/delete_orphans_approval_merge_request_rules.rb' + - 'lib/gitlab/background_migration/delete_orphans_approval_project_rules.rb' + - 'lib/gitlab/background_migration/destroy_invalid_group_members.rb' + - 'lib/gitlab/background_migration/destroy_invalid_members.rb' + - 'lib/gitlab/background_migration/destroy_invalid_project_members.rb' + - 'lib/gitlab/background_migration/disable_legacy_open_source_licence_for_recent_public_projects.rb' + - 'lib/gitlab/background_migration/disable_legacy_open_source_license_for_inactive_public_projects.rb' + - 'lib/gitlab/background_migration/disable_legacy_open_source_license_for_no_issues_no_repo_projects.rb' + - 'lib/gitlab/background_migration/disable_legacy_open_source_license_for_one_member_no_repo_projects.rb' + - 'lib/gitlab/background_migration/disable_legacy_open_source_license_for_projects_less_than_five_mb.rb' + - 'lib/gitlab/background_migration/disable_legacy_open_source_license_for_projects_less_than_one_mb.rb' + - 'lib/gitlab/background_migration/expire_o_auth_tokens.rb' + - 'lib/gitlab/background_migration/fix_approval_project_rules_without_protected_branches.rb' + - 'lib/gitlab/background_migration/fix_security_scan_statuses.rb' + - 'lib/gitlab/background_migration/migrate_shared_vulnerability_scanners.rb' + - 'lib/gitlab/background_migration/migrate_vulnerabilities_feedback_to_vulnerabilities_state_transition.rb' + - 'lib/gitlab/background_migration/populate_approval_merge_request_rules_with_security_orchestration.rb' + - 'lib/gitlab/background_migration/populate_approval_project_rules_with_security_orchestration.rb' + - 'lib/gitlab/background_migration/populate_operation_visibility_permissions_from_operations.rb' + - 'lib/gitlab/background_migration/populate_projects_star_count.rb' + - 'lib/gitlab/background_migration/prune_stale_project_export_jobs.rb' + - 'lib/gitlab/background_migration/recount_epic_cache_counts.rb' + - 'lib/gitlab/background_migration/remove_backfilled_job_artifacts_expire_at.rb' + - 'lib/gitlab/background_migration/remove_self_managed_wiki_notes.rb' + - 'lib/gitlab/background_migration/rename_task_system_note_to_checklist_item.rb' + - 'lib/gitlab/background_migration/reset_duplicate_ci_runners_token_encrypted_values.rb' + - 'lib/gitlab/background_migration/reset_duplicate_ci_runners_token_values.rb' + - 'lib/gitlab/background_migration/reset_status_on_container_repositories.rb' + - 'lib/gitlab/background_migration/sanitize_confidential_todos.rb' + - 'lib/gitlab/background_migration/set_correct_vulnerability_state.rb' + - 'lib/gitlab/background_migration/set_legacy_open_source_license_available_for_non_public_projects.rb' + - 'lib/gitlab/background_migration/update_ci_pipeline_artifacts_unknown_locked_status.rb' + - 'lib/gitlab/background_migration/update_delayed_project_removal_to_null_for_user_namespaces.rb' + - 'lib/gitlab/background_migration/update_jira_tracker_data_deployment_type_based_on_url.rb' diff --git a/app/workers/concerns/application_worker.rb b/app/workers/concerns/application_worker.rb index f51c2852da6..222d045b0ba 100644 --- a/app/workers/concerns/application_worker.rb +++ b/app/workers/concerns/application_worker.rb @@ -16,6 +16,7 @@ module ApplicationWorker SAFE_PUSH_BULK_LIMIT = 1000 included do + prefer_calling_context_feature_category false set_queue after_set_class_attribute { set_queue } diff --git a/app/workers/concerns/worker_attributes.rb b/app/workers/concerns/worker_attributes.rb index 8a135bc1853..1674ed1483a 100644 --- a/app/workers/concerns/worker_attributes.rb +++ b/app/workers/concerns/worker_attributes.rb @@ -38,12 +38,17 @@ module WorkerAttributes set_class_attribute(:feature_category, value) end + def prefer_calling_context_feature_category(preference = false) + set_class_attribute(:prefer_calling_context_feature_category, preference) + end + # Special case: if a worker is not owned, get the feature category # (if present) from the calling context. def get_feature_category feature_category = get_class_attribute(:feature_category) + calling_context_feature_category_preferred = !!get_class_attribute(:prefer_calling_context_feature_category) - return feature_category unless feature_category == :not_owned + return feature_category unless feature_category == :not_owned || calling_context_feature_category_preferred Gitlab::ApplicationContext.current_context_attribute('meta.feature_category') || feature_category end diff --git a/app/workers/database/batched_background_migration/single_database_worker.rb b/app/workers/database/batched_background_migration/single_database_worker.rb index e772216e557..b7b46937db2 100644 --- a/app/workers/database/batched_background_migration/single_database_worker.rb +++ b/app/workers/database/batched_background_migration/single_database_worker.rb @@ -16,6 +16,7 @@ module Database included do data_consistency :always feature_category :database + prefer_calling_context_feature_category true idempotent! end diff --git a/doc/development/database/batched_background_migrations.md b/doc/development/database/batched_background_migrations.md index 71df4da59c3..88fdfab9828 100644 --- a/doc/development/database/batched_background_migrations.md +++ b/doc/development/database/batched_background_migrations.md @@ -269,6 +269,7 @@ In the second (filtered) example, we know exactly 100 will be updated with each class BackfillNamespaceType < BatchedMigrationJob scope_to ->(relation) { relation.where(type: nil) } operation_name :update_all + feature_category :source_code_management def perform each_sub_batch do |sub_batch| @@ -330,6 +331,7 @@ background migration. # end operation_name :update_all + feature_category :source_code_management def perform each_sub_batch( diff --git a/doc/development/feature_categorization/index.md b/doc/development/feature_categorization/index.md index ccaff264e9a..29ca5d170f7 100644 --- a/doc/development/feature_categorization/index.md +++ b/doc/development/feature_categorization/index.md @@ -8,7 +8,7 @@ info: To determine the technical writer assigned to the Stage/Group associated w > [Introduced](https://gitlab.com/groups/gitlab-com/gl-infra/-/epics/269) in GitLab 13.2. -Each Sidekiq worker, controller action, [test example](../testing_guide/best_practices.md#feature-category-metadata) or API endpoint +Each Sidekiq worker, Batched Background migrations, controller action, [test example](../testing_guide/best_practices.md#feature-category-metadata) or API endpoint must declare a `feature_category` attribute. This attribute maps each of these to a [feature category](https://about.gitlab.com/handbook/product/categories/). This is done for error budgeting, alert routing, and team attribution. @@ -76,6 +76,24 @@ category (worker or HTTP endpoint) in metrics and logs. For instance, `ReactiveCachingWorker` can have multiple feature categories in metrics and logs. +## Batched Background Migrations + +Long running migrations (as per the [time limits guidelines](../migration_style_guide.md#how-long-a-migration-should-take)) are pulled out as batched background migrations and they should have their 'feature_category' defined. + +_Example_: + +```ruby +# File name: lib/gitlab/background_migration/my_background_migration_job.rb + +class MyBackgroundMigrationJob < BatchedMigrationJob + feature_category :gitaly + + #... +end +``` + +_Note_: [RuboCop::Cop::BackgroundMigration::FeatureCategory](https://gitlab.com/gitlab-org/gitlab/-/blob/master/rubocop/cop/background_migration/feature_category.rb) cop ensures a valid 'feature_category' is defined. + ## Rails controllers Specifying feature categories on controller actions can be done using diff --git a/doc/user/clusters/agent/troubleshooting.md b/doc/user/clusters/agent/troubleshooting.md index 6b5f58f8f76..a44d1e9685b 100644 --- a/doc/user/clusters/agent/troubleshooting.md +++ b/doc/user/clusters/agent/troubleshooting.md @@ -229,3 +229,13 @@ This error occurs when the GitLab agent tries to update an object and the object - Add the required annotations manually. - Delete the object and let the agent recreate it. - Change your [`inventory_policy`](../../infrastructure/clusters/deploy/inventory_object.md#inventory_policy-options) setting. + +## Parse error during installation + +When you install the agent, you might encounter an error that states: + +```shell +Error: parse error at (gitlab-agent/templates/observability-secret.yaml:1): unclosed action +``` + +This error is typically caused by an incompatible version of Helm. To resolve the issue, ensure that you are using a version of Helm [compatible with your version of Kubernetes](index.md#supported-cluster-versions). diff --git a/lib/gitlab/database/background_migration/batched_migration_wrapper.rb b/lib/gitlab/database/background_migration/batched_migration_wrapper.rb index ad747a8131d..f1fc3efae9e 100644 --- a/lib/gitlab/database/background_migration/batched_migration_wrapper.rb +++ b/lib/gitlab/database/background_migration/batched_migration_wrapper.rb @@ -49,6 +49,8 @@ module Gitlab def execute_job(tracking_record) job_class = tracking_record.migration_job_class + ApplicationContext.push(feature_category: fetch_feature_category(job_class)) + if job_class < Gitlab::BackgroundMigration::BatchedMigrationJob execute_batched_migration_job(job_class, tracking_record) else @@ -86,6 +88,14 @@ module Gitlab job_instance end + + def fetch_feature_category(job_class) + if job_class.respond_to?(:feature_category) + job_class.feature_category.to_s + else + Gitlab::BackgroundMigration::BatchedMigrationJob::DEFAULT_FEATURE_CATEGORY + end + end end end end diff --git a/rubocop/cop/background_migration/feature_category.rb b/rubocop/cop/background_migration/feature_category.rb new file mode 100644 index 00000000000..2c96e5f29de --- /dev/null +++ b/rubocop/cop/background_migration/feature_category.rb @@ -0,0 +1,64 @@ +# frozen_string_literal: true + +require_relative '../../migration_helpers' +require_relative '../../code_reuse_helpers' + +module RuboCop + module Cop + module BackgroundMigration + # Cop that checks if a valid 'feature_category' is defined in the batched background migration job + class FeatureCategory < RuboCop::Cop::Base + include MigrationHelpers + + FEATURE_CATEGORIES_FILE_PATH = "config/feature_categories.yml" + + MSG = "'feature_category' should be defined to better assign the ownership for batched migration jobs. "\ + "For more details refer: "\ + "https://docs.gitlab.com/ee/development/feature_categorization/#batched-background-migrations" + + INVALID_FEATURE_CATEGORY_MSG = "'feature_category' is invalid. "\ + "List of valid ones can be found in #{FEATURE_CATEGORIES_FILE_PATH}" + + RESTRICT_ON_SEND = [:feature_category].freeze + + class << self + attr_accessor :available_feature_categories + end + + def_node_search :feature_category?, <<~PATTERN + (:send nil? :feature_category ...) + PATTERN + + def on_new_investigation + super + + # Defined only once per rubocop whole run instead of each file. + fetch_available_feature_categories unless self.class.available_feature_categories.present? + end + + def on_class(node) + return unless in_background_migration?(node) && node.parent_class&.short_name == :BatchedMigrationJob + + add_offense(node) unless feature_category?(node) + end + + def on_send(node) + return unless in_background_migration?(node) + + add_offense(node, message: INVALID_FEATURE_CATEGORY_MSG) unless valid_feature_category?(node) + end + + private + + def valid_feature_category?(node) + feature_category = node.descendants.first.value + self.class.available_feature_categories.include?(feature_category.to_s) + end + + def fetch_available_feature_categories + self.class.available_feature_categories = YAML.load_file(FEATURE_CATEGORIES_FILE_PATH).to_set + end + end + end + end +end diff --git a/spec/lib/gitlab/database/background_migration/batched_migration_wrapper_spec.rb b/spec/lib/gitlab/database/background_migration/batched_migration_wrapper_spec.rb index 983f482d464..f3a292abbae 100644 --- a/spec/lib/gitlab/database/background_migration/batched_migration_wrapper_spec.rb +++ b/spec/lib/gitlab/database/background_migration/batched_migration_wrapper_spec.rb @@ -152,6 +152,7 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigrationWrapper, ' it 'runs the job with the correct arguments' do expect(job_class).to receive(:new).with(no_args).and_return(job_instance) + expect(Gitlab::ApplicationContext).to receive(:push).with(feature_category: :database) expect(job_instance).to receive(:perform).with(1, 10, 'events', 'id', 1, pause_ms, 'id', 'other_id') perform diff --git a/spec/rubocop/cop/background_migration/feature_category_spec.rb b/spec/rubocop/cop/background_migration/feature_category_spec.rb new file mode 100644 index 00000000000..359520b1d9f --- /dev/null +++ b/spec/rubocop/cop/background_migration/feature_category_spec.rb @@ -0,0 +1,71 @@ +# frozen_string_literal: true + +require 'rubocop_spec_helper' +require_relative '../../../../rubocop/cop/background_migration/feature_category' + +RSpec.describe RuboCop::Cop::BackgroundMigration::FeatureCategory, feature_category: :database do + let(:cop) { described_class.new } + + context 'for non background migrations' do + before do + allow(cop).to receive(:in_background_migration?).and_return(false) + end + + it 'does not throw any offense' do + expect_no_offenses(<<~RUBY) + module Gitlab + module BackgroundMigration + class MyJob < Gitlab::BackgroundMigration::BatchedMigrationJob + def perform; end + end + end + end + RUBY + end + end + + context 'for background migrations' do + before do + allow(cop).to receive(:in_background_migration?).and_return(true) + end + + it 'throws offense on not defining the feature_category' do + expect_offense(<<~RUBY) + module Gitlab + module BackgroundMigration + class MyJob1 < Gitlab::BackgroundMigration::BatchedMigrationJob + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ #{described_class::MSG} + end + end + end + RUBY + end + + it 'throws offense on not defining a valid feature_category' do + expect_offense(<<~RUBY) + module Gitlab + module BackgroundMigration + class MyJob1 < Gitlab::BackgroundMigration::BatchedMigrationJob + feature_category :invalid_random + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ #{described_class::INVALID_FEATURE_CATEGORY_MSG} + end + end + end + RUBY + end + + it 'will not throw offense on defining a valid feature_category' do + expect_no_offenses(<<~RUBY) + module Gitlab + module BackgroundMigration + class MyJob < Gitlab::BackgroundMigration::BatchedMigrationJob + feature_category :database + + def perform; end + end + end + end + RUBY + end + end +end diff --git a/spec/support/shared_examples/workers/batched_background_migration_worker_shared_examples.rb b/spec/support/shared_examples/workers/batched_background_migration_worker_shared_examples.rb index 09ebc495e61..80a5c4ea9cc 100644 --- a/spec/support/shared_examples/workers/batched_background_migration_worker_shared_examples.rb +++ b/spec/support/shared_examples/workers/batched_background_migration_worker_shared_examples.rb @@ -256,6 +256,7 @@ RSpec.shared_examples 'it runs batched background migration jobs' do |tracking_d Class.new(Gitlab::BackgroundMigration::BatchedMigrationJob) do job_arguments :matching_status operation_name :update_all + feature_category :code_review def perform each_sub_batch( @@ -325,16 +326,16 @@ RSpec.shared_examples 'it runs batched background migration jobs' do |tracking_d stub_const('Gitlab::BackgroundMigration::ExampleDataMigration', migration_class) end - shared_examples 'batched background migration execution' do - subject(:full_migration_run) do - # process all batches, then do an extra execution to mark the job as finished - (number_of_batches + 1).times do - described_class.new.perform + subject(:full_migration_run) do + # process all batches, then do an extra execution to mark the job as finished + (number_of_batches + 1).times do + described_class.new.perform - travel_to((migration.interval + described_class::INTERVAL_VARIANCE).seconds.from_now) - end + travel_to((migration.interval + described_class::INTERVAL_VARIANCE).seconds.from_now) end + end + shared_examples 'batched background migration execution' do it 'marks the migration record as finished' do expect { full_migration_run }.to change { migration.reload.status }.from(1).to(3) # active -> finished end @@ -404,6 +405,15 @@ RSpec.shared_examples 'it runs batched background migration jobs' do |tracking_d end it_behaves_like 'batched background migration execution' + + it 'assigns proper feature category to the context and the worker' do + expected_feature_category = migration_class.feature_category.to_s + + expect { full_migration_run }.to change { + Gitlab::ApplicationContext.current["meta.feature_category"] + }.to(expected_feature_category) + .and change { described_class.get_feature_category }.from(:database).to(expected_feature_category) + end end context 'when parallel execution is enabled', :sidekiq_inline do |