diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2023-11-17 00:29:57 +0300 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2023-11-17 00:29:57 +0300 |
commit | 899da2d359848b005984373052b2e697fd5d10f2 (patch) | |
tree | b4b93613f08ed28f8c954b2517742e8d71029ed7 | |
parent | 6d558d71eba0e5c8025e2a093ce8d144b1fdd4cf (diff) |
Add latest changes from gitlab-org/gitlab@16-6-stable-ee
-rw-r--r-- | app/models/pages_deployment.rb | 22 | ||||
-rw-r--r-- | app/services/projects/update_pages_service.rb | 3 | ||||
-rw-r--r-- | db/migrate/20231115091119_add_upload_finished_to_pages_deployments.rb | 15 | ||||
-rw-r--r-- | db/post_migrate/20231115151449_update_pages_deployments_upload_ready_default_value.rb | 14 | ||||
-rw-r--r-- | db/schema_migrations/20231115091119 | 1 | ||||
-rw-r--r-- | db/schema_migrations/20231115151449 | 1 | ||||
-rw-r--r-- | db/structure.sql | 1 | ||||
-rw-r--r-- | spec/models/pages_deployment_spec.rb | 10 | ||||
-rw-r--r-- | spec/uploaders/pages/deployment_uploader_spec.rb | 39 |
9 files changed, 86 insertions, 20 deletions
diff --git a/app/models/pages_deployment.rb b/app/models/pages_deployment.rb index 2aa36a94171..0d87a8f6cf6 100644 --- a/app/models/pages_deployment.rb +++ b/app/models/pages_deployment.rb @@ -5,6 +5,9 @@ class PagesDeployment < ApplicationRecord include EachBatch include FileStoreMounter include Gitlab::Utils::StrongMemoize + include SafelyChangeColumnDefault + + columns_changing_default :upload_ready attribute :file_store, :integer, default: -> { ::Pages::DeploymentUploader.default_store } @@ -18,7 +21,12 @@ class PagesDeployment < ApplicationRecord scope :with_files_stored_remotely, -> { where(file_store: ::ObjectStorage::Store::REMOTE) } scope :project_id_in, ->(ids) { where(project_id: ids) } scope :with_path_prefix, ->(prefix) { where("COALESCE(path_prefix, '') = ?", prefix.to_s) } - scope :active, -> { where(deleted_at: nil).order(created_at: :desc) } + + # We have to mark the PagesDeployment upload as ready to ensure we only + # serve PagesDeployment which files are already uploaded. + scope :upload_ready, -> { where(upload_ready: true) } + + scope :active, -> { upload_ready.where(deleted_at: nil).order(created_at: :desc) } scope :deactivated, -> { where('deleted_at < ?', Time.now.utc) } validates :file, presence: true @@ -64,6 +72,18 @@ class PagesDeployment < ApplicationRecord return unless previous_changes.key?(:file) store_file_now! + mark_upload_as_finished! + end + + # We have to mark the PagesDeployment upload as ready to ensure we only + # serve PagesDeployment which files are already uploaded. + # + # This is required because we're uploading the file outside of the db transaction + # (https://gitlab.com/gitlab-org/gitlab/-/merge_requests/114774) + def mark_upload_as_finished! + return unless file && file.exists? + + update_column(:upload_ready, true) end end diff --git a/app/services/projects/update_pages_service.rb b/app/services/projects/update_pages_service.rb index 83b28840d39..fd6c9a86540 100644 --- a/app/services/projects/update_pages_service.rb +++ b/app/services/projects/update_pages_service.rb @@ -98,7 +98,8 @@ module Projects file_count: deployment_update.entries_count, file_sha256: build.job_artifacts_archive.file_sha256, ci_build_id: build.id, - root_directory: build.options[:publish] + root_directory: build.options[:publish], + upload_ready: false } end diff --git a/db/migrate/20231115091119_add_upload_finished_to_pages_deployments.rb b/db/migrate/20231115091119_add_upload_finished_to_pages_deployments.rb new file mode 100644 index 00000000000..1165bd11874 --- /dev/null +++ b/db/migrate/20231115091119_add_upload_finished_to_pages_deployments.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +class AddUploadFinishedToPagesDeployments < Gitlab::Database::Migration[2.2] + milestone '16.7' + enable_lock_retries! + + def change + # Existing deployments must be considered `upload_ready` For this reason, + # the column is created with `default: true`, and then changed to + # `default: false` in post_migrate/20231115151449_update_pages_deployments_upload_ready_default_value.rb. + # This way, existing deployments are set as `upload_ready: true`, + # but new ones are created as `upload_ready: false` + add_column :pages_deployments, :upload_ready, :boolean, default: true + end +end diff --git a/db/post_migrate/20231115151449_update_pages_deployments_upload_ready_default_value.rb b/db/post_migrate/20231115151449_update_pages_deployments_upload_ready_default_value.rb new file mode 100644 index 00000000000..9ab44611499 --- /dev/null +++ b/db/post_migrate/20231115151449_update_pages_deployments_upload_ready_default_value.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +class UpdatePagesDeploymentsUploadReadyDefaultValue < Gitlab::Database::Migration[2.2] + milestone '16.7' + enable_lock_retries! + + def up + change_column_default :pages_deployments, :upload_ready, from: true, to: false + end + + def down + change_column_default :pages_deployments, :upload_ready, from: false, to: true + end +end diff --git a/db/schema_migrations/20231115091119 b/db/schema_migrations/20231115091119 new file mode 100644 index 00000000000..9e1669953fd --- /dev/null +++ b/db/schema_migrations/20231115091119 @@ -0,0 +1 @@ +8974454db089c031509a6218134ac6c6bb53bd3a028f0ba83e7329d7d6445a59
\ No newline at end of file diff --git a/db/schema_migrations/20231115151449 b/db/schema_migrations/20231115151449 new file mode 100644 index 00000000000..01d16b2a310 --- /dev/null +++ b/db/schema_migrations/20231115151449 @@ -0,0 +1 @@ +493ba17c2dd4519c734c7ea31c2c9c6651c73b0892bd17e8fade699491f78a89
\ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 1055e902056..dd784030356 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -20744,6 +20744,7 @@ CREATE TABLE pages_deployments ( path_prefix text, build_ref text, deleted_at timestamp with time zone, + upload_ready boolean DEFAULT false, CONSTRAINT check_4d04b8dc9a CHECK ((char_length(path_prefix) <= 128)), CONSTRAINT check_5f9132a958 CHECK ((size IS NOT NULL)), CONSTRAINT check_7e938c810a CHECK ((char_length(root_directory) <= 255)), diff --git a/spec/models/pages_deployment_spec.rb b/spec/models/pages_deployment_spec.rb index 1e6f8b33a86..fdd320f13ad 100644 --- a/spec/models/pages_deployment_spec.rb +++ b/spec/models/pages_deployment_spec.rb @@ -71,6 +71,16 @@ RSpec.describe PagesDeployment, feature_category: :pages do end end + describe '#upload_ready' do + it 'marks #upload_ready as true when upload finishes' do + deployment = build(:pages_deployment) + + expect { deployment.save! } + .to change { deployment.upload_ready } + .from(false).to(true) + end + end + describe '.deactivate_all', :freeze_time do let!(:deployment) { create(:pages_deployment, project: project, updated_at: 5.minutes.ago) } let!(:nil_path_prefix_deployment) { create(:pages_deployment, project: project, path_prefix: nil) } diff --git a/spec/uploaders/pages/deployment_uploader_spec.rb b/spec/uploaders/pages/deployment_uploader_spec.rb index a5fe2dfe9ba..efca5bd0cf1 100644 --- a/spec/uploaders/pages/deployment_uploader_spec.rb +++ b/spec/uploaders/pages/deployment_uploader_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Pages::DeploymentUploader do +RSpec.describe Pages::DeploymentUploader, feature_category: :pages do let(:pages_deployment) { create(:pages_deployment) } let(:uploader) { described_class.new(pages_deployment, :file) } @@ -22,38 +22,41 @@ RSpec.describe Pages::DeploymentUploader do stub_pages_object_storage end - it_behaves_like 'builds correct paths', store_dir: %r[\A\h{2}/\h{2}/\h{64}/pages_deployments/\d+\z] + describe '.default_store' do + it 'returns remote store when object storage is enabled' do + expect(described_class.default_store).to eq(ObjectStorage::Store::REMOTE) + end + end it 'preserves original file when stores it' do uploader.store!(file) expect(File.exist?(file.path)).to be true end + + it_behaves_like 'builds correct paths', + store_dir: %r[\A\h{2}/\h{2}/\h{64}/pages_deployments/\d+\z] end context 'when file is stored in valid local_path' do - before do - uploader.store!(file) + describe '.default_store' do + it 'returns local store when object storage is not enabled' do + expect(described_class.default_store).to eq(ObjectStorage::Store::LOCAL) + end end - subject { uploader.file.path } - - it { is_expected.to match(%r[#{uploader.root}/@hashed/\h{2}/\h{2}/\h{64}/pages_deployments/#{pages_deployment.id}/pages.zip]) } - - it 'preserves original file when stores it' do - expect(File.exist?(file.path)).to be true - end - end + it 'builds the right file path' do + uploader.store!(file) - describe '.default_store' do - it 'returns local store when object storage is not enabled' do - expect(described_class.default_store).to eq(ObjectStorage::Store::LOCAL) + expect(uploader.file.path).to match( + %r[#{uploader.root}/@hashed/\h{2}/\h{2}/\h{64}/pages_deployments/#{pages_deployment.id}/pages.zip] + ) end - it 'returns remote store when object storage is enabled' do - stub_pages_object_storage + it 'preserves original file when stores it' do + uploader.store!(file) - expect(described_class.default_store).to eq(ObjectStorage::Store::REMOTE) + expect(File.exist?(file.path)).to be true end end end |