From ec6af09384be025d5655dc60f5edb717f180b93f Mon Sep 17 00:00:00 2001 From: Nathan Cutler Date: Thu, 5 Jul 2018 15:50:11 +0200 Subject: [PATCH] doc: split up SubmittingPatches.rst 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 --- SubmittingPatches-backports.rst | 382 +++++++++++++++++++++ SubmittingPatches-kernel.rst | 300 +++++++++++++++++ SubmittingPatches.rst | 580 ++++++++++---------------------- 3 files changed, 859 insertions(+), 403 deletions(-) create mode 100644 SubmittingPatches-backports.rst create mode 100644 SubmittingPatches-kernel.rst diff --git a/SubmittingPatches-backports.rst b/SubmittingPatches-backports.rst new file mode 100644 index 0000000000000..323a9da2dcfb1 --- /dev/null +++ b/SubmittingPatches-backports.rst @@ -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 + +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 ? + +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 --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 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-- + +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 + (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. + + diff --git a/SubmittingPatches-kernel.rst b/SubmittingPatches-kernel.rst new file mode 100644 index 0000000000000..37dcbc6374f17 --- /dev/null +++ b/SubmittingPatches-kernel.rst @@ -0,0 +1,300 @@ +Submitting Patches to Ceph - Kernel Components +============================================== + +Submission of patches to the Ceph kernel code is subject to the same rules +and guidelines as any other patches to the Linux Kernel. These are set out in +``Documentation/process/submitting-patches.rst`` in the kernel source tree. + +What follows is a condensed version of those rules and guidelines, updated based +on the Ceph project's best practices. + + +.. contents:: + :depth: 3 + + +Signing contributions +--------------------- + +In order to keep the record of code attribution clean within the source +repository, follow these guidelines for signing patches submitted to the +project. These definitions are taken from those used by the Linux kernel +and many other open source projects. + + +1. Sign your work +################# + +To improve tracking of who did what, especially with patches that can +percolate to their final resting place in the kernel through several +layers of maintainers, we've introduced a "sign-off" procedure on +patches that are being emailed around. + +The sign-off is a simple line at the end of the explanation for the +patch, which certifies that you wrote it or otherwise have the right to +pass it on as a open-source patch. The rules are pretty simple: if you +can certify the below: + +Developer's Certificate of Origin 1.1 +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +By making a contribution to this project, I certify that: + + (a) The contribution was created in whole or in part by me and I + have the right to submit it under the open source license + indicated in the file; or + + (b) The contribution is based upon previous work that, to the best + of my knowledge, is covered under an appropriate open source + license and I have the right under that license to submit that + work with modifications, whether created in whole or in part + by me, under the same open source license (unless I am + permitted to submit under a different license), as indicated + in the file; or + + (c) The contribution was provided directly to me by some other + person who certified (a), (b) or (c) and I have not modified + it. + + (d) I understand and agree that this project and the contribution + are public and that a record of the contribution (including all + personal information I submit with it, including my sign-off) is + maintained indefinitely and may be redistributed consistent with + this project or the open source license(s) involved. + +then you just add a line saying :: + + Signed-off-by: Random J Developer + + +using your real name (sorry, no pseudonyms or anonymous contributions.) + +Some people also put extra tags at the end. They'll just be ignored for +now, but you can do this to mark internal company procedures or just +point out some special detail about the sign-off. + +If you are a subsystem or branch maintainer, sometimes you need to slightly +modify patches you receive in order to merge them, because the code is not +exactly the same in your tree and the submitters'. If you stick strictly to +rule (c), you should ask the submitter to rediff, but this is a totally +counter-productive waste of time and energy. Rule (b) allows you to adjust +the code, but then it is very impolite to change one submitter's code and +make them endorse your bugs. To solve this problem, it is recommended that +you add a line between the last Signed-off-by header and yours, indicating +the nature of your changes. While there is nothing mandatory about this, it +seems like prepending the description with your mail and/or name, all +enclosed in square brackets, is noticeable enough to make it obvious that +you are responsible for last-minute changes. Example :: + + Signed-off-by: Random J Developer + [lucky@maintainer.example.org: struct foo moved from foo.c to foo.h] + Signed-off-by: Lucky K Maintainer + +This practise is particularly helpful if you maintain a stable branch and +want at the same time to credit the author, track changes, merge the fix, +and protect the submitter from complaints. Note that under no circumstances +can you change the author's identity (the From header), as it is the one +which appears in the changelog. + +Special note to back-porters: It seems to be a common and useful practise +to insert an indication of the origin of a patch at the top of the commit +message (just after the subject line) to facilitate tracking. For instance, +here's what we see in 2.6-stable :: + + Date: Tue May 13 19:10:30 2008 +0000 + + SCSI: libiscsi regression in 2.6.25: fix nop timer handling + + commit 4cf1043593db6a337f10e006c23c69e5fc93e722 upstream + +And here's what appears in 2.4 :: + + Date: Tue May 13 22:12:27 2008 +0200 + + wireless, airo: waitbusy() won't delay + + [backport of 2.6 commit b7acbdfbd1f277c1eb23f344f899cfa4cd0bf36a] + +Whatever the format, this information provides a valuable help to people +tracking your trees, and to people trying to trouble-shoot bugs in your +tree. + + +2. When to use ``Acked-by:`` and ``Cc:`` +######################################## + +The ``Signed-off-by:`` tag indicates that the signer was involved in the +development of the patch, or that he/she was in the patch's delivery path. + +If a person was not directly involved in the preparation or handling of a +patch but wishes to signify and record their approval of it then they can +arrange to have an ``Acked-by:`` line added to the patch's changelog. + +``Acked-by:`` is often used by the maintainer of the affected code when that +maintainer neither contributed to nor forwarded the patch. + +``Acked-by:`` is not as formal as ``Signed-off-by:``. It is a record that the acker +has at least reviewed the patch and has indicated acceptance. Hence patch +mergers will sometimes manually convert an acker's "yep, looks good to me" +into an ``Acked-by:``. + +``Acked-by:`` does not necessarily indicate acknowledgement of the entire patch. +For example, if a patch affects multiple subsystems and has an ``Acked-by:`` from +one subsystem maintainer then this usually indicates acknowledgement of just +the part which affects that maintainer's code. Judgement should be used here. +When in doubt people should refer to the original discussion in the mailing +list archives. + +If a person has had the opportunity to comment on a patch, but has not +provided such comments, you may optionally add a "Cc:" tag to the patch. +This is the only tag which might be added without an explicit action by the +person it names. This tag documents that potentially interested parties +have been included in the discussion + + +3. Using ``Reported-by:``, ``Tested-by:`` and ``Reviewed-by:`` +############################################################## + +If this patch fixes a problem reported by somebody else, consider adding a +``Reported-by:`` tag to credit the reporter for their contribution. This tag should +not be added without the reporter's permission, especially if the problem was +not reported in a public forum. That said, if we diligently credit our bug +reporters, they will, hopefully, be inspired to help us again in the future. + +A ``Tested-by:`` tag indicates that the patch has been successfully tested (in +some environment) by the person named. This tag informs maintainers that +some testing has been performed, provides a means to locate testers for +future patches, and ensures credit for the testers. + +``Reviewed-by:``, instead, indicates that the patch has been reviewed and found +acceptable according to the Reviewer's Statement: + +Reviewer's statement of oversight +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +By offering my ``Reviewed-by:`` tag, I state that: + + (a) I have carried out a technical review of this patch to + evaluate its appropriateness and readiness for inclusion into + the mainline kernel. + + (b) Any problems, concerns, or questions relating to the patch + have been communicated back to the submitter. I am satisfied + with the submitter's response to my comments. + + (c) While there may be things that could be improved with this + submission, I believe that it is, at this time, (1) a + worthwhile modification to the kernel, and (2) free of known + issues which would argue against its inclusion. + + (d) While I have reviewed the patch and believe it to be sound, I + do not (unless explicitly stated elsewhere) make any + warranties or guarantees that it will achieve its stated + purpose or function properly in any given situation. + +A ``Reviewed-by`` tag is a statement of opinion that the patch is an +appropriate modification of the kernel without any remaining serious +technical issues. Any interested reviewer (who has done the work) can +offer a ``Reviewed-by`` tag for a patch. This tag serves to give credit to +reviewers and to inform maintainers of the degree of review which has been +done on the patch. ``Reviewed-by:`` tags, when supplied by reviewers known to +understand the subject area and to perform thorough reviews, will normally +increase the likelihood of your patch getting into the kernel. + + +Preparing and sending patches +----------------------------- + +For the kernel client, patches are expected to be emailed directly to the +email list ``ceph-devel@vger.kernel.org`` (note: *not* ``dev@ceph.io``) and reviewed +in the email list. + +The best way to generate a patch for manual submission is to work from +a Git checkout of the Ceph kernel client (kernel modules) repository located at +https://github.com/ceph/ceph-client. You can then generate patches +with the 'git format-patch' command. For example, + +.. code-block:: bash + + $ git format-patch HEAD^^ -o mything + +will take the last two commits and generate patches in the mything/ +directory. The commit you specify on the command line is the +'upstream' commit that you are diffing against. Note that it does +not necessarily have to be an ancestor of your current commit. You +can do something like + +.. code-block:: bash + + $ git checkout -b mything + # ... do lots of stuff ... + $ git fetch + # ...find out that origin/unstable has also moved forward... + $ git format-patch origin/unstable -o mything + +and the patches will be against origin/unstable. + +The ``-o`` dir is optional; if left off, the patch(es) will appear in +the current directory. This can quickly get messy. + +You can also add ``--cover-letter`` and get a '0000' patch in the +mything/ directory. That can be updated to include any overview +stuff for a multipart patch series. If it's a single patch, don't +bother. + +Make sure your patch does not include any extra files which do not +belong in a patch submission. Make sure to review your patch -after- +generated it with ``diff(1)``, to ensure accuracy. + +If your changes produce a lot of deltas, you may want to look into +splitting them into individual patches which modify things in +logical stages. This will facilitate easier reviewing by other +kernel developers, very important if you want your patch accepted. +There are a number of scripts which can aid in this. + +The ``git send-email`` command make it super easy to send patches +(particularly those prepared with git format patch). It is careful to +format the emails correctly so that you don't have to worry about your +email client mangling whitespace or otherwise screwing things up. It +works like so: + +.. code-block:: bash + + $ git send-email --to ceph-devel@vger.kernel.org my.patch + +for a single patch, or + +.. code-block:: bash + + $ git send-email --to ceph-devel@vger.kernel.org mything + +to send a whole patch series (prepared with, say, git format-patch). + + +No MIME, no links, no compression, no attachments. Just plain text +------------------------------------------------------------------ + +Developers need to be able to read and comment on the changes you are +submitting. It is important for a kernel developer to be able to +"quote" your changes, using standard e-mail tools, so that they may +comment on specific portions of your code. + +For this reason, all patches should be submitting e-mail "inline". +WARNING: Be wary of your editor's word-wrap corrupting your patch, +if you choose to cut-n-paste your patch. + +Do not attach the patch as a MIME attachment, compressed or not. +Many popular e-mail applications will not always transmit a MIME +attachment as plain text, making it impossible to comment on your +code. A MIME attachment also takes Linus a bit more time to process, +decreasing the likelihood of your MIME-attached change being accepted. + +Exception: If your mailer is mangling patches then someone may ask +you to re-send them using MIME. + + +Style Guide +----------- + +The Linux Kernel has coding style conventions, which are set forth in +``Documentation/process/coding-style.rst``. Please adhere to these conventions. diff --git a/SubmittingPatches.rst b/SubmittingPatches.rst index 601e88223d68a..bc2e7e951d2dc 100644 --- a/SubmittingPatches.rst +++ b/SubmittingPatches.rst @@ -2,32 +2,36 @@ Submitting Patches to Ceph ========================== -This is based on Documentation/SubmittingPatches from the Linux kernel, -but has pared down significantly and updated based on the Ceph project's -best practices. +Patches to Ceph can be divided into three categories: -The patch signing procedures and definitions are unmodified. + 1. patches targeting Ceph kernel code + 2. patches targeting the "master" branch + 3. patches targeting stable branches (e.g.: "nautilus") +Some parts of Ceph - notably the RBD and CephFS kernel clients - are maintained +within the Linux Kernel. For patches targeting this code, please refer to the +file ``SubmittingPatches-kernel.rst``. -SIGNING CONTRIBUTIONS -===================== +The rest of this document assumes that your patch relates to Ceph code that is +maintained in the GitHub repository https://github.com/ceph/ceph -In order to keep the record of code attribution clean within the source -repository, follow these guidelines for signing patches submitted to the -project. These definitions are taken from those used by the Linux kernel -and many other open source projects. +If you have a patch that fixes an issue, feel free to open a GitHub pull request +("PR") targeting the "master" branch, but do read this document first, as it +contains important information for ensuring that your PR passes code review +smoothly. +For patches targeting stable branches (e.g. "nautilus"), please also see +the file ``SubmittingPatches-backports.rst``. + +.. contents:: + :depth: 3 -1. Sign your work ------------------ -To improve tracking of who did what, especially with patches that can -percolate to their final resting place in the kernel through several -layers of maintainers, we've introduced a "sign-off" procedure on -patches that are being emailed around. +Sign your work +-------------- The sign-off is a simple line at the end of the explanation for the -patch, which certifies that you wrote it or otherwise have the right to +commit, which certifies that you wrote it or otherwise have the right to pass it on as a open-source patch. The rules are pretty simple: if you can certify the below: @@ -62,468 +66,238 @@ then you just add a line saying :: Signed-off-by: Random J Developer - using your real name (sorry, no pseudonyms or anonymous contributions.) -Some people also put extra tags at the end. They'll just be ignored for -now, but you can do this to mark internal company procedures or just -point out some special detail about the sign-off. - -If you are a subsystem or branch maintainer, sometimes you need to slightly -modify patches you receive in order to merge them, because the code is not -exactly the same in your tree and the submitters'. If you stick strictly to -rule (c), you should ask the submitter to rediff, but this is a totally -counter-productive waste of time and energy. Rule (b) allows you to adjust -the code, but then it is very impolite to change one submitter's code and -make them endorse your bugs. To solve this problem, it is recommended that -you add a line between the last Signed-off-by header and yours, indicating -the nature of your changes. While there is nothing mandatory about this, it -seems like prepending the description with your mail and/or name, all -enclosed in square brackets, is noticeable enough to make it obvious that -you are responsible for last-minute changes. Example :: - - Signed-off-by: Random J Developer - [lucky@maintainer.example.org: struct foo moved from foo.c to foo.h] - Signed-off-by: Lucky K Maintainer - -This practise is particularly helpful if you maintain a stable branch and -want at the same time to credit the author, track changes, merge the fix, -and protect the submitter from complaints. Note that under no circumstances -can you change the author's identity (the From header), as it is the one -which appears in the changelog. - -Special note to back-porters: It seems to be a common and useful practise -to insert an indication of the origin of a patch at the top of the commit -message (just after the subject line) to facilitate tracking. For instance, -here's what we see in 2.6-stable :: - - Date: Tue May 13 19:10:30 2008 +0000 - - SCSI: libiscsi regression in 2.6.25: fix nop timer handling - - commit 4cf1043593db6a337f10e006c23c69e5fc93e722 upstream - -And here's what appears in 2.4 :: - - Date: Tue May 13 22:12:27 2008 +0200 - - wireless, airo: waitbusy() won't delay - - [backport of 2.6 commit b7acbdfbd1f277c1eb23f344f899cfa4cd0bf36a] - -Whatever the format, this information provides a valuable help to people -tracking your trees, and to people trying to trouble-shoot bugs in your -tree. - - -2. When to use ``Acked-by:`` and ``Cc:`` ----------------------------------------- - -The ``Signed-off-by:`` tag indicates that the signer was involved in the -development of the patch, or that he/she was in the patch's delivery path. - -If a person was not directly involved in the preparation or handling of a -patch but wishes to signify and record their approval of it then they can -arrange to have an ``Acked-by:`` line added to the patch's changelog. - -``Acked-by:`` is often used by the maintainer of the affected code when that -maintainer neither contributed to nor forwarded the patch. - -``Acked-by:`` is not as formal as ``Signed-off-by:``. It is a record that the acker -has at least reviewed the patch and has indicated acceptance. Hence patch -mergers will sometimes manually convert an acker's "yep, looks good to me" -into an ``Acked-by:``. - -``Acked-by:`` does not necessarily indicate acknowledgement of the entire patch. -For example, if a patch affects multiple subsystems and has an ``Acked-by:`` from -one subsystem maintainer then this usually indicates acknowledgement of just -the part which affects that maintainer's code. Judgement should be used here. -When in doubt people should refer to the original discussion in the mailing -list archives. - -If a person has had the opportunity to comment on a patch, but has not -provided such comments, you may optionally add a "Cc:" tag to the patch. -This is the only tag which might be added without an explicit action by the -person it names. This tag documents that potentially interested parties -have been included in the discussion - - -3. Using ``Reported-by:``, ``Tested-by:`` and ``Reviewed-by:`` --------------------------------------------------------------- - -If this patch fixes a problem reported by somebody else, consider adding a -``Reported-by:`` tag to credit the reporter for their contribution. This tag should -not be added without the reporter's permission, especially if the problem was -not reported in a public forum. That said, if we diligently credit our bug -reporters, they will, hopefully, be inspired to help us again in the future. - -A ``Tested-by:`` tag indicates that the patch has been successfully tested (in -some environment) by the person named. This tag informs maintainers that -some testing has been performed, provides a means to locate testers for -future patches, and ensures credit for the testers. - -``Reviewed-by:``, instead, indicates that the patch has been reviewed and found -acceptable according to the Reviewer's Statement: - -Reviewer's statement of oversight -^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - -By offering my ``Reviewed-by:`` tag, I state that: - - (a) I have carried out a technical review of this patch to - evaluate its appropriateness and readiness for inclusion into - the mainline kernel. - - (b) Any problems, concerns, or questions relating to the patch - have been communicated back to the submitter. I am satisfied - with the submitter's response to my comments. - - (c) While there may be things that could be improved with this - submission, I believe that it is, at this time, (1) a - worthwhile modification to the kernel, and (2) free of known - issues which would argue against its inclusion. - - (d) While I have reviewed the patch and believe it to be sound, I - do not (unless explicitly stated elsewhere) make any - warranties or guarantees that it will achieve its stated - purpose or function properly in any given situation. - -A ``Reviewed-by`` tag is a statement of opinion that the patch is an -appropriate modification of the kernel without any remaining serious -technical issues. Any interested reviewer (who has done the work) can -offer a ``Reviewed-by`` tag for a patch. This tag serves to give credit to -reviewers and to inform maintainers of the degree of review which has been -done on the patch. ``Reviewed-by:`` tags, when supplied by reviewers known to -understand the subject area and to perform thorough reviews, will normally -increase the likelihood of your patch getting into the kernel. - - -PREPARING AND SENDING PATCHES -============================= - -The upstream repository is managed by Git. You will find that it -is easiest to work on the project and submit changes by using the -git tools, both for managing your own code and for preparing and -sending patches. - -The project will generally accept code either by pulling code directly from -a published git tree (usually on github), or via patches emailed directly -to the email list (ceph-devel@vger.kernel.org). For the kernel client, -patches are expected to be reviewed in the email list. And for everything -else, github is preferred due to the convenience of the 'pull request' -feature. - - -1. Github pull request ----------------------- - -The preferred way to submit code is by publishing your patches in a branch -in your github fork of the ceph repository and then submitting a github -pull request. - -For example, prepare your changes - -.. code-block:: bash - - # ...code furiously... - $ git commit # git gui is also quite convenient - $ git push origin mything +Git can sign off on your behalf +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -Then submit a pull request at +Please note that git makes it trivially easy to sign commits. First, set the +following config options:: - https://github.com/ceph/ceph/pulls + $ git config --list | grep user + user.email=my_real_email_address@example.com + user.name=My Real Name -and click 'New pull request'. See :ref:`_title_of_commit` for our naming -convention of pull requests. The 'hub' command-line tool, available from +Then just remember to use ``git commit -s``. Git will add the ``Signed-off-by`` +line automatically. - https://github.com/github/hub -allows you to submit pull requests directly from the command line +Separate your changes +--------------------- -.. code-block:: bash +Group *logical changes* into individual commits. - $ hub pull-request -b ceph:master -h you:mything +If you have a series of bulleted modifications, consider separating each of +those into its own commit. -Pull requests appear in the review queue at +For example, if your changes include both bug fixes and performance enhancements +for a single component, separate those changes into two or more commits. If your +changes include an API update, and a new feature which uses that new API, +separate those into two patches. - https://github.com/organizations/ceph/dashboard/pulls +On the other hand, if you make a single change that affects numerous +files, group those changes into a single commit. Thus a single logical change is +contained within a single patch. (If the change needs to be backported, that +might change the calculus, because smaller commits are easier to backport.) -You may want to ping a developer in #ceph-devel on irc.oftc.net or on the -email list to ensure your submission is noticed. -When addressing review comments, can should either add additional patches to -your branch or (better yet) squash those changes into the relevant commits so -that the sequence of changes is "clean" and gets things right the first time. -The ``git rebase -i`` command is very helpful in this process. Once you have -updated your local branch, you can simply force-push to the existing branch -in your public repository that is referenced by the pull request with +Describe your changes +--------------------- -.. code-block:: bash +Each commit has an associated commit message that is stored in git. The first +line of the commit message is the `commit title`_. The second line should be +left blank. The lines that follow constitute the `commit message`_. - $ git push -f origin mything +A commit and its message should be focused around a particular change. -and your changes will be visible from the existing pull-request. You may want -to ping the reviewer again or comment on the pull request to ensure the updates -are noticed. +Commit title +^^^^^^^^^^^^ -Sometimes your change could be based on an outdated parent commit and has -conflicts with the latest target branch, then you need to fetch the updates -from the remote branch, rebase your change onto it, and resolve the conflicts -before doing the force-push - -.. code-block:: bash - - $ git pull --rebase origin target-branch - -So that the pull request does not contain any "merge" commit. Instead of "merging" -the target branch, we expect a linear history in a pull request where you -commit on top of the remote branch. - -Q: Which branch should I target in my pull request? - -A: The target branch depends on the nature of your change: - - If you are adding a feature, target the "master" branch in your pull - request. - - If you are fixing a bug, target the named branch corresponding to the - major version that is currently in development. For example, if - Infernalis is the latest stable release and Jewel is development, target - the "jewel" branch for bugfixes. The Ceph core developers will - periodically merge this named branch into "master". When this happens, - the master branch will contain your fix as well. - - If you are fixing a bug (see above) *and* the bug exists in older stable - branches (for example, the "hammer" or "infernalis" branches), then you - should file a Redmine ticket describing your issue and fill out the - "Backport: " form field. This will notify other developers that - your commit should be cherry-picked to one or more stable branches. Then, - target the "master" branch in your pull request. - - For example, you should set "Backport: jewel, kraken" in your Redmine ticket - to indicate that you are fixing a bug that exists on the "jewel" and - "kraken" branches and that you desire that your change be cherry-picked to - those branches after it is merged into master. - -Q: How to include ``Reviewed-by: tag(s)`` in my pull request? - -A: You don't. If someone reviews your pull request, they should indicate they - have done so by commenting on it with "+1", "looks good to me", "LGTM", - and/or the entire "Reviewed-by: ..." line with their name and email address. +The text up to the first empty line in a commit message is the commit +title. It should be a single short line of at most 72 characters, +summarizing the change, and prefixed with the +subsystem or module you are changing. Also, it is conventional to use the +imperative mood in the commit title. Positive examples include:: - The developer merging the pull request should note positive reviews and - include the appropriate Reviewed-by: lines in the merge commit. + mds: add perf counter for finisher of MDSRank + osd: make the ClassHandler::mutex private +More positive examples can be obtained from the git history of the ``master`` +branch:: -2. Patch submission via ceph-devel@vger.kernel.org --------------------------------------------------- + git log -The best way to generate a patch for manual submission is to work from -a Git checkout of the Ceph source code. You can then generate patches -with the 'git format-patch' command. For example, +Some negative examples (how *not* to title a commit message):: -.. code-block:: bash + update driver X + bug fix for driver X + fix issue 99999 - $ git format-patch HEAD^^ -o mything +Further to the last negative example ("fix issue 99999"), see `Fixes line`_. -will take the last two commits and generate patches in the mything/ -directory. The commit you specify on the command line is the -'upstream' commit that you are diffing against. Note that it does -not necessarily have to be an ancestor of your current commit. You -can do something like +Commit message +^^^^^^^^^^^^^^ -.. code-block:: bash +(This section is about the body of the commit message. Please also see +the preceding section, `Commit title`_, for advice on titling commit messages.) - $ git checkout -b mything - # ... do lots of stuff ... - $ git fetch - # ...find out that origin/unstable has also moved forward... - $ git format-patch origin/unstable -o mything +In the body of your commit message, be as specific as possible. If the commit +message title was too short to fully state what the commit is doing, use the +body to explain not just the "what", but also the "why". -and the patches will be against origin/unstable. +For positive examples, peruse ``git log`` in the ``master`` branch. A negative +example would be a commit message that merely states the obvious. For example: +"this patch includes updates for subsystem X. Please apply." -The ``-o`` dir is optional; if left off, the patch(es) will appear in -the current directory. This can quickly get messy. +.. _`fixes line`: -You can also add ``--cover-letter`` and get a '0000' patch in the -mything/ directory. That can be updated to include any overview -stuff for a multipart patch series. If it's a single patch, don't -bother. +Fixes line(s) +^^^^^^^^^^^^^ -Make sure your patch does not include any extra files which do not -belong in a patch submission. Make sure to review your patch -after- -generated it with ``diff(1)``, to ensure accuracy. +If the commit fixes one or more issues tracked by http://tracker.ceph.com, +add a ``Fixes:`` line (or lines) to the commit message, to connect this change +to addressed issue(s) - for example:: -If your changes produce a lot of deltas, you may want to look into -splitting them into individual patches which modify things in -logical stages. This will facilitate easier reviewing by other -kernel developers, very important if you want your patch accepted. -There are a number of scripts which can aid in this. + Fixes: http://tracker.ceph.com/issues/12345 -The ``git send-email`` command make it super easy to send patches -(particularly those prepared with git format patch). It is careful to -format the emails correctly so that you don't have to worry about your -email client mangling whitespace or otherwise screwing things up. It -works like so: +This line should be added just before the ``Signed-off-by:`` line (see `Sign +your work`_). -.. code-block:: bash +It helps reviewers to get more context of this bug and facilitates updating of +the bug tracker. Also, anyone perusing the git history will see this line and be +able to refer to the bug tracker easily. - $ git send-email --to ceph-devel@vger.kernel.org my.patch +Here is an example showing a properly-formed commit message:: -for a single patch, or + doc: add "--foo" option to bar -.. code-block:: bash + This commit updates the man page for bar with the newly added "--foo" + option. - $ git send-email --to ceph-devel@vger.kernel.org mything + Fixes: http://tracker.ceph.com/issues/12345 + Signed-off-by: Random J Developer -to send a whole patch series (prepared with, say, git format-patch). +If a commit fixes a regression introduced by a different commit, please also +(in addition to the above) add a line referencing the SHA1 of the commit that +introduced the regression. For example:: + Fixes: 9dbe7a003989f8bb45fe14aaa587e9d60a392727 -3. Describe your changes ------------------------- -Describe the technical detail of the change(s) your patch includes. +PR best practices +----------------- -.. _title_of_commit: +PRs should be opened on branches contained in your fork of +https://github.com/ceph/ceph.git - do not push branches directly to +``ceph/ceph.git``. -Title of pull requests and title of commits -^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +PRs should target "master". If you need to add a patch to a stable branch, such +as "nautilus", see the file ``SubmittingPatches-backports.rst``. -The text up to the first empty line in a commit message is the commit -title. Ideally it is a single short line of at most 72 characters, -summarizing the change. It is required to prefix it with the -subsystem or module you are changing. For instance, the prefix -could be "doc:", "osd:", or "common:". One can use:: +In addition to a base, or "target" branch, PRs have several other components: +the `PR title`_, the `PR description`_, labels, comments, etc. Of these, the PR +title and description are relevant for new contributors. - git log +PR title +^^^^^^^^ -for more examples. It is also conventional to use the imperative mood in the -commit title. For example:: +If your PR has only one commit, the PR title can be the same as the commit title +(and GitHub will suggest this). If the PR has multiple commits, do not accept +the title GitHub suggest. Either use the title of the most relevant commit, or +write your own title. In the latter case, use the same "subsystem: short +description" convention described in `Commit title`_ for the PR title, with +the following difference: the PR title describes the entire set of changes, +while the `Commit title`_ describes only the changes in a particular commit. - mds: add perf counter for finisher of MDSRank +Keep in mind that the PR titles feed directly into the script that generates +release notes and it is tedious to clean up non-conformant PR titles at release +time. This document places no limit on the length of PR titles, but be aware +that they are subject to editing as part of the release process. -or:: +PR description +^^^^^^^^^^^^^^ - osd: make the ClassHandler::mutex private +In addition to a title, the PR also has a description field, or "body". -For GitHub, please use this "subsystem: short description" convention for -naming pull requests (PRs). These titles feed directly into the script that -generates release notes and it is tedious to clean up non-conformant PR titles -at release time. This document places no limit on the length of PR titles, but -be aware that they are subject to editing as part of the release process. +The PR description is a place for summarizing the PR as a whole. It need not +duplicate information that is already in the commit messages. It can contain +notices to maintainers, links to tracker issues and other related information, +to-do lists, etc. The PR title and description should give readers a high-level +notion of what the PR is about, quickly enabling them to decide whether they +should take a closer look. -Commit message -^^^^^^^^^^^^^^ -Be as specific as possible. The WORST descriptions possible include -things like "update driver X", "bug fix for driver X", or "this patch -includes updates for subsystem X. Please apply." +Flag your changes for backport +------------------------------ -If your description starts to get long, that's a sign that you probably -need to split up your patch. See :ref:`split_changes`. +If you believe your changes should be backported to stable branches after the PR +is merged, open a tracker issue at https://tracker.ceph.com explaining: -When you submit or resubmit a patch or patch series, include the -complete patch description and justification for it. Don't just -say that this is version N of the patch (series). Don't expect the -patch merger to refer back to earlier patch versions or referenced -URLs to find the patch description and put that into the patch. -I.e., the patch (series) and its description should be self-contained. -This benefits both the patch merger(s) and reviewers. Some reviewers -probably didn't even receive earlier versions of the patch. +1. what bug is fixed +2. why does the bug need to be fixed in -Tag the commit -^^^^^^^^^^^^^^ +and fill out the Backport field in the tracker issue. For example:: -If the patch fixes a logged bug entry, refer to that bug entry by -URL. In particular, if this patch fixes one or more issues -tracked by http://tracker.ceph.com, consider adding a ``Fixes:`` tag to -connect this change to addressed issue(s). So a line saying :: + Backport: mimic, nautilus - Fixes: http://tracker.ceph.com/issues/12345 +For information on how backports are done in the Ceph project, refer to the +document ``SubmittingPatches-backports.rst``. -is added before the ``Signed-off-by:`` line stating that this commit -addresses http://tracker.ceph.com/issues/12345. It helps the reviewer to -get more context of this bug, so she/he can hence update the issue on -the bug tracker accordingly. -So a typical commit message for revising the document could look like:: +Test your changes +----------------- - doc: add "--foo" option to bar +Before opening your PR, it's a good idea to run tests on your patchset. Doing +that is simple, though the process can take a long time to complete, especially +on older machines with less memory and spinning disks. - * update the man page for bar with the newly added "--foo" option. - * fix a typo +The most simple test is to verify that your patchset builds, at least in your +own development environment. The commands for this are:: - Fixes: http://tracker.ceph.com/issues/12345 - Signed-off-by: Random J Developer + ./install-deps.sh + ./do_cmake.sh + make -.. _split_changes: +Ceph comes with a battery of tests that can be run on a single machine. These +are collectively referred to as "make check", and can be run by executing the +following command:: -4. Separate your changes ------------------------- + ./run-make-check.sh -Separate *logical changes* into a single patch file. +If your patchset does not build, or if one or more of the "make check" tests +fails, but the error shown is not obviously related to your patchset, don't let +that dissuade you from opening a PR. The Ceph project has a Jenkins instance +which will build your PR branch and run "make check" on it in a controlled +environment. -For example, if your changes include both bug fixes and performance -enhancements for a single driver, separate those changes into two -or more patches. If your changes include an API update, and a new -driver which uses that new API, separate those into two patches. +Once your patchset builds and passes "make check", you can run even more tests +on it by issuing the following commands:: -On the other hand, if you make a single change to numerous files, -group those changes into a single patch. Thus a single logical change -is contained within a single patch. + cd build + ../qa/run-standalone.sh -If one patch depends on another patch in order for a change to be -complete, that is OK. Simply note "this patch depends on patch X" -in your patch description. +Like "make check", the standalone tests take a long time to run. They also +produce voluminous output. If one or more of the standalone tests fails, it's +likely the relevant part of the output will have scrolled off your screen or +gotten swapped out of your buffer. Therefore, it makes sense to capture the +output in a file for later analysis. -If you cannot condense your patch set into a smaller set of patches, -then only post say 15 or so at a time and wait for review and integration. -5. Document your changes ------------------------- +Document your changes +--------------------- -If you have added or modified any user-facing functionality, such -as CLI commands or their output, then the patch series or pull request -must include appropriate updates to documentation. +If you have added or modified any user-facing functionality, such as CLI +commands or their output, then the pull request must include appropriate updates +to documentation. It is the submitter's responsibility to make the changes, and the reviewer's responsibility to make sure they are not merging changes that do not have the needed updates to documentation. -Where there are areas that have absent documentation, or there is no -clear place to note the change that is being made, the reviewer should -contact the component lead, who should arrange for the missing section -to be created with sufficient detail for the patch submitter to -document their changes. +Where there are areas that have absent documentation, or there is no clear place +to note the change that is being made, the reviewer should contact the component +lead, who should arrange for the missing section to be created with sufficient +detail for the PR submitter to document their changes. When writing and/or editing documentation, follow the Google Developer Documentation Style Guide: https://developers.google.com/style/ - -6. Style check your changes ---------------------------- - -Check your patch for basic style violations, details of which can be -found in CodingStyle. - - -7. No MIME, no links, no compression, no attachments. Just plain text ----------------------------------------------------------------------- - -Developers need to be able to read and comment on the changes you are -submitting. It is important for a kernel developer to be able to -"quote" your changes, using standard e-mail tools, so that they may -comment on specific portions of your code. - -For this reason, all patches should be submitting e-mail "inline". -WARNING: Be wary of your editor's word-wrap corrupting your patch, -if you choose to cut-n-paste your patch. - -Do not attach the patch as a MIME attachment, compressed or not. -Many popular e-mail applications will not always transmit a MIME -attachment as plain text, making it impossible to comment on your -code. A MIME attachment also takes Linus a bit more time to process, -decreasing the likelihood of your MIME-attached change being accepted. - -Exception: If your mailer is mangling patches then someone may ask -you to re-send them using MIME. -