yes, gerrit UI is not the right label for this, can you please assign a
proper one so that this issue does not get lost in the noise?

Can you give an example invocation and output illustrating the issue?

Where does the 'gerrit' command line tool you are referring to come from? I'm not aware of a command line tool named 'gerrit' that comes from the Gerrit project.

'gerrit' command is a part of this chrome os repository:

https://chromium.googlesource.com/chromiumos/chromite

it's bin/gerrit in that tree.

What I believe it does is sending queries to gerrit server.

At a certain point I had a stack of 70 patches in a certain branch, so I tried to change their state using a script.

this command lists all the patches which match that filter:

gerrit search 'is:open branch:firmware-cr50-9308.B'

but they seem to be reported in no particular order.

Moving this issue into the Chromium monorail.

Moved issue gerrit:6631 to now be issue chromium:738558.

The `gerrit` cmdline tool is also not maintained by the chromium infra / ChOps org, since it is part of chromite. Mike, I think this is your baliwick?

`gerrit todo` and `gerrit search` will sort their results based on the CL number because gerrit, afaict, does not support sorting results itself.  we could add an --unsorted option or something so you get back results in the order gerrit returned them, but i'm guessing that wouldn't help you.  using your example search, it should be fairly obvious that we're sorting when all the CLs are returned in increasing order.

when it comes to commands like `gerrit review`, then imo, the tool should not be doing any inspection of the arguments at all.  if you ran:
  gerrit review 33 44 55 66 2

then gerrit better well apply the +2 to the CLs in the order you specified.  that means first 33, then 44, then 55, then 66.

so if you want a different order applied, you can sort them how you want.

that's the thing, the order 'gerrit' command presents the patches is not the order they are stacked up.

Maybe gerrit backend does present the patches in the right order and gerrit command line tool messes it up?


As of now doing 'gerrit review' on the list of CL numbers as returned by 'gerrit search' causes +2 applied out of order compared to how it should have been.

As a result the gerrit command is useless in a situation where it would have been the most beneficial (clicking +2 on 70 patches through web interface is pretty tedious)

i don't think gerrit guarantees any ordering for search results.  i posted a CL to support --sort=unsorted so you can see what it gives back.  it's kind of unusual to have an ordering that doesn't generally line up with CL numbers considering that roughly matches the order they were uploaded.

that said, the precq/cq infra is supposed to ignore freshly marked CLs for a few min precisely so that people can batch them all at once.  if you had 70 CLs with an atomic CQ-DEPEND chain, then there is literally no ordering of marking that'd be correct.  either they're all marked, or none are.

oh well, this delay does not seem to be happening on branches - the merge is attempted before 'gerrit' invocation finishes running, and it does not run for longer than a minute.

Maybe there should be a bug against precq/cq instead.

you'll need to provide actual examples of this not working.  i don't see any links here and the one query you posted yields no results.

Assigning to vbendeb@ to provide samples of how to reproduce
the problem.

the way to reproduce the problem is described in #5 above.

I had 70 outstanding patches cherry-picked from master to firmware-cr50-9308.B and tried to mark them ready by running gerrit review <cl number list> 2 

where <cl number list> was the list generated by

gerrit search 'is:open branch:firmware-cr50-9308.B' | awk -F/ '{print $6}' | xargs

Like vapier@, when I try the command, nothing comes out:

: jrbarnette $REPOS/cros.base/chromite; bin/gerrit search 'is:open branch:firmware-cr50-9308.B'
: jrbarnette $REPOS/cros.base/chromite; 

However, reading the bug a bit more, it looks like the gerrit
backend doesn't support the request.  That would mean enhancing
the command in chromite.

There's nothing wrong with the proposed enhancement that I know
of, but I doubt the work will get much traction.

there's no need to awk the output.  that's the point of the --raw flag -- it'll just dump the CL #s directly.

Recapturing the issue for the benefit of sop@ and dborowitz@

Sometimes during branch reconciliations there is a situation where there is a large stack of patches, which would take forever to mark ready using the web interface.

Chrome OS chroot includes a utility 'gerrit' which allows to control the server over command line, in particular to search for outstanding patches and mark them reviewed (+2).

The issue is that the server returns the patch numbers in no particular order, so if the script on the user side gets the list of outstanding patches as presented by server and marks them +2, the server tries merging the patches, but if the ones at the top of the stack are marked +2 before those at the bottom of the stack, the server rejects the top patches.

There should be a way to query the server such that the patches are returned in the stack order, or there should be time window of a few minutes so that the entire stack could be marked +2 before the server declares that some patches are missing.

The order that search results are returned is always last-modified-first, which of course may be completely different from topo order of the commits. Nothing's impossible, but supporting different sort orders is a moderately big change to Gerrit's search architecture.

Note that I said "search results". There is another endpoint, Get Related Changes, which is not a search and only applies to a single change, but does guarantee topo ordering:
https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#get-related-changes
(Although the exact semantics are not the simplest thing in the world to explain.)

Would it make sense for the 'gerrit' utility to use this endpoint in some cases?

I'm confused -- the gerrit server doesn't merge patches based on a vote of +2. That just marks them as submittable, it doesn't actually submit them as well.

If you want to get a stack of related changes in the correct order, use the Get Related Changes API (https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#get-related-changes), which returns an ordered list of changes which depend on each other.

I think there's a legitimate feature request here for the gerrit Query Changes API to allow sorting by various parameters, but I don't think that asking for the gerrit server to somehow implicitly batch +2 votes across changes makes any sense.

> I'm confused -- the gerrit server doesn't merge patches based on a vote of +2. That just marks them as submittable, it doesn't actually submit them as well.

Right. I'm still not sure what the script is actually doing here. Is it like:

foreach change in some search {
  vote +2 on change
  submit change
}

This is not how a human would do it, or how Gerrit works. Instead, it should do:

foreach change in some search {
  vote +2 on change
}
submit tipmost change (or any change, if they share a topic)

right, I misspoke there, it is not gerrit who actually merges the patches.

foreach change in some search {
  vote +2 on change
}

is exactly what I do, it's just the merge happens to soon.

It looks like 'get related changes' API is what's needed here, I'll take a look into it.

> it's just the merge happens to soon.

Still confused. If the script isn't responsible for merging the changes (aka clicking submit), then what is? Why is that other mystery process racing with the script?

right, i still don't think this is a bug in GoB or chromite's gerrit tool.  it sounds like CrOS's precq is kicking in faster than it should have.

but again, i can only guess because we still don't have any logs/references to failing CLs as we've asked for multiple times now.

I don't see anyone asking "for multiple times now" for any logs, what logs would those be?

The query from #15 is just an example, it does not return anything any more, because all patches have been merged.

the bottom line is, this is not a gerrit server or gerrit command line tool problem, even though it could be solved by enhancing the command line tool.

you claimed CLs got rejected, but you haven't linked to any.  we've asked explicitly for logs/links in comment #13, comment #16, comment #24, and now comment #26.

Hopefully the issue is clear now, even without logs/links: it would be a good enhancement to the 'gerrit' command tool to allow querying related patches in the proper order.

no, it is still not clear.  you haven't defined "proper order".  if you're referring to CQ-DEPEND ordering, as i already said, it's not possible for that to work (like atomic loops), so the only way to handle it is in the cq/precq bot logic, and we should already be doing it there.

as others have said, there is no "server submitting" here.  `gerrit review` only sets the Code-Review label and that's it.  `gerrit submit` would actually submit the changes, but you haven't said anywhere that's what you ran, only the review subcommand.

so unless you have logs to CLs that were improperly attempted to be merged by precq bots, i have no idea what you're talking about.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/chromite/+/b62313ac518c470f701615fe8c6664480b199f8b

commit b62313ac518c470f701615fe8c6664480b199f8b
Author: Mike Frysinger <vapier@chromium.org>
Date: Thu Jul 06 20:59:21 2017

gerrit: support unsorted search results

BUG=chromium:738558
TEST=`gerrit --sort=unsorted todo` returned results in the server order

Change-Id: I70c433231e2a51fe9aab2049d63861719284454b
Reviewed-on: https://chromium-review.googlesource.com/558149
Commit-Ready: Mike Frysinger <vapier@chromium.org>
Tested-by: Mike Frysinger <vapier@chromium.org>
Reviewed-by: Vadim Bendebury <vbendeb@chromium.org>

[modify] https://crrev.com/b62313ac518c470f701615fe8c6664480b199f8b/scripts/gerrit.py

here is the problem happening again.

there were 30 outstanding patches in the firmware-cr50-9308.B branch retrieved using "gerrit search 'is:open branch:firmware-cr50-9308.B'":

598513 598514 598515 598516 598517 598518 598519 598520 598521 598522 598523 598524 598525 598526 599487 599488 599489 599490 599491 599492 599493 599494 599495 599496 599497 599498 599499 599500 599501 599502 601426

I ran 'gerrit review' command to set review status to for this list to 2. 7 patches got merged, the rest were rejected with messages like 

"The Pre-Commit Queue failed to submit your change in https://luci-milo.appspot.com/buildbot/chromeos/pre-cq-launcher/9692 . CL:599492 depends on CL:599491, which depends on CL:599490, which depends on CL:599489, which depends on CL:599488, which depends on CL:599487, which depends on CL:598526, which depends on CL:598525, which depends on CL:598524, which depends on CL:598523, which depends on CL:598522, which depends on CL:598521, which was rejected by the CQ."

I re-ran this with the new list of patches, again retrieved using gerrit search 'is:open branch:firmware-cr50-9308.B'. This time four more patches got merged, 19 are still outstanding, for the same reason.

Do you need any more information to look into this? 

Adding Aviv for commit queue insights.

that looks like a bug in the pre-cq, not the gerrit tool

the reduced test case is just these two CLs:
  https://chromium-review.googlesource.com/598519
  https://chromium-review.googlesource.com/598520
they were marked Verified+1 at 09:27 and CQ+1 at 09:28.

the pre-cq log is here (and attached PreCQLauncher):
  https://luci-milo.appspot.com/buildbot/chromeos/pre-cq-launcher/9692

the pre-cq started tracking them at 09:27:59, but then rejected them a scant three minutes later at 09:30:45.
09:30:39: INFO: Attempting to create transaction for vbendeb:599501:b4d5924f
09:30:39: INFO: Failed creating transaction for vbendeb:599501:b4d5924f: CL:599501 depends on CL:599500, which depends on CL:599499, which was rejected by the CQ.
09:30:39: INFO: Attempting to create transaction for vbendeb:601426:adba3fd8
09:30:40: INFO: Failed creating transaction for vbendeb:601426:adba3fd8: CL:601426 depends on CL:599502, which was rejected by the CQ.
09:30:40: INFO: Attempting to create transaction for vbendeb:598521:1daf0a85
09:30:40: INFO: Failed creating transaction for vbendeb:598521:1daf0a85: CL:598521 depends on CL:598520, which depends on CL:598519, which was rejected by the CQ.
09:30:40: INFO: Attempting to create transaction for jeffandersen:*425148:*56377ffd
09:30:41: INFO: Attempting to create transaction for jeffandersen:*425149:*8178ab0d
09:30:41: INFO: Attempting to create transaction for vbendeb:598520:2ece3bbb
09:30:41: INFO: Failed creating transaction for vbendeb:598520:2ece3bbb: CL:598520 depends on CL:598519, which was rejected by the CQ.
09:30:41: INFO: Attempting to create transaction for vbendeb:599500:19d8c8bd
09:30:41: INFO: Failed creating transaction for vbendeb:599500:19d8c8bd: CL:599500 depends on CL:599499, which was rejected by the CQ.

the pre-cq needs more lag time when it comes to auto-submitting like we have with the cq.

you could have saved everyone a lot of time if you just provided these details to start with.

Deleted: precq.log 2.4 MB

precq.log 2.4 MB View Download

commit queue fix should be simple, but the gerrit tool could have been smarter and provide an explicit option to retrieve a stack of patches in order.

i doubt the ordering is relevant to this bug

that said, you didn't say whether the already existing "deps" subcommand works

nxia can you take a look when you have a chance?

Re: #33

the "deps" subcommand help message reads "List CLs matching a query, and all transitive dependencies of those CLs", it is not clear what query is meant here.

I tried passing it just CL numbers, and it seems to be doing the right thing, most of the time:

There was a stack of 21 patches on gerrit, 618279 at the bottom one and  618399 at the top. 

And these are results of running "deps" on those:

vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv
$ gerrit deps 618279
https://chromium-review.googlesource.com/#/c/618279/ CR: 2 CQ: 0 V: 0 chromiumos/platform/ec Add non-volatile flash counter
$ gerrit deps 618399
08:59:28: WARNING: A transient error occured while querying chrome-internal-review.googlesource.com:

GET /a/changes/390230/detail?o=CURRENT_REVISION&o=CURRENT_COMMIT HTTP/1.1
HTTP/1.1 403 Forbidden
Response body: '<!DOCTYPE html><html lang=en><meta charset=utf-8><meta name=viewport content="initial-scale=1, minimum-scale=1, width=device-width"><title>Error 403 (Forbidden)!!1</title><style>*{margin:0;padding:0}html,code{font:15px/22px arial,sans-serif}html{background:#fff;color:#222;padding:15px}body{color:#222;text-align:unset;margin:7% auto 0;max-width:390px;min-height:180px;padding:30px 0 15px;}* > body{background:url(//www.google.com/images/errors/robot.png) 100% 5px no-repeat;padding-right:205px}p{margin:11px 0 22px;overflow:hidden}pre{white-space:pre-wrap;}ins{color:#777;text-decoration:none}a img{border:0}@media screen and (max-width:772px){body{background:none;margin-top:0;max-width:none;padding-right:0}}#logo{background:url(//www.google.com/images/branding/googlelogo/1x/googlelogo_color_150x54dp.png) no-repeat;margin-left:-5px}@media only screen and (min-resolution:192dpi){#logo{background:url(//www.google.com/images/branding/googlelogo/2x/googlelogo_color_150x54dp.png) no-repeat 0% 0%/100% 100%;-moz-border-image:url(//www.google.com/images/branding/googlelogo/2x/googlelogo_color_150x54dp.png) 0}}@media only screen and (-webkit-min-device-pixel-ratio:2){#logo{background:url(//www.google.com/images/branding/googlelogo/2x/googlelogo_color_150x54dp.png) no-repeat;-webkit-background-size:100% 100%}}#logo{display:inline-block;height:54px;width:150px}</style><div id="af-error-container"><a href=//www.google.com/><span id=logo aria-label=Google></span></a><p><b>403.</b> <ins>That\xe2\x80\x99s an error.</ins><p>You do not have access to this page.<br/><br/> <ins>That\xe2\x80\x99s all we know.</ins></div>'
X-ErrorId: 403, EFERW-295,EFERW-269,GSHT-204,AF-288,AF-119,SSMSLRF-164,SSPF-69,GFIF-28,STSFSPSF-192,STSFSPSF-192,ATF-54,CAF-129,SCZFI-172,SCMDRF-215,EF-227,GUUAF-144,GBLHCF-114,TF-46,TF-46,GCHSRF-57,STSF-151,PLRHF-268,TTLF-154,IIIHF-46,EUCDLF-111,SSSS-548
08:59:28: WARNING: conn.sock.getpeername(): ('108.177.98.82', 443)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

I looked closer, it turns out usding "deps" on patches up to 14 above the bottom returns the properly stacked list of patches.

But for some reason "deps" fails on the top 7 patches as shown above, even after the 14 bottom patches had been merged.

Here are the 7 top patches still not merged:

$ gerrit search 'branch:firmware-cr50-9308.24.B status:open'
https://chromium-review.googlesource.com/#/c/618393/ CR: 2 CQ: 0 V: 0 ec Add U2F implementation
https://chromium-review.googlesource.com/#/c/618394/ CR: 2 CQ: 0 V: 0 ec cr50: add U2F support
https://chromium-review.googlesource.com/#/c/618395/ CR: 2 CQ: 0 V: 0 ec cr50: ensure the hash sizes are correct in U2F code
https://chromium-review.googlesource.com/#/c/618396/ CR: 2 CQ: 0 V: 0 ec g: mitigate deep-sleep abortions when using USB
https://chromium-review.googlesource.com/#/c/618397/ CR: 0 CQ: 0 V: 0 ec codesigner: accept the new command line option
https://chromium-review.googlesource.com/#/c/618398/ CR: 0 CQ: 0 V: 0 ec g: do not invoke signer with sudo unless it is necessary
https://chromium-review.googlesource.com/#/c/618399/ CR: 0 CQ: 0 V: 0 ec g: stop converting hex device id values to ints

Do you want to look at it before I merge them?

Hasn't the kernel team switched entirely to branch merges at this point?

gerrit's deps command had a few random rough edges that i fixed in  issue 767284 

i think the pre-cq logic still needs review to make sure we aren't jumping on a CL stack until they've all settled down