Bug #10310

[Workbench] Do not offer to use CrunchV1 features if they are not enabled

Added by Nico César about 5 years ago. Updated almost 5 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Radhika Chippada
Category:
Workbench
Target version:
Start date:
10/20/2016
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
0.0

Description

Running pipelines in "containers" API only cluster makes everything confusing when the pipeline is stuck without running.

It will be better to have rejection from the API when trying to create them, also a flag in workbench to hide "Run Pipeline" or the warning to the user to do the CrunchV1 tutorial.


Subtasks

Task #10372: Review branch 10310-crunch2-workbenchResolvedRadhika Chippada


Related issues

Related to Arvados - Feature #10291: [Crunch2] Config to remove APIs from the discovery document -- use this in cwl-runner to choose between containers/jobsResolved10/19/2016

Related to Arvados - Feature #10518: [Workbench] Further cosmetic cleanup in crunchV2 only scenarioResolved11/29/2016

Associated revisions

Revision d0bc46ce
Added by Radhika Chippada almost 5 years ago

closes #10310
Merge branch '10310-crunch2-workbench'

History

#1 Updated by Tom Clegg about 5 years ago

10310-workbench-disable-apis @ 2ee3d94756197297876afb64b7d5bc2789419aaf https://ci.curoverse.com/job/developer-run-tests/41/

This updates ArvadosBase.creatable? to check the discovery doc. I expect there are also some (more) buttons we'll need to wrap in "if PipelineInstance.creatable?" etc.

#2 Updated by Tom Clegg about 5 years ago

  • Status changed from New to In Progress

#3 Updated by Tom Clegg almost 5 years ago

  • Subject changed from [API] add a flag to reject CrunchV1 pipelines if they arent available to [Workbench] Do not offer to use CrunchV1 features if they are not enabled
  • Category set to Workbench

#4 Updated by Tom Morris almost 5 years ago

  • Status changed from In Progress to New
  • Assigned To set to Radhika Chippada
  • Target version set to 2016-11-09 sprint
  • Story points set to 2.0

#5 Updated by Radhika Chippada almost 5 years ago

  • Status changed from New to In Progress

#6 Updated by Radhika Chippada almost 5 years ago

I think we need to make the following updates to workbench in this context:

  • On the dashboard:
    • “Run a pipeline” should not include pipelines in the model
    • “All processes” should not include pipelines
    • “Recent pipelines and processes” should not include pipelines
  • In project#show
    • Do not include pipeline instances in “pipelines and processes” tab
    • Do not include pipeline_templates in “pipeline templates” tab
    • Do not include pipelines in “Run a pipeline” action
  • Collection “used by” tab should not include jobs and pipeline instances
  • Show error page saying "Disabled" for
    • /pipeline_instances
    • /pipeline_instances/{uuid}
    • /pipeline_templates
    • /pipeline_templates/{uuid}
    • /jobs
    • /jobs/{uuid}
    • /job_tasks
    • /job_tasks/{uuid}
  • Cosmetic issues we will not be addressing in this story:
    • In dashboard: button label would be “Run a pipeline” when we will only be offering workflows
    • In project#show:
      • tab “pipelines and processes” would only be listing “processes
      • tab “pipeline templates” would only be listing workflows

#7 Updated by Tom Clegg almost 5 years ago

Radhika Chippada wrote:

I think we need to make the following updates to workbench in this context:

  • On the dashboard:
    • “Run a pipeline” should not include pipelines in the model
    • “All processes” should not include pipelines
    • “Recent pipelines and processes” should not include pipelines
There won't be any pipeline templates or instances, so I think the only requirements here are
  • Workbench doesn't use jobs/pipelines APIs that are disabled
  • Workbench doesn't make extra API calls that will always return zero results (e.g., groups#contents with filters=[["uuid","is_a","arvados#job"]])

(We'll be doing the "migrating from jobs to containers" story soon enough, so if we can easily fix both cases now, we might as well do that.)

  • In project#show
    • Do not include pipeline instances in “pipelines and processes” tab
    • Do not include pipeline_templates in “pipeline templates” tab
    • Do not include pipelines in “Run a pipeline” action

ditto

  • Collection “used by” tab should not include jobs and pipeline instances

ditto

  • Show error page saying "Disabled" for
    • /pipeline_instances
    • /pipeline_instances/{uuid}
    • /pipeline_templates
    • /pipeline_templates/{uuid}
    • /jobs
    • /jobs/{uuid}
    • /job_tasks
    • /job_tasks/{uuid}

Rather than choosing a specific list of pages that behave properly when their APIs are disabled, can we do this for all pages?

Perhaps something like before_filter :ensure_arvados_api_exists, only: [:index, :show] in ApplicationController?

  • Cosmetic issues we will not be addressing in this story:
    • In dashboard: button label would be “Run a pipeline” when we will only be offering workflows
    • In project#show:
      • tab “pipelines and processes” would only be listing “processes
      • tab “pipeline templates” would only be listing workflows

I think it might be worth renaming "pipeline templates" to "workflows": unlike "pipeline" (a familiar term from outside Arvados), "pipeline template" is a crunch1-specific term that really doesn't belong here any more.

#8 Updated by Radhika Chippada almost 5 years ago

Branch 10310-workbench-disable-apis at 25250c3 is ready for review

  • This version checks if the discovery document offers the index or show methods on the pipeline_instances, pipeline_templates, jobs and job_tasks and skips fetching those objects when fetching "all_proceses", "rennet_processes", "pipelines and processes" etc.
  • For the /index and show requests, we now display a 404 error page. Please clarify if we prefer some other error code here
  • Tom said: "Rather than choosing a specific list of pages that behave properly when their APIs are disabled, can we do this for all pages? Perhaps something like before_filter :ensure_arvados_api_exists, only: [:index, :show] in ApplicationController?"

Yes, I was considering using a filter. However, the filter still needed to consider those listed 4 controllers only since workbench has other controller such as projects_controller, work_units_controller etc.

  • Tom said: "I think it might be worth renaming "pipeline templates" to "workflows": unlike "pipeline" (a familiar term from outside Arvados), "pipeline template" is a crunch1-specific term that really doesn't belong here any more."

Agree. I renamed this tab

  • I had difficulty checking the discovery document in notifications and hence used an exception catch handle this: workbench application_controller, line 780
  • I could not find a way to "modify" the discovery document from within a workbench test and hence I did not write the one controller test I was planning on writing. (This test was going to modify the discovery document and then fetch "recent_processes" and verify that no pipelines were listed). Tested this manually though

All tests passed at https://ci.curoverse.com/job/developer-run-tests/54/

#9 Updated by Tom Clegg almost 5 years ago

I've pushed 697f507 with a unit test that simulates disabled APIs by mocking the discovery doc. Perhaps we can use this approach to add a functional test too, e.g., to get some coverage for the notification check?

Could we make use of the existing ArvadosBase.api_exists? method instead of copying it? E.g.,

def ensure_arvados_api_exists
  if model_class < ArvadosBase && !model_class.api_exists?(action_name.to_s)
    ...
    return render_error(...)
  end
end

difficulty checking the discovery document in notifications

Any details available on this? (Maybe the above functional test idea will help?) I would expect something like this to work:

if !PipelineInstance.api_exists?(:index)
  return nil
end

I don't think we should be renaming the "Pipeline templates" tab to "Workflows" on systems where crunch1 is still in use (and is therefore the default).

#10 Updated by Radhika Chippada almost 5 years ago

Tom said:

I've pushed 697f507 with a unit test that simulates disabled APIs by mocking the discovery doc ...

This helped. I added several controller tests using this. Thanks.

Could we make use of the existing ArvadosBase.api_exists? method instead of copying it?

Updated the exists check in controller to use this method. The controller still needs logic to ensure only a subset of controllers go through logic to bypass projects_controller and work_units_controller etc

Notifications

Updated the notifications to use PipelineInstance.api_exists?

I don't think we should be renaming the "Pipeline templates" tab to "Workflows" on systems where crunch1 is still in use (and is therefore the default).

I reverted that update. The tab now uses the name "Pipeline templates" or "Workflows" depending on the environment in which it is running.

Now at b3883b in branch 10310-crunch2-only-wb

All tests passed at https://ci.curoverse.com/job/developer-run-tests/60/

#11 Updated by Tom Clegg almost 5 years ago

Radhika Chippada wrote:

Updated the exists check in controller to use this method. The controller still needs logic to ensure only a subset of controllers go through logic to bypass projects_controller and work_units_controller etc

I thought the "if model_class < ArvadosBase" bit would do that in a more generic way... does that not work?

Although in this context we're only thinking about disabling Crunch1 APIs, it's a generic feature up to this point in the stack so it seems to me we might as well keep it generic. E.g., if the API config disables the groups.list API, we know the workbench /groups/ and /projects/ pages won't work -- surely we want them to say 404 instead of Fiddlesticks, for the same reason we want that here. IOW, if the feature works in all cases, why restrict it to a few specific cases?

Unlike params['controller'].to_sym.to_s.classify.constantize, model_class already knows how to map UserAgreements and Projects to Collections and Groups, and even selects the right type for the given UUID in ActionsController.

test "#{ctrl_name} index page with crunch2" should
  • be called "#{ctrl_name} index page when API is disabled"
  • assert_response 404

The first test in disabled_api_test.rb looks like it would be simpler if split into two tests, rather than a loop with multiple "if crunch2" blocks inside. Also, as a general rule, we should use a regular user ("active") for tests unless there's some reason to test with admin privileges...

#12 Updated by Radhika Chippada almost 5 years ago

Now at b82628c in branch 10310-crunch2-workbench (I had accidentally merged Peter's branch into my branch this morning, and now yet another branch)

Tom said:

I thought the "if model_class < ArvadosBase" bit would do that in a more generic way... does that not work?

Updated this. I updated all previous "exists" checks to work off of the arvados_base exists method which made it possible for me to get it working this way.

test "#{ctrl_name} index page with crunch2" should be called "#{ctrl_name} index page when API is disabled"

Updated

assert_response 404

Updated

The first test in disabled_api_test.rb looks like it would be simpler if split into two tests, rather than a loop with multiple "if crunch2" blocks inside.

I removed the "crunch1 enabled" test scenario from here since it is covered in other tests. Made this test only focus on Crunch2 case

Also, as a general rule, we should use a regular user ("active") for tests unless there's some reason to test with admin privileges..

You are right. I was planning on correcting this and missed it. Updated

Test run @ https://ci.curoverse.com/job/developer-run-tests/63/ (Fixed to address the one failing test)

Thanks.

#13 Updated by Radhika Chippada almost 5 years ago

  • Target version changed from 2016-11-09 sprint to 2016-11-23 sprint
  • Story points changed from 2.0 to 0.0

#14 Updated by Tom Clegg almost 5 years ago

LGTM, thanks!

#15 Updated by Radhika Chippada almost 5 years ago

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

Applied in changeset arvados|commit:d0bc46ce842e7fdb71c25cc32caef0afd209c9ec.

Also available in: Atom PDF