Cinder follows the same Review guidelines outlined by the OpenStack community. This page provides additional information that is helpful for reviewers of patches to Cinder.
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.
Python 2 to Python 3 transition guidelines¶
We need to be checking the code coverage of test cases very carefully so that new code has excellent coverage. The idea is that we want these tests to fail when a backport is proposed to a stable branch and the tests are run under Python 2 (if the code is using any Python-3-only language features).
New features can use Python-3-only language constructs, but bugfixes likely to be backported should be more conservative and write for Python 2 compatibilty.
The code for drivers may continue to use the six compatibility library at their discretion.
We will not remove six from mainline Cinder code that impacts the drivers (for example, classes they inherit from).
We can remove six from code that doesn’t impact drivers, keeping in mind that backports may be more problematic, and hence making sure that we have really good test coverage.
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:
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:
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
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.
Patches which have passed vendor CI and have merged in master are exempt from this requirement upon backport to stable and/or driverfixes branches as vendors are not required to run CI on those branches. If the vendor, however, is running CI on stable and/or driverfix branches failures should not be ignored unless otherwise verified by a developer from the vendor.
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.
Unit test changes are not validated in any way by vendor’s CI. Vendor CI’s run the tempest volume tests against a change which does not include a unit test execution.
CI Job rechecks¶
CI job runs may result in false negatives for a considerable number of causes:
Not enough resources on the job runner.
Storage timeouts caused by the array running nigthly 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 locking 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.