diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2020-01-11 18:07:49 +0300 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2020-01-11 18:07:49 +0300 |
commit | 90184b64bb3412cfd291b45c8997671cdb1ca95a (patch) | |
tree | 49accc148b0fa776d8a60c9e3a9224231b7ca393 | |
parent | bac547dc784170c7d0e6a5ae14d0ff5d549c31ee (diff) |
Add latest changes from gitlab-org/gitlab@master
-rw-r--r-- | Gemfile.lock | 3 | ||||
-rw-r--r-- | app/models/ci/pipeline_schedule.rb | 10 | ||||
-rw-r--r-- | app/models/concerns/schedulable.rb | 21 | ||||
-rw-r--r-- | app/models/container_expiration_policy.rb | 11 | ||||
-rw-r--r-- | app/services/container_expiration_policy_service.rb | 15 | ||||
-rw-r--r-- | app/workers/all_queues.yml | 1 | ||||
-rw-r--r-- | app/workers/container_expiration_policy_worker.rb | 16 | ||||
-rw-r--r-- | changelogs/unreleased/15398-recurring-job.yml | 5 | ||||
-rw-r--r-- | changelogs/unreleased/jl-bump-rack-cors-1-0-6.yml | 5 | ||||
-rw-r--r-- | config/initializers/1_settings.rb | 3 | ||||
-rw-r--r-- | spec/factories/container_expiration_policies.rb | 20 | ||||
-rw-r--r-- | spec/factories/projects.rb | 6 | ||||
-rw-r--r-- | spec/models/concerns/schedulable_spec.rb | 74 | ||||
-rw-r--r-- | spec/models/container_expiration_policy_spec.rb | 35 | ||||
-rw-r--r-- | spec/services/container_expiration_policy_service_spec.rb | 31 | ||||
-rw-r--r-- | spec/workers/container_expiration_policy_worker_spec.rb | 57 |
16 files changed, 303 insertions, 10 deletions
diff --git a/Gemfile.lock b/Gemfile.lock index 989502e40a1..13e1e4ac828 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -770,7 +770,8 @@ GEM rack (>= 0.4) rack-attack (6.2.0) rack (>= 1.0, < 3) - rack-cors (1.0.2) + rack-cors (1.0.6) + rack (>= 1.6.0) rack-oauth2 (1.9.3) activesupport attr_required diff --git a/app/models/ci/pipeline_schedule.rb b/app/models/ci/pipeline_schedule.rb index 946241b7d4c..9a1445e624c 100644 --- a/app/models/ci/pipeline_schedule.rb +++ b/app/models/ci/pipeline_schedule.rb @@ -5,6 +5,7 @@ module Ci extend Gitlab::Ci::Model include Importable include StripAttribute + include Schedulable belongs_to :project belongs_to :owner, class_name: 'User' @@ -18,13 +19,10 @@ module Ci validates :description, presence: true validates :variables, variable_duplicates: true - before_save :set_next_run_at - strip_attributes :cron scope :active, -> { where(active: true) } scope :inactive, -> { where(active: false) } - scope :runnable_schedules, -> { active.where("next_run_at < ?", Time.now) } scope :preloaded, -> { preload(:owner, :project) } accepts_nested_attributes_for :variables, allow_destroy: true @@ -62,12 +60,6 @@ module Ci end end - def schedule_next_run! - save! # with set_next_run_at - rescue ActiveRecord::RecordInvalid - update_column(:next_run_at, nil) # update without validation - end - def job_variables variables&.map(&:to_runner_variable) || [] end diff --git a/app/models/concerns/schedulable.rb b/app/models/concerns/schedulable.rb new file mode 100644 index 00000000000..6fdca4f50c3 --- /dev/null +++ b/app/models/concerns/schedulable.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +module Schedulable + extend ActiveSupport::Concern + + included do + scope :runnable_schedules, -> { active.where("next_run_at < ?", Time.zone.now) } + + before_save :set_next_run_at + end + + def schedule_next_run! + save! # with set_next_run_at + rescue ActiveRecord::RecordInvalid + update_column(:next_run_at, nil) # update without validation + end + + def set_next_run_at + raise NotImplementedError + end +end diff --git a/app/models/container_expiration_policy.rb b/app/models/container_expiration_policy.rb index f60a0179c83..c929a78a7f9 100644 --- a/app/models/container_expiration_policy.rb +++ b/app/models/container_expiration_policy.rb @@ -1,14 +1,21 @@ # frozen_string_literal: true class ContainerExpirationPolicy < ApplicationRecord + include Schedulable + belongs_to :project, inverse_of: :container_expiration_policy + delegate :container_repositories, to: :project + validates :project, presence: true validates :enabled, inclusion: { in: [true, false] } validates :cadence, presence: true, inclusion: { in: ->(_) { self.cadence_options.stringify_keys } } validates :older_than, inclusion: { in: ->(_) { self.older_than_options.stringify_keys } }, allow_nil: true validates :keep_n, inclusion: { in: ->(_) { self.keep_n_options.keys } }, allow_nil: true + scope :active, -> { where(enabled: true) } + scope :preloaded, -> { preload(:project) } + def self.keep_n_options { 1 => _('%{tags} tag per image name') % { tags: 1 }, @@ -38,4 +45,8 @@ class ContainerExpirationPolicy < ApplicationRecord '90d': _('%{days} days until tags are automatically removed') % { days: 90 } } end + + def set_next_run_at + self.next_run_at = Time.zone.now + ChronicDuration.parse(cadence).seconds + end end diff --git a/app/services/container_expiration_policy_service.rb b/app/services/container_expiration_policy_service.rb new file mode 100644 index 00000000000..5d141d4d64d --- /dev/null +++ b/app/services/container_expiration_policy_service.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +class ContainerExpirationPolicyService < BaseService + def execute(container_expiration_policy) + container_expiration_policy.schedule_next_run! + + container_expiration_policy.container_repositories.find_each do |container_repository| + CleanupContainerRepositoryWorker.perform_async( + current_user.id, + container_repository.id, + container_expiration_policy.attributes.except("created_at", "updated_at") + ) + end + end +end diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index 6afc326ed37..330e9fafa69 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -10,6 +10,7 @@ - chaos:chaos_sleep - cronjob:admin_email +- cronjob:container_expiration_policy - cronjob:expire_build_artifacts - cronjob:gitlab_usage_ping - cronjob:import_export_project_cleanup diff --git a/app/workers/container_expiration_policy_worker.rb b/app/workers/container_expiration_policy_worker.rb new file mode 100644 index 00000000000..595208230f6 --- /dev/null +++ b/app/workers/container_expiration_policy_worker.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +class ContainerExpirationPolicyWorker + include ApplicationWorker + include CronjobQueue + + feature_category :container_registry + + def perform + ContainerExpirationPolicy.runnable_schedules.preloaded.find_each do |container_expiration_policy| + ContainerExpirationPolicyService.new( + container_expiration_policy.project, container_expiration_policy.project.owner + ).execute(container_expiration_policy) + end + end +end diff --git a/changelogs/unreleased/15398-recurring-job.yml b/changelogs/unreleased/15398-recurring-job.yml new file mode 100644 index 00000000000..b19983e8183 --- /dev/null +++ b/changelogs/unreleased/15398-recurring-job.yml @@ -0,0 +1,5 @@ +--- +title: Add a cron job and worker to run the Container Expiration Policies +merge_request: 21593 +author: +type: added diff --git a/changelogs/unreleased/jl-bump-rack-cors-1-0-6.yml b/changelogs/unreleased/jl-bump-rack-cors-1-0-6.yml new file mode 100644 index 00000000000..70f03296768 --- /dev/null +++ b/changelogs/unreleased/jl-bump-rack-cors-1-0-6.yml @@ -0,0 +1,5 @@ +--- +title: Update rack-cors to 1.0.6 +merge_request: 22809 +author: +type: security diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index 487ce6e7a89..ce45dfe06f8 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -467,6 +467,9 @@ Settings.cron_jobs['schedule_migrate_external_diffs_worker']['job_class'] = 'Sch Settings.cron_jobs['namespaces_prune_aggregation_schedules_worker'] ||= Settingslogic.new({}) Settings.cron_jobs['namespaces_prune_aggregation_schedules_worker']['cron'] ||= '5 1 * * *' Settings.cron_jobs['namespaces_prune_aggregation_schedules_worker']['job_class'] = 'Namespaces::PruneAggregationSchedulesWorker' +Settings.cron_jobs['container_expiration_policy_worker'] ||= Settingslogic.new({}) +Settings.cron_jobs['container_expiration_policy_worker']['cron'] ||= '50 * * * *' +Settings.cron_jobs['container_expiration_policy_worker']['job_class'] = 'ContainerExpirationPolicyWorker' Gitlab.ee do Settings.cron_jobs['adjourned_group_deletion_worker'] ||= Settingslogic.new({}) diff --git a/spec/factories/container_expiration_policies.rb b/spec/factories/container_expiration_policies.rb new file mode 100644 index 00000000000..951127a4aa7 --- /dev/null +++ b/spec/factories/container_expiration_policies.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +FactoryBot.define do + factory :container_expiration_policy, class: 'ContainerExpirationPolicy' do + association :project, factory: [:project, :without_container_expiration_policy] + cadence { '1d' } + enabled { true } + + trait :runnable do + after(:create) do |policy| + # next_run_at will be set before_save to Time.now + cadence, so this ensures the policy is active + policy.update_column(:next_run_at, Time.zone.now - 1.day) + end + end + + trait :disabled do + enabled { false } + end + end +end diff --git a/spec/factories/projects.rb b/spec/factories/projects.rb index 2608f717f1c..46efc0111c0 100644 --- a/spec/factories/projects.rb +++ b/spec/factories/projects.rb @@ -137,6 +137,12 @@ FactoryBot.define do end end + trait :without_container_expiration_policy do + after(:build) do |project| + project.class.skip_callback(:create, :after, :create_container_expiration_policy, raise: false) + end + end + # Build a custom repository by specifying a hash of `filename => content` in # the transient `files` attribute. Each file will be created in its own # commit, operating against the master branch. So, the following call: diff --git a/spec/models/concerns/schedulable_spec.rb b/spec/models/concerns/schedulable_spec.rb new file mode 100644 index 00000000000..38ae2112e01 --- /dev/null +++ b/spec/models/concerns/schedulable_spec.rb @@ -0,0 +1,74 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Schedulable do + shared_examples 'before_save callback' do + it 'updates next_run_at' do + expect { object.save! }.to change { object.next_run_at } + end + end + + shared_examples '.runnable_schedules' do + it 'returns the runnable schedules' do + results = object.class.runnable_schedules + + expect(results).to include(object) + expect(results).not_to include(non_runnable_object) + end + end + + shared_examples '#schedule_next_run!' do + it 'saves the object and sets next_run_at' do + expect { object.schedule_next_run! }.to change { object.next_run_at } + end + + it 'sets next_run_at to nil on error' do + expect(object).to receive(:save!).and_raise(ActiveRecord::RecordInvalid) + + object.schedule_next_run! + + expect(object.next_run_at).to be_nil + end + end + + context 'for a pipeline_schedule' do + # let! is used to reset the next_run_at value before each spec + let(:object) do + Timecop.freeze(1.day.ago) do + create(:ci_pipeline_schedule, :hourly) + end + end + + let(:non_runnable_object) { create(:ci_pipeline_schedule, :hourly) } + + it_behaves_like '#schedule_next_run!' + it_behaves_like 'before_save callback' + it_behaves_like '.runnable_schedules' + end + + context 'for a container_expiration_policy' do + # let! is used to reset the next_run_at value before each spec + let(:object) { create(:container_expiration_policy, :runnable) } + let(:non_runnable_object) { create(:container_expiration_policy) } + + it_behaves_like '#schedule_next_run!' + it_behaves_like 'before_save callback' + it_behaves_like '.runnable_schedules' + end + + describe '#next_run_at' do + let(:schedulable_instance) do + Class.new(ActiveRecord::Base) do + include Schedulable + + # we need a table for the dummy class to operate + self.table_name = 'users' + end.new + end + + it 'works' do + expect { schedulable_instance.set_next_run_at }.to raise_error(NotImplementedError) + end + end +end diff --git a/spec/models/container_expiration_policy_spec.rb b/spec/models/container_expiration_policy_spec.rb index 1ce76490448..15096ae8a8b 100644 --- a/spec/models/container_expiration_policy_spec.rb +++ b/spec/models/container_expiration_policy_spec.rb @@ -38,4 +38,39 @@ RSpec.describe ContainerExpirationPolicy, type: :model do it { is_expected.not_to allow_value('foo').for(:keep_n) } end end + + describe '.preloaded' do + subject { described_class.preloaded } + + before do + # container_expiration_policies are created for every new project + create_list(:project, 3) + end + + it 'preloads the associations' do + subject + + query = ActiveRecord::QueryRecorder.new { subject.each(&:project) } + + expect(query.count).to eq(2) + end + end + + describe '.runnable_schedules' do + subject { described_class.runnable_schedules } + + let!(:policy) { create(:container_expiration_policy, :runnable) } + + it 'returns the runnable schedule' do + is_expected.to eq([policy]) + end + + context 'when there are no runnable schedules' do + let!(:policy) { } + + it 'returns an empty array' do + is_expected.to be_empty + end + end + end end diff --git a/spec/services/container_expiration_policy_service_spec.rb b/spec/services/container_expiration_policy_service_spec.rb new file mode 100644 index 00000000000..1e4899c627f --- /dev/null +++ b/spec/services/container_expiration_policy_service_spec.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe ContainerExpirationPolicyService do + let_it_be(:user) { create(:user) } + let_it_be(:container_expiration_policy) { create(:container_expiration_policy, :runnable) } + let(:project) { container_expiration_policy.project } + let(:container_repository) { create(:container_repository, project: project) } + + before do + project.add_maintainer(user) + end + + describe '#execute' do + subject { described_class.new(project, user).execute(container_expiration_policy) } + + it 'kicks off a cleanup worker for the container repository' do + expect(CleanupContainerRepositoryWorker).to receive(:perform_async) + .with(user.id, container_repository.id, anything) + + subject + end + + it 'sets next_run_at on the container_expiration_policy' do + subject + + expect(container_expiration_policy.next_run_at).to be > Time.zone.now + end + end +end diff --git a/spec/workers/container_expiration_policy_worker_spec.rb b/spec/workers/container_expiration_policy_worker_spec.rb new file mode 100644 index 00000000000..48ab1614633 --- /dev/null +++ b/spec/workers/container_expiration_policy_worker_spec.rb @@ -0,0 +1,57 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe ContainerExpirationPolicyWorker do + include ExclusiveLeaseHelpers + + subject { described_class.new.perform } + + context 'With no container expiration policies' do + it 'Does not execute any policies' do + expect(ContainerExpirationPolicyService).not_to receive(:new) + + subject + end + end + + context 'With container expiration policies' do + context 'a valid policy' do + let!(:container_expiration_policy) { create(:container_expiration_policy, :runnable) } + let(:user) { container_expiration_policy.project.owner } + + it 'runs the policy' do + service = instance_double(ContainerExpirationPolicyService, execute: true) + + expect(ContainerExpirationPolicyService) + .to receive(:new).with(container_expiration_policy.project, user).and_return(service) + + subject + end + end + + context 'a disabled policy' do + let!(:container_expiration_policy) { create(:container_expiration_policy, :runnable, :disabled) } + let(:user) {container_expiration_policy.project.owner } + + it 'does not run the policy' do + expect(ContainerExpirationPolicyService) + .not_to receive(:new).with(container_expiration_policy, user) + + subject + end + end + + context 'a policy that is not due for a run' do + let!(:container_expiration_policy) { create(:container_expiration_policy) } + let(:user) {container_expiration_policy.project.owner } + + it 'does not run the policy' do + expect(ContainerExpirationPolicyService) + .not_to receive(:new).with(container_expiration_policy, user) + + subject + end + end + end +end |