diff --git a/app/services/environments/delete_managed_resources_service.rb b/app/services/environments/delete_managed_resources_service.rb index b23c161c2c0..e7b817b69fa 100644 --- a/app/services/environments/delete_managed_resources_service.rb +++ b/app/services/environments/delete_managed_resources_service.rb @@ -4,8 +4,9 @@ module Environments class DeleteManagedResourcesService include Gitlab::Utils::StrongMemoize - def initialize(environment) + def initialize(environment, current_user:) @environment = environment + @current_user = current_user end def execute @@ -15,12 +16,14 @@ module Environments Clusters::Agents::ManagedResources::DeleteWorker.perform_async(managed_resource.id) + emit_event + ServiceResponse.success end private - attr_reader :environment + attr_reader :environment, :current_user def can_delete_resources? environment.stopped? && @@ -33,5 +36,18 @@ module Environments environment.managed_resources.completed.order_id_desc.first end strong_memoize_attr :managed_resource + + def emit_event + Gitlab::InternalEvents.track_event( + 'delete_environment_for_managed_resource', + user: current_user, + project: environment.project, + additional_properties: { + label: environment.project.namespace.actual_plan_name, + property: environment.tier, + value: environment.id + } + ) + end end end diff --git a/app/services/environments/stop_service.rb b/app/services/environments/stop_service.rb index 2b555032022..9bc04dd4a63 100644 --- a/app/services/environments/stop_service.rb +++ b/app/services/environments/stop_service.rb @@ -78,7 +78,7 @@ module Environments end def delete_managed_resources(environment) - Environments::DeleteManagedResourcesService.new(environment).execute + Environments::DeleteManagedResourcesService.new(environment, current_user:).execute end end end diff --git a/config/events/delete_environment_for_managed_resource.yml b/config/events/delete_environment_for_managed_resource.yml new file mode 100644 index 00000000000..6e897302184 --- /dev/null +++ b/config/events/delete_environment_for_managed_resource.yml @@ -0,0 +1,23 @@ +--- +description: Tracks deleted managed resource +internal_events: true +action: delete_environment_for_managed_resource +identifiers: +- project +- namespace +- user +additional_properties: + label: + description: pricing tier + property: + description: environment tier + value: + description: environment id +product_group: environments +product_categories: +- environment_management +milestone: '18.0' +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/188965 +tiers: +- premium +- ultimate diff --git a/config/metrics/counts_all/count_distinct_user_id_from_delete_environment_for_managed_resource.yml b/config/metrics/counts_all/count_distinct_user_id_from_delete_environment_for_managed_resource.yml new file mode 100644 index 00000000000..b89c1f8f9f6 --- /dev/null +++ b/config/metrics/counts_all/count_distinct_user_id_from_delete_environment_for_managed_resource.yml @@ -0,0 +1,22 @@ +--- +key_path: redis_hll_counters.count_distinct_user_id_from_delete_environment_for_managed_resource +description: Count of unique users who deleted the environment using managed resource +product_group: environments +product_categories: +- environment_management +performance_indicator_type: [] +value_type: number +status: active +milestone: '18.0' +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/188965 +time_frame: +- 28d +- 7d +data_source: internal_events +data_category: optional +tiers: +- premium +- ultimate +events: +- name: delete_environment_for_managed_resource + unique: user.id diff --git a/config/metrics/counts_all/count_distinct_value_from_delete_environment_for_managed_resource.yml b/config/metrics/counts_all/count_distinct_value_from_delete_environment_for_managed_resource.yml new file mode 100644 index 00000000000..f44f16f774d --- /dev/null +++ b/config/metrics/counts_all/count_distinct_value_from_delete_environment_for_managed_resource.yml @@ -0,0 +1,22 @@ +--- +key_path: redis_hll_counters.count_distinct_value_from_delete_environment_for_managed_resource +description: Count of unique environments that deleted environments using managed resource +product_group: environments +product_categories: +- environment_management +performance_indicator_type: [] +value_type: number +status: active +milestone: '18.0' +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/188965 +time_frame: +- 28d +- 7d +data_source: internal_events +data_category: optional +tiers: +- premium +- ultimate +events: +- name: delete_environment_for_managed_resource + unique: value diff --git a/db/post_migrate/20250430105707_re_prepare_async_index_on_merge_request_commits_metadata_id.rb b/db/post_migrate/20250430105707_re_prepare_async_index_on_merge_request_commits_metadata_id.rb new file mode 100644 index 00000000000..c78bc4a94ff --- /dev/null +++ b/db/post_migrate/20250430105707_re_prepare_async_index_on_merge_request_commits_metadata_id.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +class RePrepareAsyncIndexOnMergeRequestCommitsMetadataId < Gitlab::Database::Migration[2.3] + milestone '18.0' + + INDEX_NAME = 'index_mrdc_on_merge_request_commits_metadata_id' + + # TODO: Index to be created synchronously in https://gitlab.com/gitlab-org/gitlab/-/issues/527227 + # rubocop:disable Migration/PreventIndexCreation -- this index is required as + # we will be querying data from `merge_request_commits_metadata_id` and joining + # by this column. + def up + prepare_async_index :merge_request_diff_commits, :merge_request_commits_metadata_id, name: INDEX_NAME, + where: "merge_request_commits_metadata_id IS NOT NULL" + end + # rubocop:enable Migration/PreventIndexCreation + + def down + unprepare_async_index :merge_request_diff_commits, :merge_request_commits_metadata_id, name: INDEX_NAME + end +end diff --git a/db/schema_migrations/20250430105707 b/db/schema_migrations/20250430105707 new file mode 100644 index 00000000000..1c6280a3d3a --- /dev/null +++ b/db/schema_migrations/20250430105707 @@ -0,0 +1 @@ +06daee445c8a41f7d539e05e6f75ff810b387185491d15362bdaa51a70e6bcbe \ No newline at end of file diff --git a/doc/.vale/gitlab_base/SubstitutionWarning.yml b/doc/.vale/gitlab_base/SubstitutionWarning.yml index d344debfb6b..9eec13f1024 100644 --- a/doc/.vale/gitlab_base/SubstitutionWarning.yml +++ b/doc/.vale/gitlab_base/SubstitutionWarning.yml @@ -45,6 +45,8 @@ swap: log-in: "sign in" logged in user: "authenticated user" logged-in user: "authenticated user" + lower case: "lowercase" + lower-case: "lowercase" machine-learning: "machine learning" modal dialog: "dialog" modal window: "dialog" @@ -74,6 +76,10 @@ swap: since: "because' or 'after" source (?:install|installation): self-compiled installation source (?:installs|installations): self-compiled installations + sub directories: "subdirectories" + sub-directories: "subdirectories" + sub directory: "subdirectory" + sub-directory: "subdirectory" sub group: "subgroup" sub-group: "subgroup" sub-groups: "subgroups" diff --git a/doc/development/database/adding_database_indexes.md b/doc/development/database/adding_database_indexes.md index 2f56b3b4d5f..1df376346cb 100644 --- a/doc/development/database/adding_database_indexes.md +++ b/doc/development/database/adding_database_indexes.md @@ -245,9 +245,21 @@ for new and updated records while the removal and fix are in progress. The details of the work might vary and require different approaches. Consult the Database team, reviewers, or maintainers to plan the work. -## Finding Unused Indexes +## Dropping unused indexes -To see which indexes are unused you can run the following query: +Unused indexes should be dropped because they increase [maintainence overhead](#maintenance-overhead), consume +disk space, and can degrade query planning efficiency without providing any performance benefit. +However, dropping an index that's still used could result in query performance degradation or timeouts, +potentially leading to incidents. It's important to [verify the index is unused](#verifying-that-an-index-is-unused) +on both on GitLab.com and self-managed instances prior to removal. + +- For large tables, consider [dropping the index asynchronously](#drop-indexes-asynchronously). +- For partitioned tables, only the parent index can be dropped. PostgreSQL does not permit child indexes + (i.e. the corresponding indexes on its partitions) to be independently removed. + +### Finding possible unused indexes + +To see which indexes are candidates for removal, you can run the following query: ```sql SELECT relname as table_name, indexrelname as index_name, idx_scan, idx_tup_read, idx_tup_fetch, pg_size_pretty(pg_relation_size(indexrelname::regclass)) @@ -259,28 +271,132 @@ AND idx_tup_fetch = 0 ORDER BY pg_relation_size(indexrelname::regclass) desc; ``` -This query outputs a list containing all indexes that are never used and sorts -them by indexes sizes in descending order. This query helps in -determining whether existing indexes are still required. More information on -the meaning of the various columns can be found at +This query outputs a list containing all indexes that have not been used since the stats were last reset and sorts +them by index size in descending order. More information on the meaning of the various columns can be found at . -To determine if an index is still being used on production, use [Grafana](https://dashboards.gitlab.net/explore?schemaVersion=1&panes=%7B%22pum%22%3A%7B%22datasource%22%3A%22mimir-gitlab-gprd%22%2C%22queries%22%3A%5B%7B%22refId%22%3A%22A%22%2C%22expr%22%3A%22sum+by+%28type%29%28rate%28pg_stat_user_indexes_idx_scan%7Benv%3D%5C%22gprd%5C%22%2C+indexrelname%3D%5C%22INSERT+INDEX+NAME+HERE%5C%22%7D%5B30d%5D%29%29%22%2C%22range%22%3Atrue%2C%22instant%22%3Atrue%2C%22datasource%22%3A%7B%22type%22%3A%22prometheus%22%2C%22uid%22%3A%22mimir-gitlab-gprd%22%7D%2C%22editorMode%22%3A%22code%22%2C%22legendFormat%22%3A%22__auto%22%7D%5D%2C%22range%22%3A%7B%22from%22%3A%22now-1h%22%2C%22to%22%3A%22now%22%7D%7D%7D&orgId=1): +For GitLab.com, you can check the latest generated [production reports](https://console.postgres.ai/gitlab/reports/) +on postgres.ai and inspect the `H002 Unused Indexes` file. -```sql -sum by (type)(rate(pg_stat_user_indexes_idx_scan{env="gprd", indexrelname="INSERT INDEX NAME HERE"}[30d])) -``` +{{< alert type="warning" >}} -Because the query output relies on the actual usage of your database, it -may be affected by factors such as: +These reports only show indexes that have no recorded usage **since the last statistics reset.** +They do not guarantee that the indexes are never used. -- Certain queries never being executed, thus not being able to use certain - indexes. -- Certain tables having little data, resulting in PostgreSQL using sequence - scans instead of index scans. +{{< /alert >}} -This data is only reliable for a frequently used database with -plenty of data, and using as many GitLab features as possible. +### Verifying that an index is unused + +This section contains resources to help you evaluate an index and confirm that it's safe to remove. Note that +this is only a suggested guide and is not exhaustive. Ultimately, the goal is to gather enough data to justify +dropping the index. + +Be aware that certain factors can give the false impression that an index is unused, such as: + +- There may be queries that run on self-managed but not on GitLab.com. +- The index may be used for very infrequent processes such as periodic cron jobs. +- On tables that have little data, PostgreSQL may initially prefer a sequential scan over an index scan + until the table is large enough. + +#### Investigating index usage + +1. Start by gathering all the metadata available for the index, verifying its name and definition. + - The index name in the development environment may not match production. It's important to correlate the indexes + based on definition rather than name. To check its definition, you can: + - Manually inspect [db/structure.sql](https://gitlab.com/gitlab-org/gitlab/-/blob/master/db/structure.sql) + (This file does **not** include data on dynamically generated partitions.) + - [Use Database Lab to check the status of an index.](database_lab.md#checking-indexes) + - For partitioned tables, child indexes are often named differently than the parent index. + To list all child indexes, you can: + - Run `\d+ ` in [Database Lab](database_lab.md). + - Run the following query to see the full parent-child index structure in more detail: + + ```sql + SELECT + parent_idx.relname AS parent_index, + child_tbl.relname AS child_table, + child_idx.relname AS child_index, + dep.deptype, + pg_get_indexdef(child_idx.oid) AS child_index_def + FROM + pg_class parent_idx + JOIN pg_depend dep ON dep.refobjid = parent_idx.oid + JOIN pg_class child_idx ON child_idx.oid = dep.objid + JOIN pg_index i ON i.indexrelid = child_idx.oid + JOIN pg_class child_tbl ON i.indrelid = child_tbl.oid + WHERE + parent_idx.relname = ''; + ``` + + - Run the following command in the Rails console: + + ```ruby + Gitlab::Database::PostgresPartitionedTable.by_identifier('public.').indexes + ``` + +1. For GitLab.com, you can view index usage data in [Grafana](https://dashboards.gitlab.net/goto/shHCmIxHg?orgId=1). + - Query the metric `pg_stat_user_indexes_idx_scan` filtered by the relevant index(s) for at least the last 6 months. + The query below shows index usage across all database instances combined. + + ```sql + sum by (indexrelname) (pg_stat_user_indexes_idx_scan{env="gprd", relname=~"", indexrelname=~""}) + ``` + + - For partitioned tables, we must check that **all child indexes are unused** prior to dropping the parent. + +If the data shows that an index has zero or negligible usage, it's a strong candidate for removal. However, keep in mind that +this is limited to usage on GitLab.com. We should still [investigate all related queries](#investigating-related-queries) to +ensure it can be safely removed for self-managed instances. + +An index that shows low usage may still be dropped **if** we can confirm that other existing indexes would sufficiently +support the queries using it. PostgreSQL decides which index to use based on data distribution statistics, so in certain +situations it may slightly prefer one index over another even if both indexes adequately support the query, which may +account for the occasional usage. + +#### Investigating related queries + +The following are ways to find all queries that _may_ utilize the index. It's important to understand the context in +which the queries are or may be executed so that we can determine if the index either: + +- Has no queries on GitLab.com nor on self-managed that depend on it. +- Can be sufficiently supported by other existing indexes. + +1. Investigate the origins of the index. + - Dig through the commit history, related merge requests, and issues that introduced the index. + - Try to find answers to questions such as: + - Why was the index added in the first place? What query was it meant to support? + - Does that query still exist and get executed? + - Is it only applicable to self-managed instances? + +1. Examine queries outputted from running the [`rspec:merge-auto-explain-logs`](https://gitlab.com/gitlab-org/gitlab/-/jobs/9805995367) CI job. + - This job collects and analyzes queries executed through tests. The output is saved as an artifact: `auto_explain/auto_explain.ndjson.gz` + - Since we don't always have 100% test coverage, this job may not capture all possible queries and variations. + +1. Examine queries recorded in [postgres logs](https://log.gprd.gitlab.net/app/r/s/A55hK) on Kibana. + - Generally, you can filter for `json.sql` values that contain the table name and at least one of the columns from the index definition + (including conditions). Example KQL: + + ```plaintext + json.sql: AND (json.sql: ** OR json.sql: **) + ``` + + - While there are many factors that affect index usage, the query's filtering and ordering clauses often have the most influence. + So focus on finding queries with relevant columns in the predicate/conditions rather than in the `SELECT` clause. + - Caveat: We only keep the last 7 days of logs and this data does not apply to self-managed usage. + +1. Manually search through the GitLab codebase. + - This process may be tedious but it's the most reliable way to ensure there are no other queries we missed from the previous actions, + especially ones that are infrequent or only apply to self-managed instances. + - It's possible there are queries that were introduced some time after the index was initially added, + so we can't always depend on the index origins; we must also examine the current state of the codebase. + - To help direct your search, try to gather context about how the table is used and what features access it. Look for queries + that involve any of the columns from the index definition, particularly those that are part of the filtering or ordering clauses. + - Another approach is to conduct a keyword search for the model/table name and any relevant columns. However, this could be a + trickier and long-winded process since some queries may be dynamically compiled from code across multiple files. + +After collecting the relevant queries, you can then obtain [EXPLAIN plans](understanding_explain_plans.md) to help you assess if a query +relies on the index in question. For this process, it's necessary to have a good understanding of how indexes support queries and how +their usage is affected by data distribution changes. We recommend seeking guidance from a database domain expert to help with your assessment. ## Requirements for naming indexes diff --git a/lib/gitlab/auth/oidc/step_up_authentication.rb b/lib/gitlab/auth/oidc/step_up_authentication.rb index c4c8e1f665a..5e493772464 100644 --- a/lib/gitlab/auth/oidc/step_up_authentication.rb +++ b/lib/gitlab/auth/oidc/step_up_authentication.rb @@ -8,6 +8,8 @@ module Gitlab # This module manages the configuration and validation of step-up authentication # requirements for OAuth providers, particularly focusing on admin mode access. module StepUpAuthentication + SESSION_STORE_KEY = 'omniauth_step_up_auth' + STEP_UP_AUTH_SCOPE_ADMIN_MODE = :admin_mode class << self @@ -36,17 +38,13 @@ module Gitlab # @return [Boolean] true if step-up authentication is authenticated def succeeded?(session, scope: STEP_UP_AUTH_SCOPE_ADMIN_MODE) step_up_auth_flows = - session&.dig('omniauth_step_up_auth') - .to_h - .flat_map do |provider, step_up_auth_object| - step_up_auth_object.map do |step_up_auth_scope, _| - ::Gitlab::Auth::Oidc::StepUpAuthenticationFlow.new( - session: session, - provider: provider, - scope: step_up_auth_scope - ) + omniauth_step_up_auth_session_data(session) + &.to_h + &.flat_map do |provider, step_up_auth_object| + step_up_auth_object.map do |step_up_auth_scope, _| + build_flow(provider: provider, session: session, scope: step_up_auth_scope) + end end - end step_up_auth_flows .select do |step_up_auth_flow| step_up_auth_flow.scope.to_s == scope.to_s @@ -79,6 +77,10 @@ module Gitlab oauth_raw_info.slice(*relevant_id_token_claims) end + def omniauth_step_up_auth_session_data(session) + Gitlab::NamespacedSessionStore.new(SESSION_STORE_KEY, session) + end + private def oauth_providers diff --git a/lib/gitlab/auth/oidc/step_up_authentication_flow.rb b/lib/gitlab/auth/oidc/step_up_authentication_flow.rb index a45b1537b5c..ce7361b4660 100644 --- a/lib/gitlab/auth/oidc/step_up_authentication_flow.rb +++ b/lib/gitlab/auth/oidc/step_up_authentication_flow.rb @@ -66,14 +66,17 @@ module Gitlab end def provider_scope_session_data - session.dig('omniauth_step_up_auth', provider.to_s, scope.to_s) + omniauth_step_up_auth_session_data[provider.to_s][scope.to_s] end def update_session_state(new_state) - session['omniauth_step_up_auth'] ||= {} - session['omniauth_step_up_auth'][provider.to_s] ||= {} - session['omniauth_step_up_auth'][provider.to_s][scope.to_s] ||= {} - session['omniauth_step_up_auth'][provider.to_s][scope.to_s]['state'] = new_state.to_s + omniauth_step_up_auth_session_data[provider.to_s] ||= {} + omniauth_step_up_auth_session_data[provider.to_s][scope.to_s] ||= {} + omniauth_step_up_auth_session_data[provider.to_s][scope.to_s]['state'] = new_state.to_s + end + + def omniauth_step_up_auth_session_data + ::Gitlab::Auth::Oidc::StepUpAuthentication.omniauth_step_up_auth_session_data(session) end def conditions_fulfilled?(oidc_id_token_claims) diff --git a/lib/gitlab/namespaced_session_store.rb b/lib/gitlab/namespaced_session_store.rb index 5f1f684f81b..a4ad7fd9d9a 100644 --- a/lib/gitlab/namespaced_session_store.rb +++ b/lib/gitlab/namespaced_session_store.rb @@ -16,7 +16,7 @@ module Gitlab def each(&block) return unless session - session.fetch(@namespace_key, {}).each(&block) + (session[@namespace_key] || {}).each(&block) end def [](key) diff --git a/lib/sidebars/your_work/menus/import_history_menu.rb b/lib/sidebars/your_work/menus/import_history_menu.rb index 79028a85037..ee8d47662b9 100644 --- a/lib/sidebars/your_work/menus/import_history_menu.rb +++ b/lib/sidebars/your_work/menus/import_history_menu.rb @@ -22,7 +22,10 @@ module Sidebars override :render? def render? - !!context.current_user + !!context.current_user && ( + Gitlab::CurrentSettings.bulk_import_enabled? || + Feature.enabled?(:override_bulk_import_disabled, context.current_user, type: :ops) + ) end override :active_routes diff --git a/spec/lib/sidebars/your_work/menus/import_history_menu_spec.rb b/spec/lib/sidebars/your_work/menus/import_history_menu_spec.rb new file mode 100644 index 00000000000..175e7c63cde --- /dev/null +++ b/spec/lib/sidebars/your_work/menus/import_history_menu_spec.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Sidebars::YourWork::Menus::ImportHistoryMenu, feature_category: :navigation do + let(:user) { build_stubbed(:user) } + let(:context) { Sidebars::Context.new(current_user: current_user, container: nil) } + + describe '#render?' do + using RSpec::Parameterized::TableSyntax + + subject { described_class.new(context).render? } + + where(:current_user, :bulk_import_enabled, :feature_flag_enabled, :result) do + nil | true | true | false + user | false | false | false + user | true | false | true + user | false | true | true + user | true | true | true + end + + with_them do + before do + stub_application_setting(bulk_import_enabled: bulk_import_enabled) + stub_feature_flags(override_bulk_import_disabled: feature_flag_enabled) + end + + it { is_expected.to eq(result) } + end + end +end diff --git a/spec/services/environments/delete_managed_resources_service_spec.rb b/spec/services/environments/delete_managed_resources_service_spec.rb index 0c9cc85611c..51fb0b8bb86 100644 --- a/spec/services/environments/delete_managed_resources_service_spec.rb +++ b/spec/services/environments/delete_managed_resources_service_spec.rb @@ -4,7 +4,8 @@ require 'spec_helper' RSpec.describe Environments::DeleteManagedResourcesService, feature_category: :deployment_management do let_it_be(:project) { create(:project) } - let_it_be(:environment) { create(:environment, :stopped, project: project) } + let(:user) { create(:user, developer_of: project) } + let(:environment) { create(:environment, :stopped, project: project) } let_it_be(:agent) { create(:cluster_agent, project: project) } let_it_be(:build) { create(:ci_build, project: project) } @@ -20,9 +21,9 @@ RSpec.describe Environments::DeleteManagedResourcesService, feature_category: :d end describe '#execute' do - subject(:execute) { described_class.new(environment).execute } + subject(:execute) { described_class.new(environment, current_user: user).execute } - it 'sets the status to :deleting and queues the deletion worker' do + it 'sets the status to :deleting, queues the deletion worker' do expect(Clusters::Agents::ManagedResources::DeleteWorker).to receive(:perform_async) .with(managed_resource.id).once.and_call_original @@ -30,6 +31,25 @@ RSpec.describe Environments::DeleteManagedResourcesService, feature_category: :d expect(managed_resource.reload.status).to eq('deleting') end + it 'emits an event and increment metrics' do + expect { execute } + .to trigger_internal_events('delete_environment_for_managed_resource') + .with( + user: user, + project: project, + category: 'InternalEventTracking', + additional_properties: { + label: project.namespace.actual_plan_name, + property: environment.tier, + value: environment.id + }) + .and increment_usage_metrics( + 'redis_hll_counters.count_distinct_user_id_from_delete_environment_for_managed_resource_monthly', + 'redis_hll_counters.count_distinct_user_id_from_delete_environment_for_managed_resource_weekly', + 'redis_hll_counters.count_distinct_value_from_delete_environment_for_managed_resource_monthly', + 'redis_hll_counters.count_distinct_value_from_delete_environment_for_managed_resource_weekly') + end + shared_examples 'resources can not be deleted' do it 'does not attempt to delete resources' do expect(Clusters::Agents::ManagedResources::DeleteWorker).not_to receive(:perform_async) diff --git a/spec/services/environments/stop_service_spec.rb b/spec/services/environments/stop_service_spec.rb index afbfb032dd7..f92340c541a 100644 --- a/spec/services/environments/stop_service_spec.rb +++ b/spec/services/environments/stop_service_spec.rb @@ -25,7 +25,7 @@ RSpec.describe Environments::StopService, feature_category: :continuous_delivery end it 'calls the managed resource deletion service' do - expect_next_instance_of(Environments::DeleteManagedResourcesService, environment) do |service| + expect_next_instance_of(Environments::DeleteManagedResourcesService, environment, current_user: user) do |service| expect(service).to receive(:execute).and_call_original end