Using Gerrit

About Gerrit

Gerrit is primarily a code review tool, but can also be used to track the status of patch as it is build tested, and finally merged to a branch. Developers submit patches to Gerrit using git, and there is a rich browser interface for performing code reviews. Reviewers can also pull the change directly to their own Git repository and review the change locally.

To upload changes to Gerrit, you must first register and add an ssh key to your account, then send an email request for push permission to [email protected].

Gerrit can host many git repositories, and of course each of those repositories can contain many branches.

This page describes the technical process of pushing a patch for review. In addition to the pushing a patch to Gerrit, there is an additional process for Submitting Changes that should be followed in order to ensure that your patch will be accepted for landing.

Managing Changes in Git

Whole books could be written about this topic, and there plenty of online tutorials on the web that explain this in more detail and suggest other methods of managing changes. However, this distilled version is (hopefully) enough to get started.

To make an initial checkout of the master Lustre Git repository:

   $ git clone -b master git://git.whamcloud.com/fs/lustre-release.git lustre-master
   $ cd lustre-master

This will create the master branch in your repository, which should never be modified. Typically with git, one creates a new local branch for each change being made, and a separate commit for each logical change. The branch is created based on one of the local, pristine branches that follows the main development branches, usually the master branch. A new local branch is usually create using git checkout -b:

   $ git checkout -b my_brilliant_idea master

This creates a new branch from master called my-brilliant_idea, and also makes it the current branch. This keeps independent changes separated from each other, simplifying patch management and landing.

In order ensure that the commit description contains the correct name and email address for you, it is possible to specify this directly to Git creating or modifying the $HOME/.gitconfig file in your home directory (preferred), or in the .git/config file in the local repository (e.g. if you are using a different email address for submissions to this repository). This only needs to be done one time, or once per checkout, if done in the local repository.

   [user]
       name = Random J. Developer
       email = [email protected]
   [remote "review"]
       url = ssh://[email protected]:29418/fs/lustre-release 

Now all git commit commands in any repository will use this name/email regardless of which user account is used to do the modifications. The optional [remote "review"] block adds an alias for the remote Gerrit repository to simplify pulling updates and pushing patches, and should only be added in the local .git/config file.

For Lustre patches, the code needs to follow specific Lustre Coding Style Guidelines (which is basically the Linux kernel CodingStyle, with extra details for Lustre), as do the Commit Comments for each patch. In order to help maintain the uniform code style, Lustre-specific Git commit hooks should be installed in all Lustre development trees. In the top-level directory of the checked-out Lustre repository install the hooks using:

   $ cd .git/hooks
   $ ln -sf ../../contrib/git-hooks/*commit-msg .

This will run the contrib/git-hooks/prepare-commit-msg script from the currently-checked-out Lustre tree to run the modifications through the contrib/git-hooks/checkpatch.pl script for style errors, and the contrib/git-hooks/commit-msg script afterward to verify the format of the commit message itself. The format of commit comments is described below.

Once you've made the change, and you want to save the code for testing and reviewed by others, you commit the change. Before you commit in Git, you need to identify which new or changed file(s) you want to commit using git add <filename> for a specific filename, or git add -u for all updated files. If you haven't added any new files, and you want to commit all the files that you have changed (which is the usual case), then you can use the -a option to commit:

   $ git commit -avs

This will put you into an editor to update your commit comment, and when you save it will commit the change locally. The -v option appends the diff to the edit buffer so you can review what is about to be committed. This is very useful to verify that you are committing what you want to commit, and not changes or files that are unrelated to the current change. The -s option adds a Signed-off-by: line to the commit message, which is required for all patches submitted to Lustre.

It is best to commit relatively frequently to keep your changes limited to a single fix. If you notice other, unrelated, issues that need to be fixed, then it's best to put them in a separate commit on a separate branch, so they can be submitted to Gerrit independently for review, testing, and landing. The stash command is handy for this:

   # While fixing a bug you notice something evil that must be fixed.
   # First set your current work aside:
   $ git stash

   # Next go create a new branch and purge the ugliness you just discovered:
   $ git checkout -b my-eyes_are_bleeding master
   \{ fix ugliness \}
   $ git commit -av

   # Now go back to what you were working on:
   $ git checkout -
   $ git stash pop

Formatting Git Commit Comments

Having good commit comments helps everyone that is working on the code. See Commit Comments for a detailed description of the commit comment style.

Sample Commit message:

   LU-nnnnn component: short description of change under 62 columns
   
   A more detailed explanation of the change being made.  This can be as
   detailed as you'd like, possibly several paragraphs in length.
   
   Please provide a detailed explanation of what problem was solved, a
   good high-level description of how it was solved, and which parts of
   the code were affected (including function names as needed, for easier
   searching).  Including specific error messages, stack traces, and
   similar is also good.
   
   Patches claiming performance benefits should show actual numbers with
   a description of how they were measured.
   
   Wrap lines at 70 columns or fewer.
   
   Other useful commit message labels go here.
   
   Test-Parameters: run additional testing for this patch
   Fixes: 1234567890ab ("LU-nnnn comp: old patch being fixed by this one")
   Signed-off-by: Random J Developer <[email protected]>
   Change-Id: Ica9ed1612eab0c4673dee088f8b441d806c64932

Adding the Change-Id field

Gerrit requires all changes to have the Change-Id: field, or it will be rejected. It should be added automatically by the .git/hooks/commit-msg script. See Commit Comments for a detailed description of the Change-Id: field. The Change-Id: should be kept the same for a patch when ported to different branches/repositories.

Submitting Patches for Review, Testing, and Landing

All patches that land on the main release branches in the fs/lustre-release repository must conform to the Lustre_Coding_Guidelines, pass the standard Lustre regression tests. All patches should be associated with a Lustre Jira ticket, and contain a commit message the fully meets the Commit Comment Format Requirements. Bug fixes should include additional regression tests to ensure the problem is fixed, since the presence of the bug indicates that no test is covering this functionality, and the test will prevent the bug from being reintroduced in the future. Feature patches should include new regression tests to exercise the feature, and potentially performance or load tests as appropriate for the change. All changes submitted to the fs/lustre-release repository will be tested automatically with the Lustre regression tests in lustre/tests and a link to the test results (saved in Maloo) and a pass/fail review comment will be added to the Gerrit change. For additional tests run outside of autotest, the results can either be sent to Maloo manually, or test results can be posted into the Jira ticket associated with the patch.

Any patches that require updates to the Lustre manual will also need to submit documentation updates when the patch is ready for submission. See the Lustre_Manual_Changes page for more details.

Any additional feature testing that cannot be covered by the existing or added automated Lustre tests must also be documented with a clear, step-by-step test plan, and arrangements must be made with the Lustre test team to execute these plans.

Patch Series

A change request is created by pushing one or more patches to a special branch on the Gerrit repository. To create an change request for a patch in your local branch the local commit (or series of commits) needs to be pushed to the Gerrit review repository.

Independent Patches

If you have a series of independent commits to be reviewed (i.e. patches that are not dependent on each other for functionality or overlapping code context), each one should be in a separate local branch and pushed separately to Gerrit. This includes patches which are making common or related changes to the code, but in non-overlapping files (e.g. whitespace or style cleanups). This allows these patches to be reviewed, tested, and landed independently, and will avoid the overhead and delay of repeatedly reviewing, building, and testing patches that are only refreshed because an earlier (unrelated) patch in a series is being modified.

Dependent Patch Series

If there are a series of dependent changes being made to the code (e.g. code style fixes, renaming some functions, adding function parameters, moving code from one file to another, making the actual fix for a bug, making some additional fixes to other defects found during coding), these should all go into separate patches for Patch Inspections and testing.

Having a series of smaller patches, each of which makes a single clearly understood change, speeds up inspection significantly and means those changes will land sooner. Ordering uncontroversial and easy-to-inspect changes first in a patch series ensures that they are only inspected a single time, and can land quickly. Ordering complex changes at the end of the patch series ensures that if changes are requested by the inspectors for these patches, the earlier ones do not need to be re-inspected and re-tested for each update. Following this patch submission model speeds up patch landing, and reduces the burden on the patch inspector, and test systems, and reduces the time that the patch submitter needs to spend waiting or rebasing in order to get their patches accepted.

What *Not* to Put in a Patch Series

Patch series should not be used for new code that had a series of bug fixes applied to it during local testing. The later (local) bug fixes should be squashed into the original change before submission. Note that each patch in a series must build and pass testing before it can be landed, so compile warnings or test failures in the middle of a patch series are not allowed.

Creating a Gerrit Account

It is possible to create a new Gerrit account using an OpenID provider of your choice. For Whamcloud developers, a shortcut is provided on the main Gerrit page. For other developers, it is possible to use Ubuntu Launchpad authentication, or another OpenID provider by using a login URL like https://review.whamcloud.com/login/%2Fq%2Fstatus%3Aopen?id=URL_FOR_YOUR_OPEN_ID_PROVIDER.

Submission Authorization

To avoid issues with spam and system abuse, authorization is required to submit a patch to Gerrit. Without proper account authorization, an error like the following will be reported from the git push command:

   fatal: remote error: Upload denied for project 'fs/lustre-release'

All new patch submitters should contact [email protected] to request authorization for their account.

Submission Commands

To submit your patch to Gerrit, one of the following commands should be used:

   $ git push ssh://[email protected]:29418/fs/lustre-release HEAD:refs/for/master

for the master branch of the fs/lustre-release repo, or:

   $ git push ssh://[email protected]:29418/fs/lustre-release HEAD:refs/for/b2_12

for the b2_12 branch of the fs/lustre-release repo, or:

   $ git push ssh://[email protected]:29418/tools/e2fsprogs HEAD:refs/for/master-lustre

for the master-lustre branch of the tools/e2fsprogs repo.

For convenience, you can add this to your ~/.ssh/config file:

   Host review
       Hostname review.whamcloud.com
       Port 29418
       User \{YourUserid\}
       IdentityFile ~/.ssh/my-key-id_rsa

Creating a review request for a change against master (assuming the remote alias has been added to ~/.ssh/config):

   $ git push ssh://review/fs/lustre-release HEAD:refs/for/master

Add the Gerrit server as a remote to your git repository:

   $ git remote add review ssh://review/fs/lustre-release

Automatically Building and Testing a Patch

When the patch has been pushed to the Gerrit fs/lustre-review repository, it will print a URL for that change set that will automatically be added in a comment to the Jira ticket for tracking.

All patches submitted to Gerrit will be built automatically by Jenkins for a number of different distro and kernel versions. Currently for the master branch this includes RHEL8 x86_64 client/server, Ubuntu x86_64/ARM clients, and SLES15 x86_64 client/server. The success/failure of these builds will be posted to the change in Gerrit as a link to the Jenkins build artifacts.

After a successful build on all of the configurations, one or more of the configurations will be regression tested automatically, and a link to the test results in Maloo will be posted to Gerrit.

Normally, there are many separate test sessions run for each patch submitted to the master branch. The majority of the test sessions will be enforced, which means that they must pass in order to land the patch. In some cases test sessions may be marked optional and are not required to pass before landing (e.g. sessions for new distros/kernels, new features, etc.), but it is still desirable to check if they are failing due to issues introduced by the patch to avoid test regressions.

Automated regression testing for each patch should normally pass. In some cases it might fail for a variety of reasons, such as a bug in the patch, an intermittent test failure that is present in the existing code, problems with the testing system, etc. In all failure cases, the Maloo test failure(s) should be investigated as to the root cause. Once the cause of the test failure is identified in Maloo, the failed subtest result should either be linked (via [Associate bug]) to an existing LU ticket in Jira, or if necessary a new Jira ticket with details of the failure should be filed (via [Raise bug]), and a comment added to Gerrit with the JIRA LU ticket number. At this point the test could be resubmitted, if there are no other problems with the patch, or it can be refreshed if there are defects found in the patch and/or review comments that need to be addressed (per below).

Adding Extra Test Sessions

It is also possible to add special testing requests using the Test-Parameters: label when the patch is submitted. This allows adding sessions for interop testing with older clients/servers, specific client architectures, repeat specific subtests, and more.

Requesting Patch Review

In order to actually get a patch landed, you need to request at least two reviews in Gerrit for the patch. This is done by visiting the Gerrit web page for the change (at the URL returned when the commit was pushed, or it can be found from your home page after logging in). Enter the name or email address for the reviewer and click the [Add...] button in the top right corner.

If you are unsure of who should be the reviewer of your patch, you can use the get_maintainer.pl script to get a list of potential candidates. The commit hash shown in the example is optional, just to give a concrete example here) based on the MAINTAINERS file and recent commits to the modified files, otherwise it will examine the top patch on the branch:

   $ git show 8b364fbd6bd9e0088440e6d6837861a641b923a0 | ./contrib/scripts/get_maintainer.pl
   Patrick Farrell <[email protected]> (reviewer:Lustre Grant Space,commit_signer:4/6=67%,authored:1/6=17%)
   Bobijam Xu <[email protected]> (reviewer:Lustre Client IO stack,authored:1/6=17%,removed_lines:30/82=37%)
   Andreas Dilger <[email protected]> (commit_signer:3/6=50%,authored:1/6=17%,added_lines:25/65=38%,removed_lines:20/82=24%)
   Jinshan Xiong <[email protected]> (commit_signer:2/6=33%)
   Alexey Lyashkov <[email protected]> (commit_signer:2/6=33%)
   Li Dongyang <[email protected]> (commit_signer:2/6=33%,authored:2/6=33%,added_lines:28/65=43%,removed_lines:15/82=18%)
   Alexander Boyko <[email protected]> (authored:1/6=17%,added_lines:11/65=17%,removed_lines:16/82=20%)

The list is not always providing the best candidates, since it doesn't know the difference between someone making a cosmetic patch and the experts in the code, but at least it can provide a starting point. Usually it makes sense to include the top 2-3 people, in particular anyone marked reviewer for that code. The reported list of developers will change over time, as new commits are added to the affected files.

In conjunction with the two review requests, the patch has to successfully build on all supported architecture/distro combinations, and pass the automatic testing (as reported by Maloo). Once the patch has passed two reviews (Reviewed: +1 or Reviewed: +2 from reviewers that are not the change owner), built correctly (Verified: +1 from Jenkins), and has passed autotest (Verified: +1 from Maloo) the patch will automatically appear in the Gatekeeper's list of patches to land.

It is the responsibility of the patch submitter to request the reviewers to look at the patch, to check for build/test failures and annotate them correctly, and to refresh the patch after addressing review, build, or test issues. If this is not happening in a timely manner, one option is adding a comment into the patch to alert the reviewers that the patch needs attention.

Updating Patches After Review

Gerrit makes it easy to update a patch after review, and doing this allows reviewers to see differences between the patches so they only need to review the changes between the patches instead of having to review the entire patch again. Also, the original inline comments are maintained and moved to the new patches. The key to keeping updated review requests linked to the original patch is using the same Change-Id: field in the commit comment - this is what Gerrit uses to find the original patch to update.

A critical thing to point out is that you must submit a new version of the entire patch - not just an update to the patch.

The easiest way to update the most recent commit (which is often the one you want to update), is to use "git commit --amend -a". This will "add" any modifications in the current repository and merge them into the last commit. If there are no changes, or -a is not used, this will just allow you to edit the most recent commit message. This is useful if you don't have a Change-Id: line in your commit message (because you didn't install the commit-msg hook, see [#Formatting Git Commit Comments] above), but a Change-Id: was added automatically by Gerrit. You can copy the Change-Id: from the Gerrit web page for that change and paste it into the amended commit message. Typically, the updated patch should also be rebased to the tip of the current branch before pushing it again, to ensure that it does not conflict with other changes that may have been landed since the patch was first developed.

Another way of modifying a series of existing changes is to use "git rebase -i <parent-branch>". Interactive rebase will display a list of patches in an editor, and you can reorder the patches, "edit" the patch, "reword" the commit comments, and "squash" two patches into a single patch. See the help text in the commit message editor window for more information.

Once the change has been updated, push your branch to Gerrit again, and the original request will be updated with the new version of the patch.

Basing One Change on Another Unlanded Change

It is possible to base a new change, or rebase an existing change, on an uncommitted patch in Gerrit. This might be useful if both changes are impacting the same code, and one change is clearly dependent on another for some reason, or if they will cause conflicts when merged and a decision is made to land them in a specific order. It is desirable to break a patch into multiple functionally-separate commits for several reasons. If the patches are truly independent (i.e. no overlapping changes to the same files, no dependent functions, etc), then those patches should be submitted in separate branches, and this section does not apply.

If both patches are being developed by the same person, the easiest way to have a series of dependent changes is to commit them into separate patches order on the same branch. Then, when a change is made to any patch on that branch, all of the dependent changes will also be resubmitted to Gerrit so that they will be ready to land when the earlier patches are merged.

If updates or fixes need to be made to one of the patches, these updates should be merged into the original commit where the code was added, rather than being an additional patch at the end of the series. This can be done by running "git rebase -i <parent-branch>", then marking a particular patch for edit. That will cause the earlier patches in the series up to the to be applied, then stop for interactive editing/testing of that patch. Once the patch has been updated, run "git add -u" to include the new updates into this patch, and "git rebase --continue" to merge the updates into the existing patch and continue the rebase process.

In the case where changes need to be based on a patch from another developer, it is possible to check out the desired patch from Gerrit using one of the supplied Git URLs in the [Download v] section in the top right corner of the change page. Select the Checkout options, then copy the URL and paste it at the command prompt of a previously checked-out Lustre tree. For example to fetch patchset 16 from Change 1264, create a branch for it, and then create a new branch for your local changes based on that change, use:

   [lustre]$ git fetch http://review.whamcloud.com/p/fs/lustre-release refs/changes/64/1264/16 && git checkout FETCH_HEAD
   [lustre]$ git checkout -b b_1264
   [lustre]$ git checkout -b b_1264_my_changes
   {edit local tree to make changes as usual}
   [lustre]$ git commit -a

Any changes in the b_1264_my_changes will be based on top of those of b_1264. When b_1264_my_changes is pushed to Gerrit, it will have a dependency on change 1264, so it will not be able to land until change 1264 itself is landed. It is typically not desirable to locally rebase the b_1264 branch to a new version of master or modify it, or it will cause the other developer's patch to be updated in Gerrit, and lose its existing review and test results. However, in some cases this is desirable, and Gerrit allows a user other than the original author to push an updated version of the patch (using the same Change-Id: label) in case there is a problem with the original patch (e.g. no longer applies, has a bug, etc).

If you already have the changes in a local branch (e.g. b_my_changes) and want to rebase that branch on top of another uncommitted patch from Gerrit, the process is similar:

   [lustre]$ git fetch http://review.whamcloud.com/p/fs/lustre-release refs/changes/64/1264/16 && git checkout FETCH_HEAD
   [lustre]$ git checkout -b b_1264
   [lustre]$ git checkout b_my_changes
   [lustre]$ git rebase --onto b_1264

That will rebase the patch(es) in the current branch onto the other patch (or branch) checked out from Gerrit.

Pushing a Git Tag

This is normally done by the branch maintainer, but is recorded here for future reference.

Switch to the branch you want to create a branch in (normally master or a release branch like b2_12).

Create the tag with:

   $ git tag -a TAG_NAME

then push it upstream with

   $ git push TAG_NAME

Note, this requires certain privileges on the server. If review is a secondary server for your repository, use:

   $ git push review TAG_NAME

Searching on Gerrit

This is covered on the page: Useful Gerrit Searches