Arvados

This statement needs further clarification. "Work units for container requests should logically combine both the container request and underlying container (if available). This means: It should return and display both fields when the container request and container differ."

The issue with this is, there are properties such as state, created_at, updated_at etc supported by both Container and ContainerRequest object models. We can't simply use "both" in these cases and we need to pick one or the other. Please put an X in the column titled "Use from Container" or "Use from ContainerRequest"

Case 1: For a ContainerWorkUnit (when both Container and ContainerRequest corresponding to it have this property populated)

Case 2: For a ContainerRequestWorkUnit (when both Container and ContainerRequest corresponding to it have this property populated)

Discussed with Peter and have answers to the question in note 6 and others:

• When working with a CR, check if it has a container_uuid and get the C represented by it. For those properties listed in table 6, use values from C if exists, otherwise use values from CR.

• In addition, use values from corresponding C (if exists) for log, output, started_at, finished_at

• For a CR, get children using: CR -> container_uuid -> CRs with that container_uuid as requesting_container_uuid

• Define and reuse container_work_unit for Container and ContainerRequest objects

• In the Container display, include an additional section listing "Container Requests" to which this was the container_uuid (similar to "Used in pipelines" section for a Job display)

• The name and description of a ContainerRequest are editable irrespective of its state.

Branch 9372-container-display at 4a6b74c2 addresses the requirements listed in the description as well as in note 7.

Added tests for testing cancel and description update + tested manually after creating objects at command prompt.

Also addressed #9403.

cbd67fc6b

Peter noticed that in #9318 the dashboard listed containers, but instead it should be listing container_requests. I corrected this in this branch in this commit.

While updating the dashboard test, I noticed that several pipeline_instance fixtures did not have a created_at timestamp and I added it to those fixtures. All tests passed.

I'm not keen on the "Container attribute if non-nil, otherwise ContainerRequest attribute" approach. IMO we should be explicit about the differences between the two. For example, if a ContainerRequest mounts collection uuid=FOO and the corresponding container mounts collection pdh=BAR then both FOO and BAR are interesting pieces of information. Ditto "requested git tree 'master'" vs. "container ran with git tree {sha1}". If a single container satisfies two CRs with different priorities, it should be possible to see the two different priorities. But this is the story we asked for, so let's find out by using it whether it needs "fixing". With that out of the way...

@related seems like it would be clearer if called @container. (It's always a container, right?)

Comment "This applies to a ContainerRequest in Committed or Final state with container_uuid" isn't quite true: a ContainerRequest can have a container_uuid in any state, not just Committed and Final.

Which exceptions is "rescue" meant to swallow here? Shouldn't where(...).first be nil already if there are no matching records? (If the API call fails, presumably we should admit defeat rather than carrying on as if there was no associated container...)

@related = Container.where(uuid: container_uuid).first rescue nil

ContainerWorkUnit#children seems to contain two implementations of "get children for container with UUID X" that are written differently but (afaict) accomplish the same thing. Could we pick one of them, and reduce the conditional part to "where do we find the container UUID to start the search"?

I'm not keen on the "Container attribute if non-nil, otherwise ContainerRequest attribute" approach. IMO we should be explicit about the differences between the two. For example, if a ContainerRequest mounts collection uuid=FOO and the corresponding container mounts collection pdh=BAR then both FOO and BAR are interesting pieces of information. Ditto "requested git tree 'master'" vs. "container ran with git tree {sha1}". If a single container satisfies two CRs with different priorities, it should be possible to see the two different priorities. But this is the story we asked for, so let's find out by using it whether it needs "fixing". With that out of the way...

Yes, Peter also talked about it and said we would need to revisit this after we have some more information.

@related seems like it would be clearer if called @container. (It's always a container, right?)

That is a better name. Renamed.

Comment "This applies to a ContainerRequest in Committed or Final state with container_uuid" isn't quite true: a ContainerRequest can have a container_uuid in any state, not just Committed and Final.

Updated it

Which exceptions is "rescue" meant to swallow here? @related = Container.where(uuid: container_uuid).first rescue nil

Corrected this

ContainerWorkUnit#children seems to contain two implementations of "get children for container with UUID X" that are written differently but (afaict) accomplish the same thing. Could we pick one of them, and reduce the conditional part to "where do we find the container UUID to start the search"?

In case of a container, the children are other containers; and for a CR, the children are other CRs. Except for one SQL used in both conditionals "ContainerRequest.where(requesting_container_uuid: uuid).results", the rest is different. I could move it into a separate reusable method, but I think keeping it as simple and clean as possible will help in understanding this very confusing code on future rereads.

ContainerWorkUnit#children seems to contain two implementations of "get children for container with UUID X" that are written differently but (afaict) accomplish the same thing. Could we pick one of them, and reduce the conditional part to "where do we find the container UUID to start the search"?

In case of a container, the children are other containers; and for a CR, the children are other CRs. Except for one SQL used in both conditionals "ContainerRequest.where(requesting_container_uuid: uuid).results", the rest is different. I could move it into a separate reusable method, but I think keeping it as simple and clean as possible will help in understanding this very confusing code on future rereads.

Ah. Seems to me the children should always be ContainerRequests, though. AFAIK the only reason to show a Container without a corresponding ContainerRequest is that we have no context to tell us which ContainerRequest to show (e.g., admin tools → containers index → click a container that has been used to satisfy multiple CRs). In this case we do know exactly which CR to display, so we should display it...?

Tom said:

Seems to me the children should always be ContainerRequests, though. AFAIK the only reason to show a Container without a corresponding ContainerRequest is that we have no context to tell us which ContainerRequest to show (e.g., admin tools → containers index → click a container that has been used to satisfy multiple CRs). In this case we do know exactly which CR to display, so we should display it...?

Updated Container -> children to consider only ContainerRequests to which this is the requesting_container_uuid. Thanks.

Is "rescue nil" here meant to catch @proxied.nil?, or something else?

"/#{@proxied.class.table_name}/#{uuid}" rescue nil

LGTM, thanks!

Is "rescue nil" here meant to catch @proxied.nil?, or something else?

I used this to support the case where @proxied might be a Hash. Per our conversation, updated to check if @proxied would respond_to table_name instead.