diff options
28 files changed, 694 insertions, 124 deletions
diff --git a/app/models/project.rb b/app/models/project.rb index c3136cde4c5..815124360e2 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -324,6 +324,8 @@ class Project < ApplicationRecord has_many :daily_report_results, class_name: 'Ci::DailyReportResult' + has_many :repository_storage_moves, class_name: 'ProjectRepositoryStorageMove' + accepts_nested_attributes_for :variables, allow_destroy: true accepts_nested_attributes_for :project_feature, update_only: true accepts_nested_attributes_for :import_data @@ -2073,7 +2075,12 @@ class Project < ApplicationRecord raise ArgumentError unless ::Gitlab.config.repositories.storages.key?(new_repository_storage_key) - run_after_commit { ProjectUpdateRepositoryStorageWorker.perform_async(id, new_repository_storage_key) } + storage_move = repository_storage_moves.create!( + source_storage_name: repository_storage, + destination_storage_name: new_repository_storage_key + ) + storage_move.schedule! + self.repository_read_only = true end diff --git a/app/models/project_repository_storage_move.rb b/app/models/project_repository_storage_move.rb new file mode 100644 index 00000000000..90da70120f8 --- /dev/null +++ b/app/models/project_repository_storage_move.rb @@ -0,0 +1,54 @@ +# frozen_string_literal: true + +# ProjectRepositoryStorageMove are details of repository storage moves for a +# project. For example, moving a project to another gitaly node to help +# balance storage capacity. +class ProjectRepositoryStorageMove < ApplicationRecord + include AfterCommitQueue + + belongs_to :project, inverse_of: :repository_storage_moves + + validates :project, presence: true + validates :state, presence: true + validates :source_storage_name, + on: :create, + presence: true, + inclusion: { in: ->(_) { Gitlab.config.repositories.storages.keys } } + validates :destination_storage_name, + on: :create, + presence: true, + inclusion: { in: ->(_) { Gitlab.config.repositories.storages.keys } } + + state_machine initial: :initial do + event :schedule do + transition initial: :scheduled + end + + event :start do + transition scheduled: :started + end + + event :finish do + transition started: :finished + end + + event :do_fail do + transition [:initial, :scheduled, :started] => :failed + end + + after_transition initial: :scheduled do |storage_move, _| + storage_move.run_after_commit do + ProjectUpdateRepositoryStorageWorker.perform_async( + storage_move.project_id, storage_move.destination_storage_name, + repository_storage_move_id: storage_move.id + ) + end + end + + state :initial, value: 1 + state :scheduled, value: 2 + state :started, value: 3 + state :finished, value: 4 + state :failed, value: 5 + end +end diff --git a/app/services/projects/update_repository_storage_service.rb b/app/services/projects/update_repository_storage_service.rb index 65af112124e..21cd84117d7 100644 --- a/app/services/projects/update_repository_storage_service.rb +++ b/app/services/projects/update_repository_storage_service.rb @@ -1,37 +1,49 @@ # frozen_string_literal: true module Projects - class UpdateRepositoryStorageService < BaseService - include Gitlab::ShellAdapter - + class UpdateRepositoryStorageService Error = Class.new(StandardError) SameFilesystemError = Class.new(Error) - def initialize(project) - @project = project + attr_reader :repository_storage_move + delegate :project, :destination_storage_name, to: :repository_storage_move + delegate :repository, to: :project + + def initialize(repository_storage_move) + @repository_storage_move = repository_storage_move end - def execute(new_repository_storage_key) - raise SameFilesystemError if same_filesystem?(project.repository.storage, new_repository_storage_key) + def execute + repository_storage_move.start! + + raise SameFilesystemError if same_filesystem?(repository.storage, destination_storage_name) - mirror_repositories(new_repository_storage_key) + mirror_repositories - mark_old_paths_for_archive + project.transaction do + mark_old_paths_for_archive - project.update!(repository_storage: new_repository_storage_key, repository_read_only: false) - project.leave_pool_repository - project.track_project_repository + repository_storage_move.finish! + project.update!(repository_storage: destination_storage_name, repository_read_only: false) + project.leave_pool_repository + project.track_project_repository + end enqueue_housekeeping - success + ServiceResponse.success rescue StandardError => e - project.update!(repository_read_only: false) + project.transaction do + repository_storage_move.do_fail! + project.update!(repository_read_only: false) + end Gitlab::ErrorTracking.track_exception(e, project_path: project.full_path) - error(s_("UpdateRepositoryStorage|Error moving repository storage for %{project_full_path} - %{message}") % { project_full_path: project.full_path, message: e.message }) + ServiceResponse.error( + message: s_("UpdateRepositoryStorage|Error moving repository storage for %{project_full_path} - %{message}") % { project_full_path: project.full_path, message: e.message } + ) end private @@ -40,15 +52,15 @@ module Projects Gitlab::GitalyClient.filesystem_id(old_storage) == Gitlab::GitalyClient.filesystem_id(new_storage) end - def mirror_repositories(new_repository_storage_key) - mirror_repository(new_repository_storage_key) + def mirror_repositories + mirror_repository if project.wiki.repository_exists? - mirror_repository(new_repository_storage_key, type: Gitlab::GlRepository::WIKI) + mirror_repository(type: Gitlab::GlRepository::WIKI) end end - def mirror_repository(new_storage_key, type: Gitlab::GlRepository::PROJECT) + def mirror_repository(type: Gitlab::GlRepository::PROJECT) unless wait_for_pushes(type) raise Error, s_('UpdateRepositoryStorage|Timeout waiting for %{type} repository pushes') % { type: type.name } end @@ -60,7 +72,7 @@ module Projects # Initialize a git repository on the target path new_repository = Gitlab::Git::Repository.new( - new_storage_key, + destination_storage_name, raw_repository.relative_path, raw_repository.gl_repository, full_path diff --git a/app/workers/project_update_repository_storage_worker.rb b/app/workers/project_update_repository_storage_worker.rb index ecee33e6421..7de75c6a46c 100644 --- a/app/workers/project_update_repository_storage_worker.rb +++ b/app/workers/project_update_repository_storage_worker.rb @@ -5,9 +5,19 @@ class ProjectUpdateRepositoryStorageWorker # rubocop:disable Scalability/Idempot feature_category :gitaly - def perform(project_id, new_repository_storage_key) - project = Project.find(project_id) + def perform(project_id, new_repository_storage_key, repository_storage_move_id: nil) + repository_storage_move = + if repository_storage_move_id + ProjectRepositoryStorageMove.find(repository_storage_move_id) + else + # maintain compatibility with workers queued before release + project = Project.find(project_id) + project.repository_storage_moves.create!( + source_storage_name: project.repository_storage, + destination_storage_name: new_repository_storage_key + ) + end - ::Projects::UpdateRepositoryStorageService.new(project).execute(new_repository_storage_key) + ::Projects::UpdateRepositoryStorageService.new(repository_storage_move).execute end end diff --git a/changelogs/unreleased/ajk-fix-extra-sleep-exclusive-lease.yml b/changelogs/unreleased/ajk-fix-extra-sleep-exclusive-lease.yml new file mode 100644 index 00000000000..c57a1e5fd87 --- /dev/null +++ b/changelogs/unreleased/ajk-fix-extra-sleep-exclusive-lease.yml @@ -0,0 +1,5 @@ +--- +title: Remove extra sleep when obtaining exclusive lease +merge_request: 30654 +author: +type: fixed diff --git a/changelogs/unreleased/initial_repository_storage_move.yml b/changelogs/unreleased/initial_repository_storage_move.yml new file mode 100644 index 00000000000..376f3361c49 --- /dev/null +++ b/changelogs/unreleased/initial_repository_storage_move.yml @@ -0,0 +1,5 @@ +--- +title: Store status of repository storage moves +merge_request: 29095 +author: +type: changed diff --git a/db/migrate/20200407222647_create_project_repository_storage_moves.rb b/db/migrate/20200407222647_create_project_repository_storage_moves.rb new file mode 100644 index 00000000000..402a1cdd4a6 --- /dev/null +++ b/db/migrate/20200407222647_create_project_repository_storage_moves.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +class CreateProjectRepositoryStorageMoves < ActiveRecord::Migration[6.0] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + create_table :project_repository_storage_moves do |t| + t.timestamps_with_timezone + t.integer :project_id, limit: 8, null: false + t.integer :state, limit: 2, default: 1, null: false + t.text :source_storage_name, null: false + t.text :destination_storage_name, null: false + end + + add_index :project_repository_storage_moves, :project_id + + add_text_limit(:project_repository_storage_moves, :source_storage_name, 255, constraint_name: 'project_repository_storage_moves_source_storage_name') + add_text_limit(:project_repository_storage_moves, :destination_storage_name, 255, constraint_name: 'project_repository_storage_moves_destination_storage_name') + end + + def down + remove_check_constraint(:project_repository_storage_moves, 'project_repository_storage_moves_source_storage_name') + remove_check_constraint(:project_repository_storage_moves, 'project_repository_storage_moves_destination_storage_name') + + drop_table :project_repository_storage_moves + end +end diff --git a/db/migrate/20200429015603_add_fk_to_project_repository_storage_moves.rb b/db/migrate/20200429015603_add_fk_to_project_repository_storage_moves.rb new file mode 100644 index 00000000000..a68ce66e4a6 --- /dev/null +++ b/db/migrate/20200429015603_add_fk_to_project_repository_storage_moves.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +class AddFkToProjectRepositoryStorageMoves < ActiveRecord::Migration[6.0] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def up + with_lock_retries do + add_foreign_key :project_repository_storage_moves, :projects, on_delete: :cascade # rubocop:disable Migration/AddConcurrentForeignKey + end + end + + def down + with_lock_retries do + remove_foreign_key :project_repository_storage_moves, :projects + end + end +end diff --git a/db/structure.sql b/db/structure.sql index db3cc324d4e..b5bc2d0d361 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -5171,6 +5171,27 @@ CREATE SEQUENCE public.project_repository_states_id_seq ALTER SEQUENCE public.project_repository_states_id_seq OWNED BY public.project_repository_states.id; +CREATE TABLE public.project_repository_storage_moves ( + id bigint NOT NULL, + created_at timestamp with time zone NOT NULL, + updated_at timestamp with time zone NOT NULL, + project_id bigint NOT NULL, + state smallint DEFAULT 1 NOT NULL, + source_storage_name text NOT NULL, + destination_storage_name text NOT NULL, + CONSTRAINT project_repository_storage_moves_destination_storage_name CHECK ((char_length(destination_storage_name) <= 255)), + CONSTRAINT project_repository_storage_moves_source_storage_name CHECK ((char_length(source_storage_name) <= 255)) +); + +CREATE SEQUENCE public.project_repository_storage_moves_id_seq + START WITH 1 + INCREMENT BY 1 + NO MINVALUE + NO MAXVALUE + CACHE 1; + +ALTER SEQUENCE public.project_repository_storage_moves_id_seq OWNED BY public.project_repository_storage_moves.id; + CREATE TABLE public.project_settings ( project_id integer NOT NULL, created_at timestamp with time zone NOT NULL, @@ -7640,6 +7661,8 @@ ALTER TABLE ONLY public.project_repositories ALTER COLUMN id SET DEFAULT nextval ALTER TABLE ONLY public.project_repository_states ALTER COLUMN id SET DEFAULT nextval('public.project_repository_states_id_seq'::regclass); +ALTER TABLE ONLY public.project_repository_storage_moves ALTER COLUMN id SET DEFAULT nextval('public.project_repository_storage_moves_id_seq'::regclass); + ALTER TABLE ONLY public.project_statistics ALTER COLUMN id SET DEFAULT nextval('public.project_statistics_id_seq'::regclass); ALTER TABLE ONLY public.project_tracing_settings ALTER COLUMN id SET DEFAULT nextval('public.project_tracing_settings_id_seq'::regclass); @@ -8525,6 +8548,9 @@ ALTER TABLE ONLY public.project_repositories ALTER TABLE ONLY public.project_repository_states ADD CONSTRAINT project_repository_states_pkey PRIMARY KEY (id); +ALTER TABLE ONLY public.project_repository_storage_moves + ADD CONSTRAINT project_repository_storage_moves_pkey PRIMARY KEY (id); + ALTER TABLE ONLY public.project_settings ADD CONSTRAINT project_settings_pkey PRIMARY KEY (project_id); @@ -10188,6 +10214,8 @@ CREATE INDEX index_project_repositories_on_shard_id ON public.project_repositori CREATE UNIQUE INDEX index_project_repository_states_on_project_id ON public.project_repository_states USING btree (project_id); +CREATE INDEX index_project_repository_storage_moves_on_project_id ON public.project_repository_storage_moves USING btree (project_id); + CREATE UNIQUE INDEX index_project_settings_on_push_rule_id ON public.project_settings USING btree (push_rule_id); CREATE INDEX index_project_statistics_on_namespace_id ON public.project_statistics USING btree (namespace_id); @@ -11774,6 +11802,9 @@ ALTER TABLE ONLY public.merge_request_diff_files ALTER TABLE ONLY public.status_page_settings ADD CONSTRAINT fk_rails_506e5ba391 FOREIGN KEY (project_id) REFERENCES public.projects(id) ON DELETE CASCADE; +ALTER TABLE ONLY public.project_repository_storage_moves + ADD CONSTRAINT fk_rails_5106dbd44a FOREIGN KEY (project_id) REFERENCES public.projects(id) ON DELETE CASCADE; + ALTER TABLE ONLY public.x509_commit_signatures ADD CONSTRAINT fk_rails_53fe41188f FOREIGN KEY (x509_certificate_id) REFERENCES public.x509_certificates(id) ON DELETE CASCADE; @@ -13575,6 +13606,7 @@ COPY "schema_migrations" (version) FROM STDIN; 20200407171133 20200407171417 20200407182205 +20200407222647 20200408110856 20200408133211 20200408153842 @@ -13638,5 +13670,6 @@ COPY "schema_migrations" (version) FROM STDIN; 20200424050250 20200424101920 20200427064130 +20200429015603 \. diff --git a/doc/user/application_security/dast/index.md b/doc/user/application_security/dast/index.md index cd4c0b5241f..42480e60e48 100644 --- a/doc/user/application_security/dast/index.md +++ b/doc/user/application_security/dast/index.md @@ -557,13 +557,8 @@ For details on saving and transporting Docker images as a file, see Docker's doc dast: image: registry.example.com/namespace/dast:latest - script: - - export DAST_WEBSITE=${DAST_WEBSITE:-$(cat environment_url.txt)} - - /analyze -t $DAST_WEBSITE --auto-update-addons false ``` -The option `--auto-update-addons false` instructs ZAP not to update add-ons. - ## Reports The DAST job can emit various reports. diff --git a/doc/user/incident_management/index.md b/doc/user/incident_management/index.md index 8505afb375e..8b898a7b516 100644 --- a/doc/user/incident_management/index.md +++ b/doc/user/incident_management/index.md @@ -48,6 +48,10 @@ Prometheus alerts can be set up in both: - [GitLab-managed Prometheus](../project/integrations/prometheus.md#setting-up-alerts-for-prometheus-metrics) and - [Self-managed Prometheus](../project/integrations/prometheus.md#external-prometheus-instances) installations. +#### Alert Bot user + +Behind the scenes, Prometheus alerts are created by the special Alert Bot user creating issues. This user cannot be removed but does not count toward the license limit count. + ### Alert endpoint GitLab can accept alerts from any source via a generic webhook receiver. When diff --git a/lib/gitlab/exclusive_lease.rb b/lib/gitlab/exclusive_lease.rb index 425ef5d738a..1d2c1c69423 100644 --- a/lib/gitlab/exclusive_lease.rb +++ b/lib/gitlab/exclusive_lease.rb @@ -12,6 +12,9 @@ module Gitlab # ExclusiveLease. # class ExclusiveLease + PREFIX = 'gitlab:exclusive_lease' + NoKey = Class.new(ArgumentError) + LUA_CANCEL_SCRIPT = <<~EOS.freeze local key, uuid = KEYS[1], ARGV[1] if redis.call("get", key) == uuid then @@ -34,13 +37,21 @@ module Gitlab end def self.cancel(key, uuid) + return unless key.present? + Gitlab::Redis::SharedState.with do |redis| - redis.eval(LUA_CANCEL_SCRIPT, keys: [redis_shared_state_key(key)], argv: [uuid]) + redis.eval(LUA_CANCEL_SCRIPT, keys: [ensure_prefixed_key(key)], argv: [uuid]) end end def self.redis_shared_state_key(key) - "gitlab:exclusive_lease:#{key}" + "#{PREFIX}:#{key}" + end + + def self.ensure_prefixed_key(key) + raise NoKey unless key.present? + + key.start_with?(PREFIX) ? key : redis_shared_state_key(key) end # Removes any existing exclusive_lease from redis @@ -94,6 +105,11 @@ module Gitlab ttl if ttl.positive? end end + + # Gives up this lease, allowing it to be obtained by others. + def cancel + self.class.cancel(@redis_shared_state_key, @uuid) + end end end diff --git a/lib/gitlab/exclusive_lease_helpers.rb b/lib/gitlab/exclusive_lease_helpers.rb index a1504833039..10762d83588 100644 --- a/lib/gitlab/exclusive_lease_helpers.rb +++ b/lib/gitlab/exclusive_lease_helpers.rb @@ -6,33 +6,38 @@ module Gitlab FailedToObtainLockError = Class.new(StandardError) ## - # This helper method blocks a process/thread until the other process cancel the obrainted lease key. + # This helper method blocks a process/thread until the lease can be acquired, either due to + # the lease TTL expiring, or due to the current holder explicitly releasing + # their hold. # - # Note: It's basically discouraged to use this method in the unicorn's thread, - # because it holds the connection until all `retries` is consumed. + # If the lease cannot be obtained, raises `FailedToObtainLockError`. + # + # @param [String] key The lock the thread will try to acquire. Only one thread + # in one process across all Rails instances can hold this named lock at any + # one time. + # @param [Float] ttl: The length of time the lock will be valid for. The lock + # will be automatically be released after this time, so any work should be + # completed within this time. + # @param [Integer] retries: The maximum number of times we will re-attempt + # to acquire the lock. The maximum number of attempts will be `retries + 1`: + # one for the initial attempt, and then one for every re-try. + # @param [Float|Proc] sleep_sec: Either a number of seconds to sleep, or + # a proc that computes the sleep time given the number of preceding attempts + # (from 1 to retries - 1) + # + # Note: It's basically discouraged to use this method in a unicorn thread, + # because this ties up all thread related resources until all `retries` are consumed. # This could potentially eat up all connection pools. def in_lock(key, ttl: 1.minute, retries: 10, sleep_sec: 0.01.seconds) raise ArgumentError, 'Key needs to be specified' unless key - lease = Gitlab::ExclusiveLease.new(key, timeout: ttl) - retried = false - max_attempts = 1 + retries - - until uuid = lease.try_obtain - # Keep trying until we obtain the lease. To prevent hammering Redis too - # much we'll wait for a bit. - attempt_number = max_attempts - retries - delay = sleep_sec.respond_to?(:call) ? sleep_sec.call(attempt_number) : sleep_sec - - sleep(delay) - (retries -= 1) < 0 ? break : retried ||= true - end + lease = SleepingLock.new(key, timeout: ttl, delay: sleep_sec) - raise FailedToObtainLockError, 'Failed to obtain a lock' unless uuid + lease.obtain(1 + retries) - yield(retried) + yield(lease.retried?) ensure - Gitlab::ExclusiveLease.cancel(key, uuid) + lease&.cancel end end end diff --git a/lib/gitlab/exclusive_lease_helpers/sleeping_lock.rb b/lib/gitlab/exclusive_lease_helpers/sleeping_lock.rb new file mode 100644 index 00000000000..8213c9bc042 --- /dev/null +++ b/lib/gitlab/exclusive_lease_helpers/sleeping_lock.rb @@ -0,0 +1,50 @@ +# frozen_string_literal: true + +module Gitlab + module ExclusiveLeaseHelpers + # Wrapper around ExclusiveLease that adds retry logic + class SleepingLock + delegate :cancel, to: :@lease + + def initialize(key, timeout:, delay:) + @lease = ::Gitlab::ExclusiveLease.new(key, timeout: timeout) + @delay = delay + @attempts = 0 + end + + def obtain(max_attempts) + until held? + raise FailedToObtainLockError, 'Failed to obtain a lock' if attempts >= max_attempts + + sleep(sleep_sec) unless first_attempt? + try_obtain + end + end + + def retried? + attempts > 1 + end + + private + + attr_reader :delay, :attempts + + def held? + @uuid.present? + end + + def try_obtain + @uuid ||= @lease.try_obtain + @attempts += 1 + end + + def first_attempt? + attempts.zero? + end + + def sleep_sec + delay.respond_to?(:call) ? delay.call(attempts) : delay + end + end + end +end diff --git a/qa/Rakefile b/qa/Rakefile index 9a960a60e56..9f547b92bb8 100644 --- a/qa/Rakefile +++ b/qa/Rakefile @@ -41,7 +41,13 @@ end desc "Generate data and run load tests" task generate_data_and_run_load_test: [:generate_perf_testdata, :run_artillery_load_tests] -desc "Deletes test ssh keys for a provided user" -task :delete_test_ssh_keys do - QA::Tools::DeleteTestSSHKeys.new.run +desc "Deletes test ssh keys a user" +task :delete_test_ssh_keys, [:title_portion, :delete_before] do |t, args| + QA::Tools::DeleteTestSSHKeys.new(args).run +end + +desc "Dry run of deleting test ssh keys for a user. Lists keys to be deleted" +task :delete_test_ssh_keys_dry_run, [:title_portion, :delete_before] do |t, args| + args.with_defaults(dry_run: true) + QA::Tools::DeleteTestSSHKeys.new(args).run end diff --git a/qa/qa/tools/delete_test_ssh_keys.rb b/qa/qa/tools/delete_test_ssh_keys.rb index f502542b529..953e9fc63d1 100644 --- a/qa/qa/tools/delete_test_ssh_keys.rb +++ b/qa/qa/tools/delete_test_ssh_keys.rb @@ -2,59 +2,78 @@ require_relative '../../qa' -# This script deletes all test ssh keys (with titles including 'key for ssh tests' or 'key for audit event test') of a user specified by ENV['GITLAB_USERNAME'] -# Required environment variables: GITLAB_QA_ACCESS_TOKEN, GITLAB_ADDRESS and GITLAB_USERNAME +# This script deletes all selected test ssh keys for a specific user +# Keys can be selected by a string matching part of the key's title and by created date +# - Specify `title_portion` to delete only keys that include the string provided +# - Specify `delete_before` to delete only keys that were created before the given date +# +# If `dry_run` is true the script will list the keys by title and indicate whether each will be deleted +# +# Required environment variables: GITLAB_QA_ACCESS_TOKEN and GITLAB_ADDRESS +# - GITLAB_QA_ACCESS_TOKEN should have API access and belong to the user whose keys will be deleted module QA module Tools class DeleteTestSSHKeys include Support::Api - def initialize + ITEMS_PER_PAGE = '100' + + def initialize(title_portion: 'E2E test key:', delete_before: Date.today.to_s, dry_run: false) raise ArgumentError, "Please provide GITLAB_ADDRESS" unless ENV['GITLAB_ADDRESS'] raise ArgumentError, "Please provide GITLAB_QA_ACCESS_TOKEN" unless ENV['GITLAB_QA_ACCESS_TOKEN'] - raise ArgumentError, "Please provide GITLAB_USERNAME" unless ENV['GITLAB_USERNAME'] @api_client = Runtime::API::Client.new(ENV['GITLAB_ADDRESS'], personal_access_token: ENV['GITLAB_QA_ACCESS_TOKEN']) - @username = ENV['GITLAB_USERNAME'] + @title_portion = title_portion + @delete_before = Date.parse(delete_before) + @dry_run = dry_run end def run STDOUT.puts 'Running...' - user_id = fetch_user_id - test_ssh_key_ids = fetch_test_ssh_key_ids(user_id) + keys_head_response = head Runtime::API::Request.new(@api_client, "/user/keys", per_page: ITEMS_PER_PAGE).url + total_pages = keys_head_response.headers[:x_total_pages] + + test_ssh_key_ids = fetch_test_ssh_key_ids(total_pages) STDOUT.puts "Number of test ssh keys to be deleted: #{test_ssh_key_ids.length}" - delete_ssh_keys(user_id, test_ssh_key_ids) unless test_ssh_key_ids.empty? + return if dry_run? + + delete_ssh_keys(test_ssh_key_ids) unless test_ssh_key_ids.empty? STDOUT.puts "\nDone" end private - def fetch_user_id - get_user_response = get Runtime::API::Request.new(@api_client, "/users?username=#{@username}").url - user = JSON.parse(get_user_response.body).first - raise "Unexpected user found. Expected #{@username}, found #{user['username']}" unless user['username'] == @username - - user["id"] - end + attr_reader :dry_run + alias_method :dry_run?, :dry_run - def delete_ssh_keys(user_id, ssh_key_ids) + def delete_ssh_keys(ssh_key_ids) STDOUT.puts "Deleting #{ssh_key_ids.length} ssh keys..." ssh_key_ids.each do |key_id| - delete_response = delete Runtime::API::Request.new(@api_client, "/users/#{user_id}/keys/#{key_id}").url + delete_response = delete Runtime::API::Request.new(@api_client, "/user/keys/#{key_id}").url dot_or_f = delete_response.code == 204 ? "\e[32m.\e[0m" : "\e[31mF\e[0m" print dot_or_f end end - def fetch_test_ssh_key_ids(user_id) - get_keys_response = get Runtime::API::Request.new(@api_client, "/users/#{user_id}/keys").url - JSON.parse(get_keys_response.body) - .select { |key| (key["title"].include?('key for ssh tests') || key["title"].include?('key for audit event test')) } - .map { |key| key['id'] } - .uniq + def fetch_test_ssh_key_ids(pages) + key_ids = [] + + pages.to_i.times do |page_no| + get_keys_response = get Runtime::API::Request.new(@api_client, "/user/keys", page: (page_no + 1).to_s, per_page: ITEMS_PER_PAGE).url + keys = JSON.parse(get_keys_response.body).select do |key| + to_delete = key['title'].include?(@title_portion) && Date.parse(key['created_at']) < @delete_before + + puts "Key title: #{key['title']}\tcreated_at: #{key['created_at']}\tdelete? #{to_delete}" if dry_run? + + to_delete + end + key_ids.concat(keys.map { |key| key['id'] }) + end + + key_ids.uniq end end end diff --git a/spec/factories/project_repository_storage_moves.rb b/spec/factories/project_repository_storage_moves.rb new file mode 100644 index 00000000000..aa8576834eb --- /dev/null +++ b/spec/factories/project_repository_storage_moves.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +FactoryBot.define do + factory :project_repository_storage_move, class: 'ProjectRepositoryStorageMove' do + project + + source_storage_name { 'default' } + destination_storage_name { 'default' } + + trait :scheduled do + state { ProjectRepositoryStorageMove.state_machines[:state].states[:scheduled].value } + end + end +end diff --git a/spec/lib/gitlab/exclusive_lease_helpers/sleeping_lock_spec.rb b/spec/lib/gitlab/exclusive_lease_helpers/sleeping_lock_spec.rb new file mode 100644 index 00000000000..8917eeec56f --- /dev/null +++ b/spec/lib/gitlab/exclusive_lease_helpers/sleeping_lock_spec.rb @@ -0,0 +1,102 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::ExclusiveLeaseHelpers::SleepingLock, :clean_gitlab_redis_shared_state do + include ::ExclusiveLeaseHelpers + + let(:timeout) { 1.second } + let(:delay) { 0.1.seconds } + let(:key) { SecureRandom.hex(10) } + + subject { described_class.new(key, timeout: timeout, delay: delay) } + + describe '#retried?' do + before do + stub_exclusive_lease(key, 'uuid') + end + + context 'we have not made any attempts' do + it { is_expected.not_to be_retried } + end + + context 'we just made a single (initial) attempt' do + it 'is not considered a retry' do + subject.send(:try_obtain) + + is_expected.not_to be_retried + end + end + + context 'made multiple attempts' do + it 'is considered a retry' do + 2.times { subject.send(:try_obtain) } + + is_expected.to be_retried + end + end + end + + describe '#obtain' do + context 'when the lease is not held' do + before do + stub_exclusive_lease(key, 'uuid') + end + + it 'obtains the lease on the first attempt, without sleeping' do + expect(subject).not_to receive(:sleep) + + subject.obtain(10) + + expect(subject).not_to be_retried + end + end + + context 'when the lease is held elsewhere' do + let!(:lease) { stub_exclusive_lease_taken(key) } + let(:max_attempts) { 7 } + + it 'retries to obtain a lease and raises an error' do + expect(subject).to receive(:sleep).with(delay).exactly(max_attempts - 1).times + expect(lease).to receive(:try_obtain).exactly(max_attempts).times + + expect { subject.obtain(max_attempts) }.to raise_error('Failed to obtain a lock') + end + + context 'when the delay is computed from the attempt number' do + let(:delay) { ->(n) { 3 * n } } + + it 'uses the computation to determine the sleep length' do + expect(subject).to receive(:sleep).with(3).once + expect(subject).to receive(:sleep).with(6).once + expect(subject).to receive(:sleep).with(9).once + expect(lease).to receive(:try_obtain).exactly(4).times + + expect { subject.obtain(4) }.to raise_error('Failed to obtain a lock') + end + end + + context 'when lease is granted after retry' do + it 'knows that it retried' do + expect(subject).to receive(:sleep).with(delay).exactly(3).times + expect(lease).to receive(:try_obtain).exactly(3).times { nil } + expect(lease).to receive(:try_obtain).once { 'obtained' } + + subject.obtain(max_attempts) + + expect(subject).to be_retried + end + end + end + + describe 'cancel' do + let!(:lease) { stub_exclusive_lease(key, 'uuid') } + + it 'cancels the lease' do + expect(lease).to receive(:cancel) + + subject.cancel + end + end + end +end diff --git a/spec/lib/gitlab/exclusive_lease_helpers_spec.rb b/spec/lib/gitlab/exclusive_lease_helpers_spec.rb index 065468f6b06..9914518cda5 100644 --- a/spec/lib/gitlab/exclusive_lease_helpers_spec.rb +++ b/spec/lib/gitlab/exclusive_lease_helpers_spec.rb @@ -22,9 +22,7 @@ describe Gitlab::ExclusiveLeaseHelpers, :clean_gitlab_redis_shared_state do end context 'when the lease is not obtained yet' do - before do - stub_exclusive_lease(unique_key, 'uuid') - end + let!(:lease) { stub_exclusive_lease(unique_key, 'uuid') } it 'calls the given block' do expect { |b| class_instance.in_lock(unique_key, &b) }.to yield_with_args(false) @@ -37,7 +35,7 @@ describe Gitlab::ExclusiveLeaseHelpers, :clean_gitlab_redis_shared_state do end it 'cancels the exclusive lease after the block' do - expect_to_cancel_exclusive_lease(unique_key, 'uuid') + expect(lease).to receive(:cancel).once subject end @@ -81,11 +79,21 @@ describe Gitlab::ExclusiveLeaseHelpers, :clean_gitlab_redis_shared_state do end end + context 'when we specify no retries' do + let(:options) { { retries: 0 } } + + it 'never sleeps' do + expect(class_instance).not_to receive(:sleep) + + expect { subject }.to raise_error('Failed to obtain a lock') + end + end + context 'when sleep second is specified' do let(:options) { { retries: 1, sleep_sec: 0.05.seconds } } it 'receives the specified argument' do - expect(class_instance).to receive(:sleep).with(0.05.seconds).twice + expect_any_instance_of(Object).to receive(:sleep).with(0.05.seconds).once expect { subject }.to raise_error('Failed to obtain a lock') end @@ -95,9 +103,8 @@ describe Gitlab::ExclusiveLeaseHelpers, :clean_gitlab_redis_shared_state do let(:options) { { retries: 2, sleep_sec: ->(num) { 0.1 + num } } } it 'receives the specified argument' do - expect(class_instance).to receive(:sleep).with(1.1.seconds).once - expect(class_instance).to receive(:sleep).with(2.1.seconds).once - expect(class_instance).to receive(:sleep).with(3.1.seconds).once + expect_any_instance_of(Object).to receive(:sleep).with(1.1.seconds).once + expect_any_instance_of(Object).to receive(:sleep).with(2.1.seconds).once expect { subject }.to raise_error('Failed to obtain a lock') end diff --git a/spec/lib/gitlab/exclusive_lease_spec.rb b/spec/lib/gitlab/exclusive_lease_spec.rb index 0739f622af5..2c0bb23a0b6 100644 --- a/spec/lib/gitlab/exclusive_lease_spec.rb +++ b/spec/lib/gitlab/exclusive_lease_spec.rb @@ -21,6 +21,27 @@ describe Gitlab::ExclusiveLease, :clean_gitlab_redis_shared_state do end end + describe '.redis_shared_state_key' do + it 'provides a namespaced key' do + expect(described_class.redis_shared_state_key(unique_key)) + .to start_with(described_class::PREFIX) + .and include(unique_key) + end + end + + describe '.ensure_prefixed_key' do + it 'does not double prefix a key' do + prefixed = described_class.redis_shared_state_key(unique_key) + + expect(described_class.ensure_prefixed_key(unique_key)) + .to eq(described_class.ensure_prefixed_key(prefixed)) + end + + it 'raises errors when there is no key' do + expect { described_class.ensure_prefixed_key(nil) }.to raise_error(described_class::NoKey) + end + end + describe '#renew' do it 'returns true when we have the existing lease' do lease = described_class.new(unique_key, timeout: 3600) @@ -61,18 +82,61 @@ describe Gitlab::ExclusiveLease, :clean_gitlab_redis_shared_state do end end - describe '.cancel' do - it 'can cancel a lease' do - uuid = new_lease(unique_key) - expect(uuid).to be_present - expect(new_lease(unique_key)).to eq(false) + describe 'cancellation' do + def new_lease(key) + described_class.new(key, timeout: 3600) + end - described_class.cancel(unique_key, uuid) - expect(new_lease(unique_key)).to be_present + shared_examples 'cancelling a lease' do + let(:lease) { new_lease(unique_key) } + + it 'releases the held lease' do + uuid = lease.try_obtain + expect(uuid).to be_present + expect(new_lease(unique_key).try_obtain).to eq(false) + + cancel_lease(uuid) + + expect(new_lease(unique_key).try_obtain).to be_present + end end - def new_lease(key) - described_class.new(key, timeout: 3600).try_obtain + describe '.cancel' do + def cancel_lease(uuid) + described_class.cancel(release_key, uuid) + end + + context 'when called with the unprefixed key' do + it_behaves_like 'cancelling a lease' do + let(:release_key) { unique_key } + end + end + + context 'when called with the prefixed key' do + it_behaves_like 'cancelling a lease' do + let(:release_key) { described_class.redis_shared_state_key(unique_key) } + end + end + + it 'does not raise errors when given a nil key' do + expect { described_class.cancel(nil, nil) }.not_to raise_error + end + end + + describe '#cancel' do + def cancel_lease(_uuid) + lease.cancel + end + + it_behaves_like 'cancelling a lease' + + it 'is safe to call even if the lease was never obtained' do + lease = new_lease(unique_key) + + lease.cancel + + expect(new_lease(unique_key).try_obtain).to be_present + end end end diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml index 562351f8fb9..d2790a6b858 100644 --- a/spec/lib/gitlab/import_export/all_models.yml +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -489,6 +489,7 @@ project: - compliance_framework_setting - metrics_users_starred_dashboards - alert_management_alerts +- repository_storage_moves award_emoji: - awardable - user diff --git a/spec/models/project_repository_storage_move_spec.rb b/spec/models/project_repository_storage_move_spec.rb new file mode 100644 index 00000000000..b25f4dfd634 --- /dev/null +++ b/spec/models/project_repository_storage_move_spec.rb @@ -0,0 +1,63 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ProjectRepositoryStorageMove, type: :model do + describe 'associations' do + it { is_expected.to belong_to(:project) } + end + + describe 'validations' do + it { is_expected.to validate_presence_of(:project) } + it { is_expected.to validate_presence_of(:state) } + it { is_expected.to validate_presence_of(:source_storage_name) } + it { is_expected.to validate_presence_of(:destination_storage_name) } + + context 'source_storage_name inclusion' do + subject { build(:project_repository_storage_move, source_storage_name: 'missing') } + + it "does not allow repository storages that don't match a label in the configuration" do + expect(subject).not_to be_valid + expect(subject.errors[:source_storage_name].first).to match(/is not included in the list/) + end + end + + context 'destination_storage_name inclusion' do + subject { build(:project_repository_storage_move, destination_storage_name: 'missing') } + + it "does not allow repository storages that don't match a label in the configuration" do + expect(subject).not_to be_valid + expect(subject.errors[:destination_storage_name].first).to match(/is not included in the list/) + end + end + end + + describe 'state transitions' do + using RSpec::Parameterized::TableSyntax + + context 'when in the default state' do + subject(:storage_move) { create(:project_repository_storage_move, project: project, destination_storage_name: 'test_second_storage') } + + let(:project) { create(:project) } + + before do + stub_storage_settings('test_second_storage' => { 'path' => 'tmp/tests/extra_storage' }) + end + + context 'and transits to scheduled' do + it 'triggers ProjectUpdateRepositoryStorageWorker' do + expect(ProjectUpdateRepositoryStorageWorker).to receive(:perform_async).with(project.id, 'test_second_storage', repository_storage_move_id: storage_move.id) + + storage_move.schedule! + end + end + + context 'and transits to started' do + it 'does not allow the transition' do + expect { storage_move.start! } + .to raise_error(StateMachines::InvalidTransition) + end + end + end + end +end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 8c2323eb0d8..b8c814a8ae1 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -115,6 +115,7 @@ describe Project do it { is_expected.to have_many(:alert_management_alerts) } it { is_expected.to have_many(:jira_imports) } it { is_expected.to have_many(:metrics_users_starred_dashboards).inverse_of(:project) } + it { is_expected.to have_many(:repository_storage_moves) } it_behaves_like 'model with repository' do let_it_be(:container) { create(:project, :repository, path: 'somewhere') } @@ -2837,12 +2838,16 @@ describe Project do end it 'schedules the transfer of the repository to the new storage and locks the project' do - expect(ProjectUpdateRepositoryStorageWorker).to receive(:perform_async).with(project.id, 'test_second_storage') + expect(ProjectUpdateRepositoryStorageWorker).to receive(:perform_async).with(project.id, 'test_second_storage', repository_storage_move_id: anything) project.change_repository_storage('test_second_storage') project.save! expect(project).to be_repository_read_only + expect(project.repository_storage_moves.last).to have_attributes( + source_storage_name: "default", + destination_storage_name: "test_second_storage" + ) end it "doesn't schedule the transfer if the repository is already read-only" do diff --git a/spec/services/projects/fork_service_spec.rb b/spec/services/projects/fork_service_spec.rb index c8354f6ba4e..112a41c773b 100644 --- a/spec/services/projects/fork_service_spec.rb +++ b/spec/services/projects/fork_service_spec.rb @@ -320,7 +320,13 @@ describe Projects::ForkService do allow_any_instance_of(Gitlab::Git::Repository).to receive(:checksum) .and_return(::Gitlab::Git::BLANK_SHA) - Projects::UpdateRepositoryStorageService.new(project).execute('test_second_storage') + storage_move = create( + :project_repository_storage_move, + :scheduled, + project: project, + destination_storage_name: 'test_second_storage' + ) + Projects::UpdateRepositoryStorageService.new(storage_move).execute fork_after_move = fork_project(project) pool_repository_before_move = PoolRepository.joins(:shard) .find_by(source_project: project, shards: { name: 'default' }) diff --git a/spec/services/projects/update_repository_storage_service_spec.rb b/spec/services/projects/update_repository_storage_service_spec.rb index 97df388fc15..b58dac9a6ce 100644 --- a/spec/services/projects/update_repository_storage_service_spec.rb +++ b/spec/services/projects/update_repository_storage_service_spec.rb @@ -5,7 +5,7 @@ require 'spec_helper' describe Projects::UpdateRepositoryStorageService do include Gitlab::ShellAdapter - subject { described_class.new(project) } + subject { described_class.new(repository_storage_move) } describe "#execute" do let(:time) { Time.now } @@ -17,6 +17,8 @@ describe Projects::UpdateRepositoryStorageService do context 'without wiki and design repository' do let(:project) { create(:project, :repository, repository_read_only: true, wiki_enabled: false) } + let(:destination) { 'test_second_storage' } + let(:repository_storage_move) { create(:project_repository_storage_move, :scheduled, project: project, destination_storage_name: destination) } let!(:checksum) { project.repository.checksum } let(:project_repository_double) { double(:repository) } @@ -42,9 +44,9 @@ describe Projects::UpdateRepositoryStorageService do expect(project_repository_double).to receive(:checksum) .and_return(checksum) - result = subject.execute('test_second_storage') + result = subject.execute - expect(result[:status]).to eq(:success) + expect(result).to be_success expect(project).not_to be_repository_read_only expect(project.repository_storage).to eq('test_second_storage') expect(gitlab_shell.repository_exists?('default', old_path)).to be(false) @@ -53,11 +55,13 @@ describe Projects::UpdateRepositoryStorageService do end context 'when the filesystems are the same' do + let(:destination) { project.repository_storage } + it 'bails out and does nothing' do - result = subject.execute(project.repository_storage) + result = subject.execute - expect(result[:status]).to eq(:error) - expect(result[:message]).to match(/SameFilesystemError/) + expect(result).to be_error + expect(result.message).to match(/SameFilesystemError/) end end @@ -73,9 +77,9 @@ describe Projects::UpdateRepositoryStorageService do .and_raise(Gitlab::Git::CommandError) expect(GitlabShellWorker).not_to receive(:perform_async) - result = subject.execute('test_second_storage') + result = subject.execute - expect(result[:status]).to eq(:error) + expect(result).to be_error expect(project).not_to be_repository_read_only expect(project.repository_storage).to eq('default') end @@ -94,9 +98,9 @@ describe Projects::UpdateRepositoryStorageService do .and_return('not matching checksum') expect(GitlabShellWorker).not_to receive(:perform_async) - result = subject.execute('test_second_storage') + result = subject.execute - expect(result[:status]).to eq(:error) + expect(result).to be_error expect(project).not_to be_repository_read_only expect(project.repository_storage).to eq('default') end @@ -116,9 +120,9 @@ describe Projects::UpdateRepositoryStorageService do expect(project_repository_double).to receive(:checksum) .and_return(checksum) - result = subject.execute('test_second_storage') + result = subject.execute - expect(result[:status]).to eq(:success) + expect(result).to be_success expect(project.repository_storage).to eq('test_second_storage') expect(project.reload_pool_repository).to be_nil end @@ -129,6 +133,8 @@ describe Projects::UpdateRepositoryStorageService do include_examples 'moves repository to another storage', 'wiki' do let(:project) { create(:project, :repository, repository_read_only: true, wiki_enabled: true) } let(:repository) { project.wiki.repository } + let(:destination) { 'test_second_storage' } + let(:repository_storage_move) { create(:project_repository_storage_move, :scheduled, project: project, destination_storage_name: destination) } before do project.create_wiki diff --git a/spec/support/helpers/exclusive_lease_helpers.rb b/spec/support/helpers/exclusive_lease_helpers.rb index 77703e20602..076e3cc3851 100644 --- a/spec/support/helpers/exclusive_lease_helpers.rb +++ b/spec/support/helpers/exclusive_lease_helpers.rb @@ -9,7 +9,8 @@ module ExclusiveLeaseHelpers Gitlab::ExclusiveLease, try_obtain: uuid, exists?: true, - renew: renew + renew: renew, + cancel: nil ) allow(Gitlab::ExclusiveLease) diff --git a/spec/support/shared_examples/services/projects/update_repository_storage_service_shared_examples.rb b/spec/support/shared_examples/services/projects/update_repository_storage_service_shared_examples.rb index d6166ac8188..0e6ecf49cd0 100644 --- a/spec/support/shared_examples/services/projects/update_repository_storage_service_shared_examples.rb +++ b/spec/support/shared_examples/services/projects/update_repository_storage_service_shared_examples.rb @@ -47,9 +47,9 @@ RSpec.shared_examples 'moves repository to another storage' do |repository_type| old_repository_path = repository.full_path - result = subject.execute('test_second_storage') + result = subject.execute - expect(result[:status]).to eq(:success) + expect(result).to be_success expect(project).not_to be_repository_read_only expect(project.repository_storage).to eq('test_second_storage') expect(gitlab_shell.repository_exists?('default', old_project_repository_path)).to be(false) @@ -62,7 +62,7 @@ RSpec.shared_examples 'moves repository to another storage' do |repository_type| end it 'does not enqueue a GC run' do - expect { subject.execute('test_second_storage') } + expect { subject.execute } .not_to change(GitGarbageCollectWorker.jobs, :count) end end @@ -75,23 +75,25 @@ RSpec.shared_examples 'moves repository to another storage' do |repository_type| it 'does not enqueue a GC run if housekeeping is disabled' do stub_application_setting(housekeeping_enabled: false) - expect { subject.execute('test_second_storage') } + expect { subject.execute } .not_to change(GitGarbageCollectWorker.jobs, :count) end it 'enqueues a GC run' do - expect { subject.execute('test_second_storage') } + expect { subject.execute } .to change(GitGarbageCollectWorker.jobs, :count).by(1) end end end context 'when the filesystems are the same' do + let(:destination) { project.repository_storage } + it 'bails out and does nothing' do - result = subject.execute(project.repository_storage) + result = subject.execute - expect(result[:status]).to eq(:error) - expect(result[:message]).to match(/SameFilesystemError/) + expect(result).to be_error + expect(result.message).to match(/SameFilesystemError/) end end @@ -114,9 +116,9 @@ RSpec.shared_examples 'moves repository to another storage' do |repository_type| expect(GitlabShellWorker).not_to receive(:perform_async) - result = subject.execute('test_second_storage') + result = subject.execute - expect(result[:status]).to eq(:error) + expect(result).to be_error expect(project).not_to be_repository_read_only expect(project.repository_storage).to eq('default') end @@ -142,9 +144,9 @@ RSpec.shared_examples 'moves repository to another storage' do |repository_type| expect(GitlabShellWorker).not_to receive(:perform_async) - result = subject.execute('test_second_storage') + result = subject.execute - expect(result[:status]).to eq(:error) + expect(result).to be_error expect(project).not_to be_repository_read_only expect(project.repository_storage).to eq('default') end diff --git a/spec/workers/project_update_repository_storage_worker_spec.rb b/spec/workers/project_update_repository_storage_worker_spec.rb index 57a4c2128b3..31d0d955759 100644 --- a/spec/workers/project_update_repository_storage_worker_spec.rb +++ b/spec/workers/project_update_repository_storage_worker_spec.rb @@ -9,12 +9,40 @@ describe ProjectUpdateRepositoryStorageWorker do subject { described_class.new } describe "#perform" do - it "calls the update repository storage service" do - expect_next_instance_of(Projects::UpdateRepositoryStorageService) do |instance| - expect(instance).to receive(:execute).with('new_storage') + let(:service) { double(:update_repository_storage_service) } + + before do + allow(Gitlab.config.repositories.storages).to receive(:keys).and_return(%w[default test_second_storage]) + end + + context 'without repository storage move' do + it "calls the update repository storage service" do + expect(Projects::UpdateRepositoryStorageService).to receive(:new).and_return(service) + expect(service).to receive(:execute) + + expect do + subject.perform(project.id, 'test_second_storage') + end.to change(ProjectRepositoryStorageMove, :count).by(1) + + storage_move = project.repository_storage_moves.last + expect(storage_move).to have_attributes( + source_storage_name: "default", + destination_storage_name: "test_second_storage" + ) end + end + + context 'with repository storage move' do + let!(:repository_storage_move) { create(:project_repository_storage_move) } - subject.perform(project.id, 'new_storage') + it "calls the update repository storage service" do + expect(Projects::UpdateRepositoryStorageService).to receive(:new).and_return(service) + expect(service).to receive(:execute) + + expect do + subject.perform(nil, nil, repository_storage_move_id: repository_storage_move.id) + end.not_to change(ProjectRepositoryStorageMove, :count) + end end end end |