Commit 1df93ae5 authored by David Fernandez's avatar David Fernandez

Refactor the nuget package updater service

To use the `packages_nuget_new_package_file_updater` flag which controls
how the package file is updated and how the underlying file in object
storage is moved.

With the feature flag disabled: the file is downloaded (multiple times)
to get re-uploaded. This doesn't work well with package files linked to
a heavy file on object storage.
With the feature flag enabled: the file is simply moved in object
storage (copy command followed by destroy command), thus avoiding
downloading and uploading the actual file.

This commit introduces a new service to handle this file move in object
storage.

Rollout issue: https://gitlab.com/gitlab-org/gitlab/-/issues/336511

Changelog: fixed
parent b8d4915b
......@@ -10,6 +10,9 @@ class Packages::PackageFile < ApplicationRecord
belongs_to :package
# used to move the linked file within object storage
attribute :new_file_path, default: nil
has_one :conan_file_metadatum, inverse_of: :package_file, class_name: 'Packages::Conan::FileMetadatum'
has_many :package_file_build_infos, inverse_of: :package_file, class_name: 'Packages::PackageFileBuildInfo'
has_many :pipelines, through: :package_file_build_infos
......@@ -80,6 +83,12 @@ class Packages::PackageFile < ApplicationRecord
before_save :update_size_from_file
# if a new_file_path is provided, we need
# * disable the remove_previously_stored_file callback so that carrierwave doesn't take care of the file
# * enable a new after_commit callback that will move the file in object storage
skip_callback :commit, :after, :remove_previously_stored_file, if: :execute_move_in_object_storage?
after_commit :move_in_object_storage, if: :execute_move_in_object_storage?
def download_path
Gitlab::Routing.url_helpers.download_project_package_file_path(project, self)
end
......@@ -89,6 +98,17 @@ class Packages::PackageFile < ApplicationRecord
def update_size_from_file
self.size ||= file.size
end
def execute_move_in_object_storage?
!file.file_storage? && new_file_path?
end
def move_in_object_storage
carrierwave_file = file.file
carrierwave_file.copy_to(new_file_path)
carrierwave_file.delete
end
end
Packages::PackageFile.prepend_mod_with('Packages::PackageFile')
......@@ -21,22 +21,11 @@ module Packages
try_obtain_lease do
@package_file.transaction do
if existing_package
package = link_to_existing_package
elsif symbol_package?
raise InvalidMetadataError, 'symbol package is invalid, matching package does not exist'
if use_new_package_file_updater?
new_execute
else
package = update_linked_package
legacy_execute
end
update_package(package)
# Updating file_name updates the path where the file is stored.
# We must pass the file again so that CarrierWave can handle the update
@package_file.update!(
file_name: package_filename,
file: @package_file.file
)
end
end
rescue ActiveRecord::RecordInvalid => e
......@@ -45,6 +34,52 @@ module Packages
private
def new_execute
package_to_destroy = nil
target_package = @package_file.package
if existing_package
package_to_destroy = @package_file.package
target_package = existing_package
else
if symbol_package?
raise InvalidMetadataError, 'symbol package is invalid, matching package does not exist'
end
update_linked_package
end
update_package(target_package)
::Packages::UpdatePackageFileService.new(@package_file, package_id: target_package.id, file_name: package_filename)
.execute
package_to_destroy&.destroy!
end
def legacy_execute
if existing_package
package = link_to_existing_package
elsif symbol_package?
raise InvalidMetadataError, 'symbol package is invalid, matching package does not exist'
else
package = update_linked_package
end
update_package(package)
# Updating file_name updates the path where the file is stored.
# We must pass the file again so that CarrierWave can handle the update
@package_file.update!(
file_name: package_filename,
file: @package_file.file
)
end
def use_new_package_file_updater?
::Feature.enabled?(:packages_nuget_new_package_file_updater, @package_file.project, default_enabled: :yaml)
end
def update_package(package)
return if symbol_package?
......
# frozen_string_literal: true
module Packages
class UpdatePackageFileService
delegate :file, to: :@package_file
def initialize(package_file, params)
@package_file = package_file
@params = params
end
def execute
check_params
return if same_as_params?
# we need to access the file *before* updating the attributes linked to its path/key.
file_storage_mode = file.file_storage?
@package_file.package_id = package_id if package_id
@package_file.file_name = file_name if file_name
if file_storage_mode
# package file is in mode LOCAL: we can pass the `file` to the update
@package_file.file = file
else
# package file is in mode REMOTE: don't pass the `file` to the update
# instead, pass the new file path. This will move the file
# in object storage.
@package_file.new_file_path = File.join(file.store_dir, @package_file.file_name)
end
@package_file.save!
end
private
def check_params
raise ArgumentError, 'package_file not persisted' unless @package_file.persisted?
raise ArgumentError, 'package_id and file_name are blank' if package_id.blank? && file_name.blank?
end
def same_as_params?
return false if package_id && package_id != @package_file.package_id
return false if file_name && file_name != @package_file.file_name
true
end
def package_id
@params[:package_id]
end
def file_name
@params[:file_name]
end
end
end
---
name: packages_nuget_new_package_file_updater
introduced_by_url:
rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/336511
milestone: '14.1'
type: development
group: group::package
default_enabled: false
......@@ -159,4 +159,71 @@ RSpec.describe Packages::PackageFile, type: :model do
expect { subject }.to change { package_file.size }.from(nil).to(3513)
end
end
context 'update callbacks' do
subject { package_file.save! }
shared_examples 'executing the default callback' do
it 'executes the default callback' do
expect(package_file).to receive(:remove_previously_stored_file)
expect(package_file).not_to receive(:move_in_object_storage)
subject
end
end
context 'with object storage disabled' do
let(:package_file) { create(:package_file, file_name: 'file_name.txt') }
before do
stub_package_file_object_storage(enabled: false)
end
it_behaves_like 'executing the default callback'
context 'with new_file_path set' do
before do
package_file.new_file_path = 'test'
end
it_behaves_like 'executing the default callback'
end
end
context 'with object storage enabled' do
let(:package_file) do
create(
:package_file,
file_name: 'file_name.txt',
file: CarrierWaveStringFile.new_file(
file_content: 'content',
filename: 'file_name.txt',
content_type: 'text/plain'
),
file_store: ::Packages::PackageFileUploader::Store::REMOTE
)
end
before do
stub_package_file_object_storage(enabled: true)
end
it_behaves_like 'executing the default callback'
context 'with new_file_path set' do
before do
package_file.new_file_path = 'test'
end
it 'executes the move_in_object_storage callback' do
expect(package_file).not_to receive(:remove_previously_stored_file)
expect(package_file).to receive(:move_in_object_storage).and_call_original
expect(package_file.file.file).to receive(:copy_to).and_call_original
expect(package_file.file.file).to receive(:delete).and_call_original
subject
end
end
end
end
end
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe Packages::UpdatePackageFileService do
let_it_be(:another_package) { create(:package) }
let_it_be(:old_file_name) { 'old_file_name.txt' }
let_it_be(:new_file_name) { 'new_file_name.txt' }
let(:package) { package_file.package }
let(:params) { { package_id: another_package.id, file_name: new_file_name } }
let(:service) { described_class.new(package_file, params) }
describe '#execute' do
subject { service.execute }
shared_examples 'updating package file with valid parameters' do
context 'with both parameters set' do
it 'updates the package file accordingly' do
expect { subject }
.to change { package.package_files.count }.from(1).to(0)
.and change { another_package.package_files.count }.from(0).to(1)
.and change { package_file.package_id }.from(package.id).to(another_package.id)
.and change { package_file.file_name }.from(old_file_name).to(new_file_name)
end
end
context 'with only file_name set' do
let(:params) { { file_name: new_file_name } }
it 'updates the package file accordingly' do
expect { subject }
.to not_change { package.package_files.count }
.and not_change { another_package.package_files.count }
.and not_change { package_file.package_id }
.and change { package_file.file_name }.from(old_file_name).to(new_file_name)
end
end
context 'with only package_id set' do
let(:params) { { package_id: another_package.id } }
it 'updates the package file accordingly' do
expect { subject }
.to change { package.package_files.count }.from(1).to(0)
.and change { another_package.package_files.count }.from(0).to(1)
.and change { package_file.package_id }.from(package.id).to(another_package.id)
.and not_change { package_file.file_name }
end
end
end
shared_examples 'not updating package with invalid parameters' do
context 'with blank parameters' do
let(:params) { {} }
it 'raise an argument error' do
expect { subject }.to raise_error(ArgumentError, 'package_id and file_name are blank')
end
end
context 'with non persisted package file' do
let(:package_file) { build(:package_file) }
it 'raise an argument error' do
expect { subject }.to raise_error(ArgumentError, 'package_file not persisted')
end
end
end
context 'with object storage disabled' do
let(:package_file) { create(:package_file, file_name: old_file_name) }
before do
stub_package_file_object_storage(enabled: false)
end
it_behaves_like 'updating package file with valid parameters' do
before do
expect(package_file).to receive(:remove_previously_stored_file).and_call_original
expect(package_file).not_to receive(:move_in_object_storage)
end
end
it_behaves_like 'not updating package with invalid parameters'
end
context 'with object storage enabled' do
let(:package_file) do
create(
:package_file,
file_name: old_file_name,
file: CarrierWaveStringFile.new_file(
file_content: 'content',
filename: old_file_name,
content_type: 'text/plain'
),
file_store: ::Packages::PackageFileUploader::Store::REMOTE
)
end
before do
stub_package_file_object_storage(enabled: true)
end
it_behaves_like 'updating package file with valid parameters' do
before do
expect(package_file).not_to receive(:remove_previously_stored_file)
expect(package_file).to receive(:move_in_object_storage).and_call_original
end
end
it_behaves_like 'not updating package with invalid parameters' do
before do
expect(package_file.file.file).not_to receive(:copy_to)
end
end
end
end
end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment