From e6918187568dbd01842d8d1d2c808ce16a894239 Mon Sep 17 00:00:00 2001 From: Daniel Baumann Date: Sun, 21 Apr 2024 13:54:28 +0200 Subject: Adding upstream version 18.2.2. Signed-off-by: Daniel Baumann --- SubmittingPatches-backports.rst | 415 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 415 insertions(+) create mode 100644 SubmittingPatches-backports.rst (limited to 'SubmittingPatches-backports.rst') diff --git a/SubmittingPatches-backports.rst b/SubmittingPatches-backports.rst new file mode 100644 index 000000000..0f96aec65 --- /dev/null +++ b/SubmittingPatches-backports.rst @@ -0,0 +1,415 @@ +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 + +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 ? + +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. + +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 "``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", 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 ``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 --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-- + +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 + (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". + +If you don't have sufficient GitHub permissions to set the Milestone, don't +worry. Members of the `Stable Releases and Backports team`_ periodically run +a script (``ceph-backport.sh --milestones``) which scans all PRs targetting stable +branches and automatically adds the correct Milestone tag if it is missing. + +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. + +In general, it is the responsibility of the `Stable Releases and Backports +team`_ to ensure that backport PRs are properly labelled. If in doubt, just +leave the labelling to them. + +.. _`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 ``main`` 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. + + -- cgit v1.2.3