Currently it's an error to set delete_at to a time earlier than blob_signature_ttl. Some time passes between when the container request chooses a delete_at and when the collection validation checks whether delete_at is valid. This means the container request code has to add a few seconds of grace period to avoid failing during slow times. Perhaps it would be better to change the rule so, if a client sets delete_at to a value too soon to achieve safely, it automatically gets extended to the earliest possible time?

• Expiring collections
• http://doc.arvados.org/api/methods/collections.html

11100-cr-output-ttl @ 4437774e863465c0daa41dfd9716174e18d93122

Tom Clegg wrote:

Currently it's an error to set delete_at to a time earlier than blob_signature_ttl. Some time passes between when the container request chooses a delete_at and when the collection validation checks whether delete_at is valid. This means the container request code has to add a few seconds of grace period to avoid failing during slow times. Perhaps it would be better to change the rule so, if a client sets delete_at to a value too soon to achieve safely, it automatically gets extended to the earliest possible time?

So instead of a validation error, it would just adjust it to a valid value. Yes.

With "earliest possible delete_at":

11100-cr-output-ttl @ 65121f8db54a1ed15207d050e1f48c5fc26d646b

Documentation of the output_ttl should specify units.

Should the code to adjust delete_at to the earliest valid time be performed before_validation so that the actual validation doesn't modify the record? (Not sure if rails is opinionated about validations being pure). (default_trash_interval does something similar and is in before_validation).

Rest LGTM

Peter Amstutz wrote:

Documentation of the output_ttl should specify units.

Indeed. Got it in Containers API but missed it in docs. Fixed.

Should the code to adjust delete_at to the earliest valid time be performed before_validation so that the actual validation doesn't modify the record? (Not sure if rails is opinionated about validations being pure). (default_trash_interval does something similar and is in before_validation).

Not sure about Rails either, but yes, that seems neater. Fixed.

Also added a bit to acknowledge that no tokens will expire before the previous (hence previously validated) value of delete_at.

11100-cr-output-ttl @ ff3bb22d4b2bff5666907a6eeb6cd68cd3cbe22b

Reviewed 8645888f1 - branch 11100-cwl-set-output-ttl

Some minor comments:

• File doc/user/cwl/cwl-extensions.html.textile.liquid:L86 - duplicated ‘before’. Also s/recommend/recommended/ ?
• File sdk/cwl/arvados_cwl/arv-cwl-schema.yml:L145 - Same as previous: s/recommend/recommended/
• If there’s no way to ensure that an intermediate output collection doesn’t get trashed while a step depending on it is running, maybe we should add a log warning when every intermediate output is created with a short TTL, for future debugging purposes. Didn’t check if this is done on the API serve side.

Apart from that, LGTM. Local tests ran ok.

I decided to add --trash-intermediate/--no-trash-intermediate as distinct options from --intermediate-output-ttl.

Updated documentation.

Not really sure about logging "TTL too short". We could pick some arbitrary number (60 seconds? 3600 seconds?) but it is highly workflow dependent.

Now at bee4fa82c15b2188c2105fff3b52c305b38f04a6

• File sdk/cwl/arvados_cwl/arvcontainer.py:L178 - Should that check be also on the arvados-cwl-runner side?
• Do you think it would be useful to add a test checking the new argument --trash-intermediate and its default behavior?
• Instead of logging when output TTL is too short, how about logging a message noticing when creating an output collection with the ttl? (I believe this would be on the api server side, right?).

Side note: I found a typo (Aravdos) on doc/user/cwl/cwl-runner.html.textile.liquid:129, maybe you can add that fix to the other doc updates.

Lucas Di Pentima wrote:

• File sdk/cwl/arvados_cwl/arvcontainer.py:L178 - Should that check be also on the arvados-cwl-runner side?

Done.

• Do you think it would be useful to add a test checking the new argument --trash-intermediate and its default behavior?

Yes, as it turned out I actually did forget to propagate it to the runner command line. Test added, and code fixed.

• Instead of logging when output TTL is too short, how about logging a message noticing when creating an output collection with the ttl? (I believe this would be on the api server side, right?).

Added logging CWL about intermediate outputs.

Side note: I found a typo (Aravdos) on doc/user/cwl/cwl-runner.html.textile.liquid:129, maybe you can add that fix to the other doc updates.

Fixed.

Now @ 5223e68fa0d1f1fc52c7285fa6844d7932ba9acb

Please check the tests, there's one failing on me, at least when running locally:

======================================================================
FAIL: test_done (tests.test_container.TestContainer)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/lucas/arvados_local/tmp/VENVDIR/local/lib/python2.7/site-packages/mock/mock.py", line 1305, in patched
    return func(*args, **keywargs)
  File "/home/lucas/arvados_local/sdk/cwl/tests/test_container.py", line 415, in test_done
    arvjob.collect_outputs.assert_called_with("keep:abc+123")
  File "/home/lucas/arvados_local/tmp/VENVDIR/local/lib/python2.7/site-packages/mock/mock.py", line 925, in assert_called_with
    raise AssertionError('Expected call: %s\nNot called' % (expected,))
AssertionError: Expected call: mock('keep:abc+123')
Not called

----------------------------------------------------------------------

LGTM, thanks!