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 |
Matt Riedemann | 9b6d2f2 | 2019-06-18 10:43:16 -0400 | [diff] [blame] | 14 | https://opendev.org/openstack/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 |
Matt Riedemann | 9b6d2f2 | 2019-06-18 10:43:16 -0400 | [diff] [blame] | 29 | queue <https://review.opendev.org/#/q/project:openstack/devstack>`__ |
Ian Wienand | 6f6e2fd | 2015-03-20 12:16:28 +1100 | [diff] [blame] | 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. |
Stephen Finucane | 4b8cba7 | 2019-05-21 14:17:11 +0100 | [diff] [blame] | 148 | ``VOLUME_BACKING_FILE_SIZE`` (nova-compute and cinder) or |
| 149 | ``PUBLIC_NETWORK_NAME`` (only neutron but formerly nova-network too) |
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. |