]> git.apps.os.sepia.ceph.com Git - ceph-ci.git/commitdiff
doc: detailed description of bugfixing workflow
authorNathan Cutler <ncutler@suse.com>
Sat, 5 Mar 2016 04:53:38 +0000 (05:53 +0100)
committerNathan Cutler <ncutler@suse.com>
Sat, 5 Mar 2016 14:54:09 +0000 (15:54 +0100)
Signed-off-by: Nathan Cutler <ncutler@suse.com>
doc/dev/index.rst

index 6a726963bbb036638c9f3088705149d07d6dadea..f65071b75d6ef03bb0ee906232cd39576ccead9a 100644 (file)
@@ -98,8 +98,8 @@ knowledge of git_ is essential.
 Although the `Ceph "organization"`_ includes several software repositories,
 this document covers only one: https://github.com/ceph/ceph.
 
-Issue tracker
--------------
+Redmine issue tracker
+---------------------
 
 Although `GitHub`_ is used for code, Ceph-related issues (Bugs, Features,
 Backports, Documentation, etc.) are tracked at http://tracker.ceph.com,
@@ -179,20 +179,37 @@ with:
     ./ceph health
 
 
-Basic workflow
-==============
+Issue tracker
+=============
 
-.. epigraph::
+See `Redmine issue tracker`_ for a brief introduction to the Ceph Issue Tracker.
 
-    Without bugs, there would be no software, and without software, there would
-    be no software developers.
+Issue tracker conventions
+-------------------------
 
-    --Unknown
+When you start working on an existing issue, it's nice to let the other
+developers know this - to avoid duplication of labor. Typically, this is
+done by changing the :code:`Assignee` field (to yourself) and changing the
+:code:`Status` to *In progress*. Newcomers to the Ceph community typically do not
+have sufficient privileges to update these fields, however: they can
+simply update the issue with a brief note.
 
-Having already introduced the `Issue tracker`_ and the `Source code
-repositories`_, and having touched upon `Submitting patches`_, we now
-describe these in more detail in the context of basic Ceph development
-workflows.
+.. table:: Meanings of some commonly used statuses
+
+   ================ ===========================================
+   Status           Meaning
+   ================ ===========================================
+   New              Initial status
+   In Progress      Somebody is working on it
+   Need Review      Pull request is open with a fix
+   Pending Backport Fix has been merged, backport(s) pending
+   Resolved         Fix and backports (if any) have been merged
+   ================ ===========================================
+
+Basic workflow
+==============
+
+The following chart illustrates basic development workflow:
 
 .. ditaa::
 
@@ -210,50 +227,153 @@ workflows.
            | ceph--qa--suite|                       \-------------/
            \----------------/                        |
                 ^                                    | fix changes
-                |                                    | make check
-                | review                             | teuthology-suite
-                |                                    | git commit
+                |                                    | test changes
+                | review                             | git commit
+                |                                    | 
                 |                                    v
            /--------------\                        /-------------\
            |   github     |<---------------------- | ceph/fix_1  |
            | pull request |         git push       \-------------/
            \--------------/
 
+Below we present an explanation of this chart. The explanation is written
+with the assumption that you, the reader, are a beginning developer who
+has an idea for a bugfix, but do not know exactly how to proceed.
 
-Issue tracker conventions
--------------------------
+Update the tracker
+------------------
 
-When you start working on an existing issue, it's nice to let the other
-developers know this - to avoid duplication of labor. Typically, this is
-done by changing the :code:`Assignee` field (to yourself) and changing the
-:code:`Status` to *In progress*. Newcomers to the Ceph community typically do not
-have sufficient privileges to update these fields, however: they can
-simply update the issue with a brief note.
+Before you start, you should know the `Issue tracker`_ number of the bug
+you intend to fix. If there is no tracker issue, now is the time to create
+one. 
 
-.. table:: Meanings of some commonly used statuses
+The tracker is there to explain the issue (bug) to your fellow Ceph
+developers, so take care to provide a descriptive title as well as
+sufficient information and details in the description.
 
-   ================ ===========================================
-   Status           Meaning
-   ================ ===========================================
-   New              Initial status
-   In Progress      Somebody is working on it
-   Need Review      Pull request is open with a fix
-   Pending Backport Fix has been merged, backport(s) pending
-   Resolved         Fix and backports (if any) have been merged
-   ================ ===========================================
+If you have sufficient tracker permissions, assign the bug to yourself by
+changing the ``Assignee`` field.  If your tracker permissions have not yet
+been elevated, simply add a comment to the issue to let the other
+developers know you are working on the bug.
 
-Pull requests
+Upstream code
 -------------
 
-The Ceph source code is maintained in the `ceph/ceph repository` on
-`GitHub`_.
+This section, and the ones that follow, correspond to the nodes in the
+above chart.
+
+The upstream code lives in https://github.com/ceph/ceph.git, which is
+sometimes referred to as the "upstream repo", or simply "upstream". As the
+chart illustrates, we will make a local copy of this code, modify it, test
+our modifications, and submit the modifications back to the upstream repo
+for review.
+
+A local copy of the upstream code is made by 
+
+1. forking the upstream repo on GitHub, and 
+2. cloning your fork to make a local working copy
+
+See the `the GitHub documentation
+<https://help.github.com/articles/fork-a-repo/#platform-linux>`_ for
+detailed instructions on forking. In short, if your GitHub username is
+"mygithubaccount", your fork of the upstream repo will show up at
+https://github.com/mygithubaccount/ceph. Once you have created your fork,
+you clone it by doing:
+
+.. code::
+
+    $ git clone https://github.com/mygithubaccount/ceph
+
+While it is possible to clone the upstream repo directly, in this case we
+must fork it first, because that (forking) makes it possible to open a
+GitHub pull request.
+
+For more information on using GitHub, refer to `GitHub Help
+<https://help.github.com/>`_.
+
+Local environment
+-----------------
+
+In the local environment created in the previous step, we now have a
+copy of the ``master`` branch in ``remotes/origin/master``. Since the fork
+(https://github.com/mygithubaccount/ceph.git) is frozen in time and the
+upstream repo (https://github.com/ceph/ceph.git, typically abbreviated to
+``ceph/ceph.git``) is updated frequently by other developers, we will need
+to add the upstream repo as a "remote" so we can fetch from it:
+
+.. code::
+
+    $ git remote add ceph https://github.com/ceph/ceph.git
+    $ git fetch ceph
+
+After running these commands, all the branches from ``ceph/ceph.git`` are
+downloaded to the local git repo as ``remotes/ceph/$BRANCH_NAME`` and can be
+referenced as ``ceph/$BRANCH_NAME`` in certain git commands. 
+
+For example, your local ``master`` branch can be reset to the upstream Ceph
+``master`` branch by doing:
+
+.. code::
 
-.. _`ceph/ceph project on GitHub`: https://github.com/ceph/ceph
+    $ git fetch ceph
+    $ git checkout master
+    $ git reset --hard ceph/master
 
-The `GitHub`_ web interface provides a key feature for contributing code
-to the project: the *pull request*.
+The ``master`` branch of your fork can then be synced to upstream master by
+doing:
 
-Newcomers who are uncertain how to use pull requests may read
+.. code::
+
+    $ git push -u origin master
+
+Bugfix branch
+-------------
+
+Next, create a branch for the bugfix:
+
+.. code::
+
+    $ git checkout master
+    $ git branch -b fix_1
+    $ git push -u origin fix_1
+
+This creates a ``fix_1`` branch locally and in our GitHub fork. At this
+point, the ``fix_1`` branch is identical to the ``master`` branch, but not
+for long! You are now ready to modify the code.
+
+Fix bug locally
+---------------
+
+At this point, change the status of the tracker issue to "In progress" to
+communicate to the other Ceph developers that you have begun working on a
+fix. If you don't have permission to change that field, your comment that
+you are working on the issue is sufficient.
+
+In the best case, your fix is very simple and requires only minimal testing.
+In the typical worst case, fixing the bug is an iterative process involving
+trial and error, not to mention skill. Fixing bugs is beyond the scope
+of the current discussion.
+
+For a more detailed discussion of the tools available for validating your
+bugfixes, see the `Testing`_ chapter.
+
+For now, let us just assume that you have finished work on the bugfix and
+that you have tested it and believe it works. Commit the changes to your local
+branch and push the changes to your fork like so:
+
+.. code::
+
+    $ git push origin fix_1
+
+GitHub pull request
+-------------------
+
+The next step is to open a GitHub pull request. The purpose of this step is
+to make your bugfix available to the community of Ceph developers.
+Additional testing will be done on it, and it will undergo code review.
+In short, this is the point where you "go public" with your modifications.
+
+If you are uncertain how to use pull requests, you may read
 `this GitHub pull request tutorial`_.
 
 .. _`this GitHub pull request tutorial`:
@@ -265,21 +385,125 @@ the `Git Commit Good Practice`_ article at the `OpenStack Project Wiki`_.
 .. _`Git Commit Good Practice`: https://wiki.openstack.org/wiki/GitCommitMessages
 .. _`OpenStack Project Wiki`: https://wiki.openstack.org/wiki/Main_Page
 
+Once your pull request is opened, update the `Issue tracker`_ by adding a
+comment to the bug. The update can be as simple as:
+
+.. code::
+
+    *PR*: https://github.com/ceph/ceph/pull/$NUMBER_OF_YOUR_PULL_REQUEST
+
+Automated PR validation
+-----------------------
+
+When your PR hits GitHub, the Ceph project's `Continuous Integration (CI)
+<https://en.wikipedia.org/wiki/Continuous_integration>`_
+infrastructure will test it automatically. At the time of this writing
+(March 2016), the automated CI testing included a test to check that the
+commits in the PR are properly signed (see `Submitting patches`_) and a
+``make check`` test.
+
+The latter, ``make check``, builds the PR and runs it through a battery of
+tests. These tests run on machines operated by the Ceph Continuous
+Integration (CI) team. When the tests complete, the result will be shown
+on GitHub in the pull request itself.
+
+You can (and should) also test your modifications before you open a PR. 
+Refer to the the `Testing`_ chapter for details.
+
+Integration tests AKA ceph-qa-suite
+-----------------------------------
+
+Since Ceph is a complex beast, it may also be necessary to test your fix to
+see how it behaves on real clusters running either on real or virtual
+hardware. Tests designed for this purpose live in the `ceph-qa-suite
+repository`_ and are run via the `teuthology framework`_.
+
+.. _`ceph-qa-suite repository`: https://github.com/ceph/ceph-qa-suite/
+.. _`teuthology framework`: https://github.com/ceph/teuthology
+
+If you have access to an OpenStack tenant, you are encouraged to run the
+integration tests yourself using teuthology's OpenStack backend, called
+`teuthology-openstack
+<https://github.com/dachary/teuthology/tree/openstack#openstack-backend>`_,
+and post the test results to the PR.
 
-Make check tests
+The Ceph community also uses the `Sepia lab
+<http://ceph.github.io/sepia/>`_ where the integration tests can be run on
+real hardware. Other developers may add tags like "needs-qa" to your PR.
+This allows PRs that need testing to be merged into a single branch and
+tested all at the same time. Since teuthology suites can take hours
+(even days in some cases) to run, this can save a lot of time.
+
+Integration tests are discussed in more detail in the `Testing`_ chapter.
+
+Code review
+-----------
+
+Once your bugfix has been thoroughly tested, or even during this process,
+it will be subjected to code review by other developers. This typically
+takes the form of correspondence in the PR itself, but can be supplemented
+by discussions on `IRC`_ and the `Mailing list`_.
+
+Amending your PR
+----------------
+
+While your PR is going through `Testing`_ and `Code review`_, you can
+modify it at any time by editing files in your local branch.
+
+After the changes are committed locally (to the ``fix_1`` branch in our
+example), they need to be pushed to GitHub so they appear in the PR. If the
+changes involved modification of the git history (because of a ``git
+rebase`` or ``git commit --amend``), you will need to force push your branch
+with:
+
+.. code::
+
+    $ git push --force origin fix_1
+
+Merge
+-----
+
+The bugfixing process culminates when one of the project leads decides to
+merge your PR.
+
+
+Testing
+=======
+
+Ceph has two types of tests: "make check" tests and integration tests.
+The former are run via ``GNU Make <https://www.gnu.org/software/make/>``,
+and the latter are run via the `teuthology framework`_.
+
+Make check intro
 ----------------
 
-After compiling Ceph, the ``make check`` command can be used to run a
-series of tests. They cover various aspects of Ceph and each of them
-must:
+After compiling Ceph, the ``make check`` command can be used to run the
+code through a battery of tests covering various aspects of Ceph. For
+inclusion in "make check", a test must:
 
 * bind ports that do not conflict with other tests
 * not require root access
 * not require more than one machine to run
 * complete within a few minutes
 
-Integration tests
------------------
+While it is possible to run ``make check`` directly, it can be tricky to
+correctly set up your environment. Fortunately, a script is provided to
+make it easier run "make check" on your code. It can be run from the
+top-level directory of the Ceph source tree by doing:
+
+.. code::
+
+    $ ./run-make-check.sh
+
+You will need a minimum of 8GB of RAM and 32GB of free disk space for this
+command to complete successfully. Depending on your hardware, it can take
+from 20 minutes to three hours to complete, but it's worth the wait.
+
+When you fix a bug, it's a good idea to add a test. See the `Writing make
+check tests`_ chapter.
+
+Integration tests intro
+-----------------------
 
 When a test requires multiple machines, root access or lasts for a
 longer time (for example, to simulate a realistic Ceph deployment), it
@@ -289,32 +513,41 @@ framework`_.
 
 A number of integration tests are run on a regular basis against the
 official Ceph repositories (on the master development branch and the
-stable branches). The results are visible at `sepia
-<http://pulpito.ceph.com/>`_ and `sepia.ovh
-<http://pulpito.ovh.sepia.ceph.com:8081/>`_ and are also reported on
-the `ceph-qa mailing list <http://ceph.com/resources/mailing-list-irc/>`_
-for analysis.
+stable branches). Traditionally, these tests are called "the nightlies"
+because most of the Ceph developers used to live and work in California and
+from their perspective the tests were run overnight. 
+
+The results of the nightlies are visible at either http://pulpito.ceph.com/
+and http://pulpito.ovh.sepia.ceph.com:8081/ (depending on how the tests
+were run) and are also reported on the `ceph-qa mailing list
+<http://ceph.com/resources/mailing-list-irc/>`_ for analysis.
 
 Some Ceph developers have access to the hardware running these tests
 (either bare metal or OpenStack provisioned) and are allowed to
 schedule integration tests there (the developer nick shows in the test
 results URL).
 
-Ceph developers who have access to an OpenStack tenant can use the
-`ceph-workbench ceph-qa-suite`_ command to run integration tests and
-publish the results at http://teuthology-logs.public.ceph.com.  This
-allows reviewers to verify that changes to the code base do not cause
-regressions, or to analyze test failures when they do occur.
+Ceph developers who have access to an OpenStack tenant (could be the Sepia
+OVH one or any other) can use the `ceph-workbench ceph-qa-suite`_ command
+to run integration tests and publish the results at
+http://teuthology-logs.public.ceph.com. This allows reviewers to verify
+that changes to the code base do not cause regressions, or to analyze test
+failures when they do occur.
 
-.. _`ceph-qa-suite repository`: https://github.com/ceph/ceph-qa-suite
-.. _`teuthology framework`: https://github.com/ceph/teuthology
 .. _`ceph-workbench ceph-qa-suite`: http://ceph-workbench.readthedocs.org/
 
-Integration tests : ceph-qa-suite
-=================================
+Understanding make check tests
+==============================
+
+Principles of make check tests, where to find the results, how to interpret
+them, how to find the corresponding source code, how to write a make check
+test.
 
-This is an introduction to integration tests. A detailed description
-of each option is available from ``teuthology-suite --help``.
+Understanding integration tests
+===============================
+
+This is an introduction to integration tests. A detailed
+description of each option is available from ``teuthology-suite --help``.
 
 Reading a standalone integration test
 -------------------------------------
@@ -358,12 +591,14 @@ evaluated in order and runs the corresponding python file found in the
 task comes first and installs the Ceph packages on each machine (as
 defined by the ``roles`` array). A full description of the ``install``
 task is `found in the python file
-<https://github.com/ceph/teuthology/blob/master/teuthology/task/install.py#L1146>`_.
+<https://github.com/ceph/teuthology/blob/master/teuthology/task/install.py>`_
+(search for "def task").
 
 The `ceph task
 <https://github.com/ceph/ceph-qa-suite/blob/master/tasks/ceph.py#L1232>`_
-starts OSDs and MONs as required by the ``roles`` array. It will start
-one MON (``mon.a``) and two OSDs (``osd.0`` and ``osd.1``), on the same machine.
+starts OSDs and MONs as required by the ``roles`` array. In this example,
+it will start one MON (``mon.a``) and two OSDs (``osd.0`` and ``osd.1``),
+all on the same machine.
 
 Once the Ceph cluster is healthy, the `admin_socket task
 <https://github.com/ceph/ceph-qa-suite/blob/master/tasks/admin_socket.py#L18>`_
@@ -377,8 +612,8 @@ This test can be run with::
 
   teuthology-suite --suite rados/singleton/all/admin-socket.yaml
 
-How are tests built from directories ?
---------------------------------------
+How are tests built from directories?
+-------------------------------------
 
 Most tests are not a single file but the concatenation of files
 collected from a tree. For instance, the `ceph-disk suite
@@ -479,7 +714,7 @@ The ``--filter-out`` option does the opposite (it matches test that do
 not contain a given string), and can be combined with the ``--filter``
 option.
 
-Both --filter and --filter-out take a comma-separated list of strings (which
+Both ``--filter`` and ``--filter-out`` take a comma-separated list of strings (which
 means comma are implicitly forbidden in filenames found in the
 `ceph-qa-suite repository`_). For instance::
 
@@ -502,7 +737,7 @@ with the `fs directory <https://github.com/ceph/ceph-qa-suite/tree/master/suites
 
 All these tests are required before a Ceph release is published but it
 is too much when verifying a contribution can be merged without
-risking a trivial regression. The --subset option can be used to
+risking a trivial regression. The ``--subset`` option can be used to
 reduce the number of tests that are triggered. For instance::
 
   teuthology-suite --suite rados --subset 0/4000
@@ -522,11 +757,12 @@ The ``suites`` directory of the `ceph-qa-suite repository`_ contains
 all the integration tests, for all the Ceph components.
 
 `ceph-deploy <https://github.com/ceph/ceph-qa-suite/tree/master/suites/ceph-deploy>`_
-  install a Ceph cluster with `ceph-deploy`_
+  install a Ceph cluster with ``ceph-deploy`` (`ceph-deploy man page`_)
 
 `ceph-disk <https://github.com/ceph/ceph-qa-suite/tree/master/suites/ceph-disk>`_
   verify init scripts (upstart etc.) and udev integration with
-  `ceph-disk`_, with and without dmcrypt support.
+  ``ceph-disk`` (`ceph-disk man page`_), with and without `dmcrypt
+  <https://gitlab.com/cryptsetup/cryptsetup/wikis/DMCrypt>`_ support.
 
 `dummy <https://github.com/ceph/ceph-qa-suite/tree/master/suites/dummy>`_
   get a machine, do nothing and return success (commonly used to
@@ -566,9 +802,8 @@ all the integration tests, for all the Ceph components.
   for various versions of Ceph, verify that upgrades can happen
   without disrupting an ongoing workload
 
-.. _`ceph-qa-suite repository`: https://github.com/ceph/ceph-qa-suite/
-.. _`ceph-deploy`: ../../man/8/ceph-deploy
-.. _`ceph-disk`: ../../man/8/ceph-disk
+.. _`ceph-deploy man page`: ../../man/8/ceph-deploy
+.. _`ceph-disk man page`: ../../man/8/ceph-disk
 
 Architecture
 ============