Commit 34a5f77e authored by Toon Claes's avatar Toon Claes Committed by Nick Thomas

Document database review process

See gitlab-com/gl-infra/infrastructure#6069
parent 32536044
......@@ -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
......
......@@ -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"
......@@ -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
......@@ -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<details>\n\n#{list}\n\n</details>"
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
......@@ -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)
......
......@@ -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
......
......@@ -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
......
# 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.
......@@ -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<details>\n\n#{list}\n\n</details>\n"
else
list
end
end
# @return [Hash<String,Array<String>>]
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
......@@ -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 <details> when there are more than 10 items' do
items = ('a'..'k').to_a
expect(helper.markdown_list(items)).to match(%r{<details>[^<]+</details>})
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
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment