From b031a57ae71b1fc61782b891d2a31852ab87e7f3 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Mon, 9 May 2022 18:08:30 +0000 Subject: [PATCH] Add latest changes from gitlab-org/gitlab@master --- .../migration/enqueuer_worker.rb | 36 ++++------- doc/api/features.md | 3 +- doc/development/code_review.md | 4 +- doc/user/group/roadmap/index.md | 64 +++++++++---------- lib/api/features.rb | 2 + metrics_server/metrics_server.rb | 24 ++++--- spec/metrics_server/metrics_server_spec.rb | 40 +++++++++++- spec/requests/api/features_spec.rb | 49 ++++++++++++++ .../migration/enqueuer_worker_spec.rb | 37 ++--------- 9 files changed, 159 insertions(+), 100 deletions(-) diff --git a/app/workers/container_registry/migration/enqueuer_worker.rb b/app/workers/container_registry/migration/enqueuer_worker.rb index a8468c12b8d..fd824ba6f79 100644 --- a/app/workers/container_registry/migration/enqueuer_worker.rb +++ b/app/workers/container_registry/migration/enqueuer_worker.rb @@ -6,7 +6,7 @@ module ContainerRegistry include ApplicationWorker include CronjobQueue # rubocop:disable Scalability/CronWorkerContext include Gitlab::Utils::StrongMemoize - include Gitlab::ExclusiveLeaseHelpers + include ExclusiveLeaseGuard DEFAULT_LEASE_TIMEOUT = 30.minutes.to_i.freeze @@ -18,15 +18,11 @@ module ContainerRegistry def perform re_enqueue = false + try_obtain_lease do + break unless runnable? - took_lease = with_a_lease_on_repository do - if runnable? - re_enqueue = handle_aborted_migration || handle_next_migration - end + re_enqueue = handle_aborted_migration || handle_next_migration end - - log_extra_metadata_on_done(:lease_already_taken, true) unless took_lease - re_enqueue_if_capacity if re_enqueue end @@ -37,20 +33,6 @@ module ContainerRegistry private - def with_a_lease_on_repository - repository = next_aborted_repository || next_repository - - if repository - in_lock(lease_key_for(repository), retries: 0, ttl: DEFAULT_LEASE_TIMEOUT) { yield } - else - log_extra_metadata_on_done(:no_container_repository_found, true) - end - - true - rescue FailedToObtainLockError - false - end - def handle_aborted_migration return unless next_aborted_repository @@ -183,8 +165,14 @@ module ContainerRegistry log_extra_metadata_on_done(:container_repository_migration_state, repository.migration_state) end - def lease_key_for(repository) - "container_registry:migration:enqueuer_worker:for:#{repository.id}" + # used by ExclusiveLeaseGuard + def lease_key + 'container_registry:migration:enqueuer_worker' + end + + # used by ExclusiveLeaseGuard + def lease_timeout + DEFAULT_LEASE_TIMEOUT end end end diff --git a/doc/api/features.md b/doc/api/features.md index 0ad76829651..c3800933920 100644 --- a/doc/api/features.md +++ b/doc/api/features.md @@ -129,11 +129,12 @@ POST /features/:name | `feature_group` | string | no | A Feature group name | | `user` | string | no | A GitLab username | | `group` | string | no | A GitLab group's path, for example `gitlab-org` | +| `namespace` | string | no | A GitLab group or user namespace's path, for example `gitlab-org` or username path | | `project` | string | no | A projects path, for example `gitlab-org/gitlab-foss` | | `force` | boolean | no | Skip feature flag validation checks, such as a YAML definition | You can enable or disable a feature for a `feature_group`, a `user`, -a `group`, and a `project` in a single API call. +a `group`, a `namespace` and a `project` in a single API call. ```shell curl --data "value=30" --header "PRIVATE-TOKEN: " "https://gitlab.example.com/api/v4/features/new_library" diff --git a/doc/development/code_review.md b/doc/development/code_review.md index 5f1e02c9da6..a9e74fc2be4 100644 --- a/doc/development/code_review.md +++ b/doc/development/code_review.md @@ -219,7 +219,9 @@ should be confident that: The best way to do this, and to avoid unnecessary back-and-forth with reviewers, is to perform a self-review of your own merge request, following the -[Code Review](#reviewing-a-merge-request) guidelines. +[Code Review](#reviewing-a-merge-request) guidelines. During this self-review, +try to include comments in the MR on lines +where decisions or trade-offs were made, or where a contextual explanation might aid the reviewer in more easily understanding the code. To reach the required level of confidence in their solution, an author is expected to involve other people in the investigation and implementation processes as diff --git a/doc/user/group/roadmap/index.md b/doc/user/group/roadmap/index.md index 89c5c6ed466..10ca6d139ad 100644 --- a/doc/user/group/roadmap/index.md +++ b/doc/user/group/roadmap/index.md @@ -1,5 +1,4 @@ --- -type: reference stage: Plan group: Product Planning info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/engineering/ux/technical-writing/#assignments @@ -7,7 +6,6 @@ info: To determine the technical writer assigned to the Stage/Group associated w # Roadmap **(PREMIUM)** -> - Introduced in GitLab 10.5. > - [Moved](https://gitlab.com/gitlab-org/gitlab/-/issues/198062) from GitLab Ultimate to GitLab Premium in 12.9. > - In [GitLab 12.9](https://gitlab.com/gitlab-org/gitlab/-/issues/5164) and later, the epic bars show epics' title, progress, and completed weight percentage. > - Milestones appear in roadmaps in [GitLab 12.10](https://gitlab.com/gitlab-org/gitlab/-/issues/6802), and later. @@ -27,10 +25,10 @@ You can expand epics that contain child epics to show their child epics in the r You can select the chevron (**{chevron-down}**) next to the epic title to expand and collapse the child epics. -On top of the milestone bars, you can see their title. -When you hover over a milestone bar or title, a popover appears with its title, start date, and due -date. You can also select the chevron (**{chevron-down}**) next to the **Milestones** heading to -toggle the list of the milestone bars. +On top of the milestone bars, you can see their title. When you point to a +milestone bar or title, a popover appears with its title, start date, and due +date. You can also select the chevron (**{chevron-down}**) next to the **Milestones** +heading to toggle the list of the milestone bars. ![roadmap view](img/roadmap_view_v14_3.png) @@ -41,8 +39,8 @@ toggle the list of the milestone bars. > - Filtering by epic [introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/218623) in GitLab 13.11. > - Filtering by milestone [feature flag removed](https://gitlab.com/gitlab-org/gitlab/-/issues/323917) in GitLab 14.5. -WARNING: -Filtering roadmaps by milestone might not be available to you. Check the **version history** note above for details. +NOTE: +Filtering roadmaps by milestone might not be available to you. Be sure to review this section's version history for details. When you want to explore a roadmap, there are several ways to make it easier by sorting epics or filtering them by what's important for you. @@ -52,8 +50,9 @@ You can sort epics in the Roadmap view by: - Start date - Due date -Each option contains a button that toggles the sort order between **ascending** and **descending**. -The sort option and order persist when browsing Epics, including the [epics list view](../epics/index.md). +Each option contains a button that toggles the sort order between **ascending** +and **descending**. The sort option and order persist when browsing Epics, including +the [epics list view](../epics/index.md). You can also filter epics in the Roadmap view by the epics': @@ -66,7 +65,7 @@ You can also filter epics in the Roadmap view by the epics': ![roadmap date range in weeks](img/roadmap_filters_v13_11.png) -Roadmaps can also be [visualized inside an epic](../epics/index.md#roadmap-in-epics). +You can also [visualize roadmaps inside of an epic](../epics/index.md#roadmap-in-epics). ### Roadmap settings @@ -78,38 +77,40 @@ When you enable the roadmap settings sidebar, you can use it to refine epics sho You can configure the following: - Select date range. -- Turn milestones on or off and select whether to show all, group, subgroup, or project milestones. +- Turn milestones on or off, and select whether to show all, group, subgroup, or + project milestones. - Show all, open, or closed epics. - Turn progress tracking for child issues on or off and select whether to use issue weights or counts. - The progress tracking setting is not saved in user preferences but is saved or shared using URL parameters. +The progress tracking setting isn't saved in user preferences, but is saved or +shared using URL parameters. ## Timeline duration -> - Introduced in GitLab 11.0. -> - [Moved](https://gitlab.com/gitlab-org/gitlab/-/issues/198062) from GitLab Ultimate to GitLab Premium in 12.9. +> [Moved](https://gitlab.com/gitlab-org/gitlab/-/issues/198062) from GitLab Ultimate to GitLab Premium in 12.9. ### Date range presets -> - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/204994) in GitLab 14.3. [Deployed behind the `roadmap_daterange_filter` flag](../../../administration/feature_flags.md), disabled by default. +> - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/204994) in GitLab 14.3 [with a flag](../../../administration/feature_flags.md) named `roadmap_daterange_filter`. Disabled by default. > - [Enabled on GitLab.com and self-managed](https://gitlab.com/gitlab-org/gitlab/-/issues/323917) in GitLab 14.3. -> - [Feature flag `roadmap_daterange_filter` removed](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/72419) in GitLab 14.5. +> - Generally available in GitLab 14.5. [Feature flag `roadmap_daterange_filter`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/72419) removed. -Roadmap provides three date range options, each with predetermined timeline duration: +Roadmap provides these date range options, each with a predetermined timeline duration: -- **This quarter**: includes weeks present in current quarter. -- **This year**: includes weeks or months present in current year. -- **Within 3 years**: includes weeks, months, or quarters present in the previous 18 months and - upcoming 18 months (that is, three years in total). +- **This quarter**: Includes the weeks present in the current quarter. +- **This year**: Includes the weeks or months present in the current year. +- **Within 3 years**: Includes the weeks, months, or quarters present both in + the previous 18 months and the upcoming 18 months (three years in total). ### Layout presets -Depending on selected [date range preset](#date-range-presets), Roadmap supports the following layout presets: +Depending on selected [date range preset](#date-range-presets), Roadmap supports +these layout presets: -- **Quarters**: only available when the "Within 3 years" date range is selected. -- **Months**: available when either "This year" or "Within 3 years" date range is selected. -- **Weeks** (default): available for all the date range presets. +- **Quarters**: Available only when the **Within 3 years** date range is selected. +- **Months**: Available when either **This year** or **Within 3 years** date range is selected. +- **Weeks** (default): Available for all the date range presets. ### Quarters @@ -125,12 +126,11 @@ the timeline header represent the month of the quarter. ![roadmap date range in months](img/roadmap_timeline_months.png) -In the **Months** preset, roadmap shows epics and milestones which have start or due dates -**falling within** or -**going through** currently selected date range preset, where **today** -is shown by the vertical red line in the timeline. The sub-headers underneath the month name on -the timeline header represent the date on starting day (Sunday) of the week. This preset is -selected by default. +In the **Months** preset, roadmap shows epics and milestones which have start or +due dates **falling within** or **going through** currently selected date range +preset, where **today** is shown by the vertical red line in the timeline. The +sub-headers underneath the month name on the timeline header represent the date +on the start day (Sunday) of the week. This preset is selected by default. ### Weeks diff --git a/lib/api/features.rb b/lib/api/features.rb index 51d1e5b4628..bff2817a2ec 100644 --- a/lib/api/features.rb +++ b/lib/api/features.rb @@ -70,12 +70,14 @@ module API optional :feature_group, type: String, desc: 'A Feature group name' optional :user, type: String, desc: 'A GitLab username' optional :group, type: String, desc: "A GitLab group's path, such as 'gitlab-org'" + optional :namespace, type: String, desc: "A GitLab group or user namespace path, such as 'gitlab-org'" optional :project, type: String, desc: 'A projects path, like gitlab-org/gitlab-ce' optional :force, type: Boolean, desc: 'Skip feature flag validation checks, ie. YAML definition' mutually_exclusive :key, :feature_group mutually_exclusive :key, :user mutually_exclusive :key, :group + mutually_exclusive :key, :namespace mutually_exclusive :key, :project end post ':name' do diff --git a/metrics_server/metrics_server.rb b/metrics_server/metrics_server.rb index 309334cd899..a5c448b5105 100644 --- a/metrics_server/metrics_server.rb +++ b/metrics_server/metrics_server.rb @@ -38,7 +38,8 @@ class MetricsServer # rubocop:disable Gitlab/NamespacedClass def spawn(target, metrics_dir:, **options) return spawn_ruby_server(target, metrics_dir: metrics_dir, **options) unless new_metrics_server? - settings = settings_for(target) + name = settings_key(target) + settings = ::Settings.monitoring[name] path = options[:path]&.then { |p| Pathname.new(p) } || Pathname.new('') cmd = path.join('gitlab-metrics-exporter').to_path env = { @@ -48,6 +49,13 @@ class MetricsServer # rubocop:disable Gitlab/NamespacedClass 'GME_SERVER_PORT' => settings['port'].to_s } + if settings['log_enabled'] + env['GME_LOG_FILE'] = File.join(Rails.root, 'log', "#{name}.log") + env['GME_LOG_LEVEL'] = 'info' + else + env['GME_LOG_LEVEL'] = 'quiet' + end + Process.spawn(env, cmd, err: $stderr, out: $stdout, pgroup: true).tap do |pid| Process.detach(pid) end @@ -102,14 +110,12 @@ class MetricsServer # rubocop:disable Gitlab/NamespacedClass raise "Target must be one of [puma,sidekiq]" unless %w(puma sidekiq).include?(target) end - def settings_for(target) - settings_key = - case target - when 'puma' then 'web_exporter' - when 'sidekiq' then 'sidekiq_exporter' - else ensure_valid_target!(target) - end - ::Settings.monitoring[settings_key] + def settings_key(target) + case target + when 'puma' then 'web_exporter' + when 'sidekiq' then 'sidekiq_exporter' + else ensure_valid_target!(target) + end end end diff --git a/spec/metrics_server/metrics_server_spec.rb b/spec/metrics_server/metrics_server_spec.rb index 7cc4f6724c4..4c188a6ba29 100644 --- a/spec/metrics_server/metrics_server_spec.rb +++ b/spec/metrics_server/metrics_server_spec.rb @@ -97,18 +97,38 @@ RSpec.describe MetricsServer do # rubocop:disable RSpec/FilePath end context 'for Golang server' do + let(:log_enabled) { false } + let(:settings) do + { + 'web_exporter' => { + 'enabled' => true, + 'address' => 'localhost', + 'port' => '8083', + 'log_enabled' => log_enabled + }, + 'sidekiq_exporter' => { + 'enabled' => true, + 'address' => 'localhost', + 'port' => '8082', + 'log_enabled' => log_enabled + } + } + end + let(:expected_port) { target == 'puma' ? '8083' : '8082' } let(:expected_env) do { 'GME_MMAP_METRICS_DIR' => metrics_dir, 'GME_PROBES' => 'self,mmap', 'GME_SERVER_HOST' => 'localhost', - 'GME_SERVER_PORT' => expected_port + 'GME_SERVER_PORT' => expected_port, + 'GME_LOG_LEVEL' => 'quiet' } end before do stub_env('GITLAB_GOLANG_METRICS_SERVER', '1') + allow(::Settings).to receive(:monitoring).and_return(settings) end it 'spawns a new server process and returns its PID' do @@ -133,6 +153,24 @@ RSpec.describe MetricsServer do # rubocop:disable RSpec/FilePath described_class.spawn(target, metrics_dir: metrics_dir, path: '/path/to/gme/') end + + context 'when logs are enabled' do + let(:log_enabled) { true } + let(:expected_log_file) { target == 'puma' ? 'web_exporter.log' : 'sidekiq_exporter.log' } + + it 'sets log related environment variables' do + expect(Process).to receive(:spawn).with( + expected_env.merge( + 'GME_LOG_LEVEL' => 'info', + 'GME_LOG_FILE' => File.join(Rails.root, 'log', expected_log_file) + ), + 'gitlab-metrics-exporter', + hash_including(pgroup: true) + ).and_return(99) + + described_class.spawn(target, metrics_dir: metrics_dir) + end + end end end end diff --git a/spec/requests/api/features_spec.rb b/spec/requests/api/features_spec.rb index 6c88874dd5d..4e75b0510d0 100644 --- a/spec/requests/api/features_spec.rb +++ b/spec/requests/api/features_spec.rb @@ -310,6 +310,55 @@ RSpec.describe API::Features, stub_feature_flags: false do 'definition' => known_feature_flag_definition_hash ) end + + describe 'mutually exclusive parameters' do + shared_examples 'fails to set the feature flag' do + it 'returns an error' do + expect(response).to have_gitlab_http_status(:bad_request) + expect(json_response['error']).to match(/key, \w+ are mutually exclusive/) + end + end + + context 'when key and feature_group are provided' do + before do + post api("/features/#{feature_name}", admin), params: { value: '0.01', key: 'percentage_of_actors', feature_group: 'some-value' } + end + + it_behaves_like 'fails to set the feature flag' + end + + context 'when key and user are provided' do + before do + post api("/features/#{feature_name}", admin), params: { value: '0.01', key: 'percentage_of_actors', user: 'some-user' } + end + + it_behaves_like 'fails to set the feature flag' + end + + context 'when key and group are provided' do + before do + post api("/features/#{feature_name}", admin), params: { value: '0.01', key: 'percentage_of_actors', group: 'somepath' } + end + + it_behaves_like 'fails to set the feature flag' + end + + context 'when key and namespace are provided' do + before do + post api("/features/#{feature_name}", admin), params: { value: '0.01', key: 'percentage_of_actors', namespace: 'somepath' } + end + + it_behaves_like 'fails to set the feature flag' + end + + context 'when key and project are provided' do + before do + post api("/features/#{feature_name}", admin), params: { value: '0.01', key: 'percentage_of_actors', project: 'somepath' } + end + + it_behaves_like 'fails to set the feature flag' + end + end end context 'when the feature exists' do diff --git a/spec/workers/container_registry/migration/enqueuer_worker_spec.rb b/spec/workers/container_registry/migration/enqueuer_worker_spec.rb index 0ae31422d12..d21fa589f98 100644 --- a/spec/workers/container_registry/migration/enqueuer_worker_spec.rb +++ b/spec/workers/container_registry/migration/enqueuer_worker_spec.rb @@ -253,22 +253,12 @@ RSpec.describe ContainerRegistry::Migration::EnqueuerWorker, :aggregate_failures end context 'when no repository qualifies' do - before do - allow(ContainerRepository).to receive(:ready_for_import).and_return(ContainerRepository.none) - end - - context 'idempotency' do - include_examples 'an idempotent worker' do - it_behaves_like 'no action' + include_examples 'an idempotent worker' do + before do + allow(ContainerRepository).to receive(:ready_for_import).and_return(ContainerRepository.none) end - end - context 'logging' do - it 'logs no_container_repository_found' do - expect_log_extra_metadata(no_container_repository_found: true) - - subject - end + it_behaves_like 'no action' end end @@ -334,7 +324,7 @@ RSpec.describe ContainerRegistry::Migration::EnqueuerWorker, :aggregate_failures end context 'with the exclusive lease taken' do - let(:lease_key) { "container_registry:migration:enqueuer_worker:for:#{container_repository.id}" } + let(:lease_key) { worker.send(:lease_key) } before do stub_exclusive_lease_taken(lease_key, timeout: 30.minutes) @@ -343,23 +333,6 @@ RSpec.describe ContainerRegistry::Migration::EnqueuerWorker, :aggregate_failures it 'does not perform' do expect(worker).not_to receive(:runnable?) expect(worker).not_to receive(:re_enqueue_if_capacity) - expect_log_extra_metadata(lease_already_taken: true) - - subject - end - end - - context 'with exclusive lease taken for a different repository' do - let(:lease_key) { "container_registry:migration:enqueuer_worker:for:test" } - - before do - Gitlab::ExclusiveLease.new(lease_key, timeout: 30.minutes).try_obtain - end - - it 'does perform' do - expect(worker).to receive(:runnable?).and_return(true) - expect(worker).to receive(:handle_aborted_migration).and_return(container_repository) - expect(worker).to receive(:re_enqueue_if_capacity) subject end