search

whole-tale / dashboard

Whole Tale Dashboard
http://wholetale.org
MIT License
7 stars 2 forks source link

Highlight currently running tales #550

Closed bodom0015 closed 4 years ago

bodom0015 commented 4 years ago

Problem

After refactoring the Browse and Run views, the Tale Instances panel no longer appears on the right hand side. This makes it very difficult to determine which, if any, of your Tales are currently running.

Approach

Added a panel at the top of all filtered views within Browse to highlight "Currently Running Tales".

How to Test

  1. Checkout and run this branch locally, rebuild the dashboard
  2. Login to the WholeTale Dashboard
  3. Launch a Tale (if you haven't already)
  4. Navigate to the Browse view
    • You should see the "Currently running Tales" panel appears above the listing of all existing Tales
  5. Click "Stop" beneath a running Tale
    • You should see a spinner appear indicating that your Tale is being stopped
  6. Wait for the instance to stop
    • You should see the stopped Tale disappear from the "Currently running Tales" panel
    • If no more Tales are running, then you should see the "Currently running Tales" panel disappear from the view
craig-willis commented 4 years ago

Thanks, @bodom0015. Two observations based on the current mockups and one problem:

  1. The mockups don't include the running tales separation on the Public/All tales page, but this PR does. Was this intentional?
  2. In the current PR, the running tale is displayed twice -- once above the line and once below. In the mockup this isn't the case. Running tales are above the line, not running are below.
  3. When I click Stop on the tale on "My Tales", the tab switches immediately to "All Tales". When I select "My Tales", the button says "Run" but the tale is still displayed above the line with the 1 instance running message.

Points 2 and 3 are captured in the following recording: https://recordit.co/hAnBAmHM4o

bodom0015 commented 4 years ago

Thanks for taking a look at this, @craig-willis!

  1. This is part of the reason why I had missed the "Running Tales" section in the first place. Since we plan to support "Public Tales" that are running (e.g. someone shared a Public Tale with me, and now I can run it), I am of the opinion that we should display the Running Tales even when "Public Tales" is selected as the filter. I raised this as well yesterday during the meeting in the chat, but sadly it was not captured. @Xarthisius said he believed that this was an oversight in the mockups, so I made the panel display on "Public Tales" as well. There is potentially some confusion that the filters do not (and likely should not) affect your "Currently running Tales", as this introduces more questions for which we do not yet have answers.

  2. This was intentional to simplify the implementation in the UI, as it was unclear whether the choice was intentional in the mockups not to include the above Tales in the lower section. The "Running Tales" section does a query for instances and the Tales associated with them. The lower section does a query for Tales and lists them all there. If desired, I can attempt to filter out the running Tales from above, but my initial attempts interfered with the existing filters on the page, and trying to get them all working together is quite tedious.

  3. This is likely our usual side-effect of "the instance isn't deleted in the backend yet, but you've changed views and now I need to reload". If you wait an additional ~15 seconds before switching tabs, then this should not occur. I will reiterate, as this particular bug comes up quite often, that as far as I know we still do not have a graceful way of detecting this state in the frontend.

Xarthisius commented 4 years ago
  1. This is likely our usual side-effect of "the instance isn't deleted in the backend yet, but you've changed views and now I need to reload". If you wait an additional ~15 seconds before switching tabs, then this should not occur. I will reiterate, as this particular bug comes up quite often, that as far as I know we still do not have a graceful way of detecting this state in the frontend.

Setting a custom status (InstanceStatus.DELETING) wouldn't help in that situation as Ember rather doesn't update the object upon DELETE, would it?

bodom0015 commented 4 years ago

@Xarthisius Ember-Data's deletion patterns actually do attempt to refresh the object after deletion by default. The user needs to pass a special flag to findRecord to avoid accidentally attempting to refresh a model in the background that has been deleted from the server. Here is the relevant snippet from the docs:

The backgroundReload option is used to prevent the fetching of the destroyed record, since findRecord() automatically schedules a fetch of the record from the adapter.

It is unclear if we are following the above pattern correctly in all cases, as I had a lot of problems fixing this exact problem when attempting to create PR #412.

I don't think that Ember-Data has anything to do with it though, since the issue happens on refresh as well. The problem is that stale instance data is being returned from the server. From what I understand, it is necessary for Girder to keep the instance until it has finished cleaning up the underlying container. You can hit the Girder API on Swagger to recreate the problem if you need further proof: http://recordit.co/qckFV2ff0j

Since I have no way to detect that it no longer represents a running Tale instance, it must be used to render the display like all other data. If the stale instance coming from the server had a special status for DELETING or stale: true or any way at all for me to know that it is stale, I would be able to detect and ignore it when reloading the page or changing views. See https://github.com/whole-tale/dashboard/issues/213#issuecomment-447056036

Another option might be to add a filter to GET /instance?transient=false (ideally, false by default) that will simply not return such instances. For example, GET /instance?transient=true could allow the cleanup tasks to fetch the stale records they need for cleanup, while GET /instance?transient=false would only return what the UI needs to display to the user.

bodom0015 commented 4 years ago

Once https://github.com/whole-tale/girder_wholetale/pull/357 is merged, @craig-willis' third point above should be resolved.

For verification, here is an additional Test Case:

  1. Open the Developer Console to the Networks tab
  2. Click "My Tales"
  3. Click Stop on the tale
  4. Wait for the status of the DELETE /instance/:id call to change from (pending) to 200
  5. Immediately click over to "All Tales", then back to "My Tales" again and/or refresh the page
    • The Tale you chose to Stop should not reappear in the "Currently running Tales" section above
craig-willis commented 4 years ago

This looks good. I'm also seeing what I think @ThomasThelen reported in https://github.com/whole-tale/dashboard/issues/549, but we can treat that separately.