From 34a5f77e770765f278ade00a33ef846e2e1ce3d3 Mon Sep 17 00:00:00 2001 From: Toon Claes Date: Fri, 19 Jul 2019 17:33:48 +0000 Subject: Document database review process See https://gitlab.com/gitlab-com/gl-infra/infrastructure/issues/6069 --- .gitlab/CODEOWNERS | 6 +- .../merge_request_templates/Database changes.md | 13 +-- danger/database/Dangerfile | 42 +++++---- danger/roulette/Dangerfile | 15 +-- doc/development/README.md | 1 + doc/development/code_review.md | 1 + .../contributing/merge_request_workflow.md | 3 +- doc/development/database_review.md | 101 +++++++++++++++++++++ lib/gitlab/danger/helper.rb | 26 ++++++ spec/lib/gitlab/danger/helper_spec.rb | 30 ++++++ 10 files changed, 196 insertions(+), 42 deletions(-) create mode 100644 doc/development/database_review.md diff --git a/.gitlab/CODEOWNERS b/.gitlab/CODEOWNERS index b865b212ac0..13c8b4a8458 100644 --- a/.gitlab/CODEOWNERS +++ b/.gitlab/CODEOWNERS @@ -9,8 +9,12 @@ app/assets/ @ClemMakesApps @fatihacet @filipa @iamphill @mikegreiling @timzallmann @kushalpandya @pslaughter *.scss @annabeldunstone @ClemMakesApps @fatihacet @filipa @iamphill @mikegreiling @timzallmann @kushalpandya @pslaughter -# Someone from the database team should review changes in `db/` +# Maintainers from the Database team should review changes in `db/` db/ @abrandl @NikolayS +lib/gitlab/background_migration/ @abrandl @NikolayS +lib/gitlab/database/ @abrandl @NikolayS +lib/gitlab/sql/ @abrandl @NikolayS +/ee/db/ @abrandl @NikolayS # Feature specific owners /ee/lib/gitlab/code_owners/ @reprazent diff --git a/.gitlab/merge_request_templates/Database changes.md b/.gitlab/merge_request_templates/Database changes.md index 3f457174492..2077997a0cb 100644 --- a/.gitlab/merge_request_templates/Database changes.md +++ b/.gitlab/merge_request_templates/Database changes.md @@ -39,20 +39,11 @@ When adding tables: - [ ] Ordered columns based on the [Ordering Table Columns](https://docs.gitlab.com/ee/development/ordering_table_columns.html#ordering-table-columns) guidelines - [ ] Added foreign keys to any columns pointing to data in other tables -- [ ] Added indexes for fields that are used in statements such as WHERE, ORDER BY, GROUP BY, and JOINs +- [ ] Added indexes for fields that are used in statements such as `WHERE`, `ORDER BY`, `GROUP BY`, and `JOIN`s When removing columns, tables, indexes or other structures: - [ ] Removed these in a post-deployment migration - [ ] Made sure the application no longer uses (or ignores) these structures -## General checklist - -- [ ] [Changelog entry](https://docs.gitlab.com/ee/development/changelog.html) added, if necessary -- [ ] [Documentation created/updated](https://docs.gitlab.com/ee/development/documentation/) -- [ ] [Tests added for this feature/bug](https://docs.gitlab.com/ee/development/testing_guide/index.html) -- [ ] Conforms to the [code review guidelines](https://docs.gitlab.com/ee/development/code_review.html) -- [ ] Conforms to the [merge request performance guidelines](https://docs.gitlab.com/ee/development/merge_request_performance_guidelines.html) -- [ ] Conforms to the [style guides](https://docs.gitlab.com/ee/development/contributing/style_guides.html) - -/label ~database +/label ~database ~"database::review pending" diff --git a/danger/database/Dangerfile b/danger/database/Dangerfile index 083e95b8da7..3550cb7eabf 100644 --- a/danger/database/Dangerfile +++ b/danger/database/Dangerfile @@ -8,6 +8,21 @@ updated too (unless the migration isn't changing the DB schema and isn't the most recent one). MSG +DB_MESSAGE = <<~MSG +This merge request requires a database review. To make sure these +changes are reviewed, take the following steps: + +1. Ensure the merge request has ~database and ~"database::review pending" labels. + If the merge request modifies database files, Danger will do this for you. +1. Use the [Database changes checklist](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/.gitlab/merge_request_templates/Database%20changes.md) + template or add the appropriate items to the MR description. +1. Assign and mention the database reviewer suggested by Reviewer Roulette. +MSG + +DB_FILES_MESSAGE = <<~MSG +The following files require a review from the Database team: +MSG + non_geo_db_schema_updated = !git.modified_files.grep(%r{\Adb/schema\.rb}).empty? geo_db_schema_updated = !git.modified_files.grep(%r{\Aee/db/geo/schema\.rb}).empty? @@ -24,27 +39,18 @@ end db_paths_to_review = helper.changes_by_category[:database] -unless db_paths_to_review.empty? +if gitlab.mr_labels.include?('database') || db_paths_to_review.any? message 'This merge request adds or changes files that require a ' \ - 'review from the Database team.' - - markdown(<<~MARKDOWN.strip) -## Database Review - -The following files require a review from the Database team: - -* #{db_paths_to_review.map { |path| "`#{path}`" }.join("\n* ")} + 'review from the [Database team](https://gitlab.com/groups/gl-database/-/group_members).' -To make sure these changes are reviewed, take the following steps: + markdown(DB_MESSAGE) + markdown(DB_FILES_MESSAGE + helper.markdown_list(db_paths_to_review)) if db_paths_to_review.any? -1. Edit your merge request, and add `gl-database` to the list of Group - approvers. -1. Mention `@gl-database` in a separate comment, and explain what needs to be - reviewed by the team. Please don't mention the team until your changes are - ready for review. - MARKDOWN + database_labels = helper.missing_database_labels(gitlab.mr_labels) - unless gitlab.mr_labels.include?('database') - warn 'This merge request is missing the ~database label.' + if database_labels.any? + gitlab.api.update_merge_request(gitlab.mr_json['project_id'], + gitlab.mr_json['iid'], + labels: (gitlab.mr_labels + database_labels).join(',')) end end diff --git a/danger/roulette/Dangerfile b/danger/roulette/Dangerfile index 6718e218233..19a5076778c 100644 --- a/danger/roulette/Dangerfile +++ b/danger/roulette/Dangerfile @@ -55,22 +55,15 @@ def spin_for_category(team, project, category, branch_name) "| #{helper.label_for_category(category)} | #{reviewer&.markdown_name || NO_REVIEWER} | #{maintainer&.markdown_name || NO_MAINTAINER} |" end -def build_list(items) - list = items.map { |filename| "* `#{filename}`" }.join("\n") - - if items.size > 10 - "\n
\n\n#{list}\n\n
" - else - list - end -end - changes = helper.changes_by_category # Ignore any files that are known but uncategorized. Prompt for any unknown files changes.delete(:none) categories = changes.keys - [:unknown] +# Ensure to spin for database reviewer/maintainer when ~database is applied (e.g. to review SQL queries) +categories << :database if gitlab.mr_labels.include?('database') && !categories.include?(:database) + # Single codebase MRs are reviewed using a slightly different process, so we # disable the review roulette for such MRs. # CSS Clean up MRs are reviewed using a slightly different process, so we @@ -95,5 +88,5 @@ if changes.any? && !gitlab.mr_labels.include?('single codebase') && !gitlab.mr_l markdown(MESSAGE) markdown(CATEGORY_TABLE_HEADER + rows.join("\n")) unless rows.empty? - markdown(UNKNOWN_FILES_MESSAGE + build_list(unknown)) unless unknown.empty? + markdown(UNKNOWN_FILES_MESSAGE + helper.markdown_list(unknown)) unless unknown.empty? end diff --git a/doc/development/README.md b/doc/development/README.md index 4f16473e7e2..ea5d9e10e2c 100644 --- a/doc/development/README.md +++ b/doc/development/README.md @@ -17,6 +17,7 @@ description: 'Learn how to contribute to GitLab.' - [GitLab core team & GitLab Inc. contribution process](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/PROCESS.md) - [Generate a changelog entry with `bin/changelog`](changelog.md) - [Code review guidelines](code_review.md) for reviewing code and having code reviewed +- [Database review guidelines](database_review.md) for reviewing database-related changes and complex SQL queries - [Automatic CE->EE merge](automatic_ce_ee_merge.md) - [Guidelines for implementing Enterprise Edition features](ee_features.md) - [Security process for developers](https://gitlab.com/gitlab-org/release/docs/blob/master/general/security/developer.md#security-releases-critical-non-critical-as-a-developer) diff --git a/doc/development/code_review.md b/doc/development/code_review.md index a9a46c62d38..709cd0c806b 100644 --- a/doc/development/code_review.md +++ b/doc/development/code_review.md @@ -62,6 +62,7 @@ from teams other than your own. **approved by a [backend maintainer](https://about.gitlab.com/handbook/engineering/projects/#gitlab-ce_maintainers_backend)**. 1. If your merge request includes database migrations or changes to expensive queries [^2], it must be **approved by a [database maintainer](https://about.gitlab.com/handbook/engineering/projects/#gitlab-ce_maintainers_database)**. + Read the [database review guidelines](database_review.md) for more details. 1. If your merge request includes frontend changes [^1], it must be **approved by a [frontend maintainer](https://about.gitlab.com/handbook/engineering/projects/#gitlab-ce_maintainers_frontend)**. 1. If your merge request includes UX changes [^1], it must be diff --git a/doc/development/contributing/merge_request_workflow.md b/doc/development/contributing/merge_request_workflow.md index 3325d3e074e..17328762c5b 100644 --- a/doc/development/contributing/merge_request_workflow.md +++ b/doc/development/contributing/merge_request_workflow.md @@ -103,7 +103,8 @@ If you would like quick feedback on your merge request feel free to mention some from the [core team](https://about.gitlab.com/community/core-team/) or one of the [merge request coaches](https://about.gitlab.com/team/). When having your code reviewed and when reviewing merge requests, please keep the [code review guidelines](../code_review.md) -in mind. +in mind. And if your code also makes changes to the database, or does expensive queries, +check the [database review guidelines](../database_review.md). ### Keep it simple diff --git a/doc/development/database_review.md b/doc/development/database_review.md new file mode 100644 index 00000000000..1413c2f69fb --- /dev/null +++ b/doc/development/database_review.md @@ -0,0 +1,101 @@ +# Database Review Guidelines + +This page is specific to database reviews. Please refer to our +[code review guide](code_review.md) for broader advice and best +practices for code review in general. + +## General process + +A database review is required for: + +- Changes that touch the database schema or perform data migrations, + including files in: + - `db/` + - `lib/gitlab/background_migration/` +- Changes to the database tooling, e.g.: + - migration or ActiveRecord helpers in `lib/gitlab/database/` + - load balancing +- Changes that produce SQL queries that are beyond the obvious. It is + generally up to the author of a merge request to decide whether or + not complex queries are being introduced and if they require a + database review. + +A database reviewer is expected to look out for obviously complex +queries in the change and review those closer. If the author does not +point out specific queries for review and there are no obviously +complex queries, it is enough to concentrate on reviewing the +migration only. + +It is preferable to review queries in SQL form and generally accepted +to ask the author to translate any ActiveRecord queries in SQL form +for review. + +### Roles and process + +A Merge Request author's role is to: + +- Decide whether a database review is needed. +- If database review is needed, add the ~database label. +- Use the [database changes](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/.gitlab/merge_request_templates/Database%20changes.md) + merge request template, or include the appropriate items in the MR description. + +A database **reviewer**'s role is to: + +- Perform a first-pass review on the MR and suggest improvements to the author. +- Once satisfied, relabel the MR with ~"database::reviewed", approve it, and + reassign MR to the database **maintainer** suggested by Reviewer + Roulette. + +A database **maintainer**'s role is to: + +- Perform the final database review on the MR. +- Discuss further improvements or other relevant changes with the + database reviewer and the MR author. +- Finally approve the MR and relabel the MR with ~"database::approved" +- Merge the MR if no other approvals are pending or pass it on to + other maintainers as required (frontend, backend, docs). + +### Distributing review workload + +Review workload is distributed using [reviewer roulette](code_review.md#reviewer-roulette) +([example](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/25181#note_147551725)). +The MR author should then co-assign the suggested database +**reviewer**. When they give their sign-off, they will hand over to +the suggested database **maintainer**. + +If reviewer roulette didn't suggest a database reviewer & maintainer, +make sure you have applied the ~database label and rerun the +`danger-review` CI job, or pick someone from the +[`@gl-database` team](https://gitlab.com/groups/gl-database/-/group_members). + +### How to review for database + +- Check migrations + - Review relational modeling and design choices + - Review migrations follow [database migration style guide](migration_style_guide.md), + for example + - [Check ordering of columns](ordering_table_columns.md) + - [Check indexes are present for foreign keys](migration_style_guide.md#adding-foreign-key-constraints) + - Ensure that migrations execute in a transaction or only contain + concurrent index/foreign key helpers (with transactions disabled) + - Check consistency with `db/schema.rb` and that migrations are [reversible](migration_style_guide.md#reversibility) + - For data migrations, establish a time estimate for execution +- Check post deploy migration + - Make sure we can expect post deploy migrations to finish within 1 hour max. +- Check background migrations + - Review queries (for example, make sure batch sizes are fine) + - Establish a time estimate for execution +- Query performance + - Check for any obviously complex queries and queries the author specifically + points out for review (if any) + - If not present yet, ask the author to provide SQL queries and query plans + (e.g. by using [chatops](understanding_explain_plans.md#chatops) or direct + database access) + - For given queries, review parameters regarding data distribution + - [Check query plans](understanding_explain_plans.md) and suggest improvements + to queries (changing the query, schema or adding indexes and similar) + - General guideline is for queries to come in below 100ms execution time + - If queries rely on prior migrations that are not present yet on production + (eg indexes, columns), you can use a [one-off instance from the restore + pipeline](https://ops.gitlab.net/gitlab-com/gl-infra/gitlab-restore/postgres-gprd) + in order to establish a proper testing environment. diff --git a/lib/gitlab/danger/helper.rb b/lib/gitlab/danger/helper.rb index 0fc145534bf..c0a12318990 100644 --- a/lib/gitlab/danger/helper.rb +++ b/lib/gitlab/danger/helper.rb @@ -46,6 +46,16 @@ module Gitlab ee? ? 'gitlab-ee' : 'gitlab-ce' end + def markdown_list(items) + list = items.map { |item| "* `#{item}`" }.join("\n") + + if items.size > 10 + "\n
\n\n#{list}\n\n
\n" + else + list + end + end + # @return [Hash>] def changes_by_category all_changed_files.each_with_object(Hash.new { |h, k| h[k] = [] }) do |file, hash| @@ -132,6 +142,22 @@ module Gitlab def new_teammates(usernames) usernames.map { |u| Gitlab::Danger::Teammate.new('username' => u) } end + + def missing_database_labels(current_mr_labels) + labels = if has_database_scoped_labels?(current_mr_labels) + ['database'] + else + ['database', 'database::review pending'] + end + + labels - current_mr_labels + end + + private + + def has_database_scoped_labels?(current_mr_labels) + current_mr_labels.any? { |label| label.start_with?('database::') } + end end end end diff --git a/spec/lib/gitlab/danger/helper_spec.rb b/spec/lib/gitlab/danger/helper_spec.rb index 92d90ac2fef..f11f68ab3c2 100644 --- a/spec/lib/gitlab/danger/helper_spec.rb +++ b/spec/lib/gitlab/danger/helper_spec.rb @@ -85,6 +85,20 @@ describe Gitlab::Danger::Helper do end end + describe '#markdown_list' do + it 'creates a markdown list of items' do + items = %w[a b] + + expect(helper.markdown_list(items)).to eq("* `a`\n* `b`") + end + + it 'wraps items in
when there are more than 10 items' do + items = ('a'..'k').to_a + + expect(helper.markdown_list(items)).to match(%r{
[^<]+
}) + end + end + describe '#changes_by_category' do it 'categorizes changed files' do expect(fake_git).to receive(:added_files) { %w[foo foo.md foo.rb foo.js db/foo lib/gitlab/database/foo.rb qa/foo ee/changelogs/foo.yml] } @@ -224,4 +238,20 @@ describe Gitlab::Danger::Helper do expect(teammates.map(&:username)).to eq(usernames) end end + + describe '#missing_database_labels' do + subject { helper.missing_database_labels(current_mr_labels) } + + context 'when current merge request has ~database::review pending' do + let(:current_mr_labels) { ['database::review pending', 'feature'] } + + it { is_expected.to match_array(['database']) } + end + + context 'when current merge request does not have ~database::review pending' do + let(:current_mr_labels) { ['feature'] } + + it { is_expected.to match_array(['database', 'database::review pending']) } + end + end end -- cgit v1.2.3