Arvados

Hm. The documentation says the container log will be null or a UUID until the container is finished. The existing updateLogs loop periodically updates it to a PDH. And this branch updates it to a UUID during the Locked→Running transition.

In RailsAPI, Container#update_cr_logs calls ContainerRequest#update_collections, which assumes the log is a PDH.

I think the easy solution is for the new code to save a PDH instead of UUID, and update the documentation to say "...otherwise a PDH, UUID, or null", and a better solution (unless I'm missing some reason this won't work) is to update RailsAPI and crunch-run to do what the current documentation says.

Other than that, this LGTM, thanks.

Tom Clegg wrote in #note-11:

I think the easy solution is for the new code to save a PDH instead of UUID, and update the documentation to say "...otherwise a PDH, UUID, or null", and a better solution (unless I'm missing some reason this won't work) is to update RailsAPI and crunch-run to do what the current documentation says.

I'm not sure I understand why the second option is better? ContainerRequest#update_collections ensures that every request has its own log collection. Assuming we maintain that behavior, I'm not sure what the "log is a UUID before container is finished" rule is intended to accomplish for us, in terms of implementation flexibility or what.

Without more background, it seems to me like the best option that minimizes work while maximizing consistency is to update crunch-run to always send a PDH, and update the documentation to say that the log will always be a PDH or null, perhaps with a note that this restriction may be relaxed to allow UUIDs in the future. Is there some reason I'm missing that's not a good idea?

I'm not sure I understand why the second option is better?

I was thinking it would be better in that it would allow a client to get the log collection UUID and then watch that collection for updates, rather than having to use a special-to-container-logs approach, and would eliminate the "log pdh is now X" API call after saving the collection. Practically speaking neither seems like a huge deal though.

Without more background, it seems to me like the best option that minimizes work while maximizing consistency is to update crunch-run to always send a PDH, and update the documentation to say that the log will always be a PDH or null, perhaps with a note that this restriction may be relaxed to allow UUIDs in the future. Is there some reason I'm missing that's not a good idea?

I agree this is what we should do for now, and maybe we'll revisit the UUID option in the future.

Tom Clegg wrote in #note-14:

I'm not sure I understand why the second option is better?

I was thinking it would be better in that it would allow a client to get the log collection UUID and then watch that collection for updates, rather than having to use a special-to-container-logs approach

Isn't this provided by the log propagation the API server does though? When a container request is updated with a container's new log, it makes a point to load and update its existing collection. Following updates for a specific container request's logs seems like it should just work. I realize you said "container logs" and I'm saying "container request logs" and those aren't the same thing, but practically is there any reason for clients to prefer reading the container logs directly over the container request logs?

I agree this is what we should do for now, and maybe we'll revisit the UUID option in the future.

19886-crunch-run-early-log-commit @ 6477db8a1f5ed0b2f81cf743bbea32c681c7c10c - developer-run-tests: #3449