Feature #14323

[API] Accept container mounts that specify both uuid and portable_data_hash

Added by Tom Clegg about 1 year ago. Updated about 1 year ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
API
Target version:
Start date:
11/07/2018
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
1.0
Release:
Release relationship:
Auto

Description

Update API server to match Containers API, specifically
  • "portable_data_hash", "uuid", or both may be provided in a container request.
  • If both are provided, the uuid is considered advisory, and the container uses the provided portable_data_hash.
  • (unchanged) If only the uuid is provided, the container uses the portable data hash corresponding to the given uuid at the time the container is assigned to the container request.

Subtasks

Task #14372: Review 14323-container-collection-mountsResolvedLucas Di Pentima


Related issues

Related to Arvados - Feature #14322: [CWL] Accept collection uuid in inputResolved03/13/2019

Associated revisions

Revision 664cbad5
Added by Lucas Di Pentima about 1 year ago

Merge branch '14323-container-collection-mounts'
Closes #14323

Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <>

History

#1 Updated by Tom Clegg about 1 year ago

  • Related to Feature #14322: [CWL] Accept collection uuid in input added

#2 Updated by Tom Clegg about 1 year ago

  • Description updated (diff)

#3 Updated by Tom Morris about 1 year ago

  • Target version set to 2018-10-31 sprint
  • Story points set to 1.0

#4 Updated by Lucas Di Pentima about 1 year ago

  • Assigned To set to Lucas Di Pentima

#5 Updated by Lucas Di Pentima about 1 year ago

  • Target version changed from 2018-10-31 sprint to 2018-11-14 Sprint

#6 Updated by Lucas Di Pentima about 1 year ago

  • Status changed from New to In Progress

#7 Updated by Lucas Di Pentima about 1 year ago

I have some questions about this story:

1. The fact that the uuid is considered advisory when passing both pdh & uuid means that it should be preserved on the resolved mount instead of being deleted? I ask because there're tests checking that it should be deleted.
2. When both pdh & uuid are passed, should we still have to check that they match, right? There's also a test confirming this.
3. Always resolving to PDH (this time giving priority to PDH when both are given) means that this story is only about the API server behavior change, or is something else needed?

#8 Updated by Lucas Di Pentima about 1 year ago

In the meantime:

Updates at d8a9500ef - branch 14323-container-collection-mounts
Test run: https://ci.curoverse.com/job/developer-run-tests/970/

  • Prioritize pdh over uuid when both provided
  • Don't delete uuid key on resolved mounts (as I understand, if they're advisory data, they shouldn't be deleted)
  • Update tests

#9 Updated by Tom Clegg about 1 year ago

The container request mounts can have UUID, PDH, or both. If both are given, the UUID is ignored (but preserved).

The container mounts ("resolved mounts") never have UUID, always PDH (so the mount.delete 'uuid' line should still be there). Including the UUID would interfere with container reuse.

If a container request's PDH and UUID don't match, we use the PDH anyway. This might be the only actual change here: previously this caused an error, which meant an update race would cause failures for a client that looks up the UUIDs by itself (e.g., to ensure all containers in a workflow use the same PDH for a given UUID) but still wants to preserve the UUIDs as provenance information.

#10 Updated by Lucas Di Pentima about 1 year ago

Updates at 467250f94
Test run: https://ci.curoverse.com/job/developer-run-tests/971/

Following above comments, removed the collection lookup by pdh, and only try getting the collection mount by uuid when the pdh is not provided. Updated tests.

#11 Updated by Tom Clegg about 1 year ago

Indentation mishap in container_request_test.rb.

Rest LGTM, thanks!

#13 Updated by Tom Clegg about 1 year ago

Looks like this restores the possibility of a container mount with no UUID and no PDH. This means "start with empty collection" according to the Containers API spec. Seems like we need a test for this in the API suite, rather than relying on WB integration tests.

With that, LGTM.

#14 Updated by Lucas Di Pentima about 1 year ago

Added test at 2480ed0b8, testing one more time before merging: https://ci.curoverse.com/job/developer-run-tests/975/

#15 Updated by Lucas Di Pentima about 1 year ago

  • Status changed from In Progress to Resolved

#16 Updated by Tom Morris about 1 year ago

  • Release set to 14

Also available in: Atom PDF