Markus Zoeller
71fb6389a6
The code-review guidelines document should be a somewhat agreed list of what to consider when doing a review. This patch set adds the items to check if the review contains changes which involves config options. Change-Id: I142ab25fa7fc1c4ece5a68f68c5d841c797af1be
114 lines
5.1 KiB
ReStructuredText
114 lines
5.1 KiB
ReStructuredText
==========================
|
|
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.
|
|
|
|
Upgrade-Related Concerns
|
|
========================
|
|
|
|
RPC API Versions
|
|
----------------
|
|
|
|
* If an RPC method is modified, the following needs to happen:
|
|
|
|
* The manager-side (example: compute/manager) needs a version bump
|
|
* The manager-side method needs to tolerate older calls as well as
|
|
newer calls
|
|
* Arguments can be added as long as they are optional. Arguments
|
|
cannot be removed or changed in an incompatible way.
|
|
* The RPC client code (example: compute/rpcapi.py) needs to be able
|
|
to honor a pin for the older version (see
|
|
self.client.can_send_version() calls). If we are pinned at 1.5, but
|
|
the version requirement for a method is 1.7, we need to be able to
|
|
formulate the call at version 1.5.
|
|
* Methods can drop compatibility with older versions when we bump a
|
|
major version.
|
|
|
|
* RPC methods can be deprecated by removing the client (example:
|
|
compute/rpcapi.py) implementation. However, the manager method must
|
|
continue to exist until the major version of the API is bumped.
|
|
|
|
Object Versions
|
|
---------------
|
|
|
|
* If a tracked attribute (i.e. listed in fields) or remotable method
|
|
is added, or a method is changed, the object version must be
|
|
bumped. Changes for methods follow the same rules as above for
|
|
regular RPC methods. We have tests to try to catch these changes,
|
|
which remind you to bump the version and then correct the
|
|
version-hash in the tests.
|
|
* Field types cannot be changed. If absolutely required, create a
|
|
new attribute and deprecate the old one. Ideally, support converting
|
|
the old attribute to the new one with an obj_load_attr()
|
|
handler. There are some exceptional cases where changing the type
|
|
can be allowed, but care must be taken to ensure it does not affect
|
|
the wireline API.
|
|
* New attributes should be removed from the primitive in
|
|
obj_make_compatible() if the attribute was added after the target
|
|
version.
|
|
* Remotable methods should not return unversioned structures wherever
|
|
possible. They should return objects or simple values as the return
|
|
types are not (and cannot) be checked by the hash tests.
|
|
* Remotable methods should not take complex structures as
|
|
arguments. These cannot be verified by the hash tests, and thus are
|
|
subject to drift. Either construct an object and pass that, or pass
|
|
all the simple values required to make the call.
|
|
* Changes to an object as described above will cause a hash to change
|
|
in TestObjectVersions. This is a reminder to the developer and the
|
|
reviewer that the version needs to be bumped. There are times when
|
|
we need to make a change to an object without bumping its version,
|
|
but those cases are only where the hash logic detects a change that
|
|
is not actually a compatibility issue and must be handled carefully.
|
|
|
|
Database Schema
|
|
---------------
|
|
|
|
* Changes to the database schema must generally be additive-only. This
|
|
means you can add columns, but you can't drop or alter a column. We
|
|
have some hacky tests to try to catch these things, but they are
|
|
fragile. Extreme reviewer attention to non-online alterations to the
|
|
DB schema will help us avoid disaster.
|
|
* Dropping things from the schema is a thing we need to be extremely
|
|
careful about, making sure that the column has not been used (even
|
|
present in one of our models) for at least a release.
|
|
* Data migrations must not be present in schema migrations. If data
|
|
needs to be converted to another format, or moved from one place to
|
|
another, then that must be done while the database server remains
|
|
online. Generally, this can and should be hidden within the object
|
|
layer so that an object can load from either the old or new
|
|
location, and save to the new one.
|
|
|
|
Config Options
|
|
==============
|
|
|
|
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 shows the impact and scope. The operators should
|
|
know the expected change in the behavior of Nova if they tweak this.
|
|
|
|
* Hints which services will consume this config option. Operators/Deployers
|
|
should not be forced to read the code to know which one of the services will
|
|
change its behavior nor should they set this in every ``nova.conf`` file to
|
|
be sure.
|
|
|
|
* Descriptions 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, lookout for regex validations.
|
|
|
|
* Interdependencies to other options. If other config options have to be
|
|
considered when this config option gets changed, is this described?
|