Test-infra: prow should recommend relevant approvers

[BenTheElder]

What happened: Prow suggested a root approver for review when the PR had a single file covered by leaf approvers that did not include the root approver.

The PR only touches this file config/testgrids/conformance/conformance-all.yaml

With config/testgrids/conformance/OWNERS existing with the following contents:

# See the OWNERS docs at https://go.k8s.io/owners

approvers:
- spiffxp
- timothysc

The raw comment posted by Prow is:

[APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes/test-infra/pull/16292#" title="Author self-approved">lubinsz</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **bentheelder**
You can assign the PR to them by writing `/assign @bentheelder` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubernetes%2Ftest-infra).

<details open>
Needs approval from an approver in each of these files:

- **[config/testgrids/conformance/OWNERS](https://github.com/kubernetes/test-infra/blob/master/config/testgrids/conformance/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["bentheelder"]} -->

https://github.com/kubernetes/test-infra/pull/16292#issuecomment-593761410

There's no reason I should be suggested here.

What you expected to happen:

Since config/testgrids/conformance/OWNERS has existing approvers, is the only directory in need of approval, and the comment is about getting approval, the approvers from this file should be suggested. I am not one of them, so I should not be suggested.

How to reproduce it (as minimally and precisely as possible):

Wait for prow to make suggestions for approval on a PR.

Please provide links to example occurrences, if any:

https://github.com/kubernetes/test-infra/pull/16292#issuecomment-593761410

Anything else we need to know?:

This behavior leads to excessive assignment of root approvers, we should be spreading workload to leaf owners.

cc @cjwagner
/area prow

[BenTheElder]

another instance of this at https://github.com/kubernetes/kubernetes/pull/87080, we have a specific sig-network-approvers alias for these tests, but it's suggesting me as a root approver of test/ even though that's unnecessary.

[BenTheElder]

https://github.com/kubernetes/test-infra/pull/16972 should at least mitigate this in test-infra

[BenTheElder]

https://github.com/kubernetes/test-infra/pull/16961 is another example of having granular owners fully covering the PR but auto assigning a root approver for review.

[justaugustus]

Huge +1!

[justaugustus]

/help

[k8s-ci-robot]

@justaugustus:
This request has been marked as needing help from a contributor.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

/help

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[alvaroaleman]

yeah so ironically we changed a tool of ours that automatically creates owners files for jobs based on the owners file in the target repo to copy over the approvers to the reviewers if the latter is empty for this exact same reason.

Maybe we should just do the copy-approvers-to-reviewers-if-empty(or < min reviewers? or always append approvers to reviewers?) in blunderbuss instead?

[matthyx]

Is this still open for contributors? @alvaroaleman which strategy do you recommend?

[alvaroaleman]

I think I would probably just use approvers as reviewers if there are no reviewers in blunderbuss

[matthyx]

ok
/assign

[matthyx]

@BenTheElder if you want less reviews, just rename your account to zenTheElder...

https://github.com/kubernetes/test-infra/blob/master/prow/plugins/blunderbuss/blunderbuss.go#L228 makes a sorted list of all reviewers, leafReviewers, approvers and leafApprovers - and capitalization wins (also sad news for @alvaroaleman).

I think we need to come with a better strategy than https://github.com/kubernetes/test-infra/blob/master/prow/plugins/blunderbuss/blunderbuss.go#L238 where we truncate an alphabetically sorted list...

[matthyx]

@BenTheElder @spiffxp @mrbobbytables I think I need you help on that, because such a bug means an unfair effort split between reviewers and approvers...
It is very important to make this right and communicate it through contribex to show how we care about the community.
@LappleApple maybe we could get some data about how alphabetical order influences review/approval requests?
Thanks all for your help, this is about fairness.

[spiffxp]

I have no problems with changing the List() to UnsortedList()

FWIW those lists are populated by random selection https://github.com/kubernetes/test-infra/blob/1fcf814d0181ffa94f89398ce4de06f71487d982/prow/plugins/blunderbuss/blunderbuss.go#L322

---

That said, note this isn't (directly?) related to blunderbuss's requested review selection. This is about who the approve plugin recommends for assignment in this comment: https://github.com/kubernetes/test-infra/blob/1fcf814d0181ffa94f89398ce4de06f71487d982/prow/plugins/approve/approvers/owners.go#L612

which calls out to this

https://github.com/kubernetes/test-infra/blob/1fcf814d0181ffa94f89398ce4de06f71487d982/prow/plugins/approve/approvers/owners.go#L455-L484

Ben's pointing out cases where we would really expect someone from the leaf OWNERS file to be chosen, instead of bothering to walk up to parent OWNERS files

[matthyx]

@spiffxp actually the cause of the bug might be similar... people seem to make assumptions about sets keeping the insertion order.
I wonder if I should implement a kind of https://godoc.org/k8s.io/apimachinery/pkg/util/sets that keeps that order?
Also I think a proper code audit would be necessary to check whether we have this issue elsewhere...

[matthyx]

Something like https://github.com/elliotchance/orderedmap:

Internally an *OrderedMap uses a combination of a map and linked list.

[matthyx]

@alvaroaleman can I have your opinion?

[alvaroaleman]

I think this is not because of the map but I might be wrong. I spend 30 minutes with this code but could not quickly find an explanation. Basically we:

• Try to find leaf reviewers
• If not enough, traverse up
• this doesn't yield enough results, do the same but with approvers

This doesn't really explain what Ben saw though, because there were two leaf approvers that should have been chosen before traversing up.

At this point, I would just start off by writing a testcase that reproduces the observed behavior and go from there rather than directly changing production code.

[matthyx]

Ben's pointing out cases where we would really expect someone from the leaf OWNERS file to be chosen, instead of bothering to walk up to parent OWNERS files

I have checked the code in owners.go and it seems to work correctly when playing around with the tests in approvers_test.go.

I don't think any more work besides getReviewers is needed here.

[alvaroaleman]

Sure, but please write a testcase that reproduces the issue

[matthyx]

This one?
https://github.com/kubernetes/test-infra/pull/17676/files#diff-9405520c500fc4bbd8485ac33117d546R486

Or you mean a test case in approvers_test.go?

[matthyx]

@spiffxp @alvaroaleman just to be sure:
https://github.com/kubernetes/test-infra/blob/1fcf814d0181ffa94f89398ce4de06f71487d982/prow/plugins/approve/approvers/owners.go#L612
{{range $index, $cc := .ap.GetCCs}}{{if $index}} {{end}}@{{$cc}}{{end}} means take the first one of that list?

[spiffxp]

I may be wrong, but that reads like it will output the entirety of the list returned by GetCCs, adding a space delimiter for entries after the first. I can't recall seeing more than one person suggested, so maybe GetCCs is only ever returning a single item list?

[BenTheElder]

This also seems to apply to reviewer assignment
https://github.com/kubernetes/test-infra/pull/18519
There are reviewers in the directories above these files other than the root. I am not in any of them.

I can't explain why prow is ignoring OWNERS files and going for the root.

[matthyx]

Thanks, I will have a look!

[matthyx]

This also seems to apply to reviewer assignment

18519

There are reviewers in the directories above these files other than the root. I am not in any of them.

I can't explain why prow is ignoring OWNERS files and going for the root.

I think this is another case of alphabetical sorting woe... I will take some time to run this exact code in the debugger on the actual test-infra repo and understand this particular case.

Have you seen inconsistencies on the suggested approvers comment as well (obviously not this one as Erick didn't need one).

[BenTheElder]

Even with alphabetical sorting the files were 100% covered by a deeper directory with reviewers than the root. We should be favoring leafier for both. This is especially important in a repo like kubernetes/kubernetes

[matthyx]

Hi @BenTheElder sorry for the delay... what's the version of blunderbuss running at the moment?
I have run the current code in master, against the repo as it was when Erik's PR was created and it resulted in:
time="2020-08-07T09:02:54+02:00" level=info msg="Requesting reviews from users [stevekuznetsov chases2]." plugin=blunderbuss

I suspect the running version of blunderbuss is somewhat old and doesn't have the recent improvements in reviewers election...

[BenTheElder]

prow.k8s.io should be updated in lockstep across components, and appears to
be at d23520775f
it is updated approximately once a day on weekdays.

On Fri, Aug 7, 2020 at 12:08 AM Matthias Bertschy [e-mail]
wrote:

Hi @BenTheElder https://github.com/BenTheElder sorry for the delay...
what's the version of blunderbuss running at the moment?
I have run the current code in master, against the repo as it was when
Erik's PR was created and it resulted in:
time="2020-08-07T09:02:54+02:00" level=info msg="Requesting reviews from
users [stevekuznetsov chases2]." plugin=blunderbuss

I suspect the running version of blunderbuss is somewhat old and doesn't
have the recent improvements in reviewers election...

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/kubernetes/test-infra/issues/16793#issuecomment-670365871,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AAHADK6O7OQKVI3NDDMGFRLR7OR57ANCNFSM4LLKR3QQ
.

[matthyx]

It doesn't make any sense comparing with what I am seeing...
Do we still have the logs?

[BenTheElder]

So also skimming it now I see ...

https://github.com/kubernetes/test-infra/blob/9fb86269079f93be4fc612e5fc9855f272ee1359/prow/plugins/approve/approvers/owners.go#L187

Seed:
https://github.com/kubernetes/test-infra/blob/9fb86269079f93be4fc612e5fc9855f272ee1359/prow/plugins/approve/approve.go#L429

Those seeds are hardly using many bits / having much variety ... and PRNGs are normally used repeatedly, not once and discarded.

I don't think that's related to the logic for selecting approvers simply picking from all possible approvers instead of the most specific possible, but I'm not terribly confident in the random selection either.

[matthyx]

Yeah, "random" is a great word...
But still, I don't understand why I get something different as reviewers. Back to the drawing board I guess!

[matthyx]

@BenTheElder how is it now, do you still have strange assignments with the layeredsets?

[matthyx]

Let's be optimistic and close that one then
/close

[k8s-ci-robot]

@matthyx: Closing this issue.

In response to this:

Let's be optimistic and close that one then
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.