Arvados

Sorry for the delay, this was not an easy diff to read for me :). I tried to start a fresh arvbox instance to do some manual testing, but after failing and wasting too much time on that... I gave up.

• File lib/crunchrun/container_gateway.go
  • Comment at line 84 I believe should mention /arvados/v1/container/{uuid}/log/
• File lib/controller/localdb/container_gateway_test.go
  • Lines 291-295 are commented out code that I think could be removed.
• Re: gorilla/mux (not a blocker, I think it can be a different story) would it be a good to time to forking it and use it on our own code? I think we can benefit from GitHub's security warnings this way? (not sure if we would get those otherwise)
• IIRC, the cadaver tests were an intergration test suite for KeepWeb. Do you think it would be useful/possible to add a test for the "already finished container log" case?
• For the future: There're many aspects of how the container gateway works that aren't known or fresh in my mind; I tried to find some high-level explanation on the wiki but couldn't find, I think it would be interesting to have some coarse grained explanation for devs to get up to speed without reading the code.

• Comment at line 84 I believe should mention /arvados/v1/container/{uuid}/log/

Fixed.

• Lines 291-295 are commented out code that I think could be removed.

Fixed.

• Re: gorilla/mux (not a blocker, I think it can be a different story) would it be a good to time to forking it and use it on our own code? I think we can benefit from GitHub's security warnings this way? (not sure if we would get those otherwise)

I've just subscribed to security notifications at https://github.com/gorilla/mux (does it notify earlier if we have a fork?)

• IIRC, the cadaver tests were an intergration test suite for KeepWeb. Do you think it would be useful/possible to add a test for the "already finished container log" case?

Indeed, this turned out to be very useful. Cadaver worked fine in the unfinished case, but in the proxy-to-keep-web case it noticed that the paths in the webdav responses didn't match the request, and bailed out (unless using -t to ignore).

To fix this I needed to change the proxy strategy: use the ID-in-host request form, preserve the original path, and send a new X-Webdav-Prefix header to keep-web so it knows to ignore the "/arvados/v1/containers/{uuid}/log" part.

19889-live-log-webdav @ d7f388108615d981636e464785129ce4a958a7f5 -- developer-run-tests: #3567

retry wb1 developer-run-tests-apps-workbench-integration: #3843

• For the future: There're many aspects of how the container gateway works that aren't known or fresh in my mind; I tried to find some high-level explanation on the wiki but couldn't find, I think it would be interesting to have some coarse grained explanation for devs to get up to speed without reading the code.

I think the best we have is

Efficient live access to container logs

https://doc.arvados.org/main/architecture/dispatchcloud.html

https://doc.arvados.org/main/architecture/hpc.html

This LGTM, thanks!

Re:

• Re: gorilla/mux (not a blocker, I think it can be a different story) would it be a good to time to forking it and use it on our own code? I think we can benefit from GitHub's security warnings this way? (not sure if we would get those otherwise)

I've just subscribed to security notifications at https://github.com/gorilla/mux (does it notify earlier if we have a fork?)

I think it notifies the whole team automatically instead of needing to manually subscribe, but given that you'll probably be the best person to handle those updates, this is good enough :)

While working on #18790 I discovered that arvados-controller routing wasn't working because the /containers/ routes still go through the old generic railsproxy path instead of the new path.

I fixed this, and took the opportunity to add routing for .../arvados/v1/containers/{uuid}/ssh and .../container_gateway as well. The old .../connect/{uuid}/ssh endpoints are still supported, and still used by client-side code, so 2.7 clients will work with 2.6 servers and vice versa. In a future version we can switch client-side code to use the new endpoints, and then eventually delete the old ones. Looking back I should have hacked the routing this way in the first place instead of having a temporary endpoint outside of /containers/.

Also tweaked logging so "file not found" doesn't get logged as an error. Too noisy when clients are polling for stderr.txt to exist, etc.

19889-live-log-webdav @ 6d03fdac5674dc88eff821f5e8ac70642f39a895 -- developer-run-tests: #3573

wb1 retry

developer-run-tests-apps-workbench-integration: #3852

developer-run-tests-apps-workbench-integration: #3853

Another bugfix (unhandled error leading to use of nil pointer)

19889-live-log-webdav @ b402b894c535f95cdeba0631dea1fbb9c7672a07 -- developer-run-tests: #3575

LGTM, thanks!