diff options
112 files changed, 1939 insertions, 566 deletions
diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION index 194413fa84c..03630f1d18e 100644 --- a/GITALY_SERVER_VERSION +++ b/GITALY_SERVER_VERSION @@ -1 +1 @@ -6689311d652362fc41e5b5cb53aeffede352c8b7 +b011445d9ffa82c36c771946e035852888df0730 diff --git a/app/assets/javascripts/ide/components/panes/collapsible_sidebar.vue b/app/assets/javascripts/ide/components/panes/collapsible_sidebar.vue index f1b882d8f29..87019c3b2a5 100644 --- a/app/assets/javascripts/ide/components/panes/collapsible_sidebar.vue +++ b/app/assets/javascripts/ide/components/panes/collapsible_sidebar.vue @@ -1,13 +1,9 @@ <script> import { mapActions, mapState } from 'vuex'; -import tooltip from '~/vue_shared/directives/tooltip'; import IdeSidebarNav from '../ide_sidebar_nav.vue'; export default { name: 'CollapsibleSidebar', - directives: { - tooltip, - }, components: { IdeSidebarNav, }, diff --git a/app/assets/javascripts/notes/components/comment_form.vue b/app/assets/javascripts/notes/components/comment_form.vue index cfdadbceaf6..a33cdfc6e28 100644 --- a/app/assets/javascripts/notes/components/comment_form.vue +++ b/app/assets/javascripts/notes/components/comment_form.vue @@ -3,7 +3,7 @@ import $ from 'jquery'; import { mapActions, mapGetters, mapState } from 'vuex'; import { isEmpty } from 'lodash'; import Autosize from 'autosize'; -import { GlAlert, GlIntersperse, GlLink, GlSprintf, GlButton } from '@gitlab/ui'; +import { GlAlert, GlIntersperse, GlLink, GlSprintf, GlButton, GlIcon } from '@gitlab/ui'; import { __, sprintf } from '~/locale'; import TimelineEntryItem from '~/vue_shared/components/notes/timeline_entry_item.vue'; import { deprecatedCreateFlash as Flash } from '../../flash'; @@ -38,6 +38,7 @@ export default { GlIntersperse, GlLink, GlSprintf, + GlIcon, }, mixins: [issuableStateMixin], props: { @@ -457,7 +458,7 @@ export default { class="btn btn-transparent" @click.prevent="setNoteType('comment')" > - <i aria-hidden="true" class="fa fa-check icon"></i> + <gl-icon name="check" class="icon" /> <div class="description"> <strong>{{ __('Comment') }}</strong> <p> @@ -476,7 +477,7 @@ export default { data-qa-selector="discussion_menu_item" @click.prevent="setNoteType('discussion')" > - <i aria-hidden="true" class="fa fa-check icon"></i> + <gl-icon name="check" class="icon" /> <div class="description"> <strong>{{ __('Start thread') }}</strong> <p>{{ startDiscussionDescription }}</p> diff --git a/app/assets/javascripts/related_issues/components/issue_token.vue b/app/assets/javascripts/related_issues/components/issue_token.vue index bbbdf2cdb49..7f12c10f6a1 100644 --- a/app/assets/javascripts/related_issues/components/issue_token.vue +++ b/app/assets/javascripts/related_issues/components/issue_token.vue @@ -1,5 +1,5 @@ <script> -import { GlIcon } from '@gitlab/ui'; +import { GlIcon, GlTooltipDirective } from '@gitlab/ui'; import { __, sprintf } from '~/locale'; import relatedIssuableMixin from '~/vue_shared/mixins/related_issuable_mixin'; @@ -8,6 +8,9 @@ export default { components: { GlIcon, }, + directives: { + GlTooltip: GlTooltipDirective, + }, mixins: [relatedIssuableMixin], props: { isCondensed: { @@ -52,7 +55,7 @@ export default { <component :is="computedLinkElementType" ref="link" - v-tooltip + v-gl-tooltip :class="{ 'issue-token-link': isCondensed, 'issuable-main-info': !isCondensed, @@ -84,7 +87,7 @@ export default { > <gl-icon v-if="hasState" - v-tooltip + v-gl-tooltip :class="iconClass" :name="iconName" :size="12" @@ -98,7 +101,7 @@ export default { <button v-if="canRemove" ref="removeButton" - v-tooltip + v-gl-tooltip :class="{ 'issue-token-remove-button': isCondensed, 'btn btn-default': !isCondensed, diff --git a/app/assets/javascripts/vue_shared/mixins/related_issuable_mixin.js b/app/assets/javascripts/vue_shared/mixins/related_issuable_mixin.js index c0fc055a01b..56da2637825 100644 --- a/app/assets/javascripts/vue_shared/mixins/related_issuable_mixin.js +++ b/app/assets/javascripts/vue_shared/mixins/related_issuable_mixin.js @@ -1,7 +1,6 @@ import { isEmpty } from 'lodash'; import { sprintf, __ } from '~/locale'; import { formatDate } from '~/lib/utils/datetime_utility'; -import tooltip from '~/vue_shared/directives/tooltip'; import timeagoMixin from '~/vue_shared/mixins/timeago'; const mixins = { @@ -99,9 +98,6 @@ const mixins = { default: () => ({}), }, }, - directives: { - tooltip, - }, mixins: [timeagoMixin], computed: { hasState() { diff --git a/app/assets/stylesheets/framework/dropdowns.scss b/app/assets/stylesheets/framework/dropdowns.scss index e8d37fcf40b..ebb957645a0 100644 --- a/app/assets/stylesheets/framework/dropdowns.scss +++ b/app/assets/stylesheets/framework/dropdowns.scss @@ -407,7 +407,8 @@ } } - &.droplab-item-selected i { + &.droplab-item-selected i, + &.droplab-item-selected svg { visibility: visible; } diff --git a/app/assets/stylesheets/framework/modal.scss b/app/assets/stylesheets/framework/modal.scss index d3a89348e74..399e4c97ad8 100644 --- a/app/assets/stylesheets/framework/modal.scss +++ b/app/assets/stylesheets/framework/modal.scss @@ -160,8 +160,4 @@ body.modal-open { min-height: $modal-body-height; } } - - .checkmark { - color: $green-400; - } } diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 0e49d3aa649..f7f5ef3fbad 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -22,7 +22,7 @@ class ApplicationController < ActionController::Base include Impersonation include Gitlab::Logging::CloudflareHelper include Gitlab::Utils::StrongMemoize - include ControllerWithFeatureCategory + include ::Gitlab::WithFeatureCategory before_action :authenticate_user!, except: [:route_not_found] before_action :enforce_terms!, if: :should_enforce_terms? diff --git a/app/controllers/concerns/controller_with_feature_category.rb b/app/controllers/concerns/controller_with_feature_category.rb deleted file mode 100644 index c1ff9ef2e69..00000000000 --- a/app/controllers/concerns/controller_with_feature_category.rb +++ /dev/null @@ -1,48 +0,0 @@ -# frozen_string_literal: true - -module ControllerWithFeatureCategory - extend ActiveSupport::Concern - include Gitlab::ClassAttributes - - class_methods do - def feature_category(category, actions = []) - feature_category_configuration[category] ||= [] - feature_category_configuration[category] += actions.map(&:to_s) - - validate_config!(feature_category_configuration) - end - - def feature_category_for_action(action) - category_config = feature_category_configuration.find do |_, actions| - actions.empty? || actions.include?(action) - end - - category_config&.first || superclass_feature_category_for_action(action) - end - - private - - def validate_config!(config) - empty = config.find { |_, actions| actions.empty? } - duplicate_actions = config.values.flatten.group_by(&:itself).select { |_, v| v.count > 1 }.keys - - if config.length > 1 && empty - raise ArgumentError, "#{empty.first} is defined for all actions, but other categories are set" - end - - if duplicate_actions.any? - raise ArgumentError, "Actions have multiple feature categories: #{duplicate_actions.join(', ')}" - end - end - - def feature_category_configuration - class_attributes[:feature_category_config] ||= {} - end - - def superclass_feature_category_for_action(action) - return unless superclass.respond_to?(:feature_category_for_action) - - superclass.feature_category_for_action(action) - end - end -end diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index 55f3b51ac00..1599f17dd7e 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -293,6 +293,9 @@ class ApplicationSetting < ApplicationRecord validates :container_registry_delete_tags_service_timeout, numericality: { only_integer: true, greater_than_or_equal_to: 0 } + validates :container_registry_expiration_policies_worker_capacity, + numericality: { only_integer: true, greater_than_or_equal_to: 0 } + SUPPORTED_KEY_TYPES.each do |type| validates :"#{type}_key_restriction", presence: true, key_restriction: { type: type } end diff --git a/app/models/application_setting_implementation.rb b/app/models/application_setting_implementation.rb index 8a7bd5a7ad9..ee4caa44bab 100644 --- a/app/models/application_setting_implementation.rb +++ b/app/models/application_setting_implementation.rb @@ -167,7 +167,8 @@ module ApplicationSettingImplementation user_default_internal_regex: nil, user_show_add_ssh_key_message: true, wiki_page_max_content_bytes: 50.megabytes, - container_registry_delete_tags_service_timeout: 100 + container_registry_delete_tags_service_timeout: 250, + container_registry_expiration_policies_worker_capacity: 0 } end diff --git a/app/models/ci/build_trace_chunk.rb b/app/models/ci/build_trace_chunk.rb index 6926ccd9438..38e6ecf602c 100644 --- a/app/models/ci/build_trace_chunk.rb +++ b/app/models/ci/build_trace_chunk.rb @@ -136,9 +136,10 @@ module Ci # We are using optimistic locking combined with Redis locking to ensure # that a chunk gets migrated properly. # - # We are catching an exception related to an exclusive lock not being - # acquired because it is creating a lot of noise, and is a result of - # duplicated workers running in parallel for the same build trace chunk. + # We are using until_executed deduplication strategy for workers, + # which should prevent duplicated workers running in parallel for the same build trace, + # and causing an exception related to an exclusive lock not being + # acquired # def persist_data! in_lock(*lock_params) do # exclusive Redis lock is acquired first @@ -150,6 +151,8 @@ module Ci end rescue FailedToObtainLockError metrics.increment_trace_operation(operation: :stalled) + + raise FailedToPersistDataError, 'Data migration failed due to a worker duplication' rescue ActiveRecord::StaleObjectError raise FailedToPersistDataError, <<~MSG Data migration race condition detected diff --git a/app/models/ci/daily_build_group_report_result.rb b/app/models/ci/daily_build_group_report_result.rb index 9c1670378d6..c829ee57b37 100644 --- a/app/models/ci/daily_build_group_report_result.rb +++ b/app/models/ci/daily_build_group_report_result.rb @@ -14,6 +14,7 @@ module Ci scope :with_included_projects, -> { includes(:project) } scope :by_projects, -> (ids) { where(project_id: ids) } scope :with_coverage, -> { where("(data->'coverage') IS NOT NULL") } + scope :with_default_branch, -> { where(default_branch: true) } store_accessor :data, :coverage diff --git a/app/models/container_expiration_policy.rb b/app/models/container_expiration_policy.rb index 641d244b665..3f91f6c796a 100644 --- a/app/models/container_expiration_policy.rb +++ b/app/models/container_expiration_policy.rb @@ -5,6 +5,13 @@ class ContainerExpirationPolicy < ApplicationRecord include UsageStatistics include EachBatch + POLICY_PARAMS = %w[ + older_than + keep_n + name_regex + name_regex_keep + ].freeze + belongs_to :project, inverse_of: :container_expiration_policy delegate :container_repositories, to: :project @@ -20,8 +27,8 @@ class ContainerExpirationPolicy < ApplicationRecord scope :active, -> { where(enabled: true) } scope :preloaded, -> { preload(project: [:route]) } - def self.executable - runnable_schedules.where( + def self.with_container_repositories + where( 'EXISTS (?)', ContainerRepository.select(1) .where( @@ -67,4 +74,8 @@ class ContainerExpirationPolicy < ApplicationRecord def disable! update_attribute(:enabled, false) end + + def policy_params + attributes.slice(*POLICY_PARAMS) + end end diff --git a/app/models/container_repository.rb b/app/models/container_repository.rb index d97b8776085..4adbd37608f 100644 --- a/app/models/container_repository.rb +++ b/app/models/container_repository.rb @@ -3,6 +3,9 @@ class ContainerRepository < ApplicationRecord include Gitlab::Utils::StrongMemoize include Gitlab::SQL::Pattern + include EachBatch + + WAITING_CLEANUP_STATUSES = %i[cleanup_scheduled cleanup_unfinished].freeze belongs_to :project @@ -10,6 +13,7 @@ class ContainerRepository < ApplicationRecord validates :name, uniqueness: { scope: :project_id } enum status: { delete_scheduled: 0, delete_failed: 1 } + enum expiration_policy_cleanup_status: { cleanup_unscheduled: 0, cleanup_scheduled: 1, cleanup_unfinished: 2, cleanup_ongoing: 3 } delegate :client, to: :registry @@ -24,7 +28,9 @@ class ContainerRepository < ApplicationRecord ContainerRepository .joins("INNER JOIN (#{project_scope.to_sql}) projects on projects.id=container_repositories.project_id") end + scope :for_project_id, ->(project_id) { where(project_id: project_id) } scope :search_by_name, ->(query) { fuzzy_search(query, [:name], use_minimum_char_limit: false) } + scope :waiting_for_cleanup, -> { where(expiration_policy_cleanup_status: WAITING_CLEANUP_STATUSES) } def self.exists_by_path?(path) where( diff --git a/app/services/ci/daily_build_group_report_result_service.rb b/app/services/ci/daily_build_group_report_result_service.rb index c32fc27c274..bc966fb9634 100644 --- a/app/services/ci/daily_build_group_report_result_service.rb +++ b/app/services/ci/daily_build_group_report_result_service.rb @@ -13,7 +13,8 @@ module Ci project_id: pipeline.project_id, ref_path: pipeline.source_ref_path, date: pipeline.created_at.to_date, - last_pipeline_id: pipeline.id + last_pipeline_id: pipeline.id, + default_branch: pipeline.default_branch? } aggregate(pipeline.builds.with_coverage).map do |group_name, group| diff --git a/app/services/container_expiration_policies/cleanup_service.rb b/app/services/container_expiration_policies/cleanup_service.rb new file mode 100644 index 00000000000..f2bc2beab63 --- /dev/null +++ b/app/services/container_expiration_policies/cleanup_service.rb @@ -0,0 +1,53 @@ +# frozen_string_literal: true + +module ContainerExpirationPolicies + class CleanupService + attr_reader :repository + + def initialize(repository) + @repository = repository + end + + def execute + return ServiceResponse.error(message: 'no repository') unless repository + + repository.start_expiration_policy! + + result = Projects::ContainerRepository::CleanupTagsService + .new(project, nil, policy_params.merge('container_expiration_policy' => true)) + .execute(repository) + + if result[:status] == :success + repository.update!( + expiration_policy_cleanup_status: :cleanup_unscheduled, + expiration_policy_started_at: nil + ) + success(:finished) + else + repository.cleanup_unfinished! + + success(:unfinished) + end + end + + private + + def success(cleanup_status) + ServiceResponse.success(message: "cleanup #{cleanup_status}", payload: { cleanup_status: cleanup_status, container_repository_id: repository.id }) + end + + def policy_params + return {} unless policy + + policy.policy_params + end + + def policy + project.container_expiration_policy + end + + def project + repository&.project + end + end +end diff --git a/app/services/container_expiration_policy_service.rb b/app/services/container_expiration_policy_service.rb index 80f32298323..cf5d702a9ef 100644 --- a/app/services/container_expiration_policy_service.rb +++ b/app/services/container_expiration_policy_service.rb @@ -4,20 +4,14 @@ class ContainerExpirationPolicyService < BaseService InvalidPolicyError = Class.new(StandardError) def execute(container_expiration_policy) - unless container_expiration_policy.valid? - container_expiration_policy.disable! - raise InvalidPolicyError - end - container_expiration_policy.schedule_next_run! container_expiration_policy.container_repositories.find_each do |container_repository| CleanupContainerRepositoryWorker.perform_async( nil, container_repository.id, - container_expiration_policy.attributes - .except('created_at', 'updated_at') - .merge(container_expiration_policy: true) + container_expiration_policy.policy_params + .merge(container_expiration_policy: true) ) end end diff --git a/app/views/groups/_home_panel.html.haml b/app/views/groups/_home_panel.html.haml index 97e48cdec8c..a50327ecbec 100644 --- a/app/views/groups/_home_panel.html.haml +++ b/app/views/groups/_home_panel.html.haml @@ -35,7 +35,7 @@ %li.droplab-item-selected.qa-new-project-option{ role: "button", data: { value: "new-project", text: new_project_label } } .menu-item .icon-container - = icon("check", class: "list-item-checkmark") + = sprite_icon("check", css_class: "list-item-checkmark") .description %strong= new_project_label %span= s_("GroupsTree|Create a project in this group.") @@ -43,7 +43,7 @@ %li.qa-new-subgroup-option{ role: "button", data: { value: "new-subgroup", text: new_subgroup_label } } .menu-item .icon-container - = icon("check", class: "list-item-checkmark") + = sprite_icon("check", css_class: "list-item-checkmark") .description %strong= new_subgroup_label %span= s_("GroupsTree|Create a subgroup in this group.") diff --git a/app/views/import/google_code/status.html.haml b/app/views/import/google_code/status.html.haml index 72112c128cb..0004f0de69f 100644 --- a/app/views/import/google_code/status.html.haml +++ b/app/views/import/google_code/status.html.haml @@ -43,7 +43,7 @@ - case project.import_status - when 'finished' %span - %i.fa.fa-check + = sprite_icon('check') = _("done") - when 'started' = loading_icon diff --git a/app/views/projects/blob/viewers/_gitlab_ci_yml.html.haml b/app/views/projects/blob/viewers/_gitlab_ci_yml.html.haml index 61d67a88a5a..3326cded42a 100644 --- a/app/views/projects/blob/viewers/_gitlab_ci_yml.html.haml +++ b/app/views/projects/blob/viewers/_gitlab_ci_yml.html.haml @@ -1,8 +1,8 @@ - if viewer.valid?(project: @project, sha: @commit.sha, user: @current_user) - = icon('check fw') + = sprite_icon('check') This GitLab CI configuration is valid. - else - = icon('warning fw') + = sprite_icon('warning-solid') This GitLab CI configuration is invalid: = viewer.validation_message(project: @project, sha: @commit.sha, user: @current_user) diff --git a/app/views/projects/blob/viewers/_metrics_dashboard_yml.html.haml b/app/views/projects/blob/viewers/_metrics_dashboard_yml.html.haml index de9c6c5320f..9c3f9b6c9fd 100644 --- a/app/views/projects/blob/viewers/_metrics_dashboard_yml.html.haml +++ b/app/views/projects/blob/viewers/_metrics_dashboard_yml.html.haml @@ -1,8 +1,8 @@ - if viewer.valid? - = icon('check fw') + = sprite_icon('check') = _('Metrics Dashboard YAML definition is valid.') - else - = icon('warning fw') + = sprite_icon('warning-solid') = _('Metrics Dashboard YAML definition is invalid:') %ul - viewer.errors.each do |error| diff --git a/app/views/projects/blob/viewers/_route_map.html.haml b/app/views/projects/blob/viewers/_route_map.html.haml index 024e9b4ddb2..068cf400cd5 100644 --- a/app/views/projects/blob/viewers/_route_map.html.haml +++ b/app/views/projects/blob/viewers/_route_map.html.haml @@ -1,8 +1,8 @@ - if viewer.valid? - = icon('check fw') + = sprite_icon('check') This Route Map is valid. - else - = icon('warning fw') + = sprite_icon('warning-solid') This Route Map is invalid: = viewer.validation_message diff --git a/app/views/projects/issues/_new_branch.html.haml b/app/views/projects/issues/_new_branch.html.haml index aa95cecb5fe..34260899d94 100644 --- a/app/views/projects/issues/_new_branch.html.haml +++ b/app/views/projects/issues/_new_branch.html.haml @@ -29,7 +29,7 @@ - if can_create_merge_request %li.droplab-item-selected{ role: 'button', data: { value: 'create-mr', text: create_mr_text } } .menu-item.text-nowrap - = icon('check', class: 'icon') + = sprite_icon('check', css_class: 'icon') - if can_create_confidential_merge_request? = _('Create confidential merge request and branch') - else @@ -37,7 +37,7 @@ %li{ class: [!can_create_merge_request && 'droplab-item-selected'], role: 'button', data: { value: 'create-branch', text: _('Create branch') } } .menu-item - = icon('check', class: 'icon') + = sprite_icon('check', css_class: 'icon') = _('Create branch') %li.divider.droplab-item-ignore diff --git a/app/views/projects/issues/export_csv/_modal.html.haml b/app/views/projects/issues/export_csv/_modal.html.haml index 6610af63445..6eae84efdad 100644 --- a/app/views/projects/issues/export_csv/_modal.html.haml +++ b/app/views/projects/issues/export_csv/_modal.html.haml @@ -13,7 +13,7 @@ - issues_count = issuables_count_for_state(:issues, params[:state]) - unless issues_count == -1 # The count timed out .modal-subheader - = icon('check', { class: 'checkmark' }) + = sprite_icon('check', css_class: 'gl-text-green-400') %strong.gl-ml-3 = n_('%d issue selected', '%d issues selected', issues_count) % issues_count .modal-text diff --git a/app/views/projects/mirrors/_ssh_host_keys.html.haml b/app/views/projects/mirrors/_ssh_host_keys.html.haml index 1690188f07a..4e3cd609d75 100644 --- a/app/views/projects/mirrors/_ssh_host_keys.html.haml +++ b/app/views/projects/mirrors/_ssh_host_keys.html.haml @@ -14,7 +14,7 @@ %code= fp.fingerprint - if verified_at .form-text.text-muted.js-fingerprint-verification - %i.fa.fa-check.fingerprint-verified + = sprite_icon('check', css_class: 'gl-text-green-500') Verified by - if verified_by = link_to verified_by.name, user_path(verified_by) diff --git a/app/views/shared/issuable/_close_reopen_report_toggle.html.haml b/app/views/shared/issuable/_close_reopen_report_toggle.html.haml index df441e6d0af..48d1e146629 100644 --- a/app/views/shared/issuable/_close_reopen_report_toggle.html.haml +++ b/app/views/shared/issuable/_close_reopen_report_toggle.html.haml @@ -21,7 +21,7 @@ data: { text: _("Close %{display_issuable_type}") % { display_issuable_type: display_issuable_type }, url: close_issuable_path(issuable), button_class: "#{button_class} btn-close", toggle_class: "#{toggle_class} btn-close-color" } } %button.btn.btn-transparent - = icon('check', class: 'icon') + = sprite_icon('check', css_class: 'icon') .description %strong.title = _('Close') @@ -31,7 +31,7 @@ data: { text: _("Reopen %{display_issuable_type}") % { display_issuable_type: display_issuable_type }, url: reopen_issuable_path(issuable), button_class: "#{button_class} btn-reopen", toggle_class: "#{toggle_class} btn-reopen-color" } } %button.btn.btn-transparent - = icon('check', class: 'icon') + = sprite_icon('check', css_class: 'icon') .description %strong.title = _('Reopen') diff --git a/app/views/shared/members/_invite_group.html.haml b/app/views/shared/members/_invite_group.html.haml index a87a4c6a45c..5e3a6918ab2 100644 --- a/app/views/shared/members/_invite_group.html.haml +++ b/app/views/shared/members/_invite_group.html.haml @@ -13,7 +13,7 @@ = label_tag group_access_field, _("Max access level"), class: "label-bold" .select-wrapper = select_tag group_access_field, options_for_select(access_levels, default_access_level), data: { qa_selector: 'group_access_field' }, class: "form-control select-control" - = icon('chevron-down') + = sprite_icon('chevron-down', css_class: "gl-icon gl-absolute gl-top-3 gl-right-3 gl-text-gray-200") .form-text.text-muted.gl-mb-3 - permissions_docs_path = help_page_path('user/permissions') - link_start = %q{<a href="%{url}">}.html_safe % { url: permissions_docs_path } diff --git a/app/views/shared/members/_invite_member.html.haml b/app/views/shared/members/_invite_member.html.haml index 5f9046b3dcb..18dfbc927ee 100644 --- a/app/views/shared/members/_invite_member.html.haml +++ b/app/views/shared/members/_invite_member.html.haml @@ -13,7 +13,7 @@ = label_tag :access_level, _("Choose a role permission"), class: "label-bold" .select-wrapper = select_tag :access_level, options_for_select(access_levels, default_access_level), class: "form-control project-access-select select-control" - = icon('chevron-down') + = sprite_icon('chevron-down', css_class: "gl-icon gl-absolute gl-top-3 gl-right-3 gl-text-gray-200") .form-text.text-muted.gl-mb-3 - permissions_docs_path = help_page_path('user/permissions') - link_start = %q{<a href="%{url}">}.html_safe % { url: permissions_docs_path } diff --git a/app/views/shared/members/_member.html.haml b/app/views/shared/members/_member.html.haml index 164d38986ec..6315fcf9c01 100644 --- a/app/views/shared/members/_member.html.haml +++ b/app/views/shared/members/_member.html.haml @@ -105,12 +105,12 @@ - if member.can_approve? = link_to polymorphic_path([:approve_access_request, member]), method: :post, - class: "btn btn-success align-self-center m-0 mb-2 #{'mb-sm-0 ml-sm-2' unless force_mobile_view}", + class: "btn btn-success btn-icon gl-button align-self-center m-0 mb-2 #{'mb-sm-0 ml-sm-2' unless force_mobile_view}", title: _('Grant access') do %span{ class: ('d-block d-sm-none' unless force_mobile_view) } = _('Grant access') - unless force_mobile_view - = icon('check inverse', class: 'd-none d-sm-block') + = sprite_icon('check', css_class: 'd-none d-sm-block') - if member.can_remove? - if current_user == user diff --git a/app/views/shared/notes/_comment_button.html.haml b/app/views/shared/notes/_comment_button.html.haml index e151e55d0d2..45af4b51b27 100644 --- a/app/views/shared/notes/_comment_button.html.haml +++ b/app/views/shared/notes/_comment_button.html.haml @@ -10,7 +10,7 @@ %ul#resolvable-comment-menu.dropdown-menu.dropdown-open-top{ data: { dropdown: true } } %li#comment.droplab-item-selected{ data: { value: '', 'submit-text' => _('Comment'), 'close-text' => _("Comment & close %{noteable_name}") % { noteable_name: noteable_name }, 'reopen-text' => _("Comment & reopen %{noteable_name}") % { noteable_name: noteable_name } } } %button.btn.btn-transparent - = icon('check', class: 'icon') + = sprite_icon('check', css_class: 'icon') .description %strong= _("Comment") %p @@ -20,7 +20,7 @@ %li#discussion{ data: { value: 'DiscussionNote', 'submit-text' => _('Start thread'), 'close-text' => _("Start thread & close %{noteable_name}") % { noteable_name: noteable_name }, 'reopen-text' => _("Start thread & reopen %{noteable_name}") % { noteable_name: noteable_name } } } %button.btn.btn-transparent - = icon('check', class: 'icon') + = sprite_icon('check', css_class: 'icon') .description %strong= _("Start thread") %p diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index bde1fd34138..7f5fca18c4c 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -97,7 +97,15 @@ :urgency: :low :resource_boundary: :unknown :weight: 1 - :idempotent: + :idempotent: true + :tags: [] +- :name: container_repository:container_expiration_policies_cleanup_container_repository + :feature_category: :container_registry + :has_external_dependencies: + :urgency: :low + :resource_boundary: :unknown + :weight: 1 + :idempotent: true :tags: [] - :name: container_repository:delete_container_repository :feature_category: :container_registry diff --git a/app/workers/ci/build_trace_chunk_flush_worker.rb b/app/workers/ci/build_trace_chunk_flush_worker.rb index 89400247a7b..a63b12c0d03 100644 --- a/app/workers/ci/build_trace_chunk_flush_worker.rb +++ b/app/workers/ci/build_trace_chunk_flush_worker.rb @@ -5,6 +5,8 @@ module Ci include ApplicationWorker include PipelineBackgroundQueue + deduplicate :until_executed + idempotent! # rubocop: disable CodeReuse/ActiveRecord diff --git a/app/workers/cleanup_container_repository_worker.rb b/app/workers/cleanup_container_repository_worker.rb index 80cc296fff5..1cac2858156 100644 --- a/app/workers/cleanup_container_repository_worker.rb +++ b/app/workers/cleanup_container_repository_worker.rb @@ -1,10 +1,13 @@ # frozen_string_literal: true -class CleanupContainerRepositoryWorker # rubocop:disable Scalability/IdempotentWorker +class CleanupContainerRepositoryWorker include ApplicationWorker queue_namespace :container_repository feature_category :container_registry + urgency :low + worker_resource_boundary :unknown + idempotent! loggable_arguments 2 attr_reader :container_repository, :current_user diff --git a/app/workers/container_expiration_policies/cleanup_container_repository_worker.rb b/app/workers/container_expiration_policies/cleanup_container_repository_worker.rb new file mode 100644 index 00000000000..5619fdd7f38 --- /dev/null +++ b/app/workers/container_expiration_policies/cleanup_container_repository_worker.rb @@ -0,0 +1,95 @@ +# frozen_string_literal: true + +module ContainerExpirationPolicies + class CleanupContainerRepositoryWorker + include ApplicationWorker + include LimitedCapacity::Worker + include Gitlab::Utils::StrongMemoize + + queue_namespace :container_repository + feature_category :container_registry + urgency :low + worker_resource_boundary :unknown + idempotent! + + def perform_work + return unless throttling_enabled? + return unless container_repository + + unless allowed_to_run?(container_repository) + container_repository.cleanup_unscheduled! + log_info(container_repository_id: container_repository.id, cleanup_status: :skipped) + return + end + + result = ContainerExpirationPolicies::CleanupService.new(container_repository) + .execute + log_extra_metadata_on_done(:container_repository_id, result.payload[:container_repository_id]) + log_extra_metadata_on_done(:cleanup_status, result.payload[:cleanup_status]) + end + + def remaining_work_count + cleanup_scheduled_count = ContainerRepository.cleanup_scheduled.count + cleanup_unfinished_count = ContainerRepository.cleanup_unfinished.count + total_count = cleanup_scheduled_count + cleanup_unfinished_count + + log_info( + cleanup_scheduled_count: cleanup_scheduled_count, + cleanup_unfinished_count: cleanup_unfinished_count, + cleanup_total_count: total_count + ) + + total_count + end + + def max_running_jobs + return 0 unless throttling_enabled? + + ::Gitlab::CurrentSettings.current_application_settings.container_registry_expiration_policies_worker_capacity + end + + private + + def allowed_to_run?(container_repository) + return false unless policy&.enabled && policy&.next_run_at + + Time.zone.now + max_cleanup_execution_time.seconds < policy.next_run_at + end + + def throttling_enabled? + Feature.enabled?(:container_registry_expiration_policies_throttling) + end + + def max_cleanup_execution_time + ::Gitlab::CurrentSettings.current_application_settings.container_registry_delete_tags_service_timeout + end + + def policy + project.container_expiration_policy + end + + def project + container_repository&.project + end + + def container_repository + strong_memoize(:container_repository) do + ContainerRepository.transaction do + # rubocop: disable CodeReuse/ActiveRecord + # We need a lock to prevent two workers from picking up the same row + container_repository = ContainerRepository.waiting_for_cleanup + .order(:expiration_policy_cleanup_status, :expiration_policy_started_at) + .limit(1) + .lock('FOR UPDATE SKIP LOCKED') + .first + # rubocop: enable CodeReuse/ActiveRecord + container_repository&.tap(&:cleanup_ongoing!) + end + end + end + + def log_info(extra_structure) + logger.info(structured_payload(extra_structure)) + end + end +end diff --git a/app/workers/container_expiration_policy_worker.rb b/app/workers/container_expiration_policy_worker.rb index 61ba27f00d2..c22cea98e0d 100644 --- a/app/workers/container_expiration_policy_worker.rb +++ b/app/workers/container_expiration_policy_worker.rb @@ -3,20 +3,79 @@ class ContainerExpirationPolicyWorker # rubocop:disable Scalability/IdempotentWorker include ApplicationWorker include CronjobQueue + include ExclusiveLeaseGuard feature_category :container_registry + InvalidPolicyError = Class.new(StandardError) + + BATCH_SIZE = 1000.freeze + def perform - ContainerExpirationPolicy.executable.preloaded.each_batch do |relation| - relation.each do |container_expiration_policy| - with_context(project: container_expiration_policy.project, - user: container_expiration_policy.project.owner) do |project:, user:| - ContainerExpirationPolicyService.new(project, user) - .execute(container_expiration_policy) - rescue ContainerExpirationPolicyService::InvalidPolicyError => e - Gitlab::ErrorTracking.log_exception(e, container_expiration_policy_id: container_expiration_policy.id) + throttling_enabled? ? perform_throttled : perform_unthrottled + end + + private + + def perform_unthrottled + with_runnable_policy(preloaded: true) do |policy| + with_context(project: policy.project, + user: policy.project.owner) do |project:, user:| + ContainerExpirationPolicyService.new(project, user) + .execute(policy) + end + end + end + + def perform_throttled + try_obtain_lease do + with_runnable_policy do |policy| + policy.schedule_next_run! + ContainerRepository.for_project_id(policy.id) + .each_batch do |relation| + relation.update_all(expiration_policy_cleanup_status: :cleanup_scheduled) + end + end + + ContainerExpirationPolicies::CleanupContainerRepositoryWorker.perform_with_capacity + end + end + + # TODO : remove the preload option when cleaning FF container_registry_expiration_policies_throttling + def with_runnable_policy(preloaded: false) + ContainerExpirationPolicy.runnable_schedules.each_batch(of: BATCH_SIZE) do |policies| + # rubocop: disable CodeReuse/ActiveRecord + cte = Gitlab::SQL::CTE.new(:batched_policies, policies.limit(BATCH_SIZE)) + # rubocop: enable CodeReuse/ActiveRecord + scope = cte.apply_to(ContainerExpirationPolicy.all).with_container_repositories + + scope = scope.preloaded if preloaded + + scope.each do |policy| + if policy.valid? + ContainerExpirationPolicy.transaction do + yield policy + end + else + disable_invalid_policy!(policy) end end end end + + def disable_invalid_policy!(policy) + policy.disable! + Gitlab::ErrorTracking.log_exception( + ::ContainerExpirationPolicyWorker::InvalidPolicyError.new, + container_expiration_policy_id: policy.id + ) + end + + def throttling_enabled? + Feature.enabled?(:container_registry_expiration_policies_throttling) + end + + def lease_timeout + 5.hours + end end diff --git a/changelogs/unreleased/208193-throttle-container-expiration-policy-worker-execution.yml b/changelogs/unreleased/208193-throttle-container-expiration-policy-worker-execution.yml new file mode 100644 index 00000000000..395a1cdd671 --- /dev/null +++ b/changelogs/unreleased/208193-throttle-container-expiration-policy-worker-execution.yml @@ -0,0 +1,5 @@ +--- +title: Throttle container cleanup policies execution by using a limited capacity worker +merge_request: 40740 +author: +type: changed diff --git a/changelogs/unreleased/224509-replace-add-group-member-chevron.yml b/changelogs/unreleased/224509-replace-add-group-member-chevron.yml new file mode 100644 index 00000000000..5d95c78726c --- /dev/null +++ b/changelogs/unreleased/224509-replace-add-group-member-chevron.yml @@ -0,0 +1,5 @@ +--- +title: Replace down chevron on invite member/group +merge_request: 46076 +author: +type: other diff --git a/changelogs/unreleased/225956-replace-fa-check-icons-with-gitlab-svg-check-icon.yml b/changelogs/unreleased/225956-replace-fa-check-icons-with-gitlab-svg-check-icon.yml new file mode 100644 index 00000000000..da46c724a2c --- /dev/null +++ b/changelogs/unreleased/225956-replace-fa-check-icons-with-gitlab-svg-check-icon.yml @@ -0,0 +1,5 @@ +--- +title: Replace fa-check icons with GitLab SVG check icon +merge_request: 43353 +author: +type: changed diff --git a/changelogs/unreleased/241378_until_executed_deduplication_strategy.yml b/changelogs/unreleased/241378_until_executed_deduplication_strategy.yml new file mode 100644 index 00000000000..b9f202273e5 --- /dev/null +++ b/changelogs/unreleased/241378_until_executed_deduplication_strategy.yml @@ -0,0 +1,5 @@ +--- +title: Add until_executed deduplication strategy +merge_request: 42223 +author: +type: added diff --git a/changelogs/unreleased/mo-add-default-branch-to-daily-build.yml b/changelogs/unreleased/mo-add-default-branch-to-daily-build.yml new file mode 100644 index 00000000000..811f771b92b --- /dev/null +++ b/changelogs/unreleased/mo-add-default-branch-to-daily-build.yml @@ -0,0 +1,5 @@ +--- +title: Add default_branch to ci_daily_build_group_report_result +merge_request: 45702 +author: +type: performance diff --git a/config/initializers/labkit_middleware.rb b/config/initializers/labkit_middleware.rb index ea4103f052f..748666b6cd7 100644 --- a/config/initializers/labkit_middleware.rb +++ b/config/initializers/labkit_middleware.rb @@ -1,3 +1,36 @@ # frozen_string_literal: true -Rails.application.config.middleware.use(Labkit::Middleware::Rack) +# partial backport of https://github.com/rails/rails/pull/38169 +# this is in order to be able to re-order rack middlewares. + +if ActionDispatch::MiddlewareStack.method_defined?(:move) + warn "`move` is now defined in in ActionDispatch itself: https://github.com/rails/rails/pull/38169, please remove this patch from #{__FILE__}" +else + module ActionDispatch + class MiddlewareStack + def move(target, source) + source_index = assert_index(source, :before) + source_middleware = middlewares.delete_at(source_index) + + target_index = assert_index(target, :before) + middlewares.insert(target_index, source_middleware) + end + end + end +end + +unless Rails::Configuration::MiddlewareStackProxy.method_defined?(:move) + module Rails + module Configuration + class MiddlewareStackProxy + def move(*args, &block) + @operations << ->(middleware) { middleware.send(__method__, *args, &block) } + end + ruby2_keywords(:move) if respond_to?(:ruby2_keywords, true) + end + end + end +end + +Rails.application.config.middleware.move(1, ActionDispatch::RequestId) +Rails.application.config.middleware.insert_after(ActionDispatch::RequestId, Labkit::Middleware::Rack) diff --git a/danger/commit_messages/Dangerfile b/danger/commit_messages/Dangerfile index 174fc10eef3..1a4726d4017 100644 --- a/danger/commit_messages/Dangerfile +++ b/danger/commit_messages/Dangerfile @@ -127,7 +127,7 @@ def warn_or_fail_commits(failed_linters, default_to_fail: true) Array(failed_linters).each do |linter| linter.problems.each do |problem_key, problem_desc| case problem_key - when :subject_above_warning + when :subject_too_short, :subject_above_warning warn_commit(linter.commit, problem_desc) else self.__send__("#{level}_commit", linter.commit, problem_desc) # rubocop:disable GitlabSecurity/PublicSend diff --git a/db/migrate/20200920130356_add_container_expiration_policy_worker_settings_to_application_settings.rb b/db/migrate/20200920130356_add_container_expiration_policy_worker_settings_to_application_settings.rb new file mode 100644 index 00000000000..7501f150283 --- /dev/null +++ b/db/migrate/20200920130356_add_container_expiration_policy_worker_settings_to_application_settings.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +class AddContainerExpirationPolicyWorkerSettingsToApplicationSettings < ActiveRecord::Migration[6.0] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def up + unless column_exists?(:application_settings, :container_registry_expiration_policies_worker_capacity) + add_column(:application_settings, :container_registry_expiration_policies_worker_capacity, :integer, default: 0, null: false) + end + end + + def down + if column_exists?(:application_settings, :container_registry_expiration_policies_worker_capacity) + remove_column(:application_settings, :container_registry_expiration_policies_worker_capacity) + end + end +end diff --git a/db/migrate/20200928123510_add_expiration_policy_cleanup_status_to_container_repositories.rb b/db/migrate/20200928123510_add_expiration_policy_cleanup_status_to_container_repositories.rb new file mode 100644 index 00000000000..4d611a56e9c --- /dev/null +++ b/db/migrate/20200928123510_add_expiration_policy_cleanup_status_to_container_repositories.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +class AddExpirationPolicyCleanupStatusToContainerRepositories < ActiveRecord::Migration[6.0] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + INDEX_NAME = 'idx_container_repositories_on_exp_cleanup_status_and_start_date' + + disable_ddl_transaction! + + def up + unless column_exists?(:container_repositories, :expiration_policy_cleanup_status) + add_column(:container_repositories, :expiration_policy_cleanup_status, :integer, limit: 2, default: 0, null: false) + end + + add_concurrent_index(:container_repositories, [:expiration_policy_cleanup_status, :expiration_policy_started_at], name: INDEX_NAME) + end + + def down + remove_concurrent_index(:container_repositories, [:expiration_policy_cleanup_status, :expiration_policy_started_at], name: INDEX_NAME) + + if column_exists?(:container_repositories, :expiration_policy_cleanup_status) + remove_column(:container_repositories, :expiration_policy_cleanup_status) + end + end +end diff --git a/db/migrate/20201012122428_add_container_registry_expiration_policies_worker_capacity_constraint.rb b/db/migrate/20201012122428_add_container_registry_expiration_policies_worker_capacity_constraint.rb new file mode 100644 index 00000000000..e4f7d1309a3 --- /dev/null +++ b/db/migrate/20201012122428_add_container_registry_expiration_policies_worker_capacity_constraint.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +# See https://docs.gitlab.com/ee/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddContainerRegistryExpirationPoliciesWorkerCapacityConstraint < ActiveRecord::Migration[6.0] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + CONSTRAINT_NAME = 'app_settings_registry_exp_policies_worker_capacity_positive' + + disable_ddl_transaction! + + def up + add_check_constraint :application_settings, 'container_registry_expiration_policies_worker_capacity >= 0', CONSTRAINT_NAME + end + + def down + remove_check_constraint :application_settings, CONSTRAINT_NAME + end +end diff --git a/db/migrate/20201016074302_add_index_project_id_and_id_to_container_repositories.rb b/db/migrate/20201016074302_add_index_project_id_and_id_to_container_repositories.rb new file mode 100644 index 00000000000..b2e7c62ba0c --- /dev/null +++ b/db/migrate/20201016074302_add_index_project_id_and_id_to_container_repositories.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +class AddIndexProjectIdAndIdToContainerRepositories < ActiveRecord::Migration[6.0] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + INDEX_NAME = 'index_container_repositories_on_project_id_and_id' + + disable_ddl_transaction! + + def up + add_concurrent_index(:container_repositories, [:project_id, :id], name: INDEX_NAME) + end + + def down + remove_concurrent_index(:container_repositories, [:project_id, :id], name: INDEX_NAME) + end +end diff --git a/db/migrate/20201019152046_add_default_branch_to_daily_build_group_report_result.rb b/db/migrate/20201019152046_add_default_branch_to_daily_build_group_report_result.rb new file mode 100644 index 00000000000..8a0051b8106 --- /dev/null +++ b/db/migrate/20201019152046_add_default_branch_to_daily_build_group_report_result.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +class AddDefaultBranchToDailyBuildGroupReportResult < ActiveRecord::Migration[6.0] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def change + add_column :ci_daily_build_group_report_results, :default_branch, :boolean, default: false, null: false + end +end diff --git a/db/migrate/20201021142812_add_index_to_ci_daily_build_group_report_results.rb b/db/migrate/20201021142812_add_index_to_ci_daily_build_group_report_results.rb new file mode 100644 index 00000000000..c811e3deeda --- /dev/null +++ b/db/migrate/20201021142812_add_index_to_ci_daily_build_group_report_results.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +class AddIndexToCiDailyBuildGroupReportResults < ActiveRecord::Migration[6.0] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + INDEX_NAME = 'index_ci_daily_build_group_report_results_on_project_and_date' + + disable_ddl_transaction! + + def up + add_concurrent_index( + :ci_daily_build_group_report_results, + [:project_id, :date], + order: { date: :desc }, + where: "default_branch = TRUE AND (data -> 'coverage') IS NOT NULL", + name: INDEX_NAME + ) + end + + def down + remove_concurrent_index_by_name(:ci_daily_build_group_report_results, INDEX_NAME) + end +end diff --git a/db/schema_migrations/20200920130356 b/db/schema_migrations/20200920130356 new file mode 100644 index 00000000000..ab0575958ec --- /dev/null +++ b/db/schema_migrations/20200920130356 @@ -0,0 +1 @@ +80d2beb7a1c5f60a4bf3462054fa5bcd0488152b6754f8a7164046201fcb08ed
\ No newline at end of file diff --git a/db/schema_migrations/20200928123510 b/db/schema_migrations/20200928123510 new file mode 100644 index 00000000000..db9d2889739 --- /dev/null +++ b/db/schema_migrations/20200928123510 @@ -0,0 +1 @@ +1e274e744ed9e225e2ee09afc15871a1af63857f95c5d787e8efd9943fce1bed
\ No newline at end of file diff --git a/db/schema_migrations/20201012122428 b/db/schema_migrations/20201012122428 new file mode 100644 index 00000000000..13e7e9c2cd6 --- /dev/null +++ b/db/schema_migrations/20201012122428 @@ -0,0 +1 @@ +d43764a44f6578548d8b7838dc011b7693da0b7d65cbcc1fff96a212d655024e
\ No newline at end of file diff --git a/db/schema_migrations/20201016074302 b/db/schema_migrations/20201016074302 new file mode 100644 index 00000000000..e0ed03f0ac3 --- /dev/null +++ b/db/schema_migrations/20201016074302 @@ -0,0 +1 @@ +de07bcc8166421d01382038d930cabb6a4749b314f05ca148e8d13cff947447c
\ No newline at end of file diff --git a/db/schema_migrations/20201019152046 b/db/schema_migrations/20201019152046 new file mode 100644 index 00000000000..bc364a38a46 --- /dev/null +++ b/db/schema_migrations/20201019152046 @@ -0,0 +1 @@ +e5b3bcac7150df4443879db05b18b6aeb01271d99965b2468278954dedf8413b
\ No newline at end of file diff --git a/db/schema_migrations/20201021142812 b/db/schema_migrations/20201021142812 new file mode 100644 index 00000000000..d4ddf2a3595 --- /dev/null +++ b/db/schema_migrations/20201021142812 @@ -0,0 +1 @@ +81b9b79f2ca8830b9d9e9315d93421875dfe44cfa0da6f4e9166567452a2363b
\ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 077c351e555..37b235ac108 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -9296,6 +9296,8 @@ CREATE TABLE application_settings ( automatic_purchased_storage_allocation boolean DEFAULT false NOT NULL, encrypted_ci_jwt_signing_key text, encrypted_ci_jwt_signing_key_iv text, + container_registry_expiration_policies_worker_capacity integer DEFAULT 0 NOT NULL, + CONSTRAINT app_settings_registry_exp_policies_worker_capacity_positive CHECK ((container_registry_expiration_policies_worker_capacity >= 0)), CONSTRAINT check_2dba05b802 CHECK ((char_length(gitpod_url) <= 255)), CONSTRAINT check_51700b31b5 CHECK ((char_length(default_branch_name) <= 255)), CONSTRAINT check_57123c9593 CHECK ((char_length(help_page_documentation_base_url) <= 255)), @@ -10132,7 +10134,8 @@ CREATE TABLE ci_daily_build_group_report_results ( last_pipeline_id bigint NOT NULL, ref_path text NOT NULL, group_name text NOT NULL, - data jsonb NOT NULL + data jsonb NOT NULL, + default_branch boolean DEFAULT false NOT NULL ); CREATE SEQUENCE ci_daily_build_group_report_results_id_seq @@ -11248,7 +11251,8 @@ CREATE TABLE container_repositories ( created_at timestamp without time zone NOT NULL, updated_at timestamp without time zone NOT NULL, status smallint, - expiration_policy_started_at timestamp with time zone + expiration_policy_started_at timestamp with time zone, + expiration_policy_cleanup_status smallint DEFAULT 0 NOT NULL ); CREATE SEQUENCE container_repositories_id_seq @@ -19810,6 +19814,8 @@ CREATE INDEX idx_ci_pipelines_artifacts_locked ON ci_pipelines USING btree (ci_r CREATE INDEX idx_container_exp_policies_on_project_id_next_run_at_enabled ON container_expiration_policies USING btree (project_id, next_run_at, enabled); +CREATE INDEX idx_container_repositories_on_exp_cleanup_status_and_start_date ON container_repositories USING btree (expiration_policy_cleanup_status, expiration_policy_started_at); + CREATE INDEX idx_deployment_clusters_on_cluster_id_and_kubernetes_namespace ON deployment_clusters USING btree (cluster_id, kubernetes_namespace); CREATE UNIQUE INDEX idx_environment_merge_requests_unique_index ON deployment_merge_requests USING btree (environment_id, merge_request_id); @@ -20164,6 +20170,8 @@ CREATE UNIQUE INDEX index_ci_builds_runner_session_on_build_id ON ci_builds_runn CREATE INDEX index_ci_daily_build_group_report_results_on_last_pipeline_id ON ci_daily_build_group_report_results USING btree (last_pipeline_id); +CREATE INDEX index_ci_daily_build_group_report_results_on_project_and_date ON ci_daily_build_group_report_results USING btree (project_id, date DESC) WHERE ((default_branch = true) AND ((data -> 'coverage'::text) IS NOT NULL)); + CREATE INDEX index_ci_deleted_objects_on_pick_up_at ON ci_deleted_objects USING btree (pick_up_at); CREATE INDEX index_ci_freeze_periods_on_project_id ON ci_freeze_periods USING btree (project_id); @@ -20398,6 +20406,8 @@ CREATE INDEX index_container_expiration_policies_on_next_run_at_and_enabled ON c CREATE INDEX index_container_repositories_on_project_id ON container_repositories USING btree (project_id); +CREATE INDEX index_container_repositories_on_project_id_and_id ON container_repositories USING btree (project_id, id); + CREATE UNIQUE INDEX index_container_repositories_on_project_id_and_name ON container_repositories USING btree (project_id, name); CREATE INDEX index_container_repository_on_name_trigram ON container_repositories USING gin (name gin_trgm_ops); diff --git a/doc/development/sidekiq_style_guide.md b/doc/development/sidekiq_style_guide.md index 24570cfc07b..b341c31e21d 100644 --- a/doc/development/sidekiq_style_guide.md +++ b/doc/development/sidekiq_style_guide.md @@ -165,6 +165,22 @@ job. The work is skipped because the same work would be done by the job that was scheduled first; by the time the second job executed, the first job would do nothing. +#### Strategies + +GitLab supports two deduplication strategies: + +- `until_executing` +- `until_executed` + +More [deduplication strategies have been +suggested](https://gitlab.com/gitlab-com/gl-infra/scalability/-/issues/195). If +you are implementing a worker that could benefit from a different +strategy, please comment in the issue. + +##### Until Executing + +This strategy takes a lock when a job is added to the queue, and removes that lock before the job starts. + For example, `AuthorizedProjectsWorker` takes a user ID. When the worker runs, it recalculates a user's authorizations. GitLab schedules this job each time an action potentially changes a user's @@ -173,10 +189,47 @@ same time, the second job can be skipped if the first job hasn't begun, because when the first job runs, it creates the authorizations for both projects. +```ruby +module AuthorizedProjectUpdate + class UserRefreshOverUserRangeWorker + include ApplicationWorker + + deduplicate :until_executing + idempotent! + + # ... + end +end +``` + +##### Until Executed + +This strategy takes a lock when a job is added to the queue, and removes that lock after the job finishes. +It can be used to prevent jobs from running simultaneously multiple times. + +```ruby +module Ci + class BuildTraceChunkFlushWorker + include ApplicationWorker + + deduplicate :until_executed + idempotent! + + # ... + end +end +``` + +#### Scheduling jobs in the future + GitLab doesn't skip jobs scheduled in the future, as we assume that the state will have changed by the time the job is scheduled to -execute. If you do want to deduplicate jobs scheduled in the future -this can be specified on the worker as follows: +execute. Deduplication of jobs scheduled in the feature is possible +for both `until_executed` and `until_executing` strategies. + +If you do want to deduplicate jobs scheduled in the future, +this can be specified on the worker by passing `including_scheduled: true` argument +when defining deduplication strategy: ```ruby module AuthorizedProjectUpdate @@ -191,11 +244,7 @@ module AuthorizedProjectUpdate end ``` -This strategy is called `until_executing`. More [deduplication -strategies have been -suggested](https://gitlab.com/gitlab-com/gl-infra/scalability/-/issues/195). If -you are implementing a worker that could benefit from a different -strategy, please comment in the issue. +#### Troubleshooting If the automatic deduplication were to cause issues in certain queues. This can be temporarily disabled by enabling a feature flag diff --git a/lib/api/api.rb b/lib/api/api.rb index 5a58f1adbc1..e6935d4e74a 100644 --- a/lib/api/api.rb +++ b/lib/api/api.rb @@ -48,11 +48,17 @@ module API before do coerce_nil_params_to_array! + api_endpoint = env['api.endpoint'] + feature_category = api_endpoint.options[:for].try(:feature_category_for_app, api_endpoint).to_s + + header[Gitlab::Metrics::RequestsRackMiddleware::FEATURE_CATEGORY_HEADER] = feature_category + Gitlab::ApplicationContext.push( user: -> { @current_user }, project: -> { @project }, namespace: -> { @group }, - caller_id: route.origin + caller_id: route.origin, + feature_category: feature_category ) end diff --git a/lib/api/base.rb b/lib/api/base.rb index e174cef3bad..33e47c18fcd 100644 --- a/lib/api/base.rb +++ b/lib/api/base.rb @@ -2,5 +2,30 @@ module API class Base < Grape::API::Instance # rubocop:disable API/Base + include ::Gitlab::WithFeatureCategory + + class << self + def feature_category_for_app(app) + feature_category_for_action(path_for_app(app)) + end + + def path_for_app(app) + normalize_path(app.namespace, app.options[:path].first) + end + + def route(methods, paths = ['/'], route_options = {}, &block) + if category = route_options.delete(:feature_category) + feature_category(category, Array(paths).map { |path| normalize_path(namespace, path) }) + end + + super + end + + private + + def normalize_path(namespace, path) + [namespace.presence, path.to_s.chomp('/').presence].compact.join('/') + end + end end end diff --git a/lib/api/internal/base.rb b/lib/api/internal/base.rb index 6d8f13c36e6..6934ed5b7b3 100644 --- a/lib/api/internal/base.rb +++ b/lib/api/internal/base.rb @@ -7,10 +7,16 @@ module API before { authenticate_by_gitlab_shell_token! } before do + api_endpoint = env['api.endpoint'] + feature_category = api_endpoint.options[:for].try(:feature_category_for_app, api_endpoint).to_s + + header[Gitlab::Metrics::RequestsRackMiddleware::FEATURE_CATEGORY_HEADER] = feature_category + Gitlab::ApplicationContext.push( user: -> { actor&.user }, project: -> { project }, - caller_id: route.origin + caller_id: route.origin, + feature_category: feature_category ) end diff --git a/lib/api/issues.rb b/lib/api/issues.rb index 143f9e40736..25fce1bf473 100644 --- a/lib/api/issues.rb +++ b/lib/api/issues.rb @@ -6,6 +6,8 @@ module API helpers Helpers::IssuesHelpers helpers Helpers::RateLimiter + feature_category :issue_tracking + before { authenticate_non_get! } helpers do diff --git a/lib/api/users.rb b/lib/api/users.rb index e7c1d644324..2f54b8c0fae 100644 --- a/lib/api/users.rb +++ b/lib/api/users.rb @@ -8,6 +8,8 @@ module API allow_access_with_scope :read_user, if: -> (request) { request.get? } + feature_category :users, ['/users/:id/custom_attributes', '/users/:id/custom_attributes/:key'] + resource :users, requirements: { uid: /[0-9]*/, id: /[0-9]*/ } do include CustomAttributesEndpoints @@ -93,7 +95,7 @@ module API use :optional_index_params_ee end # rubocop: disable CodeReuse/ActiveRecord - get do + get feature_category: :users do authenticated_as_admin! if params[:external].present? || (params[:extern_uid].present? && params[:provider].present?) unless current_user&.admin? @@ -134,7 +136,7 @@ module API use :with_custom_attributes end # rubocop: disable CodeReuse/ActiveRecord - get ":id" do + get ":id", feature_category: :users do user = User.find_by(id: params[:id]) not_found!('User') unless user && can?(current_user, :read_user, user) @@ -149,7 +151,7 @@ module API params do requires :user_id, type: String, desc: 'The ID or username of the user' end - get ":user_id/status", requirements: API::USER_REQUIREMENTS do + get ":user_id/status", requirements: API::USER_REQUIREMENTS, feature_category: :users do user = find_user(params[:user_id]) not_found!('User') unless user && can?(current_user, :read_user, user) @@ -170,7 +172,7 @@ module API optional :force_random_password, type: Boolean, desc: 'Flag indicating a random password will be set' use :optional_attributes end - post do + post feature_category: :users do authenticated_as_admin! params = declared_params(include_missing: false) @@ -204,7 +206,7 @@ module API use :optional_attributes end # rubocop: disable CodeReuse/ActiveRecord - put ":id" do + put ":id", feature_category: :users do authenticated_as_admin! user = User.find_by(id: params.delete(:id)) @@ -245,7 +247,7 @@ module API requires :provider, type: String, desc: 'The external provider' end # rubocop: disable CodeReuse/ActiveRecord - delete ":id/identities/:provider" do + delete ":id/identities/:provider", feature_category: :authentication_and_authorization do authenticated_as_admin! user = User.find_by(id: params[:id]) @@ -268,7 +270,7 @@ module API optional :expires_at, type: DateTime, desc: 'The expiration date of the SSH key in ISO 8601 format (YYYY-MM-DDTHH:MM:SSZ)' end # rubocop: disable CodeReuse/ActiveRecord - post ":id/keys" do + post ":id/keys", feature_category: :authentication_and_authorization do authenticated_as_admin! user = User.find_by(id: params.delete(:id)) @@ -291,7 +293,7 @@ module API requires :user_id, type: String, desc: 'The ID or username of the user' use :pagination end - get ':user_id/keys', requirements: API::USER_REQUIREMENTS do + get ':user_id/keys', requirements: API::USER_REQUIREMENTS, feature_category: :authentication_and_authorization do user = find_user(params[:user_id]) not_found!('User') unless user && can?(current_user, :read_user, user) @@ -307,7 +309,7 @@ module API requires :key_id, type: Integer, desc: 'The ID of the SSH key' end # rubocop: disable CodeReuse/ActiveRecord - delete ':id/keys/:key_id' do + delete ':id/keys/:key_id', feature_category: :authentication_and_authorization do authenticated_as_admin! user = User.find_by(id: params[:id]) @@ -332,7 +334,7 @@ module API requires :key, type: String, desc: 'The new GPG key' end # rubocop: disable CodeReuse/ActiveRecord - post ':id/gpg_keys' do + post ':id/gpg_keys', feature_category: :authentication_and_authorization do authenticated_as_admin! user = User.find_by(id: params.delete(:id)) @@ -357,7 +359,7 @@ module API use :pagination end # rubocop: disable CodeReuse/ActiveRecord - get ':id/gpg_keys' do + get ':id/gpg_keys', feature_category: :authentication_and_authorization do user = User.find_by(id: params[:id]) not_found!('User') unless user @@ -374,7 +376,7 @@ module API requires :key_id, type: Integer, desc: 'The ID of the GPG key' end # rubocop: disable CodeReuse/ActiveRecord - get ':id/gpg_keys/:key_id' do + get ':id/gpg_keys/:key_id', feature_category: :authentication_and_authorization do user = User.find_by(id: params[:id]) not_found!('User') unless user @@ -393,7 +395,7 @@ module API requires :key_id, type: Integer, desc: 'The ID of the GPG key' end # rubocop: disable CodeReuse/ActiveRecord - delete ':id/gpg_keys/:key_id' do + delete ':id/gpg_keys/:key_id', feature_category: :authentication_and_authorization do authenticated_as_admin! user = User.find_by(id: params[:id]) @@ -417,7 +419,7 @@ module API requires :key_id, type: Integer, desc: 'The ID of the GPG key' end # rubocop: disable CodeReuse/ActiveRecord - post ':id/gpg_keys/:key_id/revoke' do + post ':id/gpg_keys/:key_id/revoke', feature_category: :authentication_and_authorization do authenticated_as_admin! user = User.find_by(id: params[:id]) @@ -440,7 +442,7 @@ module API optional :skip_confirmation, type: Boolean, desc: 'Skip confirmation of email and assume it is verified' end # rubocop: disable CodeReuse/ActiveRecord - post ":id/emails" do + post ":id/emails", feature_category: :users do authenticated_as_admin! user = User.find_by(id: params.delete(:id)) @@ -464,7 +466,7 @@ module API use :pagination end # rubocop: disable CodeReuse/ActiveRecord - get ':id/emails' do + get ':id/emails', feature_category: :users do authenticated_as_admin! user = User.find_by(id: params[:id]) not_found!('User') unless user @@ -481,7 +483,7 @@ module API requires :email_id, type: Integer, desc: 'The ID of the email' end # rubocop: disable CodeReuse/ActiveRecord - delete ':id/emails/:email_id' do + delete ':id/emails/:email_id', feature_category: :users do authenticated_as_admin! user = User.find_by(id: params[:id]) not_found!('User') unless user @@ -503,7 +505,7 @@ module API optional :hard_delete, type: Boolean, desc: "Whether to remove a user's contributions" end # rubocop: disable CodeReuse/ActiveRecord - delete ":id" do + delete ":id", feature_category: :users do Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab/issues/20757') authenticated_as_admin! @@ -523,7 +525,7 @@ module API requires :id, type: Integer, desc: 'The ID of the user' end # rubocop: disable CodeReuse/ActiveRecord - post ':id/activate' do + post ':id/activate', feature_category: :authentication_and_authorization do authenticated_as_admin! user = User.find_by(id: params[:id]) @@ -538,7 +540,7 @@ module API requires :id, type: Integer, desc: 'The ID of the user' end # rubocop: disable CodeReuse/ActiveRecord - post ':id/deactivate' do + post ':id/deactivate', feature_category: :authentication_and_authorization do authenticated_as_admin! user = User.find_by(id: params[:id]) not_found!('User') unless user @@ -564,7 +566,7 @@ module API requires :id, type: Integer, desc: 'The ID of the user' end # rubocop: disable CodeReuse/ActiveRecord - post ':id/block' do + post ':id/block', feature_category: :authentication_and_authorization do authenticated_as_admin! user = User.find_by(id: params[:id]) not_found!('User') unless user @@ -589,7 +591,7 @@ module API requires :id, type: Integer, desc: 'The ID of the user' end # rubocop: disable CodeReuse/ActiveRecord - post ':id/unblock' do + post ':id/unblock', feature_category: :authentication_and_authorization do authenticated_as_admin! user = User.find_by(id: params[:id]) not_found!('User') unless user @@ -612,7 +614,7 @@ module API optional :type, type: String, values: %w[Project Namespace] use :pagination end - get ":user_id/memberships" do + get ":user_id/memberships", feature_category: :users do authenticated_as_admin! user = find_user_by_id(params) @@ -656,7 +658,9 @@ module API use :pagination optional :state, type: String, default: 'all', values: %w[all active inactive], desc: 'Filters (all|active|inactive) impersonation_tokens' end - get { present paginate(finder(declared_params(include_missing: false)).execute), with: Entities::ImpersonationToken } + get feature_category :authentication_and_authorization do + present paginate(finder(declared_params(include_missing: false)).execute), with: Entities::ImpersonationToken + end desc 'Create a impersonation token. Available only for admins.' do detail 'This feature was introduced in GitLab 9.0' @@ -667,7 +671,7 @@ module API optional :expires_at, type: Date, desc: 'The expiration date in the format YEAR-MONTH-DAY of the impersonation token' optional :scopes, type: Array, desc: 'The array of scopes of the impersonation token' end - post do + post feature_category: :authentication_and_authorization do impersonation_token = finder.build(declared_params(include_missing: false)) if impersonation_token.save @@ -684,7 +688,7 @@ module API params do requires :impersonation_token_id, type: Integer, desc: 'The ID of the impersonation token' end - get ':impersonation_token_id' do + get ':impersonation_token_id', feature_category: :authentication_and_authorization do present find_impersonation_token, with: Entities::ImpersonationToken end @@ -694,7 +698,7 @@ module API params do requires :impersonation_token_id, type: Integer, desc: 'The ID of the impersonation token' end - delete ':impersonation_token_id' do + delete ':impersonation_token_id', feature_category: :authentication_and_authorization do token = find_impersonation_token destroy_conditionally!(token) do @@ -716,7 +720,7 @@ module API desc 'Get the currently authenticated user' do success Entities::UserPublic end - get do + get feature_category: :users do entity = if current_user.admin? Entities::UserWithAdmin @@ -734,7 +738,7 @@ module API params do use :pagination end - get "keys" do + get "keys", feature_category: :authentication_and_authorization do keys = current_user.keys.preload_users present paginate(keys), with: Entities::SSHKey @@ -747,7 +751,7 @@ module API requires :key_id, type: Integer, desc: 'The ID of the SSH key' end # rubocop: disable CodeReuse/ActiveRecord - get "keys/:key_id" do + get "keys/:key_id", feature_category: :authentication_and_authorization do key = current_user.keys.find_by(id: params[:key_id]) not_found!('Key') unless key @@ -763,7 +767,7 @@ module API requires :title, type: String, desc: 'The title of the new SSH key' optional :expires_at, type: DateTime, desc: 'The expiration date of the SSH key in ISO 8601 format (YYYY-MM-DDTHH:MM:SSZ)' end - post "keys" do + post "keys", feature_category: :authentication_and_authorization do key = ::Keys::CreateService.new(current_user, declared_params(include_missing: false)).execute if key.persisted? @@ -780,7 +784,7 @@ module API requires :key_id, type: Integer, desc: 'The ID of the SSH key' end # rubocop: disable CodeReuse/ActiveRecord - delete "keys/:key_id" do + delete "keys/:key_id", feature_category: :authentication_and_authorization do key = current_user.keys.find_by(id: params[:key_id]) not_found!('Key') unless key @@ -798,7 +802,7 @@ module API params do use :pagination end - get 'gpg_keys' do + get 'gpg_keys', feature_category: :authentication_and_authorization do present paginate(current_user.gpg_keys), with: Entities::GpgKey end @@ -810,7 +814,7 @@ module API requires :key_id, type: Integer, desc: 'The ID of the GPG key' end # rubocop: disable CodeReuse/ActiveRecord - get 'gpg_keys/:key_id' do + get 'gpg_keys/:key_id', feature_category: :authentication_and_authorization do key = current_user.gpg_keys.find_by(id: params[:key_id]) not_found!('GPG Key') unless key @@ -825,7 +829,7 @@ module API params do requires :key, type: String, desc: 'The new GPG key' end - post 'gpg_keys' do + post 'gpg_keys', feature_category: :authentication_and_authorization do key = ::GpgKeys::CreateService.new(current_user, declared_params(include_missing: false)).execute if key.persisted? @@ -842,7 +846,7 @@ module API requires :key_id, type: Integer, desc: 'The ID of the GPG key' end # rubocop: disable CodeReuse/ActiveRecord - post 'gpg_keys/:key_id/revoke' do + post 'gpg_keys/:key_id/revoke', feature_category: :authentication_and_authorization do key = current_user.gpg_keys.find_by(id: params[:key_id]) not_found!('GPG Key') unless key @@ -858,7 +862,7 @@ module API requires :key_id, type: Integer, desc: 'The ID of the SSH key' end # rubocop: disable CodeReuse/ActiveRecord - delete 'gpg_keys/:key_id' do + delete 'gpg_keys/:key_id', feature_category: :authentication_and_authorization do key = current_user.gpg_keys.find_by(id: params[:key_id]) not_found!('GPG Key') unless key @@ -875,7 +879,7 @@ module API params do use :pagination end - get "emails" do + get "emails", feature_category: :users do present paginate(current_user.emails), with: Entities::Email end @@ -886,7 +890,7 @@ module API requires :email_id, type: Integer, desc: 'The ID of the email' end # rubocop: disable CodeReuse/ActiveRecord - get "emails/:email_id" do + get "emails/:email_id", feature_category: :users do email = current_user.emails.find_by(id: params[:email_id]) not_found!('Email') unless email @@ -900,7 +904,7 @@ module API params do requires :email, type: String, desc: 'The new email' end - post "emails" do + post "emails", feature_category: :users do email = Emails::CreateService.new(current_user, declared_params.merge(user: current_user)).execute if email.errors.blank? @@ -915,7 +919,7 @@ module API requires :email_id, type: Integer, desc: 'The ID of the email' end # rubocop: disable CodeReuse/ActiveRecord - delete "emails/:email_id" do + delete "emails/:email_id", feature_category: :users do email = current_user.emails.find_by(id: params[:email_id]) not_found!('Email') unless email @@ -931,7 +935,7 @@ module API use :pagination end # rubocop: disable CodeReuse/ActiveRecord - get "activities" do + get "activities", feature_category: :users do authenticated_as_admin! activities = User @@ -949,7 +953,7 @@ module API optional :emoji, type: String, desc: "The emoji to set on the status" optional :message, type: String, desc: "The status message to set" end - put "status" do + put "status", feature_category: :users do forbidden! unless can?(current_user, :update_user_status, current_user) if ::Users::SetStatusService.new(current_user, declared_params).execute @@ -962,7 +966,7 @@ module API desc 'get the status of the current user' do success Entities::UserStatus end - get 'status' do + get 'status', feature_category: :users do present current_user.status || {}, with: Entities::UserStatus end end diff --git a/lib/gitlab/metrics/requests_rack_middleware.rb b/lib/gitlab/metrics/requests_rack_middleware.rb index f6bda0dbea4..bb7ecb6128c 100644 --- a/lib/gitlab/metrics/requests_rack_middleware.rb +++ b/lib/gitlab/metrics/requests_rack_middleware.rb @@ -63,7 +63,7 @@ module Gitlab if health_endpoint RequestsRackMiddleware.http_health_requests_total.increment(status: status, method: method) else - RequestsRackMiddleware.http_request_total.increment(status: status, method: method, feature_category: feature_category || FEATURE_CATEGORY_DEFAULT) + RequestsRackMiddleware.http_request_total.increment(status: status, method: method, feature_category: feature_category.presence || FEATURE_CATEGORY_DEFAULT) end end end diff --git a/lib/gitlab/sidekiq_middleware/duplicate_jobs/strategies.rb b/lib/gitlab/sidekiq_middleware/duplicate_jobs/strategies.rb index 6fdef4c354e..63e8bee4443 100644 --- a/lib/gitlab/sidekiq_middleware/duplicate_jobs/strategies.rb +++ b/lib/gitlab/sidekiq_middleware/duplicate_jobs/strategies.rb @@ -8,6 +8,7 @@ module Gitlab STRATEGIES = { until_executing: UntilExecuting, + until_executed: UntilExecuted, none: None }.freeze diff --git a/lib/gitlab/sidekiq_middleware/duplicate_jobs/strategies/base.rb b/lib/gitlab/sidekiq_middleware/duplicate_jobs/strategies/base.rb new file mode 100644 index 00000000000..df5df590281 --- /dev/null +++ b/lib/gitlab/sidekiq_middleware/duplicate_jobs/strategies/base.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +module Gitlab + module SidekiqMiddleware + module DuplicateJobs + module Strategies + class Base + def initialize(duplicate_job) + @duplicate_job = duplicate_job + end + + def schedule(job) + raise NotImplementedError + end + + def perform(_job) + raise NotImplementedError + end + + private + + attr_reader :duplicate_job + + def strategy_name + self.class.name.to_s.demodulize.underscore.humanize.downcase + end + + def check! + # The default expiry time is the DuplicateJob::DUPLICATE_KEY_TTL already + # Only the strategies de-duplicating when scheduling + duplicate_job.check! + end + end + end + end + end +end diff --git a/lib/gitlab/sidekiq_middleware/duplicate_jobs/strategies/deduplicates_when_scheduling.rb b/lib/gitlab/sidekiq_middleware/duplicate_jobs/strategies/deduplicates_when_scheduling.rb new file mode 100644 index 00000000000..59b0e7e29da --- /dev/null +++ b/lib/gitlab/sidekiq_middleware/duplicate_jobs/strategies/deduplicates_when_scheduling.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true + +module Gitlab + module SidekiqMiddleware + module DuplicateJobs + module Strategies + module DeduplicatesWhenScheduling + def initialize(duplicate_job) + @duplicate_job = duplicate_job + end + + def schedule(job) + if deduplicatable_job? && check! && duplicate_job.duplicate? + job['duplicate-of'] = duplicate_job.existing_jid + + if duplicate_job.droppable? + Gitlab::SidekiqLogging::DeduplicationLogger.instance.log( + job, "dropped #{strategy_name}", duplicate_job.options) + return false + end + end + + yield + end + + private + + def deduplicatable_job? + !duplicate_job.scheduled? || duplicate_job.options[:including_scheduled] + end + + def check! + duplicate_job.check!(expiry) + end + + def expiry + return DuplicateJob::DUPLICATE_KEY_TTL unless duplicate_job.scheduled? + + time_diff = duplicate_job.scheduled_at.to_i - Time.now.to_i + + time_diff > 0 ? time_diff : DuplicateJob::DUPLICATE_KEY_TTL + end + end + end + end + end +end diff --git a/lib/gitlab/sidekiq_middleware/duplicate_jobs/strategies/none.rb b/lib/gitlab/sidekiq_middleware/duplicate_jobs/strategies/none.rb index cd101cd16b6..acbe0efaafa 100644 --- a/lib/gitlab/sidekiq_middleware/duplicate_jobs/strategies/none.rb +++ b/lib/gitlab/sidekiq_middleware/duplicate_jobs/strategies/none.rb @@ -5,10 +5,7 @@ module Gitlab module DuplicateJobs module Strategies # This strategy will never deduplicate a job - class None - def initialize(_duplicate_job) - end - + class None < Base def schedule(_job) yield end diff --git a/lib/gitlab/sidekiq_middleware/duplicate_jobs/strategies/until_executed.rb b/lib/gitlab/sidekiq_middleware/duplicate_jobs/strategies/until_executed.rb new file mode 100644 index 00000000000..738efa36fc8 --- /dev/null +++ b/lib/gitlab/sidekiq_middleware/duplicate_jobs/strategies/until_executed.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +module Gitlab + module SidekiqMiddleware + module DuplicateJobs + module Strategies + # This strategy takes a lock before scheduling the job in a queue and + # removes the lock after the job has executed preventing a new job to be queued + # while a job is still executing. + class UntilExecuted < Base + include DeduplicatesWhenScheduling + + def perform(_job) + yield + + duplicate_job.delete! + end + end + end + end + end +end diff --git a/lib/gitlab/sidekiq_middleware/duplicate_jobs/strategies/until_executing.rb b/lib/gitlab/sidekiq_middleware/duplicate_jobs/strategies/until_executing.rb index 46ce0eb4a91..68d66383b2b 100644 --- a/lib/gitlab/sidekiq_middleware/duplicate_jobs/strategies/until_executing.rb +++ b/lib/gitlab/sidekiq_middleware/duplicate_jobs/strategies/until_executing.rb @@ -7,50 +7,14 @@ module Gitlab # This strategy takes a lock before scheduling the job in a queue and # removes the lock before the job starts allowing a new job to be queued # while a job is still executing. - class UntilExecuting - def initialize(duplicate_job) - @duplicate_job = duplicate_job - end - - def schedule(job) - if deduplicatable_job? && check! && duplicate_job.duplicate? - job['duplicate-of'] = duplicate_job.existing_jid - - if duplicate_job.droppable? - Gitlab::SidekiqLogging::DeduplicationLogger.instance.log( - job, "dropped until executing", duplicate_job.options) - return false - end - end - - yield - end + class UntilExecuting < Base + include DeduplicatesWhenScheduling def perform(_job) duplicate_job.delete! yield end - - private - - attr_reader :duplicate_job - - def deduplicatable_job? - !duplicate_job.scheduled? || duplicate_job.options[:including_scheduled] - end - - def check! - duplicate_job.check!(expiry) - end - - def expiry - return DuplicateJob::DUPLICATE_KEY_TTL unless duplicate_job.scheduled? - - time_diff = duplicate_job.scheduled_at.to_i - Time.now.to_i - - time_diff > 0 ? time_diff : DuplicateJob::DUPLICATE_KEY_TTL - end end end end diff --git a/lib/gitlab/with_feature_category.rb b/lib/gitlab/with_feature_category.rb new file mode 100644 index 00000000000..65d21daf78a --- /dev/null +++ b/lib/gitlab/with_feature_category.rb @@ -0,0 +1,50 @@ +# frozen_string_literal: true + +module Gitlab + module WithFeatureCategory + extend ActiveSupport::Concern + include Gitlab::ClassAttributes + + class_methods do + def feature_category(category, actions = []) + feature_category_configuration[category] ||= [] + feature_category_configuration[category] += actions.map(&:to_s) + + validate_config!(feature_category_configuration) + end + + def feature_category_for_action(action) + category_config = feature_category_configuration.find do |_, actions| + actions.empty? || actions.include?(action) + end + + category_config&.first || superclass_feature_category_for_action(action) + end + + private + + def validate_config!(config) + empty = config.find { |_, actions| actions.empty? } + duplicate_actions = config.values.map(&:uniq).flatten.group_by(&:itself).select { |_, v| v.count > 1 }.keys + + if config.length > 1 && empty + raise ArgumentError, "#{empty.first} is defined for all actions, but other categories are set" + end + + if duplicate_actions.any? + raise ArgumentError, "Actions have multiple feature categories: #{duplicate_actions.join(', ')}" + end + end + + def feature_category_configuration + class_attributes[:feature_category_config] ||= {} + end + + def superclass_feature_category_for_action(action) + return unless superclass.respond_to?(:feature_category_for_action) + + superclass.feature_category_for_action(action) + end + end + end +end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 9234989c62d..e5b83248a91 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -17394,8 +17394,10 @@ msgstr "" msgid "Namespace:" msgstr "" -msgid "NamespaceStorageSize|%{namespace_name} contains a locked project" -msgstr "" +msgid "NamespaceStorageSize|%{namespace_name} contains %{locked_project_count} locked project" +msgid_plural "NamespaceStorageSize|%{namespace_name} contains %{locked_project_count} locked projects" +msgstr[0] "" +msgstr[1] "" msgid "NamespaceStorageSize|%{namespace_name} is now read-only. You cannot: %{base_message}" msgstr "" @@ -17403,17 +17405,19 @@ msgstr "" msgid "NamespaceStorageSize|If you reach 100%% storage capacity, you will not be able to: %{base_message}" msgstr "" -msgid "NamespaceStorageSize|Please purchase additional storage to unlock your projects over the free 10GB project limit. You can't %{base_message}" +msgid "NamespaceStorageSize|Please purchase additional storage to unlock your projects over the free %{free_size_limit} project limit. You can't %{base_message}" msgstr "" -msgid "NamespaceStorageSize|You have consumed all of your additional storage, please purchase more to unlock your projects over the free 10GB limit. You can't %{base_message}" +msgid "NamespaceStorageSize|You have consumed all of your additional storage, please purchase more to unlock your projects over the free %{free_size_limit} limit. You can't %{base_message}" msgstr "" msgid "NamespaceStorageSize|You have reached %{usage_in_percent} of %{namespace_name}'s storage capacity (%{used_storage} of %{storage_limit})" msgstr "" -msgid "NamespaceStorageSize|You have reached the free storage limit of 10GB on %{locked_project_count} projects. To unlock them, please purchase additional storage" -msgstr "" +msgid "NamespaceStorageSize|You have reached the free storage limit of %{free_size_limit} on %{locked_project_count} project. To unlock it, please purchase additional storage" +msgid_plural "NamespaceStorageSize|You have reached the free storage limit of %{free_size_limit} on %{locked_project_count} projects. To unlock them, please purchase additional storage" +msgstr[0] "" +msgstr[1] "" msgid "NamespaceStorageSize|push to your repository, create pipelines, create issues or add comments. To reduce storage capacity, delete unused repositories, artifacts, wikis, issues, and pipelines." msgstr "" diff --git a/scripts/utils.sh b/scripts/utils.sh index 9d188fc7b77..3829bcdf24e 100644 --- a/scripts/utils.sh +++ b/scripts/utils.sh @@ -31,8 +31,8 @@ function install_api_client_dependencies_with_apt() { } function install_gitlab_gem() { - gem install httparty --no-document --version 0.17.3 - gem install gitlab --no-document --version 4.13.0 + gem install httparty --no-document --version 0.18.1 + gem install gitlab --no-document --version 4.14.1 } function install_tff_gem() { diff --git a/spec/controllers/every_controller_spec.rb b/spec/controllers/every_controller_spec.rb index b1519c4ef1e..a1c377eff76 100644 --- a/spec/controllers/every_controller_spec.rb +++ b/spec/controllers/every_controller_spec.rb @@ -17,7 +17,7 @@ RSpec.describe "Every controller" do .compact .select { |route| route[:controller].present? && route[:action].present? } .map { |route| [constantize_controller(route[:controller]), route[:action]] } - .select { |(controller, action)| controller&.include?(ControllerWithFeatureCategory) } + .select { |(controller, action)| controller&.include?(::Gitlab::WithFeatureCategory) } .reject { |(controller, action)| controller == ApplicationController || controller == Devise::UnlocksController } end diff --git a/spec/factories/ci/daily_build_group_report_results.rb b/spec/factories/ci/daily_build_group_report_results.rb index fbb235f80ed..d836ee9567c 100644 --- a/spec/factories/ci/daily_build_group_report_results.rb +++ b/spec/factories/ci/daily_build_group_report_results.rb @@ -10,5 +10,11 @@ FactoryBot.define do data do { 'coverage' => 77.0 } end + default_branch { true } + + trait :on_feature_branch do + ref_path { Gitlab::Git::BRANCH_REF_PREFIX + 'feature' } + default_branch { false } + end end end diff --git a/spec/factories/container_repositories.rb b/spec/factories/container_repositories.rb index 34fb28d8fdd..86bb129067f 100644 --- a/spec/factories/container_repositories.rb +++ b/spec/factories/container_repositories.rb @@ -21,6 +21,18 @@ FactoryBot.define do status { :delete_failed } end + trait :cleanup_scheduled do + expiration_policy_cleanup_status { :cleanup_scheduled } + end + + trait :cleanup_unfinished do + expiration_policy_cleanup_status { :cleanup_unfinished } + end + + trait :cleanup_ongoing do + expiration_policy_cleanup_status { :cleanup_ongoing } + end + after(:build) do |repository, evaluator| next if evaluator.tags.to_a.none? diff --git a/spec/features/issues/user_creates_branch_and_merge_request_spec.rb b/spec/features/issues/user_creates_branch_and_merge_request_spec.rb index 617eac88973..e225a45481d 100644 --- a/spec/features/issues/user_creates_branch_and_merge_request_spec.rb +++ b/spec/features/issues/user_creates_branch_and_merge_request_spec.rb @@ -250,7 +250,7 @@ RSpec.describe 'User creates branch and merge request on issue page', :js do def test_selection_mark(li_create_branch, li_create_merge_request, button_create_target, button_create_merge_request) page.within(li_create_merge_request) do - expect(page).to have_css('i.fa.fa-check') + expect(page).to have_selector('[data-testid="check-icon"]') expect(button_create_target).to have_text('Create merge request') expect(button_create_merge_request).to have_text('Create merge request') end @@ -258,7 +258,7 @@ RSpec.describe 'User creates branch and merge request on issue page', :js do li_create_branch.click page.within(li_create_branch) do - expect(page).to have_css('i.fa.fa-check') + expect(page).to have_selector('[data-testid="check-icon"]') expect(button_create_target).to have_text('Create branch') expect(button_create_merge_request).to have_text('Create branch') end diff --git a/spec/frontend/issuable/related_issues/components/issue_token_spec.js b/spec/frontend/issuable/related_issues/components/issue_token_spec.js index f2cb9042ba6..1b4c6b548e2 100644 --- a/spec/frontend/issuable/related_issues/components/issue_token_spec.js +++ b/spec/frontend/issuable/related_issues/components/issue_token_spec.js @@ -137,9 +137,7 @@ describe('IssueToken', () => { }); it('tooltip should not be escaped', () => { - expect(findRemoveBtn().attributes('data-original-title')).toBe( - `Remove ${displayReference}`, - ); + expect(findRemoveBtn().attributes('aria-label')).toBe(`Remove ${displayReference}`); }); }); }); diff --git a/spec/graphql/mutations/issues/set_assignees_spec.rb b/spec/graphql/mutations/issues/set_assignees_spec.rb index 77ba511b715..9a27c5acdac 100644 --- a/spec/graphql/mutations/issues/set_assignees_spec.rb +++ b/spec/graphql/mutations/issues/set_assignees_spec.rb @@ -3,6 +3,20 @@ require 'spec_helper' RSpec.describe Mutations::Issues::SetAssignees do + context 'when the user does not have permissions' do + let_it_be(:issue) { create(:issue) } + let_it_be(:user) { create(:user) } + let_it_be(:assignee) { create(:user) } + + subject(:mutation) { described_class.new(object: nil, context: { current_user: user }, field: nil) } + + describe '#resolve' do + subject { mutation.resolve(project_path: issue.project.full_path, iid: issue.iid, assignee_usernames: [assignee.username]) } + + it_behaves_like 'permission level for issue mutation is correctly verified' + end + end + it_behaves_like 'an assignable resource' do let_it_be(:resource, reload: true) { create(:issue) } end diff --git a/spec/graphql/mutations/issues/set_confidential_spec.rb b/spec/graphql/mutations/issues/set_confidential_spec.rb index 0b2fc0ecb93..c3269e5c0c0 100644 --- a/spec/graphql/mutations/issues/set_confidential_spec.rb +++ b/spec/graphql/mutations/issues/set_confidential_spec.rb @@ -17,9 +17,7 @@ RSpec.describe Mutations::Issues::SetConfidential do subject { mutation.resolve(project_path: project.full_path, iid: issue.iid, confidential: confidential) } - it 'raises an error if the resource is not accessible to the user' do - expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable) - end + it_behaves_like 'permission level for issue mutation is correctly verified' context 'when the user can update the issue' do before do diff --git a/spec/graphql/mutations/issues/set_due_date_spec.rb b/spec/graphql/mutations/issues/set_due_date_spec.rb index a638971d966..9f8d0d6c405 100644 --- a/spec/graphql/mutations/issues/set_due_date_spec.rb +++ b/spec/graphql/mutations/issues/set_due_date_spec.rb @@ -16,9 +16,7 @@ RSpec.describe Mutations::Issues::SetDueDate do subject { mutation.resolve(project_path: issue.project.full_path, iid: issue.iid, due_date: due_date) } - it 'raises an error if the resource is not accessible to the user' do - expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable) - end + it_behaves_like 'permission level for issue mutation is correctly verified' context 'when the user can update the issue' do before do diff --git a/spec/graphql/mutations/issues/set_locked_spec.rb b/spec/graphql/mutations/issues/set_locked_spec.rb index 10438226c17..1a0af0c6c63 100644 --- a/spec/graphql/mutations/issues/set_locked_spec.rb +++ b/spec/graphql/mutations/issues/set_locked_spec.rb @@ -15,9 +15,7 @@ RSpec.describe Mutations::Issues::SetLocked do subject { mutation.resolve(project_path: issue.project.full_path, iid: issue.iid, locked: locked) } - it 'raises an error if the resource is not accessible to the user' do - expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable) - end + it_behaves_like 'permission level for issue mutation is correctly verified' context 'when the user can update the issue' do let(:mutated_issue) { subject[:issue] } diff --git a/spec/graphql/mutations/issues/set_severity_spec.rb b/spec/graphql/mutations/issues/set_severity_spec.rb index ed73d3b777e..7698118ae3e 100644 --- a/spec/graphql/mutations/issues/set_severity_spec.rb +++ b/spec/graphql/mutations/issues/set_severity_spec.rb @@ -15,11 +15,7 @@ RSpec.describe Mutations::Issues::SetSeverity do subject(:resolve) { mutation.resolve(project_path: issue.project.full_path, iid: issue.iid, severity: severity) } - context 'when the user cannot update the issue' do - it 'raises an error' do - expect { resolve }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable) - end - end + it_behaves_like 'permission level for issue mutation is correctly verified' context 'when the user can update the issue' do before do diff --git a/spec/graphql/mutations/issues/update_spec.rb b/spec/graphql/mutations/issues/update_spec.rb index f9f4bdeb6fa..ce1eb874bcf 100644 --- a/spec/graphql/mutations/issues/update_spec.rb +++ b/spec/graphql/mutations/issues/update_spec.rb @@ -35,11 +35,7 @@ RSpec.describe Mutations::Issues::Update do subject { mutation.resolve(mutation_params) } - context 'when the user cannot access the issue' do - it 'raises an error' do - expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable) - end - end + it_behaves_like 'permission level for issue mutation is correctly verified' context 'when the user can update the issue' do before do diff --git a/spec/graphql/mutations/merge_requests/set_assignees_spec.rb b/spec/graphql/mutations/merge_requests/set_assignees_spec.rb index 4ac40fc09c6..e2eab591341 100644 --- a/spec/graphql/mutations/merge_requests/set_assignees_spec.rb +++ b/spec/graphql/mutations/merge_requests/set_assignees_spec.rb @@ -3,6 +3,20 @@ require 'spec_helper' RSpec.describe Mutations::MergeRequests::SetAssignees do + context 'when the user does not have permissions' do + let_it_be(:merge_request) { create(:merge_request) } + let_it_be(:user) { create(:user) } + let_it_be(:assignee) { create(:user) } + + subject(:mutation) { described_class.new(object: nil, context: { current_user: user }, field: nil) } + + describe '#resolve' do + subject { mutation.resolve(project_path: merge_request.project.full_path, iid: merge_request.iid, assignee_usernames: [assignee.username]) } + + it_behaves_like 'permission level for merge request mutation is correctly verified' + end + end + it_behaves_like 'an assignable resource' do let_it_be(:resource, reload: true) { create(:merge_request) } end diff --git a/spec/graphql/mutations/merge_requests/set_labels_spec.rb b/spec/graphql/mutations/merge_requests/set_labels_spec.rb index 62a7f650f84..1bb303cf99b 100644 --- a/spec/graphql/mutations/merge_requests/set_labels_spec.rb +++ b/spec/graphql/mutations/merge_requests/set_labels_spec.rb @@ -18,9 +18,7 @@ RSpec.describe Mutations::MergeRequests::SetLabels do subject { mutation.resolve(project_path: merge_request.project.full_path, iid: merge_request.iid, label_ids: label_ids) } - it 'raises an error if the resource is not accessible to the user' do - expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable) - end + it_behaves_like 'permission level for merge request mutation is correctly verified' context 'when the user can update the merge request' do before do diff --git a/spec/graphql/mutations/merge_requests/set_locked_spec.rb b/spec/graphql/mutations/merge_requests/set_locked_spec.rb index aca7df5445f..03c709e9bb3 100644 --- a/spec/graphql/mutations/merge_requests/set_locked_spec.rb +++ b/spec/graphql/mutations/merge_requests/set_locked_spec.rb @@ -16,9 +16,7 @@ RSpec.describe Mutations::MergeRequests::SetLocked do subject { mutation.resolve(project_path: merge_request.project.full_path, iid: merge_request.iid, locked: locked) } - it 'raises an error if the resource is not accessible to the user' do - expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable) - end + it_behaves_like 'permission level for merge request mutation is correctly verified' context 'when the user can update the merge request' do before do diff --git a/spec/graphql/mutations/merge_requests/set_milestone_spec.rb b/spec/graphql/mutations/merge_requests/set_milestone_spec.rb index ccb2d9bd132..4de857f43e3 100644 --- a/spec/graphql/mutations/merge_requests/set_milestone_spec.rb +++ b/spec/graphql/mutations/merge_requests/set_milestone_spec.rb @@ -18,6 +18,8 @@ RSpec.describe Mutations::MergeRequests::SetMilestone do expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable) end + it_behaves_like 'permission level for merge request mutation is correctly verified' + context 'when the user can update the merge request' do before do project.add_developer(user) diff --git a/spec/graphql/mutations/merge_requests/set_wip_spec.rb b/spec/graphql/mutations/merge_requests/set_wip_spec.rb index b6cb49724fa..69f6a4328b8 100644 --- a/spec/graphql/mutations/merge_requests/set_wip_spec.rb +++ b/spec/graphql/mutations/merge_requests/set_wip_spec.rb @@ -16,9 +16,7 @@ RSpec.describe Mutations::MergeRequests::SetWip do subject { mutation.resolve(project_path: merge_request.project.full_path, iid: merge_request.iid, wip: wip) } - it 'raises an error if the resource is not accessible to the user' do - expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable) - end + it_behaves_like 'permission level for merge request mutation is correctly verified' context 'when the user can update the merge request' do before do diff --git a/spec/graphql/mutations/merge_requests/update_spec.rb b/spec/graphql/mutations/merge_requests/update_spec.rb index 4a1fdf6e74b..8acd2562ea8 100644 --- a/spec/graphql/mutations/merge_requests/update_spec.rb +++ b/spec/graphql/mutations/merge_requests/update_spec.rb @@ -18,9 +18,7 @@ RSpec.describe Mutations::MergeRequests::Update do mutation.resolve(project_path: merge_request.project.full_path, iid: merge_request.iid, **attributes) end - it 'raises an error if the resource is not accessible to the user' do - expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable) - end + it_behaves_like 'permission level for merge request mutation is correctly verified' context 'when the user can update the merge request' do before do diff --git a/spec/lib/api/every_api_endpoint_spec.rb b/spec/lib/api/every_api_endpoint_spec.rb new file mode 100644 index 00000000000..a0657b5fbed --- /dev/null +++ b/spec/lib/api/every_api_endpoint_spec.rb @@ -0,0 +1,70 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'Every API endpoint' do + context 'feature categories' do + let_it_be(:feature_categories) do + YAML.load_file(Rails.root.join('config', 'feature_categories.yml')).map(&:to_sym).to_set + end + + let_it_be(:api_endpoints) do + API::API.routes.map do |route| + [route.app.options[:for], API::Base.path_for_app(route.app)] + end + end + + let_it_be(:routes_without_category) do + api_endpoints.map do |(klass, path)| + next if klass.try(:feature_category_for_action, path) + # We'll add the rest in https://gitlab.com/gitlab-com/gl-infra/scalability/-/issues/463 + next unless klass == ::API::Users || klass == ::API::Issues + + "#{klass}##{path}" + end.compact.uniq + end + + it "has feature categories" do + expect(routes_without_category).to be_empty, "#{routes_without_category} did not have a category" + end + + it "recognizes the feature categories" do + routes_unknown_category = api_endpoints.map do |(klass, path)| + used_category = klass.try(:feature_category_for_action, path) + next unless used_category + next if used_category == :not_owned + + [path, used_category] unless feature_categories.include?(used_category) + end.compact + + expect(routes_unknown_category).to be_empty, "#{routes_unknown_category.first(10)} had an unknown category" + end + + # This is required for API::Base.path_for_app to work, as it picks + # the first path + it "has no routes with multiple paths" do + routes_with_multiple_paths = API::API.routes.select { |route| route.app.options[:path].length != 1 } + failure_routes = routes_with_multiple_paths.map { |route| "#{route.app.options[:for]}:[#{route.app.options[:path].join(', ')}]" } + + expect(routes_with_multiple_paths).to be_empty, "#{failure_routes} have multiple paths" + end + + it "doesn't define or exclude categories on removed actions", :aggregate_failures do + api_endpoints.group_by(&:first).each do |klass, paths| + existing_paths = paths.map(&:last) + used_paths = paths_defined_in_feature_category_config(klass) + non_existing_used_paths = used_paths - existing_paths + + expect(non_existing_used_paths).to be_empty, + "#{klass} used #{non_existing_used_paths} to define feature category, but the route does not exist" + end + end + end + + def paths_defined_in_feature_category_config(klass) + (klass.try(:class_attributes) || {}).fetch(:feature_category_config, {}) + .values + .flatten + .map(&:to_s) + end +end diff --git a/spec/lib/gitlab/metrics/requests_rack_middleware_spec.rb b/spec/lib/gitlab/metrics/requests_rack_middleware_spec.rb index 631325402d9..9c9ca7411bf 100644 --- a/spec/lib/gitlab/metrics/requests_rack_middleware_spec.rb +++ b/spec/lib/gitlab/metrics/requests_rack_middleware_spec.rb @@ -113,24 +113,38 @@ RSpec.describe Gitlab::Metrics::RequestsRackMiddleware do end end - context 'when a feature category header is present' do - before do - allow(app).to receive(:call).and_return([200, { described_class::FEATURE_CATEGORY_HEADER => 'issue_tracking' }, nil]) - end + context 'feature category header' do + context 'when a feature category header is present' do + before do + allow(app).to receive(:call).and_return([200, { described_class::FEATURE_CATEGORY_HEADER => 'issue_tracking' }, nil]) + end - it 'adds the feature category to the labels for http_request_total' do - expect(described_class).to receive_message_chain(:http_request_total, :increment).with(method: 'get', status: 200, feature_category: 'issue_tracking') + it 'adds the feature category to the labels for http_request_total' do + expect(described_class).to receive_message_chain(:http_request_total, :increment).with(method: 'get', status: 200, feature_category: 'issue_tracking') - subject.call(env) + subject.call(env) + end + + it 'does not record a feature category for health check endpoints' do + env['PATH_INFO'] = '/-/liveness' + + expect(described_class).to receive_message_chain(:http_health_requests_total, :increment).with(method: 'get', status: 200) + expect(described_class).not_to receive(:http_request_total) + + subject.call(env) + end end - it 'does not record a feature category for health check endpoints' do - env['PATH_INFO'] = '/-/liveness' + context 'when the feature category header is an empty string' do + before do + allow(app).to receive(:call).and_return([200, { described_class::FEATURE_CATEGORY_HEADER => '' }, nil]) + end - expect(described_class).to receive_message_chain(:http_health_requests_total, :increment).with(method: 'get', status: 200) - expect(described_class).not_to receive(:http_request_total) + it 'sets the feature category to unknown' do + expect(described_class).to receive_message_chain(:http_request_total, :increment).with(method: 'get', status: 200, feature_category: 'unknown') - subject.call(env) + subject.call(env) + end end end diff --git a/spec/lib/gitlab/sidekiq_middleware/duplicate_jobs/client_spec.rb b/spec/lib/gitlab/sidekiq_middleware/duplicate_jobs/client_spec.rb index 98350fb9b8e..4d12e4b3f6f 100644 --- a/spec/lib/gitlab/sidekiq_middleware/duplicate_jobs/client_spec.rb +++ b/spec/lib/gitlab/sidekiq_middleware/duplicate_jobs/client_spec.rb @@ -3,79 +3,84 @@ require 'spec_helper' RSpec.describe Gitlab::SidekiqMiddleware::DuplicateJobs::Client, :clean_gitlab_redis_queues do - let(:worker_class) do - Class.new do - def self.name - 'TestDeduplicationWorker' - end + shared_context 'deduplication worker class' do |strategy, including_scheduled| + let(:worker_class) do + Class.new do + def self.name + 'TestDeduplicationWorker' + end + + include ApplicationWorker + + deduplicate strategy, including_scheduled: including_scheduled - include ApplicationWorker + include ApplicationWorker - def perform(*args) + def perform(*args) + end end end - end - before do - stub_const('TestDeduplicationWorker', worker_class) + before do + stub_const('TestDeduplicationWorker', worker_class) + end end - describe '#call' do - it 'adds a correct duplicate tag to the jobs', :aggregate_failures do - TestDeduplicationWorker.bulk_perform_async([['args1'], ['args2'], ['args1']]) + shared_examples 'client duplicate job' do |strategy| + describe '#call' do + include_context 'deduplication worker class', strategy, false - job1, job2, job3 = TestDeduplicationWorker.jobs - - expect(job1['duplicate-of']).to be_nil - expect(job2['duplicate-of']).to be_nil - expect(job3['duplicate-of']).to eq(job1['jid']) - end - - context 'without scheduled deduplication' do - it "does not mark a job that's scheduled in the future as a duplicate" do - TestDeduplicationWorker.perform_async('args1') - TestDeduplicationWorker.perform_at(1.day.from_now, 'args1') - TestDeduplicationWorker.perform_in(3.hours, 'args1') + it 'adds a correct duplicate tag to the jobs', :aggregate_failures do + TestDeduplicationWorker.bulk_perform_async([['args1'], ['args2'], ['args1']]) - duplicates = TestDeduplicationWorker.jobs.map { |job| job['duplicate-of'] } + job1, job2, job3 = TestDeduplicationWorker.jobs - expect(duplicates).to all(be_nil) + expect(job1['duplicate-of']).to be_nil + expect(job2['duplicate-of']).to be_nil + expect(job3['duplicate-of']).to eq(job1['jid']) end - end - - context 'with scheduled deduplication' do - let(:scheduled_worker_class) do - Class.new do - def self.name - 'TestDeduplicationWorker' - end - include ApplicationWorker + context 'without scheduled deduplication' do + it "does not mark a job that's scheduled in the future as a duplicate" do + TestDeduplicationWorker.perform_async('args1') + TestDeduplicationWorker.perform_at(1.day.from_now, 'args1') + TestDeduplicationWorker.perform_in(3.hours, 'args1') - deduplicate :until_executing, including_scheduled: true + duplicates = TestDeduplicationWorker.jobs.map { |job| job['duplicate-of'] } - def perform(*args) - end + expect(duplicates).to all(be_nil) end end - before do - stub_const('TestDeduplicationWorker', scheduled_worker_class) - end + context 'with scheduled deduplication' do + include_context 'deduplication worker class', strategy, true - it 'adds a correct duplicate tag to the jobs', :aggregate_failures do - TestDeduplicationWorker.perform_async('args1') - TestDeduplicationWorker.perform_at(1.day.from_now, 'args1') - TestDeduplicationWorker.perform_in(3.hours, 'args1') - TestDeduplicationWorker.perform_in(3.hours, 'args2') + before do + stub_const('TestDeduplicationWorker', worker_class) + end - job1, job2, job3, job4 = TestDeduplicationWorker.jobs + it 'adds a correct duplicate tag to the jobs', :aggregate_failures do + TestDeduplicationWorker.perform_async('args1') + TestDeduplicationWorker.perform_at(1.day.from_now, 'args1') + TestDeduplicationWorker.perform_in(3.hours, 'args1') + TestDeduplicationWorker.perform_in(3.hours, 'args2') - expect(job1['duplicate-of']).to be_nil - expect(job2['duplicate-of']).to eq(job1['jid']) - expect(job3['duplicate-of']).to eq(job1['jid']) - expect(job4['duplicate-of']).to be_nil + job1, job2, job3, job4 = TestDeduplicationWorker.jobs + + expect(job1['duplicate-of']).to be_nil + expect(job2['duplicate-of']).to eq(job1['jid']) + expect(job3['duplicate-of']).to eq(job1['jid']) + expect(job4['duplicate-of']).to be_nil + end end end end + + context 'with until_executing strategy' do + it_behaves_like 'client duplicate job', :until_executing + end + + context 'with until_executed strategy' do + it_behaves_like 'client duplicate job', :until_executed + end end diff --git a/spec/lib/gitlab/sidekiq_middleware/duplicate_jobs/server_spec.rb b/spec/lib/gitlab/sidekiq_middleware/duplicate_jobs/server_spec.rb index 3f75d867936..09548d21106 100644 --- a/spec/lib/gitlab/sidekiq_middleware/duplicate_jobs/server_spec.rb +++ b/spec/lib/gitlab/sidekiq_middleware/duplicate_jobs/server_spec.rb @@ -3,39 +3,71 @@ require 'spec_helper' RSpec.describe Gitlab::SidekiqMiddleware::DuplicateJobs::Server, :clean_gitlab_redis_queues do - let(:worker_class) do - Class.new do - def self.name - 'TestDeduplicationWorker' + shared_context 'server duplicate job' do |strategy| + let(:worker_class) do + Class.new do + def self.name + 'TestDeduplicationWorker' + end + + include ApplicationWorker + + deduplicate strategy + + def perform(*args) + self.class.work + end + + def self.work + end end + end - include ApplicationWorker + before do + stub_const('TestDeduplicationWorker', worker_class) + end - def perform(*args) + around do |example| + with_sidekiq_server_middleware do |chain| + chain.add described_class + Sidekiq::Testing.inline! { example.run } end end end - before do - stub_const('TestDeduplicationWorker', worker_class) - end + context 'with until_executing strategy' do + include_context 'server duplicate job', :until_executing - around do |example| - with_sidekiq_server_middleware do |chain| - chain.add described_class - Sidekiq::Testing.inline! { example.run } + describe '#call' do + it 'removes the stored job from redis before execution' do + bare_job = { 'class' => 'TestDeduplicationWorker', 'args' => ['hello'] } + job_definition = Gitlab::SidekiqMiddleware::DuplicateJobs::DuplicateJob.new(bare_job.dup, 'test_deduplication') + + expect(Gitlab::SidekiqMiddleware::DuplicateJobs::DuplicateJob) + .to receive(:new).with(a_hash_including(bare_job), 'test_deduplication') + .and_return(job_definition).twice # once in client middleware + + expect(job_definition).to receive(:delete!).ordered.and_call_original + expect(TestDeduplicationWorker).to receive(:work).ordered.and_call_original + + TestDeduplicationWorker.perform_async('hello') + end end end - describe '#call' do - it 'removes the stored job from redis' do + context 'with until_executed strategy' do + include_context 'server duplicate job', :until_executed + + it 'removes the stored job from redis after execution' do bare_job = { 'class' => 'TestDeduplicationWorker', 'args' => ['hello'] } job_definition = Gitlab::SidekiqMiddleware::DuplicateJobs::DuplicateJob.new(bare_job.dup, 'test_deduplication') expect(Gitlab::SidekiqMiddleware::DuplicateJobs::DuplicateJob) .to receive(:new).with(a_hash_including(bare_job), 'test_deduplication') .and_return(job_definition).twice # once in client middleware - expect(job_definition).to receive(:delete!).and_call_original + + expect(TestDeduplicationWorker).to receive(:work).ordered.and_call_original + expect(job_definition).to receive(:delete!).ordered.and_call_original TestDeduplicationWorker.perform_async('hello') end diff --git a/spec/lib/gitlab/sidekiq_middleware/duplicate_jobs/strategies/until_executed_spec.rb b/spec/lib/gitlab/sidekiq_middleware/duplicate_jobs/strategies/until_executed_spec.rb new file mode 100644 index 00000000000..b3d463b6f6b --- /dev/null +++ b/spec/lib/gitlab/sidekiq_middleware/duplicate_jobs/strategies/until_executed_spec.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::SidekiqMiddleware::DuplicateJobs::Strategies::UntilExecuted do + it_behaves_like 'deduplicating jobs when scheduling', :until_executed do + describe '#perform' do + let(:proc) { -> {} } + + it 'deletes the lock after executing' do + expect(proc).to receive(:call).ordered + expect(fake_duplicate_job).to receive(:delete!).ordered + + strategy.perform({}) do + proc.call + end + end + end + end +end diff --git a/spec/lib/gitlab/sidekiq_middleware/duplicate_jobs/strategies/until_executing_spec.rb b/spec/lib/gitlab/sidekiq_middleware/duplicate_jobs/strategies/until_executing_spec.rb index 10b18052e9a..d45b6c5fcd1 100644 --- a/spec/lib/gitlab/sidekiq_middleware/duplicate_jobs/strategies/until_executing_spec.rb +++ b/spec/lib/gitlab/sidekiq_middleware/duplicate_jobs/strategies/until_executing_spec.rb @@ -1,146 +1,20 @@ # frozen_string_literal: true -require 'fast_spec_helper' +require 'spec_helper' RSpec.describe Gitlab::SidekiqMiddleware::DuplicateJobs::Strategies::UntilExecuting do - let(:fake_duplicate_job) do - instance_double(Gitlab::SidekiqMiddleware::DuplicateJobs::DuplicateJob) - end - - subject(:strategy) { described_class.new(fake_duplicate_job) } - - describe '#schedule' do - before do - allow(Gitlab::SidekiqLogging::DeduplicationLogger.instance).to receive(:log) - end - - it 'checks for duplicates before yielding' do - expect(fake_duplicate_job).to receive(:scheduled?).twice.ordered.and_return(false) - expect(fake_duplicate_job).to( - receive(:check!) - .with(Gitlab::SidekiqMiddleware::DuplicateJobs::DuplicateJob::DUPLICATE_KEY_TTL) - .ordered - .and_return('a jid')) - expect(fake_duplicate_job).to receive(:duplicate?).ordered.and_return(false) - - expect { |b| strategy.schedule({}, &b) }.to yield_control - end - - it 'checks worker options for scheduled jobs' do - expect(fake_duplicate_job).to receive(:scheduled?).ordered.and_return(true) - expect(fake_duplicate_job).to receive(:options).ordered.and_return({}) - expect(fake_duplicate_job).not_to receive(:check!) - - expect { |b| strategy.schedule({}, &b) }.to yield_control - end - - context 'job marking' do - it 'adds the jid of the existing job to the job hash' do - allow(fake_duplicate_job).to receive(:scheduled?).and_return(false) - allow(fake_duplicate_job).to receive(:check!).and_return('the jid') - allow(fake_duplicate_job).to receive(:droppable?).and_return(true) - allow(fake_duplicate_job).to receive(:options).and_return({}) - job_hash = {} + it_behaves_like 'deduplicating jobs when scheduling', :until_executing do + describe '#perform' do + let(:proc) { -> {} } - expect(fake_duplicate_job).to receive(:duplicate?).and_return(true) - expect(fake_duplicate_job).to receive(:existing_jid).and_return('the jid') + it 'deletes the lock before executing' do + expect(fake_duplicate_job).to receive(:delete!).ordered + expect(proc).to receive(:call).ordered - strategy.schedule(job_hash) {} - - expect(job_hash).to include('duplicate-of' => 'the jid') - end - - context 'scheduled jobs' do - let(:time_diff) { 1.minute } - - context 'scheduled in the past' do - it 'adds the jid of the existing job to the job hash' do - allow(fake_duplicate_job).to receive(:scheduled?).twice.and_return(true) - allow(fake_duplicate_job).to receive(:scheduled_at).and_return(Time.now - time_diff) - allow(fake_duplicate_job).to receive(:options).and_return({ including_scheduled: true }) - allow(fake_duplicate_job).to( - receive(:check!) - .with(Gitlab::SidekiqMiddleware::DuplicateJobs::DuplicateJob::DUPLICATE_KEY_TTL) - .and_return('the jid')) - allow(fake_duplicate_job).to receive(:droppable?).and_return(true) - job_hash = {} - - expect(fake_duplicate_job).to receive(:duplicate?).and_return(true) - expect(fake_duplicate_job).to receive(:existing_jid).and_return('the jid') - - strategy.schedule(job_hash) {} - - expect(job_hash).to include('duplicate-of' => 'the jid') - end + strategy.perform({}) do + proc.call end - - context 'scheduled in the future' do - it 'adds the jid of the existing job to the job hash' do - freeze_time do - allow(fake_duplicate_job).to receive(:scheduled?).twice.and_return(true) - allow(fake_duplicate_job).to receive(:scheduled_at).and_return(Time.now + time_diff) - allow(fake_duplicate_job).to receive(:options).and_return({ including_scheduled: true }) - allow(fake_duplicate_job).to( - receive(:check!).with(time_diff.to_i).and_return('the jid')) - allow(fake_duplicate_job).to receive(:droppable?).and_return(true) - job_hash = {} - - expect(fake_duplicate_job).to receive(:duplicate?).and_return(true) - expect(fake_duplicate_job).to receive(:existing_jid).and_return('the jid') - - strategy.schedule(job_hash) {} - - expect(job_hash).to include('duplicate-of' => 'the jid') - end - end - end - end - end - - context "when the job is droppable" do - before do - allow(fake_duplicate_job).to receive(:scheduled?).and_return(false) - allow(fake_duplicate_job).to receive(:check!).and_return('the jid') - allow(fake_duplicate_job).to receive(:duplicate?).and_return(true) - allow(fake_duplicate_job).to receive(:options).and_return({}) - allow(fake_duplicate_job).to receive(:existing_jid).and_return('the jid') - allow(fake_duplicate_job).to receive(:droppable?).and_return(true) - end - - it 'drops the job' do - schedule_result = nil - - expect(fake_duplicate_job).to receive(:droppable?).and_return(true) - - expect { |b| schedule_result = strategy.schedule({}, &b) }.not_to yield_control - expect(schedule_result).to be(false) - end - - it 'logs that the job was dropped' do - fake_logger = instance_double(Gitlab::SidekiqLogging::DeduplicationLogger) - - expect(Gitlab::SidekiqLogging::DeduplicationLogger).to receive(:instance).and_return(fake_logger) - expect(fake_logger).to receive(:log).with(a_hash_including({ 'jid' => 'new jid' }), 'dropped until executing', {}) - - strategy.schedule({ 'jid' => 'new jid' }) {} - end - - it 'logs the deduplication options of the worker' do - fake_logger = instance_double(Gitlab::SidekiqLogging::DeduplicationLogger) - - expect(Gitlab::SidekiqLogging::DeduplicationLogger).to receive(:instance).and_return(fake_logger) - allow(fake_duplicate_job).to receive(:options).and_return({ foo: :bar }) - expect(fake_logger).to receive(:log).with(a_hash_including({ 'jid' => 'new jid' }), 'dropped until executing', { foo: :bar }) - - strategy.schedule({ 'jid' => 'new jid' }) {} end end end - - describe '#perform' do - it 'deletes the lock before executing' do - expect(fake_duplicate_job).to receive(:delete!).ordered - expect { |b| strategy.perform({}, &b) }.to yield_control - end - end end diff --git a/spec/lib/gitlab/sidekiq_middleware/duplicate_jobs/strategies_spec.rb b/spec/lib/gitlab/sidekiq_middleware/duplicate_jobs/strategies_spec.rb index 84856238aab..e35d779f334 100644 --- a/spec/lib/gitlab/sidekiq_middleware/duplicate_jobs/strategies_spec.rb +++ b/spec/lib/gitlab/sidekiq_middleware/duplicate_jobs/strategies_spec.rb @@ -8,6 +8,10 @@ RSpec.describe Gitlab::SidekiqMiddleware::DuplicateJobs::Strategies do expect(described_class.for(:until_executing)).to eq(described_class::UntilExecuting) end + it 'returns the right class for `until_executed`' do + expect(described_class.for(:until_executed)).to eq(described_class::UntilExecuted) + end + it 'returns the right class for `none`' do expect(described_class.for(:none)).to eq(described_class::None) end diff --git a/spec/controllers/concerns/controller_with_feature_category_spec.rb b/spec/lib/gitlab/with_feature_category_spec.rb index 55e84755f5c..b6fe1c84b26 100644 --- a/spec/controllers/concerns/controller_with_feature_category_spec.rb +++ b/spec/lib/gitlab/with_feature_category_spec.rb @@ -1,13 +1,13 @@ # frozen_string_literal: true require 'fast_spec_helper' -require_relative "../../../app/controllers/concerns/controller_with_feature_category" +require_relative "../../../lib/gitlab/with_feature_category" -RSpec.describe ControllerWithFeatureCategory do +RSpec.describe Gitlab::WithFeatureCategory do describe ".feature_category_for_action" do let(:base_controller) do Class.new do - include ControllerWithFeatureCategory + include ::Gitlab::WithFeatureCategory end end @@ -56,5 +56,14 @@ RSpec.describe ControllerWithFeatureCategory do end end.to raise_error(ArgumentError, "Actions have multiple feature categories: world") end + + it "does not raise an error when multiple calls define the same action and feature category" do + expect do + Class.new(base_controller) do + feature_category :hello, [:world] + feature_category :hello, ["world"] + end + end.not_to raise_error + end end end diff --git a/spec/models/application_setting_spec.rb b/spec/models/application_setting_spec.rb index 26c97462bf2..275b35c5c24 100644 --- a/spec/models/application_setting_spec.rb +++ b/spec/models/application_setting_spec.rb @@ -72,6 +72,7 @@ RSpec.describe ApplicationSetting do it { is_expected.not_to allow_value(nil).for(:push_event_activities_limit) } it { is_expected.to validate_numericality_of(:container_registry_delete_tags_service_timeout).only_integer.is_greater_than_or_equal_to(0) } + it { is_expected.to validate_numericality_of(:container_registry_expiration_policies_worker_capacity).only_integer.is_greater_than_or_equal_to(0) } it { is_expected.to validate_numericality_of(:snippet_size_limit).only_integer.is_greater_than(0) } it { is_expected.to validate_numericality_of(:wiki_page_max_content_bytes).only_integer.is_greater_than_or_equal_to(1024) } diff --git a/spec/models/ci/build_trace_chunk_spec.rb b/spec/models/ci/build_trace_chunk_spec.rb index cdbdd2b1d20..0b0b5a7554c 100644 --- a/spec/models/ci/build_trace_chunk_spec.rb +++ b/spec/models/ci/build_trace_chunk_spec.rb @@ -594,23 +594,19 @@ RSpec.describe Ci::BuildTraceChunk, :clean_gitlab_redis_shared_state do context 'when the chunk is being locked by a different worker' do let(:metrics) { spy('metrics') } - it 'does not raise an exception' do - lock_chunk do - expect { build_trace_chunk.persist_data! }.not_to raise_error - end - end - it 'increments stalled chunk trace metric' do allow(build_trace_chunk) .to receive(:metrics) .and_return(metrics) - lock_chunk { build_trace_chunk.persist_data! } + expect do + subject - expect(metrics) - .to have_received(:increment_trace_operation) - .with(operation: :stalled) - .once + expect(metrics) + .to have_received(:increment_trace_operation) + .with(operation: :stalled) + .once + end.to raise_error(described_class::FailedToPersistDataError) end def lock_chunk(&block) diff --git a/spec/models/ci/daily_build_group_report_result_spec.rb b/spec/models/ci/daily_build_group_report_result_spec.rb index 761fd1882dc..68c5d58cfd5 100644 --- a/spec/models/ci/daily_build_group_report_result_spec.rb +++ b/spec/models/ci/daily_build_group_report_result_spec.rb @@ -104,5 +104,24 @@ RSpec.describe Ci::DailyBuildGroupReportResult do expect(subject).to contain_exactly(recent_build_group_report_result, old_build_group_report_result) end end + + describe '.with_default_branch' do + subject(:coverages) { described_class.with_default_branch } + + context 'when coverage for the default branch exist' do + let!(:recent_build_group_report_result) { create(:ci_daily_build_group_report_result, project: project) } + let!(:coverage_feature_branch) { create(:ci_daily_build_group_report_result, :on_feature_branch, project: project) } + + it 'returns coverage with the default branch' do + expect(coverages).to contain_exactly(recent_build_group_report_result) + end + end + + context 'when coverage for the default branch does not exist' do + it 'returns an empty collection' do + expect(coverages).to be_empty + end + end + end end end diff --git a/spec/models/container_expiration_policy_spec.rb b/spec/models/container_expiration_policy_spec.rb index 1d9dbe8a867..94441da49fc 100644 --- a/spec/models/container_expiration_policy_spec.rb +++ b/spec/models/container_expiration_policy_spec.rb @@ -38,6 +38,33 @@ RSpec.describe ContainerExpirationPolicy, type: :model do it { is_expected.not_to allow_value('foo').for(:keep_n) } end + describe '#disable!' do + let_it_be(:policy) { create(:container_expiration_policy) } + + subject { policy.disable! } + + it 'disables the container expiration policy' do + expect { subject }.to change { policy.reload.enabled }.from(true).to(false) + end + end + + describe '#policy_params' do + let_it_be(:policy) { create(:container_expiration_policy) } + + let(:expected) do + { + 'older_than' => policy.older_than, + 'keep_n' => policy.keep_n, + 'name_regex' => policy.name_regex, + 'name_regex_keep' => policy.name_regex_keep + } + end + + subject { policy.policy_params } + + it { is_expected.to eq(expected) } + end + context 'with a set of regexps' do valid_regexps = %w[master .* v.+ v10.1.* (?:v.+|master|release)] invalid_regexps = ['[', '(?:v.+|master|release'] @@ -104,25 +131,15 @@ RSpec.describe ContainerExpirationPolicy, type: :model do end end - describe '.executable' do - subject { described_class.executable } + describe '.with_container_repositories' do + subject { described_class.with_container_repositories } - let_it_be(:policy1) { create(:container_expiration_policy, :runnable) } + let_it_be(:policy1) { create(:container_expiration_policy) } let_it_be(:container_repository1) { create(:container_repository, project: policy1.project) } - let_it_be(:policy2) { create(:container_expiration_policy, :runnable) } + let_it_be(:policy2) { create(:container_expiration_policy) } let_it_be(:container_repository2) { create(:container_repository, project: policy2.project) } - let_it_be(:policy3) { create(:container_expiration_policy, :runnable) } + let_it_be(:policy3) { create(:container_expiration_policy) } it { is_expected.to contain_exactly(policy1, policy2) } end - - describe '#disable!' do - let_it_be(:container_expiration_policy) { create(:container_expiration_policy) } - - subject { container_expiration_policy.disable! } - - it 'disables the container expiration policy' do - expect { subject }.to change { container_expiration_policy.reload.enabled }.from(true).to(false) - end - end end diff --git a/spec/models/container_repository_spec.rb b/spec/models/container_repository_spec.rb index 2a7aaed5204..2adceb1c960 100644 --- a/spec/models/container_repository_spec.rb +++ b/spec/models/container_repository_spec.rb @@ -352,4 +352,20 @@ RSpec.describe ContainerRepository do it { is_expected.to contain_exactly(repository) } end + + describe '.for_project_id' do + subject { described_class.for_project_id(project.id) } + + it { is_expected.to contain_exactly(repository) } + end + + describe '.waiting_for_cleanup' do + let_it_be(:repository_cleanup_scheduled) { create(:container_repository, :cleanup_scheduled) } + let_it_be(:repository_cleanup_unfinished) { create(:container_repository, :cleanup_unfinished) } + let_it_be(:repository_cleanup_ongoing) { create(:container_repository, :cleanup_ongoing) } + + subject { described_class.waiting_for_cleanup } + + it { is_expected.to contain_exactly(repository_cleanup_scheduled, repository_cleanup_unfinished) } + end end diff --git a/spec/requests/api/api_spec.rb b/spec/requests/api/api_spec.rb index 7d637757f38..3cc8764de4a 100644 --- a/spec/requests/api/api_spec.rb +++ b/spec/requests/api/api_spec.rb @@ -92,4 +92,36 @@ RSpec.describe API::API do end end end + + context 'application context' do + let_it_be(:project) { create(:project) } + let_it_be(:user) { project.owner } + + it 'logs all application context fields' do + allow_any_instance_of(Gitlab::GrapeLogging::Loggers::ContextLogger).to receive(:parameters) do + Labkit::Context.current.to_h.tap do |log_context| + expect(log_context).to match('correlation_id' => an_instance_of(String), + 'meta.caller_id' => '/api/:version/projects/:id/issues', + 'meta.project' => project.full_path, + 'meta.root_namespace' => project.namespace.full_path, + 'meta.user' => user.username, + 'meta.feature_category' => 'issue_tracking') + end + end + + get(api("/projects/#{project.id}/issues", user)) + end + + it 'skips fields that do not apply' do + allow_any_instance_of(Gitlab::GrapeLogging::Loggers::ContextLogger).to receive(:parameters) do + Labkit::Context.current.to_h.tap do |log_context| + expect(log_context).to match('correlation_id' => an_instance_of(String), + 'meta.caller_id' => '/api/:version/users', + 'meta.feature_category' => 'users') + end + end + + get(api('/users')) + end + end end diff --git a/spec/services/ci/daily_build_group_report_result_service_spec.rb b/spec/services/ci/daily_build_group_report_result_service_spec.rb index f196afb05e8..e54f10cc4f4 100644 --- a/spec/services/ci/daily_build_group_report_result_service_spec.rb +++ b/spec/services/ci/daily_build_group_report_result_service_spec.rb @@ -7,6 +7,7 @@ RSpec.describe Ci::DailyBuildGroupReportResultService, '#execute' do let!(:rspec_job) { create(:ci_build, pipeline: pipeline, name: '3/3 rspec', coverage: 80) } let!(:karma_job) { create(:ci_build, pipeline: pipeline, name: '2/2 karma', coverage: 90) } let!(:extra_job) { create(:ci_build, pipeline: pipeline, name: 'extra', coverage: nil) } + let(:coverages) { Ci::DailyBuildGroupReportResult.all } it 'creates daily code coverage record for each job in the pipeline that has coverage value' do described_class.new.execute(pipeline) @@ -158,4 +159,30 @@ RSpec.describe Ci::DailyBuildGroupReportResultService, '#execute' do expect { described_class.new.execute(new_pipeline) }.not_to raise_error end end + + context 'when pipeline ref_path is the project default branch' do + let(:default_branch) { 'master' } + + before do + allow(pipeline.project).to receive(:default_branch).and_return(default_branch) + end + + it 'sets default branch to true' do + described_class.new.execute(pipeline) + + coverages.each do |coverage| + expect(coverage.default_branch).to be_truthy + end + end + end + + context 'when pipeline ref_path is not the project default branch' do + it 'sets default branch to false' do + described_class.new.execute(pipeline) + + coverages.each do |coverage| + expect(coverage.default_branch).to be_falsey + end + end + end end diff --git a/spec/services/container_expiration_policies/cleanup_service_spec.rb b/spec/services/container_expiration_policies/cleanup_service_spec.rb new file mode 100644 index 00000000000..2da35cfc3fb --- /dev/null +++ b/spec/services/container_expiration_policies/cleanup_service_spec.rb @@ -0,0 +1,62 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ContainerExpirationPolicies::CleanupService do + let_it_be(:repository, reload: true) { create(:container_repository) } + let_it_be(:project) { repository.project } + + let(:service) { described_class.new(repository) } + + describe '#execute' do + subject { service.execute } + + context 'with a successful cleanup tags service execution' do + let(:cleanup_tags_service_params) { project.container_expiration_policy.policy_params.merge('container_expiration_policy' => true) } + let(:cleanup_tags_service) { instance_double(Projects::ContainerRepository::CleanupTagsService) } + + it 'completely clean up the repository' do + expect(Projects::ContainerRepository::CleanupTagsService) + .to receive(:new).with(project, nil, cleanup_tags_service_params).and_return(cleanup_tags_service) + expect(cleanup_tags_service).to receive(:execute).with(repository).and_return(status: :success) + + response = subject + + aggregate_failures "checking the response and container repositories" do + expect(response.success?).to eq(true) + expect(response.payload).to include(cleanup_status: :finished, container_repository_id: repository.id) + expect(ContainerRepository.waiting_for_cleanup.count).to eq(0) + expect(repository.reload.cleanup_unscheduled?).to be_truthy + expect(repository.expiration_policy_started_at).to eq(nil) + end + end + end + + context 'without a successful cleanup tags service execution' do + it 'partially clean up the repository' do + expect(Projects::ContainerRepository::CleanupTagsService) + .to receive(:new).and_return(double(execute: { status: :error, message: 'timeout' })) + + response = subject + + aggregate_failures "checking the response and container repositories" do + expect(response.success?).to eq(true) + expect(response.payload).to include(cleanup_status: :unfinished, container_repository_id: repository.id) + expect(ContainerRepository.waiting_for_cleanup.count).to eq(1) + expect(repository.reload.cleanup_unfinished?).to be_truthy + expect(repository.expiration_policy_started_at).not_to eq(nil) + end + end + end + + context 'with no repository' do + let(:service) { described_class.new(nil) } + + it 'returns an error response' do + response = subject + + expect(response.success?).to eq(false) + end + end + end +end diff --git a/spec/services/container_expiration_policy_service_spec.rb b/spec/services/container_expiration_policy_service_spec.rb index dfce51d73ad..4294e6b3f06 100644 --- a/spec/services/container_expiration_policy_service_spec.rb +++ b/spec/services/container_expiration_policy_service_spec.rb @@ -27,20 +27,5 @@ RSpec.describe ContainerExpirationPolicyService do expect(container_expiration_policy.next_run_at).to be > Time.zone.now end - - context 'with an invalid container expiration policy' do - before do - allow(container_expiration_policy).to receive(:valid?).and_return(false) - end - - it 'disables it' do - expect(container_expiration_policy).not_to receive(:schedule_next_run!) - expect(CleanupContainerRepositoryWorker).not_to receive(:perform_async) - - expect { subject } - .to change { container_expiration_policy.reload.enabled }.from(true).to(false) - .and raise_error(ContainerExpirationPolicyService::InvalidPolicyError) - end - end end end diff --git a/spec/support/shared_examples/features/discussion_comments_shared_example.rb b/spec/support/shared_examples/features/discussion_comments_shared_example.rb index 9fc5d8933e5..560cfbfb117 100644 --- a/spec/support/shared_examples/features/discussion_comments_shared_example.rb +++ b/spec/support/shared_examples/features/discussion_comments_shared_example.rb @@ -56,12 +56,12 @@ RSpec.shared_examples 'thread comments' do |resource_name| expect(items.first).to have_content 'Comment' expect(items.first).to have_content "Add a general comment to this #{resource_name}." - expect(items.first).to have_selector '.fa-check' + expect(items.first).to have_selector '[data-testid="check-icon"]' expect(items.first['class']).to match 'droplab-item-selected' expect(items.last).to have_content 'Start thread' expect(items.last).to have_content "Discuss a specific suggestion or question#{' that needs to be resolved' if resource_name == 'merge request'}." - expect(items.last).not_to have_selector '.fa-check' + expect(items.last).not_to have_selector '[data-testid="check-icon"]' expect(items.last['class']).not_to match 'droplab-item-selected' end @@ -228,11 +228,11 @@ RSpec.shared_examples 'thread comments' do |resource_name| items = all("#{menu_selector} li") expect(items.first).to have_content 'Comment' - expect(items.first).not_to have_selector '.fa-check' + expect(items.first).not_to have_selector '[data-testid="check-icon"]' expect(items.first['class']).not_to match 'droplab-item-selected' expect(items.last).to have_content 'Start thread' - expect(items.last).to have_selector '.fa-check' + expect(items.last).to have_selector '[data-testid="check-icon"]' expect(items.last['class']).to match 'droplab-item-selected' end @@ -274,11 +274,11 @@ RSpec.shared_examples 'thread comments' do |resource_name| aggregate_failures do expect(items.first).to have_content 'Comment' - expect(items.first).to have_selector '.fa-check' + expect(items.first).to have_selector '[data-testid="check-icon"]' expect(items.first['class']).to match 'droplab-item-selected' expect(items.last).to have_content 'Start thread' - expect(items.last).not_to have_selector '.fa-check' + expect(items.last).not_to have_selector '[data-testid="check-icon"]' expect(items.last['class']).not_to match 'droplab-item-selected' end end diff --git a/spec/support/shared_examples/graphql/mutations/issues/permission_check_shared_examples.rb b/spec/support/shared_examples/graphql/mutations/issues/permission_check_shared_examples.rb new file mode 100644 index 00000000000..34c58f524cd --- /dev/null +++ b/spec/support/shared_examples/graphql/mutations/issues/permission_check_shared_examples.rb @@ -0,0 +1,52 @@ +# frozen_string_literal: true + +RSpec.shared_examples 'permission level for issue mutation is correctly verified' do |raises_for_all_errors = false| + before do + issue.assignees = [] + issue.author = user + end + + shared_examples_for 'when the user does not have access to the resource' do |raise_for_assigned| + it 'raises an error' do + expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable) + end + + context 'even if assigned to the issue' do + before do + issue.assignees.push(user) + end + + it 'does not modify issue' do + if raises_for_all_errors || raise_for_assigned + expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable) + else + expect(subject[:issue]).to eq issue + end + end + end + + context 'even if author of the issue' do + before do + issue.author = user + end + + it 'raises an error' do + expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable) + end + end + end + + context 'when the user is not a project member' do + it_behaves_like 'when the user does not have access to the resource', true + end + + context 'when the user is a project member' do + context 'with guest role' do + before do + issue.project.add_guest(user) + end + + it_behaves_like 'when the user does not have access to the resource', false + end + end +end diff --git a/spec/support/shared_examples/graphql/mutations/merge_requests/permission_check_shared_examples.rb b/spec/support/shared_examples/graphql/mutations/merge_requests/permission_check_shared_examples.rb new file mode 100644 index 00000000000..1ddbad1cea7 --- /dev/null +++ b/spec/support/shared_examples/graphql/mutations/merge_requests/permission_check_shared_examples.rb @@ -0,0 +1,73 @@ +# frozen_string_literal: true + +RSpec.shared_examples 'permission level for merge request mutation is correctly verified' do + before do + merge_request.assignees = [] + merge_request.reviewers = [] + merge_request.author = nil + end + + shared_examples_for 'when the user does not have access to the resource' do |raise_for_assigned| + it 'raises an error' do + expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable) + end + + context 'even if assigned to the merge request' do + before do + merge_request.assignees.push(user) + end + + it 'does not modify merge request' do + if raise_for_assigned + expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable) + else + # In some cases we simply do nothing instead of raising + # https://gitlab.com/gitlab-org/gitlab/-/issues/196241 + expect(subject[:merge_request]).to eq merge_request + end + end + end + + context 'even if reviewer of the merge request' do + before do + merge_request.reviewers.push(user) + end + + it 'raises an error' do + expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable) + end + end + + context 'even if author of the merge request' do + before do + merge_request.author = user + end + + it 'raises an error' do + expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable) + end + end + end + + context 'when the user is not a project member' do + it_behaves_like 'when the user does not have access to the resource', true + end + + context 'when the user is a project member' do + context 'with guest role' do + before do + merge_request.project.add_guest(user) + end + + it_behaves_like 'when the user does not have access to the resource', true + end + + context 'with reporter role' do + before do + merge_request.project.add_reporter(user) + end + + it_behaves_like 'when the user does not have access to the resource', false + end + end +end diff --git a/spec/support/shared_examples/lib/gitlab/sidekiq_middleware/strategy_shared_examples.rb b/spec/support/shared_examples/lib/gitlab/sidekiq_middleware/strategy_shared_examples.rb new file mode 100644 index 00000000000..2936bb354cf --- /dev/null +++ b/spec/support/shared_examples/lib/gitlab/sidekiq_middleware/strategy_shared_examples.rb @@ -0,0 +1,139 @@ +# frozen_string_literal: true + +RSpec.shared_examples 'deduplicating jobs when scheduling' do |strategy_name| + let(:fake_duplicate_job) do + instance_double(Gitlab::SidekiqMiddleware::DuplicateJobs::DuplicateJob) + end + + let(:expected_message) { "dropped #{strategy_name.to_s.humanize.downcase}" } + + subject(:strategy) { Gitlab::SidekiqMiddleware::DuplicateJobs::Strategies.for(strategy_name).new(fake_duplicate_job) } + + describe '#schedule' do + before do + allow(Gitlab::SidekiqLogging::DeduplicationLogger.instance).to receive(:log) + end + + it 'checks for duplicates before yielding' do + expect(fake_duplicate_job).to receive(:scheduled?).twice.ordered.and_return(false) + expect(fake_duplicate_job).to( + receive(:check!) + .with(Gitlab::SidekiqMiddleware::DuplicateJobs::DuplicateJob::DUPLICATE_KEY_TTL) + .ordered + .and_return('a jid')) + expect(fake_duplicate_job).to receive(:duplicate?).ordered.and_return(false) + + expect { |b| strategy.schedule({}, &b) }.to yield_control + end + + it 'checks worker options for scheduled jobs' do + expect(fake_duplicate_job).to receive(:scheduled?).ordered.and_return(true) + expect(fake_duplicate_job).to receive(:options).ordered.and_return({}) + expect(fake_duplicate_job).not_to receive(:check!) + + expect { |b| strategy.schedule({}, &b) }.to yield_control + end + + context 'job marking' do + it 'adds the jid of the existing job to the job hash' do + allow(fake_duplicate_job).to receive(:scheduled?).and_return(false) + allow(fake_duplicate_job).to receive(:check!).and_return('the jid') + allow(fake_duplicate_job).to receive(:droppable?).and_return(true) + allow(fake_duplicate_job).to receive(:options).and_return({}) + job_hash = {} + + expect(fake_duplicate_job).to receive(:duplicate?).and_return(true) + expect(fake_duplicate_job).to receive(:existing_jid).and_return('the jid') + + strategy.schedule(job_hash) {} + + expect(job_hash).to include('duplicate-of' => 'the jid') + end + + context 'scheduled jobs' do + let(:time_diff) { 1.minute } + + context 'scheduled in the past' do + it 'adds the jid of the existing job to the job hash' do + allow(fake_duplicate_job).to receive(:scheduled?).twice.and_return(true) + allow(fake_duplicate_job).to receive(:scheduled_at).and_return(Time.now - time_diff) + allow(fake_duplicate_job).to receive(:options).and_return({ including_scheduled: true }) + allow(fake_duplicate_job).to( + receive(:check!) + .with(Gitlab::SidekiqMiddleware::DuplicateJobs::DuplicateJob::DUPLICATE_KEY_TTL) + .and_return('the jid')) + allow(fake_duplicate_job).to receive(:droppable?).and_return(true) + job_hash = {} + + expect(fake_duplicate_job).to receive(:duplicate?).and_return(true) + expect(fake_duplicate_job).to receive(:existing_jid).and_return('the jid') + + strategy.schedule(job_hash) {} + + expect(job_hash).to include('duplicate-of' => 'the jid') + end + end + + context 'scheduled in the future' do + it 'adds the jid of the existing job to the job hash' do + freeze_time do + allow(fake_duplicate_job).to receive(:scheduled?).twice.and_return(true) + allow(fake_duplicate_job).to receive(:scheduled_at).and_return(Time.now + time_diff) + allow(fake_duplicate_job).to receive(:options).and_return({ including_scheduled: true }) + allow(fake_duplicate_job).to( + receive(:check!).with(time_diff.to_i).and_return('the jid')) + allow(fake_duplicate_job).to receive(:droppable?).and_return(true) + job_hash = {} + + expect(fake_duplicate_job).to receive(:duplicate?).and_return(true) + expect(fake_duplicate_job).to receive(:existing_jid).and_return('the jid') + + strategy.schedule(job_hash) {} + + expect(job_hash).to include('duplicate-of' => 'the jid') + end + end + end + end + end + + context "when the job is droppable" do + before do + allow(fake_duplicate_job).to receive(:scheduled?).and_return(false) + allow(fake_duplicate_job).to receive(:check!).and_return('the jid') + allow(fake_duplicate_job).to receive(:duplicate?).and_return(true) + allow(fake_duplicate_job).to receive(:options).and_return({}) + allow(fake_duplicate_job).to receive(:existing_jid).and_return('the jid') + allow(fake_duplicate_job).to receive(:droppable?).and_return(true) + end + + it 'drops the job' do + schedule_result = nil + + expect(fake_duplicate_job).to receive(:droppable?).and_return(true) + + expect { |b| schedule_result = strategy.schedule({}, &b) }.not_to yield_control + expect(schedule_result).to be(false) + end + + it 'logs that the job was dropped' do + fake_logger = instance_double(Gitlab::SidekiqLogging::DeduplicationLogger) + + expect(Gitlab::SidekiqLogging::DeduplicationLogger).to receive(:instance).and_return(fake_logger) + expect(fake_logger).to receive(:log).with(a_hash_including({ 'jid' => 'new jid' }), expected_message, {}) + + strategy.schedule({ 'jid' => 'new jid' }) {} + end + + it 'logs the deduplication options of the worker' do + fake_logger = instance_double(Gitlab::SidekiqLogging::DeduplicationLogger) + + expect(Gitlab::SidekiqLogging::DeduplicationLogger).to receive(:instance).and_return(fake_logger) + allow(fake_duplicate_job).to receive(:options).and_return({ foo: :bar }) + expect(fake_logger).to receive(:log).with(a_hash_including({ 'jid' => 'new jid' }), expected_message, { foo: :bar }) + + strategy.schedule({ 'jid' => 'new jid' }) {} + end + end + end +end diff --git a/spec/workers/container_expiration_policies/cleanup_container_repository_worker_spec.rb b/spec/workers/container_expiration_policies/cleanup_container_repository_worker_spec.rb new file mode 100644 index 00000000000..4604a6b1196 --- /dev/null +++ b/spec/workers/container_expiration_policies/cleanup_container_repository_worker_spec.rb @@ -0,0 +1,228 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ContainerExpirationPolicies::CleanupContainerRepositoryWorker do + let_it_be(:repository, reload: true) { create(:container_repository, :cleanup_scheduled) } + let_it_be(:project) { repository.project } + let_it_be(:policy) { project.container_expiration_policy } + let_it_be(:other_repository) { create(:container_repository) } + + let(:worker) { described_class.new } + + describe '#perform_work' do + subject { worker.perform_work } + + RSpec.shared_examples 'handling all repository conditions' do + it 'sends the repository for cleaning' do + expect(ContainerExpirationPolicies::CleanupService) + .to receive(:new).with(repository).and_return(double(execute: cleanup_service_response(repository: repository))) + expect(worker).to receive(:log_extra_metadata_on_done).with(:cleanup_status, :finished) + expect(worker).to receive(:log_extra_metadata_on_done).with(:container_repository_id, repository.id) + + subject + end + + context 'with unfinished cleanup' do + it 'logs an unfinished cleanup' do + expect(ContainerExpirationPolicies::CleanupService) + .to receive(:new).with(repository).and_return(double(execute: cleanup_service_response(status: :unfinished, repository: repository))) + expect(worker).to receive(:log_extra_metadata_on_done).with(:cleanup_status, :unfinished) + expect(worker).to receive(:log_extra_metadata_on_done).with(:container_repository_id, repository.id) + + subject + end + end + + context 'with policy running shortly' do + before do + repository.project + .container_expiration_policy + .update_column(:next_run_at, 1.minute.from_now) + end + + it 'skips the repository' do + expect(ContainerExpirationPolicies::CleanupService).not_to receive(:new) + + expect { subject }.to change { ContainerRepository.waiting_for_cleanup.count }.from(1).to(0) + expect(repository.reload.cleanup_unscheduled?).to be_truthy + end + end + + context 'with disabled policy' do + before do + repository.project + .container_expiration_policy + .disable! + end + + it 'skips the repository' do + expect(ContainerExpirationPolicies::CleanupService).not_to receive(:new) + + expect { subject }.to change { ContainerRepository.waiting_for_cleanup.count }.from(1).to(0) + expect(repository.reload.cleanup_unscheduled?).to be_truthy + end + end + end + + context 'with repository in cleanup scheduled state' do + it_behaves_like 'handling all repository conditions' + end + + context 'with repository in cleanup unfinished state' do + before do + repository.cleanup_unfinished! + end + + it_behaves_like 'handling all repository conditions' + end + + context 'with another repository in cleanup unfinished state' do + let_it_be(:another_repository) { create(:container_repository, :cleanup_unfinished) } + + it 'process the cleanup scheduled repository first' do + expect(ContainerExpirationPolicies::CleanupService) + .to receive(:new).with(repository).and_return(double(execute: cleanup_service_response(repository: repository))) + expect(worker).to receive(:log_extra_metadata_on_done).with(:cleanup_status, :finished) + expect(worker).to receive(:log_extra_metadata_on_done).with(:container_repository_id, repository.id) + + subject + end + end + + context 'with multiple repositories in cleanup unfinished state' do + let_it_be(:repository2) { create(:container_repository, :cleanup_unfinished, expiration_policy_started_at: 20.minutes.ago) } + let_it_be(:repository3) { create(:container_repository, :cleanup_unfinished, expiration_policy_started_at: 10.minutes.ago) } + + before do + repository.update!(expiration_policy_cleanup_status: :cleanup_unfinished, expiration_policy_started_at: 30.minutes.ago) + end + + it 'process the repository with the oldest expiration_policy_started_at' do + expect(ContainerExpirationPolicies::CleanupService) + .to receive(:new).with(repository).and_return(double(execute: cleanup_service_response(repository: repository))) + expect(worker).to receive(:log_extra_metadata_on_done).with(:cleanup_status, :finished) + expect(worker).to receive(:log_extra_metadata_on_done).with(:container_repository_id, repository.id) + + subject + end + end + + context 'with repository in cleanup ongoing state' do + before do + repository.cleanup_ongoing! + end + + it 'does not process it' do + expect(Projects::ContainerRepository::CleanupTagsService).not_to receive(:new) + + expect { subject }.not_to change { ContainerRepository.waiting_for_cleanup.count } + expect(repository.cleanup_ongoing?).to be_truthy + end + end + + context 'with no repository in any cleanup state' do + before do + repository.cleanup_unscheduled! + end + + it 'does not process it' do + expect(Projects::ContainerRepository::CleanupTagsService).not_to receive(:new) + + expect { subject }.not_to change { ContainerRepository.waiting_for_cleanup.count } + expect(repository.cleanup_unscheduled?).to be_truthy + end + end + + context 'with no container repository waiting' do + before do + repository.destroy! + end + + it 'does not execute the cleanup tags service' do + expect(Projects::ContainerRepository::CleanupTagsService).not_to receive(:new) + + expect { subject }.not_to change { ContainerRepository.waiting_for_cleanup.count } + end + end + + context 'with feature flag disabled' do + before do + stub_feature_flags(container_registry_expiration_policies_throttling: false) + end + + it 'is a no-op' do + expect(Projects::ContainerRepository::CleanupTagsService).not_to receive(:new) + + expect { subject }.not_to change { ContainerRepository.waiting_for_cleanup.count } + end + end + + def cleanup_service_response(status: :finished, repository:) + ServiceResponse.success(message: "cleanup #{status}", payload: { cleanup_status: status, container_repository_id: repository.id }) + end + end + + describe '#remaining_work_count' do + subject { worker.remaining_work_count } + + context 'with container repositoires waiting for cleanup' do + let_it_be(:unfinished_repositories) { create_list(:container_repository, 2, :cleanup_unfinished) } + + it { is_expected.to eq(3) } + + it 'logs the work count' do + expect_log_info( + cleanup_scheduled_count: 1, + cleanup_unfinished_count: 2, + cleanup_total_count: 3 + ) + + subject + end + end + + context 'with no container repositories waiting for cleanup' do + before do + repository.cleanup_ongoing! + end + + it { is_expected.to eq(0) } + + it 'logs 0 work count' do + expect_log_info( + cleanup_scheduled_count: 0, + cleanup_unfinished_count: 0, + cleanup_total_count: 0 + ) + + subject + end + end + end + + describe '#max_running_jobs' do + let(:capacity) { 50 } + + subject { worker.max_running_jobs } + + before do + stub_application_setting(container_registry_expiration_policies_worker_capacity: capacity) + end + + it { is_expected.to eq(capacity) } + + context 'with feature flag disabled' do + before do + stub_feature_flags(container_registry_expiration_policies_throttling: false) + end + + it { is_expected.to eq(0) } + end + end + + def expect_log_info(structure) + expect(worker.logger) + .to receive(:info).with(worker.structured_payload(structure)) + end +end diff --git a/spec/workers/container_expiration_policy_worker_spec.rb b/spec/workers/container_expiration_policy_worker_spec.rb index 6b185c30670..9ac845344cc 100644 --- a/spec/workers/container_expiration_policy_worker_spec.rb +++ b/spec/workers/container_expiration_policy_worker_spec.rb @@ -5,71 +5,152 @@ require 'spec_helper' RSpec.describe ContainerExpirationPolicyWorker do include ExclusiveLeaseHelpers - subject { described_class.new.perform } + let(:worker) { described_class.new } + let(:started_at) { nil } - RSpec.shared_examples 'not executing any policy' do - it 'does not run any policy' do - expect(ContainerExpirationPolicyService).not_to receive(:new) + describe '#perform' do + subject { worker.perform } - subject + RSpec.shared_examples 'not executing any policy' do + it 'does not run any policy' do + expect(ContainerExpirationPolicyService).not_to receive(:new) + + subject + end end - end - context 'With no container expiration policies' do - it_behaves_like 'not executing any policy' - end + context 'With no container expiration policies' do + it 'does not execute any policies' do + expect(ContainerRepository).not_to receive(:for_project_id) - context 'With container expiration policies' do - let_it_be(:container_expiration_policy, reload: true) { create(:container_expiration_policy, :runnable) } - let_it_be(:container_repository) { create(:container_repository, project: container_expiration_policy.project) } - let_it_be(:user) { container_expiration_policy.project.owner } + expect { subject }.not_to change { ContainerRepository.cleanup_scheduled.count } + end + end - context 'a valid policy' do - it 'runs the policy' do - service = instance_double(ContainerExpirationPolicyService, execute: true) + context 'with container expiration policies' do + let_it_be(:container_expiration_policy) { create(:container_expiration_policy, :runnable) } + let_it_be(:container_repository) { create(:container_repository, project: container_expiration_policy.project) } - expect(ContainerExpirationPolicyService) - .to receive(:new).with(container_expiration_policy.project, user).and_return(service) + context 'with a valid container expiration policy' do + it 'schedules the next run' do + expect { subject }.to change { container_expiration_policy.reload.next_run_at } + end - subject - end - end + it 'marks the container repository as scheduled for cleanup' do + expect { subject }.to change { container_repository.reload.cleanup_scheduled? }.from(false).to(true) + expect(ContainerRepository.cleanup_scheduled.count).to eq(1) + end - context 'a disabled policy' do - before do - container_expiration_policy.disable! + it 'calls the limited capacity worker' do + expect(ContainerExpirationPolicies::CleanupContainerRepositoryWorker).to receive(:perform_with_capacity) + + subject + end end - it_behaves_like 'not executing any policy' - end + context 'with a disabled container expiration policy' do + before do + container_expiration_policy.disable! + end - context 'a policy that is not due for a run' do - before do - container_expiration_policy.update_column(:next_run_at, 2.minutes.from_now) + it 'does not run the policy' do + expect(ContainerRepository).not_to receive(:for_project_id) + + expect { subject }.not_to change { ContainerRepository.cleanup_scheduled.count } + end end - it_behaves_like 'not executing any policy' + context 'with an invalid container expiration policy' do + let(:user) { container_expiration_policy.project.owner } + + before do + container_expiration_policy.update_column(:name_regex, '*production') + end + + it 'disables the policy and tracks an error' do + expect(ContainerRepository).not_to receive(:for_project_id) + expect(Gitlab::ErrorTracking).to receive(:log_exception).with(instance_of(described_class::InvalidPolicyError), container_expiration_policy_id: container_expiration_policy.id) + + expect { subject }.to change { container_expiration_policy.reload.enabled }.from(true).to(false) + expect(ContainerRepository.cleanup_scheduled).to be_empty + end + end end - context 'a policy linked to no container repository' do + context 'with exclusive lease taken' do before do - container_expiration_policy.container_repositories.delete_all + stub_exclusive_lease_taken(worker.lease_key, timeout: 5.hours) end - it_behaves_like 'not executing any policy' + it 'does not execute any policy' do + expect(ContainerExpirationPolicies::CleanupContainerRepositoryWorker).not_to receive(:perform_with_capacity) + expect(worker).not_to receive(:runnable_policies) + + expect { subject }.not_to change { ContainerRepository.cleanup_scheduled.count } + end end - context 'an invalid policy' do + context 'with throttling disabled' do before do - container_expiration_policy.update_column(:name_regex, '*production') + stub_feature_flags(container_registry_expiration_policies_throttling: false) end - it 'runs the policy and tracks an error' do - expect(ContainerExpirationPolicyService) - .to receive(:new).with(container_expiration_policy.project, user).and_call_original - expect(Gitlab::ErrorTracking).to receive(:log_exception).with(instance_of(ContainerExpirationPolicyService::InvalidPolicyError), container_expiration_policy_id: container_expiration_policy.id) + context 'with no container expiration policies' do + it_behaves_like 'not executing any policy' + end + + context 'with container expiration policies' do + let_it_be(:container_expiration_policy, reload: true) { create(:container_expiration_policy, :runnable) } + let_it_be(:container_repository) { create(:container_repository, project: container_expiration_policy.project) } + let_it_be(:user) { container_expiration_policy.project.owner } + + context 'a valid policy' do + it 'runs the policy' do + service = instance_double(ContainerExpirationPolicyService, execute: true) + + expect(ContainerExpirationPolicyService) + .to receive(:new).with(container_expiration_policy.project, user).and_return(service) + + subject + end + end + + context 'a disabled policy' do + before do + container_expiration_policy.disable! + end + + it_behaves_like 'not executing any policy' + end + + context 'a policy that is not due for a run' do + before do + container_expiration_policy.update_column(:next_run_at, 2.minutes.from_now) + end + + it_behaves_like 'not executing any policy' + end + + context 'a policy linked to no container repository' do + before do + container_expiration_policy.container_repositories.delete_all + end + + it_behaves_like 'not executing any policy' + end + + context 'an invalid policy' do + before do + container_expiration_policy.update_column(:name_regex, '*production') + end + + it 'disables the policy and tracks an error' do + expect(ContainerExpirationPolicyService).not_to receive(:new).with(container_expiration_policy, user) + expect(Gitlab::ErrorTracking).to receive(:log_exception).with(instance_of(described_class::InvalidPolicyError), container_expiration_policy_id: container_expiration_policy.id) - expect { subject }.to change { container_expiration_policy.reload.enabled }.from(true).to(false) + expect { subject }.to change { container_expiration_policy.reload.enabled }.from(true).to(false) + end + end end end end |