Feature #10291

[Crunch2] Config to remove APIs from the discovery document -- use this in cwl-runner to choose between containers/jobs

Added by Peter Amstutz almost 5 years ago. Updated almost 5 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
API
Target version:
Start date:
10/19/2016
Due date:
% Done:

100%

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

Subtasks

Task #10302: Review 10291-really-disable-apisResolvedRadhika Chippada


Related issues

Related to Arvados - Bug #10310: [Workbench] Do not offer to use CrunchV1 features if they are not enabledResolved10/20/2016

Associated revisions

Revision 81d1a618
Added by Tom Clegg almost 5 years ago

Merge branch '10291-discovery-blacklist' refs #10291

Revision d5524135
Added by Tom Clegg almost 5 years ago

Merge branch '10291-really-disable-apis' closes #10291

Revision 182dd352 (diff)
Added by Tom Clegg almost 5 years ago

10291: Update --help message. refs #10291

History

#1 Updated by Peter Amstutz almost 5 years ago

  • Subject changed from [Crunchv2] Discovery document specifies whether to use jobs or containers API to [Crunch2] Discovery document specifies whether to use jobs or containers API

#2 Updated by Peter Amstutz almost 5 years ago

Currently, arvados-cwl-runner defaults to the crunch v1 jobs API. This means sites using crunch v2 need to explicitly add --api=containers.

arvados-cwl-runner should be able to discover from the discovery document whether to prefer the jobs or containers API.

#3 Updated by Tom Morris almost 5 years ago

  • Story points set to 1.0

Add a config option which specifies whether the Jobs API is supported for the installation.
Blacklist filter applied to discovery document to disable create call.
arvados-cwl-runner introspects discovery document to decide which API to use.

#4 Updated by Tom Morris almost 5 years ago

  • Target version set to 2016-11-09 sprint

#5 Updated by Tom Clegg almost 5 years ago

  • Subject changed from [Crunch2] Discovery document specifies whether to use jobs or containers API to [Crunch2] Config to remove APIs from the discovery document -- use this in cwl-runner to choose between containers/jobs
  • Category set to Crunch

#6 Updated by Tom Clegg almost 5 years ago

  • Category changed from Crunch to API
  • Status changed from New to In Progress
  • Assigned To set to Tom Clegg

#7 Updated by Tom Clegg almost 5 years ago

10291-discovery-blacklist

test 6bf3b279cd2d5ea5c1d6b849265b7206461467b8

#8 Updated by Tom Clegg almost 5 years ago

  • Target version changed from 2016-11-09 sprint to 2016-10-26 sprint

#9 Updated by Radhika Chippada almost 5 years ago

Reviewing at 6bf3b27

  • Can you please include an example with multiple methods in the config yml (and expand the test to check)?
      disable_api_methods: []
      Example: ["jobs.create"]
    
  • init.py: It appears that “jobs” would be used if no “api” is specified? Do we change the order in “for api in ["jobs", "containers”]” so that “containers” is checked first?
    • BTW, if I were to do this, only 3 tests seem to get unhappy
  • Do we have any tests with "containers" api used? (On looking at the diff, I felt they all were using jobs api)

#10 Updated by Tom Clegg almost 5 years ago

Radhika Chippada wrote:

  • Can you please include an example with multiple methods in the config yml (and expand the test to check)?

Added

  • init.py: It appears that “jobs” would be used if no “api” is specified? Do we change the order in “for api in ["jobs", "containers”]” so that “containers” is checked first?
    • BTW, if I were to do this, only 3 tests seem to get unhappy

At least for now, we still want to use jobs by default if both APIs are available.

  • Do we have any tests with "containers" api used? (On looking at the diff, I felt they all were using jobs api)

That sounds like a good idea but I think it's out of scope here. tests.test_job.TestWorkflow.test_default_work_api tests the added behavior (i.e., the default changes to "containers" if "jobs" is disabled).

Updated API to return 404 for disabled APIs → 10291-really-disable-apis, test cfa2c529c7c8105e4d424b404db2c48ff2066e27

#11 Updated by Radhika Chippada almost 5 years ago

Updated API to return 404 for disabled APIs

That's better. LGTM

#12 Updated by Tom Clegg almost 5 years ago

  • Status changed from In Progress to Resolved

Applied in changeset arvados|commit:d5524135b1495b919de332df4f952926664961f5.

#13 Updated by Tom Morris almost 5 years ago

In thinking about the wide ranging impact of obsoleting a core API and introducing an incompatible API to replace it, I'm wondering if this is solving the wrong problem.

How much easier would it make our life if we just committed to making the new API backward compatible with the old? Is there a reason that wouldn't be the best solution to this problem?

#14 Updated by Peter Amstutz almost 5 years ago

Tom Morris wrote:

In thinking about the wide ranging impact of obsoleting a core API and introducing an incompatible API to replace it, I'm wondering if this is solving the wrong problem.

How much easier would it make our life if we just committed to making the new API backward compatible with the old? Is there a reason that wouldn't be the best solution to this problem?

The semantics of Crunch v2 are significantly different from Crunch v1 and they have non-overlapping behaviors such that neither one is a subset of the other. We could reimplement the crunch v1 'jobs' API using the crunch v2 API in order to support existing crunch scripts, but this requires filling in for crunch v1 behavior that doesn't exist in crunch v2, which is more work, not less.

We are using CWL as the bridge from crunch v1 to crunch v2. The branch in this ticket is in line with this strategy, by making arvados-cwl-runner automatically determine the appropriate API to submit to.

#15 Updated by Tom Clegg almost 5 years ago

A new implementation of the crunch1 API using crunch2 under the hood is surely a good feature.

However, that isn't expected to happen right away, and in the meantime it seems better if the API server doesn't offer to do things it can't do...

Also available in: Atom PDF