[LU-3690] Improve gerrit reporting of patch status

• Status: Resolved
• Project: Lustre
• Type: Task
• Priority: Minor
• Reporter: Christopher Morrone
• Assignee: Peter Jones
• Resolution: Not a Bug

Description

We use gerrit differently than it expects by default. Most importantly, there are two conditions that signal that a patch is done code review and can be sent to the Gate Keeper for landing:

• A +2 CodeReview
  OR
• 2 or more +1 CodeReviews

Unfortunately, there is currently no way to create a reliable positive or negative pattern to match that second condition. That makes it inpossible to create good filters that provide the following useful lists of patches:

• Patches that are fully reviewed and waiting for landing
• Patches that are awaiting further review

On the CDWG mailing list Denis Kondratenko provided a couple of great links that talk about possible solutions for this very problem:

• http://en.wikibooks.org/wiki/Git/Gerrit_Code_Review (A hook could - for instance - allow a project to install an automated gatekeeper to vote +2 'submit approved' when sufficient +1 votes 'looks good to me' have been received)
• http://review.whamcloud.com/Documentation/prolog-cookbook.html#_example_12_1_1_2_code_review (but I don't know if it will get right searches..)

Comments

[Peter Jones]

Thanks Chris.

[Denis Kondratenko (Inactive)]

Actually building prolog rule will not give us possibility to search for the submittable reviews. It will just allow user with submittable right to submit without review.

I have checked another way to build correct search for issues or check their stages automatically - REST API. It is supported now by Gerrit 2.5.4, but provides very few data to analyze (and couldn't be used for searches).

With Gerrit 2.6 REST API was improved a lot and there even "submittable" field there, so with that we could build scripts that will provide right searches for us.
More than that 2.6 provides huge number of other details, that possibly could be used to detect different stages of review to build scripts to gather stat - where we spend most of the time and where to improve. And maybe better tool to get some notification for people that are responsible for review now (ping developers that ball on their side for example).

http://gerrit-documentation.googlecode.com/svn/ReleaseNotes/ReleaseNotes-2.6.html#_rest_api

So maybe just updating to 2.6 will give us API that could resolve a lot of our issues for now.

[Denis Kondratenko (Inactive)]

Chris, Peter,

there was sbj in our discussion about how simple script for gerrit could looks like and what it could do.

In attachment there is example. It has some minor issues, but could give you overall idea.

BTW, it would be cool to have Intel servers support CORS . I don't really understand where it should be supported (by apache or gerrit), but I think IT guys could know. That feature could do scripts like in attachment to be interactive instead of static.

[Andreas Dilger]

One possibility that I think would be easy to implement (and also avoids the need to upgrade) is to only have Hudson/Jenkins mark the patch Verified -1 if it fails to build, but NOT add a Verified +1 if the build succeeds.

That leaves +1 open for Maloo test results and ensures that Gerrit queries for Verified = +1 only return patches that have actually passed testing instead of patches that have merely been built.

[John Hammond]

This could also be done (inelegantly) via a script that leverages Gerrit's ssh API. Using say:

ssh -p 29418 review.whamcloud.com gerrit query --all-approvals --format=JSON status:open limit:100

Or human readably:

ssh -p 29418 review.whamcloud.com gerrit query --all-approvals status:open limit:100
...
change I6e2837953902520240e9ceb251c92329d328d715
  project: fs/lustre-release
  branch: master
  id: I6e2837953902520240e9ceb251c92329d328d715
  number: 7803
  subject: LU-1345 cleanup: C89 and build cleanups
  owner:
    name: Alexey Lyashkov
    email: [e-mail]
    username: shadow
  url: http://review.whamcloud.com/7803
  createdOn: 2013-09-30 10:48:59 UTC
  lastUpdated: 2013-10-17 22:39:35 UTC
  sortKey: 00287e8f00001e7b
  open: true
  status: NEW
  patchSets:
    number: 1
    revision: 3e4dd4324536fbeaba06049a80ac131cd1fe2cd2
    parents:
 [92b43047fdfd1a61e200726ec93150543a75d2ff]
    ref: refs/changes/03/7803/1
    uploader:
      name: Alexey Lyashkov
      email: [e-mail]
      username: shadow
    createdOn: 2013-09-30 10:48:59 UTC
    approvals:
      type: VRIF
      description: Verified
      value: -1
      grantedOn: 2013-09-30 11:45:13 UTC
      by:
        name: Hudson
        username: hudson
  patchSets:
    number: 2
    revision: bc0ef1e16472cdae57fe68236e90aef66730b873
    parents:
 [63e8a4f33dc85d2957a035fc7d4b040de35c08ce]
    ref: refs/changes/03/7803/2
    uploader:
      name: Alexey Lyashkov
      email: [e-mail]
      username: shadow
    createdOn: 2013-10-01 08:02:17 UTC
    approvals:
      type: VRIF
      description: Verified
      value: 1
      grantedOn: 2013-10-02 05:45:32 UTC
      by:
        name: Hudson
        username: hudson
    approvals:
      type: CRVW
      description: Code Review
      value: -1
      grantedOn: 2013-10-04 17:16:19 UTC
      by:
        name: James Simmons
        email: [e-mail]
        username: jsimmons
    approvals:
      type: VRIF
      description: Verified
      value: -1
      grantedOn: 2013-10-03 06:48:00 UTC
      by:
        name: Maloo
        email: [e-mail]
        username: maloo
    approvals:
      type: CRVW
      description: Code Review
      value: -1
      grantedOn: 2013-10-03 14:27:13 UTC
      by:
        name: Dmitry Eremin
        email: [e-mail]
        username: dmiter
  patchSets:
    number: 3
    revision: 5122ed9a2f38d462266152fa818c195501d7c199
    parents:
 [780c49a765e6de0caeea5af7dce864f855570789]
    ref: refs/changes/03/7803/3
    uploader:
      name: Alexey Lyashkov
      email: [e-mail]
      username: shadow
    createdOn: 2013-10-16 08:17:11 UTC
    approvals:
      type: VRIF
      description: Verified
      value: 1
      grantedOn: 2013-10-16 09:03:40 UTC
      by:
        name: Hudson
        username: hudson
    approvals:
      type: CRVW
      description: Code Review
      value: -1
      grantedOn: 2013-10-16 14:29:42 UTC
      by:
        name: James Simmons
        email: [e-mail]
        username: jsimmons
    approvals:
      type: VRIF
      description: Verified
      value: -1
      grantedOn: 2013-10-17 04:08:38 UTC
      by:
        name: Maloo
        email: [e-mail]
        username: maloo
...

See http://review.whamcloud.com/Documentation/cmd-query.html for some details.

[Christopher Morrone]

.bq One possibility that I think would be easy to implement (and also avoids the need to upgrade) is to only have Hudson/Jenkins mark the patch Verified -1 if it fails to build, but NOT add a Verified +1 if the build succeeds.

What problem are you trying to solve? Right now Maloo marks +1 and Jenkins marks +2, so those are already distinct and searchable states in gerrit. The discussion in this ticket has been more focused on the Code-Review column, not the Verified column.

If we mess with the "Verified" column at all, I think what I would want to do is eliminate it altogether. Gerrit does not contrain us to just the two columns in the default configuration. We can configure it however we want to match our workflow. And I think that is the root of our problems so far: we try to shoehorn our workflow into gerrit's out-of-the-box defaults rather than changing gerrit's configuration to work with our workflow.

Case in point, +1 and +2 in Code-Review column in the gerrit defaults have different meanings than +1 and +2 in Lustre's usage. But we haven't taught gerrit our rules, so there is no way to create searches against the Code-Review column to gather the information that we really want.

[Andreas Dilger]

John has implemented some reports by interfacing with Gerrit directly:

https://wiki.hpdd.intel.com/display/Releases/2.6+Patch+Status+Report
https://wiki.hpdd.intel.com/display/~jhammond/Patch+Status

Chris, does this meet your requirements for improved reporting of pending patches?

[James A Simmons]

The first link didn't work for me but the second did. Very nice looking.

[John Hammond]

There is also https://wiki.hpdd.intel.com/display/~jhammond/Patch+Status+2 which shows the same changes but in a single table.

[Christopher Morrone]

Andreas, as James pointed out we can't see the Releases/2.6+Patch+Status+Report page.

I do like what I see at ~jhammond/Patch+Status and
~jhammond/Patch+Status+2!

But there might be some bugs to work out...or perhaps the Patch+Status page hasn't been updated in a while? For instance, change 1720/3 is listed under "Needs Verify or Review", but it looks fully verified and reviewed in gerrit. From the gerrit logs, it seems to have been that way since April, so my suspicion is a kink in the Patch+Status script that needs to be worked out.

But even without that I am getting useful insight. It looks pretty clear to me that we have an autotest issue. Quite a few tests have been waiting well over a month for autotest to be run on them. It is nice to have that easy visibility.

[Christopher Morrone]

I think the scanning script may assume that "Test-Parameters:" can only be the first field at the end, or something like that. It is missing fortestonly patches like this one:

http://review.whamcloud.com/#/c/9167/

Are we going to get to see the scripts that generate this stuff?

[John Hammond]

1720/3 was under needs verify or review since we do not count positive reviews by the author or the committer.

The commit message for http://review.whamcloud.com/#/c/9167/ uses 'Test-Requirements' instead of 'Test-Parameters'.

[Christopher Morrone]

Ah, right, fair enough. I gave it a -1 to solve that.

[Christopher Morrone]

I like the pages alot. This task is complete as far as I am concerned.

[Peter Jones]

Great - thanks Chris!

[Cory Spitz]

What are the 'official' URLs for the pages or will they stay at ~jhammond? Once we confirm the publishable URLs, I'd like to add them to the OpenSFS LWG wiki.

[John Hammond]

Hi Cory, we have decided that the ~jhammond URLs are the official ones.