diff options
33 files changed, 548 insertions, 252 deletions
diff --git a/app/finders/remote_mirror_finder.rb b/app/finders/remote_mirror_finder.rb deleted file mode 100644 index 420db0077aa..00000000000 --- a/app/finders/remote_mirror_finder.rb +++ /dev/null @@ -1,15 +0,0 @@ -# frozen_string_literal: true - -class RemoteMirrorFinder - attr_accessor :params - - def initialize(params) - @params = params - end - - # rubocop: disable CodeReuse/ActiveRecord - def execute - RemoteMirror.find_by(id: params[:id]) - end - # rubocop: enable CodeReuse/ActiveRecord -end diff --git a/app/mailers/emails/remote_mirrors.rb b/app/mailers/emails/remote_mirrors.rb index 2d8137843ec..f3938a052b0 100644 --- a/app/mailers/emails/remote_mirrors.rb +++ b/app/mailers/emails/remote_mirrors.rb @@ -3,7 +3,7 @@ module Emails module RemoteMirrors def remote_mirror_update_failed_email(remote_mirror_id, recipient_id) - @remote_mirror = RemoteMirrorFinder.new(id: remote_mirror_id).execute + @remote_mirror = RemoteMirror.find_by_id(remote_mirror_id) @project = @remote_mirror.project mail(to: recipient(recipient_id, @project.group), subject: subject('Remote mirror update failed')) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index ac88d9714ac..f705e67121f 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -384,7 +384,7 @@ module Ci return unless has_environment? strong_memoize(:expanded_environment_name) do - ExpandVariables.expand(environment, simple_variables) + ExpandVariables.expand(environment, -> { simple_variables }) end end diff --git a/app/models/remote_mirror.rb b/app/models/remote_mirror.rb index 6b5605f9999..c9ee0653d86 100644 --- a/app/models/remote_mirror.rb +++ b/app/models/remote_mirror.rb @@ -4,6 +4,8 @@ class RemoteMirror < ApplicationRecord include AfterCommitQueue include MirrorAuthentication + MAX_FIRST_RUNTIME = 3.hours + MAX_INCREMENTAL_RUNTIME = 1.hour PROTECTED_BACKOFF_DELAY = 1.minute UNPROTECTED_BACKOFF_DELAY = 5.minutes @@ -31,11 +33,18 @@ class RemoteMirror < ApplicationRecord scope :enabled, -> { where(enabled: true) } scope :started, -> { with_update_status(:started) } - scope :stuck, -> { started.where('last_update_at < ? OR (last_update_at IS NULL AND updated_at < ?)', 1.hour.ago, 3.hours.ago) } + + scope :stuck, -> do + started + .where('(last_update_started_at < ? AND last_update_at IS NOT NULL)', + MAX_INCREMENTAL_RUNTIME.ago) + .or(where('(last_update_started_at < ? AND last_update_at IS NULL)', + MAX_FIRST_RUNTIME.ago)) + end state_machine :update_status, initial: :none do event :update_start do - transition [:none, :finished, :failed] => :started + transition any => :started end event :update_finish do @@ -46,9 +55,14 @@ class RemoteMirror < ApplicationRecord transition started: :failed end + event :update_retry do + transition started: :to_retry + end + state :started state :finished state :failed + state :to_retry after_transition any => :started do |remote_mirror, _| Gitlab::Metrics.add_event(:remote_mirrors_running) @@ -138,16 +152,27 @@ class RemoteMirror < ApplicationRecord end def updated_since?(timestamp) - last_update_started_at && last_update_started_at > timestamp && !update_failed? + return false if failed? + + last_update_started_at && last_update_started_at > timestamp end def mark_for_delete_if_blank_url mark_for_destruction if url.blank? end - def mark_as_failed(error_message) - update_column(:last_error, Gitlab::UrlSanitizer.sanitize(error_message)) - update_fail + def update_error_message(error_message) + self.last_error = Gitlab::UrlSanitizer.sanitize(error_message) + end + + def mark_for_retry!(error_message) + update_error_message(error_message) + update_retry! + end + + def mark_as_failed!(error_message) + update_error_message(error_message) + update_fail! end def url=(value) @@ -190,6 +215,18 @@ class RemoteMirror < ApplicationRecord update_column(:error_notification_sent, true) end + def backoff_delay + if self.only_protected_branches + PROTECTED_BACKOFF_DELAY + else + UNPROTECTED_BACKOFF_DELAY + end + end + + def max_runtime + last_update_at.present? ? MAX_INCREMENTAL_RUNTIME : MAX_FIRST_RUNTIME + end + private def store_credentials @@ -219,14 +256,6 @@ class RemoteMirror < ApplicationRecord self.last_update_started_at >= Time.now - backoff_delay end - def backoff_delay - if self.only_protected_branches - PROTECTED_BACKOFF_DELAY - else - UNPROTECTED_BACKOFF_DELAY - end - end - def reset_fields update_columns( last_error: nil, diff --git a/app/models/repository.rb b/app/models/repository.rb index 58abfaef801..9d45a12fa6e 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -418,25 +418,29 @@ class Repository end # Runs code before pushing (= creating or removing) a tag. + # + # Note that this doesn't expire the tags. You may need to call + # expire_caches_for_tags or expire_tags_cache. def before_push_tag + repository_event(:push_tag) + end + + def expire_caches_for_tags expire_statistics_caches expire_emptiness_caches expire_tags_cache - - repository_event(:push_tag) end # Runs code before removing a tag. def before_remove_tag - expire_tags_cache - expire_statistics_caches + expire_caches_for_tags repository_event(:remove_tag) end # Runs code after removing a tag. def after_remove_tag - expire_tags_cache + expire_caches_for_tags end # Runs code after the HEAD of a repository is changed. diff --git a/app/services/projects/update_remote_mirror_service.rb b/app/services/projects/update_remote_mirror_service.rb index 1244a0f72a7..13a467a3ef9 100644 --- a/app/services/projects/update_remote_mirror_service.rb +++ b/app/services/projects/update_remote_mirror_service.rb @@ -2,31 +2,52 @@ module Projects class UpdateRemoteMirrorService < BaseService - attr_reader :errors + MAX_TRIES = 3 - def execute(remote_mirror) + def execute(remote_mirror, tries) return success unless remote_mirror.enabled? - errors = [] + update_mirror(remote_mirror) - begin - remote_mirror.ensure_remote! - repository.fetch_remote(remote_mirror.remote_name, ssh_auth: remote_mirror, no_tags: true) + success + rescue Gitlab::Git::CommandError => e + # This happens if one of the gitaly calls above fail, for example when + # branches have diverged, or the pre-receive hook fails. + retry_or_fail(remote_mirror, e.message, tries) - opts = {} - if remote_mirror.only_protected_branches? - opts[:only_branches_matching] = project.protected_branches.select(:name).map(&:name) - end + error(e.message) + rescue => e + remote_mirror.mark_as_failed!(e.message) + raise e + end + + private + + def update_mirror(remote_mirror) + remote_mirror.update_start! + + remote_mirror.ensure_remote! + repository.fetch_remote(remote_mirror.remote_name, ssh_auth: remote_mirror, no_tags: true) - remote_mirror.update_repository(opts) - rescue => e - errors << e.message.strip + opts = {} + if remote_mirror.only_protected_branches? + opts[:only_branches_matching] = project.protected_branches.select(:name).map(&:name) end - if errors.present? - error(errors.join("\n\n")) + remote_mirror.update_repository(opts) + + remote_mirror.update_finish! + end + + def retry_or_fail(mirror, message, tries) + if tries < MAX_TRIES + mirror.mark_for_retry!(message) else - success + # It's not likely we'll be able to recover from this ourselves, so we'll + # notify the users of the problem, and don't trigger any sidekiq retries + # Instead, we'll wait for the next change to try the push again, or until + # a user manually retries. + mirror.mark_as_failed!(message) end end end diff --git a/app/services/update_deployment_service.rb b/app/services/update_deployment_service.rb index 49a7d0178f4..dcafebae52d 100644 --- a/app/services/update_deployment_service.rb +++ b/app/services/update_deployment_service.rb @@ -42,7 +42,7 @@ class UpdateDeploymentService return unless environment_url @expanded_environment_url = - ExpandVariables.expand(environment_url, variables) + ExpandVariables.expand(environment_url, -> { variables }) end def environment_url diff --git a/app/views/projects/mirrors/_mirror_repos.html.haml b/app/views/projects/mirrors/_mirror_repos.html.haml index 280ec6d715b..eb100e5cf47 100644 --- a/app/views/projects/mirrors/_mirror_repos.html.haml +++ b/app/views/projects/mirrors/_mirror_repos.html.haml @@ -43,7 +43,8 @@ = _('Mirrored repositories') = render_if_exists 'projects/mirrors/mirrored_repositories_count' %th= _('Direction') - %th= _('Last update') + %th= _('Last update attempt') + %th= _('Last successful update') %th %th %tbody.js-mirrors-table-body @@ -53,6 +54,8 @@ %tr.qa-mirrored-repository-row.rspec-mirrored-repository-row{ class: ('bg-secondary' if mirror.disabled?) } %td.qa-mirror-repository-url= mirror.safe_url %td= _('Push') + %td + = mirror.last_update_started_at.present? ? time_ago_with_tooltip(mirror.last_update_started_at) : _('Never') %td.qa-mirror-last-update-at= mirror.last_update_at.present? ? time_ago_with_tooltip(mirror.last_update_at) : _('Never') %td - if mirror.disabled? diff --git a/app/workers/post_receive.rb b/app/workers/post_receive.rb index d8dfbc0faf7..622bd6f1f48 100644 --- a/app/workers/post_receive.rb +++ b/app/workers/post_receive.rb @@ -44,6 +44,8 @@ class PostReceive # Expire the branches cache so we have updated data for this push post_received.project.repository.expire_branches_cache if post_received.includes_branches? + # We only need to expire tags once per push + post_received.project.repository.expire_caches_for_tags if post_received.includes_tags? post_received.enum_for(:changes_refs).with_index do |(oldrev, newrev, ref), index| service_klass = @@ -75,6 +77,7 @@ class PostReceive def after_project_changes_hooks(post_received, user, refs, changes) hook_data = Gitlab::DataBuilder::Repository.update(post_received.project, user, changes, refs) SystemHooksService.new.execute_hooks(hook_data, :repository_update_hooks) + Gitlab::UsageDataCounters::SourceCodeCounter.count(:pushes) end def process_wiki_changes(post_received) diff --git a/app/workers/remote_mirror_notification_worker.rb b/app/workers/remote_mirror_notification_worker.rb index 5bafe8e2046..368abfeda99 100644 --- a/app/workers/remote_mirror_notification_worker.rb +++ b/app/workers/remote_mirror_notification_worker.rb @@ -4,7 +4,7 @@ class RemoteMirrorNotificationWorker include ApplicationWorker def perform(remote_mirror_id) - remote_mirror = RemoteMirrorFinder.new(id: remote_mirror_id).execute + remote_mirror = RemoteMirror.find_by_id(remote_mirror_id) # We check again if there's an error because a newer run since this job was # fired could've completed successfully. diff --git a/app/workers/repository_update_remote_mirror_worker.rb b/app/workers/repository_update_remote_mirror_worker.rb index 03a7ff2cd7a..d13c7641eb3 100644 --- a/app/workers/repository_update_remote_mirror_worker.rb +++ b/app/workers/repository_update_remote_mirror_worker.rb @@ -1,50 +1,53 @@ # frozen_string_literal: true class RepositoryUpdateRemoteMirrorWorker - UpdateAlreadyInProgressError = Class.new(StandardError) UpdateError = Class.new(StandardError) include ApplicationWorker + include Gitlab::ExclusiveLeaseHelpers sidekiq_options retry: 3, dead: false - sidekiq_retry_in { |count| 30 * count } + LOCK_WAIT_TIME = 30.seconds + MAX_TRIES = 3 - sidekiq_retries_exhausted do |msg, _| - Sidekiq.logger.warn "Failed #{msg['class']} with #{msg['args']}: #{msg['error_message']}" - end - - def perform(remote_mirror_id, scheduled_time) - remote_mirror = RemoteMirrorFinder.new(id: remote_mirror_id).execute + def perform(remote_mirror_id, scheduled_time, tries = 0) + remote_mirror = RemoteMirror.find_by_id(remote_mirror_id) + return unless remote_mirror return if remote_mirror.updated_since?(scheduled_time) - raise UpdateAlreadyInProgressError if remote_mirror.update_in_progress? + # If the update is already running, wait for it to finish before running again + # This will wait for a total of 90 seconds in 3 steps + in_lock(remote_mirror_update_lock(remote_mirror.id), + retries: 3, + ttl: remote_mirror.max_runtime, + sleep_sec: LOCK_WAIT_TIME) do + update_mirror(remote_mirror, scheduled_time, tries) + end + rescue Gitlab::ExclusiveLeaseHelpers::FailedToObtainLockError + # If an update runs longer than 1.5 minutes, we'll reschedule it + # with a backoff. The next run will check if the previous update would + # include the changes that triggered this update and become a no-op. + self.class.perform_in(remote_mirror.backoff_delay, remote_mirror.id, scheduled_time, tries) + end - remote_mirror.update_start + private - project = remote_mirror.project + def update_mirror(mirror, scheduled_time, tries) + project = mirror.project current_user = project.creator - result = Projects::UpdateRemoteMirrorService.new(project, current_user).execute(remote_mirror) - raise UpdateError, result[:message] if result[:status] == :error - - remote_mirror.update_finish - rescue UpdateAlreadyInProgressError - raise - rescue UpdateError => ex - fail_remote_mirror(remote_mirror, ex.message) - raise - rescue => ex - return unless remote_mirror + result = Projects::UpdateRemoteMirrorService.new(project, current_user).execute(mirror, tries) - fail_remote_mirror(remote_mirror, ex.message) - raise UpdateError, "#{ex.class}: #{ex.message}" + if result[:status] == :error && mirror.to_retry? + schedule_retry(mirror, scheduled_time, tries) + end end - private - - def fail_remote_mirror(remote_mirror, message) - remote_mirror.mark_as_failed(message) + def remote_mirror_update_lock(mirror_id) + [self.class.name, mirror_id].join(':') + end - Rails.logger.error(message) # rubocop:disable Gitlab/RailsLogger + def schedule_retry(mirror, scheduled_time, tries) + self.class.perform_in(mirror.backoff_delay, mirror.id, scheduled_time, tries + 1) end end diff --git a/changelogs/unreleased/bump_helm_kubectl_gitlab.yml b/changelogs/unreleased/bump_helm_kubectl_gitlab.yml new file mode 100644 index 00000000000..d768462e130 --- /dev/null +++ b/changelogs/unreleased/bump_helm_kubectl_gitlab.yml @@ -0,0 +1,5 @@ +--- +title: Bump Helm to 2.14.3 and kubectl to 1.11.10 for Kubernetes integration +merge_request: 31716 +author: +type: other diff --git a/changelogs/unreleased/bvl-remote-mirror-exception-handling.yml b/changelogs/unreleased/bvl-remote-mirror-exception-handling.yml new file mode 100644 index 00000000000..962376086b0 --- /dev/null +++ b/changelogs/unreleased/bvl-remote-mirror-exception-handling.yml @@ -0,0 +1,6 @@ +--- +title: Retry push mirrors faster when running concurrently, improve error handling + when push mirrors fail +merge_request: 31247 +author: +type: changed diff --git a/changelogs/unreleased/id-source-code-smau.yml b/changelogs/unreleased/id-source-code-smau.yml new file mode 100644 index 00000000000..6ba5068544e --- /dev/null +++ b/changelogs/unreleased/id-source-code-smau.yml @@ -0,0 +1,5 @@ +--- +title: Add usage pings for source code pushes +merge_request: 31734 +author: +type: added diff --git a/changelogs/unreleased/sh-only-flush-tags-once-per-push.yml b/changelogs/unreleased/sh-only-flush-tags-once-per-push.yml new file mode 100644 index 00000000000..502fc22ebbd --- /dev/null +++ b/changelogs/unreleased/sh-only-flush-tags-once-per-push.yml @@ -0,0 +1,5 @@ +--- +title: Only expire tag cache once per push +merge_request: 31641 +author: +type: performance diff --git a/lib/expand_variables.rb b/lib/expand_variables.rb index c83cec9dc4a..45af30f46dc 100644 --- a/lib/expand_variables.rb +++ b/lib/expand_variables.rb @@ -3,6 +3,20 @@ module ExpandVariables class << self def expand(value, variables) + variables_hash = nil + + value.gsub(/\$([a-zA-Z_][a-zA-Z0-9_]*)|\${\g<1>}|%\g<1>%/) do + variables_hash ||= transform_variables(variables) + variables_hash[$1 || $2] + end + end + + private + + def transform_variables(variables) + # Lazily initialise variables + variables = variables.call if variables.is_a?(Proc) + # Convert hash array to variables if variables.is_a?(Array) variables = variables.reduce({}) do |hash, variable| @@ -11,9 +25,7 @@ module ExpandVariables end end - value.gsub(/\$([a-zA-Z_][a-zA-Z0-9_]*)|\${\g<1>}|%\g<1>%/) do - variables[$1 || $2] - end + variables end end end diff --git a/lib/gitlab/git_post_receive.rb b/lib/gitlab/git_post_receive.rb index 6c7f23a673c..24d752b8a4b 100644 --- a/lib/gitlab/git_post_receive.rb +++ b/lib/gitlab/git_post_receive.rb @@ -33,6 +33,12 @@ module Gitlab end end + def includes_tags? + enum_for(:changes_refs).any? do |_oldrev, _newrev, ref| + Gitlab::Git.tag_ref?(ref) + end + end + private def deserialize_changes(changes) diff --git a/lib/gitlab/kubernetes/helm.rb b/lib/gitlab/kubernetes/helm.rb index 42c4745ff98..6e4286589c1 100644 --- a/lib/gitlab/kubernetes/helm.rb +++ b/lib/gitlab/kubernetes/helm.rb @@ -3,8 +3,8 @@ module Gitlab module Kubernetes module Helm - HELM_VERSION = '2.12.3'.freeze - KUBECTL_VERSION = '1.11.7'.freeze + HELM_VERSION = '2.14.3'.freeze + KUBECTL_VERSION = '1.11.10'.freeze NAMESPACE = 'gitlab-managed-apps'.freeze SERVICE_ACCOUNT = 'tiller'.freeze CLUSTER_ROLE_BINDING = 'tiller-admin'.freeze diff --git a/lib/gitlab/usage_data.rb b/lib/gitlab/usage_data.rb index 7e3a695e52a..16a076a8811 100644 --- a/lib/gitlab/usage_data.rb +++ b/lib/gitlab/usage_data.rb @@ -142,7 +142,8 @@ module Gitlab Gitlab::UsageDataCounters::WebIdeCounter, Gitlab::UsageDataCounters::NoteCounter, Gitlab::UsageDataCounters::SnippetCounter, - Gitlab::UsageDataCounters::SearchCounter + Gitlab::UsageDataCounters::SearchCounter, + Gitlab::UsageDataCounters::SourceCodeCounter ] end diff --git a/lib/gitlab/usage_data_counters/source_code_counter.rb b/lib/gitlab/usage_data_counters/source_code_counter.rb new file mode 100644 index 00000000000..8a1771a7bd1 --- /dev/null +++ b/lib/gitlab/usage_data_counters/source_code_counter.rb @@ -0,0 +1,8 @@ +# frozen_string_literal: true + +module Gitlab::UsageDataCounters + class SourceCodeCounter < BaseCounter + KNOWN_EVENTS = %w[pushes].freeze + PREFIX = 'source_code' + end +end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index d33c62031c4..27435576495 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -6419,9 +6419,15 @@ msgstr "" msgid "Last seen" msgstr "" +msgid "Last successful update" +msgstr "" + msgid "Last update" msgstr "" +msgid "Last update attempt" +msgstr "" + msgid "Last updated" msgstr "" diff --git a/spec/lib/expand_variables_spec.rb b/spec/lib/expand_variables_spec.rb index 099d7b6b67c..394efa85701 100644 --- a/spec/lib/expand_variables_spec.rb +++ b/spec/lib/expand_variables_spec.rb @@ -4,62 +4,131 @@ require 'spec_helper' describe ExpandVariables do describe '#expand' do - subject { described_class.expand(value, variables) } + context 'table tests' do + using RSpec::Parameterized::TableSyntax - tests = [ - { value: 'key', - result: 'key', - variables: [] }, - { value: 'key$variable', - result: 'key', - variables: [] }, - { value: 'key$variable', - result: 'keyvalue', - variables: [ - { key: 'variable', value: 'value' } - ] }, - { value: 'key${variable}', - result: 'keyvalue', - variables: [ - { key: 'variable', value: 'value' } - ] }, - { value: 'key$variable$variable2', - result: 'keyvalueresult', - variables: [ - { key: 'variable', value: 'value' }, - { key: 'variable2', value: 'result' } - ] }, - { value: 'key${variable}${variable2}', - result: 'keyvalueresult', - variables: [ - { key: 'variable', value: 'value' }, - { key: 'variable2', value: 'result' } - ] }, - { value: 'key$variable2$variable', - result: 'keyresultvalue', - variables: [ - { key: 'variable', value: 'value' }, - { key: 'variable2', value: 'result' } - ] }, - { value: 'key${variable2}${variable}', - result: 'keyresultvalue', - variables: [ - { key: 'variable', value: 'value' }, - { key: 'variable2', value: 'result' } - ] }, - { value: 'review/$CI_COMMIT_REF_NAME', - result: 'review/feature/add-review-apps', - variables: [ - { key: 'CI_COMMIT_REF_NAME', value: 'feature/add-review-apps' } - ] } - ] + where do + { + "no expansion": { + value: 'key', + result: 'key', + variables: [] + }, + "missing variable": { + value: 'key$variable', + result: 'key', + variables: [] + }, + "simple expansion": { + value: 'key$variable', + result: 'keyvalue', + variables: [ + { key: 'variable', value: 'value' } + ] + }, + "simple with hash of variables": { + value: 'key$variable', + result: 'keyvalue', + variables: { + 'variable' => 'value' + } + }, + "complex expansion": { + value: 'key${variable}', + result: 'keyvalue', + variables: [ + { key: 'variable', value: 'value' } + ] + }, + "simple expansions": { + value: 'key$variable$variable2', + result: 'keyvalueresult', + variables: [ + { key: 'variable', value: 'value' }, + { key: 'variable2', value: 'result' } + ] + }, + "complex expansions": { + value: 'key${variable}${variable2}', + result: 'keyvalueresult', + variables: [ + { key: 'variable', value: 'value' }, + { key: 'variable2', value: 'result' } + ] + }, + "complex expansions with missing variable": { + value: 'key${variable}${variable2}', + result: 'keyvalue', + variables: [ + { key: 'variable', value: 'value' } + ] + }, + "out-of-order expansion": { + value: 'key$variable2$variable', + result: 'keyresultvalue', + variables: [ + { key: 'variable', value: 'value' }, + { key: 'variable2', value: 'result' } + ] + }, + "out-of-order complex expansion": { + value: 'key${variable2}${variable}', + result: 'keyresultvalue', + variables: [ + { key: 'variable', value: 'value' }, + { key: 'variable2', value: 'result' } + ] + }, + "review-apps expansion": { + value: 'review/$CI_COMMIT_REF_NAME', + result: 'review/feature/add-review-apps', + variables: [ + { key: 'CI_COMMIT_REF_NAME', value: 'feature/add-review-apps' } + ] + }, + "do not lazily access variables when no expansion": { + value: 'key', + result: 'key', + variables: -> { raise NotImplementedError } + }, + "lazily access variables": { + value: 'key$variable', + result: 'keyvalue', + variables: -> { [{ key: 'variable', value: 'value' }] } + } + } + end + + with_them do + subject { ExpandVariables.expand(value, variables) } # rubocop:disable RSpec/DescribedClass + + it { is_expected.to eq(result) } + end + end + + context 'lazily inits variables' do + let(:variables) { -> { [{ key: 'variable', value: 'result' }] } } + + subject { described_class.expand(value, variables) } + + context 'when expanding variable' do + let(:value) { 'key$variable$variable2' } + + it 'calls block at most once' do + expect(variables).to receive(:call).once.and_call_original + + is_expected.to eq('keyresult') + end + end + + context 'when no expansion is needed' do + let(:value) { 'key' } - tests.each do |test| - context "#{test[:value]} resolves to #{test[:result]}" do - let(:value) { test[:value] } - let(:variables) { test[:variables] } + it 'does not call block' do + expect(variables).not_to receive(:call) - it { is_expected.to eq(test[:result]) } + is_expected.to eq('key') + end end end end diff --git a/spec/lib/gitlab/git_post_receive_spec.rb b/spec/lib/gitlab/git_post_receive_spec.rb index 1911e954df9..4c20d945585 100644 --- a/spec/lib/gitlab/git_post_receive_spec.rb +++ b/spec/lib/gitlab/git_post_receive_spec.rb @@ -49,4 +49,47 @@ describe ::Gitlab::GitPostReceive do end end end + + describe '#includes_tags?' do + context 'with no tags' do + let(:changes) do + <<~EOF + 654321 210987 refs/notags/tag1 + 654322 210986 refs/heads/test1 + 654323 210985 refs/merge-requests/mr1 + EOF + end + + it 'returns false' do + expect(subject.includes_tags?).to be_falsey + end + end + + context 'with tags' do + let(:changes) do + <<~EOF + 654322 210986 refs/heads/test1 + 654321 210987 refs/tags/tag1 + 654323 210985 refs/merge-requests/mr1 + EOF + end + + it 'returns true' do + expect(subject.includes_tags?).to be_truthy + end + end + + context 'with malformed changes' do + let(:changes) do + <<~EOF + ref/tags/1 a + sometag refs/tags/2 + EOF + end + + it 'returns false' do + expect(subject.includes_tags?).to be_falsey + end + end + end end diff --git a/spec/lib/gitlab/kubernetes/helm/pod_spec.rb b/spec/lib/gitlab/kubernetes/helm/pod_spec.rb index 06c8d127951..fce2aded786 100644 --- a/spec/lib/gitlab/kubernetes/helm/pod_spec.rb +++ b/spec/lib/gitlab/kubernetes/helm/pod_spec.rb @@ -30,7 +30,7 @@ describe Gitlab::Kubernetes::Helm::Pod do it 'generates the appropriate specifications for the container' do container = subject.generate.spec.containers.first expect(container.name).to eq('helm') - expect(container.image).to eq('registry.gitlab.com/gitlab-org/cluster-integration/helm-install-image/releases/2.12.3-kube-1.11.7') + expect(container.image).to eq('registry.gitlab.com/gitlab-org/cluster-integration/helm-install-image/releases/2.14.3-kube-1.11.10') expect(container.env.count).to eq(3) expect(container.env.map(&:name)).to match_array([:HELM_VERSION, :TILLER_NAMESPACE, :COMMAND_SCRIPT]) expect(container.command).to match_array(["/bin/sh"]) diff --git a/spec/lib/gitlab/usage_data_counters/source_code_counter_spec.rb b/spec/lib/gitlab/usage_data_counters/source_code_counter_spec.rb new file mode 100644 index 00000000000..47077345e0c --- /dev/null +++ b/spec/lib/gitlab/usage_data_counters/source_code_counter_spec.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::UsageDataCounters::SourceCodeCounter do + it_behaves_like 'a redis usage counter', 'Source Code', :pushes + + it_behaves_like 'a redis usage counter with totals', :source_code, pushes: 5 +end diff --git a/spec/lib/gitlab/usage_data_spec.rb b/spec/lib/gitlab/usage_data_spec.rb index bf36273251b..f63f3b454e7 100644 --- a/spec/lib/gitlab/usage_data_spec.rb +++ b/spec/lib/gitlab/usage_data_spec.rb @@ -71,7 +71,8 @@ describe Gitlab::UsageData do web_ide_views: a_kind_of(Integer), web_ide_commits: a_kind_of(Integer), web_ide_merge_requests: a_kind_of(Integer), - navbar_searches: a_kind_of(Integer) + navbar_searches: a_kind_of(Integer), + source_code_pushes: a_kind_of(Integer) ) end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 29a589eba20..83ce58235dd 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -2252,6 +2252,21 @@ describe Project do end end + describe '#mark_stuck_remote_mirrors_as_failed!' do + it 'fails stuck remote mirrors' do + project = create(:project, :repository, :remote_mirror) + + project.remote_mirrors.first.update( + update_status: :started, + last_update_started_at: 2.days.ago + ) + + expect do + project.mark_stuck_remote_mirrors_as_failed! + end.to change { project.remote_mirrors.stuck.count }.from(1).to(0) + end + end + describe '#ancestors_upto' do let(:parent) { create(:group) } let(:child) { create(:group, parent: parent) } diff --git a/spec/models/remote_mirror_spec.rb b/spec/models/remote_mirror_spec.rb index 687b0935c55..7edeb56efe2 100644 --- a/spec/models/remote_mirror_spec.rb +++ b/spec/models/remote_mirror_spec.rb @@ -153,14 +153,14 @@ describe RemoteMirror, :mailer do end end - describe '#mark_as_failed' do + describe '#mark_as_failed!' do let(:remote_mirror) { create(:remote_mirror) } let(:error_message) { 'http://user:pass@test.com/root/repoC.git/' } let(:sanitized_error_message) { 'http://*****:*****@test.com/root/repoC.git/' } subject do remote_mirror.update_start - remote_mirror.mark_as_failed(error_message) + remote_mirror.mark_as_failed!(error_message) end it 'sets the update_status to failed' do @@ -204,8 +204,8 @@ describe RemoteMirror, :mailer do it 'includes mirrors that were started over an hour ago' do mirror = create_mirror(url: 'http://cantbeblank', update_status: 'started', - last_update_at: 3.hours.ago, - updated_at: 2.hours.ago) + last_update_started_at: 3.hours.ago, + last_update_at: 2.hours.ago) expect(described_class.stuck.last).to eq(mirror) end @@ -214,7 +214,7 @@ describe RemoteMirror, :mailer do mirror = create_mirror(url: 'http://cantbeblank', update_status: 'started', last_update_at: nil, - updated_at: 4.hours.ago) + last_update_started_at: 4.hours.ago) expect(described_class.stuck.last).to eq(mirror) end diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index fa243876632..e68de2e73a8 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -1744,12 +1744,23 @@ describe Repository do end end - describe '#before_push_tag' do + describe '#expires_caches_for_tags' do it 'flushes the cache' do expect(repository).to receive(:expire_statistics_caches) expect(repository).to receive(:expire_emptiness_caches) expect(repository).to receive(:expire_tags_cache) + repository.expire_caches_for_tags + end + end + + describe '#before_push_tag' do + it 'logs an event' do + expect(repository).not_to receive(:expire_statistics_caches) + expect(repository).not_to receive(:expire_emptiness_caches) + expect(repository).not_to receive(:expire_tags_cache) + expect(repository).to receive(:repository_event).with(:push_tag) + repository.before_push_tag end end diff --git a/spec/services/git/tag_push_service_spec.rb b/spec/services/git/tag_push_service_spec.rb index 418952b52da..7e008637182 100644 --- a/spec/services/git/tag_push_service_spec.rb +++ b/spec/services/git/tag_push_service_spec.rb @@ -26,8 +26,8 @@ describe Git::TagPushService do subject end - it 'flushes the tags cache' do - expect(project.repository).to receive(:expire_tags_cache) + it 'does not flush the tags cache' do + expect(project.repository).not_to receive(:expire_tags_cache) subject end diff --git a/spec/services/projects/update_remote_mirror_service_spec.rb b/spec/services/projects/update_remote_mirror_service_spec.rb index be2811ab1e7..4396ccab584 100644 --- a/spec/services/projects/update_remote_mirror_service_spec.rb +++ b/spec/services/projects/update_remote_mirror_service_spec.rb @@ -10,49 +10,91 @@ describe Projects::UpdateRemoteMirrorService do subject(:service) { described_class.new(project, project.creator) } - describe "#execute" do + describe '#execute' do + subject(:execute!) { service.execute(remote_mirror, 0) } + before do project.repository.add_branch(project.owner, 'existing-branch', 'master') allow(remote_mirror).to receive(:update_repository).and_return(true) end - it "ensures the remote exists" do + it 'ensures the remote exists' do stub_fetch_remote(project, remote_name: remote_name, ssh_auth: remote_mirror) expect(remote_mirror).to receive(:ensure_remote!) - service.execute(remote_mirror) + execute! end - it "fetches the remote repository" do + it 'fetches the remote repository' do expect(project.repository) .to receive(:fetch_remote) - .with(remote_mirror.remote_name, no_tags: true, ssh_auth: remote_mirror) + .with(remote_mirror.remote_name, no_tags: true, ssh_auth: remote_mirror) - service.execute(remote_mirror) + execute! end - it "returns success when updated succeeds" do + it 'marks the mirror as started when beginning' do + expect(remote_mirror).to receive(:update_start!).and_call_original + + execute! + end + + it 'marks the mirror as successfully finished' do stub_fetch_remote(project, remote_name: remote_name, ssh_auth: remote_mirror) - result = service.execute(remote_mirror) + result = execute! expect(result[:status]).to eq(:success) + expect(remote_mirror).to be_finished + end + + it 'marks the mirror as failed and raises the error when an unexpected error occurs' do + allow(project.repository).to receive(:fetch_remote).and_raise('Badly broken') + + expect { execute! }.to raise_error /Badly broken/ + + expect(remote_mirror).to be_failed + expect(remote_mirror.last_error).to include('Badly broken') + end + + context 'when the update fails because of a `Gitlab::Git::CommandError`' do + before do + allow(project.repository).to receive(:fetch_remote).and_raise(Gitlab::Git::CommandError.new('fetch failed')) + end + + it 'wraps `Gitlab::Git::CommandError`s in a service error' do + expect(execute!).to eq(status: :error, message: 'fetch failed') + end + + it 'marks the mirror as to be retried' do + execute! + + expect(remote_mirror).to be_to_retry + expect(remote_mirror.last_error).to include('fetch failed') + end + + it "marks the mirror as failed after #{described_class::MAX_TRIES} tries" do + service.execute(remote_mirror, described_class::MAX_TRIES) + + expect(remote_mirror).to be_failed + expect(remote_mirror.last_error).to include('fetch failed') + end end context 'when syncing all branches' do - it "push all the branches the first time" do + it 'push all the branches the first time' do stub_fetch_remote(project, remote_name: remote_name, ssh_auth: remote_mirror) expect(remote_mirror).to receive(:update_repository).with({}) - service.execute(remote_mirror) + execute! end end context 'when only syncing protected branches' do - it "sync updated protected branches" do + it 'sync updated protected branches' do stub_fetch_remote(project, remote_name: remote_name, ssh_auth: remote_mirror) protected_branch = create_protected_branch(project) remote_mirror.only_protected_branches = true @@ -61,7 +103,7 @@ describe Projects::UpdateRemoteMirrorService do .to receive(:update_repository) .with(only_branches_matching: [protected_branch.name]) - service.execute(remote_mirror) + execute! end def create_protected_branch(project) diff --git a/spec/workers/post_receive_spec.rb b/spec/workers/post_receive_spec.rb index 081d95d4d79..4c9fc0e3a24 100644 --- a/spec/workers/post_receive_spec.rb +++ b/spec/workers/post_receive_spec.rb @@ -14,6 +14,10 @@ describe PostReceive do create(:project, :repository, auto_cancel_pending_pipelines: 'disabled') end + def perform(changes: base64_changes) + described_class.new.perform(gl_repository, key_id, changes) + end + context "as a sidekiq worker" do it "responds to #perform" do expect(described_class.new).to respond_to(:perform) @@ -28,7 +32,7 @@ describe PostReceive do it "returns false and logs an error" do expect(Gitlab::GitLogger).to receive(:error).with("POST-RECEIVE: #{error_message}") - expect(described_class.new.perform(gl_repository, key_id, base64_changes)).to be(false) + expect(perform).to be(false) end end @@ -39,7 +43,7 @@ describe PostReceive do expect(Git::TagPushService).not_to receive(:new) expect_next_instance_of(SystemHooksService) { |service| expect(service).to receive(:execute_hooks) } - described_class.new.perform(gl_repository, key_id, "") + perform(changes: "") end end @@ -50,7 +54,7 @@ describe PostReceive do expect(Git::BranchPushService).not_to receive(:new) expect(Git::TagPushService).not_to receive(:new) - expect(described_class.new.perform(gl_repository, key_id, base64_changes)).to be false + expect(perform).to be false end end @@ -81,12 +85,24 @@ describe PostReceive do expect(Git::TagPushService).not_to receive(:new) - described_class.new.perform(gl_repository, key_id, base64_changes) + perform end end - context 'tags' do - let(:changes) { '123456 789012 refs/tags/tag' } + context "tags" do + let(:changes) do + <<~EOF + 654321 210987 refs/tags/tag1 + 654322 210986 refs/tags/tag2 + 654323 210985 refs/tags/tag3 + 654324 210984 refs/tags/tag4 + 654325 210983 refs/tags/tag5 + EOF + end + + before do + expect(Gitlab::GlRepository).to receive(:parse).and_return([project, Gitlab::GlRepository::PROJECT]) + end it 'does not expire branches cache' do expect(project.repository).not_to receive(:expire_branches_cache) @@ -94,14 +110,24 @@ describe PostReceive do described_class.new.perform(gl_repository, key_id, base64_changes) end - it 'calls Git::TagPushService' do - expect_next_instance_of(Git::TagPushService) do |service| + it "only invalidates tags once" do + expect(project.repository).to receive(:repository_event).exactly(5).times.with(:push_tag).and_call_original + expect(project.repository).to receive(:expire_caches_for_tags).once.and_call_original + expect(project.repository).to receive(:expire_tags_cache).once.and_call_original + + described_class.new.perform(gl_repository, key_id, base64_changes) + end + + it "calls Git::TagPushService" do + expect(Git::BranchPushService).not_to receive(:new) + + expect_any_instance_of(Git::TagPushService) do |service| expect(service).to receive(:execute).and_return(true) end expect(Git::BranchPushService).not_to receive(:new) - described_class.new.perform(gl_repository, key_id, base64_changes) + perform end end @@ -112,7 +138,7 @@ describe PostReceive do expect(Git::BranchPushService).not_to receive(:new) expect(Git::TagPushService).not_to receive(:new) - described_class.new.perform(gl_repository, key_id, base64_changes) + perform end end @@ -129,7 +155,7 @@ describe PostReceive do let(:changes_count) { changes.lines.count } - subject { described_class.new.perform(gl_repository, key_id, base64_changes) } + subject { perform } context "with valid .gitlab-ci.yml" do before do @@ -198,7 +224,13 @@ describe PostReceive do it 'calls SystemHooksService' do expect_any_instance_of(SystemHooksService).to receive(:execute_hooks).with(fake_hook_data, :repository_update_hooks).and_return(true) - described_class.new.perform(gl_repository, key_id, base64_changes) + perform + end + + it 'increments the usage data counter of pushes event' do + counter = Gitlab::UsageDataCounters::SourceCodeCounter + + expect { perform }.to change { counter.read(:pushes) }.by(1) end end end @@ -215,7 +247,7 @@ describe PostReceive do # a second to ensure we see changes. Timecop.freeze(1.second.from_now) do expect do - described_class.new.perform(gl_repository, key_id, base64_changes) + perform project.reload end.to change(project, :last_activity_at) .and change(project, :last_repository_updated_at) @@ -226,7 +258,8 @@ describe PostReceive do context "webhook" do it "fetches the correct project" do expect(Project).to receive(:find_by).with(id: project.id.to_s) - described_class.new.perform(gl_repository, key_id, base64_changes) + + perform end it "does not run if the author is not in the project" do @@ -236,7 +269,7 @@ describe PostReceive do expect(project).not_to receive(:execute_hooks) - expect(described_class.new.perform(gl_repository, key_id, base64_changes)).to be_falsey + expect(perform).to be_falsey end it "asks the project to trigger all hooks" do @@ -245,7 +278,7 @@ describe PostReceive do expect(project).to receive(:execute_hooks).twice expect(project).to receive(:execute_services).twice - described_class.new.perform(gl_repository, key_id, base64_changes) + perform end it "enqueues a UpdateMergeRequestsWorker job" do @@ -253,7 +286,7 @@ describe PostReceive do expect(UpdateMergeRequestsWorker).to receive(:perform_async).with(project.id, project.owner.id, any_args) - described_class.new.perform(gl_repository, key_id, base64_changes) + perform end end end diff --git a/spec/workers/repository_update_remote_mirror_worker_spec.rb b/spec/workers/repository_update_remote_mirror_worker_spec.rb index 4de51ecb3e9..66d517332ba 100644 --- a/spec/workers/repository_update_remote_mirror_worker_spec.rb +++ b/spec/workers/repository_update_remote_mirror_worker_spec.rb @@ -2,99 +2,70 @@ require 'rails_helper' -describe RepositoryUpdateRemoteMirrorWorker do +describe RepositoryUpdateRemoteMirrorWorker, :clean_gitlab_redis_shared_state do subject { described_class.new } - let(:remote_mirror) { create(:project, :repository, :remote_mirror).remote_mirrors.first } + let(:remote_mirror) { create(:remote_mirror) } let(:scheduled_time) { Time.now - 5.minutes } around do |example| Timecop.freeze(Time.now) { example.run } end - describe '#perform' do - context 'with status none' do - before do - remote_mirror.update(update_status: 'none') - end - - it 'sets status as finished when update remote mirror service executes successfully' do - expect_any_instance_of(Projects::UpdateRemoteMirrorService).to receive(:execute).with(remote_mirror).and_return(status: :success) - - expect { subject.perform(remote_mirror.id, Time.now) }.to change { remote_mirror.reload.update_status }.to('finished') - end - - it 'resets the notification flag upon success' do - expect_any_instance_of(Projects::UpdateRemoteMirrorService).to receive(:execute).with(remote_mirror).and_return(status: :success) - remote_mirror.update_column(:error_notification_sent, true) - - expect { subject.perform(remote_mirror.id, Time.now) }.to change { remote_mirror.reload.error_notification_sent }.to(false) - end - - it 'sets status as failed when update remote mirror service executes with errors' do - error_message = 'fail!' - - expect_next_instance_of(Projects::UpdateRemoteMirrorService) do |service| - expect(service).to receive(:execute).with(remote_mirror).and_return(status: :error, message: error_message) - end + def expect_mirror_service_to_return(mirror, result, tries = 0) + expect_next_instance_of(Projects::UpdateRemoteMirrorService) do |service| + expect(service).to receive(:execute).with(mirror, tries).and_return(result) + end + end - # Mock the finder so that it returns an object we can set expectations on - expect_next_instance_of(RemoteMirrorFinder) do |finder| - expect(finder).to receive(:execute).and_return(remote_mirror) - end - expect(remote_mirror).to receive(:mark_as_failed).with(error_message) + describe '#perform' do + it 'calls out to the service to perform the update' do + expect_mirror_service_to_return(remote_mirror, status: :success) - expect do - subject.perform(remote_mirror.id, Time.now) - end.to raise_error(RepositoryUpdateRemoteMirrorWorker::UpdateError, error_message) - end + subject.perform(remote_mirror.id, scheduled_time) + end - it 'does nothing if last_update_started_at is higher than the time the job was scheduled in' do - remote_mirror.update(last_update_started_at: Time.now) + it 'does not do anything if the mirror was already updated' do + remote_mirror.update(last_update_started_at: Time.now, update_status: :finished) - expect_any_instance_of(RemoteMirror).to receive(:updated_since?).with(scheduled_time).and_return(true) - expect_any_instance_of(Projects::UpdateRemoteMirrorService).not_to receive(:execute).with(remote_mirror) + expect(Projects::UpdateRemoteMirrorService).not_to receive(:new) - expect(subject.perform(remote_mirror.id, scheduled_time)).to be_nil - end + subject.perform(remote_mirror.id, scheduled_time) end - context 'with unexpected error' do - it 'marks mirror as failed' do - allow_any_instance_of(Projects::UpdateRemoteMirrorService).to receive(:execute).with(remote_mirror).and_raise(RuntimeError) + it 'schedules a retry when the mirror is marked for retrying' do + remote_mirror = create(:remote_mirror, update_status: :to_retry) + expect_mirror_service_to_return(remote_mirror, status: :error, message: 'Retry!') - expect do - subject.perform(remote_mirror.id, Time.now) - end.to raise_error(RepositoryUpdateRemoteMirrorWorker::UpdateError) - expect(remote_mirror.reload.update_status).to eq('failed') - end - end + expect(described_class) + .to receive(:perform_in) + .with(remote_mirror.backoff_delay, remote_mirror.id, scheduled_time, 1) - context 'with another worker already running' do - before do - remote_mirror.update(update_status: 'started') - end - - it 'raises RemoteMirrorUpdateAlreadyInProgressError' do - expect do - subject.perform(remote_mirror.id, Time.now) - end.to raise_error(RepositoryUpdateRemoteMirrorWorker::UpdateAlreadyInProgressError) - end + subject.perform(remote_mirror.id, scheduled_time) end - context 'with status failed' do - before do - remote_mirror.update(update_status: 'failed') + it 'clears the lease if there was an unexpected exception' do + expect_next_instance_of(Projects::UpdateRemoteMirrorService) do |service| + expect(service).to receive(:execute).with(remote_mirror, 1).and_raise('Unexpected!') end + expect { subject.perform(remote_mirror.id, Time.now, 1) }.to raise_error('Unexpected!') - it 'sets status as finished if last_update_started_at is higher than the time the job was scheduled in' do - remote_mirror.update(last_update_started_at: Time.now) + lease = Gitlab::ExclusiveLease.new("#{described_class.name}:#{remote_mirror.id}", timeout: 1.second) - expect_any_instance_of(RemoteMirror).to receive(:updated_since?).with(scheduled_time).and_return(false) - expect_any_instance_of(Projects::UpdateRemoteMirrorService).to receive(:execute).with(remote_mirror).and_return(status: :success) + expect(lease.try_obtain).not_to be_nil + end - expect { subject.perform(remote_mirror.id, scheduled_time) }.to change { remote_mirror.reload.update_status }.to('finished') - end + it 'retries 3 times for the worker to finish before rescheduling' do + expect(subject).to receive(:in_lock) + .with("#{described_class.name}:#{remote_mirror.id}", + retries: 3, + ttl: remote_mirror.max_runtime, + sleep_sec: described_class::LOCK_WAIT_TIME) + .and_raise(Gitlab::ExclusiveLeaseHelpers::FailedToObtainLockError) + expect(described_class).to receive(:perform_in) + .with(remote_mirror.backoff_delay, remote_mirror.id, scheduled_time, 0) + + subject.perform(remote_mirror.id, scheduled_time) end end end |