Code Reviews

Code reviews are a critical component of all OpenStack projects. Neutron accepts patches from many diverse people with diverse backgrounds, employers, and experience levels. Code reviews provide a way to enforce a level of consistency across the project, and also allow for the careful on boarding of contributions from new contributors.

Neutron Code Review Practices

Neutron follows the code review guidelines as set forth for all OpenStack projects. It is expected that all reviewers are following the guidelines set forth on that page.

In addition to that, the following rules are to follow:

  • Any change that requires a new feature from Neutron runtime dependencies requires special review scrutiny to make sure such a change does not break a supported platform (examples of those platforms are latest Ubuntu LTS or CentOS). Runtime dependencies include but are not limited to: kernel, daemons and tools as defined in oslo.rootwrap filter files, runlevel management systems, as well as other elements of Neutron execution environment.

    Note

    For some components, the list of supported platforms can be wider than usual. For example, Open vSwitch agent is expected to run successfully in Win32 runtime environment.

    1. All such changes must be tagged with UpgradeImpact in their commit messages.

    2. Reviewers are then advised to make an effort to check if the newly proposed runtime dependency is fulfilled on supported platforms.

    3. Specifically, reviewers and authors are advised to use existing gate and experimental platform specific jobs to validate those patches. To trigger experimental jobs, use the usual protocol (posting check experimental comment in Gerrit). CI will then execute and report back a baseline of Neutron tests for platforms of interest and will provide feedback on the effect of the runtime change required.

    4. If review identifies that the proposed change would break a supported platform, advise to rework the patch so that it’s no longer breaking the platform. One of the common ways of achieving that is gracefully falling back to alternative means on older platforms, another is hiding the new code behind a conditional, potentially controlled with a oslo.config option.

      Note

      Neutron team retains the right to remove any platform conditionals in future releases. Platform owners are expected to accommodate in due course, or otherwise see their platforms broken. The team also retains the right to discontinue support for unresponsive platforms.

    5. The change should also include a new sanity check that would help interested parties to identify their platform limitation in timely manner.

  • Special attention should also be paid to changes in Neutron that can impact the Stadium and the wider family of networking-related projects (referred to as sub-projects below). These changes include:

    1. Renaming or removal of methods.

    2. Addition or removal of positional arguments.

    3. Renaming or removal of constants.

    To mitigate the risk of impacting the sub-projects with these changes, the following measures are suggested:

    1. Use of the online tool codesearch to ascertain how the proposed changes will affect the code of the sub-projects.

    2. Review the results of the non-voting check and 3rd party CI jobs executed by the sub-projects against the proposed change, which are returned by Zuul in the change’s Gerrit page.

    When impacts are identified as a result of the above steps, every effort must be made to work with the affected sub-projects to resolve the issues.

  • Any change that modifies or introduces a new API should have test coverage in neutron-tempest-plugin or tempest test suites. There should be at least one API test added for a new feature, but it is preferred that both API and scenario tests be added where it is appropriate.

    Scenario tests should cover not only the base level of new functionality, but also standard ways in which the functionality can be used. For example, if the feature adds a new kind of networking (like e.g. trunk ports) then tests should make sure that instances can use IPs provided by that networking, can be migrated, etc.

    It is also preferred that some negative test cases, like API tests to ensure that correct HTTP error is returned when wrong data is provided, will be added where it is appropriate.

  • It is usually enough for any “mechanical” changes, like e.g. translation imports or imports of updated CI templates, to have only one +2 Code-Review vote to be approved. If there is any uncertainty about a specific patch, it is better to wait for review from another core reviewer before approving the patch.

Neutron Spec Review Practices

In addition to code reviews, Neutron also maintains a BP specification git repository. Detailed instructions for the use of this repository are provided here. It is expected that Neutron core team members are actively reviewing specifications which are pushed out for review to the specification repository. In addition, there is a neutron-drivers team, composed of a handful of Neutron core reviewers, who can approve and merge Neutron specs.

Some guidelines around this process are provided below:

  • Once a specification has been pushed, it is expected that it will not be approved for at least 3 days after a first Neutron core reviewer has reviewed it. This allows for additional cores to review the specification.

  • For blueprints which the core team deems of High or Critical importance, core reviewers may be assigned based on their subject matter expertise.

  • Specification priority will be set by the PTL with review by the core team once the specification is approved.

Tracking Review Statistics

Stackalytics provides some nice interfaces to track review statistics. The links are provided below. These statistics are used to track not only Neutron core reviewer statistics, but also to track review statistics for potential future core members.