Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Split the existing SubmittingPatches.rst into three files: * SubmittingPatches.rst (general guidelines, for GitHub/master branch) * SubmittingPatches-kernel.rst (for kernel patches) * SubmittingPatches-backports.rst (for backports) Fixes: http://tracker.ceph.com/issues/20953 Signed-off-by: Nathan Cutler <ncutler@suse.com>
- Loading branch information
Showing
3 changed files
with
859 additions
and
403 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,382 @@ | ||
| 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:: | ||
|
|
||
| pip 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:: | ||
|
|
||
| backport-create-issue --user <tracker_username> --password <tracker_password> 99999 | ||
|
|
||
| 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> 99999 | ||
|
|
||
|
|
||
| .. _`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. | ||
|
|
||
|
|
Oops, something went wrong.