dist-git PR lookup by head commit is unreliable

[nforro]

This code tries to find a corresponding dist-git PR by head commit SHA:

https://github.com/packit/packit-service/blob/0c14c87d1d6e5a871fac5ab3958db40af27b0880/packit_service/worker/mixin.py#L255-L269

However, there can be multiple PRs with the same head commit, for example if a project wants to avoid divergent branches and disallows direct pushes at the same time, see e.g.:

https://src.fedoraproject.org/rpms/python-trove-classifiers/pull-request/28 https://src.fedoraproject.org/rpms/python-trove-classifiers/pull-request/31 https://src.fedoraproject.org/rpms/python-trove-classifiers/pull-request/32

[lbarcziova]

We could:

• also check that PR target branch matches
• do get_pr_list only for merged PRs. This would be a step backwards, as we used to do this previously, but we were hitting race conditions as the PR can be just recently merged. To mitigate this, we could now retry with some backoff. EDIT: as @mfocko mentioned on standup, we don't know whether we need to retry as from the message from fedmsg it is not clear whether we are handling merged PR

[lbarcziova]

From arch discussion: let's try to check Pagure code and whether we could add the missing info - indicator if the push originated from merged PR

[nforro]

From arch discussion: let's try to check Pagure code and whether we could add the missing info - indicator if the push originated from merged PR

I thought about that and I believe it should be possible, but it will be more complicated than the previous changes.

[majamassarini]

Looking at the code I am not so sure that changing the PushPagureEvent is the right thing to do. Even though we react only to a merged PR, I think we can still have the same error.

My understanding of the problem is as follow:

packit creates rawhide PR gotmax creates f39 PR (from packit one and it has exactly the same commits)

Now we suppose gotmax merges his own PR as first. We could react only to the merging of packit rawhide PR but still we will load the gotmax PR (already merged) because it is the first merged and has the same sha.

I am thinking if we should instead loop over all the PRs with the same sha and if one of them has the proper configuration use it. And probably this should be just a workaround until we don't cover, with packit-service, the gotmax's use case. It should be packit-service to create the PRs with the same code, not him.

WDYT? Am I missing something?

[lbarcziova]

We could react only to the merging of packit rawhide PR but still we will load the gotmax PR (already merged) because it is the first merged and has the same sha.

If we checked also the target branch of the PR, we would not get the gotmax PR.

[nforro]

Yes, checking PR state won't help but checking its target branch should as we know the branch to which the commits are being pushed.

From arch discussion: let's try to check Pagure code and whether we could add the missing info - indicator if the push originated from merged PR

Do we still want to do this? Should we open a separate issue for that?

[majamassarini]

I discussed this with Laura and we think the following could work:

I have 3 PRs that are pointing at the same dist-git branch.

• for a push commit the dg_branch in the event will no match any target_branch in any PR and it should be safe not to react to this events
• for a merge commit the dg_branch in the event will match exactly one PR.target_branch

so it should be enough just to check dg_branch and PR.target_branch.

For now I will do just the above.

[LecrisUT]

In the current implementation, do we account for which PR the current commit corresponds to? E.g. if there are 2 PRs with the same commit open (targeting the same branch), but only one of them is merged, do we select the correct one to filter allowed_pr_authors? Currently the recent PR seems to address just the case of different PRs targeting different branches.

The spec for the PR: https://fedora-arc.readthedocs.io/en/latest/dist-git-move/messaging.html#closing-a-pull-request indicates that we can query the status of each PR (properties.merged)

Original discussion: https://matrix.to/#/!ySjfdvNvPOsVtWHNDb:fedora.im/$2KFlGJlneT9ZYuZ3b2RaWTFtVWy8QaMSs2WsaatzwQs?via=fedora.im&via=matrix.org&via=one.ems.host

[nforro]

If there are two PRs with the same head commit targeting the same branch then the problem persists. But such scenario is very unlikely, isn't it? What we could do is to sort the resulting list of PRs by merged state (merged PRs would be at the end) before returning the first item.

[LecrisUT]

If there are two PRs with the same head commit targeting the same branch then the problem persists. But such scenario is very unlikely, isn't it?

Maybe, but that is also the case where handling such nuance is most important, i.e. when there is a PR opened by packit and another PR is opened to circumvent allowed_pr_authors and build in side-tag.

[nforro]

Good point. I think the only reliable way to solve this is to include (potential) PR metadata in the push event, so there is no need for guessing on Packit side. Let us discuss this on arch meeting (we still have the discuss label here).

[majamassarini]

If there are two PRs with the same head commit targeting the same branch then the problem persists. But such scenario is very unlikely, isn't it?

Maybe, but that is also the case where handling such nuance is most important, i.e. when there is a PR opened by packit and another PR is opened to circumvent allowed_pr_authors and build in side-tag.

In this case, the PR opened by Packit couldn't be closed? If it can be closed, we shouldn't have problems to choose the right one. And probably we should explain properly in the documentation which are the steps to follow to work effectively with Packit in the scenario of building in side-tag or fast forward Packit commits.

[LecrisUT]

In this case, the PR opened by Packit couldn't be closed? If it can be closed, we shouldn't have problems to choose the right one.

As far as I understand the documentation ^1, if the PR is closed or merged, they are both treated as closed. The property properties.merged seems to be used to distinguish between closed/merged. I am not sure if the spec is consistent when you request the list of PRs as well, or if it's specific to "Closing a pull request".

[lbarcziova]

As far as I understand the documentation 1, if the PR is closed or merged, they are both treated as closed. The property properties.merged seems to be used to distinguish between closed/merged. I am not sure if the spec is consistent when you request the list of PRs as well, or if it's specific to "Closing a pull request".

This is however not relevant for Packit, as we do not react to the message about closing, but to the message about push (git.receive). The PR is then being checked via API.

[LecrisUT]

The PR is then being checked via API

But status=PRStatus.all is used instead of status=PRStatus.merged (or maybe equivalent). The open/closed PRs ^1 can still sneak in there.

[lbarcziova]

Yes, as I wrote previously, we get all the PRs because previously we were hitting this race condition mentioned in the comment when we were getting only merged PRs. It is definitely a weak point in our code now and currently the use-case of having 2 PRs with same commits opened by different users would not work, but the solution would be a little bit more complicated as Nikola wrote.

[LecrisUT]

:thinking: What about changing the listener to pagure.pull-request.closed. In the result we should already know the true value of the PR after everything is settled

[lbarcziova]

That's true, but it would lead to duplicating of build triggering, as we would receive both messages about closed PR and pushed commit :/

[lbarcziova]

Notes from arch:

• we cannot rely on getting the correct PR via API
• @nforro had a look into the Pagure code and it should not be that complicated to put the info about PR into the git.receive message

[LecrisUT]

• @nforro had a look into the Pagure code and it should not be that complicated to put the info about PR into the git.receive message

So that approach would involve updating the pagure repo right? Wouldn't it still have the same potential race condition? I thought there is a movement to mirror and migrate these to gitlab?

That's true, but it would lead to duplicating of build triggering, as we would receive both messages about closed PR and pushed commit :/

My thought on this was to replace the trigger, i.e.:

• pagure.pull-request.closed listener: checks koji_build for PR triggers with allow_pr_authors, label filters, etc.
• pagure.git.receive listener: checks koji_build for commit triggers, filtering out any commit that belongs to a PR
  • This of course means that if commit belongs to rawhide PR, but it was pushed manually to the other branches, the automation will not trigger. But at that point the packager is already doing manual operations so they can readily make additional fedpkg calls to build
  • Documenting the behavior would be important, but there are other edge-cases, e.g. if packit.yaml is merged on release branches before rawhide

[nforro]

So that approach would involve updating the pagure repo right?

Pagure source code, to be specific.

Wouldn't it still have the same potential race condition?

What race condition do you mean? There wouldn't be a need for a lookup at all, and no need for an API call to get the PR state (that could be incorrect at that time), because we would know the ID of the PR and we would know for sure it's been merged.

I thought there is a movement to mirror and migrate these to gitlab?

If/when that happens we will have to rewrite a lot more than just this :sweat_smile:

[nforro]

My thought on this was to replace the trigger, i.e.:

• pagure.pull-request.closed listener: checks koji_build for PR triggers with allow_pr_authors, label filters, etc.

• pagure.git.receive listener: checks koji_build for commit triggers, filtering out any commit that belongs to a PR

  • This of course means that if commit belongs to rawhide PR, but it was pushed manually to the other branches, the automation will not trigger. But at that point the packager is already doing manual operations so they can readily make additional fedpkg calls to build
  • Documenting the behavior would be important, but there are other edge-cases, e.g. if packit.yaml is merged on release branches before rawhide

That definitely makes sense, however it would be much more difficult to implement.

[lbarcziova]

pagure.git.receive listener: checks koji_build for commit triggers, filtering out any commit that belongs to a PR

but without changes to the pagure source code, this is still not reliably doable at the moment

[LecrisUT]

pagure.git.receive listener: checks koji_build for commit triggers, filtering out any commit that belongs to a PR

but without changes to the pagure source code, this is still not reliably doable at the moment

Well the main issue was that from the API we couldn't get the correct state of the PR (merged/closed/open), but we are still able to determine that a commit belongs to a PR or not. Unless someone is incredibly fast at opening and merging PRs.

What race condition do you mean? There wouldn't be a need for a lookup at all, and no need for an API call to get the PR state (that could be incorrect at that time), because we would know the ID of the PR and we would know for sure it's been merged.

Hmm, indeed. Would depend on the backend implementation. It seems you would want to separate the sending of pagure.git.receive from the PR merge side and git push side? Or would it still be triggered by any push to the branch, but you would internally look for the PR that matches it? I think this latter part can still be prone to race conditions when the pagure worker for the git commit is faster than the worker for the PR if these 2 are async, but of course it all depends on the implementation.

Maybe there is some github-like logic, where pushing the merged/rebased commits to target branch would automatically mark the PR as merged, in which case the status of the PR can be ignored by pagure.git.receive, unless it is marked as closed.

[nforro]

Or would it still be triggered by any push to the branch, but you would internally look for the PR that matches it?

It shouldn't be necessary to look for anything: https://pagure.io/pagure/blob/c15e7251f70b2280e272698183c10554da02d3aa/f/pagure/hooks/init.py#_311

[nforro]

Pagure PR: https://pagure.io/pagure/pull-request/5452