| Dean Troyer | 07c3557 | 2012-03-05 07:15:30 -0600 | [diff] [blame] | 1 | Contributing to DevStack | 
 | 2 | ======================== | 
 | 3 |  | 
 | 4 |  | 
 | 5 | General | 
 | 6 | ------- | 
 | 7 |  | 
| Dean Troyer | b8dd27b | 2013-10-17 12:03:55 -0500 | [diff] [blame] | 8 | DevStack is written in UNIX shell script.  It uses a number of bash-isms | 
| Dean Troyer | d97ee30 | 2015-02-04 12:35:39 -0600 | [diff] [blame] | 9 | and so is limited to Bash (version 4 and up) and compatible shells. | 
| Dean Troyer | b8dd27b | 2013-10-17 12:03:55 -0500 | [diff] [blame] | 10 | Shell script was chosen because it best illustrates the steps used to | 
 | 11 | set up and interact with OpenStack components. | 
| Dean Troyer | 07c3557 | 2012-03-05 07:15:30 -0600 | [diff] [blame] | 12 |  | 
| Adrien Cunin | eaff3e1 | 2014-10-21 13:46:54 +0200 | [diff] [blame] | 13 | DevStack's official repository is located on git.openstack.org at | 
 | 14 | https://git.openstack.org/openstack-dev/devstack.  Besides the master branch that | 
| Dean Troyer | 07c3557 | 2012-03-05 07:15:30 -0600 | [diff] [blame] | 15 | tracks the OpenStack trunk branches a separate branch is maintained for all | 
 | 16 | OpenStack releases starting with Diablo (stable/diablo). | 
 | 17 |  | 
| Dean Troyer | 6d04fd7 | 2012-12-21 11:03:37 -0600 | [diff] [blame] | 18 | Contributing code to DevStack follows the usual OpenStack process as described | 
 | 19 | in `How To Contribute`__ in the OpenStack wiki.  `DevStack's LaunchPad project`__ | 
| Roger Luethi | c21cfc9 | 2014-07-22 15:35:02 +0200 | [diff] [blame] | 20 | contains the usual links for blueprints, bugs, etc. | 
| Dean Troyer | 6d04fd7 | 2012-12-21 11:03:37 -0600 | [diff] [blame] | 21 |  | 
 | 22 | __ contribute_ | 
| zhangbailin | c63d933 | 2017-09-01 19:46:16 -0700 | [diff] [blame] | 23 | .. _contribute: https://docs.openstack.org/infra/manual/developers.html | 
| Dean Troyer | 6d04fd7 | 2012-12-21 11:03:37 -0600 | [diff] [blame] | 24 |  | 
 | 25 | __ lp_ | 
 | 26 | .. _lp: https://launchpad.net/~devstack | 
 | 27 |  | 
| Ian Wienand | 6f6e2fd | 2015-03-20 12:16:28 +1100 | [diff] [blame] | 28 | The `Gerrit review | 
 | 29 | queue <https://review.openstack.org/#/q/project:openstack-dev/devstack,n,z>`__ | 
 | 30 | is used for all commits. | 
 | 31 |  | 
| Dean Troyer | 07c3557 | 2012-03-05 07:15:30 -0600 | [diff] [blame] | 32 | The primary script in DevStack is ``stack.sh``, which performs the bulk of the | 
 | 33 | work for DevStack's use cases.  There is a subscript ``functions`` that contains | 
 | 34 | generally useful shell functions and is used by a number of the scripts in | 
 | 35 | DevStack. | 
 | 36 |  | 
 | 37 | A number of additional scripts can be found in the ``tools`` directory that may | 
| Dean Troyer | cc6b443 | 2013-04-08 15:38:03 -0500 | [diff] [blame] | 38 | be useful in supporting DevStack installations.  Of particular note are ``info.sh`` | 
| Adam Spiers | d903476 | 2013-10-04 23:20:24 +0100 | [diff] [blame] | 39 | to collect and report information about the installed system, and ``install_prereqs.sh`` | 
| Dean Troyer | cc6b443 | 2013-04-08 15:38:03 -0500 | [diff] [blame] | 40 | that handles installation of the prerequisite packages for DevStack.  It is | 
 | 41 | suitable, for example, to pre-load a system for making a snapshot. | 
| Dean Troyer | 07c3557 | 2012-03-05 07:15:30 -0600 | [diff] [blame] | 42 |  | 
| Ian Wienand | 6f6e2fd | 2015-03-20 12:16:28 +1100 | [diff] [blame] | 43 | Repo Layout | 
 | 44 | ----------- | 
 | 45 |  | 
 | 46 | The DevStack repo generally keeps all of the primary scripts at the root | 
 | 47 | level. | 
 | 48 |  | 
 | 49 | ``doc`` - Contains the Sphinx source for the documentation. | 
| Chang Liu | 2c42fd0 | 2018-08-03 16:15:20 +0800 | [diff] [blame] | 50 | A complete doc build can be run with ``tox -edocs``. | 
| Ian Wienand | 6f6e2fd | 2015-03-20 12:16:28 +1100 | [diff] [blame] | 51 |  | 
| Ian Wienand | 6f6e2fd | 2015-03-20 12:16:28 +1100 | [diff] [blame] | 52 | ``extras.d`` - Contains the dispatch scripts called by the hooks in | 
 | 53 | ``stack.sh``, ``unstack.sh`` and ``clean.sh``. See :doc:`the plugins | 
 | 54 | docs <plugins>` for more information. | 
 | 55 |  | 
 | 56 | ``files`` - Contains a variety of otherwise lost files used in | 
 | 57 | configuring and operating DevStack. This includes templates for | 
 | 58 | configuration files and the system dependency information. This is also | 
 | 59 | where image files are downloaded and expanded if necessary. | 
 | 60 |  | 
 | 61 | ``lib`` - Contains the sub-scripts specific to each project. This is | 
 | 62 | where the work of managing a project's services is located. Each | 
 | 63 | top-level project (Keystone, Nova, etc) has a file here. Additionally | 
 | 64 | there are some for system services and project plugins.  These | 
 | 65 | variables and functions are also used by related projects, such as | 
 | 66 | Grenade, to manage a DevStack installation. | 
 | 67 |  | 
 | 68 | ``samples`` - Contains a sample of the local files not included in the | 
 | 69 | DevStack repo. | 
 | 70 |  | 
 | 71 | ``tests`` - the DevStack test suite is rather sparse, mostly consisting | 
 | 72 | of test of specific fragile functions in the ``functions`` and | 
 | 73 | ``functions-common`` files. | 
 | 74 |  | 
 | 75 | ``tools`` - Contains a collection of stand-alone scripts. While these | 
 | 76 | may reference the top-level DevStack configuration they can generally be | 
 | 77 | run alone. There are also some sub-directories to support specific | 
 | 78 | environments such as XenServer. | 
 | 79 |  | 
| Dean Troyer | 07c3557 | 2012-03-05 07:15:30 -0600 | [diff] [blame] | 80 |  | 
 | 81 | Scripts | 
 | 82 | ------- | 
 | 83 |  | 
 | 84 | DevStack scripts should generally begin by calling ``env(1)`` in the shebang line:: | 
 | 85 |  | 
 | 86 |     #!/usr/bin/env bash | 
 | 87 |  | 
 | 88 | Sometimes the script needs to know the location of the DevStack install directory. | 
 | 89 | ``TOP_DIR`` should always point there, even if the script itself is located in | 
 | 90 | a subdirectory:: | 
 | 91 |  | 
| Dean Troyer | b8dd27b | 2013-10-17 12:03:55 -0500 | [diff] [blame] | 92 |     # Keep track of the current DevStack directory. | 
| Dean Troyer | 07c3557 | 2012-03-05 07:15:30 -0600 | [diff] [blame] | 93 |     TOP_DIR=$(cd $(dirname "$0") && pwd) | 
 | 94 |  | 
 | 95 | Many scripts will utilize shared functions from the ``functions`` file.  There are | 
 | 96 | also rc files (``stackrc`` and ``openrc``) that are often included to set the primary | 
 | 97 | configuration of the user environment:: | 
 | 98 |  | 
| Dean Troyer | b8dd27b | 2013-10-17 12:03:55 -0500 | [diff] [blame] | 99 |     # Keep track of the current DevStack directory. | 
| Dean Troyer | 51fb454 | 2012-03-09 22:21:59 -0600 | [diff] [blame] | 100 |     TOP_DIR=$(cd $(dirname "$0") && pwd) | 
| Dean Troyer | 07c3557 | 2012-03-05 07:15:30 -0600 | [diff] [blame] | 101 |  | 
 | 102 |     # Import common functions | 
| Dean Troyer | 51fb454 | 2012-03-09 22:21:59 -0600 | [diff] [blame] | 103 |     source $TOP_DIR/functions | 
| Dean Troyer | 07c3557 | 2012-03-05 07:15:30 -0600 | [diff] [blame] | 104 |  | 
 | 105 |     # Import configuration | 
| Dean Troyer | 51fb454 | 2012-03-09 22:21:59 -0600 | [diff] [blame] | 106 |     source $TOP_DIR/openrc | 
| Dean Troyer | 07c3557 | 2012-03-05 07:15:30 -0600 | [diff] [blame] | 107 |  | 
 | 108 | ``stack.sh`` is a rather large monolithic script that flows through from beginning | 
| Dean Troyer | cc6b443 | 2013-04-08 15:38:03 -0500 | [diff] [blame] | 109 | to end.  It has been broken down into project-specific subscripts (as noted above) | 
 | 110 | located in ``lib`` to make ``stack.sh`` more manageable and to promote code reuse. | 
| Dean Troyer | 05f2365 | 2012-08-29 15:20:21 -0500 | [diff] [blame] | 111 |  | 
 | 112 | These library sub-scripts have a number of fixed entry points, some of which may | 
 | 113 | just be stubs.  These entry points will be called by ``stack.sh`` in the | 
 | 114 | following order:: | 
 | 115 |  | 
 | 116 |     install_XXXX | 
 | 117 |     configure_XXXX | 
 | 118 |     init_XXXX | 
 | 119 |     start_XXXX | 
 | 120 |     stop_XXXX | 
 | 121 |     cleanup_XXXX | 
 | 122 |  | 
 | 123 | There is a sub-script template in ``lib/templates`` to be used in creating new | 
 | 124 | service sub-scripts.  The comments in ``<>`` are meta comments describing | 
 | 125 | how to use the template and should be removed. | 
| Dean Troyer | 07c3557 | 2012-03-05 07:15:30 -0600 | [diff] [blame] | 126 |  | 
| Dean Troyer | 6d04fd7 | 2012-12-21 11:03:37 -0600 | [diff] [blame] | 127 | In order to show the dependencies and conditions under which project functions | 
 | 128 | are executed the top-level conditional testing for things like ``is_service_enabled`` | 
 | 129 | should be done in ``stack.sh``.  There may be nested conditionals that need | 
 | 130 | to be in the sub-script, such as testing for keystone being enabled in | 
 | 131 | ``configure_swift()``. | 
 | 132 |  | 
| Dean Troyer | 07c3557 | 2012-03-05 07:15:30 -0600 | [diff] [blame] | 133 |  | 
| Dean Troyer | 2b7ce5a | 2013-01-10 13:22:45 -0600 | [diff] [blame] | 134 | stackrc | 
 | 135 | ------- | 
 | 136 |  | 
 | 137 | ``stackrc`` is the global configuration file for DevStack.  It is responsible for | 
| Dean Troyer | b8dd27b | 2013-10-17 12:03:55 -0500 | [diff] [blame] | 138 | calling ``local.conf`` (or ``localrc`` if it exists) so local user configuration | 
 | 139 | is recognized. | 
| Dean Troyer | 2b7ce5a | 2013-01-10 13:22:45 -0600 | [diff] [blame] | 140 |  | 
 | 141 | The criteria for what belongs in ``stackrc`` can be vaguely summarized as | 
 | 142 | follows: | 
 | 143 |  | 
| Dean Troyer | b8dd27b | 2013-10-17 12:03:55 -0500 | [diff] [blame] | 144 | * All project repositories and branches handled directly in ``stack.sh`` | 
 | 145 | * Global configuration that may be referenced in ``local.conf``, i.e. ``DEST``, ``DATA_DIR`` | 
| Dean Troyer | 2b7ce5a | 2013-01-10 13:22:45 -0600 | [diff] [blame] | 146 | * Global service configuration like ``ENABLED_SERVICES`` | 
 | 147 | * Variables used by multiple services that do not have a clear owner, i.e. | 
| Daniel Genin | d470867 | 2014-10-31 15:01:29 -0400 | [diff] [blame] | 148 |   ``VOLUME_BACKING_FILE_SIZE`` (nova-compute, nova-volumes and cinder) or | 
 | 149 |   ``PUBLIC_NETWORK_NAME`` (nova-network and neutron) | 
| Dean Troyer | 2b7ce5a | 2013-01-10 13:22:45 -0600 | [diff] [blame] | 150 | * Variables that can not be cleanly declared in a project file due to | 
 | 151 |   dependency ordering, i.e. the order of sourcing the project files can | 
 | 152 |   not be changed for other reasons but the earlier file needs to dereference a | 
 | 153 |   variable set in the later file.  This should be rare. | 
 | 154 |  | 
| Dean Troyer | b8dd27b | 2013-10-17 12:03:55 -0500 | [diff] [blame] | 155 | Also, variable declarations in ``stackrc`` before ``local.conf`` is sourced | 
 | 156 | do NOT allow overriding (the form | 
 | 157 | ``FOO=${FOO:-baz}``); if they did then they can already be changed in ``local.conf`` | 
| Dean Troyer | 2b7ce5a | 2013-01-10 13:22:45 -0600 | [diff] [blame] | 158 | and can stay in the project file. | 
 | 159 |  | 
| Dean Troyer | cc6b443 | 2013-04-08 15:38:03 -0500 | [diff] [blame] | 160 |  | 
| Dean Troyer | 07c3557 | 2012-03-05 07:15:30 -0600 | [diff] [blame] | 161 | Documentation | 
 | 162 | ------------- | 
 | 163 |  | 
| Dean Troyer | f5cb1ce | 2014-10-21 11:16:58 -0500 | [diff] [blame] | 164 | The DevStack repo now contains all of the static pages of devstack.org in | 
 | 165 | the ``doc/source`` directory. The OpenStack CI system rebuilds the docs after every | 
 | 166 | commit and updates devstack.org (now a redirect to docs.openstack.org/developer/devstack). | 
| Dean Troyer | 07c3557 | 2012-03-05 07:15:30 -0600 | [diff] [blame] | 167 |  | 
 | 168 | All of the scripts are processed with shocco_ to render them with the comments | 
 | 169 | as text describing the script below.  For this reason we tend to be a little | 
 | 170 | verbose in the comments _ABOVE_ the code they pertain to.  Shocco also supports | 
 | 171 | Markdown formatting in the comments; use it sparingly.  Specifically, ``stack.sh`` | 
 | 172 | uses Markdown headers to divide the script into logical sections. | 
 | 173 |  | 
| Dean Troyer | b8dd27b | 2013-10-17 12:03:55 -0500 | [diff] [blame] | 174 | .. _shocco: https://github.com/dtroyer/shocco/tree/rst_support | 
 | 175 |  | 
 | 176 | The script used to drive <code>shocco</code> is <code>tools/build_docs.sh</code>. | 
| Dean Troyer | f5cb1ce | 2014-10-21 11:16:58 -0500 | [diff] [blame] | 177 | The complete docs build is also handled with <code>tox -edocs</code> per the | 
 | 178 | OpenStack project standard. | 
| Dean Troyer | 07c3557 | 2012-03-05 07:15:30 -0600 | [diff] [blame] | 179 |  | 
 | 180 |  | 
| Sean Dague | 6db2892 | 2013-11-22 12:16:02 -0500 | [diff] [blame] | 181 | Bash Style Guidelines | 
 | 182 | ~~~~~~~~~~~~~~~~~~~~~ | 
| Roger Luethi | 473b628 | 2014-04-13 13:47:42 +0200 | [diff] [blame] | 183 | DevStack defines a bash set of best practices for maintaining large | 
| Sean Dague | 6db2892 | 2013-11-22 12:16:02 -0500 | [diff] [blame] | 184 | collections of bash scripts. These should be considered as part of the | 
 | 185 | review process. | 
 | 186 |  | 
| Dean Troyer | f5cb1ce | 2014-10-21 11:16:58 -0500 | [diff] [blame] | 187 | DevStack uses the bashate_ style checker | 
 | 188 | to enforce basic guidelines, similar to pep8 and flake8 tools for Python. The | 
 | 189 | list below is not complete for what bashate checks, nor is it all checked | 
 | 190 | by bashate.  So many lines of code, so little time. | 
 | 191 |  | 
 | 192 | .. _bashate: https://pypi.python.org/pypi/bashate | 
| Sean Dague | 6db2892 | 2013-11-22 12:16:02 -0500 | [diff] [blame] | 193 |  | 
 | 194 | Whitespace Rules | 
 | 195 | ---------------- | 
 | 196 |  | 
 | 197 | - lines should not include trailing whitespace | 
 | 198 | - there should be no hard tabs in the file | 
 | 199 | - indents are 4 spaces, and all indentation should be some multiple of | 
 | 200 |   them | 
 | 201 |  | 
 | 202 | Control Structure Rules | 
 | 203 | ----------------------- | 
| Ian Wienand | 6f6e2fd | 2015-03-20 12:16:28 +1100 | [diff] [blame] | 204 |  | 
| Sean Dague | 6db2892 | 2013-11-22 12:16:02 -0500 | [diff] [blame] | 205 | - then should be on the same line as the if | 
 | 206 | - do should be on the same line as the for | 
 | 207 |  | 
 | 208 | Example:: | 
 | 209 |  | 
 | 210 |   if [[ -r $TOP_DIR/local.conf ]]; then | 
 | 211 |       LRC=$(get_meta_section_files $TOP_DIR/local.conf local) | 
 | 212 |       for lfile in $LRC; do | 
 | 213 |           if [[ "$lfile" == "localrc" ]]; then | 
 | 214 |               if [[ -r $TOP_DIR/localrc ]]; then | 
 | 215 |                   warn $LINENO "localrc and local.conf:[[local]] both exist, using localrc" | 
 | 216 |               else | 
 | 217 |                   echo "# Generated file, do not edit" >$TOP_DIR/.localrc.auto | 
 | 218 |                   get_meta_section $TOP_DIR/local.conf local $lfile >>$TOP_DIR/.localrc.auto | 
 | 219 |               fi | 
 | 220 |           fi | 
 | 221 |       done | 
 | 222 |   fi | 
 | 223 |  | 
 | 224 | Variables and Functions | 
 | 225 | ----------------------- | 
| Ian Wienand | 6f6e2fd | 2015-03-20 12:16:28 +1100 | [diff] [blame] | 226 |  | 
| Sean Dague | 6db2892 | 2013-11-22 12:16:02 -0500 | [diff] [blame] | 227 | - functions should be used whenever possible for clarity | 
 | 228 | - functions should use ``local`` variables as much as possible to | 
 | 229 |   ensure they are isolated from the rest of the environment | 
 | 230 | - local variables should be lower case, global variables should be | 
 | 231 |   upper case | 
 | 232 | - function names should_have_underscores, NotCamelCase. | 
| Ian Wienand | aee18c7 | 2014-02-21 15:35:08 +1100 | [diff] [blame] | 233 | - functions should be declared as per the regex ^function foo {$ | 
 | 234 |   with code starting on the next line | 
| Ian Wienand | c7df4df | 2015-03-20 12:18:52 +1100 | [diff] [blame] | 235 |  | 
 | 236 |  | 
 | 237 | Review Criteria | 
| Clark Boylan | 2a6112e | 2017-05-12 10:16:33 -0700 | [diff] [blame] | 238 | --------------- | 
| Ian Wienand | c7df4df | 2015-03-20 12:18:52 +1100 | [diff] [blame] | 239 |  | 
 | 240 | There are some broad criteria that will be followed when reviewing | 
 | 241 | your change | 
 | 242 |  | 
 | 243 | * **Is it passing tests** -- your change will not be reviewed | 
| Swapnil Kulkarni (coolsvap) | 7f0be4f | 2015-11-20 10:52:59 +0530 | [diff] [blame] | 244 |   thoroughly unless the official CI has run successfully against it. | 
| Ian Wienand | c7df4df | 2015-03-20 12:18:52 +1100 | [diff] [blame] | 245 |  | 
 | 246 | * **Does this belong in DevStack** -- DevStack reviewers have a | 
 | 247 |   default position of "no" but are ready to be convinced by your | 
 | 248 |   change. | 
 | 249 |  | 
 | 250 |   For very large changes, you should consider :doc:`the plugins system | 
 | 251 |   <plugins>` to see if your code is better abstracted from the main | 
 | 252 |   repository. | 
 | 253 |  | 
 | 254 |   For smaller changes, you should always consider if the change can be | 
 | 255 |   encapsulated by per-user settings in ``local.conf``.  A common example | 
 | 256 |   is adding a simple config-option to an ``ini`` file.  Specific flags | 
 | 257 |   are not usually required for this, although adding documentation | 
 | 258 |   about how to achieve a larger goal (which might include turning on | 
 | 259 |   various settings, etc) is always welcome. | 
 | 260 |  | 
 | 261 | * **Work-arounds** -- often things get broken and DevStack can be in a | 
 | 262 |   position to fix them.  Work-arounds are fine, but should be | 
 | 263 |   presented in the context of fixing the root-cause of the problem. | 
 | 264 |   This means it is well-commented in the code and the change-log and | 
 | 265 |   mostly likely includes links to changes or bugs that fix the | 
 | 266 |   underlying problem. | 
 | 267 |  | 
 | 268 | * **Should this be upstream** -- DevStack generally does not override | 
 | 269 |   default choices provided by projects and attempts to not | 
| Atsushi SAKAI | 2040143 | 2015-07-27 20:42:44 +0900 | [diff] [blame] | 270 |   unexpectedly modify behavior. | 
| Ian Wienand | c7df4df | 2015-03-20 12:18:52 +1100 | [diff] [blame] | 271 |  | 
 | 272 | * **Context in commit messages** -- DevStack touches many different | 
 | 273 |   areas and reviewers need context around changes to make good | 
 | 274 |   decisions.  We also always want it to be clear to someone -- perhaps | 
 | 275 |   even years from now -- why we were motivated to make a change at the | 
 | 276 |   time. | 
 | 277 |  | 
 | 278 | * **Reviewers** -- please see ``MAINTAINERS.rst`` for a list of people | 
 | 279 |   that should be added to reviews of various sub-systems. | 
| Clark Boylan | 2a6112e | 2017-05-12 10:16:33 -0700 | [diff] [blame] | 280 |  | 
 | 281 |  | 
 | 282 | Making Changes, Testing, and CI | 
 | 283 | ------------------------------- | 
 | 284 |  | 
 | 285 | Changes to Devstack are tested by automated continuous integration jobs | 
 | 286 | that run on a variety of Linux Distros using a handful of common | 
 | 287 | configurations. What this means is that every change to Devstack is | 
 | 288 | self testing. One major benefit of this is that developers do not | 
 | 289 | typically need to add new non voting test jobs to add features to | 
 | 290 | Devstack. Instead the features can be added, then if testing passes | 
 | 291 | with the feature enabled the change is ready to merge (pending code | 
 | 292 | review). | 
 | 293 |  | 
 | 294 | A concrete example of this was the switch from screen based service | 
 | 295 | management to systemd based service management. No new jobs were | 
 | 296 | created for this. Instead the features were added to devstack, tested | 
 | 297 | locally and in CI using a change that enabled the feature, then once | 
 | 298 | the enabling change was passing and the new behavior communicated and | 
 | 299 | documented it was merged. | 
 | 300 |  | 
 | 301 | Using this process has been proven to be effective and leads to | 
 | 302 | quicker implementation of desired features. |