Skip to content
Snippets Groups Projects

Create iid sequence for ci_pipelines with new projects

Merged Marius Bobin requested to merge 423421-fix-race-cond into master
All threads resolved!

What does this MR do and why?

Create iid sequence for ci_pipelines with new projects

The initialization of the sequence is not atomic and due to race conditions some pipelines could reinitialize multiple times, resulting in attempts to insert rows with duplicate values. This fix moves the initialization from the first pipeline creation into the project creation, so the sequence will be already initialized when the first pipelines are created.

Changelog: fixed

Screenshots or screen recordings

Before

When the first pipeline is created, we have an update followed by an insert to generate the sequence. On edge cases, the first pipeline will not be one pipeline, but many created concurrently from Sidekiq. This can initialize the sequence more than once.

[11] pry(main)> FactoryBot.create(:ci_pipeline, project: project)
  Update InternalId (0.7ms)  UPDATE "internal_ids" SET "last_value" = ("internal_ids"."last_value" + 1) WHERE "internal_ids"."project_id" = 231 AND "internal_ids"."usage" = 5 RETURNING "last_value" /*application:console,db_config_name:main,console_hostname:rocket-sled.local,console_username:marius,line:/app/models/internal_id.rb:164:in `update_record!'*/
  Ci::Pipeline Maximum (2.5ms)  SELECT MAX("ci_pipelines"."iid") FROM "ci_pipelines" WHERE "ci_pipelines"."project_id" = 231 /*application:console,db_config_name:ci,console_hostname:rocket-sled.local,console_username:marius,line:/app/models/ci/pipeline.rb:66:in `block in <class:Pipeline>'*/
  Ci::Pipeline Count (0.2ms)  SELECT COUNT(*) FROM "ci_pipelines" WHERE "ci_pipelines"."project_id" = 231 /*application:console,db_config_name:ci,console_hostname:rocket-sled.local,console_username:marius,line:/app/models/ci/pipeline.rb:66:in `block in <class:Pipeline>'*/
  InternalId Insert (2.0ms)  INSERT INTO "internal_ids" ("project_id","namespace_id","usage","last_value") VALUES (231, NULL, 5, 1) ON CONFLICT  DO NOTHING RETURNING "id" /*application:console,db_config_name:main,console_hostname:rocket-sled.local,console_username:marius,line:/app/models/internal_id.rb:178:in `create_record!'*/
  Ci::Ref Load (0.5ms)  SELECT "ci_refs".* FROM "ci_refs" WHERE "ci_refs"."project_id" = 231 AND "ci_refs"."ref_path" = 'refs/heads/master' LIMIT 1 /*application:console,db_config_name:ci,console_hostname:rocket-sled.local,console_username:marius,line:/app/models/application_record.rb:79:in `safe_find_or_create_by'*/
  TRANSACTION (0.0ms)  BEGIN /*application:console,db_config_name:ci,console_hostname:rocket-sled.local,console_username:marius,line:/lib/gitlab/database/schema_cache_with_renamed_table.rb:21:in `primary_keys'*/
  Ci::Ref Create (0.4ms)  INSERT INTO "ci_refs" ("project_id", "ref_path", "lock_version") VALUES (231, 'refs/heads/master', 0) RETURNING "id" /*application:console,db_config_name:ci,console_hostname:rocket-sled.local,console_username:marius,line:/app/models/application_record.rb:87:in `block in safe_find_or_create_by'*/
  TRANSACTION (0.1ms)  COMMIT /*application:console,db_config_name:ci,console_hostname:rocket-sled.local,console_username:marius,line:/lib/gitlab/database.rb:423:in `commit'*/
  TRANSACTION (0.1ms)  BEGIN /*application:console,db_config_name:ci,console_hostname:rocket-sled.local,console_username:marius,line:<internal:kernel>:90:in `tap'*/
  Ci::Pipeline Create (1.7ms)  INSERT INTO "ci_pipelines" ("ref", "sha", "created_at", "updated_at", "project_id", "status", "source", "protected", "iid", "ci_ref_id", "partition_id", "lock_version") VALUES ('master', 'b83d6e391c22777fca1ed3012fce84f633d7fed0', '2023-08-30 13:58:30.060043', '2023-08-30 13:58:30.060043', 231, 'pending', 1, FALSE, 1, 244, 100, 0) RETURNING "id" /*application:console,db_config_name:ci,console_hostname:rocket-sled.local,console_username:marius,line:<internal:kernel>:90:in `tap'*/
  TRANSACTION (0.1ms)  COMMIT 

After

The internal_ids record is created right after the project gets created, so the first pipelines will get the iid value from the increment call, without needing to initialize the sequence:

[18] pry(main)> FactoryBot.create(:ci_pipeline, project: project)
  Update InternalId (0.7ms)  UPDATE "internal_ids" SET "last_value" = ("internal_ids"."last_value" + 1) WHERE "internal_ids"."project_id" = 230 AND "internal_ids"."usage" = 5 RETURNING "last_value" /*application:console,db_config_name:main,console_hostname:rocket-sled.local,console_username:marius,line:/app/models/internal_id.rb:166:in `update_record!'*/
  Ci::Ref Load (0.4ms)  SELECT "ci_refs".* FROM "ci_refs" WHERE "ci_refs"."project_id" = 230 AND "ci_refs"."ref_path" = 'refs/heads/master' LIMIT 1 /*application:console,db_config_name:ci,console_hostname:rocket-sled.local,console_username:marius,line:/app/models/application_record.rb:79:in `safe_find_or_create_by'*/
  TRANSACTION (0.2ms)  BEGIN /*application:console,db_config_name:ci,console_hostname:rocket-sled.local,console_username:marius,line:/app/models/application_record.rb:87:in `block in safe_find_or_create_by'*/
  Ci::Ref Create (0.5ms)  INSERT INTO "ci_refs" ("project_id", "ref_path", "lock_version") VALUES (230, 'refs/heads/master', 0) RETURNING "id" /*application:console,db_config_name:ci,console_hostname:rocket-sled.local,console_username:marius,line:/app/models/application_record.rb:87:in `block in safe_find_or_create_by'*/
  TRANSACTION (0.2ms)  COMMIT /*application:console,db_config_name:ci,console_hostname:rocket-sled.local,console_username:marius,line:/lib/gitlab/database.rb:423:in `commit'*/
  TRANSACTION (0.1ms)  BEGIN /*application:console,db_config_name:ci,console_hostname:rocket-sled.local,console_username:marius,line:<internal:kernel>:90:in `tap'*/
  Ci::Pipeline Create (1.3ms)  INSERT INTO "ci_pipelines" ("ref", "sha", "created_at", "updated_at", "project_id", "status", "source", "protected", "iid", "ci_ref_id", "partition_id", "lock_version") VALUES ('master', 'b83d6e391c22777fca1ed3012fce84f633d7fed0', '2023-08-30 13:52:14.803045', '2023-08-30 13:52:14.803045', 230, 'pending', 1, FALSE, 1, 243, 100, 0) RETURNING "id" /*application:console,db_config_name:ci,console_hostname:rocket-sled.local,console_username:marius,line:<internal:kernel>:90:in `tap'*/
  TRANSACTION (0.2ms)  COMMIT

How to set up and validate locally

Numbered steps to set up and validate the change are strongly suggested.

MR acceptance checklist

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

Related to #423421 (closed)

Edited by Marius Bobin

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Marius Bobin changed the description

    changed the description

  • Marius Bobin added 1 commit

    added 1 commit

    • 07fdcf4b - Create iid sequence for ci_pipelines with new projects

    Compare with previous version

  • Marius Bobin requested review from @reprazent

    requested review from @reprazent

  • Bob Van Landuyt removed review request for @reprazent

    removed review request for @reprazent

  • Contributor

    Allure report

    allure-report-publisher generated test report!

    e2e-test-on-gdk: :exclamation: test report for a9da1076

    expand test summary
    +------------------------------------------------------------------+
    |                          suites summary                          |
    +-------------+--------+--------+---------+-------+-------+--------+
    |             | passed | failed | skipped | flaky | total | result |
    +-------------+--------+--------+---------+-------+-------+--------+
    | Data Stores | 20     | 0      | 0       | 0     | 20    | ✅     |
    | Plan        | 47     | 0      | 0       | 0     | 47    | ✅     |
    | Create      | 38     | 0      | 0       | 2     | 38    | ❗     |
    | Govern      | 34     | 0      | 0       | 0     | 34    | ✅     |
    | Manage      | 12     | 0      | 1       | 0     | 13    | ✅     |
    | Verify      | 8      | 0      | 0       | 0     | 8     | ✅     |
    +-------------+--------+--------+---------+-------+-------+--------+
    | Total       | 159    | 0      | 1       | 2     | 160   | ❗     |
    +-------------+--------+--------+---------+-------+-------+--------+
  • Marius Bobin added 2 commits

    added 2 commits

    Compare with previous version

  • Bob Van Landuyt approved this merge request

    approved this merge request

  • Laura Montemayor approved this merge request

    approved this merge request

  • Marius Bobin added 1 commit

    added 1 commit

    • a9da1076 - Apply 1 suggestion(s) to 1 file(s)

    Compare with previous version

  • Laura Montemayor resolved all threads

    resolved all threads

  • Laura Montemayor approved this merge request

    approved this merge request

  • Laura Montemayor enabled an automatic merge when the pipeline for a0959300 succeeds

    enabled an automatic merge when the pipeline for a0959300 succeeds

  • Laura Montemayor mentioned in commit 2c042ff8

    mentioned in commit 2c042ff8

  • added workflowstaging label and removed workflowcanary label

  • Marius Bobin mentioned in merge request !130835 (merged)

    mentioned in merge request !130835 (merged)

  • Marius Bobin mentioned in merge request !130836 (merged)

    mentioned in merge request !130836 (merged)

  • Daphne Kua mentioned in issue #519457

    mentioned in issue #519457

  • Please register or sign in to reply
    Loading