mirror of https://github.com/ceph/ceph
381 lines
17 KiB
ReStructuredText
381 lines
17 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 ``main`` branch 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 ``main`` first
|
|
* commits to stable branches should be cherry-picked from ``main``
|
|
* before starting to cherry-pick a set of commits from ``main``, grep the ``main`` git history for the SHA1 of each ``main`` 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.
|
|
* when backporting a ``main`` PR to a stable branch, double-check that the backport PR contains cherry-picks of all of the ``main`` PR's commits. If any commit needs to be omitted, declare and explain this in the PR.
|
|
* cherry-picks must be done using ``git cherry-pick -x``
|
|
* if a cherry-pick from ``main`` is not feasible and a direct fix is being undertaken, this must be explained
|
|
* 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 ``main`` 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 ``main``. If it does, then the proper course of action is to
|
|
create a ``main`` branch tracker (see `Tracker workflow`_) and fix the issue in ``main``,
|
|
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 ``main``, there are several
|
|
possibilities:
|
|
|
|
1. it's a regression introduced into the stable branch by a bad backport
|
|
2. the issue was fixed in ``main`` by some massive refactoring that cannot be backported
|
|
3. the issue was already fixed in ``main`` 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
|
|
``main``, 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 ``main``, this may have already been done - see the
|
|
``Fixes:`` line in the ``main`` PR. If the ``main`` PR has already been merged and
|
|
there is no associated ``main`` branch tracker issue, you can create a ``main`` branch tracker
|
|
issue and fill in the fields as described below.
|
|
|
|
This ``main`` branch 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 ``main`` changes are to be cherry-picked should
|
|
be listed in the "Backport" field. For example::
|
|
|
|
Backport: mimic, nautilus
|
|
|
|
Once the PR targeting ``main`` 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 ``main`` PR has been merged, after checking that the change really needs
|
|
to be backported and the Backport field has been populated, change the ``main``
|
|
branch 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.
|
|
|
|
Authors of pull requests are responsible for creating associated backport pull
|
|
requests. As long as you have sufficient permissions at
|
|
https://tracker.ceph.com, you can `create Backport tracker issues` and `stage
|
|
backports`_ yourself. Read these linked sections to learn how to create
|
|
backport tracker issues and how to stage backports:
|
|
|
|
.. _`create backport tracker issues`:
|
|
.. _`backport tracker issue`:
|
|
|
|
Creating Backport tracker issues
|
|
--------------------------------
|
|
|
|
To track backporting efforts, "backport tracker issues" can be created from
|
|
a parent "``main`` branch tracker issue". The ``main`` branch 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 ``main`` branch tracker issue,
|
|
issues can be created in the backport tracker issue for tracking the backporting work.
|
|
|
|
Under ordinary circumstances, the developer who merges the ``main`` PR will flag
|
|
the ``main`` branch tracker issue for backport by changing the Status to "Pending
|
|
Backport".
|
|
|
|
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 ``main`` 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 ``main``.
|
|
|
|
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"), put it in ``~/.redmine_key`` and run the script like so::
|
|
|
|
backport-create-issue 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 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 ``main`` 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 ``main``
|
|
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 the ``main`` branch.
|
|
To do this from anywhere and from any branch use the following
|
|
alias that will use the most recent script in ``upstream/main`` of your
|
|
local ceph clone on every call::
|
|
|
|
alias ceph-backport.sh="bash <(git --git-dir=$pathToCephClone/.git --no-pager show upstream/main:src/script/ceph-backport.sh)"
|
|
|
|
``ceph-backport.sh`` 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
|
|
authenticate 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.
|
|
|
|
Once the script hits the first cherry-pick conflict, it will no longer provide
|
|
any cherry-picking assistance, so in that case it's up to you to resolve the conflict(s)
|
|
(as described in `Conflict resolution`_) and finish cherry-picking
|
|
all of the remaining commits. Once you are satisfied that the backport is complete in
|
|
your local branch, `ceph-backport.sh` can finish the job of creating the pull request
|
|
and updating the backport tracker issue. To make that happen, just re-run the script
|
|
exactly as you did before::
|
|
|
|
ceph-backport.sh $BACKPORT_TRACKER_ID
|
|
|
|
The script will detect that it is running from a branch with the same name as the one it
|
|
would normally create on the first run and continues after the cherry-picking phase.
|
|
|
|
For a quick reference on CLI, that contains above information, you can run::
|
|
|
|
ceph-backport.sh --usage
|
|
|
|
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 ``main`` issue. You might be tempted
|
|
to modify it so it points to the backport issue, but - please - don't do that.
|
|
First, the ``main`` 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.
|
|
|
|
If you use `ceph-backport.sh` for your backport creation (which is recommended),
|
|
read up at the end of `The ceph-backport.sh script`_ on how to continue from here.
|
|
|
|
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".
|
|
|
|
Next, check which component label was applied to the ``main`` PR corresponding to
|
|
this backport, and double-check that that label is applied to the backport PR as
|
|
well. For example, if the ``main`` PR carries the component label "core", the
|
|
backport PR should also get that label.
|
|
|
|
.. _`backport PR reviewing`:
|
|
.. _`backport PR testing`:
|
|
.. _`backport PR merging`:
|
|
|
|
Reviewing, testing, and merging of backport PRs
|
|
-----------------------------------------------
|
|
|
|
Once your backport PR is open, it will be reviewed and tested. When the PR has
|
|
been 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.)
|