Feature #8470

[Crunch2] [API] Use exact values for runtime_constraints and mounts in Container records

Added by Peter Amstutz over 4 years ago. Updated about 4 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
Crunch
Target version:
Start date:
06/23/2016
Due date:
% Done:

100%

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

Description

Currently, the API satisfies a Container Request by copying its attributes into a new Container.

However, a Container record is meant to specify a container unambiguously, while a Container Request describes a set of acceptable Containers (e.g., "between 1 and 2 GB of RAM") and uses identifiers like UUIDs and git branch names, which resolve to different values at different times.

New behavior:
  • If cr.container_image is not already a collection PDH, resolve it (as a docker tag or a collection UUID) to a PDH. Use the resulting PDH as the container_image for the new Container.
  • If any entries in cr.mounts specify collection UUIDs, look them up and specify them as PDH instead in the new Container.
  • If any entries in runtime_constraints specify ranges (e.g., "vcpus":[2,null] signifying "at least two cores"), use the minimum (2 in this case) as the value for the corresponding key in the new Container's runtime_constraints.

Subtasks

Task #9477: Use numbers for runtime constraintsResolvedTom Clegg

Task #9478: Resolve collections to PDHResolvedTom Clegg

Task #9479: Resolve docker image tag to PDHResolvedTom Clegg

Task #9480: Review 8470-resolve-container-reqResolvedRadhika Chippada


Related issues

Related to Arvados - Story #9273: [Crunch2] Do not satisfy container requests that use non-content-address names for docker images, git commits, or collectionsClosed

Associated revisions

Revision 01ea0e9f
Added by Tom Clegg about 4 years ago

Merge branch '8470-resolve-container-req'

closes #8470

History

#1 Updated by Peter Amstutz over 4 years ago

  • Subject changed from [Crunch2] API resolves Docker, API, collection uuids to content addresses in container request to [Crunch2] API resolves Docker, git, collection uuids to content addresses in container request

#2 Updated by Peter Amstutz over 4 years ago

Should also include satisfying defaults such as RAM and CPUs.

#3 Updated by Tom Clegg about 4 years ago

  • Category set to Crunch
  • Assigned To set to Tom Clegg

#4 Updated by Tom Clegg about 4 years ago

  • Subject changed from [Crunch2] API resolves Docker, git, collection uuids to content addresses in container request to [Crunch2] [API] Use exact values for runtime_constraints and mounts in Container records
  • Description updated (diff)

#5 Updated by Tom Clegg about 4 years ago

  • Target version set to 2016-07-06 sprint

#6 Updated by Tom Clegg about 4 years ago

  • Story points set to 1.0

#7 Updated by Tom Clegg about 4 years ago

  • Status changed from New to In Progress

#8 Updated by Tom Clegg about 4 years ago

8470-resolve-container-req @ be4ca15

#9 Updated by Radhika Chippada about 4 years ago

container_request.rb

  • In resolve method
    • the comment “TODO: resolve container_image to a content address.” seems to have been addressed in container_image_for_container method?
    • If it is not too much to ask, in “Container.create!(command: self.command,” can you place all self.X items first and then the derived ones to improve readability of code?
  • In mounts_for_container, line 139, can we say “mount #{uuid} not found” instead of “collection #{uuid} …”? Also, “uuid #{uuid} has portable_data_hash …”, can you say “mount uuid #{uuid} has …”?

container_request_test.rb

  • In ‘test "Container request max priority" do’ , please add a comment explaining why cr2 priority is expected.
  • In ‘test “Container makes container request, then is cancelled" do’ :
    • Add an assertion to check priority on cr after line 223 (after cancel)
    • I think you can use create_minimal_req! for the first cr creation
    • Can you please add a comment in this or in test “container cancelled finalizes request” or in code listing canceling what cancels what. I think a comment listing the relationships between Cs and CRs and Requesting_Cs and Container_uuids would be quite helpful.
  • “resolve runtime constraint range #{mounts} to values” => mounts is not from runtime constraints, correct? should the name of this test be something like “resolve mounts to values …”?

#10 Updated by Tom Clegg about 4 years ago

Radhika Chippada wrote:

  • the comment “TODO: resolve container_image to a content address.” seems to have been addressed in container_image_for_container method?

indeed. removed.

  • If it is not too much to ask, in “Container.create!(command: self.command,” can you place all self.X items first and then the derived ones to improve readability of code?

done

  • In mounts_for_container, line 139, can we say “mount #{uuid} not found” instead of “collection #{uuid} …”? Also, “uuid #{uuid} has portable_data_hash …”, can you say “mount uuid #{uuid} has …”?

improved error messages

  • In ‘test "Container request max priority" do’ , please add a comment explaining why cr2 priority is expected.

done

  • In ‘test “Container makes container request, then is cancelled" do’ :
    • Add an assertion to check priority on cr after line 223 (after cancel)
    • I think you can use create_minimal_req! for the first cr creation
    • Can you please add a comment in this or in test “container cancelled finalizes request” or in code listing canceling what cancels what. I think a comment listing the relationships between Cs and CRs and Requesting_Cs and Container_uuids would be quite helpful.

fixed up some comments here

  • “resolve runtime constraint range #{mounts} to values” => mounts is not from runtime constraints, correct? should the name of this test be something like “resolve mounts to values …”?

whoops, yes, fixed... thanks

#11 Updated by Tom Clegg about 4 years ago

  • Status changed from In Progress to Resolved
  • % Done changed from 75 to 100

Applied in changeset arvados|commit:01ea0e9faa0b29ef747699f7f4b728d4e888ef83.

Also available in: Atom PDF