]>
Commit | Line | Data |
---|---|---|
9f95a23c TL |
1 | Submitting Patches to Ceph - Backports |
2 | ====================================== | |
3 | ||
4 | Most likely you're reading this because you intend to submit a GitHub pull | |
5 | request ("PR") targeting one of the stable branches ("nautilus", etc.) at | |
6 | https://github.com/ceph/ceph. | |
7 | ||
8 | Before you open that PR, please read this entire document or, at the very least, | |
9 | the following two sections: `General principles`_ and `Cherry-picking rules`_. | |
10 | ||
11 | ||
12 | .. contents:: | |
13 | :depth: 3 | |
14 | ||
15 | ||
16 | General principles | |
17 | ------------------ | |
18 | ||
19 | To help the people who will review your backport, please state either in the | |
20 | backport PR, or in the backport tracker issue, or in the master tracker issue: | |
21 | ||
22 | 1. what bug is fixed | |
23 | 2. why this fix is the minimal way to do it | |
24 | 3. why does this need to be fixed in <release> | |
25 | ||
26 | The above should be followed especially in cases when the backport could be seen | |
27 | as introducing, into a stable branch, code that is not related to a particular | |
28 | bug or issue. | |
29 | ||
30 | Rationale: every modification of a stable branch carries a certain risk of | |
31 | introducing a regression. To minimize this risk, backports should be as | |
32 | straightforward and narrowly-targeted as possible. As a stable release series | |
33 | ages, the importance of following these general principles rises. | |
34 | ||
35 | ||
36 | Cherry-picking rules | |
37 | -------------------- | |
38 | ||
39 | The following rules, which have been codified from "best practices" developed | |
40 | over years of backporting, apply to the actual backport implementation: | |
41 | ||
42 | * all fixes should land in master first | |
43 | * commits to stable branches should be cherry-picked from master | |
44 | * 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. | |
f67539c2 | 45 | * when backporting a master PR to a stable branch, double-check that the backport PR contains cherry-picks of all of the master PR's commits. If any commit needs to be omitted, declare and explain this in the PR. |
9f95a23c | 46 | * cherry-picks must be done using ``git cherry-pick -x`` |
f67539c2 | 47 | * if a cherry-pick from master is not feasible and a direct fix is being undertaken, this must be explained |
9f95a23c TL |
48 | * 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 |
49 | * the "Conflicts" section must mention all files where changes had to be made manually (not just conflicts flagged by git) | |
50 | * the "Conflicts" section should also describe the manual changes that were made | |
51 | * 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) | |
52 | ||
53 | For more information on tracker issues, see `Tracker workflow`_. | |
54 | ||
55 | For more information on conflict resolution and writing the "Conflicts" section, | |
56 | see `Conflict resolution`_. | |
57 | ||
58 | Adhering to these rules will make your backport easier for reviewers to | |
59 | understand. Not adhering to these rules creates additional work for reviewers | |
60 | and may cause your backport PR to be rejected. | |
61 | ||
62 | Notes on the cherry-picking rules | |
63 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | |
64 | ||
65 | What does "all fixes should land in master first" mean? What if I just need to | |
66 | fix the issue in <release>? | |
67 | ||
68 | As the person fixing the issue, you are required to first check whether the | |
69 | issue exists in master. If it does, then the proper course of action is to | |
70 | create a master tracker (see `Tracker workflow`_) and fix the issue in master, | |
71 | first, and only then cherry-pick the fix to the stable branches that have the | |
72 | issue. | |
73 | ||
74 | If the issue exists in the stable branch, but not in master, there are several | |
75 | possibilities: | |
76 | ||
77 | 1. it's a regression introduced into the stable branch by a bad backport | |
78 | 2. the issue was fixed in master by some massive refactoring that cannot be backported | |
79 | 3. the issue was already fixed in master by a cherry-pickable commit | |
80 | ||
81 | In cases 1 and 2, it's permissible to fix the issue directly in the most recent | |
82 | stable branch, subject to the rule "if a commit could not be cherry-picked from | |
83 | master, the commit message must explain why that was not possible". Once the | |
84 | fix has landed in the most recent stable branch, it can be cherry-picked to | |
85 | older stable branches from there. | |
86 | ||
87 | In case 3, the issue should be handled like any other backport - read on. | |
88 | ||
89 | ||
90 | Tracker workflow | |
91 | ---------------- | |
92 | ||
93 | Any change that is to be backported to multiple stable branches should have | |
94 | an associated tracker issue at https://tracker.ceph.com. | |
95 | ||
96 | For fixes already merged to master, this may have already been done - see the | |
97 | ``Fixes:`` line in the master PR. If the master PR has already been merged and | |
98 | there is no associated master tracker issue, you can create a master tracker | |
99 | issue and fill in the fields as described below. | |
100 | ||
101 | This master tracker issue should be in the "Bug" or "Feature" | |
102 | trackers of the relevant subproject under the "Ceph" parent project (or | |
103 | in the "Ceph" project itself if none of the subprojects are a good fit). | |
104 | The stable branches to which the master changes are to be cherry-picked should | |
105 | be listed in the "Backport" field. For example:: | |
106 | ||
107 | Backport: mimic, nautilus | |
108 | ||
109 | Once the PR targeting master is open, add the PR number assigned by GitHub to | |
110 | the tracker issue. For example, if the PR number is 99999:: | |
111 | ||
112 | Pull request ID: 99999 | |
113 | ||
114 | Once the master PR has been merged, after checking that the change really needs | |
115 | to be backported and the Backport field has been populated, change the master | |
116 | tracker issue's ``Status`` field to "Pending Backport". | |
117 | ||
118 | Status: Pending Backport | |
119 | ||
120 | If you do not have sufficient permissions to modify any field of the tracker | |
121 | issue, just add a comment describing what changes you would like to make. | |
122 | Someone with permissions will make the necessary modifications on your behalf. | |
123 | ||
124 | For straightforward backports, that's all that you (as the developer of the fix) | |
125 | need to do. Volunteers from the `Stable Releases and Backports team`_ will | |
126 | proceed to create Backport issues to track the necessary backports and stage the | |
127 | backports by opening GitHub PRs with the cherry-picks. If you don't want to | |
128 | wait, and provided you have sufficient permissions at https://tracker.ceph.com, | |
129 | you can `create Backport tracker issues` and `stage backports`_ yourself. In | |
130 | that case, read on. | |
131 | ||
132 | ||
133 | .. _`create backport tracker issues`: | |
134 | .. _`backport tracker issue`: | |
135 | ||
136 | Creating Backport tracker issues | |
137 | -------------------------------- | |
138 | ||
139 | To track backporting efforts, "backport tracker issues" can be created from | |
140 | a parent "master tracker issue". The master tracker issue is described in the | |
141 | previous section, `Tracker workflow`_. This section focuses the backport tracker | |
142 | issue. | |
143 | ||
144 | Once the entire `Tracker workflow`_ has been completed for the master issue, | |
145 | issues can be created in the Backport tracker for tracking the backporting work. | |
146 | ||
147 | Under ordinary circumstances, the developer who merges the master PR will flag | |
148 | the master tracker issue for backport by changing the Status to "Pending | |
149 | Backport", and volunteers from the `Stable Releases and Backports team`_ | |
150 | periodically create backport tracker issues by running the | |
151 | ``backport-create-issue`` script. They also do the actual backporting. But that | |
152 | does take time and you may not want to wait. | |
153 | ||
154 | You might be tempted to forge ahead and create the backport issues yourself. | |
155 | Please don't do that - it is difficult (bordering on impossible) to get all the | |
156 | fields correct when creating backport issues manually, and why even try when | |
157 | there is a script that gets it right every time? Setting up the script requires | |
158 | a small up-front time investment. Once that is done, creating backport issues | |
159 | becomes trivial. | |
160 | ||
161 | The backport-create-issue script | |
162 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | |
163 | ||
164 | The script used to create backport issues is located at | |
165 | ``src/script/backport-create-issue`` in the master branch. Though there might be | |
166 | an older version of this script in a stable branch, do not use it. Only use the | |
167 | most recent version from master. | |
168 | ||
169 | Once you have the script somewhere in your PATH, you can proceed to install the | |
170 | dependencies. | |
171 | ||
172 | The dependencies are: | |
173 | ||
174 | * python3 | |
175 | * python-redmine | |
176 | ||
177 | Python 3 should already be present on any recent Linux installation. The second | |
178 | dependency, `python-redmine`_, can be obtained from PyPi:: | |
179 | ||
180 | pip3 install --user python-redmine | |
181 | ||
182 | ||
183 | .. _`python-redmine`: https://pypi.org/project/python-redmine/ | |
184 | ||
185 | Then, try to run the script:: | |
186 | ||
187 | backport-create-issue --help | |
188 | ||
189 | This should produce a usage message. | |
190 | ||
191 | Finally, run the script to actually create the Backport issues. | |
192 | For example, if the tracker issue number is 55555:: | |
193 | ||
194 | backport-create-issue --user <tracker_username> --password <tracker_password> 55555 | |
195 | ||
196 | The script needs to know your https://tracker.ceph.com credentials in order to | |
197 | authenticate to Redmine. In lieu of providing your literal username and password | |
198 | on the command line, you could also obtain a REST API key ("My account" -> "API | |
f67539c2 | 199 | access key"), put it in ``~/.redmine_key`` and run the script like so:: |
9f95a23c | 200 | |
f67539c2 | 201 | backport-create-issue 55555 |
9f95a23c TL |
202 | |
203 | ||
204 | .. _`stage backports`: | |
205 | .. _`stage the backport`: | |
206 | .. _`staging a backport`: | |
207 | ||
208 | Opening a backport PR | |
209 | --------------------- | |
210 | ||
211 | Once the `Tracker workflow`_ is completed and the `backport tracker issue`_ has | |
212 | been created, it's time to open a backport PR. One possibility is to do this | |
213 | manually, while taking care to follow the `cherry-picking rules`_. However, this | |
214 | can result in a backport that is not properly staged. For example, the PR | |
215 | description might not contain a link to the `backport tracker issue`_ (a common | |
216 | oversight). You might even forget to update the `backport tracker issue`_. | |
217 | ||
218 | In the past, much time was lost, and much frustration caused, by the necessity | |
219 | of staging backports manually. Now, fortunately, there is a script available | |
220 | which automates the process and takes away most of the guesswork. | |
221 | ||
222 | The ceph-backport.sh script | |
223 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ | |
224 | ||
f67539c2 | 225 | Similar to the case of `creating backport tracker issues`_, staging the actual |
9f95a23c TL |
226 | backport PR and updating the Backport tracker issue is difficult - if not |
227 | impossible - to get right if you're doing it manually, and quickly becomes | |
228 | tedious if you do it more than once in a long while. | |
229 | ||
230 | The ``ceph-backport.sh`` script automates the entire process of cherry-picking | |
231 | the commits from the master PR, opening the GitHub backport PR, and | |
232 | cross-linking the GitHub backport PR with the correct Backport tracker issue. | |
233 | The script can also be used to good effect if you have already manually prepared | |
234 | the backport branch with the cherry-picks in it. | |
235 | ||
236 | The script is located at ``src/script/ceph-backport.sh`` in the ``master`` | |
237 | branch. Though there might be an older version of this script in a stable | |
f67539c2 TL |
238 | branch, do not use it. Only use the most recent version from the master branch. |
239 | To do this from anywhere and from any branch use the following | |
240 | alias that will use the most recent script in ``upstream/master`` of your | |
241 | local ceph clone on every call:: | |
9f95a23c | 242 | |
f67539c2 TL |
243 | alias ceph-backport="bash <(git --git-dir=$pathToCephClone/.git --no-pager show upstream/master:src/script/ceph-backport.sh)" |
244 | ||
245 | ``ceph-backport.sh`` is just a bash script, so the only dependency is ``bash`` | |
246 | itself, but it does need to be run in the top level of a local clone of | |
247 | ``ceph/ceph.git``. A small up-front time investment is required to get the | |
248 | script working in your environment. This is because the script needs to | |
249 | authenticate itself (i.e., as you) in order to use the GitHub and Redmine REST | |
250 | API services. | |
9f95a23c TL |
251 | |
252 | The script is self-documenting. Just run the script and proceed from there. | |
253 | ||
254 | Once the script has been set up properly, you can validate the setup like so:: | |
255 | ||
256 | ceph-backport.sh --setup | |
257 | ||
258 | Once you have this saying "Overall setup is OK", you have two options for | |
259 | staging the backport: either leave everything to the script, or prepare the | |
260 | backport branch yourself and use the script only for creating the PR and | |
261 | updating the Backport tracker issue. | |
262 | ||
263 | If you prefer to leave everything to the script, just provide the Backport | |
264 | tracker issue number to the script:: | |
265 | ||
266 | ceph-backport.sh 55555 | |
267 | ||
268 | The script will start by creating the backport branch in your local git clone. | |
269 | The script always uses the following format for naming the branch:: | |
270 | ||
271 | wip-<backport_issue_number>-<name_of_stable_branch> | |
272 | ||
273 | For example, if the Backport tracker issue number is 55555 and it's targeting | |
274 | the stable branch "nautilus", the backport branch would be named:: | |
275 | ||
276 | wip-55555-nautilus | |
277 | ||
278 | If you prefer to create the backport branch yourself, just do that. Be sure to | |
279 | name the backport branch as described above. (It's a good idea to use this | |
280 | branch naming convention for all your backporting work.) Then, run the script:: | |
281 | ||
282 | ceph-backport.sh 55555 | |
283 | ||
284 | The script will see that the backport branch already exists, and use it. | |
285 | ||
f67539c2 TL |
286 | Once the script hits the first cherry-pick conflict, it will no longer provide |
287 | any cherry-picking assistance, so in that case it's up to you to resolve the conflict(s) | |
288 | (as described in `Conflict resolution`_) and finish cherry-picking | |
289 | all of the remaining commits. Once you are satisfied that the backport is complete in | |
290 | your local branch, `ceph-backport.sh` can finish the job of creating the pull request | |
291 | and updating the backport tracker issue. To make that happen, just re-run the script | |
292 | exactly as you did before:: | |
293 | ||
294 | ceph-backport.sh $BACKPORT_TRACKER_ID | |
295 | ||
296 | The script will detect that it is running from a branch with the same name as the one it | |
297 | would normally create on the first run and continues after the cherry-picking phase. | |
298 | ||
299 | For a quick reference on CLI, that contains above information, you can run:: | |
300 | ||
301 | ceph-backport.sh --usage | |
302 | ||
9f95a23c TL |
303 | Conflict resolution |
304 | ^^^^^^^^^^^^^^^^^^^ | |
305 | ||
306 | If git reports conflicts, the script will abort to allow you to resolve the | |
307 | conflicts manually. | |
308 | ||
309 | Once the conflicts are resolved, complete the cherry-pick :: | |
310 | ||
311 | git cherry-pick --continue | |
312 | ||
313 | Git will present a draft commit message with a "Conflicts" section. | |
314 | ||
315 | Unfortunately, in recent versions of git, the Conflicts section is commented | |
316 | out. Since the Conflicts section is mandatory for Ceph backports that do not | |
317 | apply cleanly, you will need to uncomment the entire "Conflicts" section | |
f67539c2 | 318 | of the commit message before committing the cherry-pick. You can also |
9f95a23c TL |
319 | include commentary on what the conflicts were and how you resolved |
320 | them. For example:: | |
321 | ||
322 | Conflicts: | |
323 | src/foo/bar.cc | |
324 | - mimic does not have blatz; use batlo instead | |
325 | ||
326 | When editing the cherry-pick commit message, leave everything before the | |
327 | "cherry picked from" line unchanged. Any edits you make should be in the part | |
328 | following that line. Here is an example:: | |
329 | ||
330 | osd: check batlo before setting blatz | |
331 | ||
332 | Setting blatz requires special precautions. Check batlo first. | |
f67539c2 | 333 | |
9f95a23c TL |
334 | Fixes: https://tracker.ceph.com/issues/99999 |
335 | Signed-off-by: Random J Developer <random@developer.example.com> | |
336 | (cherry picked from commit 01d73020da12f40ccd95ea1e49cfcf663f1a3a75) | |
f67539c2 | 337 | |
9f95a23c TL |
338 | Conflicts: |
339 | src/osd/batlo.cc | |
340 | - add_batlo_check has an extra arg in newer code | |
341 | ||
342 | Naturally, the ``Fixes`` line points to the master issue. You might be tempted | |
343 | to modify it so it points to the backport issue, but - please - don't do that. | |
344 | First, the master issue points to all the backport issues, and second, *any* | |
345 | editing of the original commit message calls the entire backport into doubt, | |
346 | simply because there is no good reason for such editing. | |
347 | ||
348 | The part below the ``(cherry picked from commit ...)`` line is fair game for | |
349 | editing. If you need to add additional information to the cherry-pick commit | |
350 | message, append that information below this line. Once again: do not modify the | |
351 | original commit message. | |
352 | ||
f67539c2 TL |
353 | If you use `ceph-backport.sh` for your backport creation (which is recommended), |
354 | read up at the end of `The ceph-backport.sh script`_ on how to continue from here. | |
9f95a23c TL |
355 | |
356 | Labelling of backport PRs | |
357 | ------------------------- | |
358 | ||
359 | Once the backport PR is open, the first order of business is to set the | |
360 | Milestone tag to the stable release the backport PR is targeting. For example, | |
361 | if the PR is targeting "nautilus", set the Milestone tag to "nautilus". | |
362 | ||
f67539c2 TL |
363 | If you don't have sufficient GitHub permissions to set the Milestone, don't |
364 | worry. Members of the `Stable Releases and Backports team`_ periodically run | |
365 | a script (``ceph-backport.sh --milestones``) which scans all PRs targetting stable | |
366 | branches and automatically adds the correct Milestone tag if it is missing. | |
9f95a23c | 367 | |
f67539c2 TL |
368 | Next, check which component label was applied to the master PR corresponding to |
369 | this backport, and double-check that that label is applied to the backport PR as | |
370 | well. For example, if the master PR carries the component label "core", the | |
371 | backport PR should also get that label. | |
9f95a23c | 372 | |
f67539c2 TL |
373 | In general, it is the responsibility of the `Stable Releases and Backports |
374 | team`_ to ensure that backport PRs are properly labelled. If in doubt, just | |
375 | leave the labelling to them. | |
9f95a23c TL |
376 | |
377 | .. _`backport PR reviewing`: | |
378 | .. _`backport PR testing`: | |
379 | .. _`backport PR merging`: | |
380 | ||
381 | Reviewing, testing, and merging of backport PRs | |
382 | ----------------------------------------------- | |
383 | ||
384 | Once your backport PR is open and the Milestone is set properly, the | |
385 | `Stable Releases and Backports team` will take care of getting the PR | |
386 | reviewed and tested. Once the PR is reviewed and tested, it will be merged. | |
387 | ||
388 | If you would like to facilitate this process, you can solicit reviews and run | |
389 | integration tests on the PR. In this case, add comments to the PR describing the | |
390 | tests you ran and their results. | |
391 | ||
392 | Once the PR has been reviewed and deemed to have undergone sufficient testing, | |
393 | it will be merged. Even if you have sufficient GitHub permissions to merge the | |
394 | PR, please do *not* merge it yourself. (Uncontrolled merging to stable branches | |
395 | unnecessarily complicates the release preparation process, which is done by | |
396 | volunteers.) | |
397 | ||
398 | ||
399 | Stable Releases and Backports team | |
400 | ---------------------------------- | |
401 | ||
402 | Ceph has a `Stable Releases and Backports`_ team, staffed by volunteers, | |
403 | which is charged with maintaining the stable releases and backporting bugfixes | |
404 | from the master branch to them. (That team maintains a wiki, accessible by | |
405 | clicking the `Stable Releases and Backports`_ link, which describes various | |
406 | workflows in the backporting lifecycle.) | |
407 | ||
408 | .. _`Stable Releases and Backports`: http://tracker.ceph.com/projects/ceph-releases/wiki | |
409 | ||
410 | Ordinarily, it is enough to fill out the "Backport" field in the bug (tracker | |
411 | issue). The volunteers from the Stable Releases and Backports team will | |
412 | backport the fix, run regression tests on it, and include it in one or more | |
413 | future point releases. | |
414 | ||
415 |