Arvados

f137e22a65cc29d00146a4735fe54d72b299984b

Complete test suite being run @ https://ci.curoverse.com/job/developer-test-job/191/

Container reuse according to the described criteria.
Tests written to cover the cases when several candidates exist for a single request.
Corrected some preexisting test to be compatible with this new behaviour.

• As Tom clarified in IRC, please split the big SQL query in find_reusable into a few as below
  • For state “Complete” : “select state=Complete group by output limit 2”. If you get 2, it means we have two different outputs and we do not want to reuse either. If you got just 1, do another query “select where state=Complete order by finished_at limit 1” and use this oldest record.
  • If no containers in Complete state (none or can’t reuse), get all containers in state = Running order by progress desc, started_at asc limit 1 and use it
  • Otherwise, get containers in state [Locked, Queued] order by state, priority desc, created_at asc, limit 1 and use it

• It appears that the deep_sort_hash is copied over from job.rb. Please consider moving it to arvados_base from job instead of copying

• Please remove output from other containers fixtures that are in non-Complete state

• container_test.rb
  • You do not need to_json in these asserts in test “assert_equal c.environment.to_json, Container.deep_sort_hash(env).to_json”
  • Progress is between 0.0 to 1.0, so it would be nice if you change progress 15.0 to 0.15 etc
  • The test “find_reusable method should select running container most likely to finish sooner” has same progress for both first and second. Can you please add a test where progress is higher and that it gets selected
  • Can you add another test like “find_reusable method should not select failed container” where there is one failed and one running (or any other state you like) and verify that the other gets selected?

• Ideally these new tests in container_test.rb could have been in container_request_test.rb so that we can see the reuse logic working as desired. Can you at least add a couple tests in container_request_test such that:
  • CR1 in Committed state; CR2 in Uncommitted state + assert container_uuid is nil; update CR2 to Committed state and assert CR2.container_uuid == CR1.container_uuid
  • CR1 in Committed state; CR2 with slightly different attributes in Uncommitted state + assert container_uuid is nil; update CR2 to Committed state and assert CR2.container_uuid != CR1.container_uuid

Previous corrections addressed at a02b4f41cb495ecfea49ac0da1f6da0aec385f7d

Added an additional test to check that no completed container is selected if there are more than 1 different outputs.

Regarding the deep_sort_hash(env).to_json comment: If we just compare hashes and not serialized hashes, the assert will be successful even if the hashes are sorted differently, that's why they're serialized on the assertion.

Complete test suite being run @ https://ci.curoverse.com/job/developer-test-job/193/

at a02b4f4

• find_reusable
  • The "completed" SQL is pretty awesome :)
  • The completed.order should be “finished_at asc” so that we fetch the “oldest” completed CR
  • You can please accomplish the “Locked” and “Queued” state handling in one query using order by state asc

• In test "Container request #{(env1 == env2) ? 'does' : 'does not'} reuse container when committed" , please add a not_nil assert on cr1.container_uuid at line 416 (or check cr2.container_uuid is not_nil after line 422)

• In test "find_reusable method should not select completed container when inconsistent outputs exist”, you can call c_older and c_rerent as c_output1 and c_output2 instead

• Nit: If you don’t mind, please replace the “winner” with “selected or chosen container” in the tests?

Thanks.

Comments addressed at 3c69428
Thanks for the compliment! :)

LGTM