+some blink dev / infra folks ...

Yes, this is a very well known, sadly old problem; see  bug 310382 , and the slightly related bug 655202.

The "latest" copy is used to display the "test results" as per the link at the bottom of https://test-results.appspot.com/dashboards/flakiness_dashboard.html#testType=webkit_layout_tests&tests=external%2Fwpt%2Flongtask-timing%2Flongtask-in-sibling-iframe-crossorigin.html which says "Only shows actual results/diffs from the most recent *failure* on each bot.".

I've long thought that this was a confusing UI at best, and I would much rather just display the results from a single bot at a time (or a few, as selected), and from the actual results at that selected revision. If we did that, we could get rid of the "latest" copy, and it should be a pretty simple change in the test-results UI.

You ask about whether the "latest" data is necessary on the tryservers at all; that's a good question, and not one that I had thought of before. I think the answer is no, and so we could remove it pretty easily. It might make the UI a little confusing, but it's probably already confusing, since the "latest" result would be from some arbitrary job, not yours.

In my ideal world, I'd actually do a couple other things:

1) All of these files already exist as cache isolated files on the isolateserver, and so by changing the archive step to use your new artifact format, in theory we could link directly to the individual test result in the cache. This probably solves 90% of our users' needs.

2) There may be some cases where it's useful to fetch the entire results for local browsing, and so you still want something like a .zip file; in order to get a zip file,you still need to download the results from multiple shards to the builder, merge them together, and then re-upload them (either to the isolateserver or logdog, or a GS bucket). Alternatively, if we knew we had either a cached copy of every file or a persistent copy, we could just download the file and assemble it (like the swarming client does). There is some question about how to handle things that might expire from the isolateserver cache (e.g., for links from bugs to test results). I have ideas for that, too, but that's increasingly far afield.

In response to point 2), there is already a zip file uploaded which contains the exact same directory tree (I verified one manually myself). You can see this in the log in the description; that takes ~4 seconds to upload to google storage. 

Yes, in case I wasn't clear. As noted in the old bug, the current script uploads *4* copies of the test results, two as zip files and two as recursive copies. It's the recursive copies which are horribly slow, which is why I'd like to get rid of them altogether. 

Wow, I took for granted that archiving took a long time, and never looked into it. Now everything makes sense: archiving and uploading a tarball really shouldn't take that long; it's the recursive upload step that's the culprit. It's a known issue that recursive file operations like this are slow on Windows (we also have trouble in upstream WPT walking the big directory tree). Thanks for pointing this out!

One data point from myself: I've never used the "show results" button on the test-results dashboard. It's very confusing which build the results are from. What I usually do is: click on a result cell to show the popup -> build log -> layout_test_results, and search for the test in the viewer. It might be useful to add a link in the popup to the layout_test_results viewer directly.

dpranke@: There are actually some cases we'd like to download the whole result zip, but it's definitely not a common use case. I personally wouldn't mind using a command line tool (e.g. to download shards and assemble locally), if that'd make the infra healthier / faster, as long as there is still a way to do it. And that's just my personal opinions. foolip@, do you know anyone frequently downloading the whole layout test result zip?

 Issue 789133  has been merged into this issue.

I want to make progress on this bug.

I have a few ideas:

* Change the archive layout test results to not upload "latest" results on tryservers. This would save us a few minutes on them.

* Add zip file support to the test results server, and have it download the zip file result in JS, unzip, and add results to the page. This is doable I think; there's a zip javascript library out there I found via Google. Not sure how performant it is, but I think it could work. (We also might want to disable this for tests with '(with patch)' in it)

I think both of these are good ideas, and well worth pursuing. So, ask and ye shall receive, I'll assign it to you :).

For the first one, I think the next step would be to send a note to blink-dev to describe the proposed change and see if anyone strongly objects (and, if so, why). It may be that we need to tweak the test-results UI somewhat, but I'd bet we could safely just stop uploading it and get rid of the "latest" results UI in test-results altogether. I'll be happy to help with this part if need be.

For the second one, an alternative to the client-side unzip/display could be to add that functionality to test-results and do it there. A potential upside to this might be that then we've have an entrypoint that we can use to cleanly audit who is accessing which test results. I think eventually the right thing to do is to make sure that any time someone is retrieving a test result, they hit an app engine page instead of a GCS bucket directly; that would allow us to do smarter things with data retention and display of the results.

Please do not add JS gzip code to the test results web interface. If you absolutely must compress the data on the wire, I believe you can use "Content-Encoding: gzip" and let the browser decompress it natively.

And also, that frontend is doing waaaaaay to much processing in JS already. It bricks lower-end chromebooks today.

Re #10: We need to read a zip file, and display certain files in it on the page. So I don't think we can use gzip for that. I do like the idea in #9 of adding an endpoint on the backend to do this for us though. 

Re: #11 Doing it on the server SGTM. 

Thanks for looking into this, martiniss!

Regarding the two ideas:

* Removing the "latest" upload is a great first step. And I totally agree with dpranke@'s comment on this.
* The zip is tens of MiB big, with thousands of files in it. Handling the zip in JS may be impractical. In fact, I'm confused why we need to unzip. Are we trying to get rid of recursive upload altogether? Otherwise, doesn't that provide direct access to individual files via GCS? (I'm not familiar with test-result, so I might be missing something.)

Re #13; We are getting rid of recursive upload. We need to display files on the test results server though. Right now when you click "Show Results" on the app, it does a bunch of HTTP requests to GCS. This depends on recursive upload. If we remove that, we'll need to use the zip file as the source of information instead.

I think I'll send a PSA of sorts to blink-dev saying that I'm going to be working on changing how this script works, with a few proposed action items and a link to this bug. Maybe an "Intent to Implement"? Although based on me scanning the list those seem to be a somewhat formalized email template.

Re #14: Ahh I see. I thought you were to keep one recursive + one archived. I guess getting rid of all recursive uploads makes sense as it saves a lot of time on bots (esp. win bots), with the cost of some added complexity in test-result.

Also, "Intent to Implement" is usually meant for implementing web-exposed features (web APIs, etc.), which is a step in a formal procedure. Probably better to avoid confusion by using other subjects.

Based on my previous thinking about this problem, and my rough understanding of the ZIP format, I think you could implement this pretty easily with 2-3 short "range" HTTP requests to extract an individual member of the archive, rather than having to decompress the whole zip file. It's possible that if you did need to fetch and decompress the whole zip file, it'd (a) be awfully slow and (b) you'd hit app engine limits.

That said, prototyping something that did fetch and decompress everything would be the easy and obvious first step, of course.

martiniss@ is right that "Content-Encoding: gzip" wouldn't work. And, yes, getting rid of the recursive upload should be the goal, since that is orders-of-magnitude slower than uploading the single zip file. 

I wouldn't use an "Intent to implement" for this, it's too low-level and test-specific. Just a regular email should be fine.

I sent out an email (https://groups.google.com/a/chromium.org/d/msg/blink-dev/JbUa9OIwRAE/k_3zkLr-AAAJ), and got a few responses. Several people seem to still use "Show Results", so I don't think we can delete it directly. If we do, we should at least make sure their workflow is still supported.

What sort of responses? I'd be curious to know how people are using this.

I got 7 responses. 2 of them were generally positive that I was trying to make things better. The others were people saying they used either "Show results", or the results page (https://storage.googleapis.com/chromium-layout-test-archives/linux_layout_tests_layout_ng/2869/layout-test-results/results.html) which is currently only accessible because we upload the whole directory. Theoretically we could whitelist that file. 

Re #19:

There are two separate issues here. The "Show results" button links to the "most recent" result, which is really not clearly defined (see earlier discussion). Are people use that button aware what they are looking at? This button is also equivalent to manually find the failure in the grid and click through a couple links (build log-> layout_test_results -> search for the test name). If there is strong need for the button, this process can probably be automated in the test-results app without requiring the extra upload.

The second issue, which I also just realized, is about the result viewer (results.html). This is crucial for many Blink developers. The HTML itself is static. However, it loads a gigantic result JSON, and, upon clicking on a test, loads the individual result text/image. This currently depends on recursive upload.

Yeah, and the results viewer is linked from the build result page. So we'd need to probably keep that around. Which is problematic, and can't exactly be implemented directly with the zip file.

We could link to test results (or something else), which can unzip the file in "the cloud". Maybe?

Right, that's what we discussed with the new test-results endpoint that did that.

We definitely can't get rid of the ability to click through from a build step link to results.html and all of the related data and test artifacts.

I did some quick checks on cloud storage access logs, and several people have looked at the results pages. So we do definitely need those to work.

Also, switching to the new artifact format I designed for test results won't help currently, as the upload script uses `gsutil -r`, which is what is slow already. So this would need a custom solution, it seems.

I'm not sure what a good long term solution is, but it does seem to be a somewhat complicated problem. I'll at least work on a CL or two to disable uploading latest results on tryservers.

We probably should just dup this into  bug 310382 , no?

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/build/+/6c5b53851fb4e48c18898c7ff58dfb2395b5d5a6

commit 6c5b53851fb4e48c18898c7ff58dfb2395b5d5a6
Author: Stephen Martinis <martiniss@chromium.org>
Date: Wed Jan 10 21:10:39 2018

Don't archive latest layout test results on tryserver

Bug:  792737 
Change-Id: I780270cf2e86777a2ed041b884495f78cf654d0e
Reviewed-on: https://chromium-review.googlesource.com/858424
Commit-Queue: Stephen Martinis <martiniss@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>

[modify] https://crrev.com/6c5b53851fb4e48c18898c7ff58dfb2395b5d5a6/scripts/slave/recipes/blink_downstream.expected/full_client_v8_fyi_V8_Blink_Mac_fail.json
[modify] https://crrev.com/6c5b53851fb4e48c18898c7ff58dfb2395b5d5a6/scripts/slave/recipes/blink_downstream.expected/webkit_tests_interrupted.json
[modify] https://crrev.com/6c5b53851fb4e48c18898c7ff58dfb2395b5d5a6/scripts/slave/recipes/blink_downstream.expected/full_client_v8_fyi_V8_Blink_Win_fail.json
[modify] https://crrev.com/6c5b53851fb4e48c18898c7ff58dfb2395b5d5a6/scripts/slave/recipes/blink_downstream.expected/minimal_pass_continues.json
[modify] https://crrev.com/6c5b53851fb4e48c18898c7ff58dfb2395b5d5a6/scripts/slave/recipes/blink_downstream.expected/full_client_v8_fyi_V8_Blink_Linux_64__dbg__fail.json
[modify] https://crrev.com/6c5b53851fb4e48c18898c7ff58dfb2395b5d5a6/scripts/slave/recipes/blink_downstream.expected/webkit_tests_unexpected_error.json
[modify] https://crrev.com/6c5b53851fb4e48c18898c7ff58dfb2395b5d5a6/scripts/slave/recipes/blink_downstream.expected/full_client_v8_fyi_V8_Blink_Linux_64_fail.json
[modify] https://crrev.com/6c5b53851fb4e48c18898c7ff58dfb2395b5d5a6/scripts/slave/recipe_modules/chromium_tests/tests/steps/generate_isolated_script.expected/custom_webkit_tests_step_name.json
[modify] https://crrev.com/6c5b53851fb4e48c18898c7ff58dfb2395b5d5a6/scripts/slave/recipes/blink_downstream.expected/full_client_v8_fyi_V8_Blink_Linux_64___future_pass.json
[modify] https://crrev.com/6c5b53851fb4e48c18898c7ff58dfb2395b5d5a6/scripts/slave/recipe_modules/chromium_tests/tests/steps/blink_test.expected/android.json
[modify] https://crrev.com/6c5b53851fb4e48c18898c7ff58dfb2395b5d5a6/scripts/slave/recipes/blink_downstream.expected/full_client_v8_fyi_V8_Blink_Linux_64__dbg__pass.json
[modify] https://crrev.com/6c5b53851fb4e48c18898c7ff58dfb2395b5d5a6/scripts/slave/recipe_modules/chromium_tests/tests/steps/blink_test.expected/unexpected_flakes.json
[modify] https://crrev.com/6c5b53851fb4e48c18898c7ff58dfb2395b5d5a6/scripts/slave/recipe_modules/chromium_tests/tests/steps/generate_isolated_script.py
[modify] https://crrev.com/6c5b53851fb4e48c18898c7ff58dfb2395b5d5a6/scripts/slave/recipes/blink_downstream.expected/full_client_v8_fyi_V8_Blink_Mac_pass.json
[modify] https://crrev.com/6c5b53851fb4e48c18898c7ff58dfb2395b5d5a6/scripts/slave/recipes/blink_downstream.expected/full_client_v8_fyi_V8_Blink_Linux_64_pass.json
[modify] https://crrev.com/6c5b53851fb4e48c18898c7ff58dfb2395b5d5a6/scripts/slave/recipes/blink_downstream.expected/full_client_v8_fyi_V8_Blink_Linux_64___future_fail.json
[modify] https://crrev.com/6c5b53851fb4e48c18898c7ff58dfb2395b5d5a6/scripts/slave/recipe_modules/chromium_tests/tests/steps/blink_test.expected/win.json
[modify] https://crrev.com/6c5b53851fb4e48c18898c7ff58dfb2395b5d5a6/scripts/slave/recipe_modules/chromium_tests/steps.py
[modify] https://crrev.com/6c5b53851fb4e48c18898c7ff58dfb2395b5d5a6/scripts/slave/recipes/blink_downstream.expected/too_many_failures_for_retcode.json
[modify] https://crrev.com/6c5b53851fb4e48c18898c7ff58dfb2395b5d5a6/scripts/slave/chromium/archive_layout_test_results.py
[modify] https://crrev.com/6c5b53851fb4e48c18898c7ff58dfb2395b5d5a6/scripts/slave/recipe_modules/chromium_tests/tests/steps/blink_test.expected/big.json
[modify] https://crrev.com/6c5b53851fb4e48c18898c7ff58dfb2395b5d5a6/scripts/slave/README.recipes.md
[modify] https://crrev.com/6c5b53851fb4e48c18898c7ff58dfb2395b5d5a6/scripts/slave/recipe_modules/chromium_tests/tests/steps/blink_test.py
[modify] https://crrev.com/6c5b53851fb4e48c18898c7ff58dfb2395b5d5a6/scripts/slave/recipes/blink_downstream.expected/full_client_v8_fyi_V8_Blink_Win_pass.json

We could dup into that. This bug is a bit more focused; it's about removing results upload. So it would fix that bug maybe? I'm fine with merging or not.

Windows archiving time got cut in half. See attached graph.

Deleted: archive_time_faster.png 289 KB

	archive_time_faster.png 289 KB View Download

Driveby ideas:

results.html and unpacked directories are only viewed for a small percentage of builds.

Not doing recursive upload seems like a big win.

Why not create the directory tree on demand?

You could build a build-results.chromium.org server. It'd serve static results.html, and download/extract the zip on demand. Cache could be prewarmed
by pre-extracting results if build has failed.

We could build something as suggested in #29. That would probably need to go on a team's OKR; I could maybe build that this quarter, but I am somewhat busy.

I have a question about the results that we're archiving. To take https://ci.chromium.org/buildbot/tryserver.chromium.mac/mac_chromium_rel_ng/642830 as an example, it has 8000 files in it, and running `find . -type f | wc -l' gives me 16497. There's only one unexpected failed test in that; I believe the rest of the files in there are for expected failed tests, or unexpected passing tests. Why are we archiving these files? Deleting them would delete a ton of files from this directory we're uploading. Is there a history here?


(from the layout test command)
* Passed: 58971 (58872 expected, 99 unexpected)
* Skipped: 12839 (12839 expected, 0 unexpected)
* Failed: 1528 (1527 expected, >>>1 unexpected<<<)
* Flaky: 35 (35 expected, 0 unexpected)

Unexpected passes and expected failures are needed for rebaselining (and
investigating). Each test may produce ~5 files (text & image outputs,
expected output, stdout, stderr). Yet the number still doesn't add up.
Perhaps I missed something.

Yes, I think the thing in #29 is the way to go, though I'd just add an entry point to test-results to do it, it shouldn't be too hard.

I expect there's a lot of tests that produce extraneous stderr output even though they otherwise pass.

I hacked up a ~50 lines of go which unpacks a zip file and reads out a file. You can use it to view the results file just fine, compare https://13861-965cb36-tainted-martiniss-dot-test-results-test-hrd.appspot.com/data/layout_results/mac_chromium_rel_ng/644817/layout-test-results/results.html to https://storage.googleapis.com/chromium-layout-test-archives/mac_chromium_rel_ng/644817/layout-test-results/results.html

Looks like it loads fairly fast, with no caching. It could end up making a lot of network traffic for the app, but other than that it seems like what I wrote would just work. 

https://chromium-review.googlesource.com/c/infra/infra/+/905787 is a CL to add this.

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

commit 71f2f93493d14ca35f6a30b4dd84958854105616
Author: Stephen Martinis <martiniss@chromium.org>
Date: Wed Feb 07 23:59:23 2018

Add zip handler for layout test results

This will be used to replace links to google storage. See bug for
more background.

Bug:  792737 
Change-Id: Id670390cd467746e32b399b1bf04cbb7025d8b7f
Reviewed-on: https://chromium-review.googlesource.com/905787
Commit-Queue: Stephen Martinis <martiniss@chromium.org>
Reviewed-by: Sean McCullough <seanmccullough@chromium.org>

[modify] https://crrev.com/71f2f93493d14ca35f6a30b4dd84958854105616/go/src/infra/appengine/test-results/frontend/handlers.go
[add] https://crrev.com/71f2f93493d14ca35f6a30b4dd84958854105616/go/src/infra/appengine/test-results/frontend/zip.go

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

commit 7b273edad30d3c7307c471c1338b64c894562522
Author: Stephen Martinis <martiniss@chromium.org>
Date: Thu Feb 08 00:41:33 2018

Don't authenticate layout result GCS requests

These should be publically accessible.

Bug:  792737 
Change-Id: I2a24fe094f2ded36b02be697c54a90f3460aa38f
Reviewed-on: https://chromium-review.googlesource.com/907852
Reviewed-by: Sean McCullough <seanmccullough@chromium.org>
Commit-Queue: Stephen Martinis <martiniss@chromium.org>

[modify] https://crrev.com/7b273edad30d3c7307c471c1338b64c894562522/go/src/infra/appengine/test-results/frontend/zip.go

https://chromium.googlesource.com/chromium/tools/build/+/fd1cb48acd7a5531b899d4597b82c7246802f2b3 landed (wrong bug). I'll look tonight to make sure the links are still working. 

An update: I reverted https://chromium.googlesource.com/chromium/tools/build/+/fd1cb48acd7a5531b899d4597b82c7246802f2b3 today because of a few bugs discovered and reported by users. I've fixed the bugs, and will probably reland that change tomorrow. 

Once that has stuck, I'd like to remove the recursive upload. I'll check google storage access logs after a few days to see if I've missed any references to them. I believe that I'll also need to change the existing test results JS code to reference the new endpoint rather than google storage.

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

commit 3c4fb685d5b1939119ed00d49f892091f5322635
Author: Stephen Martinis <martiniss@chromium.org>
Date: Fri Feb 09 20:55:08 2018

Test results zip handler: Add user agent caching

Also adds basic tests.

Bug:  792737 

Change-Id: Ia4ce7f798f8498afbd1b268bc2dd2db5ca4517b6
Reviewed-on: https://chromium-review.googlesource.com/910327
Commit-Queue: Stephen Martinis <martiniss@chromium.org>
Reviewed-by: Sean McCullough <seanmccullough@chromium.org>

[add] https://crrev.com/3c4fb685d5b1939119ed00d49f892091f5322635/go/src/infra/appengine/test-results/frontend/zip_test.go
[modify] https://crrev.com/3c4fb685d5b1939119ed00d49f892091f5322635/go/src/infra/appengine/test-results/frontend/zip.go

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

commit 5ba8c4d278893cbab14be08c78ee40cc3a5a3222
Author: Stephen Martinis <martiniss@chromium.org>
Date: Fri Feb 09 21:09:48 2018

Flakiness dashboard: Change layout test result base URL

Bug:  792737 
Change-Id: Ia81327b8453732a53eb02dc5b09652953c4af829
Reviewed-on: https://chromium-review.googlesource.com/910084
Commit-Queue: Stephen Martinis <martiniss@chromium.org>
Reviewed-by: Sean McCullough <seanmccullough@chromium.org>

[modify] https://crrev.com/5ba8c4d278893cbab14be08c78ee40cc3a5a3222/go/src/infra/appengine/test-results/frontend/static/dashboards/js/flakiness_dashboard.js

I'm landing https://chromium-review.googlesource.com/c/chromium/tools/build/+/911971 and https://chromium-review.googlesource.com/c/infra/infra/+/910084, which will switch everything over to point at the test results server. 

Just a big thank you. I made a suggestion, and you made it work. Looking forward to faster Window7 test runs.

Update! Everything seems to be going well. I haven't seen any new bugs come in, and I've been looking through the test result server logs, and no 500s or weird errors seem to be popping up. 

I might land a CL to remove recursive upload today. Might wait until tomorrow.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/build/+/1e08f94ae5ca0489d1c2ee700029c65495756b3b

commit 1e08f94ae5ca0489d1c2ee700029c65495756b3b
Author: Stephen Martinis <martiniss@chromium.org>
Date: Mon Feb 12 23:14:57 2018

Archive layout tests: Remove recursive uploads

These are no longer needed, due to the addition of a HTTP
endpoint on the test results server which serves individual files.

Bug:  792737 
Change-Id: I46ca0e1310aed96eec0c5896a6f8fb0c46ccf8ca
Reviewed-on: https://chromium-review.googlesource.com/914715
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Commit-Queue: Stephen Martinis <martiniss@chromium.org>

[modify] https://crrev.com/1e08f94ae5ca0489d1c2ee700029c65495756b3b/scripts/slave/chromium/archive_layout_test_results.py

Tentatively marking as fixed, based on #44. I'll verify tomorrow.

Actually, just found a few successful builds. https://ci.chromium.org/buildbot/tryserver.chromium.mac/mac_chromium_rel_ng/649022 is one, which takes *2 seconds* to archive results. Woo-hoo!

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

commit f18ab3dcd3c3ede9bcbaa6621af1a2658d36901b
Author: Quinten Yearsley <qyearsley@chromium.org>
Date: Thu Feb 15 17:04:57 2018

Update layout test results base URL

Bug:  811976 ,  792737 
Change-Id: I80224870f2cf97e28d61d8bce19a596f8d582497
Reviewed-on: https://chromium-review.googlesource.com/917404
Commit-Queue: Quinten Yearsley <qyearsley@chromium.org>
Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org>
Reviewed-by: Stephen Martinis <martiniss@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537051}
[modify] https://crrev.com/f18ab3dcd3c3ede9bcbaa6621af1a2658d36901b/third_party/WebKit/Tools/Scripts/webkitpy/common/net/buildbot.py
[modify] https://crrev.com/f18ab3dcd3c3ede9bcbaa6621af1a2658d36901b/third_party/WebKit/Tools/Scripts/webkitpy/common/net/buildbot_unittest.py
[modify] https://crrev.com/f18ab3dcd3c3ede9bcbaa6621af1a2658d36901b/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/try_flag_unittest.py
[modify] https://crrev.com/f18ab3dcd3c3ede9bcbaa6621af1a2658d36901b/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_cl_unittest.py
[modify] https://crrev.com/f18ab3dcd3c3ede9bcbaa6621af1a2658d36901b/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_unittest.py