From 57ea996bb1fc3cb16ff19ff9917c8395fe0b4caa Mon Sep 17 00:00:00 2001 From: Sean McGinnis Date: Fri, 31 May 2019 15:53:14 -0500 Subject: [PATCH] Docs: Add new driver review checklist This is the start of a review checklist for new drivers to help the driver contributors and the core reviewers to watch for some common issues we've seen. Change-Id: Ib676658b00cf43e07daf8f4af9b2ee0c7c17cb17 Signed-off-by: Sean McGinnis --- doc/source/contributor/drivers.rst | 12 ++ .../contributor/new_driver_checklist.rst | 138 ++++++++++++++++++ 2 files changed, 150 insertions(+) create mode 100644 doc/source/contributor/new_driver_checklist.rst diff --git a/doc/source/contributor/drivers.rst b/doc/source/contributor/drivers.rst index ddc4af9acfc..ff85ecc26c3 100644 --- a/doc/source/contributor/drivers.rst +++ b/doc/source/contributor/drivers.rst @@ -113,6 +113,18 @@ tests. The details for the required volume driver interfaces can be found in the ``cinder/interface/volume_*_driver.py`` source. +New Driver Review Checklist +--------------------------- + +There are some common issues caught during the review of new driver patches +that can easily be avoided. New driver maintainers should review the +:doc:`new_driver_checklist` for some things to watch out for. + +.. toctree:: + :hidden: + + new_driver_checklist + Driver Development Documentations --------------------------------- diff --git a/doc/source/contributor/new_driver_checklist.rst b/doc/source/contributor/new_driver_checklist.rst new file mode 100644 index 00000000000..edf0f43e3c7 --- /dev/null +++ b/doc/source/contributor/new_driver_checklist.rst @@ -0,0 +1,138 @@ +=========================== +New Driver Review Checklist +=========================== + +Reviewers can use this list for some common things to watch for when doing new +driver reviews. This list is by no means exhaustive, but does try to capture +some of things that have been found in past reviews. + +.. note:: + + Feel free to propose additional items to help make this a more complete + list. + +Review Checklist +---------------- + +* Driver Code + + * Passing all gate tests + * Driver keeps all configuration in ``cinder.conf`` and not in separate + vendor specific config file. + + * xml files for configs are forbidden + +* Common gotchas + + * Handles detach where ``connector == None`` for force detach + * Create from snapshot and clone properly account for new volume size being + larger than original volume size + * Volume not found in delete calls should return success + * Ensure proper code format w/ pep8 (``tox -e pep8``), but start here first: + https://docs.openstack.org/hacking/latest/user/hacking.html + + * ``tox -e fast8`` can be used as a quick check only against modified files + + + * Unit tests included for all but trivial code in driver + * All source code files contain Apache 2 copyright header + + * Stating copyright for vendor is optional + * Don't attribute copyright to the OpenStack Foundation + + * Run ``tox -e compliance`` to make sure all required interfaces are + implemented. + * Required in driver: + + * Concrete driver implementation has decorator ``@interface.volumedriver`` + * ``VERSION`` constant defined in driver class + * ``CI_WIKI_NAME`` constant defined in driver class + * well documented version history in the comment block for the main driver + class. + * Support minimum driver features + (https://docs.openstack.org/cinder/latest/contributor/drivers.html#minimum-features ) + * Meet release deadline(s) + (https://releases.openstack.org/train/schedule.html#t-cinder-driver-deadline ) + + * Driver does not add unnecessary new config options + + * For example, adding vendor_username instead of using the common san_login + + * Driver specific exceptions inherit from ``VolumeDriverException`` or + ``VolumeBackendAPIException`` + + * Exceptions should be defined with driver code + + * Logging level is appropriate for content + + * General tracing should be at debug level + * Things operators should be aware of should be at Info level + * Issues that are of concern but may not have an impact on actual operation + should be warning + * Issues operators need to take action on or should definitely know about + should be ERROR + * Messages about a failure should include the snapshot or volume in + question. + + * All exception messages that could be raised to users should be marked for + translation with _() + * Any additional libraries needed for a driver must be added to the global + requirements. + + * https://wiki.openstack.org/wiki/Requirements#Adding_a_Requirement_to_an_OpenStack_Project + * Pypi installable libraries should be added to driver section in setup.cfg + * Binary dependencies need to be OSI licensed and added to bindep.txt + + * Third Party CI checks + + * Responds correctly to recheck from "run-" + * Tempest run console log available + * ``cinder.conf`` and all cinder service logs available + * LVM driver is not being configured in ``local.conf/cinder.conf`` + * Only the driver in question should be in ``cinder.conf`` and enabled + + * ``default_volume_type`` and ``enabled_backends`` in ``cinder.conf``, OR + * ``CINDER_DEFAULT_VOLUME_TYPE`` and ``CINDER_ENABLED_BACKENDS`` in + ``local.conf``, OR + * ``TEMPEST_VOLUME_DRIVER`` and ``TEMPEST_VOLUME_VENDER`` in + ``local.conf`` + + * specify correct patch for each CI run + + * ``CINDER_BRANCH`` in ``local.conf``, OR + * ``git fetch https://review.opendev.org/openstack/cinder refs/changes/56/657856/2 && git checkout cherry-pick`` + (https://wiki.openstack.org/wiki/Cinder/tested-3rdParty-drivers ) + + * CI runs ``tox -e all -- *volume*`` + + * Any skipped tests need to be clearly documented why they are being + skipped including the plan for getting rid of the need to skip them. + * https://opendev.org/openstack/cinder-tempest-plugin needs to be installed + so those tempest tests run as well. + * ``tox`` | ``tempest`` with ``--subunit`` helps generate HTML output + (https://docs.openstack.org/os-testr/latest/user/subunit2html.html ) + * ``tox`` | ``tempest`` with ``--concurrency=`` for specifying ```` + number of test runners + + * CI must run Cinder services using Python 3.7. + * CI does not report failures or exception due to the CI operation and not + due to test failures due to code changes. + * *optional, but highly recommended:* CI only runs on third party CI recheck + trigger or on successful +1 from Zuul. + * CI only runs on patches to the master branch unless they are intentionally + set up to be able to properly run stable branch testing. + +* Included with driver patch + + * Release note stating something like "New volume driver added for Blah blah + blah storage" + + * See Reno usage information here: + https://docs.openstack.org/reno/latest/user/usage.html + + * Driver added to ``doc/source/reference/support-matrix.ini`` and + ``doc/source/reference/support-matrix.rst`` + * Driver configuration information added under + ``doc/source/configuration/block-storage/drivers`` + * Update ``cinder/opts.py`` including the new driver library options using + the command ``tox -e genopts``