From 0a7455d6be1f96b4b00bf266e0f02760b7cd3c6f Mon Sep 17 00:00:00 2001 From: huzaifaiftikhar1 Date: Fri, 22 Apr 2022 15:39:58 +0530 Subject: [PATCH 1/7] Add inactive projects deletion feature Administrators of large GitLab instances often find that over time projects become inactive and are no longer used. These projects take up unnecessary disk space. This feature adds an automated way to identify these projects, warn the maintainers ahead of time, and then delete the projects if they are still inactive. Changelog: added --- app/models/application_setting.rb | 7 + app/models/event.rb | 4 + app/models/project.rb | 12 ++ app/workers/all_queues.yml | 18 +++ .../inactive_projects_deletion_cron_worker.rb | 82 ++++++++++ ...e_projects_deletion_notification_worker.rb | 34 ++++ .../inactive_projects_deletion.yml | 8 + config/initializers/1_settings.rb | 3 + config/sidekiq_queues.yml | 2 + ee/app/models/ee/project.rb | 16 ++ .../inactive_projects_deletion_cron_worker.rb | 24 +++ ee/spec/models/project_spec.rb | 100 ++++++++++-- ...tive_projects_deletion_cron_worker_spec.rb | 149 ++++++++++++++++++ spec/models/application_setting_spec.rb | 9 ++ spec/models/event_spec.rb | 19 ++- spec/models/project_spec.rb | 43 ++++- spec/support/helpers/project_helpers.rb | 7 + spec/workers/every_sidekiq_worker_spec.rb | 1 + ...tive_projects_deletion_cron_worker_spec.rb | 148 +++++++++++++++++ ...jects_deletion_notification_worker_spec.rb | 42 +++++ 20 files changed, 711 insertions(+), 17 deletions(-) create mode 100644 app/workers/projects/inactive_projects_deletion_cron_worker.rb create mode 100644 app/workers/projects/inactive_projects_deletion_notification_worker.rb create mode 100644 config/feature_flags/development/inactive_projects_deletion.yml create mode 100644 ee/app/workers/ee/projects/inactive_projects_deletion_cron_worker.rb create mode 100644 ee/spec/workers/ee/projects/inactive_projects_deletion_cron_worker_spec.rb create mode 100644 spec/workers/projects/inactive_projects_deletion_cron_worker_spec.rb create mode 100644 spec/workers/projects/inactive_projects_deletion_notification_worker_spec.rb diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index bf68101934b2a6..8b337d70864cb1 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -645,6 +645,7 @@ def self.kroki_formats_attributes reset_memoized_terms end after_commit :expire_performance_bar_allowed_user_ids_cache, if: -> { previous_changes.key?('performance_bar_allowed_group_id') } + after_commit :reset_deletion_warning_redis_key, if: :saved_change_to_inactive_projects_delete_after_months? def validate_grafana_url validate_url(parsed_grafana_url, :grafana_url, GRAFANA_URL_ERROR_MESSAGE) @@ -775,6 +776,12 @@ def validate_url(parsed_url, name, error_message) ) end end + + def reset_deletion_warning_redis_key + Gitlab::Redis::SharedState.with do |redis| + redis.del('inactive_projects_deletion_warning_email_notified') + end + end end ApplicationSetting.prepend_mod_with('ApplicationSetting') diff --git a/app/models/event.rb b/app/models/event.rb index e9a98c06b59685..cdbd2d2169ada0 100644 --- a/app/models/event.rb +++ b/app/models/event.rb @@ -357,6 +357,10 @@ def reset_project_activity Project.unscoped.where(id: project_id) .where('last_activity_at <= ?', RESET_PROJECT_ACTIVITY_INTERVAL.ago) .touch_all(:last_activity_at, time: created_at) # rubocop: disable Rails/SkipsModelValidations + + Gitlab::Redis::SharedState.with do |redis| + redis.hdel('inactive_projects_deletion_warning_email_notified', "project:#{project.id}") + end end def authored_by?(user) diff --git a/app/models/project.rb b/app/models/project.rb index 027b2257f2e13b..5d86798759ec2b 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -745,6 +745,16 @@ def self.wrap_with_cte(collection) Project.with(cte.to_arel).from(cte.alias_to(Project.arel_table)) end + def self.inactive + project_statistics = ::ProjectStatistics.arel_table + minimum_size_mb = ::Gitlab::CurrentSettings.inactive_projects_min_size_mb.megabytes + last_activity_cutoff = ::Gitlab::CurrentSettings.inactive_projects_send_warning_email_after_months.months.ago + + joins(:statistics) + .where((project_statistics[:storage_size]).gt(minimum_size_mb)) + .where('last_activity_at < ?', last_activity_cutoff) + end + scope :active, -> { joins(:issues, :notes, :merge_requests).order('issues.created_at, notes.created_at, merge_requests.created_at DESC') } scope :abandoned, -> { where('projects.last_activity_at < ?', 6.months.ago) } @@ -2886,6 +2896,8 @@ def enqueue_record_project_target_platforms end def inactive? + return false unless Feature.enabled?(:inactive_projects_deletion, root_namespace, default_enabled: :yaml) + (statistics || build_statistics).storage_size > ::Gitlab::CurrentSettings.inactive_projects_min_size_mb.megabytes && last_activity_at < ::Gitlab::CurrentSettings.inactive_projects_send_warning_email_after_months.months.ago end diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index c436335a24484f..28253f1c1d2001 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -579,6 +579,15 @@ :weight: 1 :idempotent: :tags: [] +- :name: cronjob:projects_inactive_projects_deletion_cron + :worker_name: Projects::InactiveProjectsDeletionCronWorker + :feature_category: :source_code_management + :has_external_dependencies: + :urgency: :low + :resource_boundary: :unknown + :weight: 1 + :idempotent: + :tags: [] - :name: cronjob:projects_schedule_refresh_build_artifacts_size_statistics :worker_name: Projects::ScheduleRefreshBuildArtifactsSizeStatisticsWorker :feature_category: :build_artifacts @@ -2794,6 +2803,15 @@ :weight: 1 :idempotent: :tags: [] +- :name: projects_inactive_projects_deletion_notification + :worker_name: Projects::InactiveProjectsDeletionNotificationWorker + :feature_category: :source_code_management + :has_external_dependencies: + :urgency: :low + :resource_boundary: :unknown + :weight: 1 + :idempotent: + :tags: [] - :name: projects_post_creation :worker_name: Projects::PostCreationWorker :feature_category: :source_code_management diff --git a/app/workers/projects/inactive_projects_deletion_cron_worker.rb b/app/workers/projects/inactive_projects_deletion_cron_worker.rb new file mode 100644 index 00000000000000..be500e415968c4 --- /dev/null +++ b/app/workers/projects/inactive_projects_deletion_cron_worker.rb @@ -0,0 +1,82 @@ +# frozen_string_literal: true + +module Projects + class InactiveProjectsDeletionCronWorker # rubocop:disable Scalability/IdempotentWorker + include ApplicationWorker + include Gitlab::Utils::StrongMemoize + include CronjobQueue + + data_consistency :always + feature_category :source_code_management + + INTERVAL = 2.seconds.to_i + + def perform + return unless ::Gitlab::CurrentSettings.delete_inactive_projects? + + admin_user = User.admins.humans.active.first + + notified_inactive_projects = deletion_warning_notified_projects + + Project.inactive.without_deleted.find_each(batch_size: 100).with_index do |project, index| # rubocop: disable CodeReuse/ActiveRecord + next unless Feature.enabled?(:inactive_projects_deletion, project.root_namespace, default_enabled: :yaml) + + delay = index * INTERVAL + + with_context(project: project, user: admin_user) do + deletion_warning_email_sent_on = notified_inactive_projects["project:#{project.id}"] + + if send_deletion_warning_email?(deletion_warning_email_sent_on, project) + ::Projects::InactiveProjectsDeletionNotificationWorker.perform_in(delay, project.id, deletion_date) + elsif deletion_warning_email_sent_on && delete_due_to_inactivity?(deletion_warning_email_sent_on) + delete_redis_entry(project) + delete_project(project, admin_user) + end + end + end + end + + private + + # Redis key 'inactive_projects_deletion_warning_email_notified' is a hash. It stores the date when the + # deletion warning notification email was sent for an inactive project. The fields and values look like: + # {"project:1"=>"2022-04-22", "project:5"=>"2022-04-22", "project:7"=>"2022-04-25"} + # @return [Hash] + def deletion_warning_notified_projects + Gitlab::Redis::SharedState.with do |redis| + redis.hgetall('inactive_projects_deletion_warning_email_notified') + end + end + + def grace_months_after_deletion_notification + strong_memoize(:grace_months_after_deletion_notification) do + (::Gitlab::CurrentSettings.inactive_projects_delete_after_months - + ::Gitlab::CurrentSettings.inactive_projects_send_warning_email_after_months).months + end + end + + def send_deletion_warning_email?(deletion_warning_email_sent_on, project) + deletion_warning_email_sent_on.blank? + end + + def delete_due_to_inactivity?(deletion_warning_email_sent_on) + deletion_warning_email_sent_on < grace_months_after_deletion_notification.ago + end + + def deletion_date + Date.parse(grace_months_after_deletion_notification.from_now.to_s).to_s + end + + def delete_project(project, user) + ::Projects::DestroyService.new(project, user, {}).async_execute + end + + def delete_redis_entry(project) + Gitlab::Redis::SharedState.with do |redis| + redis.hdel('inactive_projects_deletion_warning_email_notified', "project:#{project.id}") + end + end + end +end + +Projects::InactiveProjectsDeletionCronWorker.prepend_mod diff --git a/app/workers/projects/inactive_projects_deletion_notification_worker.rb b/app/workers/projects/inactive_projects_deletion_notification_worker.rb new file mode 100644 index 00000000000000..f4a1590c5110fe --- /dev/null +++ b/app/workers/projects/inactive_projects_deletion_notification_worker.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +module Projects + class InactiveProjectsDeletionNotificationWorker # rubocop:disable Scalability/IdempotentWorker + include ApplicationWorker + include ExceptionBacktrace + + data_consistency :always + sidekiq_options retry: 3 + feature_category :source_code_management + + def perform(project_id, deletion_date) + project = Project.find(project_id) + + notification_service.inactive_project_deletion_warning(project, deletion_date) + + update_deletion_warning_notified_projects(project_id) + rescue ActiveRecord::RecordNotFound => error + Gitlab::ErrorTracking.log_exception(error, project_id: project_id) + end + + private + + def notification_service + @notification_service ||= NotificationService.new + end + + def update_deletion_warning_notified_projects(project_id) + Gitlab::Redis::SharedState.with do |redis| + redis.hset('inactive_projects_deletion_warning_email_notified', "project:#{project_id}", Date.current) + end + end + end +end diff --git a/config/feature_flags/development/inactive_projects_deletion.yml b/config/feature_flags/development/inactive_projects_deletion.yml new file mode 100644 index 00000000000000..e9bb91f62cc174 --- /dev/null +++ b/config/feature_flags/development/inactive_projects_deletion.yml @@ -0,0 +1,8 @@ +--- +name: inactive_projects_deletion +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/85689 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/357968 +milestone: '15.0' +type: development +group: group::compliance +default_enabled: false diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index 24547c78d9e4d9..5b15b119b3aeb3 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -626,6 +626,9 @@ Settings.cron_jobs['projects_schedule_refresh_build_artifacts_size_statistics_worker'] ||= Settingslogic.new({}) Settings.cron_jobs['projects_schedule_refresh_build_artifacts_size_statistics_worker']['cron'] ||= '2/17 * * * *' Settings.cron_jobs['projects_schedule_refresh_build_artifacts_size_statistics_worker']['job_class'] = 'Projects::ScheduleRefreshBuildArtifactsSizeStatisticsWorker' +Settings.cron_jobs['inactive_projects_deletion_cron_worker'] ||= Settingslogic.new({}) +Settings.cron_jobs['inactive_projects_deletion_cron_worker']['cron'] ||= '0 1 * * *' +Settings.cron_jobs['inactive_projects_deletion_cron_worker']['job_class'] = 'Projects::InactiveProjectsDeletionCronWorker' Gitlab.ee do Settings.cron_jobs['analytics_devops_adoption_create_all_snapshots_worker'] ||= Settingslogic.new({}) diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml index f3df7608052225..ddaaaf05dbdd6a 100644 --- a/config/sidekiq_queues.yml +++ b/config/sidekiq_queues.yml @@ -359,6 +359,8 @@ - 1 - - projects_git_garbage_collect - 1 +- - projects_inactive_projects_deletion_notification + - 1 - - projects_post_creation - 1 - - projects_process_sync_events diff --git a/ee/app/models/ee/project.rb b/ee/app/models/ee/project.rb index eaf53d395fd952..db2820edd746ed 100644 --- a/ee/app/models/ee/project.rb +++ b/ee/app/models/ee/project.rb @@ -308,6 +308,20 @@ def with_web_entity_associations def with_api_entity_associations super.preload(group: [:ip_restrictions, :saml_provider]) end + + override :inactive + def inactive + return super unless ::Gitlab.com? + + statistics = ::ProjectStatistics.arel_table + minimum_size_mb = ::Gitlab::CurrentSettings.inactive_projects_min_size_mb.megabytes + last_activity_cutoff = ::Gitlab::CurrentSettings.inactive_projects_send_warning_email_after_months.months.ago + + for_plan_name(::Plan.default_plans) + .joins(:statistics) + .where((statistics[:storage_size]).gt(minimum_size_mb)) + .where('last_activity_at < ?', last_activity_cutoff) + end end def can_store_security_reports? @@ -858,6 +872,8 @@ def all_security_orchestration_policy_configurations override :inactive? def inactive? + return false unless ::Feature.enabled?(:inactive_projects_deletion, root_namespace, default_enabled: :yaml) + ::Gitlab.com? && root_namespace.paid? ? false : super end diff --git a/ee/app/workers/ee/projects/inactive_projects_deletion_cron_worker.rb b/ee/app/workers/ee/projects/inactive_projects_deletion_cron_worker.rb new file mode 100644 index 00000000000000..92b04dd76bd849 --- /dev/null +++ b/ee/app/workers/ee/projects/inactive_projects_deletion_cron_worker.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +module EE + module Projects + module InactiveProjectsDeletionCronWorker + extend ::Gitlab::Utils::Override + + override :delete_project + def delete_project(project, user) + return super unless License.feature_available?(:adjourned_deletion_for_projects_and_groups) + return super unless project.adjourned_deletion_configured? + + ::Projects::MarkForDeletionService.new(project, user, {}).execute + end + + override :send_deletion_warning_email? + def send_deletion_warning_email?(deletion_warning_email_sent_on, project) + return false if project.marked_for_deletion_at? + + super + end + end + end +end diff --git a/ee/spec/models/project_spec.rb b/ee/spec/models/project_spec.rb index 99b707f9f87647..47564950e72eb9 100644 --- a/ee/spec/models/project_spec.rb +++ b/ee/spec/models/project_spec.rb @@ -5,6 +5,7 @@ RSpec.describe Project do include ProjectForksHelper include ::EE::GeoHelpers + include ::ProjectHelpers using RSpec::Parameterized::TableSyntax let(:project) { create(:project) } @@ -3336,33 +3337,102 @@ def stub_default_url_options(host) end describe '#inactive?' do - context 'when Gitlab.com', :saas do - context 'when project belongs to paid namespace' do - before do - stub_application_setting(inactive_projects_min_size_mb: 5) - stub_application_setting(inactive_projects_send_warning_email_after_months: 24) + context 'when feature flag is disabled' do + before do + stub_feature_flags(inactive_projects_deletion: false) + end + + it 'returns false' do + expect(project.inactive?).to eq(false) + end + end + + context 'when feature flag is enabled' do + before do + stub_feature_flags(inactive_projects_deletion: true) + end + + context 'when Gitlab.com', :saas do + context 'when project belongs to paid namespace' do + before do + stub_application_setting(inactive_projects_min_size_mb: 5) + stub_application_setting(inactive_projects_send_warning_email_after_months: 24) + end + + it 'returns false' do + ultimate_group = create(:group_with_plan, plan: :ultimate_plan) + ultimate_project = create(:project, last_activity_at: 3.years.ago, namespace: ultimate_group) + + expect(ultimate_project.inactive?).to eq(false) + end end - it 'returns false' do - ultimate_group = create(:group_with_plan, plan: :ultimate_plan) - ultimate_project = create(:project, last_activity_at: 3.years.ago, namespace: ultimate_group) + context 'when project belongs to free namespace' do + let_it_be(:no_plan_group) { create(:group_with_plan, plan: nil) } + let_it_be_with_reload(:project) { create(:project, namespace: no_plan_group) } - expect(ultimate_project.inactive?).to eq(false) + it_behaves_like 'returns true if project is inactive' end end - context 'when project belongs to free namespace' do - let_it_be(:no_plan_group) { create(:group_with_plan, plan: nil) } - let_it_be_with_reload(:project) { create(:project, namespace: no_plan_group) } + context 'when not Gitlab.com' do + let_it_be_with_reload(:project) { create(:project, name: 'test-project') } it_behaves_like 'returns true if project is inactive' end end + end + + describe '.inactive', :saas do + before do + stub_application_setting(inactive_projects_min_size_mb: 5) + stub_application_setting(inactive_projects_send_warning_email_after_months: 24) + end + + it 'returns inactive projects belonging to free namespace' do + ultimate_group = create(:group_with_plan, plan: :ultimate_plan) + premium_group = create(:group_with_plan, plan: :premium_plan) + free_plan_group = create(:group_with_plan, plan: :free_plan) + + free_small_active_project = + create_project_with_statistics(free_plan_group, with_data: true, size_multiplier: 1.kilobyte).tap do |project| + project.update!(last_activity_at: 7.days.ago) + end + + free_small_inactive_project = + create_project_with_statistics(free_plan_group, with_data: true, size_multiplier: 1.kilobyte).tap do |project| + project.update!(last_activity_at: 3.years.ago) + end - context 'when not Gitlab.com' do - let_it_be_with_reload(:project) { create(:project, name: 'test-project') } + free_large_inactive_project = + create_project_with_statistics(free_plan_group, with_data: true, size_multiplier: 10.megabytes).tap do |project| + project.update!(last_activity_at: 3.years.ago) + end + + free_large_active_project = + create_project_with_statistics(free_plan_group, with_data: true, size_multiplier: 10.megabytes).tap do |project| + project.update!(last_activity_at: 7.days.ago) + end + + paid_small_active_project = + create_project_with_statistics(premium_group, with_data: true, size_multiplier: 1.megabyte).tap do |project| + project.update!(last_activity_at: 7.days.ago) + end + + paid_small_inactive_project = + create_project_with_statistics(premium_group, with_data: true, size_multiplier: 1.megabyte).tap do |project| + project.update!(last_activity_at: 7.years.ago) + end + + paid_large_inactive_project = + create_project_with_statistics(ultimate_group, with_data: true, size_multiplier: 1.gigabyte).tap do |project| + project.update!(last_activity_at: 7.years.ago) + end - it_behaves_like 'returns true if project is inactive' + expect(described_class.inactive).to contain_exactly(free_large_inactive_project) + expect(described_class.inactive) + .not_to include(free_small_active_project, free_small_inactive_project, free_large_active_project, + paid_small_active_project, paid_small_inactive_project, paid_large_inactive_project) end end end diff --git a/ee/spec/workers/ee/projects/inactive_projects_deletion_cron_worker_spec.rb b/ee/spec/workers/ee/projects/inactive_projects_deletion_cron_worker_spec.rb new file mode 100644 index 00000000000000..b396c9a287cbfc --- /dev/null +++ b/ee/spec/workers/ee/projects/inactive_projects_deletion_cron_worker_spec.rb @@ -0,0 +1,149 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Projects::InactiveProjectsDeletionCronWorker do + include ProjectHelpers + + describe "#perform", :clean_gitlab_redis_shared_state, :sidekiq_inline do + subject(:worker) { described_class.new } + + let_it_be(:admin_user) { create(:user, :admin) } + let_it_be(:non_admin_user) { create(:user) } + let_it_be(:group) { create(:group) } + let_it_be(:new_blank_project) do + create_project_with_statistics.tap do |project| + project.update!(last_activity_at: Time.current) + end + end + + let_it_be(:inactive_blank_project) do + create_project_with_statistics.tap do |project| + project.update!(last_activity_at: 13.months.ago) + end + end + + let_it_be(:inactive_large_project) do + create_project_with_statistics(group, with_data: true, size_multiplier: 2.gigabytes) + .tap { |project| project.update!(last_activity_at: 2.years.ago) } + end + + let_it_be(:active_large_project) do + create_project_with_statistics(group, with_data: true, size_multiplier: 2.gigabytes) + .tap { |project| project.update!(last_activity_at: 1.month.ago) } + end + + before do + stub_application_setting(inactive_projects_min_size_mb: 5) + stub_application_setting(inactive_projects_send_warning_email_after_months: 12) + stub_application_setting(inactive_projects_delete_after_months: 14) + stub_application_setting(deletion_adjourned_period: 7) + stub_application_setting(delete_inactive_projects: true) + stub_feature_flags(inactive_projects_deletion: true) + end + + it 'does not send deletion warning email for inactive projects that are already marked for deletion' do + inactive_large_project.update!(marked_for_deletion_at: Date.current) + + expect(::Projects::InactiveProjectsDeletionNotificationWorker).not_to receive(:perform_in) + expect(::Projects::DestroyService).not_to receive(:new) + expect(::Projects::MarkForDeletionService).not_to receive(:perform_in) + + worker.perform + + Gitlab::Redis::SharedState.with do |redis| + expect(redis.hget('inactive_projects_deletion_warning_email_notified', + "project:#{inactive_large_project.id}")).to be_nil + end + end + + context 'when adjourned_deletion_for_projects_and_groups feature is not available' do + before do + stub_licensed_features(adjourned_deletion_for_projects_and_groups: false) + end + + it 'invokes Projects::DestroyService for projects that are inactive even after being notified' do + Gitlab::Redis::SharedState.with do |redis| + redis.hset('inactive_projects_deletion_warning_email_notified', "project:#{inactive_large_project.id}", + 15.months.ago) + end + + expect(::Projects::InactiveProjectsDeletionNotificationWorker).not_to receive(:perform_in) + expect(::Projects::MarkForDeletionService).not_to receive(:perform_in) + expect(::Projects::DestroyService).to receive(:new).with(inactive_large_project, admin_user, {}) + .at_least(:once).and_call_original + + worker.perform + + expect(inactive_large_project.reload.pending_delete).to eq(true) + expect(inactive_large_project.reload.marked_for_deletion_at).to be_nil + + Gitlab::Redis::SharedState.with do |redis| + expect(redis.hget('inactive_projects_deletion_warning_email_notified', + "project:#{inactive_large_project.id}")).to be_nil + end + end + end + + context 'when adjourned_deletion_for_projects_and_groups feature is available' do + before do + stub_licensed_features(adjourned_deletion_for_projects_and_groups: true) + end + + context 'when adjourned_deletion_configured is not configured for the project' do + before do + group.namespace_settings.update!(delayed_project_removal: false) + end + + it 'invokes Projects::DestroyService if adjourned_deletion_configured not configured for the project' do + Gitlab::Redis::SharedState.with do |redis| + redis.hset('inactive_projects_deletion_warning_email_notified', "project:#{inactive_large_project.id}", + 15.months.ago) + end + + expect(::Projects::InactiveProjectsDeletionNotificationWorker).not_to receive(:perform_in) + expect(::Projects::MarkForDeletionService).not_to receive(:perform_in) + expect(::Projects::DestroyService).to receive(:new).with(inactive_large_project, admin_user, {}) + .at_least(:once).and_call_original + + worker.perform + + expect(inactive_large_project.reload.pending_delete).to eq(true) + expect(inactive_large_project.reload.marked_for_deletion_at).to be_nil + + Gitlab::Redis::SharedState.with do |redis| + expect(redis.hget('inactive_projects_deletion_warning_email_notified', + "project:#{inactive_large_project.id}")).to be_nil + end + end + end + + context 'when adjourned_deletion_configured is configured for the project' do + before do + group.namespace_settings.update!(delayed_project_removal: true) + end + + it 'invokes Projects::MarkForDeletionService for projects that are inactive even after being notified' do + Gitlab::Redis::SharedState.with do |redis| + redis.hset('inactive_projects_deletion_warning_email_notified', "project:#{inactive_large_project.id}", + 15.months.ago) + end + + expect(::Projects::InactiveProjectsDeletionNotificationWorker).not_to receive(:perform_in) + expect(::Projects::MarkForDeletionService).to receive(:new).with(inactive_large_project, admin_user, {}) + .and_call_original + + worker.perform + + expect(inactive_large_project.reload.pending_delete).to eq(false) + expect(inactive_large_project.reload.marked_for_deletion_at).not_to be_nil + + Gitlab::Redis::SharedState.with do |redis| + expect(redis.hget('inactive_projects_deletion_warning_email_notified', + "project:#{inactive_large_project.id}")).to be_nil + end + end + end + end + end +end diff --git a/spec/models/application_setting_spec.rb b/spec/models/application_setting_spec.rb index 9f0f056d14e283..13cec5743caaf8 100644 --- a/spec/models/application_setting_spec.rb +++ b/spec/models/application_setting_spec.rb @@ -1344,5 +1344,14 @@ def expect_invalid it { is_expected.to validate_numericality_of(:inactive_projects_delete_after_months).is_greater_than(0) } it { is_expected.to validate_numericality_of(:inactive_projects_min_size_mb).is_greater_than_or_equal_to(0) } + + it "deletes the redis key used for tracking inactive projects deletion warning emails when setting is updated", + :clean_gitlab_redis_shared_state do + Gitlab::Redis::SharedState.with do |redis| + expect(redis).to receive(:del).with("inactive_projects_deletion_warning_email_notified") + end + + setting.update!(inactive_projects_delete_after_months: 6) + end end end diff --git a/spec/models/event_spec.rb b/spec/models/event_spec.rb index f099015e63e82a..8f61369958e28e 100644 --- a/spec/models/event_spec.rb +++ b/spec/models/event_spec.rb @@ -834,7 +834,13 @@ def visible_to_all_except(*roles) end end - context 'when a project was updated more than 1 hour ago' do + context 'when a project was updated more than 1 hour ago', :clean_gitlab_redis_shared_state do + before do + ::Gitlab::Redis::SharedState.with do |redis| + redis.hset('inactive_projects_deletion_warning_email_notified', "project:#{project.id}", Date.current) + end + end + it 'updates the project' do project.touch(:last_activity_at, time: 1.year.ago) # rubocop: disable Rails/SkipsModelValidations @@ -845,6 +851,17 @@ def visible_to_all_except(*roles) expect(project.last_activity_at).to be_like_time(event.created_at) expect(project.updated_at).to be_like_time(event.created_at) end + + it "deletes the redis key for if the project was inactive" do + Gitlab::Redis::SharedState.with do |redis| + expect(redis).to receive(:hdel).with('inactive_projects_deletion_warning_email_notified', + "project:#{project.id}") + end + + project.touch(:last_activity_at, time: 1.year.ago) # rubocop: disable Rails/SkipsModelValidations + + create_push_event(project, project.first_owner) + end end end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 7b01c653981bd5..7fa0caeda54b16 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -8,6 +8,7 @@ include ExternalAuthorizationServiceHelpers include ReloadHelpers include StubGitlabCalls + include ProjectHelpers using RSpec::Parameterized::TableSyntax let_it_be(:namespace) { create_default(:namespace).freeze } @@ -8264,7 +8265,47 @@ def has_external_wiki describe '#inactive?' do let_it_be_with_reload(:project) { create(:project, name: 'test-project') } - it_behaves_like 'returns true if project is inactive' + context 'when feature flag is enabled' do + before do + stub_feature_flags(inactive_projects_deletion: true) + end + + it_behaves_like 'returns true if project is inactive' + end + + context 'when feature flag is disabled' do + before do + stub_feature_flags(inactive_projects_deletion: false) + end + + it 'returns false' do + expect(project.inactive?).to eq(false) + end + end + end + + describe '.inactive' do + before do + stub_application_setting(inactive_projects_min_size_mb: 5) + stub_application_setting(inactive_projects_send_warning_email_after_months: 12) + end + + it 'returns projects that are inactive' do + new_blank_project = create_project_with_statistics.tap do |project| + project.update!(last_activity_at: Time.current) + end + inactive_blank_project = create_project_with_statistics.tap do |project| + project.update!(last_activity_at: 13.months.ago) + end + inactive_large_project = create_project_with_statistics(with_data: true, size_multiplier: 2.gigabytes) + .tap { |project| project.update!(last_activity_at: 2.years.ago) } + active_large_project = create_project_with_statistics(with_data: true, size_multiplier: 2.gigabytes) + .tap { |project| project.update!(last_activity_at: 1.month.ago) } + + expect(described_class.inactive).to contain_exactly(inactive_large_project) + expect(described_class.inactive) + .not_to include(new_blank_project, inactive_blank_project, active_large_project) + end end private diff --git a/spec/support/helpers/project_helpers.rb b/spec/support/helpers/project_helpers.rb index 89f0163b4b65ac..9e45f284951dd0 100644 --- a/spec/support/helpers/project_helpers.rb +++ b/spec/support/helpers/project_helpers.rb @@ -24,4 +24,11 @@ def update_feature_access_level(project, access_level) project.update!(params) end + + def create_project_with_statistics(namespace = nil, with_data: false, size_multiplier: 1) + project = namespace.present? ? create(:project, namespace: namespace) : create(:project) + project.tap do |p| + create(:project_statistics, project: p, with_data: with_data, size_multiplier: size_multiplier) + end + end end diff --git a/spec/workers/every_sidekiq_worker_spec.rb b/spec/workers/every_sidekiq_worker_spec.rb index adee70fbf87fdb..ca858bcba193e9 100644 --- a/spec/workers/every_sidekiq_worker_spec.rb +++ b/spec/workers/every_sidekiq_worker_spec.rb @@ -389,6 +389,7 @@ 'ProjectTemplateExportWorker' => false, 'ProjectUpdateRepositoryStorageWorker' => 3, 'Projects::GitGarbageCollectWorker' => false, + 'Projects::InactiveProjectsDeletionNotificationWorker' => 3, 'Projects::PostCreationWorker' => 3, 'Projects::ScheduleBulkRepositoryShardMovesWorker' => 3, 'Projects::UpdateRepositoryStorageWorker' => 3, diff --git a/spec/workers/projects/inactive_projects_deletion_cron_worker_spec.rb b/spec/workers/projects/inactive_projects_deletion_cron_worker_spec.rb new file mode 100644 index 00000000000000..2d34ff18473760 --- /dev/null +++ b/spec/workers/projects/inactive_projects_deletion_cron_worker_spec.rb @@ -0,0 +1,148 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Projects::InactiveProjectsDeletionCronWorker do + include ProjectHelpers + + describe "#perform" do + subject(:worker) { described_class.new } + + let_it_be(:admin_user) { create(:user, :admin) } + let_it_be(:non_admin_user) { create(:user) } + let_it_be(:new_blank_project) do + create_project_with_statistics.tap do |project| + project.update!(last_activity_at: Time.current) + end + end + + let_it_be(:inactive_blank_project) do + create_project_with_statistics.tap do |project| + project.update!(last_activity_at: 13.months.ago) + end + end + + let_it_be(:inactive_large_project) do + create_project_with_statistics(with_data: true, size_multiplier: 2.gigabytes) + .tap { |project| project.update!(last_activity_at: 2.years.ago) } + end + + let_it_be(:active_large_project) do + create_project_with_statistics(with_data: true, size_multiplier: 2.gigabytes) + .tap { |project| project.update!(last_activity_at: 1.month.ago) } + end + + before do + stub_application_setting(inactive_projects_min_size_mb: 5) + stub_application_setting(inactive_projects_send_warning_email_after_months: 12) + stub_application_setting(inactive_projects_delete_after_months: 14) + end + + context 'when delete inactive projects feature is disabled' do + before do + stub_application_setting(delete_inactive_projects: false) + end + + it 'does not invoke Projects::InactiveProjectsDeletionNotificationWorker' do + expect(::Projects::InactiveProjectsDeletionNotificationWorker).not_to receive(:perform_in) + expect(::Projects::DestroyService).not_to receive(:new) + + worker.perform + end + + it 'does not delete the inactive projects' do + worker.perform + + expect(inactive_large_project.reload.pending_delete).to eq(false) + end + end + + context 'when delete inactive projects feature is enabled' do + before do + stub_application_setting(delete_inactive_projects: true) + end + + context 'when feature flag is disabled' do + before do + stub_feature_flags(inactive_projects_deletion: false) + end + + it 'does not invoke Projects::InactiveProjectsDeletionNotificationWorker' do + expect(::Projects::InactiveProjectsDeletionNotificationWorker).not_to receive(:perform_in) + expect(::Projects::DestroyService).not_to receive(:new) + + worker.perform + end + + it 'does not delete the inactive projects' do + worker.perform + + expect(inactive_large_project.reload.pending_delete).to eq(false) + end + end + + context 'when feature flag is enabled', :clean_gitlab_redis_shared_state, :sidekiq_inline do + let_it_be(:delay) { anything } + + before do + stub_feature_flags(inactive_projects_deletion: true) + end + + it 'invokes Projects::InactiveProjectsDeletionNotificationWorker for inactive projects' do + Gitlab::Redis::SharedState.with do |redis| + expect(redis).to receive(:hset).with('inactive_projects_deletion_warning_email_notified', + "project:#{inactive_large_project.id}", Date.current) + end + expect(::Projects::InactiveProjectsDeletionNotificationWorker).to receive(:perform_in).with( + delay, inactive_large_project.id, deletion_date).and_call_original + expect(::Projects::DestroyService).not_to receive(:new) + + worker.perform + end + + it 'does not invoke InactiveProjectsDeletionNotificationWorker for already notified inactive projects' do + Gitlab::Redis::SharedState.with do |redis| + redis.hset('inactive_projects_deletion_warning_email_notified', "project:#{inactive_large_project.id}", + Date.current) + end + + expect(::Projects::InactiveProjectsDeletionNotificationWorker).not_to receive(:perform_in) + expect(::Projects::DestroyService).not_to receive(:new) + + worker.perform + end + + it 'invokes Projects::DestroyService for projects that are inactive even after being notified' do + Gitlab::Redis::SharedState.with do |redis| + redis.hset('inactive_projects_deletion_warning_email_notified', "project:#{inactive_large_project.id}", + 15.months.ago) + end + + expect(::Projects::InactiveProjectsDeletionNotificationWorker).not_to receive(:perform_in) + expect(::Projects::DestroyService).to receive(:new).with(inactive_large_project, admin_user, {}) + .at_least(:once).and_call_original + + worker.perform + + expect(inactive_large_project.reload.pending_delete).to eq(true) + + Gitlab::Redis::SharedState.with do |redis| + expect(redis.hget('inactive_projects_deletion_warning_email_notified', + "project:#{inactive_large_project.id}")).to be_nil + end + end + end + end + end + + private + + def grace_months_after_deletion_notification + (::Gitlab::CurrentSettings.inactive_projects_delete_after_months - + ::Gitlab::CurrentSettings.inactive_projects_send_warning_email_after_months).months + end + + def deletion_date + Date.parse(grace_months_after_deletion_notification.from_now.to_s).to_s + end +end diff --git a/spec/workers/projects/inactive_projects_deletion_notification_worker_spec.rb b/spec/workers/projects/inactive_projects_deletion_notification_worker_spec.rb new file mode 100644 index 00000000000000..141527f76c9176 --- /dev/null +++ b/spec/workers/projects/inactive_projects_deletion_notification_worker_spec.rb @@ -0,0 +1,42 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Projects::InactiveProjectsDeletionNotificationWorker do + describe "#perform" do + subject(:worker) { described_class.new } + + let_it_be(:deletion_date) { Date.current } + let_it_be(:non_existing_project_id) { non_existing_record_id } + let_it_be(:project) { create(:project) } + + it 'invokes NotificationService and calls inactive_project_deletion_warning' do + expect_next_instance_of(NotificationService) do |notification| + expect(notification).to receive(:inactive_project_deletion_warning).with(project, deletion_date) + end + + worker.perform(project.id, deletion_date) + end + + it 'adds the project_id to redis key that tracks the deletion warning emails' do + Gitlab::Redis::SharedState.with do |redis| + expect(redis).to receive(:hset).with('inactive_projects_deletion_warning_email_notified', + "project:#{project.id}", Date.current) + end + + worker.perform(project.id, deletion_date) + + Gitlab::Redis::SharedState.with do |redis| + expect(redis.hget('inactive_projects_deletion_warning_email_notified', + "project:#{project.id}")).to eq(Date.current.to_s) + end + end + + it 'rescues and logs the exception if project does not exist' do + expect(Gitlab::ErrorTracking).to receive(:log_exception).with(instance_of(ActiveRecord::RecordNotFound), + { project_id: non_existing_project_id }) + + worker.perform(non_existing_project_id, deletion_date) + end + end +end -- GitLab From 604d0faa655194a9acbc02257a8874f01983ac2c Mon Sep 17 00:00:00 2001 From: huzaifaiftikhar1 Date: Thu, 5 May 2022 15:15:35 +0530 Subject: [PATCH 2/7] Remove the feature flag from `inactive?` method in project.rb --- app/models/project.rb | 2 -- ee/app/models/ee/project.rb | 2 -- ee/spec/models/project_spec.rb | 52 ++++++++++++---------------------- spec/models/project_spec.rb | 26 +++-------------- 4 files changed, 22 insertions(+), 60 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index 5d86798759ec2b..f3d8c3dfced420 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -2896,8 +2896,6 @@ def enqueue_record_project_target_platforms end def inactive? - return false unless Feature.enabled?(:inactive_projects_deletion, root_namespace, default_enabled: :yaml) - (statistics || build_statistics).storage_size > ::Gitlab::CurrentSettings.inactive_projects_min_size_mb.megabytes && last_activity_at < ::Gitlab::CurrentSettings.inactive_projects_send_warning_email_after_months.months.ago end diff --git a/ee/app/models/ee/project.rb b/ee/app/models/ee/project.rb index db2820edd746ed..6f12b7e69965dc 100644 --- a/ee/app/models/ee/project.rb +++ b/ee/app/models/ee/project.rb @@ -872,8 +872,6 @@ def all_security_orchestration_policy_configurations override :inactive? def inactive? - return false unless ::Feature.enabled?(:inactive_projects_deletion, root_namespace, default_enabled: :yaml) - ::Gitlab.com? && root_namespace.paid? ? false : super end diff --git a/ee/spec/models/project_spec.rb b/ee/spec/models/project_spec.rb index 47564950e72eb9..6038c907ef71be 100644 --- a/ee/spec/models/project_spec.rb +++ b/ee/spec/models/project_spec.rb @@ -3337,50 +3337,34 @@ def stub_default_url_options(host) end describe '#inactive?' do - context 'when feature flag is disabled' do - before do - stub_feature_flags(inactive_projects_deletion: false) - end - - it 'returns false' do - expect(project.inactive?).to eq(false) - end - end - - context 'when feature flag is enabled' do - before do - stub_feature_flags(inactive_projects_deletion: true) - end - - context 'when Gitlab.com', :saas do - context 'when project belongs to paid namespace' do - before do - stub_application_setting(inactive_projects_min_size_mb: 5) - stub_application_setting(inactive_projects_send_warning_email_after_months: 24) - end - - it 'returns false' do - ultimate_group = create(:group_with_plan, plan: :ultimate_plan) - ultimate_project = create(:project, last_activity_at: 3.years.ago, namespace: ultimate_group) - - expect(ultimate_project.inactive?).to eq(false) - end + context 'when Gitlab.com', :saas do + context 'when project belongs to paid namespace' do + before do + stub_application_setting(inactive_projects_min_size_mb: 5) + stub_application_setting(inactive_projects_send_warning_email_after_months: 24) end - context 'when project belongs to free namespace' do - let_it_be(:no_plan_group) { create(:group_with_plan, plan: nil) } - let_it_be_with_reload(:project) { create(:project, namespace: no_plan_group) } + it 'returns false' do + ultimate_group = create(:group_with_plan, plan: :ultimate_plan) + ultimate_project = create(:project, last_activity_at: 3.years.ago, namespace: ultimate_group) - it_behaves_like 'returns true if project is inactive' + expect(ultimate_project.inactive?).to eq(false) end end - context 'when not Gitlab.com' do - let_it_be_with_reload(:project) { create(:project, name: 'test-project') } + context 'when project belongs to free namespace' do + let_it_be(:no_plan_group) { create(:group_with_plan, plan: nil) } + let_it_be_with_reload(:project) { create(:project, namespace: no_plan_group) } it_behaves_like 'returns true if project is inactive' end end + + context 'when not Gitlab.com' do + let_it_be_with_reload(:project) { create(:project, name: 'test-project') } + + it_behaves_like 'returns true if project is inactive' + end end describe '.inactive', :saas do diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 7fa0caeda54b16..ed5b3d4e0bed31 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -8265,23 +8265,7 @@ def has_external_wiki describe '#inactive?' do let_it_be_with_reload(:project) { create(:project, name: 'test-project') } - context 'when feature flag is enabled' do - before do - stub_feature_flags(inactive_projects_deletion: true) - end - - it_behaves_like 'returns true if project is inactive' - end - - context 'when feature flag is disabled' do - before do - stub_feature_flags(inactive_projects_deletion: false) - end - - it 'returns false' do - expect(project.inactive?).to eq(false) - end - end + it_behaves_like 'returns true if project is inactive' end describe '.inactive' do @@ -8291,20 +8275,18 @@ def has_external_wiki end it 'returns projects that are inactive' do - new_blank_project = create_project_with_statistics.tap do |project| + create_project_with_statistics.tap do |project| project.update!(last_activity_at: Time.current) end - inactive_blank_project = create_project_with_statistics.tap do |project| + create_project_with_statistics.tap do |project| project.update!(last_activity_at: 13.months.ago) end inactive_large_project = create_project_with_statistics(with_data: true, size_multiplier: 2.gigabytes) .tap { |project| project.update!(last_activity_at: 2.years.ago) } - active_large_project = create_project_with_statistics(with_data: true, size_multiplier: 2.gigabytes) + create_project_with_statistics(with_data: true, size_multiplier: 2.gigabytes) .tap { |project| project.update!(last_activity_at: 1.month.ago) } expect(described_class.inactive).to contain_exactly(inactive_large_project) - expect(described_class.inactive) - .not_to include(new_blank_project, inactive_blank_project, active_large_project) end end -- GitLab From efa7196bd2d40486ec472d75cd8d9d29dc427792 Mon Sep 17 00:00:00 2001 From: huzaifaiftikhar1 Date: Thu, 5 May 2022 15:45:01 +0530 Subject: [PATCH 3/7] Update RSpecs to use `to_date` method to make the examples descriptive --- .../projects/inactive_projects_deletion_cron_worker.rb | 2 +- .../inactive_projects_deletion_cron_worker_spec.rb | 6 +++--- spec/models/application_setting_spec.rb | 7 +++++-- .../inactive_projects_deletion_cron_worker_spec.rb | 4 ++-- 4 files changed, 11 insertions(+), 8 deletions(-) diff --git a/app/workers/projects/inactive_projects_deletion_cron_worker.rb b/app/workers/projects/inactive_projects_deletion_cron_worker.rb index be500e415968c4..8399ffba962f72 100644 --- a/app/workers/projects/inactive_projects_deletion_cron_worker.rb +++ b/app/workers/projects/inactive_projects_deletion_cron_worker.rb @@ -64,7 +64,7 @@ def delete_due_to_inactivity?(deletion_warning_email_sent_on) end def deletion_date - Date.parse(grace_months_after_deletion_notification.from_now.to_s).to_s + grace_months_after_deletion_notification.from_now.to_date.to_s end def delete_project(project, user) diff --git a/ee/spec/workers/ee/projects/inactive_projects_deletion_cron_worker_spec.rb b/ee/spec/workers/ee/projects/inactive_projects_deletion_cron_worker_spec.rb index b396c9a287cbfc..1f378f72dda97b 100644 --- a/ee/spec/workers/ee/projects/inactive_projects_deletion_cron_worker_spec.rb +++ b/ee/spec/workers/ee/projects/inactive_projects_deletion_cron_worker_spec.rb @@ -65,7 +65,7 @@ it 'invokes Projects::DestroyService for projects that are inactive even after being notified' do Gitlab::Redis::SharedState.with do |redis| redis.hset('inactive_projects_deletion_warning_email_notified', "project:#{inactive_large_project.id}", - 15.months.ago) + 15.months.ago.to_date.to_s) end expect(::Projects::InactiveProjectsDeletionNotificationWorker).not_to receive(:perform_in) @@ -98,7 +98,7 @@ it 'invokes Projects::DestroyService if adjourned_deletion_configured not configured for the project' do Gitlab::Redis::SharedState.with do |redis| redis.hset('inactive_projects_deletion_warning_email_notified', "project:#{inactive_large_project.id}", - 15.months.ago) + 15.months.ago.to_date.to_s) end expect(::Projects::InactiveProjectsDeletionNotificationWorker).not_to receive(:perform_in) @@ -126,7 +126,7 @@ it 'invokes Projects::MarkForDeletionService for projects that are inactive even after being notified' do Gitlab::Redis::SharedState.with do |redis| redis.hset('inactive_projects_deletion_warning_email_notified', "project:#{inactive_large_project.id}", - 15.months.ago) + 15.months.ago.to_date.to_s) end expect(::Projects::InactiveProjectsDeletionNotificationWorker).not_to receive(:perform_in) diff --git a/spec/models/application_setting_spec.rb b/spec/models/application_setting_spec.rb index 13cec5743caaf8..3e8d22056532f7 100644 --- a/spec/models/application_setting_spec.rb +++ b/spec/models/application_setting_spec.rb @@ -1348,10 +1348,13 @@ def expect_invalid it "deletes the redis key used for tracking inactive projects deletion warning emails when setting is updated", :clean_gitlab_redis_shared_state do Gitlab::Redis::SharedState.with do |redis| - expect(redis).to receive(:del).with("inactive_projects_deletion_warning_email_notified") + redis.hset("inactive_projects_deletion_warning_email_notified", "project:1", "2020-01-01") end - setting.update!(inactive_projects_delete_after_months: 6) + Gitlab::Redis::SharedState.with do |redis| + expect { setting.update!(inactive_projects_delete_after_months: 6) } + .to change { redis.hgetall('inactive_projects_deletion_warning_email_notified') }.to({}) + end end end end diff --git a/spec/workers/projects/inactive_projects_deletion_cron_worker_spec.rb b/spec/workers/projects/inactive_projects_deletion_cron_worker_spec.rb index 2d34ff18473760..afad827c591a21 100644 --- a/spec/workers/projects/inactive_projects_deletion_cron_worker_spec.rb +++ b/spec/workers/projects/inactive_projects_deletion_cron_worker_spec.rb @@ -103,7 +103,7 @@ it 'does not invoke InactiveProjectsDeletionNotificationWorker for already notified inactive projects' do Gitlab::Redis::SharedState.with do |redis| redis.hset('inactive_projects_deletion_warning_email_notified', "project:#{inactive_large_project.id}", - Date.current) + Date.current.to_s) end expect(::Projects::InactiveProjectsDeletionNotificationWorker).not_to receive(:perform_in) @@ -115,7 +115,7 @@ it 'invokes Projects::DestroyService for projects that are inactive even after being notified' do Gitlab::Redis::SharedState.with do |redis| redis.hset('inactive_projects_deletion_warning_email_notified', "project:#{inactive_large_project.id}", - 15.months.ago) + 15.months.ago.to_date.to_s) end expect(::Projects::InactiveProjectsDeletionNotificationWorker).not_to receive(:perform_in) -- GitLab From c55d158c17d4f00f5248f35a58970028702b40e4 Mon Sep 17 00:00:00 2001 From: huzaifaiftikhar1 Date: Thu, 5 May 2022 16:48:24 +0530 Subject: [PATCH 4/7] Add audit event for sending deletion warning email to inactive projects --- .../inactive_projects_deletion_cron_worker.rb | 8 +++++++- .../inactive_projects_deletion_cron_worker.rb | 12 ++++++++++++ ...ctive_projects_deletion_cron_worker_spec.rb | 18 ++++++++++++++++++ spec/support/helpers/project_helpers.rb | 9 +++++++++ ...ctive_projects_deletion_cron_worker_spec.rb | 11 ----------- 5 files changed, 46 insertions(+), 12 deletions(-) diff --git a/app/workers/projects/inactive_projects_deletion_cron_worker.rb b/app/workers/projects/inactive_projects_deletion_cron_worker.rb index 8399ffba962f72..240f4cd57408e4 100644 --- a/app/workers/projects/inactive_projects_deletion_cron_worker.rb +++ b/app/workers/projects/inactive_projects_deletion_cron_worker.rb @@ -16,6 +16,8 @@ def perform admin_user = User.admins.humans.active.first + return unless admin_user + notified_inactive_projects = deletion_warning_notified_projects Project.inactive.without_deleted.find_each(batch_size: 100).with_index do |project, index| # rubocop: disable CodeReuse/ActiveRecord @@ -27,7 +29,7 @@ def perform deletion_warning_email_sent_on = notified_inactive_projects["project:#{project.id}"] if send_deletion_warning_email?(deletion_warning_email_sent_on, project) - ::Projects::InactiveProjectsDeletionNotificationWorker.perform_in(delay, project.id, deletion_date) + send_notification(delay, project, admin_user) elsif deletion_warning_email_sent_on && delete_due_to_inactivity?(deletion_warning_email_sent_on) delete_redis_entry(project) delete_project(project, admin_user) @@ -76,6 +78,10 @@ def delete_redis_entry(project) redis.hdel('inactive_projects_deletion_warning_email_notified', "project:#{project.id}") end end + + def send_notification(delay, project, user) + ::Projects::InactiveProjectsDeletionNotificationWorker.perform_in(delay, project.id, deletion_date) + end end end diff --git a/ee/app/workers/ee/projects/inactive_projects_deletion_cron_worker.rb b/ee/app/workers/ee/projects/inactive_projects_deletion_cron_worker.rb index 92b04dd76bd849..df8e88ef6e80ea 100644 --- a/ee/app/workers/ee/projects/inactive_projects_deletion_cron_worker.rb +++ b/ee/app/workers/ee/projects/inactive_projects_deletion_cron_worker.rb @@ -19,6 +19,18 @@ def send_deletion_warning_email?(deletion_warning_email_sent_on, project) super end + + override :send_notification + def send_notification(delay, project, user) + super + + ::AuditEventService.new( + user, + project, + action: :custom, + custom_message: "Project is scheduled to be deleted on #{deletion_date} due to inactivity." + ).for_project.security_event + end end end end diff --git a/ee/spec/workers/ee/projects/inactive_projects_deletion_cron_worker_spec.rb b/ee/spec/workers/ee/projects/inactive_projects_deletion_cron_worker_spec.rb index 1f378f72dda97b..5ad1e91045c895 100644 --- a/ee/spec/workers/ee/projects/inactive_projects_deletion_cron_worker_spec.rb +++ b/ee/spec/workers/ee/projects/inactive_projects_deletion_cron_worker_spec.rb @@ -33,6 +33,8 @@ .tap { |project| project.update!(last_activity_at: 1.month.ago) } end + let_it_be(:delay) { anything } + before do stub_application_setting(inactive_projects_min_size_mb: 5) stub_application_setting(inactive_projects_send_warning_email_after_months: 12) @@ -57,6 +59,22 @@ end end + it 'invokes Projects::InactiveProjectsDeletionNotificationWorker for inactive projects and logs audit event' do + Gitlab::Redis::SharedState.with do |redis| + expect(redis).to receive(:hset).with('inactive_projects_deletion_warning_email_notified', + "project:#{inactive_large_project.id}", Date.current) + end + expect(::Projects::InactiveProjectsDeletionNotificationWorker).to receive(:perform_in).with( + delay, inactive_large_project.id, deletion_date).and_call_original + expect(::Projects::DestroyService).not_to receive(:new) + + expect { worker.perform } + .to change { AuditEvent.count }.by(1) + + expect(AuditEvent.last.details[:custom_message]) + .to eq("Project is scheduled to be deleted on #{deletion_date} due to inactivity.") + end + context 'when adjourned_deletion_for_projects_and_groups feature is not available' do before do stub_licensed_features(adjourned_deletion_for_projects_and_groups: false) diff --git a/spec/support/helpers/project_helpers.rb b/spec/support/helpers/project_helpers.rb index 9e45f284951dd0..2ea6405e48c38b 100644 --- a/spec/support/helpers/project_helpers.rb +++ b/spec/support/helpers/project_helpers.rb @@ -31,4 +31,13 @@ def create_project_with_statistics(namespace = nil, with_data: false, size_multi create(:project_statistics, project: p, with_data: with_data, size_multiplier: size_multiplier) end end + + def grace_months_after_deletion_notification + (::Gitlab::CurrentSettings.inactive_projects_delete_after_months - + ::Gitlab::CurrentSettings.inactive_projects_send_warning_email_after_months).months + end + + def deletion_date + Date.parse(grace_months_after_deletion_notification.from_now.to_s).to_s + end end diff --git a/spec/workers/projects/inactive_projects_deletion_cron_worker_spec.rb b/spec/workers/projects/inactive_projects_deletion_cron_worker_spec.rb index afad827c591a21..3463b87a7be394 100644 --- a/spec/workers/projects/inactive_projects_deletion_cron_worker_spec.rb +++ b/spec/workers/projects/inactive_projects_deletion_cron_worker_spec.rb @@ -134,15 +134,4 @@ end end end - - private - - def grace_months_after_deletion_notification - (::Gitlab::CurrentSettings.inactive_projects_delete_after_months - - ::Gitlab::CurrentSettings.inactive_projects_send_warning_email_after_months).months - end - - def deletion_date - Date.parse(grace_months_after_deletion_notification.from_now.to_s).to_s - end end -- GitLab From 0ed0329fdbdeae6e29e1f717e70855200a338cbb Mon Sep 17 00:00:00 2001 From: huzaifaiftikhar1 Date: Fri, 6 May 2022 12:22:45 +0530 Subject: [PATCH 5/7] Use the first active administrator user for inactive projects deletion --- app/workers/projects/inactive_projects_deletion_cron_worker.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/workers/projects/inactive_projects_deletion_cron_worker.rb b/app/workers/projects/inactive_projects_deletion_cron_worker.rb index 240f4cd57408e4..9b00366ad30f20 100644 --- a/app/workers/projects/inactive_projects_deletion_cron_worker.rb +++ b/app/workers/projects/inactive_projects_deletion_cron_worker.rb @@ -14,7 +14,7 @@ class InactiveProjectsDeletionCronWorker # rubocop:disable Scalability/Idempoten def perform return unless ::Gitlab::CurrentSettings.delete_inactive_projects? - admin_user = User.admins.humans.active.first + admin_user = User.admins.active.first return unless admin_user -- GitLab From b1a6d8bf6403a357c059941614c4b7806b76bd0d Mon Sep 17 00:00:00 2001 From: huzaifaiftikhar1 Date: Fri, 6 May 2022 18:46:26 +0530 Subject: [PATCH 6/7] Remove default_enabled as per the new guideline --- app/workers/projects/inactive_projects_deletion_cron_worker.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/workers/projects/inactive_projects_deletion_cron_worker.rb b/app/workers/projects/inactive_projects_deletion_cron_worker.rb index 9b00366ad30f20..a0c1da2e2ba902 100644 --- a/app/workers/projects/inactive_projects_deletion_cron_worker.rb +++ b/app/workers/projects/inactive_projects_deletion_cron_worker.rb @@ -21,7 +21,7 @@ def perform notified_inactive_projects = deletion_warning_notified_projects Project.inactive.without_deleted.find_each(batch_size: 100).with_index do |project, index| # rubocop: disable CodeReuse/ActiveRecord - next unless Feature.enabled?(:inactive_projects_deletion, project.root_namespace, default_enabled: :yaml) + next unless Feature.enabled?(:inactive_projects_deletion, project.root_namespace) delay = index * INTERVAL -- GitLab From 17569f4404b08058a774d8ae2ad286e2dacd30fe Mon Sep 17 00:00:00 2001 From: huzaifaiftikhar1 Date: Thu, 12 May 2022 17:20:18 +0530 Subject: [PATCH 7/7] Add InactiveProjectsDeletionWarningTracker and minor refactoring * Encapsulate the redis logic for inactive projects deletion feature inside InactiveProjectsDeletionWarningTracker. * Make the workers idempotent. * Update the feature_category to the correct owners. --- app/models/application_setting.rb | 4 +- app/models/event.rb | 4 +- app/workers/all_queues.yml | 8 +-- .../inactive_projects_deletion_cron_worker.rb | 25 ++----- ...e_projects_deletion_notification_worker.rb | 17 ++--- .../inactive_projects_deletion_cron_worker.rb | 2 + ...ctive_projects_deletion_warning_tracker.rb | 47 +++++++++++++ ..._projects_deletion_warning_tracker_spec.rb | 67 +++++++++++++++++++ ...tive_projects_deletion_cron_worker_spec.rb | 2 + ...jects_deletion_notification_worker_spec.rb | 9 ++- 10 files changed, 140 insertions(+), 45 deletions(-) create mode 100644 lib/gitlab/inactive_projects_deletion_warning_tracker.rb create mode 100644 spec/lib/gitlab/inactive_projects_deletion_warning_tracker_spec.rb diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index 8b337d70864cb1..435e046f3f7ea8 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -778,9 +778,7 @@ def validate_url(parsed_url, name, error_message) end def reset_deletion_warning_redis_key - Gitlab::Redis::SharedState.with do |redis| - redis.del('inactive_projects_deletion_warning_email_notified') - end + Gitlab::InactiveProjectsDeletionWarningTracker.reset_all end end diff --git a/app/models/event.rb b/app/models/event.rb index cdbd2d2169ada0..f53d56e735e40b 100644 --- a/app/models/event.rb +++ b/app/models/event.rb @@ -358,9 +358,7 @@ def reset_project_activity .where('last_activity_at <= ?', RESET_PROJECT_ACTIVITY_INTERVAL.ago) .touch_all(:last_activity_at, time: created_at) # rubocop: disable Rails/SkipsModelValidations - Gitlab::Redis::SharedState.with do |redis| - redis.hdel('inactive_projects_deletion_warning_email_notified', "project:#{project.id}") - end + Gitlab::InactiveProjectsDeletionWarningTracker.new(project.id).reset end def authored_by?(user) diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index 28253f1c1d2001..63e5210936bec3 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -581,12 +581,12 @@ :tags: [] - :name: cronjob:projects_inactive_projects_deletion_cron :worker_name: Projects::InactiveProjectsDeletionCronWorker - :feature_category: :source_code_management + :feature_category: :compliance_management :has_external_dependencies: :urgency: :low :resource_boundary: :unknown :weight: 1 - :idempotent: + :idempotent: true :tags: [] - :name: cronjob:projects_schedule_refresh_build_artifacts_size_statistics :worker_name: Projects::ScheduleRefreshBuildArtifactsSizeStatisticsWorker @@ -2805,12 +2805,12 @@ :tags: [] - :name: projects_inactive_projects_deletion_notification :worker_name: Projects::InactiveProjectsDeletionNotificationWorker - :feature_category: :source_code_management + :feature_category: :compliance_management :has_external_dependencies: :urgency: :low :resource_boundary: :unknown :weight: 1 - :idempotent: + :idempotent: true :tags: [] - :name: projects_post_creation :worker_name: Projects::PostCreationWorker diff --git a/app/workers/projects/inactive_projects_deletion_cron_worker.rb b/app/workers/projects/inactive_projects_deletion_cron_worker.rb index a0c1da2e2ba902..2c3f4191502d82 100644 --- a/app/workers/projects/inactive_projects_deletion_cron_worker.rb +++ b/app/workers/projects/inactive_projects_deletion_cron_worker.rb @@ -1,13 +1,14 @@ # frozen_string_literal: true module Projects - class InactiveProjectsDeletionCronWorker # rubocop:disable Scalability/IdempotentWorker + class InactiveProjectsDeletionCronWorker include ApplicationWorker include Gitlab::Utils::StrongMemoize include CronjobQueue + idempotent! data_consistency :always - feature_category :source_code_management + feature_category :compliance_management INTERVAL = 2.seconds.to_i @@ -18,7 +19,7 @@ def perform return unless admin_user - notified_inactive_projects = deletion_warning_notified_projects + notified_inactive_projects = Gitlab::InactiveProjectsDeletionWarningTracker.notified_projects Project.inactive.without_deleted.find_each(batch_size: 100).with_index do |project, index| # rubocop: disable CodeReuse/ActiveRecord next unless Feature.enabled?(:inactive_projects_deletion, project.root_namespace) @@ -31,7 +32,7 @@ def perform if send_deletion_warning_email?(deletion_warning_email_sent_on, project) send_notification(delay, project, admin_user) elsif deletion_warning_email_sent_on && delete_due_to_inactivity?(deletion_warning_email_sent_on) - delete_redis_entry(project) + Gitlab::InactiveProjectsDeletionWarningTracker.new(project.id).reset delete_project(project, admin_user) end end @@ -40,16 +41,6 @@ def perform private - # Redis key 'inactive_projects_deletion_warning_email_notified' is a hash. It stores the date when the - # deletion warning notification email was sent for an inactive project. The fields and values look like: - # {"project:1"=>"2022-04-22", "project:5"=>"2022-04-22", "project:7"=>"2022-04-25"} - # @return [Hash] - def deletion_warning_notified_projects - Gitlab::Redis::SharedState.with do |redis| - redis.hgetall('inactive_projects_deletion_warning_email_notified') - end - end - def grace_months_after_deletion_notification strong_memoize(:grace_months_after_deletion_notification) do (::Gitlab::CurrentSettings.inactive_projects_delete_after_months - @@ -73,12 +64,6 @@ def delete_project(project, user) ::Projects::DestroyService.new(project, user, {}).async_execute end - def delete_redis_entry(project) - Gitlab::Redis::SharedState.with do |redis| - redis.hdel('inactive_projects_deletion_warning_email_notified', "project:#{project.id}") - end - end - def send_notification(delay, project, user) ::Projects::InactiveProjectsDeletionNotificationWorker.perform_in(delay, project.id, deletion_date) end diff --git a/app/workers/projects/inactive_projects_deletion_notification_worker.rb b/app/workers/projects/inactive_projects_deletion_notification_worker.rb index f4a1590c5110fe..0bf808fd753be8 100644 --- a/app/workers/projects/inactive_projects_deletion_notification_worker.rb +++ b/app/workers/projects/inactive_projects_deletion_notification_worker.rb @@ -1,20 +1,23 @@ # frozen_string_literal: true module Projects - class InactiveProjectsDeletionNotificationWorker # rubocop:disable Scalability/IdempotentWorker + class InactiveProjectsDeletionNotificationWorker include ApplicationWorker include ExceptionBacktrace - data_consistency :always + idempotent! + data_consistency :sticky sidekiq_options retry: 3 - feature_category :source_code_management + feature_category :compliance_management def perform(project_id, deletion_date) + return if Gitlab::InactiveProjectsDeletionWarningTracker.new(project_id).notified? + project = Project.find(project_id) notification_service.inactive_project_deletion_warning(project, deletion_date) - update_deletion_warning_notified_projects(project_id) + Gitlab::InactiveProjectsDeletionWarningTracker.new(project_id).mark_notified rescue ActiveRecord::RecordNotFound => error Gitlab::ErrorTracking.log_exception(error, project_id: project_id) end @@ -24,11 +27,5 @@ def perform(project_id, deletion_date) def notification_service @notification_service ||= NotificationService.new end - - def update_deletion_warning_notified_projects(project_id) - Gitlab::Redis::SharedState.with do |redis| - redis.hset('inactive_projects_deletion_warning_email_notified', "project:#{project_id}", Date.current) - end - end end end diff --git a/ee/app/workers/ee/projects/inactive_projects_deletion_cron_worker.rb b/ee/app/workers/ee/projects/inactive_projects_deletion_cron_worker.rb index df8e88ef6e80ea..89a68df4ce5f1c 100644 --- a/ee/app/workers/ee/projects/inactive_projects_deletion_cron_worker.rb +++ b/ee/app/workers/ee/projects/inactive_projects_deletion_cron_worker.rb @@ -8,6 +8,7 @@ module InactiveProjectsDeletionCronWorker override :delete_project def delete_project(project, user) return super unless License.feature_available?(:adjourned_deletion_for_projects_and_groups) + # Can't use `project.adjourned_deletion?` see https://gitlab.com/gitlab-org/gitlab/-/merge_requests/85689#note_943072034 return super unless project.adjourned_deletion_configured? ::Projects::MarkForDeletionService.new(project, user, {}).execute @@ -15,6 +16,7 @@ def delete_project(project, user) override :send_deletion_warning_email? def send_deletion_warning_email?(deletion_warning_email_sent_on, project) + # Can't use `project.marked_for_deletion?`, see https://gitlab.com/gitlab-org/gitlab/-/merge_requests/85689#note_943072064 return false if project.marked_for_deletion_at? super diff --git a/lib/gitlab/inactive_projects_deletion_warning_tracker.rb b/lib/gitlab/inactive_projects_deletion_warning_tracker.rb new file mode 100644 index 00000000000000..f3f8e774b4b662 --- /dev/null +++ b/lib/gitlab/inactive_projects_deletion_warning_tracker.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true + +module Gitlab + class InactiveProjectsDeletionWarningTracker + attr_reader :project_id + + DELETION_TRACKING_REDIS_KEY = 'inactive_projects_deletion_warning_email_notified' + + # Redis key 'inactive_projects_deletion_warning_email_notified' is a hash. It stores the date when the + # deletion warning notification email was sent for an inactive project. The fields and values look like: + # {"project:1"=>"2022-04-22", "project:5"=>"2022-04-22", "project:7"=>"2022-04-25"} + # @return [Hash] + def self.notified_projects + Gitlab::Redis::SharedState.with do |redis| + redis.hgetall(DELETION_TRACKING_REDIS_KEY) + end + end + + def self.reset_all + Gitlab::Redis::SharedState.with do |redis| + redis.del(DELETION_TRACKING_REDIS_KEY) + end + end + + def initialize(project_id) + @project_id = project_id + end + + def notified? + Gitlab::Redis::SharedState.with do |redis| + redis.hexists(DELETION_TRACKING_REDIS_KEY, "project:#{project_id}") + end + end + + def mark_notified + Gitlab::Redis::SharedState.with do |redis| + redis.hset(DELETION_TRACKING_REDIS_KEY, "project:#{project_id}", Date.current) + end + end + + def reset + Gitlab::Redis::SharedState.with do |redis| + redis.hdel(DELETION_TRACKING_REDIS_KEY, "project:#{project_id}") + end + end + end +end diff --git a/spec/lib/gitlab/inactive_projects_deletion_warning_tracker_spec.rb b/spec/lib/gitlab/inactive_projects_deletion_warning_tracker_spec.rb new file mode 100644 index 00000000000000..4eb2388f3f7e5b --- /dev/null +++ b/spec/lib/gitlab/inactive_projects_deletion_warning_tracker_spec.rb @@ -0,0 +1,67 @@ +# frozen_string_literal: true + +require "spec_helper" + +RSpec.describe Gitlab::InactiveProjectsDeletionWarningTracker do + let_it_be(:project_id) { 1 } + + describe '.notified_projects', :clean_gitlab_redis_shared_state do + before do + freeze_time do + Gitlab::InactiveProjectsDeletionWarningTracker.new(project_id).mark_notified + end + end + + it 'returns the list of projects for which deletion warning email has been sent' do + expected_hash = { "project:1" => "#{Date.current}" } + + expect(Gitlab::InactiveProjectsDeletionWarningTracker.notified_projects).to eq(expected_hash) + end + end + + describe '.reset_all' do + before do + Gitlab::InactiveProjectsDeletionWarningTracker.new(project_id).mark_notified + end + + it 'deletes all the projects for which deletion warning email was sent' do + Gitlab::InactiveProjectsDeletionWarningTracker.reset_all + + expect(Gitlab::InactiveProjectsDeletionWarningTracker.notified_projects).to eq({}) + end + end + + describe '#notified?' do + before do + Gitlab::InactiveProjectsDeletionWarningTracker.new(project_id).mark_notified + end + + it 'returns true if the project has already been notified' do + expect(Gitlab::InactiveProjectsDeletionWarningTracker.new(project_id).notified?).to eq(true) + end + + it 'returns false if the project has not been notified' do + expect(Gitlab::InactiveProjectsDeletionWarningTracker.new(2).notified?).to eq(false) + end + end + + describe '#mark_notified' do + it 'marks the project as being notified' do + Gitlab::InactiveProjectsDeletionWarningTracker.new(project_id).mark_notified + + expect(Gitlab::InactiveProjectsDeletionWarningTracker.new(project_id).notified?).to eq(true) + end + end + + describe '#reset' do + before do + Gitlab::InactiveProjectsDeletionWarningTracker.new(project_id).mark_notified + end + + it 'resets the project as not being notified' do + Gitlab::InactiveProjectsDeletionWarningTracker.new(project_id).reset + + expect(Gitlab::InactiveProjectsDeletionWarningTracker.new(project_id).notified?).to eq(false) + end + end +end diff --git a/spec/workers/projects/inactive_projects_deletion_cron_worker_spec.rb b/spec/workers/projects/inactive_projects_deletion_cron_worker_spec.rb index 3463b87a7be394..0e7b4ea504c500 100644 --- a/spec/workers/projects/inactive_projects_deletion_cron_worker_spec.rb +++ b/spec/workers/projects/inactive_projects_deletion_cron_worker_spec.rb @@ -132,6 +132,8 @@ end end end + + it_behaves_like 'an idempotent worker' end end end diff --git a/spec/workers/projects/inactive_projects_deletion_notification_worker_spec.rb b/spec/workers/projects/inactive_projects_deletion_notification_worker_spec.rb index 141527f76c9176..3ddfec0d346de4 100644 --- a/spec/workers/projects/inactive_projects_deletion_notification_worker_spec.rb +++ b/spec/workers/projects/inactive_projects_deletion_notification_worker_spec.rb @@ -19,11 +19,6 @@ end it 'adds the project_id to redis key that tracks the deletion warning emails' do - Gitlab::Redis::SharedState.with do |redis| - expect(redis).to receive(:hset).with('inactive_projects_deletion_warning_email_notified', - "project:#{project.id}", Date.current) - end - worker.perform(project.id, deletion_date) Gitlab::Redis::SharedState.with do |redis| @@ -38,5 +33,9 @@ worker.perform(non_existing_project_id, deletion_date) end + + it_behaves_like 'an idempotent worker' do + let(:job_args) { [project.id, deletion_date] } + end end end -- GitLab