From 75cc012ebb6d39c08b1423180206f1ce2a6af8bb Mon Sep 17 00:00:00 2001 From: Olivier El Mekki Date: Tue, 31 Oct 2023 15:23:53 +0100 Subject: [PATCH 01/13] Emit event when a release is published Previously, there was no event to which to subscribe to if we wanted to add logic both 1/ when a release is created with not setting a different release date and 2/ when the release date of a pre-existing release record has come. This commit introduces the `::Projects::ReleasePublishedEvent` event in `EventStore`, which can be subscribed to implement logic when a release happens. Changelog: added --- .../projects/release_published_event.rb | 15 +++++++++++ app/models/release.rb | 5 ++++ app/workers/all_queues.yml | 18 +++++++++++++ app/workers/releases/publish_event_worker.rb | 26 +++++++++++++++++++ config/initializers/1_settings.rb | 3 +++ ...0358_add_publish_event_fired_to_release.rb | 11 ++++++++ db/schema_migrations/20231024090358 | 1 + db/structure.sql | 3 ++- spec/models/release_spec.rb | 19 ++++++++++++++ .../releases/publish_event_worker_spec.rb | 21 +++++++++++++++ 10 files changed, 121 insertions(+), 1 deletion(-) create mode 100644 app/events/projects/release_published_event.rb create mode 100644 app/workers/releases/publish_event_worker.rb create mode 100644 db/migrate/20231024090358_add_publish_event_fired_to_release.rb create mode 100644 db/schema_migrations/20231024090358 create mode 100644 spec/workers/releases/publish_event_worker_spec.rb diff --git a/app/events/projects/release_published_event.rb b/app/events/projects/release_published_event.rb new file mode 100644 index 00000000000000..f0be95b893ed80 --- /dev/null +++ b/app/events/projects/release_published_event.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +module Projects + class ReleasePublishedEvent < ::Gitlab::EventStore::Event + def schema + { + 'type' => 'object', + 'properties' => { + 'release_id' => { 'type' => 'integer' } + }, + 'required' => %w[release_id] + } + end + end +end diff --git a/app/models/release.rb b/app/models/release.rb index 1cd623e1254a44..d0068f22a1d7be 100644 --- a/app/models/release.rb +++ b/app/models/release.rb @@ -54,6 +54,7 @@ class Release < ApplicationRecord scope :recent, -> { sorted.limit(MAX_NUMBER_TO_DISPLAY) } scope :without_evidence, -> { left_joins(:evidences).where(::Releases::Evidence.arel_table[:id].eq(nil)) } scope :released_within_2hrs, -> { where(released_at: Time.zone.now - 1.hour..Time.zone.now + 1.hour) } + scope :publishing_not_advertised, -> { where(publish_event_fired: false) } scope :for_projects, ->(projects) { where(project_id: projects) } scope :by_tag, ->(tag) { where(tag: tag) } @@ -97,6 +98,10 @@ def latest_for_projects(projects, order_by: 'released_at') .from("(VALUES #{project_ids_list}) projects (id)") .joins("INNER JOIN LATERAL (#{join_query.to_sql}) #{Release.table_name} ON TRUE") end + + def waiting_for_publish_event + publishing_not_advertised.released_within_2hrs.includes(:project) + end end def to_param diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index ec5156bb1d0de3..2803e0f7eb007c 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -3,6 +3,15 @@ # # Do not edit it manually! --- +- :name: activity_pub:activity_pub_projects_publish_release_activities + :worker_name: ActivityPub::Projects::PublishReleaseActivitiesWorker + :feature_category: :release_orchestration + :has_external_dependencies: true + :urgency: :low + :resource_boundary: :unknown + :weight: 1 + :idempotent: true + :tags: [] - :name: activity_pub:activity_pub_projects_releases_subscription :worker_name: ActivityPub::Projects::ReleasesSubscriptionWorker :feature_category: :release_orchestration @@ -786,6 +795,15 @@ :weight: 1 :idempotent: false :tags: [] +- :name: cronjob:releases_publish_event + :worker_name: Releases::PublishEventWorker + :feature_category: :release_orchestration + :has_external_dependencies: false + :urgency: :low + :resource_boundary: :unknown + :weight: 1 + :idempotent: true + :tags: [] - :name: cronjob:remove_expired_group_links :worker_name: RemoveExpiredGroupLinksWorker :feature_category: :system_access diff --git a/app/workers/releases/publish_event_worker.rb b/app/workers/releases/publish_event_worker.rb new file mode 100644 index 00000000000000..c4fd1c9357c980 --- /dev/null +++ b/app/workers/releases/publish_event_worker.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +module Releases + class PublishEventWorker + include ApplicationWorker + include Gitlab::ExclusiveLeaseHelpers + include CronjobQueue + include Gitlab::Utils::StrongMemoize + + idempotent! + data_consistency :always # rubocop:disable SidekiqLoadBalancing/WorkerDataConsistency -- usual for EventStore jobs. + feature_category :release_orchestration + + def perform + Release.waiting_for_publish_event.each do |release| + with_context(project: release.project) do + release.toggle!(:publish_event_fired) + + ::Gitlab::EventStore.publish( + ::Projects::ReleasePublishedEvent.new(data: { release_id: release.id }) + ) + end + end + end + end +end diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index ade5465f8ea7ab..b772a948dae8f2 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -634,6 +634,9 @@ Settings.cron_jobs['manage_evidence_worker'] ||= {} Settings.cron_jobs['manage_evidence_worker']['cron'] ||= '0 * * * *' Settings.cron_jobs['manage_evidence_worker']['job_class'] = 'Releases::ManageEvidenceWorker' +Settings.cron_jobs['publish_release_worker'] ||= {} +Settings.cron_jobs['publish_release_worker']['cron'] ||= '20 * * * *' +Settings.cron_jobs['publish_release_worker']['job_class'] = 'Releases::PublishEventWorker' Settings.cron_jobs['user_status_cleanup_batch_worker'] ||= {} Settings.cron_jobs['user_status_cleanup_batch_worker']['cron'] ||= '* * * * *' Settings.cron_jobs['user_status_cleanup_batch_worker']['job_class'] = 'UserStatusCleanup::BatchWorker' diff --git a/db/migrate/20231024090358_add_publish_event_fired_to_release.rb b/db/migrate/20231024090358_add_publish_event_fired_to_release.rb new file mode 100644 index 00000000000000..629544c2968c13 --- /dev/null +++ b/db/migrate/20231024090358_add_publish_event_fired_to_release.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +class AddPublishEventFiredToRelease < Gitlab::Database::Migration[2.1] + def up + add_column :releases, :publish_event_fired, :boolean, default: false + end + + def down + remove_column :releases, :publish_event_fired, :boolean + end +end diff --git a/db/schema_migrations/20231024090358 b/db/schema_migrations/20231024090358 new file mode 100644 index 00000000000000..48d45c94c951c6 --- /dev/null +++ b/db/schema_migrations/20231024090358 @@ -0,0 +1 @@ +d21a058fb071021764e24d3265a7f8cc5aef9a6b4d7aca6e4011814ce37aaddf \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 754c8139b37269..e5aa301a555a8f 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -22754,7 +22754,8 @@ CREATE TABLE releases ( author_id integer, name character varying, sha character varying, - released_at timestamp with time zone NOT NULL + released_at timestamp with time zone NOT NULL, + publish_event_fired boolean DEFAULT false ); CREATE SEQUENCE releases_id_seq diff --git a/spec/models/release_spec.rb b/spec/models/release_spec.rb index bff9f73e44aaee..9a63936c140467 100644 --- a/spec/models/release_spec.rb +++ b/spec/models/release_spec.rb @@ -156,6 +156,7 @@ describe 'latest releases' do let_it_be(:yesterday) { Time.zone.now - 1.day } + let_it_be(:today) { Time.zone.now } let_it_be(:tomorrow) { Time.zone.now + 1.day } let_it_be(:project2) { create(:project) } @@ -176,6 +177,14 @@ create(:release, project: project2, released_at: tomorrow, created_at: yesterday) end + let_it_be(:project2_release3) do + create(:release, project: project2, released_at: today, created_at: yesterday) + end + + let_it_be(:project2_release4) do + create(:release, project: project2, released_at: today, created_at: yesterday, publish_event_fired: true) + end + let(:args) { {} } describe '.latest' do @@ -240,6 +249,16 @@ end end end + + describe '.waiting_for_publish_event' do + let(:releases) { [project2_release3] } + + subject(:waiting) { described_class.waiting_for_publish_event } + + it "find today's releases not yet published" do + expect(waiting).to match_array(releases) + end + end end describe '#assets_count' do diff --git a/spec/workers/releases/publish_event_worker_spec.rb b/spec/workers/releases/publish_event_worker_spec.rb new file mode 100644 index 00000000000000..32cb1f52bea09a --- /dev/null +++ b/spec/workers/releases/publish_event_worker_spec.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Releases::PublishEventWorker, feature_category: :release_evidence do + let_it_be(:project) { create(:project, :repository) } + let_it_be_with_reload(:release) { create(:release, project: project, released_at: Time.current) } + + before do + allow(Gitlab::EventStore).to receive(:publish).and_return(true) + described_class.new.perform + end + + it 'broadcasts the published event' do + expect(Gitlab::EventStore).to have_received(:publish).with(Projects::ReleasePublishedEvent) + end + + it 'sets the release as published' do + expect(release.publish_event_fired).to be_truthy + end +end -- GitLab From 26d346e81264941cee014ed765d4f6a0ab071da7 Mon Sep 17 00:00:00 2001 From: Olivier El Mekki Date: Fri, 17 Nov 2023 14:58:36 +0100 Subject: [PATCH 02/13] FIX remove worker def from a later MR This part is supposed to be added by the last commit in [the overarching MR](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/134995). --- app/workers/all_queues.yml | 9 --------- 1 file changed, 9 deletions(-) diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index 2803e0f7eb007c..90198e99433695 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -3,15 +3,6 @@ # # Do not edit it manually! --- -- :name: activity_pub:activity_pub_projects_publish_release_activities - :worker_name: ActivityPub::Projects::PublishReleaseActivitiesWorker - :feature_category: :release_orchestration - :has_external_dependencies: true - :urgency: :low - :resource_boundary: :unknown - :weight: 1 - :idempotent: true - :tags: [] - :name: activity_pub:activity_pub_projects_releases_subscription :worker_name: ActivityPub::Projects::ReleasesSubscriptionWorker :feature_category: :release_orchestration -- GitLab From 76a8624758d3c24834d43cc0d574d88590afea1c Mon Sep 17 00:00:00 2001 From: Olivier El Mekki Date: Fri, 17 Nov 2023 15:55:04 +0100 Subject: [PATCH 03/13] REFACTOR update migration The migration was more than 3 weeks old. --- ...b => 20231117144453_add_publish_event_fired_to_release.rb} | 4 +++- db/schema_migrations/20231024090358 | 1 - db/schema_migrations/20231117144453 | 1 + 3 files changed, 4 insertions(+), 2 deletions(-) rename db/migrate/{20231024090358_add_publish_event_fired_to_release.rb => 20231117144453_add_publish_event_fired_to_release.rb} (90%) delete mode 100644 db/schema_migrations/20231024090358 create mode 100644 db/schema_migrations/20231117144453 diff --git a/db/migrate/20231024090358_add_publish_event_fired_to_release.rb b/db/migrate/20231117144453_add_publish_event_fired_to_release.rb similarity index 90% rename from db/migrate/20231024090358_add_publish_event_fired_to_release.rb rename to db/migrate/20231117144453_add_publish_event_fired_to_release.rb index 629544c2968c13..717ceba9d2f61d 100644 --- a/db/migrate/20231024090358_add_publish_event_fired_to_release.rb +++ b/db/migrate/20231117144453_add_publish_event_fired_to_release.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true -class AddPublishEventFiredToRelease < Gitlab::Database::Migration[2.1] +class AddPublishEventFiredToRelease < Gitlab::Database::Migration[2.2] + milestone '16.7' + def up add_column :releases, :publish_event_fired, :boolean, default: false end diff --git a/db/schema_migrations/20231024090358 b/db/schema_migrations/20231024090358 deleted file mode 100644 index 48d45c94c951c6..00000000000000 --- a/db/schema_migrations/20231024090358 +++ /dev/null @@ -1 +0,0 @@ -d21a058fb071021764e24d3265a7f8cc5aef9a6b4d7aca6e4011814ce37aaddf \ No newline at end of file diff --git a/db/schema_migrations/20231117144453 b/db/schema_migrations/20231117144453 new file mode 100644 index 00000000000000..2c9050717314e8 --- /dev/null +++ b/db/schema_migrations/20231117144453 @@ -0,0 +1 @@ +ada8547d006a8ec96632ca294c7491dbc8f348ef390fd9926bac1336c1df6acf \ No newline at end of file -- GitLab From b2fe764b6cb8fdf42e759c1c60302af837704fd4 Mon Sep 17 00:00:00 2001 From: Olivier El Mekki Date: Tue, 12 Dec 2023 16:37:53 +0100 Subject: [PATCH 04/13] REFACTOR rename publish_event_fired to release_published_at --- app/models/release.rb | 2 +- app/workers/releases/publish_event_worker.rb | 2 +- ...1117144453_add_publish_event_fired_to_release.rb | 13 ------------- ...212154022_add_release_published_at_to_release.rb | 13 +++++++++++++ db/schema_migrations/20231117144453 | 1 - db/schema_migrations/20231212154022 | 1 + db/structure.sql | 2 +- spec/models/release_spec.rb | 2 +- spec/workers/releases/publish_event_worker_spec.rb | 2 +- 9 files changed, 19 insertions(+), 19 deletions(-) delete mode 100644 db/migrate/20231117144453_add_publish_event_fired_to_release.rb create mode 100644 db/migrate/20231212154022_add_release_published_at_to_release.rb delete mode 100644 db/schema_migrations/20231117144453 create mode 100644 db/schema_migrations/20231212154022 diff --git a/app/models/release.rb b/app/models/release.rb index d0068f22a1d7be..ec7bc35708714c 100644 --- a/app/models/release.rb +++ b/app/models/release.rb @@ -54,7 +54,7 @@ class Release < ApplicationRecord scope :recent, -> { sorted.limit(MAX_NUMBER_TO_DISPLAY) } scope :without_evidence, -> { left_joins(:evidences).where(::Releases::Evidence.arel_table[:id].eq(nil)) } scope :released_within_2hrs, -> { where(released_at: Time.zone.now - 1.hour..Time.zone.now + 1.hour) } - scope :publishing_not_advertised, -> { where(publish_event_fired: false) } + scope :publishing_not_advertised, -> { where(release_published_at: nil) } scope :for_projects, ->(projects) { where(project_id: projects) } scope :by_tag, ->(tag) { where(tag: tag) } diff --git a/app/workers/releases/publish_event_worker.rb b/app/workers/releases/publish_event_worker.rb index c4fd1c9357c980..293debc0ac50c3 100644 --- a/app/workers/releases/publish_event_worker.rb +++ b/app/workers/releases/publish_event_worker.rb @@ -14,7 +14,7 @@ class PublishEventWorker def perform Release.waiting_for_publish_event.each do |release| with_context(project: release.project) do - release.toggle!(:publish_event_fired) + release.touch(:release_published_at) ::Gitlab::EventStore.publish( ::Projects::ReleasePublishedEvent.new(data: { release_id: release.id }) diff --git a/db/migrate/20231117144453_add_publish_event_fired_to_release.rb b/db/migrate/20231117144453_add_publish_event_fired_to_release.rb deleted file mode 100644 index 717ceba9d2f61d..00000000000000 --- a/db/migrate/20231117144453_add_publish_event_fired_to_release.rb +++ /dev/null @@ -1,13 +0,0 @@ -# frozen_string_literal: true - -class AddPublishEventFiredToRelease < Gitlab::Database::Migration[2.2] - milestone '16.7' - - def up - add_column :releases, :publish_event_fired, :boolean, default: false - end - - def down - remove_column :releases, :publish_event_fired, :boolean - end -end diff --git a/db/migrate/20231212154022_add_release_published_at_to_release.rb b/db/migrate/20231212154022_add_release_published_at_to_release.rb new file mode 100644 index 00000000000000..d477a98004fd8b --- /dev/null +++ b/db/migrate/20231212154022_add_release_published_at_to_release.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +class AddReleasePublishedAtToRelease < Gitlab::Database::Migration[2.2] + milestone '16.7' + + def up + add_column :releases, :release_published_at, :datetime_with_timezone + end + + def down + remove_column :releases, :release_published_at, :datetime_with_timezone + end +end diff --git a/db/schema_migrations/20231117144453 b/db/schema_migrations/20231117144453 deleted file mode 100644 index 2c9050717314e8..00000000000000 --- a/db/schema_migrations/20231117144453 +++ /dev/null @@ -1 +0,0 @@ -ada8547d006a8ec96632ca294c7491dbc8f348ef390fd9926bac1336c1df6acf \ No newline at end of file diff --git a/db/schema_migrations/20231212154022 b/db/schema_migrations/20231212154022 new file mode 100644 index 00000000000000..9f9967d0326345 --- /dev/null +++ b/db/schema_migrations/20231212154022 @@ -0,0 +1 @@ +c005eb8901f1ebb85dedb044d627396f591bd760a0315dc3f45171def0f972e5 \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index e5aa301a555a8f..467beac0672326 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -22755,7 +22755,7 @@ CREATE TABLE releases ( name character varying, sha character varying, released_at timestamp with time zone NOT NULL, - publish_event_fired boolean DEFAULT false + release_published_at timestamp with time zone ); CREATE SEQUENCE releases_id_seq diff --git a/spec/models/release_spec.rb b/spec/models/release_spec.rb index 9a63936c140467..4a4cb1ae46a4f7 100644 --- a/spec/models/release_spec.rb +++ b/spec/models/release_spec.rb @@ -182,7 +182,7 @@ end let_it_be(:project2_release4) do - create(:release, project: project2, released_at: today, created_at: yesterday, publish_event_fired: true) + create(:release, project: project2, released_at: today, created_at: yesterday, release_published_at: today) end let(:args) { {} } diff --git a/spec/workers/releases/publish_event_worker_spec.rb b/spec/workers/releases/publish_event_worker_spec.rb index 32cb1f52bea09a..c5c5949cf812e1 100644 --- a/spec/workers/releases/publish_event_worker_spec.rb +++ b/spec/workers/releases/publish_event_worker_spec.rb @@ -16,6 +16,6 @@ end it 'sets the release as published' do - expect(release.publish_event_fired).to be_truthy + expect(release.release_published_at).not_to be_nil end end -- GitLab From 8ba9e57be9d7532234a8104509edfe3898aed0ed Mon Sep 17 00:00:00 2001 From: Olivier El Mekki Date: Tue, 12 Dec 2023 17:41:55 +0100 Subject: [PATCH 05/13] REFACTOR make the cron job run more often --- config/initializers/1_settings.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index b772a948dae8f2..cd2bfd85d0a303 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -635,7 +635,7 @@ Settings.cron_jobs['manage_evidence_worker']['cron'] ||= '0 * * * *' Settings.cron_jobs['manage_evidence_worker']['job_class'] = 'Releases::ManageEvidenceWorker' Settings.cron_jobs['publish_release_worker'] ||= {} -Settings.cron_jobs['publish_release_worker']['cron'] ||= '20 * * * *' +Settings.cron_jobs['publish_release_worker']['cron'] ||= '20,50 * * * *' Settings.cron_jobs['publish_release_worker']['job_class'] = 'Releases::PublishEventWorker' Settings.cron_jobs['user_status_cleanup_batch_worker'] ||= {} Settings.cron_jobs['user_status_cleanup_batch_worker']['cron'] ||= '* * * * *' -- GitLab From 6b03ce5cdf6fb286c875fb9f754485f98c2b771f Mon Sep 17 00:00:00 2001 From: Olivier El Mekki Date: Tue, 12 Dec 2023 17:49:21 +0100 Subject: [PATCH 06/13] ADD log the amount of release published --- app/workers/releases/publish_event_worker.rb | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/app/workers/releases/publish_event_worker.rb b/app/workers/releases/publish_event_worker.rb index 293debc0ac50c3..aaaa1166a27a2a 100644 --- a/app/workers/releases/publish_event_worker.rb +++ b/app/workers/releases/publish_event_worker.rb @@ -12,6 +12,8 @@ class PublishEventWorker feature_category :release_orchestration def perform + releases_published = 0 + Release.waiting_for_publish_event.each do |release| with_context(project: release.project) do release.touch(:release_published_at) @@ -19,8 +21,12 @@ def perform ::Gitlab::EventStore.publish( ::Projects::ReleasePublishedEvent.new(data: { release_id: release.id }) ) + + releases_published += 1 end end + + log_extra_metadata_on_done(:releases_published, releases_published) if releases_published > 0 end end end -- GitLab From 6c9f27913b548ce815204ed3fee85f233bfe22d0 Mon Sep 17 00:00:00 2001 From: Olivier El Mekki Date: Wed, 13 Dec 2023 18:09:44 +0100 Subject: [PATCH 07/13] REFACTOR add batching and limits to the worker --- app/models/release.rb | 3 ++- app/workers/releases/publish_event_worker.rb | 18 ++++++++++-------- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/app/models/release.rb b/app/models/release.rb index ec7bc35708714c..c2bc044440316c 100644 --- a/app/models/release.rb +++ b/app/models/release.rb @@ -54,7 +54,7 @@ class Release < ApplicationRecord scope :recent, -> { sorted.limit(MAX_NUMBER_TO_DISPLAY) } scope :without_evidence, -> { left_joins(:evidences).where(::Releases::Evidence.arel_table[:id].eq(nil)) } scope :released_within_2hrs, -> { where(released_at: Time.zone.now - 1.hour..Time.zone.now + 1.hour) } - scope :publishing_not_advertised, -> { where(release_published_at: nil) } + scope :publishing_not_advertised, -> { where(release_published_at: nil).limit(MAX_NUMBER_TO_PUBLISH) } scope :for_projects, ->(projects) { where(project_id: projects) } scope :by_tag, ->(tag) { where(tag: tag) } @@ -67,6 +67,7 @@ class Release < ApplicationRecord delegate :repository, to: :project MAX_NUMBER_TO_DISPLAY = 3 + MAX_NUMBER_TO_PUBLISH = 5000 class << self # In the future, we should support `order_by=semver`; diff --git a/app/workers/releases/publish_event_worker.rb b/app/workers/releases/publish_event_worker.rb index aaaa1166a27a2a..cbc60201775d35 100644 --- a/app/workers/releases/publish_event_worker.rb +++ b/app/workers/releases/publish_event_worker.rb @@ -14,16 +14,18 @@ class PublishEventWorker def perform releases_published = 0 - Release.waiting_for_publish_event.each do |release| - with_context(project: release.project) do - release.touch(:release_published_at) + Release.waiting_for_publish_event.each_batch(of: 100) do |releases| + releases.each do |release| + with_context(project: release.project) do + ::Gitlab::EventStore.publish( + ::Projects::ReleasePublishedEvent.new(data: { release_id: release.id }) + ) - ::Gitlab::EventStore.publish( - ::Projects::ReleasePublishedEvent.new(data: { release_id: release.id }) - ) - - releases_published += 1 + releases_published += 1 + end end + + releases.touch_all(:release_published_at) end log_extra_metadata_on_done(:releases_published, releases_published) if releases_published > 0 -- GitLab From 978f25736e2a5cf6721e43e47fdae252a14bf37f Mon Sep 17 00:00:00 2001 From: Olivier El Mekki Date: Thu, 14 Dec 2023 16:32:57 +0100 Subject: [PATCH 08/13] ADD check that releases_access_level is not 'disabled' --- app/workers/releases/publish_event_worker.rb | 10 +++--- .../releases/publish_event_worker_spec.rb | 34 ++++++++++++++++--- 2 files changed, 35 insertions(+), 9 deletions(-) diff --git a/app/workers/releases/publish_event_worker.rb b/app/workers/releases/publish_event_worker.rb index cbc60201775d35..8ec33c293a8a72 100644 --- a/app/workers/releases/publish_event_worker.rb +++ b/app/workers/releases/publish_event_worker.rb @@ -17,11 +17,13 @@ def perform Release.waiting_for_publish_event.each_batch(of: 100) do |releases| releases.each do |release| with_context(project: release.project) do - ::Gitlab::EventStore.publish( - ::Projects::ReleasePublishedEvent.new(data: { release_id: release.id }) - ) + unless release.project.releases_access_level == ProjectFeature::DISABLED + ::Gitlab::EventStore.publish( + ::Projects::ReleasePublishedEvent.new(data: { release_id: release.id }) + ) - releases_published += 1 + releases_published += 1 + end end end diff --git a/spec/workers/releases/publish_event_worker_spec.rb b/spec/workers/releases/publish_event_worker_spec.rb index c5c5949cf812e1..ed1b48c8cf95a6 100644 --- a/spec/workers/releases/publish_event_worker_spec.rb +++ b/spec/workers/releases/publish_event_worker_spec.rb @@ -8,14 +8,38 @@ before do allow(Gitlab::EventStore).to receive(:publish).and_return(true) - described_class.new.perform end - it 'broadcasts the published event' do - expect(Gitlab::EventStore).to have_received(:publish).with(Projects::ReleasePublishedEvent) + describe 'when the releases feature is not disabled' do + before do + project.update!(releases_access_level: 'enabled') + described_class.new.perform + end + + it 'broadcasts the published event' do + expect(Gitlab::EventStore).to have_received(:publish).with(Projects::ReleasePublishedEvent) + end + + it 'sets the release as published' do + expect(release.release_published_at).not_to be_nil + end end - it 'sets the release as published' do - expect(release.release_published_at).not_to be_nil + describe 'when the releases feature is disabled' do + before do + project.update!(releases_access_level: 'disabled') + described_class.new.perform + end + + it 'does not broadcasts the published event' do + expect(Gitlab::EventStore).not_to have_received(:publish).with(Projects::ReleasePublishedEvent) + end + + # Having a release created with the releases feature disabled is a bogus state anyway. + # Setting it as published prevents having such releases piling up forever in the + # `publishing_not_advertised` scope. + it 'sets the release as published' do + expect(release.release_published_at).not_to be_nil + end end end -- GitLab From a080e5760d8eb1711833f714ab409bb79e7cf48e Mon Sep 17 00:00:00 2001 From: Olivier El Mekki Date: Mon, 18 Dec 2023 16:38:51 +0100 Subject: [PATCH 09/13] REFACTOR simplify migration and enable lock retries --- .../20231212154022_add_release_published_at_to_release.rb | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/db/migrate/20231212154022_add_release_published_at_to_release.rb b/db/migrate/20231212154022_add_release_published_at_to_release.rb index d477a98004fd8b..15cf10f5d2a943 100644 --- a/db/migrate/20231212154022_add_release_published_at_to_release.rb +++ b/db/migrate/20231212154022_add_release_published_at_to_release.rb @@ -2,12 +2,9 @@ class AddReleasePublishedAtToRelease < Gitlab::Database::Migration[2.2] milestone '16.7' + enable_lock_retries! - def up + def change add_column :releases, :release_published_at, :datetime_with_timezone end - - def down - remove_column :releases, :release_published_at, :datetime_with_timezone - end end -- GitLab From e2a6e8fb4f968969b8eb8d26b3b41b7ac25e315b Mon Sep 17 00:00:00 2001 From: Olivier El Mekki Date: Wed, 20 Dec 2023 10:49:38 +0100 Subject: [PATCH 10/13] ADD index --- ...09_add_release_published_at_index_to_release.rb | 14 ++++++++++++++ db/schema_migrations/20231220094609 | 1 + db/structure.sql | 2 ++ 3 files changed, 17 insertions(+) create mode 100644 db/migrate/20231220094609_add_release_published_at_index_to_release.rb create mode 100644 db/schema_migrations/20231220094609 diff --git a/db/migrate/20231220094609_add_release_published_at_index_to_release.rb b/db/migrate/20231220094609_add_release_published_at_index_to_release.rb new file mode 100644 index 00000000000000..d3e793327e4ddc --- /dev/null +++ b/db/migrate/20231220094609_add_release_published_at_index_to_release.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +class AddReleasePublishedAtIndexToRelease < Gitlab::Database::Migration[2.2] + milestone '16.7' + disable_ddl_transaction! + + def up + add_concurrent_index :releases, :release_published_at, name: 'releases_published_at_index' + end + + def down + remove_concurrent_index :releases, :release_published_at, name: 'releases_published_at_index' + end +end diff --git a/db/schema_migrations/20231220094609 b/db/schema_migrations/20231220094609 new file mode 100644 index 00000000000000..30ece81d4a013b --- /dev/null +++ b/db/schema_migrations/20231220094609 @@ -0,0 +1 @@ +aab891f39866b4933cadd8295ecaa1c9f8a256cda832b734dfb1911580187bf3 \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 467beac0672326..e3900b7b7ebf22 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -35396,6 +35396,8 @@ CREATE INDEX partial_index_user_id_app_id_created_at_token_not_revoked ON oauth_ CREATE UNIQUE INDEX pm_checkpoints_path_components ON pm_checkpoints USING btree (purl_type, data_type, version_format); +CREATE INDEX releases_published_at_index ON releases USING btree (release_published_at); + CREATE INDEX scan_finding_approval_mr_rule_index_id ON approval_merge_request_rules USING btree (id) WHERE (report_type = 4); CREATE INDEX scan_finding_approval_mr_rule_index_merge_request_id ON approval_merge_request_rules USING btree (merge_request_id) WHERE (report_type = 4); -- GitLab From 865ae6a3fb8f5a8b7a5eae6288079a979192bbe6 Mon Sep 17 00:00:00 2001 From: Alper Akgun Date: Wed, 20 Dec 2023 14:43:08 +0100 Subject: [PATCH 11/13] REFACTOR change milestone --- .../20231212154022_add_release_published_at_to_release.rb | 2 +- .../20231220094609_add_release_published_at_index_to_release.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/db/migrate/20231212154022_add_release_published_at_to_release.rb b/db/migrate/20231212154022_add_release_published_at_to_release.rb index 15cf10f5d2a943..8ecb51a8cf12b1 100644 --- a/db/migrate/20231212154022_add_release_published_at_to_release.rb +++ b/db/migrate/20231212154022_add_release_published_at_to_release.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class AddReleasePublishedAtToRelease < Gitlab::Database::Migration[2.2] - milestone '16.7' + milestone '16.8' enable_lock_retries! def change diff --git a/db/migrate/20231220094609_add_release_published_at_index_to_release.rb b/db/migrate/20231220094609_add_release_published_at_index_to_release.rb index d3e793327e4ddc..bc5891ad1983ab 100644 --- a/db/migrate/20231220094609_add_release_published_at_index_to_release.rb +++ b/db/migrate/20231220094609_add_release_published_at_index_to_release.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class AddReleasePublishedAtIndexToRelease < Gitlab::Database::Migration[2.2] - milestone '16.7' + milestone '16.8' disable_ddl_transaction! def up -- GitLab From f8a3ce5f2404d2d7d6acac768a2cceb43d7b3e60 Mon Sep 17 00:00:00 2001 From: Olivier El Mekki Date: Thu, 21 Dec 2023 16:08:38 +0100 Subject: [PATCH 12/13] FIX n+1 and remove useless included modules --- app/models/release.rb | 4 ++-- app/workers/releases/publish_event_worker.rb | 12 ++++-------- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/app/models/release.rb b/app/models/release.rb index c2bc044440316c..40235a68c1c3f1 100644 --- a/app/models/release.rb +++ b/app/models/release.rb @@ -54,7 +54,7 @@ class Release < ApplicationRecord scope :recent, -> { sorted.limit(MAX_NUMBER_TO_DISPLAY) } scope :without_evidence, -> { left_joins(:evidences).where(::Releases::Evidence.arel_table[:id].eq(nil)) } scope :released_within_2hrs, -> { where(released_at: Time.zone.now - 1.hour..Time.zone.now + 1.hour) } - scope :publishing_not_advertised, -> { where(release_published_at: nil).limit(MAX_NUMBER_TO_PUBLISH) } + scope :publishing_not_advertised, -> { where(release_published_at: nil) } scope :for_projects, ->(projects) { where(project_id: projects) } scope :by_tag, ->(tag) { where(tag: tag) } @@ -101,7 +101,7 @@ def latest_for_projects(projects, order_by: 'released_at') end def waiting_for_publish_event - publishing_not_advertised.released_within_2hrs.includes(:project) + publishing_not_advertised.released_within_2hrs.joins(:project).merge(Project.with_feature_enabled(:releases)).limit(MAX_NUMBER_TO_PUBLISH) end end diff --git a/app/workers/releases/publish_event_worker.rb b/app/workers/releases/publish_event_worker.rb index 8ec33c293a8a72..8bcc580dceb6c4 100644 --- a/app/workers/releases/publish_event_worker.rb +++ b/app/workers/releases/publish_event_worker.rb @@ -3,9 +3,7 @@ module Releases class PublishEventWorker include ApplicationWorker - include Gitlab::ExclusiveLeaseHelpers include CronjobQueue - include Gitlab::Utils::StrongMemoize idempotent! data_consistency :always # rubocop:disable SidekiqLoadBalancing/WorkerDataConsistency -- usual for EventStore jobs. @@ -17,13 +15,11 @@ def perform Release.waiting_for_publish_event.each_batch(of: 100) do |releases| releases.each do |release| with_context(project: release.project) do - unless release.project.releases_access_level == ProjectFeature::DISABLED - ::Gitlab::EventStore.publish( - ::Projects::ReleasePublishedEvent.new(data: { release_id: release.id }) - ) + ::Gitlab::EventStore.publish( + ::Projects::ReleasePublishedEvent.new(data: { release_id: release.id }) + ) - releases_published += 1 - end + releases_published += 1 end end -- GitLab From 4de5b49234ff1586dac441a50e8d26bf2a4a2b7d Mon Sep 17 00:00:00 2001 From: Olivier El Mekki Date: Thu, 21 Dec 2023 19:14:21 +0100 Subject: [PATCH 13/13] REFACTOR publishing_not_advertised -> unpublished --- app/models/release.rb | 4 ++-- spec/workers/releases/publish_event_worker_spec.rb | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/models/release.rb b/app/models/release.rb index 40235a68c1c3f1..7bacc69f038239 100644 --- a/app/models/release.rb +++ b/app/models/release.rb @@ -54,7 +54,7 @@ class Release < ApplicationRecord scope :recent, -> { sorted.limit(MAX_NUMBER_TO_DISPLAY) } scope :without_evidence, -> { left_joins(:evidences).where(::Releases::Evidence.arel_table[:id].eq(nil)) } scope :released_within_2hrs, -> { where(released_at: Time.zone.now - 1.hour..Time.zone.now + 1.hour) } - scope :publishing_not_advertised, -> { where(release_published_at: nil) } + scope :unpublished, -> { where(release_published_at: nil) } scope :for_projects, ->(projects) { where(project_id: projects) } scope :by_tag, ->(tag) { where(tag: tag) } @@ -101,7 +101,7 @@ def latest_for_projects(projects, order_by: 'released_at') end def waiting_for_publish_event - publishing_not_advertised.released_within_2hrs.joins(:project).merge(Project.with_feature_enabled(:releases)).limit(MAX_NUMBER_TO_PUBLISH) + unpublished.released_within_2hrs.joins(:project).merge(Project.with_feature_enabled(:releases)).limit(MAX_NUMBER_TO_PUBLISH) end end diff --git a/spec/workers/releases/publish_event_worker_spec.rb b/spec/workers/releases/publish_event_worker_spec.rb index ed1b48c8cf95a6..86dd09a756fc46 100644 --- a/spec/workers/releases/publish_event_worker_spec.rb +++ b/spec/workers/releases/publish_event_worker_spec.rb @@ -37,7 +37,7 @@ # Having a release created with the releases feature disabled is a bogus state anyway. # Setting it as published prevents having such releases piling up forever in the - # `publishing_not_advertised` scope. + # `unpublished` scope. it 'sets the release as published' do expect(release.release_published_at).not_to be_nil end -- GitLab