Bug 229004 – [check-github-mirror-integrity] Differentiate between slow sync and collapsed commits

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Jonathan Bedard]

We should modify check-github-mirror-integrity to distinguish between the sync process being slow (or broken) and git-svn collapsing commits.

[Radar WebKit Bug Importer]

<rdar://problem/81795644>

[Jonathan Bedard]

Created attachment 435350 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 435350 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=435350&action=review

> Tools/Scripts/check-github-mirror-integrity:58
> +        print('This indicates a serious error with the repository, please inform [e-mail] urgently')

Do we still need to ask people to inform [e-mail], given that emails are already sent? Maybe this should say something like "A notification will be sent to WebKit admins".

[Jonathan Bedard]

Comment on attachment 435350 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=435350&action=review

>> Tools/Scripts/check-github-mirror-integrity:58
>> +        print('This indicates a serious error with the repository, please inform [e-mail] urgently')
> 
> Do we still need to ask people to inform [e-mail], given that emails are already sent? Maybe this should say something like "A notification will be sent to WebKit admins".

Probably not, I don't expect this script will be run out of automation

[Jonathan Bedard]

Created attachment 435355 [details]
Patch

[Aakash Jain]

Comment on attachment 435350 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=435350&action=review

> Tools/Scripts/check-github-mirror-integrity:48
> +    timestamps_equal = mirror_tip.timestamp == canonical_tip.timestamp

what is this timestamp? is it timestamp of tot commit?

> Tools/Scripts/check-github-mirror-integrity:55
> +    if timestamps_equal and mirror_tip.identifier != canonical_tip.identifier:

How is this case possible?
Also what if timestamps_equal is False? Does that go to default failure case? If so, is this if condition even required?

> Tools/Scripts/check-github-mirror-integrity:63
> +        print('GitHub is {} behind svn.webkit.org, but seems to be updating, This is ok.'.format(pluralize(difference, 'commit')))

how do we know about "seems to be updating"

[Jonathan Bedard]

(In reply to Aakash Jain from comment #6)
> Comment on attachment 435350 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=435350&action=review
> 
> > Tools/Scripts/check-github-mirror-integrity:48
> > +    timestamps_equal = mirror_tip.timestamp == canonical_tip.timestamp
> 
> what is this timestamp? is it timestamp of tot commit?

Yes, it's the timestamp of the tot commit for a given repository representation.

> 
> > Tools/Scripts/check-github-mirror-integrity:55
> > +    if timestamps_equal and mirror_tip.identifier != canonical_tip.identifier:
> 
> How is this case possible?
> Also what if timestamps_equal is False? Does that go to default failure
> case? If so, is this if condition even required?

This case is the one we're really looking out for. Where git-svn thinks that it's correctly updated things (ie, the timestamps for the time of tree match), but the identifier indicates that we've missed a commit.

> 
> > Tools/Scripts/check-github-mirror-integrity:63
> > +        print('GitHub is {} behind svn.webkit.org, but seems to be updating, This is ok.'.format(pluralize(difference, 'commit')))
> 
> how do we know about "seems to be updating"

To get to this case, two things must be true:
1) identifiers don't match
2) The timestamp of the tip of tree SVN commit is recent enough that we could plausibly be in the process of updating the mirror

It's the second bit that leads us to believe we are still updating. Not enough time has passed to be suspicious of the mirror process yet, I figure we should usually have everything forwarded within 5 minutes of the original commit.

[Aakash Jain]

Comment on attachment 435350 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=435350&action=review

r=me with comments

>>> Tools/Scripts/check-github-mirror-integrity:55
>>> +    if timestamps_equal and mirror_tip.identifier != canonical_tip.identifier:
>> 
>> How is this case possible?
>> Also what if timestamps_equal is False? Does that go to default failure case? If so, is this if condition even required?
> 
> This case is the one we're really looking out for. Where git-svn thinks that it's correctly updated things (ie, the timestamps for the time of tree match), but the identifier indicates that we've missed a commit.

ok

>>> Tools/Scripts/check-github-mirror-integrity:58
>>> +        print('This indicates a serious error with the repository, please inform [e-mail] urgently')
>> 
>> Do we still need to ask people to inform [e-mail], given that emails are already sent? Maybe this should say something like "A notification will be sent to WebKit admins".
> 
> Probably not, I don't expect this script will be run out of automation

we run this in automation in commit-queue

> Tools/Scripts/check-github-mirror-integrity:62
> +    if time.time() < canonical_tip.timestamp + 5 * 60:

Might be better to save this 5 minute time in a variable, e.g. TIME_LIMIT_FOR_SYNCING = 5*60

>>> Tools/Scripts/check-github-mirror-integrity:63
>>> +        print('GitHub is {} behind svn.webkit.org, but seems to be updating, This is ok.'.format(pluralize(difference, 'commit')))
>> 
>> how do we know about "seems to be updating"
> 
> To get to this case, two things must be true:
> 1) identifiers don't match
> 2) The timestamp of the tip of tree SVN commit is recent enough that we could plausibly be in the process of updating the mirror
> 
> It's the second bit that leads us to believe we are still updating. Not enough time has passed to be suspicious of the mirror process yet, I figure we should usually have everything forwarded within 5 minutes of the original commit.

It would be better to say "might be updating", instead of "seems to be updating", since we have no indication if it is updating or is permanently stuck for some reason.

[Jonathan Bedard]

Created attachment 435421 [details]
Patch for landing

[EWS]

Committed r280967 (240473@main): <https://commits.webkit.org/240473@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 435421 [details].