Gitlab MR issue: pre-receive hook failed. Try again.

According to the MR: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/44843 There maybe a gitaly logs related to that.

assigned to @droy

@droy can you assist here?

This seems normal when there`s a merge conflict in the branch. A rebase/merge may be required.

https://docs.gitlab.com/ee/user/project/merge_requests/fast_forward_merge.html

@droy That doesn't seem to be the case. We've checked that already on local clones. Also, we have different MR with the same error: eclipse/oniro-core/oniro!1 (closed) And we really need those logs to understand what's happening.

@pastanki can you try this operation again, and note the exact date/time ? Just so we have a pinpoint. @fgurr can look through the logs tomorrow morning (CET)

@fgurr

Wed Mar 30 06:51:50 CEST 2022

I'm still looking through the logs

I tried again at Wed Mar 30 16:52:02 CEST 2022

From our git-eca-rest-api logs (timestamps are UTC):

2022-03-30 04:51:55,938 WARN  [...] No users found for mail 'nawab.ahmad.reshi@huawei.com'
2022-03-30 04:51:55,938 ERROR [...] Author must have an Eclipse Account

I assume this was caused by the following commit from the bugs_template branch:

landgraf/oniro@e4f54367

@fgurr Is this the ECA git hook? If yes, does it check the entire history? Shouldn't it only check the commit delta brought by the MR?

I tried again at Wed Mar 30 16:52:02 CEST 2022

2022-03-30 04:51:55,938 ERROR [...] Author must have an Eclipse Account

Hmmm...

@malowe Would it be possible for you to look at this? The question above is interesting.

Yes this is the ECA git hook.

I'll let my colleagues @cguindon or @malowe comment whether the history is checked or not.

Another thing we can do: we can schedule an exact time, disable the ECA hook, perform your merge and see. It would at least unblock you and confirm it's the ECA.

Right now the history checks the MR against the designated "main" branch of a repo to attempt to only check commits that are new to the trunk of the repository. It's not ideal, but we don't really have a lot of information passed to the hook. We only have old head commit, new head commit, and what repo it is. We don't necessarily know what branch it's being committed to from what I remember when I built the solution.

@droy that won't be scalable as we have a set of MRs coming (or in transit already) @malowe But this is a bit confusing now because this same hook succeeds on merges to another branch (which also has a history for the same email reported in the logs - nawab.ahmad.reshi@huawei.com). And all the MRs against that branch are working as expected. Here is an example: eclipse/oniro-core/oniro!34 (merged). So it seems that the ECA actually checks the additional commits (not the entire history in this case). Why would the hook behave differently (fail) on the dunfell branch?

@fgurr This commit is not part of the MR. It's in the repo already: landgraf/oniro@e4f54367

@landgraf @agherzan No it is not.

It is in @landgraf 's private fork. The ECA checker does not apply to private forks. But everything that is merged back to the canonical repo must go through the ECA checker. Nawab is NOT a committer, therefore needs an EF account and needs to sign the ECA, for their code to be merged into the canonical repo.

AFAICT, the ECA check in on-side and correct.

@droy I'm not sure I follow. Why would it care about the fork's history in this case?

The ECA check is only applied to the canonical repo. In this case, you're merging a private fork into 'dunfell' which is in the canonical repo. The ECA check cares about everything that goes into the canon repo, regardless of where it comes from.

As mentioned, an earlier commit from the same person to the 'dunfell' branch is being reviewed, as it should have failed as well.

In the end, each person who intends on contributing code to your project must have an EF account and sign the ECA.

@droy Nawab's commit is in the repo already: eclipse/oniro-core/oniro@e4f54367 My private fork adds only one single commit eclipse/oniro-core/oniro!14 (merged) (commits)

We are investigating how it was possible that Nawab's commit ever made it into the dunfell branch, while it should have failed the ECA check.

Crap, @agherzan I mistakenly edited your comment above, instead of replying to it. Apologies. I think I lost a sentence or two.

My response is: The ECA check is only applied to the canonical repo. In this case, you're merging a private fork into 'dunfell' which is in the canonical repo. The ECA check cares about everything that goes into the canon repo, regardless of where it comes from.

As mentioned, an earlier commit from the same person to the 'dunfell' branch is being reviewed, as it should have failed as well.

In the end, each person who intends on contributing code to your project must have an EF account and sign the ECA.

After much investigation, we cannot explain how nawab...@huawei.com 's commit passed into dunfell. Between the possibility of an ECA glitch, temporary removal of the ECA for some reason, temporary admin access to others, GitLab's allowing of secondary email addresses, we don't know. I'll open a feature request against our ECA to have a better audit log.

Nawab is a committer using @nawab and nawab...@huawei-partners.com email address. That is the only email address we recognize formally (other than secondary email addresses in GitLab). We strongly recommend that person uses that email address for commits to the Oniro project.

In this case, the ECA is correct in that nawab...@huawei.com violates the ECA check. The commit(s) should be amended to use the correct email: nawab...@huawei-partners.com

@droy Let me try to clarify the situation we are seeing here.

The https://gitlab.eclipse.org/eclipse/oniro-core/oniro repo has a couple of branches that were pushed via the initial code push for repository initialisation. And that is the explanation of how that commit ended up in history. And that commit is not the only one without a valid ECA - the history has a series of individuals that are not with the project anymore and their ECA validation would fail. See here for how we handled this in the past eclipse/oniro-core/oniro!2 (merged).

Now, we have two main branches in this repository: dunfell and kirkstone. Both branches used to work just fine. And by that, I mean that MRs were passing the ECA merge hook resulting in a successful merge. See for example eclipse/oniro-core/oniro!2 (merged) on dunfell and many other merges on kirkstone (https://gitlab.eclipse.org/eclipse/oniro-core/oniro/-/merge_requests?scope=all&state=merged&target_branch=kirkstone). A couple of weeks ago (some days before opening this issue) the behaviour changed. All MRs on kirkstone continue to pass the ECA and merges succeed while the MRs on dunfell started to fail consistently. All these MRs use the same "fork and MR" development flow - so nothing changed in that regard.

The error you guys found in the logs seems to be inconsistent with what I explained above. The same author has merge commits in both of these branches:

dunfell:

commit e4f543672a2845520f5e978f26880eb559f0941e
Author: NawabAhmad <nawab.ahmad.reshi@huawei.com>
Date:   Fri Feb 4 21:22:15 2022 +0530
    Add introductory content for meta-openharmony
    I added the content as per review on the MR.
    Signed-off-by: NawabAhmad <nawab.ahmad.reshi@huawei.com>

kirkstone:

commit 592529bdcc0c2251dec5dd0fec961801db643838
Author: NawabAhmad <nawab.ahmad.reshi@huawei.com>
Date:   Sun Feb 6 21:46:07 2022 +0530
    Add meta-openharmony content to the existing RTD docs
    Signed-off-by: NawabAhmad <nawab.ahmad.reshi@huawei.com>

We can verify that these commits are indeed in different branches:

$ git branch -a --contains e4f543672a2845520f5e978f26880eb559f0941e
  remotes/eclipse/dunfell
$ git branch -a --contains 592529bdcc0c2251dec5dd0fec961801db643838
  remotes/eclipse/kirkstone

Now, if the ECA would check the entire history, pushing a copy of these branches should fail on the same author but here is the thing, it succeeds on kirkstone:

$ git push eclipse-ssh eclipse-ssh/kirkstone:refs/heads/test -f
Total 0 (delta 0), reused 0 (delta 0)
remote: Commit: _nil            ~
remote: Warnings:
remote:         A commit is required to validate
remote: Any warnings or errors noted above may indicate compliance issues with committer ECA requirements. More information may be found on https://www.eclipse.org/legal/ECA.php
remote: To create a merge request for test, visit:
remote:   https://gitlab.eclipse.org/eclipse/oniro-core/oniro/-/merge_requests/new?merge_request%5Bsource_branch%5D=test
remote:
To gitlab.eclipse.org:eclipse/oniro-core/oniro.git
 * [new branch]      eclipse-ssh/kirkstone -> test

Mind the warning and the strange _nil output. But the ECA passes. Now, doing the same on dunfell, it fails as expected:

andrei@phase10:~/work/oniroproject/ws/oniro$ git push eclipse-ssh eclipse-ssh/dunfell:refs/heads/test -f
Total 0 (delta 0), reused 0 (delta 0)
remote: Commit: d94e84811d2b285123f4d54a4cbded0245237c43                X
remote:         Reviewing commit: d94e84811d2b285123f4d54a4cbded0245237c43
remote:         Authored by: Gururaj Shetty <gururaj.shetty@huawei.com>
remote:         Eclipse user 'shettygururaj'(author) is not a committer on the project.
remote:         Eclipse user 'shettygururaj'(author) does not have a current Eclipse Contributor Agreement (ECA) on file.
remote: If there are multiple commits, please ensure that each author has a ECA.
remote:         Eclipse user 'shettygururaj'(committer) is not a committer on the project.
remote:         Eclipse user 'shettygururaj'(committer) does not have a current Eclipse Contributor Agreement (ECA) on file.
remote: If there are multiple commits, please ensure that each author has a ECA.
remote: Errors:
remote: GL-HOOK-ERR: An Eclipse Contributor Agreement is required for Eclipse user 'shettygururaj'(author).
remote: GL-HOOK-ERR: An Eclipse Contributor Agreement is required for Eclipse user 'shettygururaj'(committer).
remote: Any warnings or errors noted above may indicate compliance issues with committer ECA requirements. More information may be found on https://www.eclipse.org/legal/ECA.php
remote: Commit: 095a766951934b4c4b9a51432719976d374bb9bf                ✔
remote:
remote:         Reviewing commit: 095a766951934b4c4b9a51432719976d374bb9bf
remote:         Authored by: Pavel Zhukov <pavel.zhukov@huawei.com>
remote:         Eclipse user 'landgraf'(author) is a committer on the project.
remote:         Eclipse user 'landgraf'(committer) is a committer on the project.
remote: Commit: bb465c2dc3f528d34617d3149d30e2fb595deb0b                X
remote:         Reviewing commit: bb465c2dc3f528d34617d3149d30e2fb595deb0b
remote:         Authored by: Alberto Pianon <pianon@array.eu>
remote:         Could not find an Eclipse user with mail 'pianon@array.eu' for author of commit bb465c2dc3f528d34617d3149d30e2fb595deb0b
remote: Errors:
remote: GL-HOOK-ERR: Author must have an Eclipse Account
remote: Any warnings or errors noted above may indicate compliance issues with committer ECA requirements. More information may be found on https://www.eclipse.org/legal/ECA.php
remote: Commit: e4f543672a2845520f5e978f26880eb559f0941e                X
remote:         Reviewing commit: e4f543672a2845520f5e978f26880eb559f0941e
remote:         Authored by: NawabAhmad <nawab.ahmad.reshi@huawei.com>
remote:         Could not find an Eclipse user with mail 'nawab.ahmad.reshi@huawei.com' for author of commit e4f543672a2845520f5e978f26880eb559f0941e
remote: Errors:
remote: GL-HOOK-ERR: Author must have an Eclipse Account
remote: Any warnings or errors noted above may indicate compliance issues with committer ECA requirements. More information may be found on https://www.eclipse.org/legal/ECA.php
To gitlab.eclipse.org:eclipse/oniro-core/oniro.git
 ! [remote rejected] eclipse-ssh/dunfell -> test (pre-receive hook declined)

As you can see, on dunfell, that exact commit, e4f543672a2845520f5e978f26880eb559f0941e, is one of the offending entries.

This means that if the ECA checks the entire branch with every merge, it means that merge requests on both of these branches should fail. This is not the case (again, dunfell merges fail while kirkstone merges succeed and this is consistent with the small test I've shown above with creating/pushing a branch copy).

Another interesting aspect is that from the logs above the check against the dunfell branch only reports that the following commits can't be validated: d94e84811d2b285123f4d54a4cbded0245237c43 bb465c2dc3f528d34617d3149d30e2fb595deb0b e4f543672a2845520f5e978f26880eb559f0941e The history of the same branch has a way larger history with commits that also don't have an ECA check (you can take for example any commit from Zbigniew Bodek).

With all the above said, the only explanation that I can come up with is that the ECA check doesn't validate the additional commits brought in by a merge nor the entire history of the destination branch (merged branch). It actually does something in the middle (probably the additional commits and a couple of more from the history of the destination branch). In any case, there is no way for us to get over this without rewriting the history which is something undesirable for obvious reasons or disabling the ECA check (which is also undesirable as it will create a bottleneck and defeat the purpose of the check in the first place). What we expect is that the ECA check is to be enforced only for the incoming changes ignoring any existing, merged entries in the git history. This situation now blocks us from pushing any change to the dunfell branch even though all the changes we want to push pass ECA - this fail is on the existing history, not on the incoming changes.

We have managed to figure out the mystery of inconsistent behaviour on the dunfell branch. Nawab, the author of the offending commit, has gotten his email changed internally. The domain changed from @huawei.com to @huawei-partners.com. Once he reflected that in his EF account, the ECA hook started to fail. This is just an additional data point that raises the question of what would be a way to handle such situations.

A change of the email address was one of our working theories. According to our records Nawab registered an EF account on 2022-01-05 with the @huawei-partners.com address and never changed it though.

In that case (maybe Nawab forgot the timeline of events) the way we pushed the initial code was the way this commit got it without the check. Nevertheless, the important part of it now is how to deal with this both in the future and now to unblock development.

The https://gitlab.eclipse.org/eclipse/oniro-core/oniro repo has a couple of branches that were pushed via the initial code push for repository initialisation. And that is the explanation of how that commit ended up in history. And that commit is not the only one without a valid ECA - the history has a series of individuals that are not with the project anymore and their ECA validation would fail.

You are right that commits were pushed while the ECA check was turned off (see #895 (comment 542591)) on Feb 15, 2022.

According to the activity log you pushed Nawab's commit eclipse/oniro-core/oniro@e4f54367 to branch dunfell on March 5, 2022 and this should have failed, since the commit used Nawab's old @huawei.com email address which does not map to his Eclipse.org account.

We don't have an explanation for that (yet).

On March 5, 2022 you also pushed new branch kirkstone which should have failed the ECA check as well (if all commits in the history were checked). If the ECA check only applies to the last x (e.g. 10) commits, it would actually make sense, since kirkstone had ~40 commits after Nawab's commit.

We will keep digging.

Thanks for the update and confirmation that we are on the same page. Hopefully, we will find a solution to this sooner rather than later.

Hi everyone. Do we have any progress on this? Apologies for the push but this is becoming a major blocker for development and progression on our dunfell-based release. Is there anything we can do to assist you in this? @droy @fgurr

We are looking into the issues with the ECA check. @malowe can you give a rough time estimate?

We took some time to discuss this and figure out what the best way forward is. So in the short term to unblock, we are going to patch in a check that replaces the old email with the new email. This should be in place until our new solution is live in the next few months. Once the new solution is up, we will be able to more easily tweak these checks and fix edge cases.

I'm hoping to have this fix live by EOD Eastern time. Please keep in mind this is a very specific fix and will not work for other users with changed/updated emails.

Ok, @malowe. Let's see if that unblocks us at least. Do we know now what commits the checker takes into consideration when handling a push? As I said it doesn't seem to be only the new commits nor the entire history. Have we figured out what it does?

The current logic for this is hosted in a public project on this Gitlab if you'd like to take a look, but I can sum it up. It looks in reference to the default branch and grabs all new commits in relation to the new head commit. This isn't a perfect solution and is one of the big reasons we are planning on a dedicated dev period. We've learned a lot and can definitely make this better and more transparent with what we know now.

That is much clearer now. Any idea what was the reason to do the loop against the default branch as opposed to the old head (previous_head_commit)?

I was the person who wrote this so I'm the right person to ask haha! 2 factors went into this. The first is that I wanted to reduce checks against commits that were already validated in the default branch. In cases where you merge in changes from external branches, you have a ton of commits to validate that have already been validated. This was an attempt to reduce that. The second was that I was still learning about direct interaction with git for automated management, as well as building out our common infra to support more advanced features. So with the extra knowledge I have now I could likely do this much better than the initial implementation that hasn't had concentrated effort since 2020

Just as an update, the simple fix has been tested locally, and I've let the team know we're ready to roll to staging and then production for this. It's a simple dumb fix for now until we start working on the API as a whole and fully address this.

@droy When can we expect better audit log ? Will they be visible in MR ? Like mentioned here: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/44843 ?

We are planning to work on #1017 (closed) in Q2.

I've created #1099 (closed) requesting an audit log/trail.

Closing this issue. Please let us know if you need assistance rewriting the offending commit.

I don't think we are clear on either what the issue is or how we can proceed so please keep the issue open. See above for a detailed explanation that I wrote.

mentioned in merge request eclipse/oniro-core/oniro!14 (merged)

@agherzan @pastanki @landgraf we've pushed the fix for this commit up, so you should be able to commit now. This is a bandage fix, but should do well enough for now. We plan on revisiting some of this functionality later this quarter, so we'll have a more permanent fix soon-ish. Please let us know if this resolves your issue.

Thanks @malowe. I'll test this extensively tomorrow and keep you updated.

@malowe We have managed to merge a couple of merge requests on dunfell. So something did get fixed but now, on an MR for the same branch, it fails with a different error: eclipse/oniro-core/oniro!63 (merged)

2022-04-08 14:05:58,127 WARN  [...] No users found for mail 'pianon@array.eu'
2022-04-08 14:05:58,127 ERROR [...] Author must have an Eclipse Account
2022-04-08 14:06:39,048 WARN  [...] Could not find user account with mail 'pianon@array.eu'

Alberto's email address in his Eclipse.org account is alberto@pianon.eu. So I guess we need to do the same kind of hack again.

@fgurr but we still miss something. I was able to merge eclipse/oniro-core/oniro!14 (merged) a couple of hours ago and the offending commit above is before 14. After being able to merge 14 without problems, the eclipse/oniro-core/oniro!63 (merged) started to fail. Both are on the same branch and again, the commit above was in history before any of these MRs.

I don't know why you were able to merge eclipse/oniro-core/oniro!14 (merged). I would have expected it to fail due to Alberto's commit as well. Maybe @malowe has an idea?

@fgurr Would you please be so kind as to handle this corner case too? In the same way we did with the last workaround. Thanks in advance. We can deploy the workaround while we investigate why this happened for 63 and not for 14.

We're going to investigate into this a bit more to see if we can figure this out and hopefully see if there may be other lurking issues. I'm not sure why this is triggering since this is an older commit and should have also been problematic before, but I'll be looking into it today

The ECA shouldn't need to validate commits that already exist in the history. For testing purposes, I created a local git bare repo and normal git repo for creating new commits.

I installed the following git pre-received hook on the git bare repo:

#!/usr/bin/env python

import sys
import fileinput
import subprocess

# Read in each ref that the user is trying to update
for line in fileinput.input():
    line_array = line.split(' ')
    line_array.pop(2)

    sha = subprocess.check_output(["git", "rev-list", '...'.join(line_array)])
    print "validate: " + sha;

# Abort the push
# sys.exit(1)

Inspired by: https://www.atlassian.com/git/tutorials/git-hooks#server-side-hooks

This outputs something like:

remote: pre-receive: Trying to push ref: 816242af54a8d58afc1bdb8e2fb0cd67c5772ccf 887d69efde4c6aa70dbd9e7799298bc7156cdf5c refs/heads/master

Using that information, we can then extract the list of new commits: https://git-scm.com/docs/git-rev-list

git rev-list 816242af54a8d58afc1bdb8e2fb0cd67c5772ccf...887d69efde4c6aa70dbd9e7799298bc7156cdf5c

887d69efde4c6aa70dbd9e7799298bc7156cdf5c 846713309d12962d50224f8b7e2c64cb39db6afe

These 2 commits SHA are the new ones that I just push and these are the only 2 commits that the ECA validator needs to validate.

@malowe Is currently investigating if this solution could work for us. This will require some testing before we consider changing our current pre-receive hook.

If anyone has suggestions on a better solution, please don't hesitate to share them with us!

@malowe indeed. Something like that I also had in mind when talking to @malowe. It sounds like the diff should always be against the old head as opposed to the default branch.

I've updated an MR with new logic that is stable in testing but will need additional testing on staging before moving to production: eclipsefdn/it/api/git-eca-rest-api!72 (merged)

Thanks Martin for getting a PR done so quickly!

I agree that this needs a bit more testing and it's not something we should deploy on a Friday!

Thanks @malowe. This looks reasonable to me.

We are planning to deploy this to production on Tuesday morning as we've completed testing for this during the morning. We'll keep this issue posted for when we update it.

Thanks for the update @malowe. I'll keep an eye on this.

It may be resolved now, as it was deployed early. Let us know if it works for you!

@malowe We have been playing with this for a good while now and everything seems to work as expected. Thanks for dealing with this and we can close this issue now.

Thanks for your patience! I'm glad we were able to find a more stable solution without any hacks haha!

closed

mentioned in issue eclipse/oniro-core/oniro#470 (closed)

added to epic &12 (closed)

Gitlab MR issue: pre-receive hook failed. Try again.

Summary

Steps to reproduce

What is the current bug behavior?

What is the expected correct behavior?

Relevant logs and/or screenshots

Designs

Child items ...

Activity