From fc3b88090cd58b3e5eafa132b926a4f2479e9ea2 Mon Sep 17 00:00:00 2001 From: Kassio Borges Date: Fri, 2 Jun 2023 16:59:33 +0100 Subject: [PATCH 01/18] PoC: Add review apps for pages build on MRs --- app/models/ci/build.rb | 4 +++ app/models/merge_request.rb | 4 +++ app/models/pages/merge_request_review_app.rb | 23 ++++++++++++ app/models/project.rb | 4 +++ app/services/pages/deploy_review_app.rb | 35 +++++++++++++++++++ app/workers/pages_worker.rb | 7 +++- db/docs/pages_merge_request_review_apps.yml | 10 ++++++ ...create_pages_merge_requests_review_apps.rb | 18 ++++++++++ db/schema_migrations/20230602104800 | 1 + lib/gitlab/pages/virtual_host_finder.rb | 11 +++++- 10 files changed, 115 insertions(+), 2 deletions(-) create mode 100644 app/models/pages/merge_request_review_app.rb create mode 100644 app/services/pages/deploy_review_app.rb create mode 100644 db/docs/pages_merge_request_review_apps.yml create mode 100644 db/migrate/20230602104800_create_pages_merge_requests_review_apps.rb create mode 100644 db/schema_migrations/20230602104800 diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index bb1bfe8c889298..773a8972a76be6 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -66,6 +66,10 @@ class Build < Ci::Processable has_many :terraform_state_versions, class_name: 'Terraform::StateVersion', foreign_key: :ci_build_id, inverse_of: :build + has_many :pages_merge_request_review_apps, + class_name: 'Pages::MergeRequestReviewApp', + inverse_of: :ci_build + accepts_nested_attributes_for :runner_session, update_only: true accepts_nested_attributes_for :job_variables diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 116108ceaf9595..06be7490f5d4cf 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -120,6 +120,10 @@ def merge_request_diff has_many :created_environments, class_name: 'Environment', foreign_key: :merge_request_id, inverse_of: :merge_request has_many :assignment_events, class_name: 'ResourceEvents::MergeRequestAssignmentEvent', inverse_of: :merge_request + has_many :pages_merge_request_review_apps, + class_name: 'Pages::MergeRequestReviewApp', + inverse_of: :merge_request + KNOWN_MERGE_PARAMS = [ :auto_merge_strategy, :should_remove_source_branch, diff --git a/app/models/pages/merge_request_review_app.rb b/app/models/pages/merge_request_review_app.rb new file mode 100644 index 00000000000000..29d648ba8eda5f --- /dev/null +++ b/app/models/pages/merge_request_review_app.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +module Pages + class MergeRequestReviewApp < ::ApplicationRecord + self.table_name = 'pages_merge_request_review_apps' + + belongs_to :project, + class_name: 'Project', + inverse_of: :pages_merge_request_review_apps, + optional: false + + belongs_to :merge_request, + class_name: 'MergeRequest', + inverse_of: :pages_merge_request_review_apps, + optional: false + + belongs_to :ci_build, + class_name: 'Ci::Build', + inverse_of: :pages_merge_request_review_apps, + foreign_key: :build_id, + optional: false + end +end diff --git a/app/models/project.rb b/app/models/project.rb index 452a5c8973c395..63f3d06482f836 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -456,6 +456,10 @@ def self.integration_association_name(name) has_one :build_artifacts_size_refresh, class_name: 'Projects::BuildArtifactsSizeRefresh' + has_many :pages_merge_request_review_apps, + class_name: 'Pages::MergeRequestReviewApp', + inverse_of: :project + accepts_nested_attributes_for :variables, allow_destroy: true accepts_nested_attributes_for :project_feature, update_only: true accepts_nested_attributes_for :project_setting, update_only: true diff --git a/app/services/pages/deploy_review_app.rb b/app/services/pages/deploy_review_app.rb new file mode 100644 index 00000000000000..1666a115ad0e3b --- /dev/null +++ b/app/services/pages/deploy_review_app.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +module Pages + class DeployReviewApp + def initialize(build) + @project = build.project + @build = build + end + + def execute + Pages::MergeRequestReviewApp.create!( + project: project, + merge_request: build.merge_request, + build_id: build.id, + slug: slug + ) + end + + private + + attr_reader :project, :build + + def slug + namespace = project.root_namespace.path.slice(0, 10) + path = "#{namespace}-#{project.path.slice(0, 23)}" + sha = + Digest::SHA2 + .new(256) + .hexdigest("#{project.id}-#{build.merge_request.id}-#{build.id}") + .slice(0, 62 - path.length) + + Gitlab::Utils.slugify("#{path}-#{sha}") + end + end +end diff --git a/app/workers/pages_worker.rb b/app/workers/pages_worker.rb index adb6d38fd12958..53e0e2b15ce48b 100644 --- a/app/workers/pages_worker.rb +++ b/app/workers/pages_worker.rb @@ -17,6 +17,11 @@ def perform(action, *arg) def deploy(build_id) build = Ci::Build.find_by_id(build_id) - Projects::UpdatePagesService.new(build.project, build).execute + + if build.merge_request.present? + Pages::DeployReviewApp.new(build).execute + else + Projects::UpdatePagesService.new(build.project, build).execute + end end end diff --git a/db/docs/pages_merge_request_review_apps.yml b/db/docs/pages_merge_request_review_apps.yml new file mode 100644 index 00000000000000..b1ea82b7aed5d8 --- /dev/null +++ b/db/docs/pages_merge_request_review_apps.yml @@ -0,0 +1,10 @@ +--- +table_name: pages_merge_request_review_apps +classes: +- Pages::MergeRequestReviewApp +feature_categories: +- pages +description: Associates a review app slug with project, merge request and build +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/122498 +milestone: '16.1' +gitlab_schema: gitlab_main diff --git a/db/migrate/20230602104800_create_pages_merge_requests_review_apps.rb b/db/migrate/20230602104800_create_pages_merge_requests_review_apps.rb new file mode 100644 index 00000000000000..55787c70e0d2bd --- /dev/null +++ b/db/migrate/20230602104800_create_pages_merge_requests_review_apps.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +class CreatePagesMergeRequestsReviewApps < Gitlab::Database::Migration[2.1] + def up + create_table(:pages_merge_request_review_apps) do |t| + t.datetime_with_timezone :created_at, null: false + t.datetime_with_timezone :updated_at + t.references :project, index: true, null: false, foreign_key: { on_delete: :cascade } + t.references :merge_request, index: true, null: false, foreign_key: { on_delete: :cascade } + t.bigint :build_id, index: true, null: false + t.text :slug, null: false, limit: 255, unique: true + end + end + + def down + drop_table :pages_review_merge_requests + end +end diff --git a/db/schema_migrations/20230602104800 b/db/schema_migrations/20230602104800 new file mode 100644 index 00000000000000..b4608020eed5e7 --- /dev/null +++ b/db/schema_migrations/20230602104800 @@ -0,0 +1 @@ +28b8dcf7b5c028642d03844a9c440c9389e7452a40994a0feda798fad624cdc5 \ No newline at end of file diff --git a/lib/gitlab/pages/virtual_host_finder.rb b/lib/gitlab/pages/virtual_host_finder.rb index 5fec60188f8513..8d505462252669 100644 --- a/lib/gitlab/pages/virtual_host_finder.rb +++ b/lib/gitlab/pages/virtual_host_finder.rb @@ -16,7 +16,8 @@ def execute name = host.delete_suffix(gitlab_host) by_namespace_domain(name) || - by_unique_domain(name) + by_unique_domain(name) || + by_review_app_domain(name) else by_custom_domain(host) end @@ -26,6 +27,14 @@ def execute attr_reader :host + def by_review_app_domain(name) + deploy = Pages::ReviewAppDeploys.by_slug(name) + + return unless Feature.enabled?(:pages_review_apps, deploy.project) + + ::Pages::VirtualDomain.new(projects: [deploy.project]) + end + def by_unique_domain(name) project = Project.by_pages_enabled_unique_domain(name) -- GitLab From cc7c774d16771cabf9f10c02205c667e59324d71 Mon Sep 17 00:00:00 2001 From: Kassio Borges Date: Tue, 6 Jun 2023 22:06:59 +0100 Subject: [PATCH 02/18] Use a relation with PagesDeployment Pros: We can leverage from most of LookupPath logic Cons: We have to change the logic for the PagesDeployment clean up --- app/models/ci/build.rb | 4 --- app/models/pages/lookup_path.rb | 10 ++---- app/models/pages/merge_request_review_app.rb | 16 ++++----- app/models/pages/virtual_domain.rb | 21 +++++++----- app/models/pages_deployment.rb | 4 +++ app/models/project.rb | 8 ----- ....rb => create_merge_request_review_app.rb} | 12 +++---- app/services/projects/update_pages_service.rb | 10 ++++++ app/workers/pages_worker.rb | 7 +--- ...create_pages_merge_requests_review_apps.rb | 5 ++- db/structure.sql | 34 +++++++++++++++++++ lib/gitlab/pages/virtual_host_finder.rb | 12 +++++-- 12 files changed, 89 insertions(+), 54 deletions(-) rename app/services/pages/{deploy_review_app.rb => create_merge_request_review_app.rb} (69%) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 773a8972a76be6..bb1bfe8c889298 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -66,10 +66,6 @@ class Build < Ci::Processable has_many :terraform_state_versions, class_name: 'Terraform::StateVersion', foreign_key: :ci_build_id, inverse_of: :build - has_many :pages_merge_request_review_apps, - class_name: 'Pages::MergeRequestReviewApp', - inverse_of: :ci_build - accepts_nested_attributes_for :runner_session, update_only: true accepts_nested_attributes_for :job_variables diff --git a/app/models/pages/lookup_path.rb b/app/models/pages/lookup_path.rb index 864ea04c019031..df65f8717294fd 100644 --- a/app/models/pages/lookup_path.rb +++ b/app/models/pages/lookup_path.rb @@ -6,10 +6,11 @@ class LookupPath LegacyStorageDisabledError = Class.new(::StandardError) - def initialize(project, trim_prefix: nil, domain: nil) + def initialize(project, trim_prefix: nil, domain: nil, deployment: nil) @project = project @domain = domain @trim_prefix = trim_prefix || project.full_path + @deployment = deployment || project.pages_metadatum.pages_deployment end def project_id @@ -70,11 +71,6 @@ def root_directory private - attr_reader :project, :trim_prefix, :domain - - def deployment - project.pages_metadatum.pages_deployment - end - strong_memoize_attr :deployment + attr_reader :project, :trim_prefix, :domain, :deployment end end diff --git a/app/models/pages/merge_request_review_app.rb b/app/models/pages/merge_request_review_app.rb index 29d648ba8eda5f..e66a8b1fa71d86 100644 --- a/app/models/pages/merge_request_review_app.rb +++ b/app/models/pages/merge_request_review_app.rb @@ -4,20 +4,20 @@ module Pages class MergeRequestReviewApp < ::ApplicationRecord self.table_name = 'pages_merge_request_review_apps' - belongs_to :project, - class_name: 'Project', - inverse_of: :pages_merge_request_review_apps, - optional: false - belongs_to :merge_request, class_name: 'MergeRequest', inverse_of: :pages_merge_request_review_apps, optional: false - belongs_to :ci_build, - class_name: 'Ci::Build', + belongs_to :pages_deployment, + class_name: 'PagesDeployment', inverse_of: :pages_merge_request_review_apps, - foreign_key: :build_id, optional: false + + has_one :project, through: :pages_deployment + + validates :slug, presence: true, uniqueness: true + + scope :by_slug, ->(value) { find_by(slug: value) } end end diff --git a/app/models/pages/virtual_domain.rb b/app/models/pages/virtual_domain.rb index fafbe449c8c982..69a3e56419f12e 100644 --- a/app/models/pages/virtual_domain.rb +++ b/app/models/pages/virtual_domain.rb @@ -2,11 +2,12 @@ module Pages class VirtualDomain - def initialize(projects:, cache: nil, trim_prefix: nil, domain: nil) + def initialize(projects:, cache: nil, trim_prefix: nil, domain: nil, lookup_paths: nil) @projects = projects @cache = cache @trim_prefix = trim_prefix @domain = domain + @lookup_paths = lookup_paths end def certificate @@ -18,14 +19,10 @@ def key end def lookup_paths - paths = projects.map do |project| - project.pages_lookup_path(trim_prefix: trim_prefix, domain: domain) - end - - # TODO: remove in https://gitlab.com/gitlab-org/gitlab/-/issues/328715 - paths = paths.select(&:source) - - paths.sort_by(&:prefix).reverse + build_lookup_paths + .select(&:source) # TODO: remove in https://gitlab.com/gitlab-org/gitlab/-/issues/328715 + .sort_by(&:prefix) + .reverse end # cache_key is required by #present_cached in ::API::Internal::Pages @@ -36,5 +33,11 @@ def cache_key private attr_reader :projects, :trim_prefix, :domain, :cache + + def build_lookup_paths + @lookup_paths || projects.map do |project| + Pages::LookupPath.new(project, trim_prefix: trim_prefix, domain: domain) + end + end end end diff --git a/app/models/pages_deployment.rb b/app/models/pages_deployment.rb index fa29cbf835236c..98817f5d67ef9a 100644 --- a/app/models/pages_deployment.rb +++ b/app/models/pages_deployment.rb @@ -13,6 +13,10 @@ class PagesDeployment < ApplicationRecord belongs_to :project, optional: false belongs_to :ci_build, class_name: 'Ci::Build', optional: true + has_many :pages_merge_request_review_apps, + class_name: 'Pages::MergeRequestReviewApp', + inverse_of: :pages_deployment + scope :older_than, -> (id) { where('id < ?', id) } scope :migrated_from_legacy_storage, -> { where(file: MIGRATED_FILE_NAME) } scope :with_files_stored_locally, -> { where(file_store: ::ObjectStorage::Store::LOCAL) } diff --git a/app/models/project.rb b/app/models/project.rb index 63f3d06482f836..4d42829e178a29 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -456,10 +456,6 @@ def self.integration_association_name(name) has_one :build_artifacts_size_refresh, class_name: 'Projects::BuildArtifactsSizeRefresh' - has_many :pages_merge_request_review_apps, - class_name: 'Pages::MergeRequestReviewApp', - inverse_of: :project - accepts_nested_attributes_for :variables, allow_destroy: true accepts_nested_attributes_for :project_feature, update_only: true accepts_nested_attributes_for :project_setting, update_only: true @@ -2866,10 +2862,6 @@ def access_request_approvers_to_be_notified recipients end - def pages_lookup_path(trim_prefix: nil, domain: nil) - Pages::LookupPath.new(self, trim_prefix: trim_prefix, domain: domain) - end - def closest_setting(name) setting = read_attribute(name) setting = closest_namespace_setting(name) if setting.nil? diff --git a/app/services/pages/deploy_review_app.rb b/app/services/pages/create_merge_request_review_app.rb similarity index 69% rename from app/services/pages/deploy_review_app.rb rename to app/services/pages/create_merge_request_review_app.rb index 1666a115ad0e3b..55be9cf80fd914 100644 --- a/app/services/pages/deploy_review_app.rb +++ b/app/services/pages/create_merge_request_review_app.rb @@ -1,24 +1,24 @@ # frozen_string_literal: true module Pages - class DeployReviewApp - def initialize(build) - @project = build.project + class CreateMergeRequestReviewApp + def initialize(pages_deployment:, build:) + @pages_deployment = pages_deployment @build = build + @project = pages_deployment.project end def execute Pages::MergeRequestReviewApp.create!( - project: project, merge_request: build.merge_request, - build_id: build.id, + pages_deployment: pages_deployment, slug: slug ) end private - attr_reader :project, :build + attr_reader :project, :pages_deployment, :build def slug namespace = project.root_namespace.path.slice(0, 10) diff --git a/app/services/projects/update_pages_service.rb b/app/services/projects/update_pages_service.rb index 403f645392c237..7ec15b2a19116c 100644 --- a/app/services/projects/update_pages_service.rb +++ b/app/services/projects/update_pages_service.rb @@ -34,6 +34,7 @@ def execute if deployment_update.valid? update_project_pages_deployment(deployment) + create_merge_request_review_app(deployment) success else error(deployment_update.errors.first.full_message) @@ -142,5 +143,14 @@ def publish_deployed_event Gitlab::EventStore.publish(event) end + + def create_merge_request_review_app(deployment) + return unless build.merge_request.present? + + Pages::CreateMergeRequestReviewApp.new( + pages_deployment: deployment, + build: build + ).execute + end end end diff --git a/app/workers/pages_worker.rb b/app/workers/pages_worker.rb index 53e0e2b15ce48b..adb6d38fd12958 100644 --- a/app/workers/pages_worker.rb +++ b/app/workers/pages_worker.rb @@ -17,11 +17,6 @@ def perform(action, *arg) def deploy(build_id) build = Ci::Build.find_by_id(build_id) - - if build.merge_request.present? - Pages::DeployReviewApp.new(build).execute - else - Projects::UpdatePagesService.new(build.project, build).execute - end + Projects::UpdatePagesService.new(build.project, build).execute end end diff --git a/db/migrate/20230602104800_create_pages_merge_requests_review_apps.rb b/db/migrate/20230602104800_create_pages_merge_requests_review_apps.rb index 55787c70e0d2bd..ce62cd005cb9bb 100644 --- a/db/migrate/20230602104800_create_pages_merge_requests_review_apps.rb +++ b/db/migrate/20230602104800_create_pages_merge_requests_review_apps.rb @@ -5,14 +5,13 @@ def up create_table(:pages_merge_request_review_apps) do |t| t.datetime_with_timezone :created_at, null: false t.datetime_with_timezone :updated_at - t.references :project, index: true, null: false, foreign_key: { on_delete: :cascade } + t.references :pages_deployment, index: true, null: false, foreign_key: { on_delete: :cascade } t.references :merge_request, index: true, null: false, foreign_key: { on_delete: :cascade } - t.bigint :build_id, index: true, null: false t.text :slug, null: false, limit: 255, unique: true end end def down - drop_table :pages_review_merge_requests + drop_table :pages_merge_request_review_apps end end diff --git a/db/structure.sql b/db/structure.sql index 77b32db0edac9e..6f0694840d146d 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -20022,6 +20022,25 @@ CREATE SEQUENCE pages_domains_id_seq ALTER SEQUENCE pages_domains_id_seq OWNED BY pages_domains.id; +CREATE TABLE pages_merge_request_review_apps ( + id bigint NOT NULL, + created_at timestamp with time zone NOT NULL, + updated_at timestamp with time zone, + pages_deployment_id bigint NOT NULL, + merge_request_id bigint NOT NULL, + slug text NOT NULL, + CONSTRAINT check_b4f6eb173e CHECK ((char_length(slug) <= 255)) +); + +CREATE SEQUENCE pages_merge_request_review_apps_id_seq + START WITH 1 + INCREMENT BY 1 + NO MINVALUE + NO MAXVALUE + CACHE 1; + +ALTER SEQUENCE pages_merge_request_review_apps_id_seq OWNED BY pages_merge_request_review_apps.id; + CREATE TABLE path_locks ( id integer NOT NULL, path character varying NOT NULL, @@ -25614,6 +25633,8 @@ ALTER TABLE ONLY pages_domain_acme_orders ALTER COLUMN id SET DEFAULT nextval('p ALTER TABLE ONLY pages_domains ALTER COLUMN id SET DEFAULT nextval('pages_domains_id_seq'::regclass); +ALTER TABLE ONLY pages_merge_request_review_apps ALTER COLUMN id SET DEFAULT nextval('pages_merge_request_review_apps_id_seq'::regclass); + ALTER TABLE ONLY path_locks ALTER COLUMN id SET DEFAULT nextval('path_locks_id_seq'::regclass); ALTER TABLE ONLY personal_access_tokens ALTER COLUMN id SET DEFAULT nextval('personal_access_tokens_id_seq'::regclass); @@ -27932,6 +27953,9 @@ ALTER TABLE ONLY pages_domain_acme_orders ALTER TABLE ONLY pages_domains ADD CONSTRAINT pages_domains_pkey PRIMARY KEY (id); +ALTER TABLE ONLY pages_merge_request_review_apps + ADD CONSTRAINT pages_merge_request_review_apps_pkey PRIMARY KEY (id); + ALTER TABLE ONLY path_locks ADD CONSTRAINT path_locks_pkey PRIMARY KEY (id); @@ -32212,6 +32236,10 @@ CREATE INDEX index_pages_domains_on_verified_at_and_enabled_until ON pages_domai CREATE INDEX index_pages_domains_on_wildcard ON pages_domains USING btree (wildcard); +CREATE INDEX index_pages_merge_request_review_apps_on_merge_request_id ON pages_merge_request_review_apps USING btree (merge_request_id); + +CREATE INDEX index_pages_merge_request_review_apps_on_pages_deployment_id ON pages_merge_request_review_apps USING btree (pages_deployment_id); + CREATE INDEX p_ci_builds_user_id_name_idx ON ONLY p_ci_builds USING btree (user_id, name) WHERE (((type)::text = 'Ci::Build'::text) AND ((name)::text = ANY (ARRAY[('container_scanning'::character varying)::text, ('dast'::character varying)::text, ('dependency_scanning'::character varying)::text, ('license_management'::character varying)::text, ('license_scanning'::character varying)::text, ('sast'::character varying)::text, ('coverage_fuzzing'::character varying)::text, ('secret_detection'::character varying)::text]))); CREATE INDEX index_partial_ci_builds_on_user_id_name_parser_features ON ci_builds USING btree (user_id, name) WHERE (((type)::text = 'Ci::Build'::text) AND ((name)::text = ANY (ARRAY[('container_scanning'::character varying)::text, ('dast'::character varying)::text, ('dependency_scanning'::character varying)::text, ('license_management'::character varying)::text, ('license_scanning'::character varying)::text, ('sast'::character varying)::text, ('coverage_fuzzing'::character varying)::text, ('secret_detection'::character varying)::text]))); @@ -36496,6 +36524,9 @@ ALTER TABLE ONLY boards_epic_user_preferences ALTER TABLE ONLY group_wiki_repositories ADD CONSTRAINT fk_rails_26f867598c FOREIGN KEY (group_id) REFERENCES namespaces(id) ON DELETE CASCADE; +ALTER TABLE ONLY pages_merge_request_review_apps + ADD CONSTRAINT fk_rails_26fd306173 FOREIGN KEY (merge_request_id) REFERENCES merge_requests(id) ON DELETE CASCADE; + ALTER TABLE ONLY lfs_file_locks ADD CONSTRAINT fk_rails_27a1d98fa8 FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE CASCADE; @@ -37810,6 +37841,9 @@ ALTER TABLE ONLY merge_request_blocks ALTER TABLE ONLY protected_branch_unprotect_access_levels ADD CONSTRAINT fk_rails_e9eb8dc025 FOREIGN KEY (protected_branch_id) REFERENCES protected_branches(id) ON DELETE CASCADE; +ALTER TABLE ONLY pages_merge_request_review_apps + ADD CONSTRAINT fk_rails_ea97949acd FOREIGN KEY (pages_deployment_id) REFERENCES pages_deployments(id) ON DELETE CASCADE; + ALTER TABLE ONLY alert_management_alert_user_mentions ADD CONSTRAINT fk_rails_eb2de0cdef FOREIGN KEY (note_id) REFERENCES notes(id) ON DELETE CASCADE; diff --git a/lib/gitlab/pages/virtual_host_finder.rb b/lib/gitlab/pages/virtual_host_finder.rb index 8d505462252669..b88caf27c62af5 100644 --- a/lib/gitlab/pages/virtual_host_finder.rb +++ b/lib/gitlab/pages/virtual_host_finder.rb @@ -28,11 +28,17 @@ def execute attr_reader :host def by_review_app_domain(name) - deploy = Pages::ReviewAppDeploys.by_slug(name) + review_app = ::Pages::MergeRequestReviewApp.by_slug(name) - return unless Feature.enabled?(:pages_review_apps, deploy.project) + return unless review_app - ::Pages::VirtualDomain.new(projects: [deploy.project]) + ::Pages::VirtualDomain.new( + projects: [review_app.project], + lookup_paths: [::Pages::LookupPath.new( + review_app.project, + deployment: review_app.pages_deployment + )] + ) end def by_unique_domain(name) -- GitLab From 0bcec254eb9087e3a5114e360e3fdf2237bf94cf Mon Sep 17 00:00:00 2001 From: Kassio Borges Date: Wed, 7 Jun 2023 20:48:30 +0100 Subject: [PATCH 03/18] Ensure to only update the deploy version in project_pages_metadatum when the build is not on a MR --- app/services/projects/update_pages_service.rb | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/app/services/projects/update_pages_service.rb b/app/services/projects/update_pages_service.rb index 7ec15b2a19116c..1cdcf9708d4cf0 100644 --- a/app/services/projects/update_pages_service.rb +++ b/app/services/projects/update_pages_service.rb @@ -31,14 +31,21 @@ def execute deployment = create_pages_deployment(artifacts_path, build) break error('The uploaded artifact size does not match the expected value') unless deployment - - if deployment_update.valid? - update_project_pages_deployment(deployment) + break error(deployment_update.errors.first.full_message) unless deployment_update.valid? + + # TODO:: to avoid "overwriting" the production pages deploy + # we only create the review app but do not update the pages deployment id in + # the project_pages_metadatum + # + # On the final version, the creation of the review app deploy will be a + # manual process by the user, which we can probably extract from here. + if build.merge_request.present? create_merge_request_review_app(deployment) - success else - error(deployment_update.errors.first.full_message) + update_project_pages_deployment(deployment) end + + success end rescue StandardError => e error(e.message) @@ -145,8 +152,6 @@ def publish_deployed_event end def create_merge_request_review_app(deployment) - return unless build.merge_request.present? - Pages::CreateMergeRequestReviewApp.new( pages_deployment: deployment, build: build -- GitLab From 51f292a4dce89a4112e8dc186e5f06ba7fee990b Mon Sep 17 00:00:00 2001 From: Kassio Borges Date: Wed, 7 Jun 2023 23:16:59 +0100 Subject: [PATCH 04/18] Only build pages-preview for pages:preview job --- app/models/ci/build.rb | 6 ++++++ app/models/pages/merge_request_review_app.rb | 4 ++++ app/models/project.rb | 14 +++++++------- .../pages/create_merge_request_review_app.rb | 4 ++-- app/services/projects/update_pages_service.rb | 11 +++++++---- app/workers/pages_worker.rb | 8 ++++---- 6 files changed, 30 insertions(+), 17 deletions(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index bb1bfe8c889298..00d6c7f84e3f2b 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -329,6 +329,7 @@ def clone_accessors build.run_after_commit do BuildSuccessWorker.perform_async(id) PagesWorker.perform_async(:deploy, id) if build.pages_generator? + PagesWorker.perform_async(:deploy, id, true) if build.pages_preview_generator? end end @@ -413,6 +414,11 @@ def pages_generator? name == 'pages' end + def pages_preview_generator? + Gitlab.config.pages.enabled && + name == 'pages:preview' + end + def runnable? true end diff --git a/app/models/pages/merge_request_review_app.rb b/app/models/pages/merge_request_review_app.rb index e66a8b1fa71d86..56b694aeb18bf3 100644 --- a/app/models/pages/merge_request_review_app.rb +++ b/app/models/pages/merge_request_review_app.rb @@ -19,5 +19,9 @@ class MergeRequestReviewApp < ::ApplicationRecord validates :slug, presence: true, uniqueness: true scope :by_slug, ->(value) { find_by(slug: value) } + + def url + project.pages_url_for(slug) + end end end diff --git a/app/models/project.rb b/app/models/project.rb index 4d42829e178a29..9cf2e83abec13e 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -3237,13 +3237,6 @@ def frozen_outbound_job_token_scopes? end strong_memoize_attr :frozen_outbound_job_token_scopes? - private - - def pages_unique_domain_enabled? - Feature.enabled?(:pages_unique_domain, self) && - project_setting.pages_unique_domain_enabled? - end - def pages_url_for(domain) # The host in URL always needs to be downcased Gitlab.config.pages.url.sub(%r{^https?://}) do |prefix| @@ -3251,6 +3244,13 @@ def pages_url_for(domain) end.downcase end + private + + def pages_unique_domain_enabled? + Feature.enabled?(:pages_unique_domain, self) && + project_setting.pages_unique_domain_enabled? + end + # overridden in EE def project_group_links_with_preload project_group_links diff --git a/app/services/pages/create_merge_request_review_app.rb b/app/services/pages/create_merge_request_review_app.rb index 55be9cf80fd914..5acc61693c1724 100644 --- a/app/services/pages/create_merge_request_review_app.rb +++ b/app/services/pages/create_merge_request_review_app.rb @@ -10,7 +10,7 @@ def initialize(pages_deployment:, build:) def execute Pages::MergeRequestReviewApp.create!( - merge_request: build.merge_request, + merge_request: MergeRequest.last, pages_deployment: pages_deployment, slug: slug ) @@ -26,7 +26,7 @@ def slug sha = Digest::SHA2 .new(256) - .hexdigest("#{project.id}-#{build.merge_request.id}-#{build.id}") + .hexdigest("#{project.id}-#{build.id}") .slice(0, 62 - path.length) Gitlab::Utils.slugify("#{path}-#{sha}") diff --git a/app/services/projects/update_pages_service.rb b/app/services/projects/update_pages_service.rb index 1cdcf9708d4cf0..a078703708b0de 100644 --- a/app/services/projects/update_pages_service.rb +++ b/app/services/projects/update_pages_service.rb @@ -9,10 +9,11 @@ class UpdatePagesService < BaseService attr_reader :build, :deployment_update - def initialize(project, build) + def initialize(project, build, pages_preview = false) @project = project @build = build @deployment_update = ::Gitlab::Pages::DeploymentUpdate.new(project, build) + @pages_preview = pages_preview end def execute @@ -39,8 +40,8 @@ def execute # # On the final version, the creation of the review app deploy will be a # manual process by the user, which we can probably extract from here. - if build.merge_request.present? - create_merge_request_review_app(deployment) + if pages_preview + create_pages_preview(deployment) else update_project_pages_deployment(deployment) end @@ -54,6 +55,8 @@ def execute private + attr_reader :pages_preview + def success @commit_status.success @project.mark_pages_as_deployed @@ -151,7 +154,7 @@ def publish_deployed_event Gitlab::EventStore.publish(event) end - def create_merge_request_review_app(deployment) + def create_pages_preview(deployment) Pages::CreateMergeRequestReviewApp.new( pages_deployment: deployment, build: build diff --git a/app/workers/pages_worker.rb b/app/workers/pages_worker.rb index adb6d38fd12958..a4fa29824a17da 100644 --- a/app/workers/pages_worker.rb +++ b/app/workers/pages_worker.rb @@ -10,13 +10,13 @@ class PagesWorker # rubocop:disable Scalability/IdempotentWorker loggable_arguments 0, 1 worker_resource_boundary :cpu - def perform(action, *arg) - deploy(*arg) if action == 'deploy' + def perform(action, build_id, pages_preview = false) + deploy(build_id, pages_preview) if action == 'deploy' end - def deploy(build_id) + def deploy(build_id, pages_preview) build = Ci::Build.find_by_id(build_id) - Projects::UpdatePagesService.new(build.project, build).execute + Projects::UpdatePagesService.new(build.project, build, pages_preview).execute end end -- GitLab From b3fbad01006c5a838c01e211de6d9454ee103841 Mon Sep 17 00:00:00 2001 From: Kassio Borges Date: Thu, 8 Jun 2023 16:16:59 +0100 Subject: [PATCH 05/18] Rename to MergeRequestPreviews --- app/models/ci/build.rb | 2 +- app/models/merge_request.rb | 2 +- ...review_app.rb => merge_request_preview.rb} | 8 +--- app/models/pages_deployment.rb | 2 +- .../pages/create_merge_request_preview.rb | 39 +++++++++++++++++++ .../pages/create_merge_request_review_app.rb | 35 ----------------- app/services/projects/update_pages_service.rb | 17 ++++---- .../pages/merge_request_review_worker.rb | 26 +++++++++++++ app/workers/pages_worker.rb | 8 ++-- db/docs/pages_merge_request_review_apps.yml | 4 +- ...00_create_pages_merge_request_previews.rb} | 8 ++-- db/structure.sql | 34 ---------------- lib/gitlab/pages/virtual_host_finder.rb | 2 +- 13 files changed, 90 insertions(+), 97 deletions(-) rename app/models/pages/{merge_request_review_app.rb => merge_request_preview.rb} (75%) create mode 100644 app/services/pages/create_merge_request_preview.rb delete mode 100644 app/services/pages/create_merge_request_review_app.rb create mode 100644 app/workers/pages/merge_request_review_worker.rb rename db/migrate/{20230602104800_create_pages_merge_requests_review_apps.rb => 20230602104800_create_pages_merge_request_previews.rb} (54%) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 00d6c7f84e3f2b..fa4e79f680f005 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -329,7 +329,7 @@ def clone_accessors build.run_after_commit do BuildSuccessWorker.perform_async(id) PagesWorker.perform_async(:deploy, id) if build.pages_generator? - PagesWorker.perform_async(:deploy, id, true) if build.pages_preview_generator? + Pages::MergeRequestReviewWorker.perform_async(id) if build.pages_preview_generator? end end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 06be7490f5d4cf..dab69d8b662f3a 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -121,7 +121,7 @@ def merge_request_diff has_many :assignment_events, class_name: 'ResourceEvents::MergeRequestAssignmentEvent', inverse_of: :merge_request has_many :pages_merge_request_review_apps, - class_name: 'Pages::MergeRequestReviewApp', + class_name: 'Pages::MergeRequestPreviews', inverse_of: :merge_request KNOWN_MERGE_PARAMS = [ diff --git a/app/models/pages/merge_request_review_app.rb b/app/models/pages/merge_request_preview.rb similarity index 75% rename from app/models/pages/merge_request_review_app.rb rename to app/models/pages/merge_request_preview.rb index 56b694aeb18bf3..3fcb75329ed8d5 100644 --- a/app/models/pages/merge_request_review_app.rb +++ b/app/models/pages/merge_request_preview.rb @@ -1,8 +1,8 @@ # frozen_string_literal: true module Pages - class MergeRequestReviewApp < ::ApplicationRecord - self.table_name = 'pages_merge_request_review_apps' + class MergeRequestPreview < ::ApplicationRecord + self.table_name = 'pages_merge_request_previews' belongs_to :merge_request, class_name: 'MergeRequest', @@ -19,9 +19,5 @@ class MergeRequestReviewApp < ::ApplicationRecord validates :slug, presence: true, uniqueness: true scope :by_slug, ->(value) { find_by(slug: value) } - - def url - project.pages_url_for(slug) - end end end diff --git a/app/models/pages_deployment.rb b/app/models/pages_deployment.rb index 98817f5d67ef9a..b0360297269e9e 100644 --- a/app/models/pages_deployment.rb +++ b/app/models/pages_deployment.rb @@ -14,7 +14,7 @@ class PagesDeployment < ApplicationRecord belongs_to :ci_build, class_name: 'Ci::Build', optional: true has_many :pages_merge_request_review_apps, - class_name: 'Pages::MergeRequestReviewApp', + class_name: 'Pages::MergeRequestPreviews', inverse_of: :pages_deployment scope :older_than, -> (id) { where('id < ?', id) } diff --git a/app/services/pages/create_merge_request_preview.rb b/app/services/pages/create_merge_request_preview.rb new file mode 100644 index 00000000000000..4d787b1253ddd2 --- /dev/null +++ b/app/services/pages/create_merge_request_preview.rb @@ -0,0 +1,39 @@ +# frozen_string_literal: true + +module Pages + class CreateMergeRequestPreview + def initialize(pages_deployment:, merge_request_id:) + @pages_deployment = pages_deployment + @merge_request_id = merge_request_id + @project = pages_deployment.project + end + + def execute + Pages::MergeRequestPreview.find_or_initialize_by(merge_request_id: merge_request_id).tap do |preview| + if preview.persisted? + # TODO: Delete existing pages preview deploy for this MR + else + preview.slug = slug + end + + preview.pages_deployment_id = pages_deployment.id + preview.save! + end + end + + private + + attr_reader :project, :pages_deployment, :merge_request_id + + def slug + path = "preview-#{project.path.slice(0, 23)}" + sha = + Digest::SHA2 + .new(256) + .hexdigest("#{path}-#{merge_request_id}") + .slice(0, 62 - path.length) + + Gitlab::Utils.slugify("#{path}-#{sha}") + end + end +end diff --git a/app/services/pages/create_merge_request_review_app.rb b/app/services/pages/create_merge_request_review_app.rb deleted file mode 100644 index 5acc61693c1724..00000000000000 --- a/app/services/pages/create_merge_request_review_app.rb +++ /dev/null @@ -1,35 +0,0 @@ -# frozen_string_literal: true - -module Pages - class CreateMergeRequestReviewApp - def initialize(pages_deployment:, build:) - @pages_deployment = pages_deployment - @build = build - @project = pages_deployment.project - end - - def execute - Pages::MergeRequestReviewApp.create!( - merge_request: MergeRequest.last, - pages_deployment: pages_deployment, - slug: slug - ) - end - - private - - attr_reader :project, :pages_deployment, :build - - def slug - namespace = project.root_namespace.path.slice(0, 10) - path = "#{namespace}-#{project.path.slice(0, 23)}" - sha = - Digest::SHA2 - .new(256) - .hexdigest("#{project.id}-#{build.id}") - .slice(0, 62 - path.length) - - Gitlab::Utils.slugify("#{path}-#{sha}") - end - end -end diff --git a/app/services/projects/update_pages_service.rb b/app/services/projects/update_pages_service.rb index a078703708b0de..7f8d45869a3c73 100644 --- a/app/services/projects/update_pages_service.rb +++ b/app/services/projects/update_pages_service.rb @@ -9,11 +9,11 @@ class UpdatePagesService < BaseService attr_reader :build, :deployment_update - def initialize(project, build, pages_preview = false) + def initialize(project, build, merge_request_preview: false) @project = project @build = build @deployment_update = ::Gitlab::Pages::DeploymentUpdate.new(project, build) - @pages_preview = pages_preview + @merge_request_preview = merge_request_preview end def execute @@ -40,8 +40,8 @@ def execute # # On the final version, the creation of the review app deploy will be a # manual process by the user, which we can probably extract from here. - if pages_preview - create_pages_preview(deployment) + if merge_request_preview + create_merge_request_preview(deployment) else update_project_pages_deployment(deployment) end @@ -55,7 +55,7 @@ def execute private - attr_reader :pages_preview + attr_reader :merge_request_preview def success @commit_status.success @@ -154,10 +154,11 @@ def publish_deployed_event Gitlab::EventStore.publish(event) end - def create_pages_preview(deployment) - Pages::CreateMergeRequestReviewApp.new( + def create_merge_request_preview(deployment) + Pages::CreateMergeRequestPreview.new( pages_deployment: deployment, - build: build + # TODO: for some reason, build.merge_request sometimes is nil + merge_request_id: build.pipeline.all_merge_requests.order(iid: :asc).first.id ).execute end end diff --git a/app/workers/pages/merge_request_review_worker.rb b/app/workers/pages/merge_request_review_worker.rb new file mode 100644 index 00000000000000..79dec4b5680cc9 --- /dev/null +++ b/app/workers/pages/merge_request_review_worker.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +module Pages + class MergeRequestReviewWorker + include ApplicationWorker + + data_consistency :always + + sidekiq_options retry: 3 + feature_category :pages + worker_resource_boundary :cpu + + def perform(build_id) + build = Ci::Build.find_by_id(build_id) + + # TODO: for some reason, build.merge_request sometimes is nil + return if build.pipeline.all_merge_requests.order(iid: :asc).first.blank? + + Projects::UpdatePagesService.new( + build.project, + build, + merge_request_preview: true + ).execute + end + end +end diff --git a/app/workers/pages_worker.rb b/app/workers/pages_worker.rb index a4fa29824a17da..adb6d38fd12958 100644 --- a/app/workers/pages_worker.rb +++ b/app/workers/pages_worker.rb @@ -10,13 +10,13 @@ class PagesWorker # rubocop:disable Scalability/IdempotentWorker loggable_arguments 0, 1 worker_resource_boundary :cpu - def perform(action, build_id, pages_preview = false) - deploy(build_id, pages_preview) if action == 'deploy' + def perform(action, *arg) + deploy(*arg) if action == 'deploy' end - def deploy(build_id, pages_preview) + def deploy(build_id) build = Ci::Build.find_by_id(build_id) - Projects::UpdatePagesService.new(build.project, build, pages_preview).execute + Projects::UpdatePagesService.new(build.project, build).execute end end diff --git a/db/docs/pages_merge_request_review_apps.yml b/db/docs/pages_merge_request_review_apps.yml index b1ea82b7aed5d8..5785f3fc1b0802 100644 --- a/db/docs/pages_merge_request_review_apps.yml +++ b/db/docs/pages_merge_request_review_apps.yml @@ -1,7 +1,7 @@ --- -table_name: pages_merge_request_review_apps +table_name: pages_merge_request_previews classes: -- Pages::MergeRequestReviewApp +- Pages::MergeRequestPreviews feature_categories: - pages description: Associates a review app slug with project, merge request and build diff --git a/db/migrate/20230602104800_create_pages_merge_requests_review_apps.rb b/db/migrate/20230602104800_create_pages_merge_request_previews.rb similarity index 54% rename from db/migrate/20230602104800_create_pages_merge_requests_review_apps.rb rename to db/migrate/20230602104800_create_pages_merge_request_previews.rb index ce62cd005cb9bb..791f9fa90db43d 100644 --- a/db/migrate/20230602104800_create_pages_merge_requests_review_apps.rb +++ b/db/migrate/20230602104800_create_pages_merge_request_previews.rb @@ -1,17 +1,17 @@ # frozen_string_literal: true -class CreatePagesMergeRequestsReviewApps < Gitlab::Database::Migration[2.1] +class CreatePagesMergeRequestPreviews < Gitlab::Database::Migration[2.1] def up - create_table(:pages_merge_request_review_apps) do |t| + create_table(:pages_merge_request_previews) do |t| t.datetime_with_timezone :created_at, null: false t.datetime_with_timezone :updated_at t.references :pages_deployment, index: true, null: false, foreign_key: { on_delete: :cascade } - t.references :merge_request, index: true, null: false, foreign_key: { on_delete: :cascade } + t.references :merge_request, index: { unique: true }, null: false, foreign_key: { on_delete: :cascade } t.text :slug, null: false, limit: 255, unique: true end end def down - drop_table :pages_merge_request_review_apps + drop_table :pages_merge_request_previews end end diff --git a/db/structure.sql b/db/structure.sql index 6f0694840d146d..77b32db0edac9e 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -20022,25 +20022,6 @@ CREATE SEQUENCE pages_domains_id_seq ALTER SEQUENCE pages_domains_id_seq OWNED BY pages_domains.id; -CREATE TABLE pages_merge_request_review_apps ( - id bigint NOT NULL, - created_at timestamp with time zone NOT NULL, - updated_at timestamp with time zone, - pages_deployment_id bigint NOT NULL, - merge_request_id bigint NOT NULL, - slug text NOT NULL, - CONSTRAINT check_b4f6eb173e CHECK ((char_length(slug) <= 255)) -); - -CREATE SEQUENCE pages_merge_request_review_apps_id_seq - START WITH 1 - INCREMENT BY 1 - NO MINVALUE - NO MAXVALUE - CACHE 1; - -ALTER SEQUENCE pages_merge_request_review_apps_id_seq OWNED BY pages_merge_request_review_apps.id; - CREATE TABLE path_locks ( id integer NOT NULL, path character varying NOT NULL, @@ -25633,8 +25614,6 @@ ALTER TABLE ONLY pages_domain_acme_orders ALTER COLUMN id SET DEFAULT nextval('p ALTER TABLE ONLY pages_domains ALTER COLUMN id SET DEFAULT nextval('pages_domains_id_seq'::regclass); -ALTER TABLE ONLY pages_merge_request_review_apps ALTER COLUMN id SET DEFAULT nextval('pages_merge_request_review_apps_id_seq'::regclass); - ALTER TABLE ONLY path_locks ALTER COLUMN id SET DEFAULT nextval('path_locks_id_seq'::regclass); ALTER TABLE ONLY personal_access_tokens ALTER COLUMN id SET DEFAULT nextval('personal_access_tokens_id_seq'::regclass); @@ -27953,9 +27932,6 @@ ALTER TABLE ONLY pages_domain_acme_orders ALTER TABLE ONLY pages_domains ADD CONSTRAINT pages_domains_pkey PRIMARY KEY (id); -ALTER TABLE ONLY pages_merge_request_review_apps - ADD CONSTRAINT pages_merge_request_review_apps_pkey PRIMARY KEY (id); - ALTER TABLE ONLY path_locks ADD CONSTRAINT path_locks_pkey PRIMARY KEY (id); @@ -32236,10 +32212,6 @@ CREATE INDEX index_pages_domains_on_verified_at_and_enabled_until ON pages_domai CREATE INDEX index_pages_domains_on_wildcard ON pages_domains USING btree (wildcard); -CREATE INDEX index_pages_merge_request_review_apps_on_merge_request_id ON pages_merge_request_review_apps USING btree (merge_request_id); - -CREATE INDEX index_pages_merge_request_review_apps_on_pages_deployment_id ON pages_merge_request_review_apps USING btree (pages_deployment_id); - CREATE INDEX p_ci_builds_user_id_name_idx ON ONLY p_ci_builds USING btree (user_id, name) WHERE (((type)::text = 'Ci::Build'::text) AND ((name)::text = ANY (ARRAY[('container_scanning'::character varying)::text, ('dast'::character varying)::text, ('dependency_scanning'::character varying)::text, ('license_management'::character varying)::text, ('license_scanning'::character varying)::text, ('sast'::character varying)::text, ('coverage_fuzzing'::character varying)::text, ('secret_detection'::character varying)::text]))); CREATE INDEX index_partial_ci_builds_on_user_id_name_parser_features ON ci_builds USING btree (user_id, name) WHERE (((type)::text = 'Ci::Build'::text) AND ((name)::text = ANY (ARRAY[('container_scanning'::character varying)::text, ('dast'::character varying)::text, ('dependency_scanning'::character varying)::text, ('license_management'::character varying)::text, ('license_scanning'::character varying)::text, ('sast'::character varying)::text, ('coverage_fuzzing'::character varying)::text, ('secret_detection'::character varying)::text]))); @@ -36524,9 +36496,6 @@ ALTER TABLE ONLY boards_epic_user_preferences ALTER TABLE ONLY group_wiki_repositories ADD CONSTRAINT fk_rails_26f867598c FOREIGN KEY (group_id) REFERENCES namespaces(id) ON DELETE CASCADE; -ALTER TABLE ONLY pages_merge_request_review_apps - ADD CONSTRAINT fk_rails_26fd306173 FOREIGN KEY (merge_request_id) REFERENCES merge_requests(id) ON DELETE CASCADE; - ALTER TABLE ONLY lfs_file_locks ADD CONSTRAINT fk_rails_27a1d98fa8 FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE CASCADE; @@ -37841,9 +37810,6 @@ ALTER TABLE ONLY merge_request_blocks ALTER TABLE ONLY protected_branch_unprotect_access_levels ADD CONSTRAINT fk_rails_e9eb8dc025 FOREIGN KEY (protected_branch_id) REFERENCES protected_branches(id) ON DELETE CASCADE; -ALTER TABLE ONLY pages_merge_request_review_apps - ADD CONSTRAINT fk_rails_ea97949acd FOREIGN KEY (pages_deployment_id) REFERENCES pages_deployments(id) ON DELETE CASCADE; - ALTER TABLE ONLY alert_management_alert_user_mentions ADD CONSTRAINT fk_rails_eb2de0cdef FOREIGN KEY (note_id) REFERENCES notes(id) ON DELETE CASCADE; diff --git a/lib/gitlab/pages/virtual_host_finder.rb b/lib/gitlab/pages/virtual_host_finder.rb index b88caf27c62af5..9a1b387db3e1ce 100644 --- a/lib/gitlab/pages/virtual_host_finder.rb +++ b/lib/gitlab/pages/virtual_host_finder.rb @@ -28,7 +28,7 @@ def execute attr_reader :host def by_review_app_domain(name) - review_app = ::Pages::MergeRequestReviewApp.by_slug(name) + review_app = ::Pages::MergeRequestPreview.by_slug(name) return unless review_app -- GitLab From 53b21f4d5ac3bb4bb2efdb130b1d7b74aa3d6fd1 Mon Sep 17 00:00:00 2001 From: Kassio Borges Date: Thu, 8 Jun 2023 23:35:54 +0100 Subject: [PATCH 06/18] Follow migration guidelines --- ...00_create_pages_merge_request_previews.rb} | 9 ++++-- ...ign_key_to_pages_merge_request_previews.rb | 19 ++++++++++++ ...ign_key_to_pages_merge_request_previews.rb | 19 ++++++++++++ db/schema_migrations/20230602104800 | 1 - db/schema_migrations/20230610104800 | 1 + db/schema_migrations/20230610104801 | 1 + db/schema_migrations/20230610104802 | 1 + db/structure.sql | 31 +++++++++++++++++++ 8 files changed, 78 insertions(+), 4 deletions(-) rename db/migrate/{20230602104800_create_pages_merge_request_previews.rb => 20230610104800_create_pages_merge_request_previews.rb} (56%) create mode 100644 db/migrate/20230610104801_add_pages_deployment_foreign_key_to_pages_merge_request_previews.rb create mode 100644 db/migrate/20230610104802_add_merge_request_foreign_key_to_pages_merge_request_previews.rb delete mode 100644 db/schema_migrations/20230602104800 create mode 100644 db/schema_migrations/20230610104800 create mode 100644 db/schema_migrations/20230610104801 create mode 100644 db/schema_migrations/20230610104802 diff --git a/db/migrate/20230602104800_create_pages_merge_request_previews.rb b/db/migrate/20230610104800_create_pages_merge_request_previews.rb similarity index 56% rename from db/migrate/20230602104800_create_pages_merge_request_previews.rb rename to db/migrate/20230610104800_create_pages_merge_request_previews.rb index 791f9fa90db43d..6e03108b8cf403 100644 --- a/db/migrate/20230602104800_create_pages_merge_request_previews.rb +++ b/db/migrate/20230610104800_create_pages_merge_request_previews.rb @@ -5,9 +5,12 @@ def up create_table(:pages_merge_request_previews) do |t| t.datetime_with_timezone :created_at, null: false t.datetime_with_timezone :updated_at - t.references :pages_deployment, index: true, null: false, foreign_key: { on_delete: :cascade } - t.references :merge_request, index: { unique: true }, null: false, foreign_key: { on_delete: :cascade } - t.text :slug, null: false, limit: 255, unique: true + t.bigint :pages_deployment_id, null: false + t.bigint :merge_request_id, null: false + t.text :slug, null: false, limit: 63, unique: true + + t.index :pages_deployment_id + t.index :merge_request_id end end diff --git a/db/migrate/20230610104801_add_pages_deployment_foreign_key_to_pages_merge_request_previews.rb b/db/migrate/20230610104801_add_pages_deployment_foreign_key_to_pages_merge_request_previews.rb new file mode 100644 index 00000000000000..1142a4ef077afc --- /dev/null +++ b/db/migrate/20230610104801_add_pages_deployment_foreign_key_to_pages_merge_request_previews.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +class AddPagesDeploymentForeignKeyToPagesMergeRequestPreviews < Gitlab::Database::Migration[2.1] + disable_ddl_transaction! + + def up + add_concurrent_foreign_key :pages_merge_request_previews, + :pages_deployments, + column: :pages_deployment_id, + on_delete: :cascade + end + + def down + with_lock_retries do + remove_foreign_key :pages_merge_request_previews, + column: :pages_deployment_id + end + end +end diff --git a/db/migrate/20230610104802_add_merge_request_foreign_key_to_pages_merge_request_previews.rb b/db/migrate/20230610104802_add_merge_request_foreign_key_to_pages_merge_request_previews.rb new file mode 100644 index 00000000000000..f7bfcb37e895cc --- /dev/null +++ b/db/migrate/20230610104802_add_merge_request_foreign_key_to_pages_merge_request_previews.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +class AddMergeRequestForeignKeyToPagesMergeRequestPreviews < Gitlab::Database::Migration[2.1] + disable_ddl_transaction! + + def up + add_concurrent_foreign_key :pages_merge_request_previews, + :merge_requests, + column: :merge_request_id, + on_delete: :cascade + end + + def down + with_lock_retries do + remove_foreign_key :pages_merge_request_previews, + column: :merge_request_id + end + end +end diff --git a/db/schema_migrations/20230602104800 b/db/schema_migrations/20230602104800 deleted file mode 100644 index b4608020eed5e7..00000000000000 --- a/db/schema_migrations/20230602104800 +++ /dev/null @@ -1 +0,0 @@ -28b8dcf7b5c028642d03844a9c440c9389e7452a40994a0feda798fad624cdc5 \ No newline at end of file diff --git a/db/schema_migrations/20230610104800 b/db/schema_migrations/20230610104800 new file mode 100644 index 00000000000000..ccb865b59c86fa --- /dev/null +++ b/db/schema_migrations/20230610104800 @@ -0,0 +1 @@ +ea93f93c2006fbff8f8944f70eaf676e51e5d13eaa670fc72376cd3d6b7f0282 \ No newline at end of file diff --git a/db/schema_migrations/20230610104801 b/db/schema_migrations/20230610104801 new file mode 100644 index 00000000000000..5316ea72b7316a --- /dev/null +++ b/db/schema_migrations/20230610104801 @@ -0,0 +1 @@ +c9f46a0dbd965909327b249420b959e206a8c2e6327eff324593f190a7c3c963 \ No newline at end of file diff --git a/db/schema_migrations/20230610104802 b/db/schema_migrations/20230610104802 new file mode 100644 index 00000000000000..0dac0630199035 --- /dev/null +++ b/db/schema_migrations/20230610104802 @@ -0,0 +1 @@ +97b185b854b727aeca3390cf9925174ef4b232fa40c7000787c9d99701613080 \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 77b32db0edac9e..fb74ed0ead7e27 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -20022,6 +20022,25 @@ CREATE SEQUENCE pages_domains_id_seq ALTER SEQUENCE pages_domains_id_seq OWNED BY pages_domains.id; +CREATE TABLE pages_merge_request_previews ( + id bigint NOT NULL, + created_at timestamp with time zone NOT NULL, + updated_at timestamp with time zone, + pages_deployment_id bigint NOT NULL, + merge_request_id bigint NOT NULL, + slug text NOT NULL, + CONSTRAINT check_5ed1917d35 CHECK ((char_length(slug) <= 63)) +); + +CREATE SEQUENCE pages_merge_request_previews_id_seq + START WITH 1 + INCREMENT BY 1 + NO MINVALUE + NO MAXVALUE + CACHE 1; + +ALTER SEQUENCE pages_merge_request_previews_id_seq OWNED BY pages_merge_request_previews.id; + CREATE TABLE path_locks ( id integer NOT NULL, path character varying NOT NULL, @@ -25614,6 +25633,8 @@ ALTER TABLE ONLY pages_domain_acme_orders ALTER COLUMN id SET DEFAULT nextval('p ALTER TABLE ONLY pages_domains ALTER COLUMN id SET DEFAULT nextval('pages_domains_id_seq'::regclass); +ALTER TABLE ONLY pages_merge_request_previews ALTER COLUMN id SET DEFAULT nextval('pages_merge_request_previews_id_seq'::regclass); + ALTER TABLE ONLY path_locks ALTER COLUMN id SET DEFAULT nextval('path_locks_id_seq'::regclass); ALTER TABLE ONLY personal_access_tokens ALTER COLUMN id SET DEFAULT nextval('personal_access_tokens_id_seq'::regclass); @@ -27932,6 +27953,9 @@ ALTER TABLE ONLY pages_domain_acme_orders ALTER TABLE ONLY pages_domains ADD CONSTRAINT pages_domains_pkey PRIMARY KEY (id); +ALTER TABLE ONLY pages_merge_request_previews + ADD CONSTRAINT pages_merge_request_previews_pkey PRIMARY KEY (id); + ALTER TABLE ONLY path_locks ADD CONSTRAINT path_locks_pkey PRIMARY KEY (id); @@ -32212,6 +32236,10 @@ CREATE INDEX index_pages_domains_on_verified_at_and_enabled_until ON pages_domai CREATE INDEX index_pages_domains_on_wildcard ON pages_domains USING btree (wildcard); +CREATE INDEX index_pages_merge_request_previews_on_merge_request_id ON pages_merge_request_previews USING btree (merge_request_id); + +CREATE INDEX index_pages_merge_request_previews_on_pages_deployment_id ON pages_merge_request_previews USING btree (pages_deployment_id); + CREATE INDEX p_ci_builds_user_id_name_idx ON ONLY p_ci_builds USING btree (user_id, name) WHERE (((type)::text = 'Ci::Build'::text) AND ((name)::text = ANY (ARRAY[('container_scanning'::character varying)::text, ('dast'::character varying)::text, ('dependency_scanning'::character varying)::text, ('license_management'::character varying)::text, ('license_scanning'::character varying)::text, ('sast'::character varying)::text, ('coverage_fuzzing'::character varying)::text, ('secret_detection'::character varying)::text]))); CREATE INDEX index_partial_ci_builds_on_user_id_name_parser_features ON ci_builds USING btree (user_id, name) WHERE (((type)::text = 'Ci::Build'::text) AND ((name)::text = ANY (ARRAY[('container_scanning'::character varying)::text, ('dast'::character varying)::text, ('dependency_scanning'::character varying)::text, ('license_management'::character varying)::text, ('license_scanning'::character varying)::text, ('sast'::character varying)::text, ('coverage_fuzzing'::character varying)::text, ('secret_detection'::character varying)::text]))); @@ -35200,6 +35228,9 @@ ALTER TABLE ONLY issues ALTER TABLE ONLY merge_requests ADD CONSTRAINT fk_06067f5644 FOREIGN KEY (latest_merge_request_diff_id) REFERENCES merge_request_diffs(id) ON DELETE SET NULL; +ALTER TABLE ONLY pages_merge_request_previews + ADD CONSTRAINT fk_078398e5be FOREIGN KEY (merge_request_id) REFERENCES merge_requests(id) ON DELETE CASCADE; + ALTER TABLE ONLY user_interacted_projects ADD CONSTRAINT fk_0894651f08 FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE CASCADE; -- GitLab From 68e09fbeffc8d917c0722ed98ead674617cdc6d2 Mon Sep 17 00:00:00 2001 From: Kassio Borges Date: Fri, 9 Jun 2023 22:03:26 +0100 Subject: [PATCH 07/18] Improve data associations Also, improve strategy used to create/update Pages::MergeRequestPreview records. --- app/models/ci/build.rb | 2 +- app/models/merge_request.rb | 4 +- app/models/pages/merge_request_preview.rb | 29 ++++++++++++-- app/models/pages_deployment.rb | 6 +-- .../pages/create_merge_request_preview.rb | 39 ------------------- .../update_merge_request_preview_service.rb | 27 +++++++++++++ app/services/projects/update_pages_service.rb | 35 +++++++---------- .../pages/merge_request_review_worker.rb | 2 +- ...ign_key_to_pages_merge_request_previews.rb | 2 +- lib/gitlab/pages/virtual_host_finder.rb | 2 +- 10 files changed, 76 insertions(+), 72 deletions(-) delete mode 100644 app/services/pages/create_merge_request_preview.rb create mode 100644 app/services/pages/update_merge_request_preview_service.rb diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index fa4e79f680f005..d9bb41ccf55f9b 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -416,7 +416,7 @@ def pages_generator? def pages_preview_generator? Gitlab.config.pages.enabled && - name == 'pages:preview' + name == Pages::MergeRequestPreview::CI_JOB_NAME end def runnable? diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index dab69d8b662f3a..48f44eb1d96028 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -120,8 +120,8 @@ def merge_request_diff has_many :created_environments, class_name: 'Environment', foreign_key: :merge_request_id, inverse_of: :merge_request has_many :assignment_events, class_name: 'ResourceEvents::MergeRequestAssignmentEvent', inverse_of: :merge_request - has_many :pages_merge_request_review_apps, - class_name: 'Pages::MergeRequestPreviews', + has_one :pages_preview, + class_name: 'Pages::MergeRequestPreview', inverse_of: :merge_request KNOWN_MERGE_PARAMS = [ diff --git a/app/models/pages/merge_request_preview.rb b/app/models/pages/merge_request_preview.rb index 3fcb75329ed8d5..75dd971014a0bc 100644 --- a/app/models/pages/merge_request_preview.rb +++ b/app/models/pages/merge_request_preview.rb @@ -2,22 +2,43 @@ module Pages class MergeRequestPreview < ::ApplicationRecord + SLUG_PREFIX = 'preview' + CI_JOB_NAME = 'pages:preview' + self.table_name = 'pages_merge_request_previews' belongs_to :merge_request, class_name: 'MergeRequest', - inverse_of: :pages_merge_request_review_apps, + inverse_of: :pages_preview, optional: false - belongs_to :pages_deployment, + belongs_to :deployment, class_name: 'PagesDeployment', - inverse_of: :pages_merge_request_review_apps, + foreign_key: :pages_deployment_id, + inverse_of: :preview, optional: false - has_one :project, through: :pages_deployment + has_one :project, through: :deployment validates :slug, presence: true, uniqueness: true scope :by_slug, ->(value) { find_by(slug: value) } + + before_validation :generate_slug, unless: :slug + + private + + def generate_slug + return unless deployment + + path = "#{SLUG_PREFIX}-#{project.path.slice(0, 23)}" + sha = + Digest::SHA2 + .new(256) + .hexdigest("#{path}-#{merge_request_id}") + .slice(0, 62 - path.length) + + self.slug = Gitlab::Utils.slugify("#{path}-#{sha}") + end end end diff --git a/app/models/pages_deployment.rb b/app/models/pages_deployment.rb index b0360297269e9e..46db6b03ff7aa4 100644 --- a/app/models/pages_deployment.rb +++ b/app/models/pages_deployment.rb @@ -13,9 +13,9 @@ class PagesDeployment < ApplicationRecord belongs_to :project, optional: false belongs_to :ci_build, class_name: 'Ci::Build', optional: true - has_many :pages_merge_request_review_apps, - class_name: 'Pages::MergeRequestPreviews', - inverse_of: :pages_deployment + has_one :preview, + class_name: 'Pages::MergeRequestPreview', + inverse_of: :deployment scope :older_than, -> (id) { where('id < ?', id) } scope :migrated_from_legacy_storage, -> { where(file: MIGRATED_FILE_NAME) } diff --git a/app/services/pages/create_merge_request_preview.rb b/app/services/pages/create_merge_request_preview.rb deleted file mode 100644 index 4d787b1253ddd2..00000000000000 --- a/app/services/pages/create_merge_request_preview.rb +++ /dev/null @@ -1,39 +0,0 @@ -# frozen_string_literal: true - -module Pages - class CreateMergeRequestPreview - def initialize(pages_deployment:, merge_request_id:) - @pages_deployment = pages_deployment - @merge_request_id = merge_request_id - @project = pages_deployment.project - end - - def execute - Pages::MergeRequestPreview.find_or_initialize_by(merge_request_id: merge_request_id).tap do |preview| - if preview.persisted? - # TODO: Delete existing pages preview deploy for this MR - else - preview.slug = slug - end - - preview.pages_deployment_id = pages_deployment.id - preview.save! - end - end - - private - - attr_reader :project, :pages_deployment, :merge_request_id - - def slug - path = "preview-#{project.path.slice(0, 23)}" - sha = - Digest::SHA2 - .new(256) - .hexdigest("#{path}-#{merge_request_id}") - .slice(0, 62 - path.length) - - Gitlab::Utils.slugify("#{path}-#{sha}") - end - end -end diff --git a/app/services/pages/update_merge_request_preview_service.rb b/app/services/pages/update_merge_request_preview_service.rb new file mode 100644 index 00000000000000..18587b3ad78f99 --- /dev/null +++ b/app/services/pages/update_merge_request_preview_service.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +module Pages + class UpdateMergeRequestPreviewService + def initialize(pages_deployment:, merge_request:) + @pages_deployment = pages_deployment + @merge_request = merge_request + end + + def execute + if merge_request_preview.deployment.present? + # TODO: enqueue to delete last merge request pages preview deploy + end + + merge_request_preview.deployment = pages_deployment + merge_request_preview.save! + end + + private + + attr_reader :pages_deployment, :merge_request + + def merge_request_preview + merge_request.pages_preview || merge_request.build_pages_preview + end + end +end diff --git a/app/services/projects/update_pages_service.rb b/app/services/projects/update_pages_service.rb index 7f8d45869a3c73..d4ddb0c5d90d63 100644 --- a/app/services/projects/update_pages_service.rb +++ b/app/services/projects/update_pages_service.rb @@ -2,6 +2,8 @@ module Projects class UpdatePagesService < BaseService + include Gitlab::Utils::StrongMemoize + # old deployment can be cached by pages daemon # so we need to give pages daemon some time update cache # 10 minutes is enough, but 30 feels safer @@ -40,12 +42,9 @@ def execute # # On the final version, the creation of the review app deploy will be a # manual process by the user, which we can probably extract from here. - if merge_request_preview - create_merge_request_preview(deployment) - else - update_project_pages_deployment(deployment) - end + break update_merge_request_preview(deployment) if @merge_request_preview + update_project_pages_deployment(deployment) success end rescue StandardError => e @@ -55,8 +54,6 @@ def execute private - attr_reader :merge_request_preview - def success @commit_status.success @project.mark_pages_as_deployed @@ -120,14 +117,6 @@ def update_project_pages_deployment(deployment) ) end - def ref - build.ref - end - - def artifacts - build.artifacts_file.path - end - def register_attempt pages_deployments_total_counter.increment end @@ -137,12 +126,14 @@ def register_failure end def pages_deployments_total_counter - @pages_deployments_total_counter ||= Gitlab::Metrics.counter(:pages_deployments_total, "Counter of GitLab Pages deployments triggered") + Gitlab::Metrics.counter(:pages_deployments_total, "Counter of GitLab Pages deployments triggered") end + strong_memoize_attr :pages_deployments_total_counter def pages_deployments_failed_total_counter - @pages_deployments_failed_total_counter ||= Gitlab::Metrics.counter(:pages_deployments_failed_total, "Counter of GitLab Pages deployments which failed") + Gitlab::Metrics.counter(:pages_deployments_failed_total, "Counter of GitLab Pages deployments which failed") end + strong_memoize_attr :pages_deployments_failed_total_counter def publish_deployed_event event = ::Pages::PageDeployedEvent.new(data: { @@ -154,11 +145,15 @@ def publish_deployed_event Gitlab::EventStore.publish(event) end - def create_merge_request_preview(deployment) - Pages::CreateMergeRequestPreview.new( + def update_merge_request_preview(deployment) + @commit_status.success + + return if build.pipeline.all_merge_requests.order(iid: :asc).blank? # rubocop: disable CodeReuse/ActiveRecord + + ::Pages::UpdateMergeRequestPreviewService.new( pages_deployment: deployment, # TODO: for some reason, build.merge_request sometimes is nil - merge_request_id: build.pipeline.all_merge_requests.order(iid: :asc).first.id + merge_request: build.pipeline.all_merge_requests.order(iid: :asc).first # rubocop: disable CodeReuse/ActiveRecord ).execute end end diff --git a/app/workers/pages/merge_request_review_worker.rb b/app/workers/pages/merge_request_review_worker.rb index 79dec4b5680cc9..3976a1f296322b 100644 --- a/app/workers/pages/merge_request_review_worker.rb +++ b/app/workers/pages/merge_request_review_worker.rb @@ -14,7 +14,7 @@ def perform(build_id) build = Ci::Build.find_by_id(build_id) # TODO: for some reason, build.merge_request sometimes is nil - return if build.pipeline.all_merge_requests.order(iid: :asc).first.blank? + return if build.pipeline.all_merge_requests.order(iid: :asc).blank? Projects::UpdatePagesService.new( build.project, diff --git a/db/migrate/20230610104801_add_pages_deployment_foreign_key_to_pages_merge_request_previews.rb b/db/migrate/20230610104801_add_pages_deployment_foreign_key_to_pages_merge_request_previews.rb index 1142a4ef077afc..3fb66151796dbd 100644 --- a/db/migrate/20230610104801_add_pages_deployment_foreign_key_to_pages_merge_request_previews.rb +++ b/db/migrate/20230610104801_add_pages_deployment_foreign_key_to_pages_merge_request_previews.rb @@ -7,7 +7,7 @@ def up add_concurrent_foreign_key :pages_merge_request_previews, :pages_deployments, column: :pages_deployment_id, - on_delete: :cascade + on_delete: :nullify end def down diff --git a/lib/gitlab/pages/virtual_host_finder.rb b/lib/gitlab/pages/virtual_host_finder.rb index 9a1b387db3e1ce..4f84e697d2849b 100644 --- a/lib/gitlab/pages/virtual_host_finder.rb +++ b/lib/gitlab/pages/virtual_host_finder.rb @@ -36,7 +36,7 @@ def by_review_app_domain(name) projects: [review_app.project], lookup_paths: [::Pages::LookupPath.new( review_app.project, - deployment: review_app.pages_deployment + deployment: review_app.deployment )] ) end -- GitLab From 6f4459450c65046614077fa37d859cd4fb6241f3 Mon Sep 17 00:00:00 2001 From: Kassio Borges Date: Fri, 9 Jun 2023 23:17:32 +0100 Subject: [PATCH 08/18] Use a real random value in the preview slug --- app/models/pages/merge_request_preview.rb | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/app/models/pages/merge_request_preview.rb b/app/models/pages/merge_request_preview.rb index 75dd971014a0bc..8a241aa165854c 100644 --- a/app/models/pages/merge_request_preview.rb +++ b/app/models/pages/merge_request_preview.rb @@ -20,25 +20,21 @@ class MergeRequestPreview < ::ApplicationRecord has_one :project, through: :deployment + before_validation :generate_slug, unless: :slug + + validates :merge_request_id, uniqueness: true validates :slug, presence: true, uniqueness: true scope :by_slug, ->(value) { find_by(slug: value) } - before_validation :generate_slug, unless: :slug - private def generate_slug - return unless deployment - - path = "#{SLUG_PREFIX}-#{project.path.slice(0, 23)}" - sha = - Digest::SHA2 - .new(256) - .hexdigest("#{path}-#{merge_request_id}") - .slice(0, 62 - path.length) + return if deployment.blank? - self.slug = Gitlab::Utils.slugify("#{path}-#{sha}") + self.slug = Gitlab::Utils.slugify( + "#{SLUG_PREFIX}-#{project.path.slice(0, 23)}-#{SecureRandom.hex(30)}" + ) end end end -- GitLab From 1ea1ce0d7865627bfa9a51cffd579bdf10f5b2cb Mon Sep 17 00:00:00 2001 From: Kassio Borges Date: Fri, 9 Jun 2023 23:33:19 +0100 Subject: [PATCH 09/18] Delete _old_ deploys when MR is updated --- app/models/pages/merge_request_preview.rb | 2 +- app/models/pages_deployment.rb | 8 +++++++- app/models/project.rb | 18 +++++++++++------- .../update_merge_request_preview_service.rb | 5 ++++- app/services/projects/update_pages_service.rb | 7 +------ .../delete_merge_request_review_worker.rb | 17 +++++++++++++++++ lib/gitlab/pages/virtual_host_finder.rb | 2 +- 7 files changed, 42 insertions(+), 17 deletions(-) create mode 100644 app/workers/pages/delete_merge_request_review_worker.rb diff --git a/app/models/pages/merge_request_preview.rb b/app/models/pages/merge_request_preview.rb index 8a241aa165854c..f51b939a3a3665 100644 --- a/app/models/pages/merge_request_preview.rb +++ b/app/models/pages/merge_request_preview.rb @@ -25,7 +25,7 @@ class MergeRequestPreview < ::ApplicationRecord validates :merge_request_id, uniqueness: true validates :slug, presence: true, uniqueness: true - scope :by_slug, ->(value) { find_by(slug: value) } + scope :find_by_slug, ->(value) { find_by(slug: value) } private diff --git a/app/models/pages_deployment.rb b/app/models/pages_deployment.rb index 46db6b03ff7aa4..fe542e4f7cf695 100644 --- a/app/models/pages_deployment.rb +++ b/app/models/pages_deployment.rb @@ -8,6 +8,11 @@ class PagesDeployment < ApplicationRecord MIGRATED_FILE_NAME = "_migrated.zip" + # old deployment can be cached by pages daemon + # so we need to give pages daemon some time update cache + # 10 minutes is enough, but 30 feels safer + OLD_DEPLOYMENTS_DESTRUCTION_DELAY = 30.minutes.freeze + attribute :file_store, :integer, default: -> { ::Pages::DeploymentUploader.default_store } belongs_to :project, optional: false @@ -17,7 +22,8 @@ class PagesDeployment < ApplicationRecord class_name: 'Pages::MergeRequestPreview', inverse_of: :deployment - scope :older_than, -> (id) { where('id < ?', id) } + scope :find_by_id, ->(id) { find_by(id: id) } + scope :older_than, ->(id) { where('id < ?', id) } scope :migrated_from_legacy_storage, -> { where(file: MIGRATED_FILE_NAME) } scope :with_files_stored_locally, -> { where(file_store: ::ObjectStorage::Store::LOCAL) } scope :with_files_stored_remotely, -> { where(file_store: ::ObjectStorage::Store::REMOTE) } diff --git a/app/models/project.rb b/app/models/project.rb index 9cf2e83abec13e..452a5c8973c395 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -2862,6 +2862,10 @@ def access_request_approvers_to_be_notified recipients end + def pages_lookup_path(trim_prefix: nil, domain: nil) + Pages::LookupPath.new(self, trim_prefix: trim_prefix, domain: domain) + end + def closest_setting(name) setting = read_attribute(name) setting = closest_namespace_setting(name) if setting.nil? @@ -3237,13 +3241,6 @@ def frozen_outbound_job_token_scopes? end strong_memoize_attr :frozen_outbound_job_token_scopes? - def pages_url_for(domain) - # The host in URL always needs to be downcased - Gitlab.config.pages.url.sub(%r{^https?://}) do |prefix| - "#{prefix}#{domain}." - end.downcase - end - private def pages_unique_domain_enabled? @@ -3251,6 +3248,13 @@ def pages_unique_domain_enabled? project_setting.pages_unique_domain_enabled? end + def pages_url_for(domain) + # The host in URL always needs to be downcased + Gitlab.config.pages.url.sub(%r{^https?://}) do |prefix| + "#{prefix}#{domain}." + end.downcase + end + # overridden in EE def project_group_links_with_preload project_group_links diff --git a/app/services/pages/update_merge_request_preview_service.rb b/app/services/pages/update_merge_request_preview_service.rb index 18587b3ad78f99..f91b395135b17c 100644 --- a/app/services/pages/update_merge_request_preview_service.rb +++ b/app/services/pages/update_merge_request_preview_service.rb @@ -9,7 +9,10 @@ def initialize(pages_deployment:, merge_request:) def execute if merge_request_preview.deployment.present? - # TODO: enqueue to delete last merge request pages preview deploy + Pages::DeleteMergeRequestReviewWorker.perform_in( + PagesDeployment::OLD_DEPLOYMENTS_DESTRUCTION_DELAY, + merge_request_preview.deployment_id + ) end merge_request_preview.deployment = pages_deployment diff --git a/app/services/projects/update_pages_service.rb b/app/services/projects/update_pages_service.rb index d4ddb0c5d90d63..7e6ffdfb582377 100644 --- a/app/services/projects/update_pages_service.rb +++ b/app/services/projects/update_pages_service.rb @@ -4,11 +4,6 @@ module Projects class UpdatePagesService < BaseService include Gitlab::Utils::StrongMemoize - # old deployment can be cached by pages daemon - # so we need to give pages daemon some time update cache - # 10 minutes is enough, but 30 feels safer - OLD_DEPLOYMENTS_DESTRUCTION_DELAY = 30.minutes.freeze - attr_reader :build, :deployment_update def initialize(project, build, merge_request_preview: false) @@ -111,7 +106,7 @@ def create_pages_deployment(artifacts_path, build) def update_project_pages_deployment(deployment) project.update_pages_deployment!(deployment) DestroyPagesDeploymentsWorker.perform_in( - OLD_DEPLOYMENTS_DESTRUCTION_DELAY, + PagesDeployment::OLD_DEPLOYMENTS_DESTRUCTION_DELAY, project.id, deployment.id ) diff --git a/app/workers/pages/delete_merge_request_review_worker.rb b/app/workers/pages/delete_merge_request_review_worker.rb new file mode 100644 index 00000000000000..f7375297aedc31 --- /dev/null +++ b/app/workers/pages/delete_merge_request_review_worker.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +module Pages + class DeleteMergeRequestReviewWorker + include ApplicationWorker + + data_consistency :always + + sidekiq_options retry: 3 + feature_category :pages + worker_resource_boundary :cpu + + def perform(deployment_id) + PagesDeployment.find_by_id(deployment_id)&.delete! + end + end +end diff --git a/lib/gitlab/pages/virtual_host_finder.rb b/lib/gitlab/pages/virtual_host_finder.rb index 4f84e697d2849b..f53f3dd2d8ea16 100644 --- a/lib/gitlab/pages/virtual_host_finder.rb +++ b/lib/gitlab/pages/virtual_host_finder.rb @@ -28,7 +28,7 @@ def execute attr_reader :host def by_review_app_domain(name) - review_app = ::Pages::MergeRequestPreview.by_slug(name) + review_app = ::Pages::MergeRequestPreview.find_by_slug(name) return unless review_app -- GitLab From 5066703520f77aab37ed339cbdc03e8050e5411a Mon Sep 17 00:00:00 2001 From: Kassio Borges Date: Mon, 12 Jun 2023 16:36:11 +0100 Subject: [PATCH 10/18] Add the preview deletion flow --- app/events/merge_requests/closed_event.rb | 19 +++++++++++++++++++ app/events/merge_requests/merged_event.rb | 19 +++++++++++++++++++ app/models/ci/build.rb | 2 +- app/models/pages/merge_request_preview.rb | 1 + app/models/pages_deployment.rb | 4 ++++ app/services/merge_requests/close_service.rb | 10 ++++++++++ app/services/merge_requests/merge_service.rb | 14 ++++++++++++++ .../update_merge_request_preview_service.rb | 4 ++-- app/services/projects/update_pages_service.rb | 2 +- .../clear_merge_request_previews_worker.rb | 19 +++++++++++++++++++ ...=> create_merge_request_preview_worker.rb} | 2 +- ...=> delete_merge_request_preview_worker.rb} | 4 ++-- lib/gitlab/event_store.rb | 2 ++ lib/gitlab/pages/virtual_host_finder.rb | 2 +- 14 files changed, 96 insertions(+), 8 deletions(-) create mode 100644 app/events/merge_requests/closed_event.rb create mode 100644 app/events/merge_requests/merged_event.rb create mode 100644 app/workers/pages/clear_merge_request_previews_worker.rb rename app/workers/pages/{merge_request_review_worker.rb => create_merge_request_preview_worker.rb} (93%) rename app/workers/pages/{delete_merge_request_review_worker.rb => delete_merge_request_preview_worker.rb} (71%) diff --git a/app/events/merge_requests/closed_event.rb b/app/events/merge_requests/closed_event.rb new file mode 100644 index 00000000000000..dff184d23056a5 --- /dev/null +++ b/app/events/merge_requests/closed_event.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +module MergeRequests + class ClosedEvent < Gitlab::EventStore::Event + def schema + { + 'type' => 'object', + 'required' => %w[ + current_user_id + merge_request_id + ], + 'properties' => { + 'current_user_id' => { 'type' => 'integer' }, + 'merge_request_id' => { 'type' => 'integer' } + } + } + end + end +end diff --git a/app/events/merge_requests/merged_event.rb b/app/events/merge_requests/merged_event.rb new file mode 100644 index 00000000000000..8edc1a1c005499 --- /dev/null +++ b/app/events/merge_requests/merged_event.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +module MergeRequests + class MergedEvent < Gitlab::EventStore::Event + def schema + { + 'type' => 'object', + 'required' => %w[ + current_user_id + merge_request_id + ], + 'properties' => { + 'current_user_id' => { 'type' => 'integer' }, + 'merge_request_id' => { 'type' => 'integer' } + } + } + end + end +end diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index d9bb41ccf55f9b..559658d6bfc036 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -329,7 +329,7 @@ def clone_accessors build.run_after_commit do BuildSuccessWorker.perform_async(id) PagesWorker.perform_async(:deploy, id) if build.pages_generator? - Pages::MergeRequestReviewWorker.perform_async(id) if build.pages_preview_generator? + Pages::CreateMergeRequestPreviewWorker.perform_async(id) if build.pages_preview_generator? end end diff --git a/app/models/pages/merge_request_preview.rb b/app/models/pages/merge_request_preview.rb index f51b939a3a3665..4f5cf08884325f 100644 --- a/app/models/pages/merge_request_preview.rb +++ b/app/models/pages/merge_request_preview.rb @@ -25,6 +25,7 @@ class MergeRequestPreview < ::ApplicationRecord validates :merge_request_id, uniqueness: true validates :slug, presence: true, uniqueness: true + scope :with_deployment, -> { where.not(pages_deployment_id: nil) } scope :find_by_slug, ->(value) { find_by(slug: value) } private diff --git a/app/models/pages_deployment.rb b/app/models/pages_deployment.rb index fe542e4f7cf695..2327e0ace09b22 100644 --- a/app/models/pages_deployment.rb +++ b/app/models/pages_deployment.rb @@ -28,6 +28,10 @@ class PagesDeployment < ApplicationRecord scope :with_files_stored_locally, -> { where(file_store: ::ObjectStorage::Store::LOCAL) } scope :with_files_stored_remotely, -> { where(file_store: ::ObjectStorage::Store::REMOTE) } scope :project_id_in, ->(ids) { where(project_id: ids) } + scope :for_merge_request_preview, ->(merge_request_id) do + joins(:preview) + .where(pages_merge_request_previews: { merge_request_id: merge_request_id }) + end validates :file, presence: true validates :file_store, presence: true, inclusion: { in: ObjectStorage::SUPPORTED_STORES } diff --git a/app/services/merge_requests/close_service.rb b/app/services/merge_requests/close_service.rb index 62928e05a8995b..64d3ece5ca2eb6 100644 --- a/app/services/merge_requests/close_service.rb +++ b/app/services/merge_requests/close_service.rb @@ -25,6 +25,7 @@ def execute(merge_request, commit = nil) abort_auto_merge(merge_request, 'merge request was closed') cleanup_refs(merge_request) trigger_merge_request_merge_status_updated(merge_request) + publish_event end merge_request @@ -48,6 +49,15 @@ def expire_unapproved_key(merge_request) def trigger_merge_request_merge_status_updated(merge_request) GraphqlTriggers.merge_request_merge_status_updated(merge_request) end + + def publish_event + event = MergeRequests::ClosedEvent.new(data: { + current_user_id: current_user.id, + merge_request_id: merge_request.id + }) + + Gitlab::EventStore.publish(event) + end end end diff --git a/app/services/merge_requests/merge_service.rb b/app/services/merge_requests/merge_service.rb index 5e41375e7a09eb..99f4fd2e8dd48b 100644 --- a/app/services/merge_requests/merge_service.rb +++ b/app/services/merge_requests/merge_service.rb @@ -45,6 +45,11 @@ def execute(merge_request, options = {}) exclusive_lease(merge_request.id).cancel end + def success(...) + publish_event + super + end + private def validate! @@ -203,5 +208,14 @@ def exclusive_lease(merge_request_id) Gitlab::ExclusiveLease.new(lease_key, timeout: LEASE_TIMEOUT) end end + + def publish_event + event = MergeRequests::MergedEvent.new(data: { + current_user_id: current_user.id, + merge_request_id: merge_request.id + }) + + Gitlab::EventStore.publish(event) + end end end diff --git a/app/services/pages/update_merge_request_preview_service.rb b/app/services/pages/update_merge_request_preview_service.rb index f91b395135b17c..56bcfac781cdd3 100644 --- a/app/services/pages/update_merge_request_preview_service.rb +++ b/app/services/pages/update_merge_request_preview_service.rb @@ -9,8 +9,8 @@ def initialize(pages_deployment:, merge_request:) def execute if merge_request_preview.deployment.present? - Pages::DeleteMergeRequestReviewWorker.perform_in( - PagesDeployment::OLD_DEPLOYMENTS_DESTRUCTION_DELAY, + ::Pages::DeleteMergeRequestPreviewWorker.perform_in( + ::PagesDeployment::OLD_DEPLOYMENTS_DESTRUCTION_DELAY, merge_request_preview.deployment_id ) end diff --git a/app/services/projects/update_pages_service.rb b/app/services/projects/update_pages_service.rb index 7e6ffdfb582377..8c73f45730abf5 100644 --- a/app/services/projects/update_pages_service.rb +++ b/app/services/projects/update_pages_service.rb @@ -106,7 +106,7 @@ def create_pages_deployment(artifacts_path, build) def update_project_pages_deployment(deployment) project.update_pages_deployment!(deployment) DestroyPagesDeploymentsWorker.perform_in( - PagesDeployment::OLD_DEPLOYMENTS_DESTRUCTION_DELAY, + ::PagesDeployment::OLD_DEPLOYMENTS_DESTRUCTION_DELAY, project.id, deployment.id ) diff --git a/app/workers/pages/clear_merge_request_previews_worker.rb b/app/workers/pages/clear_merge_request_previews_worker.rb new file mode 100644 index 00000000000000..a240eca7dc35bd --- /dev/null +++ b/app/workers/pages/clear_merge_request_previews_worker.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +module Pages + class ClearMergeRequestPreviewsWorker + include ApplicationWorker + + data_consistency :always + + sidekiq_options retry: 3 + feature_category :pages + worker_resource_boundary :cpu + + def perform(event) + PagesDeployment + .for_merge_request_preview(event['merge_request_id']) + .delete_all! + end + end +end diff --git a/app/workers/pages/merge_request_review_worker.rb b/app/workers/pages/create_merge_request_preview_worker.rb similarity index 93% rename from app/workers/pages/merge_request_review_worker.rb rename to app/workers/pages/create_merge_request_preview_worker.rb index 3976a1f296322b..b7cbbe09b668ca 100644 --- a/app/workers/pages/merge_request_review_worker.rb +++ b/app/workers/pages/create_merge_request_preview_worker.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module Pages - class MergeRequestReviewWorker + class CreateMergeRequestPreviewWorker include ApplicationWorker data_consistency :always diff --git a/app/workers/pages/delete_merge_request_review_worker.rb b/app/workers/pages/delete_merge_request_preview_worker.rb similarity index 71% rename from app/workers/pages/delete_merge_request_review_worker.rb rename to app/workers/pages/delete_merge_request_preview_worker.rb index f7375297aedc31..7491181677ca75 100644 --- a/app/workers/pages/delete_merge_request_review_worker.rb +++ b/app/workers/pages/delete_merge_request_preview_worker.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module Pages - class DeleteMergeRequestReviewWorker + class DeleteMergeRequestPreviewWorker include ApplicationWorker data_consistency :always @@ -11,7 +11,7 @@ class DeleteMergeRequestReviewWorker worker_resource_boundary :cpu def perform(deployment_id) - PagesDeployment.find_by_id(deployment_id)&.delete! + ::PagesDeployment.find_by_id(deployment_id)&.delete! end end end diff --git a/lib/gitlab/event_store.rb b/lib/gitlab/event_store.rb index ce71ee594f24b9..2db00cacf6f37b 100644 --- a/lib/gitlab/event_store.rb +++ b/lib/gitlab/event_store.rb @@ -56,6 +56,8 @@ def self.configure!(store) store.subscribe ::Pages::InvalidateDomainCacheWorker, to: ::PagesDomains::PagesDomainUpdatedEvent store.subscribe ::Pages::InvalidateDomainCacheWorker, to: ::PagesDomains::PagesDomainCreatedEvent + store.subscribe ::Pages::DeleteMergeRequestPreview, to: ::MergeRequests::ApprovedEvent + store.subscribe ::MergeRequests::CreateApprovalEventWorker, to: ::MergeRequests::ApprovedEvent store.subscribe ::MergeRequests::CreateApprovalNoteWorker, to: ::MergeRequests::ApprovedEvent store.subscribe ::MergeRequests::ResolveTodosAfterApprovalWorker, to: ::MergeRequests::ApprovedEvent diff --git a/lib/gitlab/pages/virtual_host_finder.rb b/lib/gitlab/pages/virtual_host_finder.rb index f53f3dd2d8ea16..bc7559c972ebee 100644 --- a/lib/gitlab/pages/virtual_host_finder.rb +++ b/lib/gitlab/pages/virtual_host_finder.rb @@ -28,7 +28,7 @@ def execute attr_reader :host def by_review_app_domain(name) - review_app = ::Pages::MergeRequestPreview.find_by_slug(name) + review_app = ::Pages::MergeRequestPreview.with_deployment.find_by_slug(name) return unless review_app -- GitLab From 0e04cf4d817fc4b0d7bfee5f8af6b2f4a9a14f36 Mon Sep 17 00:00:00 2001 From: Kassio Borges Date: Mon, 12 Jun 2023 21:25:03 +0100 Subject: [PATCH 11/18] Add events to delete _old_ preview deployments --- .../merge_request_preview_outdated_event.rb | 17 +++++++++++++++++ app/models/ci/build.rb | 4 ++-- app/services/merge_requests/close_service.rb | 4 ++-- app/services/merge_requests/merge_service.rb | 9 ++------- .../update_merge_request_preview_service.rb | 17 +++++++++++------ app/services/projects/update_pages_service.rb | 3 ++- .../create_merge_request_preview_worker.rb | 7 +++++-- .../delete_merge_request_preview_worker.rb | 18 ++++++++++++------ lib/gitlab/event_store.rb | 4 +++- 9 files changed, 56 insertions(+), 27 deletions(-) create mode 100644 app/events/pages/merge_request_preview_outdated_event.rb diff --git a/app/events/pages/merge_request_preview_outdated_event.rb b/app/events/pages/merge_request_preview_outdated_event.rb new file mode 100644 index 00000000000000..16d13a6d3774a7 --- /dev/null +++ b/app/events/pages/merge_request_preview_outdated_event.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +module Pages + class MergeRequestPreviewOutdatedEvent < Gitlab::EventStore::Event + def schema + { + 'type' => 'object', + 'required' => %w[ + pages_deployment_id + ], + 'properties' => { + 'pages_deployment_id' => { 'type' => 'integer' } + } + } + end + end +end diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 559658d6bfc036..235bd1f6c5d41b 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -328,8 +328,8 @@ def clone_accessors after_transition any => [:success] do |build| build.run_after_commit do BuildSuccessWorker.perform_async(id) - PagesWorker.perform_async(:deploy, id) if build.pages_generator? - Pages::CreateMergeRequestPreviewWorker.perform_async(id) if build.pages_preview_generator? + ::PagesWorker.perform_async(:deploy, id) if build.pages_generator? + ::Pages::CreateMergeRequestPreviewWorker.perform_async(id) if build.pages_preview_generator? end end diff --git a/app/services/merge_requests/close_service.rb b/app/services/merge_requests/close_service.rb index 64d3ece5ca2eb6..0ca90d19b0abfe 100644 --- a/app/services/merge_requests/close_service.rb +++ b/app/services/merge_requests/close_service.rb @@ -25,7 +25,7 @@ def execute(merge_request, commit = nil) abort_auto_merge(merge_request, 'merge request was closed') cleanup_refs(merge_request) trigger_merge_request_merge_status_updated(merge_request) - publish_event + publish_event(merge_request) end merge_request @@ -50,7 +50,7 @@ def trigger_merge_request_merge_status_updated(merge_request) GraphqlTriggers.merge_request_merge_status_updated(merge_request) end - def publish_event + def publish_event(merge_request) event = MergeRequests::ClosedEvent.new(data: { current_user_id: current_user.id, merge_request_id: merge_request.id diff --git a/app/services/merge_requests/merge_service.rb b/app/services/merge_requests/merge_service.rb index 99f4fd2e8dd48b..e18270167f78a8 100644 --- a/app/services/merge_requests/merge_service.rb +++ b/app/services/merge_requests/merge_service.rb @@ -34,7 +34,7 @@ def execute(merge_request, options = {}) if commit after_merge clean_merge_jid - success + publish_event(merge_request) end end @@ -45,11 +45,6 @@ def execute(merge_request, options = {}) exclusive_lease(merge_request.id).cancel end - def success(...) - publish_event - super - end - private def validate! @@ -209,7 +204,7 @@ def exclusive_lease(merge_request_id) end end - def publish_event + def publish_event(merge_request) event = MergeRequests::MergedEvent.new(data: { current_user_id: current_user.id, merge_request_id: merge_request.id diff --git a/app/services/pages/update_merge_request_preview_service.rb b/app/services/pages/update_merge_request_preview_service.rb index 56bcfac781cdd3..663d581c6624b0 100644 --- a/app/services/pages/update_merge_request_preview_service.rb +++ b/app/services/pages/update_merge_request_preview_service.rb @@ -8,12 +8,7 @@ def initialize(pages_deployment:, merge_request:) end def execute - if merge_request_preview.deployment.present? - ::Pages::DeleteMergeRequestPreviewWorker.perform_in( - ::PagesDeployment::OLD_DEPLOYMENTS_DESTRUCTION_DELAY, - merge_request_preview.deployment_id - ) - end + publish_outdated_preview_event(merge_request_preview.pages_deployment_id) merge_request_preview.deployment = pages_deployment merge_request_preview.save! @@ -26,5 +21,15 @@ def execute def merge_request_preview merge_request.pages_preview || merge_request.build_pages_preview end + + def publish_outdated_preview_event(deployment_id) + return unless deployment_id.present? + + event = ::Pages::MergeRequestPreviewOutdatedEvent.new(data: { + pages_deployment_id: deployment_id + }) + + Gitlab::EventStore.publish(event) + end end end diff --git a/app/services/projects/update_pages_service.rb b/app/services/projects/update_pages_service.rb index 8c73f45730abf5..5be3af8910fb43 100644 --- a/app/services/projects/update_pages_service.rb +++ b/app/services/projects/update_pages_service.rb @@ -105,7 +105,7 @@ def create_pages_deployment(artifacts_path, build) def update_project_pages_deployment(deployment) project.update_pages_deployment!(deployment) - DestroyPagesDeploymentsWorker.perform_in( + ::DestroyPagesDeploymentsWorker.perform_in( ::PagesDeployment::OLD_DEPLOYMENTS_DESTRUCTION_DELAY, project.id, deployment.id @@ -141,6 +141,7 @@ def publish_deployed_event end def update_merge_request_preview(deployment) + # Mark pages:deploy as successful @commit_status.success return if build.pipeline.all_merge_requests.order(iid: :asc).blank? # rubocop: disable CodeReuse/ActiveRecord diff --git a/app/workers/pages/create_merge_request_preview_worker.rb b/app/workers/pages/create_merge_request_preview_worker.rb index b7cbbe09b668ca..c3d41ec4678292 100644 --- a/app/workers/pages/create_merge_request_preview_worker.rb +++ b/app/workers/pages/create_merge_request_preview_worker.rb @@ -4,8 +4,11 @@ module Pages class CreateMergeRequestPreviewWorker include ApplicationWorker - data_consistency :always + idempotent! + data_consistency :always # rubocop: disable SidekiqLoadBalancing/WorkerDataConsistency + + loggable_arguments 0 sidekiq_options retry: 3 feature_category :pages worker_resource_boundary :cpu @@ -14,7 +17,7 @@ def perform(build_id) build = Ci::Build.find_by_id(build_id) # TODO: for some reason, build.merge_request sometimes is nil - return if build.pipeline.all_merge_requests.order(iid: :asc).blank? + return if build.pipeline.all_merge_requests.order(iid: :asc).blank? # rubocop: disable CodeReuse/ActiveRecord Projects::UpdatePagesService.new( build.project, diff --git a/app/workers/pages/delete_merge_request_preview_worker.rb b/app/workers/pages/delete_merge_request_preview_worker.rb index 7491181677ca75..703c011e9c4d9e 100644 --- a/app/workers/pages/delete_merge_request_preview_worker.rb +++ b/app/workers/pages/delete_merge_request_preview_worker.rb @@ -2,16 +2,22 @@ module Pages class DeleteMergeRequestPreviewWorker - include ApplicationWorker + include Gitlab::EventStore::Subscriber - data_consistency :always + idempotent! - sidekiq_options retry: 3 feature_category :pages - worker_resource_boundary :cpu - def perform(deployment_id) - ::PagesDeployment.find_by_id(deployment_id)&.delete! + def handle_event(event) + if event.data[:pages_deployment_id].present? + ::PagesDeployment + .find_by_id(event.data[:pages_deployment_id]) + .delete + elsif event.data[:merge_request_id].present? + ::PagesDeployment + .for_merge_request_preview(event.data[:merge_request_id]) + .delete_all + end end end end diff --git a/lib/gitlab/event_store.rb b/lib/gitlab/event_store.rb index 2db00cacf6f37b..db2c5a9fa84e65 100644 --- a/lib/gitlab/event_store.rb +++ b/lib/gitlab/event_store.rb @@ -56,7 +56,9 @@ def self.configure!(store) store.subscribe ::Pages::InvalidateDomainCacheWorker, to: ::PagesDomains::PagesDomainUpdatedEvent store.subscribe ::Pages::InvalidateDomainCacheWorker, to: ::PagesDomains::PagesDomainCreatedEvent - store.subscribe ::Pages::DeleteMergeRequestPreview, to: ::MergeRequests::ApprovedEvent + store.subscribe ::Pages::DeleteMergeRequestPreviewWorker, to: ::Pages::MergeRequestPreviewOutdatedEvent + store.subscribe ::Pages::DeleteMergeRequestPreviewWorker, to: ::MergeRequests::ApprovedEvent + store.subscribe ::Pages::DeleteMergeRequestPreviewWorker, to: ::MergeRequests::ClosedEvent store.subscribe ::MergeRequests::CreateApprovalEventWorker, to: ::MergeRequests::ApprovedEvent store.subscribe ::MergeRequests::CreateApprovalNoteWorker, to: ::MergeRequests::ApprovedEvent -- GitLab From 29125193ba3d2414250d66b43ec5f98e5eb3937f Mon Sep 17 00:00:00 2001 From: Kassio Borges Date: Mon, 12 Jun 2023 23:27:24 +0100 Subject: [PATCH 12/18] When deploying a new version of a pages site, do not delete pages MR previews --- app/models/pages_deployment.rb | 10 ++++++++++ app/services/pages/destroy_deployments_service.rb | 5 ++++- app/services/projects/update_pages_service.rb | 3 ++- app/workers/destroy_pages_deployments_worker.rb | 10 +++++++--- ...230610104800_create_pages_merge_request_previews.rb | 2 +- 5 files changed, 24 insertions(+), 6 deletions(-) diff --git a/app/models/pages_deployment.rb b/app/models/pages_deployment.rb index 2327e0ace09b22..8101349ad1d073 100644 --- a/app/models/pages_deployment.rb +++ b/app/models/pages_deployment.rb @@ -28,11 +28,21 @@ class PagesDeployment < ApplicationRecord scope :with_files_stored_locally, -> { where(file_store: ::ObjectStorage::Store::LOCAL) } scope :with_files_stored_remotely, -> { where(file_store: ::ObjectStorage::Store::REMOTE) } scope :project_id_in, ->(ids) { where(project_id: ids) } + scope :for_merge_request_preview, ->(merge_request_id) do joins(:preview) .where(pages_merge_request_previews: { merge_request_id: merge_request_id }) end + scope :without_merge_request_preview, -> do + where( + 'NOT EXISTS (?)', + Pages::MergeRequestPreview + .select(1) + .where('pages_merge_request_previews.pages_deployment_id = pages_deployments.id') + ) + end + validates :file, presence: true validates :file_store, presence: true, inclusion: { in: ObjectStorage::SUPPORTED_STORES } validates :size, presence: true, numericality: { greater_than: 0, only_integer: true } diff --git a/app/services/pages/destroy_deployments_service.rb b/app/services/pages/destroy_deployments_service.rb index 45d906bec7ac10..fea2a1eda0217a 100644 --- a/app/services/pages/destroy_deployments_service.rb +++ b/app/services/pages/destroy_deployments_service.rb @@ -2,14 +2,17 @@ module Pages class DestroyDeploymentsService - def initialize(project, last_deployment_id = nil) + def initialize(project, last_deployment_id = nil, skip_merge_request_previews: false) @project = project @last_deployment_id = last_deployment_id + @skip_merge_request_previews = skip_merge_request_previews end def execute deployments_to_destroy = @project.pages_deployments + deployments_to_destroy = deployments_to_destroy.without_merge_request_preview if @skip_merge_request_previews deployments_to_destroy = deployments_to_destroy.older_than(@last_deployment_id) if @last_deployment_id + deployments_to_destroy.find_each(&:destroy) # rubocop: disable CodeReuse/ActiveRecord end end diff --git a/app/services/projects/update_pages_service.rb b/app/services/projects/update_pages_service.rb index 5be3af8910fb43..6003926ab3541c 100644 --- a/app/services/projects/update_pages_service.rb +++ b/app/services/projects/update_pages_service.rb @@ -108,7 +108,8 @@ def update_project_pages_deployment(deployment) ::DestroyPagesDeploymentsWorker.perform_in( ::PagesDeployment::OLD_DEPLOYMENTS_DESTRUCTION_DELAY, project.id, - deployment.id + deployment.id, + true ) end diff --git a/app/workers/destroy_pages_deployments_worker.rb b/app/workers/destroy_pages_deployments_worker.rb index 7fa73648dd2323..28ec3a1000f356 100644 --- a/app/workers/destroy_pages_deployments_worker.rb +++ b/app/workers/destroy_pages_deployments_worker.rb @@ -7,15 +7,19 @@ class DestroyPagesDeploymentsWorker idempotent! - loggable_arguments 0, 1 + loggable_arguments 0, 1, 2 sidekiq_options retry: 3 feature_category :pages - def perform(project_id, last_deployment_id = nil) + def perform(project_id, last_deployment_id = nil, skip_merge_request_previews = false) project = Project.find_by_id(project_id) return unless project - ::Pages::DestroyDeploymentsService.new(project, last_deployment_id).execute + ::Pages::DestroyDeploymentsService.new( + project, + last_deployment_id, + skip_merge_request_previews: skip_merge_request_previews + ).execute end end diff --git a/db/migrate/20230610104800_create_pages_merge_request_previews.rb b/db/migrate/20230610104800_create_pages_merge_request_previews.rb index 6e03108b8cf403..da61e5c45b2998 100644 --- a/db/migrate/20230610104800_create_pages_merge_request_previews.rb +++ b/db/migrate/20230610104800_create_pages_merge_request_previews.rb @@ -4,7 +4,7 @@ class CreatePagesMergeRequestPreviews < Gitlab::Database::Migration[2.1] def up create_table(:pages_merge_request_previews) do |t| t.datetime_with_timezone :created_at, null: false - t.datetime_with_timezone :updated_at + t.datetime_with_timezone :updated_at, null: false t.bigint :pages_deployment_id, null: false t.bigint :merge_request_id, null: false t.text :slug, null: false, limit: 63, unique: true -- GitLab From ff8a35952f072c15320f19b3a5355f00cf2bfe10 Mon Sep 17 00:00:00 2001 From: Kassio Borges Date: Mon, 12 Jun 2023 23:52:13 +0100 Subject: [PATCH 13/18] add not null to updated at --- db/structure.sql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/db/structure.sql b/db/structure.sql index fb74ed0ead7e27..c0ef114fe82e5e 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -20025,7 +20025,7 @@ ALTER SEQUENCE pages_domains_id_seq OWNED BY pages_domains.id; CREATE TABLE pages_merge_request_previews ( id bigint NOT NULL, created_at timestamp with time zone NOT NULL, - updated_at timestamp with time zone, + updated_at timestamp with time zone NOT NULL, pages_deployment_id bigint NOT NULL, merge_request_id bigint NOT NULL, slug text NOT NULL, -- GitLab From c63c3378f0bea02c024b322b8b1a6ea41e819091 Mon Sep 17 00:00:00 2001 From: Kassio Borges Date: Tue, 13 Jun 2023 18:52:03 +0100 Subject: [PATCH 14/18] Add 'CI_PAGES_MERGE_REQUEST_PREVIEW_URL' Now it's possible to read the 'CI_PAGES_MERGE_REQUEST_PREVIEW_URL' during the `pages:preview` build. --- app/models/ci/build.rb | 11 ++++++++--- app/models/pages/merge_request_preview.rb | 4 ++++ app/models/project.rb | 14 +++++++------- app/services/projects/update_pages_service.rb | 5 ++--- .../pages/create_merge_request_preview_worker.rb | 3 +-- lib/gitlab/ci/variables/builder.rb | 4 ++++ 6 files changed, 26 insertions(+), 15 deletions(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 235bd1f6c5d41b..d82a1d0bd470fe 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -329,7 +329,7 @@ def clone_accessors build.run_after_commit do BuildSuccessWorker.perform_async(id) ::PagesWorker.perform_async(:deploy, id) if build.pages_generator? - ::Pages::CreateMergeRequestPreviewWorker.perform_async(id) if build.pages_preview_generator? + ::Pages::CreateMergeRequestPreviewWorker.perform_async(id) if build.pages_merge_request_preview_generator? end end @@ -414,11 +414,16 @@ def pages_generator? name == 'pages' end - def pages_preview_generator? - Gitlab.config.pages.enabled && + def pages_merge_request_preview_generator? + merge_request.present? && + Gitlab.config.pages.enabled && name == Pages::MergeRequestPreview::CI_JOB_NAME end + def pages_merge_request_preview_url + (merge_request.pages_preview || merge_request.create_pages_preview).url + end + def runnable? true end diff --git a/app/models/pages/merge_request_preview.rb b/app/models/pages/merge_request_preview.rb index 4f5cf08884325f..5c3c7a03b50725 100644 --- a/app/models/pages/merge_request_preview.rb +++ b/app/models/pages/merge_request_preview.rb @@ -28,6 +28,10 @@ class MergeRequestPreview < ::ApplicationRecord scope :with_deployment, -> { where.not(pages_deployment_id: nil) } scope :find_by_slug, ->(value) { find_by(slug: value) } + def url + project.pages_url_for(slug) + end + private def generate_slug diff --git a/app/models/project.rb b/app/models/project.rb index 452a5c8973c395..9c89fdfce67873 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -3241,13 +3241,6 @@ def frozen_outbound_job_token_scopes? end strong_memoize_attr :frozen_outbound_job_token_scopes? - private - - def pages_unique_domain_enabled? - Feature.enabled?(:pages_unique_domain, self) && - project_setting.pages_unique_domain_enabled? - end - def pages_url_for(domain) # The host in URL always needs to be downcased Gitlab.config.pages.url.sub(%r{^https?://}) do |prefix| @@ -3255,6 +3248,13 @@ def pages_url_for(domain) end.downcase end + private + + def pages_unique_domain_enabled? + Feature.enabled?(:pages_unique_domain, self) && + project_setting.pages_unique_domain_enabled? + end + # overridden in EE def project_group_links_with_preload project_group_links diff --git a/app/services/projects/update_pages_service.rb b/app/services/projects/update_pages_service.rb index 6003926ab3541c..bf9bee8ca7c90f 100644 --- a/app/services/projects/update_pages_service.rb +++ b/app/services/projects/update_pages_service.rb @@ -145,12 +145,11 @@ def update_merge_request_preview(deployment) # Mark pages:deploy as successful @commit_status.success - return if build.pipeline.all_merge_requests.order(iid: :asc).blank? # rubocop: disable CodeReuse/ActiveRecord + return if build.merge_request.blank? ::Pages::UpdateMergeRequestPreviewService.new( pages_deployment: deployment, - # TODO: for some reason, build.merge_request sometimes is nil - merge_request: build.pipeline.all_merge_requests.order(iid: :asc).first # rubocop: disable CodeReuse/ActiveRecord + merge_request: build.merge_request.blank? ).execute end end diff --git a/app/workers/pages/create_merge_request_preview_worker.rb b/app/workers/pages/create_merge_request_preview_worker.rb index c3d41ec4678292..1ef5eaa09ac8f9 100644 --- a/app/workers/pages/create_merge_request_preview_worker.rb +++ b/app/workers/pages/create_merge_request_preview_worker.rb @@ -16,8 +16,7 @@ class CreateMergeRequestPreviewWorker def perform(build_id) build = Ci::Build.find_by_id(build_id) - # TODO: for some reason, build.merge_request sometimes is nil - return if build.pipeline.all_merge_requests.order(iid: :asc).blank? # rubocop: disable CodeReuse/ActiveRecord + return if build.merge_request.blank? Projects::UpdatePagesService.new( build.project, diff --git a/lib/gitlab/ci/variables/builder.rb b/lib/gitlab/ci/variables/builder.rb index cae3a966bc61ee..d909a9133b1671 100644 --- a/lib/gitlab/ci/variables/builder.rb +++ b/lib/gitlab/ci/variables/builder.rb @@ -139,6 +139,10 @@ def predefined_variables(job) # Set environment name here so we can access it when evaluating the job's rules variables.append(key: 'CI_ENVIRONMENT_NAME', value: job.environment) if job.environment + + if job.pages_merge_request_preview_generator? + variables.append(key: 'CI_PAGES_MERGE_REQUEST_PREVIEW_URL', value: job.pages_merge_request_preview_url) + end end end -- GitLab From 53de25764d1ea48d92d6c1efa8984efe13d4331f Mon Sep 17 00:00:00 2001 From: Kassio Borges Date: Wed, 14 Jun 2023 10:17:52 +0100 Subject: [PATCH 15/18] check if project exists before building url --- app/models/pages/merge_request_preview.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/pages/merge_request_preview.rb b/app/models/pages/merge_request_preview.rb index 5c3c7a03b50725..74788f75fcb761 100644 --- a/app/models/pages/merge_request_preview.rb +++ b/app/models/pages/merge_request_preview.rb @@ -29,7 +29,7 @@ class MergeRequestPreview < ::ApplicationRecord scope :find_by_slug, ->(value) { find_by(slug: value) } def url - project.pages_url_for(slug) + project&.pages_url_for(slug) end private -- GitLab From db853f6eea5a77ebe4a7d7284ad5539a9a81a75f Mon Sep 17 00:00:00 2001 From: Kassio Borges Date: Wed, 14 Jun 2023 17:55:13 +0100 Subject: [PATCH 16/18] fix typo --- app/services/projects/update_pages_service.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/services/projects/update_pages_service.rb b/app/services/projects/update_pages_service.rb index bf9bee8ca7c90f..ef84d0f66d6c7f 100644 --- a/app/services/projects/update_pages_service.rb +++ b/app/services/projects/update_pages_service.rb @@ -149,7 +149,7 @@ def update_merge_request_preview(deployment) ::Pages::UpdateMergeRequestPreviewService.new( pages_deployment: deployment, - merge_request: build.merge_request.blank? + merge_request: build.merge_request ).execute end end -- GitLab From ff2d1ea8455f2c8d30c497421820c0086e6ff3a2 Mon Sep 17 00:00:00 2001 From: Kassio Borges Date: Thu, 15 Jun 2023 00:36:07 +0100 Subject: [PATCH 17/18] Fix merge_request checks and slug building --- app/models/pages/merge_request_preview.rb | 8 ++++---- app/workers/pages/create_merge_request_preview_worker.rb | 2 +- .../20230610104800_create_pages_merge_request_previews.rb | 4 ++-- db/structure.sql | 7 ++----- lib/gitlab/pages/virtual_host_finder.rb | 2 +- 5 files changed, 10 insertions(+), 13 deletions(-) diff --git a/app/models/pages/merge_request_preview.rb b/app/models/pages/merge_request_preview.rb index 74788f75fcb761..3c3326e3c2b98e 100644 --- a/app/models/pages/merge_request_preview.rb +++ b/app/models/pages/merge_request_preview.rb @@ -16,9 +16,11 @@ class MergeRequestPreview < ::ApplicationRecord class_name: 'PagesDeployment', foreign_key: :pages_deployment_id, inverse_of: :preview, - optional: false + optional: true - has_one :project, through: :deployment + has_one :project, + through: :merge_request, + source: :target_project before_validation :generate_slug, unless: :slug @@ -35,8 +37,6 @@ def url private def generate_slug - return if deployment.blank? - self.slug = Gitlab::Utils.slugify( "#{SLUG_PREFIX}-#{project.path.slice(0, 23)}-#{SecureRandom.hex(30)}" ) diff --git a/app/workers/pages/create_merge_request_preview_worker.rb b/app/workers/pages/create_merge_request_preview_worker.rb index 1ef5eaa09ac8f9..823a3efcf7ac51 100644 --- a/app/workers/pages/create_merge_request_preview_worker.rb +++ b/app/workers/pages/create_merge_request_preview_worker.rb @@ -16,7 +16,7 @@ class CreateMergeRequestPreviewWorker def perform(build_id) build = Ci::Build.find_by_id(build_id) - return if build.merge_request.blank? + return if build&.merge_request.blank? Projects::UpdatePagesService.new( build.project, diff --git a/db/migrate/20230610104800_create_pages_merge_request_previews.rb b/db/migrate/20230610104800_create_pages_merge_request_previews.rb index da61e5c45b2998..b4d126fc0bfa40 100644 --- a/db/migrate/20230610104800_create_pages_merge_request_previews.rb +++ b/db/migrate/20230610104800_create_pages_merge_request_previews.rb @@ -5,12 +5,12 @@ def up create_table(:pages_merge_request_previews) do |t| t.datetime_with_timezone :created_at, null: false t.datetime_with_timezone :updated_at, null: false - t.bigint :pages_deployment_id, null: false + t.bigint :pages_deployment_id t.bigint :merge_request_id, null: false t.text :slug, null: false, limit: 63, unique: true t.index :pages_deployment_id - t.index :merge_request_id + t.index :merge_request_id, unique: true end end diff --git a/db/structure.sql b/db/structure.sql index c0ef114fe82e5e..20ffe520d75d64 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -20026,7 +20026,7 @@ CREATE TABLE pages_merge_request_previews ( id bigint NOT NULL, created_at timestamp with time zone NOT NULL, updated_at timestamp with time zone NOT NULL, - pages_deployment_id bigint NOT NULL, + pages_deployment_id bigint, merge_request_id bigint NOT NULL, slug text NOT NULL, CONSTRAINT check_5ed1917d35 CHECK ((char_length(slug) <= 63)) @@ -32236,7 +32236,7 @@ CREATE INDEX index_pages_domains_on_verified_at_and_enabled_until ON pages_domai CREATE INDEX index_pages_domains_on_wildcard ON pages_domains USING btree (wildcard); -CREATE INDEX index_pages_merge_request_previews_on_merge_request_id ON pages_merge_request_previews USING btree (merge_request_id); +CREATE UNIQUE INDEX index_pages_merge_request_previews_on_merge_request_id ON pages_merge_request_previews USING btree (merge_request_id); CREATE INDEX index_pages_merge_request_previews_on_pages_deployment_id ON pages_merge_request_previews USING btree (pages_deployment_id); @@ -35228,9 +35228,6 @@ ALTER TABLE ONLY issues ALTER TABLE ONLY merge_requests ADD CONSTRAINT fk_06067f5644 FOREIGN KEY (latest_merge_request_diff_id) REFERENCES merge_request_diffs(id) ON DELETE SET NULL; -ALTER TABLE ONLY pages_merge_request_previews - ADD CONSTRAINT fk_078398e5be FOREIGN KEY (merge_request_id) REFERENCES merge_requests(id) ON DELETE CASCADE; - ALTER TABLE ONLY user_interacted_projects ADD CONSTRAINT fk_0894651f08 FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE CASCADE; diff --git a/lib/gitlab/pages/virtual_host_finder.rb b/lib/gitlab/pages/virtual_host_finder.rb index bc7559c972ebee..7e8875c8123e52 100644 --- a/lib/gitlab/pages/virtual_host_finder.rb +++ b/lib/gitlab/pages/virtual_host_finder.rb @@ -30,7 +30,7 @@ def execute def by_review_app_domain(name) review_app = ::Pages::MergeRequestPreview.with_deployment.find_by_slug(name) - return unless review_app + return if review_app.blank? ::Pages::VirtualDomain.new( projects: [review_app.project], -- GitLab From 06919d6ac0a07ed3796922cc2d7b938ada47249f Mon Sep 17 00:00:00 2001 From: Kassio Borges Date: Thu, 15 Jun 2023 11:19:32 +0100 Subject: [PATCH 18/18] Use * as slug separator --- app/models/pages/merge_request_preview.rb | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/app/models/pages/merge_request_preview.rb b/app/models/pages/merge_request_preview.rb index 3c3326e3c2b98e..a298e473fe1ddf 100644 --- a/app/models/pages/merge_request_preview.rb +++ b/app/models/pages/merge_request_preview.rb @@ -37,9 +37,12 @@ def url private def generate_slug - self.slug = Gitlab::Utils.slugify( - "#{SLUG_PREFIX}-#{project.path.slice(0, 23)}-#{SecureRandom.hex(30)}" - ) + self.slug = format( + '%s*%s*%s', + SLUG_PREFIX, + project.path.slice(0, 23), + SecureRandom.hex(30) + )[0..62] end end end -- GitLab