diff --git a/ee/app/models/concerns/geo/blob_replicator_strategy.rb b/ee/app/models/concerns/geo/blob_replicator_strategy.rb index b2e3b4cf5f6d4b797c6b086e456ffdba943d3602..41975af4a6606b8dcadad23252417ca1e6593d02 100644 --- a/ee/app/models/concerns/geo/blob_replicator_strategy.rb +++ b/ee/app/models/concerns/geo/blob_replicator_strategy.rb @@ -140,13 +140,17 @@ def removed_blob_path(uploader_class, path) File.join(uploader_class.constantize.root, path) end - # Returns a checksum of the file + # Returns a checksum for the local files and file size for remote ones # - # @return [String] SHA256 hash of the carrierwave file + # @return [String] SHA256 hash or file size def calculate_checksum raise 'File is not checksummable' unless checksummable? - model.sha256_hexdigest(blob_path) + if carrierwave_uploader.file_storage? + model.sha256_hexdigest(blob_path) + else + carrierwave_uploader.file.size.to_s + end end # Returns whether the file exists on disk or in remote storage @@ -178,7 +182,11 @@ def download # # @return [Boolean] whether it can generate a checksum def checksummable? - carrierwave_uploader.file_storage? && file_exists? + if Feature.enabled?(:geo_object_storage_verification) + file_exists? + else + carrierwave_uploader.file_storage? && file_exists? + end end # Return whether it's immutable diff --git a/ee/app/models/concerns/geo/replicable_model.rb b/ee/app/models/concerns/geo/replicable_model.rb index 678332ff0a4eb482c7477e2214754aa5dbcfdcd0..5a7baec0893e0ecd4f1a1bf2e0555697a5d7f1ee 100644 --- a/ee/app/models/concerns/geo/replicable_model.rb +++ b/ee/app/models/concerns/geo/replicable_model.rb @@ -31,11 +31,15 @@ module ReplicableModel scope :available_replicables, -> { all } # On primary, `verifiables` are records that can be checksummed and/or are replicable. - # On secondary, `verifiables` are records that have already been replicated # and (ideally) have been checksummed on the primary - - scope :verifiables, -> { self.respond_to?(:with_files_stored_locally) ? available_replicables.with_files_stored_locally : available_replicables } + scope :verifiables, -> do + if Feature.enabled?(:geo_object_storage_verification) + available_replicables + else + self.respond_to?(:with_files_stored_locally) ? available_replicables.with_files_stored_locally : available_replicables + end + end # When storing verification details in the same table as the model, # the scope `available_verifiables` returns only those records diff --git a/ee/config/feature_flags/development/geo_object_storage_verification.yml b/ee/config/feature_flags/development/geo_object_storage_verification.yml new file mode 100644 index 0000000000000000000000000000000000000000..6e30f5d9f8cef65d6cbda76ecce846e8c80db67f --- /dev/null +++ b/ee/config/feature_flags/development/geo_object_storage_verification.yml @@ -0,0 +1,8 @@ +--- +name: geo_object_storage_verification +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/127408 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/410387 +milestone: '16.4' +type: development +group: group::geo +default_enabled: false diff --git a/ee/lib/gitlab/geo/replication/blob_downloader.rb b/ee/lib/gitlab/geo/replication/blob_downloader.rb index d9b0a653d2e9a1edbe6316fb65b66d87501592e6..b72e1624e1346f4e2106a4bc89a717cf7ff36052 100644 --- a/ee/lib/gitlab/geo/replication/blob_downloader.rb +++ b/ee/lib/gitlab/geo/replication/blob_downloader.rb @@ -233,7 +233,11 @@ def checksum_mismatch?(file_path) end def actual_checksum(file_path) - @actual_checksum = Digest::SHA256.file(file_path).hexdigest + @actual_checksum = if file_storage? + Digest::SHA256.file(file_path).hexdigest + elsif Feature.enabled?(:geo_object_storage_verification) + File.size(file_path).to_s + end end def non_success_response_result(response, url) diff --git a/ee/spec/lib/gitlab/geo/replication/blob_downloader_spec.rb b/ee/spec/lib/gitlab/geo/replication/blob_downloader_spec.rb index 5921ed86138e1e9ea100a06370fd1f38f0528786..51887ade00c5ddac8ce3a80a5ea04bddc4cc32e1 100644 --- a/ee/spec/lib/gitlab/geo/replication/blob_downloader_spec.rb +++ b/ee/spec/lib/gitlab/geo/replication/blob_downloader_spec.rb @@ -70,6 +70,8 @@ let(:sync_object_storage) { true } context 'when the primary proxies remote storage' do + let(:model_record) { create(:package_file) } + it 'returns success' do content = replicator.carrierwave_uploader.file.read size = content.bytesize @@ -87,6 +89,7 @@ let(:content) { replicator.carrierwave_uploader.file.read } let(:size) { content.bytesize } let(:remote_url) { replicator.carrierwave_uploader.url } + let(:model_record) { create(:package_file) } before do # Set up to ensure that our redirect follow implementation does @@ -229,6 +232,29 @@ expect_blob_downloader_result(result, success: true, bytes_downloaded: content.bytesize, primary_missing_file: false) end end + + context 'when file is in object storage and has the filesize as a checksum on primary' do + let!(:secondary_object_storage) { create(:geo_node, sync_object_storage: true) } + + before do + stub_package_file_object_storage(enabled: true, direct_upload: true) + stub_current_geo_node(secondary_object_storage) + end + + let!(:model_record) { create(:package_file, :npm, :object_storage) } + + it 'returns a successful result' do + allow(replicator).to receive(:primary_checksum).and_return("3") + + content = 'foo' # 3 bytes + stub_request(:get, subject.resource_url) + .to_return(status: 200, body: content) + + result = subject.execute + + expect_blob_downloader_result(result, success: true, bytes_downloaded: content.bytesize, primary_missing_file: false) + end + end end end diff --git a/ee/spec/models/concerns/geo/replicable_model_spec.rb b/ee/spec/models/concerns/geo/replicable_model_spec.rb index 36fe1044b7c771f22adb4ad003ca40d18a8dd7ad..85af56d72aa1dd52b302490b1ddac8a4c1a82664 100644 --- a/ee/spec/models/concerns/geo/replicable_model_spec.rb +++ b/ee/spec/models/concerns/geo/replicable_model_spec.rb @@ -69,23 +69,40 @@ end describe '.verifiables' do - context 'when the model can be filtered by locally stored files' do - it 'filters by locally stored files' do - allow(DummyModel).to receive(:respond_to?).with(:all).and_call_original - allow(DummyModel).to receive(:respond_to?).with(:with_files_stored_locally).and_return(true) + context 'when geo_object_storage_verification feature flag is disabled' do + before do + stub_feature_flags(geo_object_storage_verification: false) + end - expect(DummyModel).to receive(:with_files_stored_locally) + context 'when the model can be filtered by locally stored files' do + it 'filters by locally stored files' do + allow(DummyModel).to receive(:respond_to?).with(:all).and_call_original + allow(DummyModel).to receive(:respond_to?).with(:with_files_stored_locally).and_return(true) - DummyModel.verifiables + expect(DummyModel).to receive(:with_files_stored_locally) + + DummyModel.verifiables + end end - end - context 'when the model cannot be filtered by locally stored files' do - it 'does not filter by locally stored files' do - allow(DummyModel).to receive(:respond_to?).with(:all).and_call_original - allow(DummyModel).to receive(:respond_to?).with(:with_files_stored_locally).and_return(false) + context 'when the model cannot be filtered by locally stored files' do + it 'does not filter by locally stored files' do + allow(DummyModel).to receive(:respond_to?).with(:all).and_call_original + allow(DummyModel).to receive(:respond_to?).with(:with_files_stored_locally).and_return(false) + + expect(DummyModel).not_to receive(:with_files_stored_locally) + + DummyModel.verifiables + end + end + end - expect(DummyModel).not_to receive(:with_files_stored_locally) + # We don't need to test the case when geo_object_storage_verification is enabled + # because the whole .verifiables method won't be needed anymore after the FF is removed. + # This one has only symbolic meaning before the removal + context 'when geo_object_storage_verification feature flag is enabled' do + it 'aliasses to .available_replicables' do + expect(DummyModel).to receive(:available_replicables) DummyModel.verifiables end diff --git a/ee/spec/models/ee/ci/job_artifact_spec.rb b/ee/spec/models/ee/ci/job_artifact_spec.rb index 01e72bebe76a064fb142364b14d346447b8e79f5..ada26916ac3a988ff2fe0e17e9b4348cd15e998e 100644 --- a/ee/spec/models/ee/ci/job_artifact_spec.rb +++ b/ee/spec/models/ee/ci/job_artifact_spec.rb @@ -8,35 +8,6 @@ let_it_be(:job) { create(:ci_build) } - describe '#save_verification_details' do - let(:verifiable_model_record) { build(:ci_job_artifact, :trace, job: job) } - let(:verification_state_table_class) { verifiable_model_record.class.verification_state_table_class } - - before do - stub_primary_node - end - - context 'when direct upload is enabled for trace artifacts' do - before do - stub_artifacts_object_storage(JobArtifactUploader, direct_upload: true) - end - - it 'does not create verification details' do - expect { verifiable_model_record.save! }.not_to change { verification_state_table_class.count } - end - end - - context 'when direct upload is not enabled' do - before do - stub_artifacts_object_storage(JobArtifactUploader, direct_upload: false) - end - - it 'creates verification details' do - expect { verifiable_model_record.save! }.to change { verification_state_table_class.count }.by(1) - end - end - end - include_examples 'a replicable model with a separate table for verification state' do before do stub_artifacts_object_storage diff --git a/ee/spec/support/shared_examples/models/concerns/blob_replicator_strategy_shared_examples.rb b/ee/spec/support/shared_examples/models/concerns/blob_replicator_strategy_shared_examples.rb index 45da48770ac62c2c4360d24250eff5b687a46d2f..6687d870506fccc00a00d622dcd2aeb9428b83df 100644 --- a/ee/spec/support/shared_examples/models/concerns/blob_replicator_strategy_shared_examples.rb +++ b/ee/spec/support/shared_examples/models/concerns/blob_replicator_strategy_shared_examples.rb @@ -276,10 +276,13 @@ context 'when the file is remotely stored' do it 'raises an error' do - carrierwave_uploader = double(file_storage?: false) - allow(subject).to receive(:carrierwave_uploader).and_return(carrierwave_uploader) + carrierwave_uploader = replicator.carrierwave_uploader + allow(carrierwave_uploader).to receive(:file_storage?).and_return(false) + allow(carrierwave_uploader).to receive_message_chain(:file, size: 65112) + allow(carrierwave_uploader).to receive_message_chain(:file, exists?: true) + allow(replicator).to receive(:carrierwave_uploader).and_return(carrierwave_uploader) - expect { subject.calculate_checksum }.to raise_error('File is not checksummable') + expect(subject.calculate_checksum).to eq('65112') end end end diff --git a/ee/spec/support/shared_examples/models/concerns/replicable_model_with_separate_table_shared_examples.rb b/ee/spec/support/shared_examples/models/concerns/replicable_model_with_separate_table_shared_examples.rb index dca2e4ddccc95cc7231f158a4a735d5156a2e167..55bd19f974d7a15f146d28d940d3c74e2292dd90 100644 --- a/ee/spec/support/shared_examples/models/concerns/replicable_model_with_separate_table_shared_examples.rb +++ b/ee/spec/support/shared_examples/models/concerns/replicable_model_with_separate_table_shared_examples.rb @@ -12,6 +12,10 @@ RSpec.shared_examples 'a replicable model with a separate table for verification state' do include EE::GeoHelpers + before do + stub_feature_flags(geo_object_storage_verification: false) + end + context 'on a primary node' do before do stub_primary_node