Nullify merge_request_metrics pipeline_id on pipeline deletion
What does this MR do and why?
Before, when a pipeline is deleted, the associated merge_request_metrics
record will be deleted as well. This is a destructive thing as merge_request_metrics
include other data not related to a pipeline.
To fix this issue, instead of deleting the association record, we nullify the pipeline_id
on merge_request_metrics
instead. When this happens, related build data (latest_build_started_at
and latest_build_finished_at
) will be nullified as well. This is to ensure that when those data are requested, it'll still behave the same way as before.
How to set up and validate locally
Numbered steps to set up and validate the change are strongly suggested.
- Run migration.
- Create a MR with pipeline.
- Let the pipeline run.
- Merge the MR.
- Delete the pipeline.
- Wait for 2 minutes (this is to ensure the job to clean up loose foreign keys has already ran).
- Make a request to
api/v4/projects/:project_id/merge_requests/:merge_request_iid
and check ifmerged_at
is not null.
MR acceptance checklist
This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.
-
I have evaluated the MR acceptance checklist for this MR.
Merge request reports
Activity
changed milestone to %14.10
assigned to @patrickbajao
Reviewer roulette
Changes that require review have been detected!
Please refer to the table below for assigning reviewers and maintainers suggested by Danger in the specified category:
Category Reviewer Maintainer backend Dominic Bauer ( @bauerdominic
) (UTC+2, 6 hours behind@patrickbajao
)Etienne Baqué ( @ebaque
) (UTC+2, 6 hours behind@patrickbajao
)database Matthias Käppler ( @mkaeppler
) (UTC+2, 6 hours behind@patrickbajao
)Mayra Cabrera ( @mayra-cabrera
) (UTC-5, 13 hours behind@patrickbajao
)~migration No reviewer available No maintainer available To spread load more evenly across eligible reviewers, Danger has picked a candidate for each review slot, based on their timezone. Feel free to override these selections if you think someone else would be better-suited or use the GitLab Review Workload Dashboard to find other available reviewers.
To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines. Please consider assigning a reviewer or maintainer who is a domain expert in the area of the merge request.
Once you've decided who will review this merge request, assign them as a reviewer! Danger does not automatically notify them for you.
Generated by
Dangeradded 1 commit
- 35c1e7e8 - Nullify merge_request_metrics pipeline_id on pipeline deletion
Database migrations
Migrations included in this change have been executed on gitlab.com data for testing purposes. For details, please see the migration testing pipeline (limited access).
Migration Type Total runtime Result DB size change 20220412060931 - AddNullifyBuildDataTriggerOnMergeRequestMetrics Regular 1.6 s +8.00 KiB Runtime Histogram for all migrations
Query Runtime Count 0 seconds - 0.01 seconds 0 0.01 seconds - 0.1 seconds 2 0.1 seconds - 1 second 0 1 second - 5 minutes 0 5 minutes + 0 Migration: 20220412060931 - AddNullifyBuildDataTriggerOnMergeRequestMetrics
- Type: Regular
- Duration: 1.6 s
- Database size change: +8.00 KiB
Query Calls Total Time Max Time Mean Time Rows CREATE OR REPLACE FUNCTION nullify_merge_request_metrics_build_data() RETURNS TRIGGER AS $$ BEGIN IF (OLD.pipeline_id IS NOT NULL) AND (NEW.pipeline_id IS NULL) THEN NEW.latest_build_started_at = NULL; NEW.latest_build_finished_at = NULL; END IF; RETURN NEW; END $$ LANGUAGE PLPGSQL
/*application:test,db_config_name:main*/1 30.4 ms 30.4 ms 30.4 ms 0 CREATE TRIGGER nullify_merge_request_metrics_build_data_on_update BEFORE
UPDATE ON merge_request_metrics FOR EACH ROW EXECUTE FUNCTION nullify_merge_request_metrics_build_data()
/*application:test,db_config_name:main*/1 21.4 ms 21.4 ms 21.4 ms 0 Histogram for AddNullifyBuildDataTriggerOnMergeRequestMetrics
Query Runtime Count 0 seconds - 0.01 seconds 0 0.01 seconds - 0.1 seconds 2 0.1 seconds - 1 second 0 1 second - 5 minutes 0 5 minutes + 0
Other migrations pending on GitLab.com
Migration Type Total runtime Result DB size change Clone Details
Clone ID Clone Created At Clone Data Timestamp Expected Removal Time database-testing-1144497
2022-04-13T02:05:28Z 2022-04-12T14:13:53Z 2022-04-13 14:07:14 +0000 Artifacts
Brought to you by gitlab-org/database-team/gitlab-com-database-testing. Epic
- Resolved by Huzaifa Iftikhar
- A deleted user
added database-testing-automation label
- Resolved by Patrick Bair
Hi @ahegyi, this is the solution I mentioned in #26911 (comment 895582245). What are your thoughts about it?
I see that we have helpers in the codebase for adding DB triggers and functions so I used them. I don't see any guidelines about them though in the docs (I may be just missing it). Are there things that I need to consider (e.g. performance concerns) regarding triggers?
Sorry for directly asking you and skipping database reviewer. But since you were already involved in the discussion on the related issue, I feel it'll be easier this way.
P.S. This is still in draft as I'm still testing this further.
changed milestone to %15.0
requested review from @ahegyi
mentioned in issue #26911 (closed)
added 1243 commits
-
35c1e7e8...8dbf42f6 - 1242 commits from branch
master
- ace456fb - Nullify merge_request_metrics pipeline_id on pipeline deletion
-
35c1e7e8...8dbf42f6 - 1242 commits from branch
Suggested Reviewers (beta)
This is an experimental ML-based code reviewer recommendation system created by ~"group::applied ml".
The individuals below may be good candidates to participate in the review based on various factors.
After you review all recommendations, please assign reviewers manually, as this is not done automatically.
You can use slash commands in comments to quickly assign
/assign_reviewer @user1
.Reviewers @rkadam3
,@agarciatesares
,@mikolaj_wawrzyniak
,@lmejia2
,@ali-gitlab
If you do not believe these recommendations are useful or if you do not want to use any of the suggestions, please apply the label Bad Suggested Reviewer. You can also provide feedback for this feature on this issue:
https://gitlab.com/gitlab-org/gitlab/-/issues/357923
.Automatically generated by Suggested Reviewers Bot
Edited by GitLab Reviewer-Recommender Botadded database databasereview pending labels
- Resolved by Etienne Baqué
Hi @huzaifaiftikhar1, can you please review this from backend perspective?
requested review from @huzaifaiftikhar1
added databasereviewed label and removed databasereview pending label
@ahegyi
, thanks for approving this merge request.This is the first time the merge request is approved. To ensure full test coverage, a new pipeline has been started.
For more info, please refer to the following links:
requested review from @pbair
added databaseapproved label and removed databasereviewed label
removed review request for @pbair
requested review from @ebaque and removed review request for @huzaifaiftikhar1
mentioned in issue gitlab-com/www-gitlab-com#13148 (closed)
added 366 commits
-
ace456fb...4a2daee2 - 365 commits from branch
master
- 4cbd01b1 - Nullify merge_request_metrics pipeline_id on pipeline deletion
-
ace456fb...4a2daee2 - 365 commits from branch
mentioned in commit 55fb1d31
added workflowstaging-canary label
added workflowcanary label and removed workflowstaging-canary label
added workflowstaging label and removed workflowcanary label
added workflowproduction label and removed workflowstaging label
added releasedcandidate label
mentioned in merge request kubitus-project/kubitus-installer!1040 (merged)
mentioned in issue #391920