Skip to content

Move merge request build failure todo creation to an async worker [RUN ALL RSPEC] [RUN AS-IF-FOSS]

Allison Browne requested to merge 324369-api-jobs-request-the-assign_runner-likely-has-n-1-problem into master

What does this MR do?

This moves todo creation on merge request pipeline failure from being synchronously executed by the jobs/request endpoint (which already executes many queries) to asynchronous execution triggered by the BuildFinishedWorker.

This Worker is idempotent, and I checked that the service can be performed multiple times without creating multiple todos if one already exists for that mr/build.

Note: when producing a todo manually, todos are only created for the head_pipeline failure and not subsequent pipeline runs for the same commit.

Screenshots (strongly suggested)

Screen_Shot_2021-03-26_at_11.07.44_AM

Database

queries executed when todo is not created (todo already exists)

  CommitStatus Load (7.8ms)  SELECT "ci_builds".* FROM "ci_builds" INNER JOIN "ci_pipelines" ON "ci_pipelines"."id" = "ci_builds"."commit_id" INNER JOIN "projects" ON "projects"."id" = "ci_builds"."project_id" WHERE "ci_builds"."id" = 1273 LIMIT 1 /*application:console,line:/app/workers/ci/merge_requests/add_todo_when_build_fails_worker.rb:13:in `perform'*/
  Project Load (1.1ms)  SELECT "projects".* FROM "projects" WHERE "projects"."id" = 23 LIMIT 1 /*application:console,line:/app/workers/ci/merge_requests/add_todo_when_build_fails_worker.rb:15:in `perform'*/
  Ci::Pipeline Load (0.8ms)  SELECT "ci_pipelines".* FROM "ci_pipelines" WHERE "ci_pipelines"."id" = 225 LIMIT 1 /*application:console,line:/app/services/merge_requests/add_todo_when_build_fails_service.rb:10:in `execute'*/
  MergeRequest Load (2.0ms)  SELECT "merge_requests".* FROM "merge_requests" WHERE "merge_requests"."id" = 74 AND ("merge_requests"."state_id" IN (1)) /*application:console,line:/app/services/merge_requests/base_service.rb:166:in `pipeline_merge_requests'*/
  User Load (1.9ms)  SELECT "users".* FROM "users" WHERE "users"."id" = 1 LIMIT 1 /*application:console,line:/app/models/merge_request.rb:683:in `merge_participants'*/
  Project Load (0.6ms)  SELECT "projects".* FROM "projects" WHERE "projects"."id" = 23 LIMIT 1 /*application:console,line:/app/policies/issuable_policy.rb:4:in `block in <class:IssuablePolicy>'*/
  ProjectFeature Load (0.5ms)  SELECT "project_features".* FROM "project_features" WHERE "project_features"."project_id" = 23 LIMIT 1 /*application:console,line:/app/policies/project_policy.rb:711:in `access_allowed_to?'*/
  Group Load (1.5ms)  SELECT "namespaces".* FROM "namespaces" WHERE "namespaces"."type" = 'Group' AND "namespaces"."id" = 1 AND "namespaces"."type" = 'Group' LIMIT 1 /*application:console,line:/ee/app/policies/ee/project_policy.rb:342:in `block (2 levels) in <module:ProjectPolicy>'*/
  Todo Exists? (0.9ms)  SELECT 1 AS one FROM "todos" WHERE "todos"."user_id" = 1 AND ("todos"."state" IN ('pending')) AND "todos"."project_id" = 23 AND "todos"."target_id" = 74 AND "todos"."target_type" = 'MergeRequest' LIMIT 1 /*application:console,line:/app/services/todo_service.rb:224:in `block in create_todos'*/
  Feature::FlipperGate Load (0.3ms)  SELECT "feature_gates".* FROM "feature_gates" WHERE "feature_gates"."feature_key" = 'multiple_todos' /*application:console,line:/lib/feature.rb:84:in `enabled?'*/
  Route Load (0.8ms)  SELECT "routes".* FROM "routes" WHERE "routes"."source_id" = 23 AND "routes"."source_type" = 'Project' LIMIT 1 /*application:console,line:/app/models/concerns/routable.rb:103:in `full_path'*/

when todo is created:

  CommitStatus Load (7.2ms)  SELECT "ci_builds".* FROM "ci_builds" INNER JOIN "ci_pipelines" ON "ci_pipelines"."id" = "ci_builds"."commit_id" INNER JOIN "projects" ON "projects"."id" = "ci_builds"."project_id" WHERE "ci_builds"."id" = 1273 LIMIT 1 /*application:console,line:/app/workers/ci/merge_requests/add_todo_when_build_fails_worker.rb:13:in `perform'*/
  Project Load (0.8ms)  SELECT "projects".* FROM "projects" WHERE "projects"."id" = 23 LIMIT 1 /*application:console,line:/app/workers/ci/merge_requests/add_todo_when_build_fails_worker.rb:15:in `perform'*/
  Ci::Pipeline Load (0.6ms)  SELECT "ci_pipelines".* FROM "ci_pipelines" WHERE "ci_pipelines"."id" = 225 LIMIT 1 /*application:console,line:/app/services/merge_requests/add_todo_when_build_fails_service.rb:10:in `execute'*/
  MergeRequest Load (2.1ms)  SELECT "merge_requests".* FROM "merge_requests" WHERE "merge_requests"."id" = 74 AND ("merge_requests"."state_id" IN (1)) /*application:console,line:/app/services/merge_requests/base_service.rb:166:in `pipeline_merge_requests'*/
  User Load (2.1ms)  SELECT "users".* FROM "users" WHERE "users"."id" = 1 LIMIT 1 /*application:console,line:/app/models/merge_request.rb:683:in `merge_participants'*/
  Project Load (0.7ms)  SELECT "projects".* FROM "projects" WHERE "projects"."id" = 23 LIMIT 1 /*application:console,line:/app/policies/issuable_policy.rb:4:in `block in <class:IssuablePolicy>'*/
  ProjectFeature Load (0.6ms)  SELECT "project_features".* FROM "project_features" WHERE "project_features"."project_id" = 23 LIMIT 1 /*application:console,line:/app/policies/project_policy.rb:711:in `access_allowed_to?'*/
  Group Load (1.6ms)  SELECT "namespaces".* FROM "namespaces" WHERE "namespaces"."type" = 'Group' AND "namespaces"."id" = 1 AND "namespaces"."type" = 'Group' LIMIT 1 /*application:console,line:/ee/app/policies/ee/project_policy.rb:342:in `block (2 levels) in <module:ProjectPolicy>'*/
  Todo Exists? (0.9ms)  SELECT 1 AS one FROM "todos" WHERE "todos"."user_id" = 1 AND ("todos"."state" IN ('pending')) AND "todos"."project_id" = 23 AND "todos"."target_id" = 74 AND "todos"."target_type" = 'MergeRequest' LIMIT 1 /*application:console,line:/app/services/todo_service.rb:224:in `block in create_todos'*/
   (0.1ms)  BEGIN /*application:console,line:/lib/gitlab/database.rb:342:in `block in transaction'*/
  User Load (0.4ms)  SELECT "users".* FROM "users" WHERE "users"."id" = 1 LIMIT 1 /*application:console,line:/lib/gitlab/database.rb:342:in `block in transaction'*/
  User Load (0.4ms)  SELECT "users".* FROM "users" WHERE "users"."id" = 1 LIMIT 1 /*application:console,line:/lib/gitlab/database.rb:342:in `block in transaction'*/
  Project Load (0.5ms)  SELECT "projects".* FROM "projects" WHERE "projects"."id" = 23 LIMIT 1 /*application:console,line:/lib/gitlab/database.rb:342:in `block in transaction'*/
  Todo Create (1.2ms)  INSERT INTO "todos" ("user_id", "project_id", "target_id", "target_type", "author_id", "action", "state", "created_at", "updated_at") VALUES (1, 23, 74, 'MergeRequest', 1, 3, 'pending', '2021-03-26 21:00:17.332565', '2021-03-26 21:00:17.332565') RETURNING "id" /*application:console,line:/lib/gitlab/database.rb:342:in `block in transaction'*/
  MergeRequest Load (0.4ms)  SELECT "merge_requests".* FROM "merge_requests" WHERE "merge_requests"."id" = 74 LIMIT 1 /*application:console,line:/app/models/todo.rb:210:in `target'*/
  Project Load (0.5ms)  SELECT "projects".* FROM "projects" WHERE "projects"."id" = 23 /*application:console,line:/app/models/todo.rb:210:in `target'*/
  Route Load (0.6ms)  SELECT "routes".* FROM "routes" WHERE "routes"."source_type" = 'Project' AND "routes"."source_id" = 23 /*application:console,line:/app/models/todo.rb:210:in `target'*/
  Namespace Load (0.5ms)  SELECT "namespaces".* FROM "namespaces" WHERE "namespaces"."id" = 1 /*application:console,line:/app/models/todo.rb:210:in `target'*/
  Route Load (0.3ms)  SELECT "routes".* FROM "routes" WHERE "routes"."source_type" = 'Namespace' AND "routes"."source_id" = 1 /*application:console,line:/app/models/todo.rb:210:in `target'*/
  Project Load (0.4ms)  SELECT "projects".* FROM "projects" WHERE "projects"."id" = 23 /*application:console,line:/app/models/todo.rb:210:in `target'*/
  Route Load (0.2ms)  SELECT "routes".* FROM "routes" WHERE "routes"."source_type" = 'Project' AND "routes"."source_id" = 23 /*application:console,line:/app/models/todo.rb:210:in `target'*/
  Namespace Load (0.3ms)  SELECT "namespaces".* FROM "namespaces" WHERE "namespaces"."id" = 1 /*application:console,line:/app/models/todo.rb:210:in `target'*/
  Route Load (0.2ms)  SELECT "routes".* FROM "routes" WHERE "routes"."source_type" = 'Namespace' AND "routes"."source_id" = 1 /*application:console,line:/app/models/todo.rb:210:in `target'*/
  MergeRequestAssignee Load (0.5ms)  SELECT "merge_request_assignees".* FROM "merge_request_assignees" WHERE "merge_request_assignees"."merge_request_id" = 74 /*application:console,line:/app/models/todo.rb:210:in `target'*/
  User Load (0.4ms)  SELECT "users".* FROM "users" WHERE "users"."id" = 1 /*application:console,line:/app/models/todo.rb:210:in `target'*/
  Note Load (2.0ms)  SELECT "notes".* FROM "notes" WHERE "notes"."type" IN ('DiffNote', 'DiscussionNote') AND "notes"."noteable_type" IN ('MergeRequest', 'DesignManagement::Design') AND "notes"."system" = FALSE AND "notes"."resolved_at" IS NULL AND "notes"."noteable_id" = 74 /*application:console,line:/app/models/todo.rb:210:in `target'*/
  LabelLink Load (0.5ms)  SELECT "label_links".* FROM "label_links" WHERE "label_links"."target_type" = 'MergeRequest' AND "label_links"."target_id" = 74 /*application:console,line:/app/models/todo.rb:210:in `target'*/
  Timelog Load (0.6ms)  SELECT "timelogs".* FROM "timelogs" WHERE "timelogs"."merge_request_id" = 74 /*application:console,line:/app/models/todo.rb:210:in `target'*/
  MergeRequestDiff Load (0.6ms)  SELECT "merge_request_diffs".* FROM "merge_request_diffs" WHERE "merge_request_diffs"."id" = 87 /*application:console,line:/app/models/todo.rb:210:in `target'*/
  MergeRequestReviewer Load (0.4ms)  SELECT "merge_request_reviewers".* FROM "merge_request_reviewers" WHERE "merge_request_reviewers"."merge_request_id" = 74 /*application:console,line:/app/models/todo.rb:210:in `target'*/
  ProjectFeature Load (0.2ms)  SELECT "project_features".* FROM "project_features" WHERE "project_features"."project_id" = 23 /*application:console,line:/app/models/todo.rb:210:in `target'*/
  MergeRequest::Metrics Load (1.0ms)  SELECT "merge_request_metrics".* FROM "merge_request_metrics" WHERE "merge_request_metrics"."merge_request_id" = 74 /*application:console,line:/app/models/todo.rb:210:in `target'*/
  MergeRequestBlock Load (0.4ms)  SELECT "merge_request_blocks".* FROM "merge_request_blocks" WHERE "merge_request_blocks"."blocked_merge_request_id" = 74 /*application:console,line:/app/models/todo.rb:210:in `target'*/
   (0.5ms)  COMMIT /*application:console,line:/lib/gitlab/database.rb:342:in `block in transaction'*/
   (0.5ms)  SELECT COUNT(*) FROM "todos" WHERE "todos"."user_id" = 1 AND ("todos"."state" IN ('done')) /*application:console,line:/app/models/user.rb:1634:in `block in todos_done_count'*/
   (0.3ms)  SELECT COUNT(*) FROM "todos" WHERE "todos"."user_id" = 1 AND ("todos"."state" IN ('pending')) /*application:console,line:/app/models/user.rb:1640:in `block in todos_pending_count'*/
  Route Load (0.5ms)  SELECT "routes".* FROM "routes" WHERE "routes"."source_id" = 23 AND "routes"."source_type" = 'Project' LIMIT 1 /*application:console,line:/app/models/concerns/routable.rb:103:in `full_path'*/

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • Label as security and @ mention @gitlab-com/gl-security/appsec
  • The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • Security reports checked/validated by a reviewer from the AppSec team

Related to #324369 (closed)

Edited by 🤖 GitLab Bot 🤖

Merge request reports