Arvados

We should consider undoing or narrowing the reuse changes we made in #20606 after we implement this. If Arvados gets better about retrying, then odds go up that the reuse narrowing is more likely to be wasteful than helpful.

19982-spot-instance @ 4370bab5da1c4ed4d7100d3d344749166a2982f4

developer-run-tests: #4365

• All agreed upon points are implemented / addressed.
  • yes
• Anything not implemented (discovered or discussed during work) has a follow-up story.
  • n/a
• Code is tested and passing, both automated and manual, what manual testing was done is described
  • added new unit tests.
• Documentation has been updated.
  • added to list of CWL extensions
• Behaves appropriately at the intended scale (describe intended scale).
  • no effect on scale
• Considered backwards and forwards compatibility issues between client and server.
  • if preemptionNotice is not set, has no effect.
• Follows our coding standards and GUI style guidelines.
  • yes

Checks for the presence of a non-empty container["runtime_status"]["preemptionNotice"]. If so, and feature is enabled, resubmits with preemptible = false to increase the likelihood of making progress when the spot market has limited capacity.

Because I was already changing code related to re-submitting containers, I also made the changes noted in #21413 for out-of-memory retry. The most significant change is deleting the failed attempts. Users were confused by having failed steps listed when the workflow ran successfully. On the other hand, it could complicate debugging -- although it does (a) log the container request uuid and (b) doesn't delete the collection with the logs of the failed step, so that might be sufficient.

19982-spot-instance @ 1d756458d601d70b56d8eac0d8d4387569092d68

developer-run-tests: #4366

Doc updates

Some comments:

• File sdk/cwl/tests/test_container.py
  • Line 1342: I think it would make test cases a lot easier to follow if the expected result is included as a parameter instead of being calculated on the fly. I know repeating text is not tidy but in this case I think improves readability and makes it clear what the test author's intentions were.
  • Line 1465: There's some commented out code
• File sdk/cwl/arvados_cwl/arvcontainer.py Line 8: I believe this is an unused import
• Re: deleting failed-and-retried containers: I think users' opinion that's confusing are valid, but I it seems to me that that's just a UI problem, not a backend problem. I'm thinking for example that keeping the failed preempted attempts are useful for cost accounting and also as a data point to make decisions on suitability of preemptible instances over time.
• I think we also need the new a-c-r flags to be documented in doc/user/cwl/cwl-run-options.html.textile.liquid

Lucas Di Pentima wrote in #note-50:

Some comments:

• File sdk/cwl/tests/test_container.py
  • Line 1342: I think it would make test cases a lot easier to follow if the expected result is included as a parameter instead of being calculated on the fly. I know repeating text is not tidy but in this case I think improves readability and makes it clear what the test author's intentions were.

Done.

• Line 1465: There's some commented out code

I removed all the commented-out arv_docker_clear_cache

• File sdk/cwl/arvados_cwl/arvcontainer.py Line 8: I believe this is an unused import

Yea, I don't know where that came from.

• I think we also need the new a-c-r flags to be documented in doc/user/cwl/cwl-run-options.html.textile.liquid

Done.

• Re: deleting failed-and-retried containers: I think users' opinion that's confusing are valid, but I it seems to me that that's just a UI problem, not a backend problem. I'm thinking for example that keeping the failed preempted attempts are useful for cost accounting and also as a data point to make decisions on suitability of preemptible instances over time.

Let's talk about this at standup

19982-spot-instance @ 16a847f510a6b81520a942622ad8ffd38a9cc68f

developer-run-tests: #4388

19982-spot-instance @ 16cf7bf16464fbf01360dea4e07859d1256a8990

developer-run-tests: #4394

Now sets "arv:failed_container_resubmitted" on the old container request.

Updated test & documentation.

This LGTM, thanks!