Skip to content

Add loose FK between package tables and ci pipelines

David Fernandez requested to merge 345664-add-build-infos-pipelines-loose-keys into master

⚔ Context

In the context of database decomposition, we have to remove all the database links to ci_* tables.

In package, we have two tables that have foreign keys to table ci_pipelines:

  • packages_build_infos
  • packages_package_file_build_infos

Now, build info tables have this requirement of not be destroyed when a pipeline is destroyed. To achieve this when the foreign key existed, we used a CASCADE SET NULL restriction which worked well.

In !70614 (merged), we removed the database restriction and added has_many X, dependent: :nullify statements to simulate the same behavior purely on the rails side.

The problem is that we can't do that due to https://docs.gitlab.com/ee/development/database/multiple_databases.html#removing-cross-database-transactions:

When at least two different databases are modified during a transaction initiated on any database server, the application triggers a cross-database modification error (only in test environment).

and that's exactly what will happen with the dependent: :nullify option.

As suggested in #345664 (closed), the way to fix this is by using loose foreign keys.

We can use them here despite the drawbacks because:

  • build_info objects are never accessed directly (such as /packages/23/build_infos/42)
  • the are mainly used for object navigation between a package and a pipeline.

🔬 What does this MR do and why?

  • Add loose foreign keys between the package build info tables and ci pipelines table
  • Remove the not so useful associations from the ci pipeline model
    • objects are navigated from package -> pipeline and never the other way around = we don't need the association on the pipeline object to go to a package related object.

🎥 Screenshots or screen recordings

We created a package and uploaded it the same version using 3 pipelines:

Packages::Package.find(789).build_infos.map(&:pipeline_id)
=> [367, 368, 369]

After deleting a pipeline and waiting for the background worker to cleanup the foreign keys, we get the intended result:

Packages::Package.find(789).build_infos.map(&:pipeline_id)
=> [368, 369, nil]

The pipeline_id in the last build info object has been nullified ✅

🍵 How to set up and validate locally

In a rails console:

# creates a package
def fixture_file_upload(*args, **kwargs)
  Rack::Test::UploadedFile.new(*args, **kwargs)
end

# note the package id here
pkg = FactoryBot.create(:npm_package, project: Project.first) # execute until it succeeds

# creates 5 pipelines
Packages::BuildInfo.create!(package_id: pkg.id, pipeline_id: FactoryBot.create(:ci_pipeline).id) # execute until it succeeds
Packages::BuildInfo.create!(package_id: pkg.id, pipeline_id: FactoryBot.create(:ci_pipeline).id) # execute until it succeeds
Packages::BuildInfo.create!(package_id: pkg.id, pipeline_id: FactoryBot.create(:ci_pipeline).id) # execute until it succeeds

# check the build infos
pkg.reload.build_infos.map(&:pipeline_id)
=> [370, 371, 372]

# destroy a pipeline
Ci::Pipeline.find(372).destroy!

# Wait for the cleanup job to run or run it manually
LooseForeignKeys::CleanupWorker.new.perform

# check the build infos again
pkg.build_infos.reload.map(&:pipeline_id)
=> [370, 371, nil]

🚀 MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by David Fernandez

Merge request reports