Code Review Guide for Nova¶
This is a very terse set of points for reviewers to consider when looking at nova code. These are things that are important for the continued smooth operation of Nova, but that tend to be carried as “tribal knowledge” instead of being written down. It is an attempt to boil down some of those things into nearly checklist format. Further explanation about why some of these things are important belongs elsewhere and should be linked from here.
When making a change to the nova API, we should always follow the API WG guidelines rather than going for “local” consistency. Developers and reviewers should read all of the guidelines, but they are very long. So here are some key points:
projectshould be used in the REST API instead of
servershould be used in the REST API instead of
computeshould be used in the REST API instead of
- URL should not include underscores; use hyphens (‘-‘) instead.
- The field names contained in a request/response body should use snake_case style, not CamelCase or Mixed_Case style.
- Synchronous resource creation:
- Asynchronous resource creation:
- Synchronous resource deletion:
204 No Content
- For all other successful operations:
- Synchronous resource creation:
The central place where all config options should reside is the
package. Options that are in named sections of
nova.conf, such as
[serial_console], should be in their own module. Options that are in the
[DEFAULT] section should be placed in modules that represent a natural
grouping. For example, all of the options that affect the scheduler would be
scheduler.py file, and all the networking options would be moved
A config option should be checked for:
A short description which explains what it does. If it is a unit (e.g. timeouts or so) describe the unit which is used (seconds, megabyte, mebibyte, ...).
A long description which explains the impact and scope. The operators should know the expected change in the behavior of Nova if they tweak this.
Descriptions/Validations for the possible values.
- If this is an option with numeric values (int, float), describe the edge cases (like the min value, max value, 0, -1).
- If this is a DictOpt, describe the allowed keys.
- If this is a StrOpt, list any possible regex validations, or provide a list of acceptable and/or prohibited values.
Previously used sections which explained which services consume a specific config option and which options are related to each other got dropped because they are too hard to maintain: http://lists.openstack.org/pipermail/openstack-dev/2016-May/095538.html
Third Party Tests¶
Any change that is not tested well by the Jenkins check jobs must have a recent +1 vote from an appropriate third party test (or tests) on the latest patchset, before a core reviewer is allowed to make a +2 vote.
At a minimum, we must ensure that any technology specific code has a +1 from the relevant third party test, on the latest patchset, before a +2 vote can be applied. Specifically, changes to nova/virt/driver/<NNNN> need a +1 vote from the respective third party CI. For example, if you change something in the XenAPI virt driver, you must wait for a +1 from the XenServer CI on the latest patchset, before you can give that patch set a +2 vote.
This is important to ensure:
- We keep those drivers stable
- We don’t break that third party CI
- Long term, we should ensure that any patch a third party CI is allowed to vote on, can be blocked from merging by that third party CI. But we need a lot more work to make something like that feasible, hence the proposed compromise.
- While its possible to break a virt driver CI system by changing code that is outside the virt drivers, this policy is not focusing on fixing that. A third party test failure should always be investigated, but the failure of a third party test to report in a timely manner should not block others.
- We are only talking about the testing of in-tree code. Please note the only public API is our REST API, see: Development policies
Should I run the experimental queue jobs on this change?¶
Because we can’t run all CI jobs in the check and gate pipelines, some jobs can be executed on demand, thanks to the experimental pipeline. To run the experimental jobs, you need to comment your Gerrit review with “check experimental”.
The experimental jobs aim to test specific features, such as LXC containers or DVR with multiple nodes. Also, it might be useful to run them when we want to test backward compatibility with tools that deploy OpenStack outside Devstack (e.g. TripleO, etc). They can produce a non-voting feedback of whether the system continues to work when we deprecate or remove some options or features in Nova.
The experimental queue can also be used to test that new CI jobs are correct before making them voting.
- Use the
utf8charset only where necessary. Some string fields, such as hex-stringified UUID values, MD5 fingerprints, SHA1 hashes or base64-encoded data, are always interpreted using ASCII encoding. A hex-stringified UUID value in
latin1is 1/3 the size of the same field in
utf8, impacting performance without bringing any benefit. If there are no string type columns in the table, or the string type columns contain only the data described above, then stick with
If a new microversion API is added, the following needs to happen:
- A new patch for the microversion API change in python-novaclient side should be submitted.
- If the microversion changes the response schema, a new schema and test for the microversion must be added to Tempest. The microversion change in Nova should not be merged until the Tempest test is submitted and at least passing; it does not need to be merged yet as long as it is testing the Nova change via Depends-On. The Nova microversion change commit message should reference the Change-Id of the Tempest test for reviewers to identify it.
What is reno ?¶
Nova uses reno for providing release notes in-tree. That means that a patch can include a reno file or a series can have a follow-on change containing that file explaining what the impact is.
A reno file is a YAML file written in the releasenotes/notes tree which is generated using the reno tool this way:
$ tox -e venv -- reno new <name-your-file>
<name-your-file> can be
bp-<blueprint_name> for a
bug-XXXXXX for a bugfix.
Refer to the reno documentation for the full list of sections.
When a release note is needed¶
A release note is required anytime a reno section is needed. Below are some examples for each section. Any sections that would be blank should be left out of the note file entirely. If no section is needed, then you know you don’t need to provide a release note :-)
- The patch has an UpgradeImpact tag
- A DB change needs some deployer modification (like a migration)
- A configuration option change (deprecation, removal or modified default)
- some specific changes that have a DocImpact tag but require further action from an deployer perspective
- any patch that requires an action from the deployer in general
- If the patch fixes a known vulnerability
- If the patch has an APIImpact tag
- For nova-manage and python-novaclient changes, if it adds or changes a new command, including adding new options to existing commands
- not all blueprints in general, just the ones impacting a contractual API
- a new virt driver is provided or an existing driver impacts the HypervisorSupportMatrix
- Bugfixes categorized as Critical in Launchpad impacting users
- No clear definition of such bugfixes. Hairy long-standing bugs with high importance that have been fixed are good candidates though.
Three sections are left intentionally unexplained (
other). Those are targeted to be filled in close to the release time for
providing details about the soon-ish release. Don’t use them unless you know
exactly what you are doing.