Code Reviews¶

Gerrit¶

Cinder uses the Gerrit tool to review proposed code changes. The review site is https://review.opendev.org

Gerrit is a complete replacement for Github pull requests. All Github pull requests to the Cinder repository will be ignored.

See Quick Reference for information on quick reference for developers. See Getting Started for information on how to get started using Gerrit. See Development Workflow for more detailed information on how to work with Gerrit.

The Great Change¶

With the demise of Python 2.7 in January 2020, beginning with the Ussuri development cycle, Cinder only needs to support Python 3 runtimes (in particular, 3.6 and 3.7). Thus we can begin to incorporate Python 3 language features and remove Python 2 compatibility code. At the same time, however, we are still supporting stable branches that must support Python 2. Our biggest interaction with the stable branches is backporting bugfixes, where in the ideal case, we’re just doing a simple cherry-pick of a commit from master to the stable branches. You can see that there’s some tension here.

With that in mind, here are some guidelines for reviewers and developers that the Cinder community has agreed on during this phase where we want to write pure Python 3 but still must support Python 2 code.

Targeting Milestones¶

In an effort to guide team review priorities the Cinder team has adopted the process of adding comments to reviews to target a milestone for a particular patch. This process is not required for all patches but is beneficial for patches that may be time sensitive. For example patches that need to land earlier in the release cycle so as to get additional test time or because later development activities are dependent upon that functionality merging.

To target a patch to a milestone a reviewer should add a comment using the following format:

target-<release>-<milestone>

Release should be used to indicate the release to which the patch should be targeted, all lower case. The milestone is a single number, 1 to 3, indicating the milestone number. So, to target a patch to land in Milestone 2 of the Rocky release a comment like the following would be added:

target-rocky-2

Adding this tag allows reviewers to search for these tags and use them as a guide in review priorities.

Targeting patches should be done by Cinder Core Review Team members. If a patch developer feels that a patch should be targeted to a milestone the developer should bring the request up to the Cinder team in a weekly meeting or on the #openstack-cinder IRC channel.

Reviewing Vendor Patches¶

It is important to consider, when reviewing patches to a vendor’s Cinder driver, whether the patch passes the vendor’s CI process. CI reports are the only tool we have to ensure that a patch works with the Vendor’s driver. A patch to a vendor’s driver that does not pass that vendor’s CI should not be merged. If a patch is submitted by a person that does not work with the vendor that owns the driver, a +1 review from someone at that vendor is also required. Finally, a patch should not be merged before the Vendor’s CI has run against the patch.

Unit Tests¶

Cinder requires unit tests with all patches that introduce a new branch or function in the code. Changes that do not come with a unit test change should be considered closely and usually returned to the submitter with a request for the addition of unit test.

CI Job rechecks¶

CI job runs may result in false negatives for a considerable number of causes:

• Network failures.

• Not enough resources on the job runner.

• Storage timeouts caused by the array running nightly maintenance jobs.

• External service failure: pypi, package repositories, etc.

• Non cinder components spurious bugs.

And the list goes on and on.

When we detect one of these cases the normal procedure is to run a recheck writing a comment with recheck for core Zuul jobs, or the specific third party CI recheck command, for example run-DellEMC PowerStore CI.

These false negative have periods of time where they spike, for example when there are spurious failures, and a lot of rechecks are necessary until a valid result is posted by the CI job. And it’s in these periods of time where people acquire the tendency to blindly issue rechecks without looking at the errors reported by the jobs.

When these blind checks happen on real patch failures or with external services that are going to be out for a while, they lead to wasted resources as well as longer result times for patches in other projects.

The Cinder community has noticed this tendency and wants to fix it, so now it is strongly encouraged to avoid issuing naked rechecks and instead issue them with additional information to indicate that we have looked at the failure and confirmed it is unrelated to the patch.

Here are some real examples of proper rechecks:

• Spurious issue in other component: recheck tempest-integrated-storage : intermittent failure nova bug #1836754

• Deployment issue on the job: recheck cinder-plugin-ceph-tempest timed out, errors all over the place

• External service failure: Third party recheck grenade : Failed to retrieve .deb packages

Another common case for blindly rechecking a patch is when it is only changing a specific driver but there are failures on jobs that don’t use that driver. In such cases we still have to look at the failures, because they can be failures that are going to take a while to fix, and issuing a recheck will be futile at that time and we should wait for a couple of hours, or maybe even a day, before issuing a recheck that can yield the desired result.

Efficient Review Guidelines¶

This section will guide you through the best practices you can follow to do quality code reviews:

• Failing Gate: You can check for jobs like pep8, py36, py38, functional etc that are generic to all the patches and look for possible failures in linting, unit test, functional test etc and provide feedback on fixing it. Usually it’s the author’s responsibility to do a local run of tox and ensure they don’t fail upstream but if something is failing on gate and the author is not be aware about how to fix it then we can provide valuable guidance on it. There are also jobs specific to particular area of code (for example, cinder-plugin-ceph-tempest for the RBD volume driver, devstack-plugin-nfs-tempest-full for the generic NFS driver etc) so look for issues in the jobs if they are related to the code changes proposed. There is a past example on why we should check these jobs, the devstack-plugin-nfs-tempest-full is a non-voting job and was failing on one of the FS drivers related patch which got merged and started failing the NetApp CI blocking the netapp features during that time.

• Documentation: Check whether the patch proposed requires documentation or not and ensure the proper documentation is added. If the proper documentation is added then the next step is to check the status of docs job if it’s failing or passing. If it passes, you can check how it looks in HTML as follows: Go to openstack-tox-docs job link -> View Log -> docs and go to the appropriate section for which the documentation is added. Rendering: We do have a job for checking failures related to document changes proposed (openstack-tox-docs) but we need to be aware that even if a document change passes all the syntactical rules, it still might not be logically correct i.e. after rendering it could be possible that the bullet points are not under the desired section or the spacing and indentation is not as desired. It is always good to check the final document after rendering in the docs job which might yield possible logical errors.

• Readability: In a large codebase (like Cinder), Readability is a big factor as remembering the logic of every code path is not feasible and contributors change from time to time. We should adapt to writing readable code which is easy to follow and can be understood by anyone having knowledge about Python constructs and working of Cinder. Sometimes it happens that a logic can only be written in a complex way, in that case, it’s always good practice to add a comment describing the functionality. So, if a logic proposed is not readable, do ask/suggest a more readable version of it and if that’s not feasible then asking for a comment that would explain it is also a valid review point.

• Type Annotations: There has been an ongoing effort to implement type annotations all across Cinder with the help of mypy tooling. Certain areas of code already adapt to mypy coding style and it’s good practice that new code merging into Cinder should also adapt to it. We, as reviewers, should ensure that new code proposed should include mypy constructs.

• Microversions: Cinder uses the microversion framework for implementing new feature that causes a change in the API behavior (request/response) while maintaining backward compatibility at the same time. There have been examples in the past where a patch adding a new microversion misses file(s) where the microversion changes are necessary so it’s a good practice for the author and reviewer to ensure that all files associated with a microversion change should be updated. You can find the list of files and changes required in our Microversion Doc.

• Downvoting reason: It often happens that the reviewer adds a bunch of comments some of which they would like to be addressed (blocking) and some of them are good to have but not a hard requirement (non-blocking). It’s a good practice for the reviewer to mention for which comments is the -1 valid so to make sure they are always addressed.

• Testing: Always check if the patch adds the associated unit, functional and tempest tests depending on the change.

• Commit Message: There are few things that we should make sure the commit message includes:

  1) Make sure the author clearly explains in the commit message why the code changes are necessary and how exactly the code changes fix the issue.

  2) It should have the appropriate tags (Eg: Closes-Bug, Related-Bug, Blueprint, Depends-On etc). For detailed information refer to external references in commit message.

  3) It should follow the guidelines of commit message length i.e. 50 characters for the summary line and 72 characters for the description. More information can be found at Summary of Git commit message structure.

  4) Sometimes it happens that the author updates the code but forgets to update the commit message leaving the commit describing the old changes. Verify that the commit message is updated as per code changes.

• Release Notes: There are different cases where a releasenote is required like fixing a bug, adding a feature, changing areas affecting upgrade etc. You can refer to the Release notes section in our contributor docs for more information.

• Ways of reviewing: There are various ways you can go about reviewing a patch, following are some of the standard ways you can follow to provide valuable feedback on the patch:

  1) Testing it in local environment: The easiest way to check the correctness of a code change proposed is to reproduce the issue (steps should be in launchpad bug) and try the same steps after applying the patch to your environment and see if the provided code changes fix the issue. You can also go a little further to think of possible corner cases where an end user might possibly face issues again and provide the same feedback to cover those cases in the original change proposed.

  2) Optimization: If you’re not aware about the code path the patch is fixing, you can still go ahead and provide valuable feedback about the python code if that can be optimized to improve maintainability or performance.

  3) Perform Dry Run: Sometimes the code changes are on code paths that we don’t have or can’t create environment for (like vendor driver changes or optional service changes like cinder-backup) so we can read through the code or use some example values to perform a dry run of the code and see if it fails in that scenario.