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

If a running container is updated with a portable data hash in its log field, that collection will be propagated to the logs of associated container request(s). This behaivor may be extended to UUID updates in the future.

I'm not sure it fits here to say "if X happens" where X is never supposed to be done other than by a system component. Compare started_at -- "Null if container has not yet started" rather than getting into whether/how it gets updated through the API by a dispatcher. Maybe we should stick with "PDH when log data is available, otherwise null" and leave out the "we might change this API" part? (I know I asked for it earlier but the UUID angle seems less appealing every day.)

That would get rid of "behaivor" too ;)

The code update LGTM.

Tom Clegg wrote in #note-16:

I'm not sure it fits here to say "if X happens" where X is never supposed to be done other than by a system component.

I'll only do one round of this, since it's kind of tangential to the branch, but this kind of opens a philosophical question about who's the audience for the API documentation, yeah? I agree only a dispatcher should be updating this field—which means someone who's writing their own dispatcher might like to know about this behavior in the API server. And we have clear enough boundaries between components you could actually imagine someone doing that.

I'd say the audience of that page is application developers. Arvados developers currently rely on reading the railsapi source code.

Thinking as a non-Arvados-developer, the proposed wording ("if a running container is updated...") makes me wonder "is this suggesting I/anyone can update a container log with a PDH? with a non-PDH? or is this trying to tell me I always need to check the container log because sometimes the CR log field doesn't get auto-updated?"

It might be more productive to enforce "container log must be pdh or nothing" in railsapi because it does not handle UUIDs in a desirable way. Then the documented API is much friendlier: c.log is a PDH and cr.log_uuid has the latest saved logs.

I've updated the documentation in the branch at 7f359d51c3dca6c04003d56f3f18f86cd0916bae. I admit I'm not doing a test run since this new commit just touches one line of documentation, and there aren't any links in that line that could be broken.

I'm personally fine with a Rails API change like you suggest too, but IMO it should be a separate story, mostly because I think we should try to get as much feedback as practical before narrowing the API like that. I don't think that's a change that should get rolled into a branch that was never intended to touch the API server.

Technically it's crunch-run that does the updating so "the dispatcher will update" is perhaps slightly misleading. (Or we could just say "system" if we're trying to focus on what to expect from the system rather than how its parts work.)

Either way LGTM, thanks.