Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1828573003:
Add support for swarming priority and expiration in the test spec (Closed)
|
Created:
4 years, 9 months ago by Sergiy Byelozyorov Modified:
4 years, 9 months ago CC:
chromium-reviews, infra-reviews+build_chromium.org, Ken Russell (switch to Gerrit), kjellander-cc_chromium.org Base URL:
https://chromium.googlesource.com/chromium/tools/build.git@master Target Ref:
refs/heads/master Project:
build Visibility:
Public. |
DescriptionAdd support for swarming priority and expiration in the test spec
R=maruel@chromium.org, phajdan.jr@chromium.org
CC=kbr@chromium.org
BUG=582625
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=299493
Patch Set 1 #Patch Set 2 : Fix #Patch Set 3 : Fix #
Total comments: 8
Patch Set 4 : Addressed comments #
Total comments: 7
Patch Set 5 : Rebase #Patch Set 6 : Addressed comments #
Messages
Total messages: 21 (5 generated)
Description was changed from ========== Add support for swarming priority and expiration in the test spec R=maruel@chromium.org, phajdan.jr@chromium.org BUG=582625 ========== to ========== Add support for swarming priority and expiration in the test spec R=maruel@chromium.org, phajdan.jr@chromium.org CC=kbr@chromium.org BUG=582625 ==========
https://codereview.chromium.org/1828573003/diff/40001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/swarming/api.py (right): https://codereview.chromium.org/1828573003/diff/40001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/swarming/api.py:15: NAMED_PRIORITIES = { I wonder if it should go in chromium_tests or here. https://codereview.chromium.org/1828573003/diff/40001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/swarming/api.py:16: 'min': 1, # higest possible priority The infrastructure should never use anything below 10 (e.g. higher priority than 10) so change 'min' accordingly. https://codereview.chromium.org/1828573003/diff/40001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/swarming/api.py:21: 'max': 255, # lowest possible priority You should use 'highest' and 'lowest' to reduce confusion. https://codereview.chromium.org/1828573003/diff/40001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/swarming/api.py:596: return int(adjustment) I prefer to not support that and just use names for now. It's more futureproof.
https://codereview.chromium.org/1828573003/diff/40001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/swarming/api.py (right): https://codereview.chromium.org/1828573003/diff/40001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/swarming/api.py:15: NAMED_PRIORITIES = { On 2016/03/23 12:55:32, M-A Ruel wrote: > I wonder if it should go in chromium_tests or here. I don't have a strong preference. But if it goes to chromium_tests, I can't use 'default' and 'min'/'max' in this module. https://codereview.chromium.org/1828573003/diff/40001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/swarming/api.py:16: 'min': 1, # higest possible priority On 2016/03/23 12:55:32, M-A Ruel wrote: > The infrastructure should never use anything below 10 (e.g. higher priority than > 10) so change 'min' accordingly. Done. Note that this also changes the logic of the default_priority setter as it checks for values to be between min and max. https://codereview.chromium.org/1828573003/diff/40001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/swarming/api.py:21: 'max': 255, # lowest possible priority On 2016/03/23 12:55:31, M-A Ruel wrote: > You should use 'highest' and 'lowest' to reduce confusion. Done. https://codereview.chromium.org/1828573003/diff/40001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/swarming/api.py:596: return int(adjustment) On 2016/03/23 12:55:31, M-A Ruel wrote: > I prefer to not support that and just use names for now. It's more futureproof. Done.
phajdan.jr@chromium.org changed reviewers: + kbr@chromium.org
+kbr In general I'm fine with this. https://codereview.chromium.org/1828573003/diff/60001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/swarming/api.py (right): https://codereview.chromium.org/1828573003/diff/60001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/swarming/api.py:15: NAMED_PRIORITIES = { Wasn't the idea for these to be relative rather than absolute?
https://codereview.chromium.org/1828573003/diff/60001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/swarming/api.py (right): https://codereview.chromium.org/1828573003/diff/60001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/swarming/api.py:15: NAMED_PRIORITIES = { On 2016/03/23 15:08:04, Paweł Hajdan Jr. wrote: > Wasn't the idea for these to be relative rather than absolute? I was only briefed by Ken on what exactly we were planning to do, so I may have missed the point. Relative to what?
Thanks for working on this. https://codereview.chromium.org/1828573003/diff/60001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/swarming/api.py (right): https://codereview.chromium.org/1828573003/diff/60001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/swarming/api.py:15: NAMED_PRIORITIES = { On 2016/03/23 15:09:53, Sergiy Byelozyorov wrote: > On 2016/03/23 15:08:04, Paweł Hajdan Jr. wrote: > > Wasn't the idea for these to be relative rather than absolute? > > I was only briefed by Ken on what exactly we were planning to do, so I may have > missed the point. Relative to what? Each entry in the src/testing/buildbot JSON files describes launching the test both on the CQ and the waterfalls, but the priorities for those jobs computed by the recipe code are different. If we want to be able to choose a "lower" priority for one particular long-running test, then "lower" for example needs to mean "lower with respect to the default priority for either a presubmit or non-presubmit job, depending on what type this job is."
https://codereview.chromium.org/1828573003/diff/60001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/swarming/api.py (right): https://codereview.chromium.org/1828573003/diff/60001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/swarming/api.py:15: NAMED_PRIORITIES = { On 2016/03/23 17:11:29, Ken Russell wrote: > On 2016/03/23 15:09:53, Sergiy Byelozyorov wrote: > > On 2016/03/23 15:08:04, Paweł Hajdan Jr. wrote: > > > Wasn't the idea for these to be relative rather than absolute? > > > > I was only briefed by Ken on what exactly we were planning to do, so I may > have > > missed the point. Relative to what? > > Each entry in the src/testing/buildbot JSON files describes launching the test > both on the CQ and the waterfalls, but the priorities for those jobs computed by > the recipe code are different. If we want to be able to choose a "lower" > priority for one particular long-running test, then "lower" for example needs to > mean "lower with respect to the default priority for either a presubmit or > non-presubmit job, depending on what type this job is." (I hadn't figured out what the sliding scale should be for precommit and non-precommit jobs. Both should have some range of priorities that are selectable via these keywords.)
https://codereview.chromium.org/1828573003/diff/60001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/swarming/api.py (right): https://codereview.chromium.org/1828573003/diff/60001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/swarming/api.py:15: NAMED_PRIORITIES = { On 2016/03/23 17:12:45, Ken Russell wrote: > On 2016/03/23 17:11:29, Ken Russell wrote: > > On 2016/03/23 15:09:53, Sergiy Byelozyorov wrote: > > > On 2016/03/23 15:08:04, Paweł Hajdan Jr. wrote: > > > > Wasn't the idea for these to be relative rather than absolute? > > > > > > I was only briefed by Ken on what exactly we were planning to do, so I may > > have > > > missed the point. Relative to what? > > > > Each entry in the src/testing/buildbot JSON files describes launching the test > > both on the CQ and the waterfalls, but the priorities for those jobs computed > by > > the recipe code are different. If we want to be able to choose a "lower" > > priority for one particular long-running test, then "lower" for example needs > to > > mean "lower with respect to the default priority for either a presubmit or > > non-presubmit job, depending on what type this job is." > > (I hadn't figured out what the sliding scale should be for precommit and > non-precommit jobs. Both should have some range of priorities that are > selectable via these keywords.) Let me see if I understand this on an example of a gpu_tests step. Normally on CQ it is scheduled with 30 and on waterfall with 25. So if one sets priority_adjustment to higher for this test, then we'll deduct X from the priority when running on CQ, but keep the priority for waterfall. This way, if X is 5, we'll make CQ schedule gpu_tests with same priority (30-5=25) as on waterfall, and, if X is 10, then with higher priority (30-10=20) than waterfall.
https://codereview.chromium.org/1828573003/diff/60001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/swarming/api.py (right): https://codereview.chromium.org/1828573003/diff/60001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/swarming/api.py:15: NAMED_PRIORITIES = { On 2016/03/23 18:08:24, Sergiy Byelozyorov wrote: > On 2016/03/23 17:12:45, Ken Russell wrote: > > On 2016/03/23 17:11:29, Ken Russell wrote: > > > On 2016/03/23 15:09:53, Sergiy Byelozyorov wrote: > > > > On 2016/03/23 15:08:04, Paweł Hajdan Jr. wrote: > > > > > Wasn't the idea for these to be relative rather than absolute? > > > > > > > > I was only briefed by Ken on what exactly we were planning to do, so I may > > > have > > > > missed the point. Relative to what? > > > > > > Each entry in the src/testing/buildbot JSON files describes launching the > test > > > both on the CQ and the waterfalls, but the priorities for those jobs > computed > > by > > > the recipe code are different. If we want to be able to choose a "lower" > > > priority for one particular long-running test, then "lower" for example > needs > > to > > > mean "lower with respect to the default priority for either a presubmit or > > > non-presubmit job, depending on what type this job is." > > > > (I hadn't figured out what the sliding scale should be for precommit and > > non-precommit jobs. Both should have some range of priorities that are > > selectable via these keywords.) > > Let me see if I understand this on an example of a gpu_tests step. Normally on > CQ it is scheduled with 30 and on waterfall with 25. So if one sets > priority_adjustment to higher for this test, then we'll deduct X from the > priority when running on CQ, but keep the priority for waterfall. This way, if X > is 5, we'll make CQ schedule gpu_tests with same priority (30-5=25) as on > waterfall, and, if X is 10, then with higher priority (30-10=20) than waterfall. The intent is that the priority_adjustment apply to the jobs on both the CQ and waterfall. So if the CQ schedules jobs between priorities 25 and 35, and the waterfall between priorities 20 and 30, then the "default" priority would set it to 30 for a CQ job and 25 for a waterfall job. "higher" priority might set it to 27 for a CQ job and 22 for a waterfall job.
On 2016/03/23 21:35:42, Ken Russell wrote: > https://codereview.chromium.org/1828573003/diff/60001/scripts/slave/recipe_mo... > File scripts/slave/recipe_modules/swarming/api.py (right): > > https://codereview.chromium.org/1828573003/diff/60001/scripts/slave/recipe_mo... > scripts/slave/recipe_modules/swarming/api.py:15: NAMED_PRIORITIES = { > On 2016/03/23 18:08:24, Sergiy Byelozyorov wrote: > > On 2016/03/23 17:12:45, Ken Russell wrote: > > > On 2016/03/23 17:11:29, Ken Russell wrote: > > > > On 2016/03/23 15:09:53, Sergiy Byelozyorov wrote: > > > > > On 2016/03/23 15:08:04, Paweł Hajdan Jr. wrote: > > > > > > Wasn't the idea for these to be relative rather than absolute? > > > > > > > > > > I was only briefed by Ken on what exactly we were planning to do, so I > may > > > > have > > > > > missed the point. Relative to what? > > > > > > > > Each entry in the src/testing/buildbot JSON files describes launching the > > test > > > > both on the CQ and the waterfalls, but the priorities for those jobs > > computed > > > by > > > > the recipe code are different. If we want to be able to choose a "lower" > > > > priority for one particular long-running test, then "lower" for example > > needs > > > to > > > > mean "lower with respect to the default priority for either a presubmit or > > > > non-presubmit job, depending on what type this job is." > > > > > > (I hadn't figured out what the sliding scale should be for precommit and > > > non-precommit jobs. Both should have some range of priorities that are > > > selectable via these keywords.) > > > > Let me see if I understand this on an example of a gpu_tests step. Normally on > > CQ it is scheduled with 30 and on waterfall with 25. So if one sets > > priority_adjustment to higher for this test, then we'll deduct X from the > > priority when running on CQ, but keep the priority for waterfall. This way, if > X > > is 5, we'll make CQ schedule gpu_tests with same priority (30-5=25) as on > > waterfall, and, if X is 10, then with higher priority (30-10=20) than > waterfall. > > The intent is that the priority_adjustment apply to the jobs on both the CQ and > waterfall. So if the CQ schedules jobs between priorities 25 and 35, and the > waterfall between priorities 20 and 30, then the "default" priority would set it > to 30 for a CQ job and 25 for a waterfall job. "higher" priority might set it to > 27 for a CQ job and 22 for a waterfall job. What Ken said. Ken's request is more about relative task priority for the tests that require priority bumping compared to the others.
Patchset #5 (id:80001) has been deleted
https://codereview.chromium.org/1828573003/diff/60001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/swarming/api.py (right): https://codereview.chromium.org/1828573003/diff/60001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/swarming/api.py:15: NAMED_PRIORITIES = { On 2016/03/23 21:35:42, Ken Russell wrote: > On 2016/03/23 18:08:24, Sergiy Byelozyorov wrote: > > On 2016/03/23 17:12:45, Ken Russell wrote: > > > On 2016/03/23 17:11:29, Ken Russell wrote: > > > > On 2016/03/23 15:09:53, Sergiy Byelozyorov wrote: > > > > > On 2016/03/23 15:08:04, Paweł Hajdan Jr. wrote: > > > > > > Wasn't the idea for these to be relative rather than absolute? > > > > > > > > > > I was only briefed by Ken on what exactly we were planning to do, so I > may > > > > have > > > > > missed the point. Relative to what? > > > > > > > > Each entry in the src/testing/buildbot JSON files describes launching the > > test > > > > both on the CQ and the waterfalls, but the priorities for those jobs > > computed > > > by > > > > the recipe code are different. If we want to be able to choose a "lower" > > > > priority for one particular long-running test, then "lower" for example > > needs > > > to > > > > mean "lower with respect to the default priority for either a presubmit or > > > > non-presubmit job, depending on what type this job is." > > > > > > (I hadn't figured out what the sliding scale should be for precommit and > > > non-precommit jobs. Both should have some range of priorities that are > > > selectable via these keywords.) > > > > Let me see if I understand this on an example of a gpu_tests step. Normally on > > CQ it is scheduled with 30 and on waterfall with 25. So if one sets > > priority_adjustment to higher for this test, then we'll deduct X from the > > priority when running on CQ, but keep the priority for waterfall. This way, if > X > > is 5, we'll make CQ schedule gpu_tests with same priority (30-5=25) as on > > waterfall, and, if X is 10, then with higher priority (30-10=20) than > waterfall. > > The intent is that the priority_adjustment apply to the jobs on both the CQ and > waterfall. So if the CQ schedules jobs between priorities 25 and 35, and the > waterfall between priorities 20 and 30, then the "default" priority would set it > to 30 for a CQ job and 25 for a waterfall job. "higher" priority might set it to > 27 for a CQ job and 22 for a waterfall job. Done.
Awesome. Thank you for implementing this. I don't know whether the precise numbers are the desired ones -- I had thought there would be some sort of interpolation, rather than addition of absolute numbers -- but it seems fine as a start. LGTM
Thanks Ken. M-A or Pawel, can you also PTAL?
LGTM
The CQ bit was checked by sergiyb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1828573003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1828573003/120001
Message was sent while issue was closed.
Description was changed from ========== Add support for swarming priority and expiration in the test spec R=maruel@chromium.org, phajdan.jr@chromium.org CC=kbr@chromium.org BUG=582625 ========== to ========== Add support for swarming priority and expiration in the test spec R=maruel@chromium.org, phajdan.jr@chromium.org CC=kbr@chromium.org BUG=582625 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=299493 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001) as http://src.chromium.org/viewvc/chrome?view=rev&revision=299493 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
