Arvados

give some indication of how busy the cluster is, such as number of nodes busy/idle

Review feedback for branch 3605-node-info-field:

Just a couple very minor comments:

• apps/workbench/app/views/application/_arvados_attr_value.html.erb
  Just an observation. I tried to fix this in the past and Tom suggested that the user's of this should not send null attr parameter :).

• Is it possible to test the newly added api accessible info attribute? May be an integration test?

• Several api server tests are failing for me; but they are also failing in master branch. So, may not be anything to do with your code updates.

So, if api server tests are not failing for you, LGTM. Thanks.

(As discussed in IRC) Please add a new "properties" hash on the node model, leaving info as an admin-only hash, rather than combining the fields in one hash and pruning it at runtime.

Looking at 4cf16efd on 3605-node-info-field

The .to_i in the tests reveal that a .to_i should have happened during node#ping or even way back at arvados/v1/nodes_controller.rb line 29 when they were accepted from the client using a type-unaware encoding. (But I know this isn't a new bug, so I'll understand if you punt.)

Everything else looks great. Thanks.

3605-node-info-field changed #ping to do to_i on total_cpu_cores, total_ram_mb, and total_scratch_mb.

My second review feedback:

• apps/workbench/app/assets/javascripts/dates.js

The timestamp display in the dashboard (2:41 PM 09/19/2014) is much different than in other areas in workbench such as Pipeline instance Advanced tab ( 2014-09-19 18:43:43 UTC). The timestamps in all workbench pages should use the same display strategy to be consistent.

I also think (2:41 PM 09/19/2014) is better to be displayed as (09/19/2014 2:41 PM)

• Compute nodes panel

Unless there is a serious performance concern about displaying all the details during dashboard loading, I think we should display the details that appear when individual items in that panel are clicked (compute0, compute1 etc) right when Queue, Nodes. We also should consider replacing the clicks on Queue, Nodes etc with a single clickable item (such as the drop down in the panel title)

• workbench/app/controllers/application_controller.rb

I think the helper_method :running_pipelines should use desc order.

PipelineInstance.order(["started_at asc", "created_at asc"

Currently, the individual day’s items are displayed in reverse chronological order ( 4xphq-d1hrv-vng31w4sz1j7f6c Started at 2:31 PM 07/08/2014, 4xphq-d1hrv-tq6scqzdm61fnub Started at 2:38 PM 07/08/2014 etc)

• apps/workbench/app/models/job.rb

After merging in work from 3899, this should go away? (“def self.state job”)

• UserProfileTest < ActionDispatch::IntegrationTest
  “assert page.has_text?('Active pipelines'), 'No text - My projects' “ change to say “No text - Active pipelines”

Radhika Chippada wrote:

Peter,
As we discussed, I will provide my feedback in two steps.

• Compute nodes panel has a drop-down arrow, but is not really showing any drop-down menu.

It does toggle collapsing of the nodes and provides an affordance for clicking. I don't think a separate icon would be any clearer.

• Clicking on the Panel title (Compute node status) or any of the items (Queue, Nodes, Busy, Idle) displays a new row (please see the attachment 1). Is it possible to have a separate icon in this panel or panel title to show this new row rather than showing it when any one of them is clicked. It is not obvious otherwise.

The table is no longer clickable to avoid confusion thinking that different that the table cells are buttons.

• The information in the Compute node status panel needs to be reversed. I think it is desirable that we have a title Row (Queue Nodes Busy Idle) on top, and the number (info) row below it.

Since this is one of the few parts that's based on an actual design suggestion from our graphic designer, I'd prefer to keep in this way.

• In the "Recently finished pipelines" panel, it might be helpful if the output file names are listed to the right (pull-right), so that they are displayed right below the Output button.

Fixed.

Minor thing - in _show_advanced.html.erb, the spelling of "curl" is deliberately not "Curl", please don't fix. http://curl.haxx.se/

In ApplicationController.running_pipelines, it looks like pipelines that are running in the user's terminal will not show up here; they'll be hidden until they fail/succeed, at which point they will show up in the "recently finished" section. It would be better to show both kinds, and show a distinction between the two Running states. (The sentence "The new pipeline instance will also show up on the Workbench Dashboard." at http://doc.arvados.org/user/topics/running-pipeline-command-line.html should continue to be correct.)

• This is probably a good opportunity to fix the "content_for :tab_line_buttons inside content_for :content_top" weirdness. Presumably those should be two separate content_for sections?
• This is probably a good opportunity to remove the :page_content functionality that (AFAIK) has never been used or tested. It looks like it has already drifted far, far away from any likelihood of functioning correctly.

Radhika Chippada wrote:

My second review feedback:

• apps/workbench/app/assets/javascripts/dates.js

The timestamp display in the dashboard (2:41 PM 09/19/2014) is much different than in other areas in workbench such as Pipeline instance Advanced tab ( 2014-09-19 18:43:43 UTC). The timestamps in all workbench pages should use the same display strategy to be consistent.

#3629

I also think (2:41 PM 09/19/2014) is better to be displayed as (09/19/2014 2:41 PM)

• Compute nodes panel

Unless there is a serious performance concern about displaying all the details during dashboard loading, I think we should display the details that appear when individual items in that panel are clicked (compute0, compute1 etc) right when Queue, Nodes. We also should consider replacing the clicks on Queue, Nodes etc with a single clickable item (such as the drop down in the panel title)

I'm not entirely sure I understand the comment. The compute nodes list is hidden by default because if the number of compute nodes is very large (having 20 or 40 nodes in the future seems perfectly reasonable) it takes up a lot of space.

• workbench/app/controllers/application_controller.rb

I think the helper_method :running_pipelines should use desc order.

PipelineInstance.order(["started_at asc", "created_at asc"

Currently, the individual day’s items are displayed in reverse chronological order ( 4xphq-d1hrv-vng31w4sz1j7f6c Started at 2:31 PM 07/08/2014, 4xphq-d1hrv-tq6scqzdm61fnub Started at 2:38 PM 07/08/2014 etc)

That's intentional, the idea is to show the running pipelines at the top.

• apps/workbench/app/models/job.rb

After merging in work from 3899, this should go away? (“def self.state job”)

Probably, that code is also able to make sense of the jobs that are copied into the pipeline components, but I have to avoid using the job record copy because it's sometimes stale.

• UserProfileTest < ActionDispatch::IntegrationTest
  “assert page.has_text?('Active pipelines'), 'No text - My projects' “ change to say “No text - Active pipelines”

Fixed.

• Display only my stuff
• Show project & owner for each pipeline
• Warning signal for error conditions (e.g., no log output)
• Error message

• drop the "nodes" total
• relabel "idle" to "idle nodes"
• relabel "busy" to "busy nodes"
• relabel "queue" to "queued jobs"
• relabel "Compute node status" panel title to "Cluster status" (or "Crunch job and worker status", or...?)

The expand/collapse feature on "Compute node status" panel is weird in that it doesn't expand/collapse the panel, but affects just a portion of the panel, far away from the caret. It's also a bit disappointing to click "expand" and see such a tiny amount of information that it might as well have been shown up front. Perhaps the easiest solution here is to show all nodes all the time, but put them in a div with a max-height that gets scrollbars when the number of nodes is big.

Tom Clegg wrote:

Minor thing - in _show_advanced.html.erb, the spelling of "curl" is deliberately not "Curl", please don't fix. http://curl.haxx.se/

Reverted.

In ApplicationController.running_pipelines, it looks like pipelines that are running in the user's terminal will not show up here; they'll be hidden until they fail/succeed, at which point they will show up in the "recently finished" section. It would be better to show both kinds, and show a distinction between the two Running states. (The sentence "The new pipeline instance will also show up on the Workbench Dashboard." at http://doc.arvados.org/user/topics/running-pipeline-command-line.html should continue to be correct.)

Put it back, didn't add new UI to distinguish them. There are stale client-run pipelines on 4xphq that need to be stopped.

default_show.html.erb is an unhelpful filename in an area where it's already hard to follow which file does what. Could we use a filename more suggestive of what it does? (It looks like it renders the page title, name, description, and copy/move buttons.)

Changed to "title_and_buttons".

• This is probably a good opportunity to fix the "content_for :tab_line_buttons inside content_for :content_top" weirdness. Presumably those should be two separate content_for sections?

Fixed.

• This is probably a good opportunity to remove the :page_content functionality that (AFAIK) has never been used or tested. It looks like it has already drifted far, far away from any likelihood of functioning correctly.

Took that out.