New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 803971 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

Tricium: Failed to call Launcher.Launch ... missing paths to analyze

Project Member Reported by qyears...@chromium.org, Jan 19 2018

Issue description

This is a recurring error in the tricium-dev logs:

Failed to call Launcher.Launch :: {"error":"rpc error: code = InvalidArgument desc = missing paths to analyze"

Example cases:

 Run ID 5710606233501696
 Run ID 6234694920175616
 Run ID 5471866718257152

For some cases, I can't find the corresponding gerrit change.

For others, e.g. run 6234694920175616, it appears to map to an issue like https://gerrit-review.googlesource.com/c/gerrit/+/152792 that should have paths to analyze.

I believe I saw this error in logs for an instance deployed more than a week ago.

The error is logged here:
https://cs.chromium.org/chromium/infra/go/src/infra/tricium/appengine/launcher/rpc_launch.go?l=47
The launch request is created in the analyze request:
https://cs.chromium.org/chromium/infra/go/src/infra/tricium/appengine/frontend/rpc_analyze.go?l=165
 

Comment 1 by emso@chromium.org, Jan 22 2018

Labels: Tricium
Note, there are tasks in the launcher-queue that are retried periodically that hit this error.
Cc: qyears...@chromium.org
 Issue 817808  has been merged into this issue.
Cc: -qyears...@chromium.org
Owner: qyears...@chromium.org
Status: Assigned (was: Available)
Note, examining one of those changes above, https://gerrit-review.googlesource.com/c/gerrit/+/152792/1, reveals that in some cases, there *really are* no paths to analyze in a CL.

And of course, now that we're filtering out deleted files, it's certainly possible that there will be only deleted files.

Of course, the commit message is there, and we might want to treat that as a file. It can't be checked out exactly the same as other files in GitFileIsolator, but we could treat it as a special case there.

Possible solutions:

 - If there are no files, don't launch analysis.
 - If there are no files, launch analysis and then finish shortly afterwards  with no results.
 - Count "Commit message" as a file. This potentially has the advantage that we could then put robocomments in the commit message (e.g. for spelling errors, whitespace issues, invalid commit footers). Then we could assume there's always one file.
I'm not sure what to do yet, but for now I have a proposal:

1. First, launch analysis even if there are no paths.

2. Later, if we want to treat commit message as a regular file:
  a. Add support for the "/COMMIT_MSG" magic path in GitFileIsolator
  b. Add "/COMMIT_MSG" for all analyze requests for Gerrit changes.
Status: Started (was: Assigned)
Made an initial CL https://chromium-review.googlesource.com/c/infra/infra/+/945008 to allow empty path list. But upon further consideration, I think it makes sense to avoid launching any workflow if there's definitely no work to do.

If we do want to analyze the commit message, then we'll probably want to do it by adding /COMMIT_MSG in the list of paths in the AnalyzeRequest, in which case the list of paths won't be empty.

Since it's still possible for the list of paths to be legitimately empty in any incoming AnalyzeRequest, I think it'd make sense to:

 1 Allow an AnalysisRequest with empty paths,
   but just log and abort without launching a workflow run.
 2 Don't allow a LaunchRequest with empty paths.

Another thought:

If we do want to analyze the commit message, then we may want to do it in gerrit/poll.go, because /COMMIT_MSG will be avilable for Gerrit changes, but not necessarily for all repos/refs that could be put in an AnalyzeRequest.

In that case, if there are no Paths then there's nothing to analyze, so we could abort before creating an AnalyzeRequest. Proposed CL for this: https://canary-chromium-review.googlesource.com/c/infra/infra/+/956606
Project Member

Comment 8 by bugdroid1@chromium.org, Mar 13 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/infra/+/7ac76e75866a349747d441b3f24cde3f2330299f

commit 7ac76e75866a349747d441b3f24cde3f2330299f
Author: Quinten Yearsley <qyearsley@chromium.org>
Date: Tue Mar 13 19:49:23 2018

Don't analyze changes with no files

This change makes the Gerrit poller not make analyze
requests for changes with no files.

This is the case whenever the only file is /MERGE_LIST,
or if the change only deletes files, etc.

Related change https://crrev.com/c/950241 also adds
validation for empty analyze requests added by the poller,
which shouldn't happen any more after this CL.

Bug:  803971 
Change-Id: Ic94255441f879c994b1a2ad3ffb5c793f28d347c
Reviewed-on: https://chromium-review.googlesource.com/956606
Reviewed-by: Marc-Antoine Ruel <maruel@chromium.org>
Commit-Queue: Marc-Antoine Ruel <maruel@chromium.org>

[modify] https://crrev.com/7ac76e75866a349747d441b3f24cde3f2330299f/go/src/infra/tricium/appengine/gerrit/poll.go
[modify] https://crrev.com/7ac76e75866a349747d441b3f24cde3f2330299f/go/src/infra/tricium/appengine/gerrit/poll_test.go

Project Member

Comment 9 by bugdroid1@chromium.org, Mar 13 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/infra/+/7a301aca42c3da44c195309c9ec4a0fa02d986ba

commit 7a301aca42c3da44c195309c9ec4a0fa02d986ba
Author: Quinten Yearsley <qyearsley@chromium.org>
Date: Tue Mar 13 21:48:36 2018

Validate AnalyzeRequests coming from the analyze queue.

AnalyzeRequests going to the RPC endpoint are checked;
they should be checked in the same way if they come through
the analyze queue.

The expected result of this is that CLs with only deleted
files would now cause an error to be logged in the analyze
queue handler.

Related change:
https://crrev.com/c/956606 Don't create analyze requests
for changes with no paths.

Bug:  803971 
Change-Id: I711d756ca4d220230305e5cd6e7f898526bf2619
Reviewed-on: https://chromium-review.googlesource.com/950241
Reviewed-by: Marc-Antoine Ruel <maruel@chromium.org>
Commit-Queue: Quinten Yearsley <qyearsley@chromium.org>

[modify] https://crrev.com/7a301aca42c3da44c195309c9ec4a0fa02d986ba/go/src/infra/tricium/appengine/frontend/handlers.go
[modify] https://crrev.com/7a301aca42c3da44c195309c9ec4a0fa02d986ba/go/src/infra/tricium/appengine/frontend/handlers_test.go
[modify] https://crrev.com/7a301aca42c3da44c195309c9ec4a0fa02d986ba/go/src/infra/tricium/appengine/common/testing/testing.go
[modify] https://crrev.com/7a301aca42c3da44c195309c9ec4a0fa02d986ba/go/src/infra/tricium/appengine/frontend/run_test.go

Status: Fixed (was: Started)
In theory this should now be fixed in the latest version.

Sign in to add a comment