New issue
Advanced search Search tips

Issue 758929 link

Starred by 1 user
Status: Fixed
Owner:
qyears...@chromium.org
Closed: Sep 2017
Cc:
robertma@chromium.org
foolip@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

[WPT import] wpt-importer exits before CLs actually land

Project Member Reported by raphael....@intel.com, Aug 25 2017 
I've recently noticed that wpt-importer seems to consider a job finished as soon as it manages to CQ+2 its CL (or at some point close to that). In other words, it does not actually wait for the change to be processed and the CL to be officially closed and marked as merged.

This didn't cause any troubles in the past, but changing wpt-importer to run continuously has shown that a new import job can start before the previous CL actually lands.

Case in point:
* https://luci-milo.appspot.com/buildbot/chromium.infra.cron/wpt-importer/578 ran just fine and finished with "All jobs finished" + "Update completed". Meanwhile, chromium_presubmit was still processing the job's respective CL before merging it.
* Meanwhile, as soon as job 578 finished, https://luci-milo.appspot.com/buildbot/chromium.infra.cron/wpt-importer/579 started, and it says "Importing wpt@88f290de1d6561f27e7b1e4bde8ab21eadcd61d2 to Chromium ae419bbed497aacbd2b2d93842118eb0d49f9ebe".
* Chromium revision ae419bbed497aacbd2b2d93842118eb0d49f9ebe is the one immediately preceding ebf2c7e52defa851cec0025877ec8f0e4a3909d7, which is the commit corresponding to job 578's CL.
* Consequently, job 579's CL also included the changes from job 578 (the css/css-value-3 import and a change to a WebRTC test) because it was using a Chromium revision that did not incorporate that commit.
* The new job (579) is currently running fine, as when the trybots apply the diff the bits which are already in are just skipped. I think everything's going to work and the duplicate changes will be removed when the CL is rebased, but it's still at least weird.

Comment 1 by foolip@chromium.org, Aug 25 2017 

Good find, thanks for filing this, Raphael!

If expectations are updated by both CLs with overlapping imports, then conflicts are quite likely. I don't think disaster is one of the possible outcomes, but it does end up confusing.

Leaving as P3, but if the effort to fix is small, it's still worth doing I think.

Comment 2 by qyears...@chromium.org, Aug 25 2017 

Indeed, good diagnosis of what happened.

Checking CL status (closed/open etc) can be done with git-cl (git cl status --field=status), and there's already waiting logic in our GitCL helper module.


So I'm imagining is that in TestImporter we should have something like:

   self.git_cl.wait_for_closed_status()

And GitCL should be refactored a bit so that we have a wait_for(func) function extracted out of the function we use to wait for try jobs to finish.

Comment 3 by foolip@chromium.org, Aug 28 2017

Labels: -Pri-3 Pri-2

Comment 4 by robertma@chromium.org, Aug 29 2017 

Two failure modes caused by this issue I've seen so far:

* One CL is a subset of (including identical to) another CL, so when the subset CL is submitted after the other CL, there will be no changes and chromium_presubmit fails (empty diff).
* Two parallel CLs are slightly different (usually due to MANIFEST.js), and the later cannot be submitted (all CQ bots fail because of patch error).

It seems like this is happening almost every other import. Still no real harm though.

Comment 5 by foolip@chromium.org, Aug 31 2017 

I believe this was the cause of failure in https://build.chromium.org/p/chromium.infra.cron/builders/wpt-importer/builds/1000

There was a patch failure in CQ in WPT_BASE_MANIFEST.json, and only the importer itself has modified that file recently.
Project Member

Comment 7 by bugdroid1@chromium.org, Sep 1 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/be03d45198ac59099799e34d649494901b5b53c1

commit be03d45198ac59099799e34d649494901b5b53c1
Author: Quinten Yearsley <qyearsley@chromium.org>
Date: Fri Sep 01 20:07:24 2017

Refactoring: extract a wait_for function in GitCL.

Bug:  758929 
Change-Id: Ie2abde17d26673d0ffe4cc3fc838acdc618307bc
Reviewed-on: https://chromium-review.googlesource.com/641782
Commit-Queue: Quinten Yearsley <qyearsley@chromium.org>
Reviewed-by: Philip Jägenstedt <foolip@chromium.org>
Cr-Commit-Position: refs/heads/master@{#499269}
[modify] https://crrev.com/be03d45198ac59099799e34d649494901b5b53c1/third_party/WebKit/Tools/Scripts/webkitpy/common/net/git_cl.py
[modify] https://crrev.com/be03d45198ac59099799e34d649494901b5b53c1/third_party/WebKit/Tools/Scripts/webkitpy/common/net/git_cl_unittest.py

Comment 8 by qyears...@chromium.org, Sep 7 2017

Cc: -qyears...@chromium.org
Owner: qyears...@chromium.org
Status: Assigned (was: Available)

Comment 10 by qyears...@chromium.org, Sep 15 2017

Status: Fixed (was: Assigned)
In theory this should be fixed now.
►

Sign in to add a comment