Feature #15736

[WB2] add fallback mechanism to site manager for adding ephemeral clusters

Added by Ward Vandewege 10 months ago. Updated 8 months ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Target version:
Start date:
11/04/2019
Due date:
% Done:

100%

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

Description

In workbench, it is possible to add a cluster to the site manager and log in to a remote user, even if that cluster is not defined in the federation and there is no remote user account that WB can access: the user gets redirected to the appropriate SSO, and then eventually returned to the requesting WB, having a logged in session with the remote cluster.

For WB2, this doesn't work anymore: the user simply sees "Could not validate cluster" when trying to do this. E.g. when trying to add qr1hi.arvadosapi.com from wb2.4xphq's site manager.

We want to keep supporting this feature to allow for multisite search to an ephemeral cluster.


Subtasks

Task #15787: Review 15736-site-mgrResolvedPeter Amstutz

Associated revisions

Revision 8dd64fbb
Added by Peter Amstutz 9 months ago

Merge branch '15736-site-mgr' refs #15736

Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <>

History

#1 Updated by Ward Vandewege 10 months ago

  • Status changed from New to In Progress

#2 Updated by Ward Vandewege 10 months ago

  • Description updated (diff)
  • Status changed from In Progress to New

#3 Updated by Ward Vandewege 10 months ago

  • Target version set to To Be Groomed

#4 Updated by Peter Amstutz 9 months ago

  • Assigned To set to Peter Amstutz
  • Target version changed from To Be Groomed to 2019-11-06 Sprint

#5 Updated by Peter Amstutz 9 months ago

  • Story points set to 2.0

#6 Updated by Peter Amstutz 9 months ago

15736-site-mgr @ commit:d6538e3ec3a545258ff9073ef88501fe2c75a4f0

Can now add other clusters that are not part of the federation to multisite search by going through login process.

To test: run wb2 with c97qk as the local cluster (config.json has "API_HOST": "c97qk.arvadosapi.com"). Go to site manager and add qr1hi.arvadosapi.com. It will take you to log in, then return to wb2 "add-session" endpoint, record the session, and return to site manager.

Also improves error handling for searches. Errors in session validation are reported. Also, if one search fails in multisite search, it does not prevent other searches from being displayed.

#7 Updated by Lucas Di Pentima 9 months ago

  • There’s a test that needs updating at src/store/auth/auth-action.test.ts
  • On wb1 there’s the option to remove a remote cluster session from outside the federation, maybe it should be added on wb2?
  • When logging off some remote clusters (either from the federation or not), immediate searches seem to be done on the remaining logged in remotes, but if the app is reloaded, the remote sessions get logged in back.

#8 Updated by Peter Amstutz 9 months ago

Lucas Di Pentima wrote:

  • There’s a test that needs updating at src/store/auth/auth-action.test.ts

Fixed.

  • On wb1 there’s the option to remove a remote cluster session from outside the federation, maybe it should be added on wb2?

Sure. Added.

  • When logging off some remote clusters (either from the federation or not), immediate searches seem to be done on the remaining logged in remotes, but if the app is reloaded, the remote sessions get logged in back.

The new default behavior is to always try to log in when the app loads. Right now, the "not logged in" state can mean either it previously tried to connect and failed, or that the user manually pressed log out. The user can also now delete it from the list. Although, members of the federation are always re-added to the sessions list, so they will come back on the next app refresh.

We could add a new "disabled" state but actually I think the current behavior performing federation log in by default is actually better for the user, it reduces failure modes where sites that should have been search don't get searched (compare the earlier WB2 multi-site search behavior prior to my recent branches, it has a sticky log-in flag that isn't re-validated by default, we've already had support requests about search not working because it didn't log in by default.)

15736-site-mgr @ commit:ceb4f50aeca2bb6b0354a7bd96a599b4a14147fe

#9 Updated by Lucas Di Pentima 9 months ago

On wb1, the removable cluster sessions are the ones not part of the federation. Here, all sessions are removable, even the local one. When removing all sessions and then trying to add one, I get an error like:

Objects are not valid as a React child (found: Error: Could not validate cluster). If you meant to render a collection of children, use an array instead.
    in p (created by FormHelperText)
    in FormHelperText (created by WithStyles(FormHelperText))
    in WithStyles(FormHelperText) (created by TextField)
    in div (created by FormControl)
[...]

If not possible to match wb1's functionality, I think at least we shouldn't allow removing the local cluster's session.

With that, it LGTM.

#10 Updated by Peter Amstutz 9 months ago

Lucas Di Pentima wrote:

On wb1, the removable cluster sessions are the ones not part of the federation. Here, all sessions are removable, even the local one. When removing all sessions and then trying to add one, I get an error like:

[...]

If not possible to match wb1's functionality, I think at least we shouldn't allow removing the local cluster's session.

With that, it LGTM.

Thanks, the delete button doesn't show for those.

In the process, found and fixed a config loading bug as well.

15736-site-mgr @ commit:a1cfba8c2407131a8c5d6c21dc741d6319f8648a

#11 Updated by Lucas Di Pentima 9 months ago

The updates LGTM, but I think I catched another issue (sorry for not testing it earlier!): When searching, if I try to click on a search result from a non-federated remote cluster, nothing happens. I've checked the JS console & Dev network tab and couldn't see any kind of activity/error message.

#12 Updated by Peter Amstutz 9 months ago

Lucas Di Pentima wrote:

The updates LGTM, but I think I catched another issue (sorry for not testing it earlier!): When searching, if I try to click on a search result from a non-federated remote cluster, nothing happens. I've checked the JS console & Dev network tab and couldn't see any kind of activity/error message.

So the issue was no config entry for the non-federated remote cluster, so it didn't have a workbench address to use.

I did some more refactoring, and now it saves the config from every cluster that it fetches. Also eliminated some redundant config fetching.

To assist navigation, I added the cluster id to the title bar as well.

15736-site-mgr @ commit:087d49d5c43866c8a20e8ac830ccc9b12188408f

#13 Updated by Lucas Di Pentima 9 months ago

Clicking now works for me, thanks!

One more issue I've just found: when adding a remote cluster, it isn't being added to the localStorage.sessions array, so it dissapears once the page is reloaded. I thought I checked this but I tried it even on commit:ceb4f50a and it wasn't working then.

#14 Updated by Peter Amstutz 9 months ago

Lucas Di Pentima wrote:

Clicking now works for me, thanks!

One more issue I've just found: when adding a remote cluster, it isn't being added to the localStorage.sessions array, so it dissapears once the page is reloaded. I thought I checked this but I tried it even on commit:ceb4f50a and it wasn't working then.

Argh! I swear that was working before.

#15 Updated by Lucas Di Pentima 9 months ago

So after some more testing, we've found that this happens when using 4xphq as a local cluster. Using c97qk does work correctly.

#16 Updated by Peter Amstutz 9 months ago

Lucas Di Pentima wrote:

So after some more testing, we've found that this happens when using 4xphq as a local cluster. Using c97qk does work correctly.

Ok, I don't know how the cluster itself factors into this, but I found the bug. It was saving the session with the newly added cluster, but then a pending callback was saving the session again with an older value, that overwrote and forgot the newly added cluster. Whoops.

I suspect that "sync sessions to local storage" ought to be hooked up as some kind of listener or middleware instead of the current ad hoc method (do some stuff, then save), but I don't want to get into it.

15736-site-mgr @ commit:dec81d42222814723ff0386665928045ff41eac8

#17 Updated by Lucas Di Pentima 9 months ago

commit:dec81d42222814723ff0386665928045ff41eac8 worked great, LGTM. Thanks!

#18 Updated by Peter Amstutz 9 months ago

  • Status changed from New to Resolved

#19 Updated by Tom Morris 8 months ago

  • Release set to 22

Also available in: Atom PDF