mirror of https://github.com/ceph/ceph
385 lines
16 KiB
ReStructuredText
385 lines
16 KiB
ReStructuredText
Submitting Patches to Ceph - Backports
|
|
======================================
|
|
|
|
Most likely you're reading this because you intend to submit a GitHub pull
|
|
request ("PR") targeting one of the stable branches ("nautilus", etc.) at
|
|
https://github.com/ceph/ceph.
|
|
|
|
Before you open that PR, please read this entire document or, at the very least,
|
|
the following two sections: `General principles`_ and `Cherry-picking rules`_.
|
|
|
|
|
|
.. contents::
|
|
:depth: 3
|
|
|
|
|
|
General principles
|
|
------------------
|
|
|
|
To help the people who will review your backport, please state either in the
|
|
backport PR, or in the backport tracker issue, or in the master tracker issue:
|
|
|
|
1. what bug is fixed
|
|
2. why this fix is the minimal way to do it
|
|
3. why does this need to be fixed in <release>
|
|
|
|
The above should be followed especially in cases when the backport could be seen
|
|
as introducing, into a stable branch, code that is not related to a particular
|
|
bug or issue.
|
|
|
|
Rationale: every modification of a stable branch carries a certain risk of
|
|
introducing a regression. To minimize this risk, backports should be as
|
|
straightforward and narrowly-targeted as possible. As a stable release series
|
|
ages, the importance of following these general principles rises.
|
|
|
|
|
|
Cherry-picking rules
|
|
--------------------
|
|
|
|
The following rules, which have been codified from "best practices" developed
|
|
over years of backporting, apply to the actual backport implementation:
|
|
|
|
* all fixes should land in master first
|
|
* commits to stable branches should be cherry-picked from master
|
|
* before starting to cherry-pick a set of commits from master, grep the master git history for the SHA1 of each master commit (using ``git log --grep``) to check for follow-up fixes. Include any follow-up fixes found in the set of commits to be cherry-picked.
|
|
* cherry-picks must be done using ``git cherry-pick -x``
|
|
* if a commit could not be cherry-picked from master, the commit message must explain why that was not possible
|
|
* the commit message generated by ``git cherry-pick -x`` must not be modified, except to add a "Conflicts" section below the "cherry picked from commit ..." line added by git
|
|
* the "Conflicts" section must mention all files where changes had to be made manually (not just conflicts flagged by git)
|
|
* the "Conflicts" section should also describe the manual changes that were made
|
|
* if a change is to be backported to multiple stable branches, a tracker issue is needed, so the backports can be tracked (if a change is only to be backported to the most recent stable branch, a tracker issue is not strictly required)
|
|
|
|
For more information on tracker issues, see `Tracker workflow`_.
|
|
|
|
For more information on conflict resolution and writing the "Conflicts" section,
|
|
see `Conflict resolution`_.
|
|
|
|
Adhering to these rules will make your backport easier for reviewers to
|
|
understand. Not adhering to these rules creates additional work for reviewers
|
|
and may cause your backport PR to be rejected.
|
|
|
|
Notes on the cherry-picking rules
|
|
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
|
|
|
What does "all fixes should land in master first" mean? What if I just need to
|
|
fix the issue in <release>?
|
|
|
|
As the person fixing the issue, you are required to first check whether the
|
|
issue exists in master. If it does, then the proper course of action is to
|
|
create a master tracker (see `Tracker workflow`_) and fix the issue in master,
|
|
first, and only then cherry-pick the fix to the stable branches that have the
|
|
issue.
|
|
|
|
If the issue exists in the stable branch, but not in master, there are several
|
|
possibilities:
|
|
|
|
1. it's a regression introduced into the stable branch by a bad backport
|
|
2. the issue was fixed in master by some massive refactoring that cannot be backported
|
|
3. the issue was already fixed in master by a cherry-pickable commit
|
|
|
|
In cases 1 and 2, it's permissible to fix the issue directly in the most recent
|
|
stable branch, subject to the rule "if a commit could not be cherry-picked from
|
|
master, the commit message must explain why that was not possible". Once the
|
|
fix has landed in the most recent stable branch, it can be cherry-picked to
|
|
older stable branches from there.
|
|
|
|
In case 3, the issue should be handled like any other backport - read on.
|
|
|
|
|
|
Tracker workflow
|
|
----------------
|
|
|
|
Any change that is to be backported to multiple stable branches should have
|
|
an associated tracker issue at https://tracker.ceph.com.
|
|
|
|
For fixes already merged to master, this may have already been done - see the
|
|
``Fixes:`` line in the master PR. If the master PR has already been merged and
|
|
there is no associated master tracker issue, you can create a master tracker
|
|
issue and fill in the fields as described below.
|
|
|
|
This master tracker issue should be in the "Bug" or "Feature"
|
|
trackers of the relevant subproject under the "Ceph" parent project (or
|
|
in the "Ceph" project itself if none of the subprojects are a good fit).
|
|
The stable branches to which the master changes are to be cherry-picked should
|
|
be listed in the "Backport" field. For example::
|
|
|
|
Backport: mimic, nautilus
|
|
|
|
Once the PR targeting master is open, add the PR number assigned by GitHub to
|
|
the tracker issue. For example, if the PR number is 99999::
|
|
|
|
Pull request ID: 99999
|
|
|
|
Once the master PR has been merged, after checking that the change really needs
|
|
to be backported and the Backport field has been populated, change the master
|
|
tracker issue's ``Status`` field to "Pending Backport".
|
|
|
|
Status: Pending Backport
|
|
|
|
If you do not have sufficient permissions to modify any field of the tracker
|
|
issue, just add a comment describing what changes you would like to make.
|
|
Someone with permissions will make the necessary modifications on your behalf.
|
|
|
|
For straightforward backports, that's all that you (as the developer of the fix)
|
|
need to do. Volunteers from the `Stable Releases and Backports team`_ will
|
|
proceed to create Backport issues to track the necessary backports and stage the
|
|
backports by opening GitHub PRs with the cherry-picks. If you don't want to
|
|
wait, and provided you have sufficient permissions at https://tracker.ceph.com,
|
|
you can `create Backport tracker issues` and `stage backports`_ yourself. In
|
|
that case, read on.
|
|
|
|
|
|
.. _`create backport tracker issues`:
|
|
.. _`backport tracker issue`:
|
|
|
|
Creating Backport tracker issues
|
|
--------------------------------
|
|
|
|
To track backporting efforts, "backport tracker issues" can be created from
|
|
a parent "master tracker issue". The master tracker issue is described in the
|
|
previous section, `Tracker workflow`_. This section focuses the backport tracker
|
|
issue.
|
|
|
|
Once the entire `Tracker workflow`_ has been completed for the master issue,
|
|
issues can be created in the Backport tracker for tracking the backporting work.
|
|
|
|
Under ordinary circumstances, the developer who merges the master PR will flag
|
|
the master tracker issue for backport by changing the Status to "Pending
|
|
Backport", and volunteers from the `Stable Releases and Backports team`_
|
|
periodically create backport tracker issues by running the
|
|
``backport-create-issue`` script. They also do the actual backporting. But that
|
|
does take time and you may not want to wait.
|
|
|
|
You might be tempted to forge ahead and create the backport issues yourself.
|
|
Please don't do that - it is difficult (bordering on impossible) to get all the
|
|
fields correct when creating backport issues manually, and why even try when
|
|
there is a script that gets it right every time? Setting up the script requires
|
|
a small up-front time investment. Once that is done, creating backport issues
|
|
becomes trivial.
|
|
|
|
The backport-create-issue script
|
|
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
|
|
|
The script used to create backport issues is located at
|
|
``src/script/backport-create-issue`` in the master branch. Though there might be
|
|
an older version of this script in a stable branch, do not use it. Only use the
|
|
most recent version from master.
|
|
|
|
Once you have the script somewhere in your PATH, you can proceed to install the
|
|
dependencies.
|
|
|
|
The dependencies are:
|
|
|
|
* python3
|
|
* python-redmine
|
|
|
|
Python 3 should already be present on any recent Linux installation. The second
|
|
dependency, `python-redmine`_, can be obtained from PyPi::
|
|
|
|
pip3 install --user python-redmine
|
|
|
|
|
|
.. _`python-redmine`: https://pypi.org/project/python-redmine/
|
|
|
|
Then, try to run the script::
|
|
|
|
backport-create-issue --help
|
|
|
|
This should produce a usage message.
|
|
|
|
Finally, run the script to actually create the Backport issues.
|
|
For example, if the tracker issue number is 55555::
|
|
|
|
backport-create-issue --user <tracker_username> --password <tracker_password> 55555
|
|
|
|
The script needs to know your https://tracker.ceph.com credentials in order to
|
|
authenticate to Redmine. In lieu of providing your literal username and password
|
|
on the command line, you could also obtain a REST API key ("My account" -> "API
|
|
access key") and run the script like so::
|
|
|
|
backport-create-issue --key <tracker_api_key> 55555
|
|
|
|
|
|
.. _`stage backports`:
|
|
.. _`stage the backport`:
|
|
.. _`staging a backport`:
|
|
|
|
Opening a backport PR
|
|
---------------------
|
|
|
|
Once the `Tracker workflow`_ is completed and the `backport tracker issue`_ has
|
|
been created, it's time to open a backport PR. One possibility is to do this
|
|
manually, while taking care to follow the `cherry-picking rules`_. However, this
|
|
can result in a backport that is not properly staged. For example, the PR
|
|
description might not contain a link to the `backport tracker issue`_ (a common
|
|
oversight). You might even forget to update the `backport tracker issue`_.
|
|
|
|
In the past, much time was lost, and much frustration caused, by the necessity
|
|
of staging backports manually. Now, fortunately, there is a script available
|
|
which automates the process and takes away most of the guesswork.
|
|
|
|
The ceph-backport.sh script
|
|
^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
|
|
|
Similar to the case of creating the `Backport tracker issues`_, staging the actual
|
|
backport PR and updating the Backport tracker issue is difficult - if not
|
|
impossible - to get right if you're doing it manually, and quickly becomes
|
|
tedious if you do it more than once in a long while.
|
|
|
|
The ``ceph-backport.sh`` script automates the entire process of cherry-picking
|
|
the commits from the master PR, opening the GitHub backport PR, and
|
|
cross-linking the GitHub backport PR with the correct Backport tracker issue.
|
|
The script can also be used to good effect if you have already manually prepared
|
|
the backport branch with the cherry-picks in it.
|
|
|
|
The script is located at ``src/script/ceph-backport.sh`` in the ``master``
|
|
branch. Though there might be an older version of this script in a stable
|
|
branch, do not use it. Only use the most recent version from master.
|
|
|
|
This is just a bash script, so the only dependency is ``bash`` itself, but it
|
|
does need to be run in the top level of a local clone of ``ceph/ceph.git``.
|
|
A small up-front time investment is required to get the script working in your
|
|
environment. This is because the script needs to autenticate itself (i.e., as
|
|
you) in order to use the GitHub and Redmine REST API services.
|
|
|
|
The script is self-documenting. Just run the script and proceed from there.
|
|
|
|
Once the script has been set up properly, you can validate the setup like so::
|
|
|
|
ceph-backport.sh --setup
|
|
|
|
Once you have this saying "Overall setup is OK", you have two options for
|
|
staging the backport: either leave everything to the script, or prepare the
|
|
backport branch yourself and use the script only for creating the PR and
|
|
updating the Backport tracker issue.
|
|
|
|
If you prefer to leave everything to the script, just provide the Backport
|
|
tracker issue number to the script::
|
|
|
|
ceph-backport.sh 55555
|
|
|
|
The script will start by creating the backport branch in your local git clone.
|
|
The script always uses the following format for naming the branch::
|
|
|
|
wip-<backport_issue_number>-<name_of_stable_branch>
|
|
|
|
For example, if the Backport tracker issue number is 55555 and it's targeting
|
|
the stable branch "nautilus", the backport branch would be named::
|
|
|
|
wip-55555-nautilus
|
|
|
|
If you prefer to create the backport branch yourself, just do that. Be sure to
|
|
name the backport branch as described above. (It's a good idea to use this
|
|
branch naming convention for all your backporting work.) Then, run the script::
|
|
|
|
ceph-backport.sh 55555
|
|
|
|
The script will see that the backport branch already exists, and use it.
|
|
|
|
Conflict resolution
|
|
^^^^^^^^^^^^^^^^^^^
|
|
|
|
If git reports conflicts, the script will abort to allow you to resolve the
|
|
conflicts manually.
|
|
|
|
Once the conflicts are resolved, complete the cherry-pick ::
|
|
|
|
git cherry-pick --continue
|
|
|
|
Git will present a draft commit message with a "Conflicts" section.
|
|
|
|
Unfortunately, in recent versions of git, the Conflicts section is commented
|
|
out. Since the Conflicts section is mandatory for Ceph backports that do not
|
|
apply cleanly, you will need to uncomment the entire "Conflicts" section
|
|
of the commit message before committing the cherry-pick. You can also
|
|
include commentary on what the conflicts were and how you resolved
|
|
them. For example::
|
|
|
|
Conflicts:
|
|
src/foo/bar.cc
|
|
- mimic does not have blatz; use batlo instead
|
|
|
|
When editing the cherry-pick commit message, leave everything before the
|
|
"cherry picked from" line unchanged. Any edits you make should be in the part
|
|
following that line. Here is an example::
|
|
|
|
osd: check batlo before setting blatz
|
|
|
|
Setting blatz requires special precautions. Check batlo first.
|
|
|
|
Fixes: https://tracker.ceph.com/issues/99999
|
|
Signed-off-by: Random J Developer <random@developer.example.com>
|
|
(cherry picked from commit 01d73020da12f40ccd95ea1e49cfcf663f1a3a75)
|
|
|
|
Conflicts:
|
|
src/osd/batlo.cc
|
|
- add_batlo_check has an extra arg in newer code
|
|
|
|
Naturally, the ``Fixes`` line points to the master issue. You might be tempted
|
|
to modify it so it points to the backport issue, but - please - don't do that.
|
|
First, the master issue points to all the backport issues, and second, *any*
|
|
editing of the original commit message calls the entire backport into doubt,
|
|
simply because there is no good reason for such editing.
|
|
|
|
The part below the ``(cherry picked from commit ...)`` line is fair game for
|
|
editing. If you need to add additional information to the cherry-pick commit
|
|
message, append that information below this line. Once again: do not modify the
|
|
original commit message.
|
|
|
|
|
|
Labelling of backport PRs
|
|
-------------------------
|
|
|
|
Once the backport PR is open, the first order of business is to set the
|
|
Milestone tag to the stable release the backport PR is targeting. For example,
|
|
if the PR is targeting "nautilus", set the Milestone tag to "nautilus".
|
|
|
|
If you don't have sufficient GitHub permissions to set the Milestone, add
|
|
a comment to the PR with the following content::
|
|
|
|
@ceph/backport-admins
|
|
|
|
This will alert the `Stable Releases and Backports team`_ to your PR, and
|
|
someone will ensure your PR gets the proper labels.
|
|
|
|
|
|
.. _`backport PR reviewing`:
|
|
.. _`backport PR testing`:
|
|
.. _`backport PR merging`:
|
|
|
|
Reviewing, testing, and merging of backport PRs
|
|
-----------------------------------------------
|
|
|
|
Once your backport PR is open and the Milestone is set properly, the
|
|
`Stable Releases and Backports team` will take care of getting the PR
|
|
reviewed and tested. Once the PR is reviewed and tested, it will be merged.
|
|
|
|
If you would like to facilitate this process, you can solicit reviews and run
|
|
integration tests on the PR. In this case, add comments to the PR describing the
|
|
tests you ran and their results.
|
|
|
|
Once the PR has been reviewed and deemed to have undergone sufficient testing,
|
|
it will be merged. Even if you have sufficient GitHub permissions to merge the
|
|
PR, please do *not* merge it yourself. (Uncontrolled merging to stable branches
|
|
unnecessarily complicates the release preparation process, which is done by
|
|
volunteers.)
|
|
|
|
|
|
Stable Releases and Backports team
|
|
----------------------------------
|
|
|
|
Ceph has a `Stable Releases and Backports`_ team, staffed by volunteers,
|
|
which is charged with maintaining the stable releases and backporting bugfixes
|
|
from the master branch to them. (That team maintains a wiki, accessible by
|
|
clicking the `Stable Releases and Backports`_ link, which describes various
|
|
workflows in the backporting lifecycle.)
|
|
|
|
.. _`Stable Releases and Backports`: http://tracker.ceph.com/projects/ceph-releases/wiki
|
|
|
|
Ordinarily, it is enough to fill out the "Backport" field in the bug (tracker
|
|
issue). The volunteers from the Stable Releases and Backports team will
|
|
backport the fix, run regression tests on it, and include it in one or more
|
|
future point releases.
|
|
|
|
|