Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Custom URL #1307

Closed
wants to merge 8 commits into from
Closed

Custom URL #1307

wants to merge 8 commits into from

Conversation

appolloford
Copy link
Contributor

Description

  • What is implemented or fixed? Mention the linked issue(s), if any.
    This function is implemented for deploying CARTA onto HPC servers controlled by job scheduler (e.g. slurm). If a user launches CARTA on a computing node, we need the URL prefix information to do the reverse proxy. The approach is used in Open OnDemand to set interactive apps

  • How does this PR solve the issue? Give a brief summary.
    This PR allows users to customize the URL by an environment variable CARTA_URL_PREFIX. If launching CARTA_URL_PREFIX=foo ./carta_backend locally, the URL will be changed to http://localhost:port/foo/?token=.....

  • Are there any companion PRs (frontend, protobuf)?
    No

  • Is there anything else that testers should know (e.g. exactly how to reproduce the issue)?
    We have tested the frontend on our HPC server, but we are not sure the influence to the the scripting/database/config

Checklist

  • changelog updated / no changelog update needed
  • e2e test passing / corresponding fix added / new e2e test created
  • protobuf updated to the latest dev commit / no protobuf update needed
  • added reviewers and assignee
  • added ZenHub estimate, milestone, and release

@kswang1029
Copy link
Contributor

@appolloford have you considered to use the controller deployment? https://carta-controller.readthedocs.io/en/dev/ Are you trying to deploy CARTA to serve multiple users?

@appolloford
Copy link
Contributor Author

appolloford commented Sep 1, 2023

@kswang1029 No, we cannot install the controller. The idea of HPC server is sharing servers among jobs and it is controlled by job scheduler. We cannot guarantee a server continuously on only for CARTA. Also, users consume their quota (e.g. 1000 CPU hours per month) to launch their jobs. If we keep a controller there to control the sessions, it is hard to know how much resources they have consumed.
This approach we proposed makes users launch CARTA when they need and each of them will have their own host. They can also control how many resources they want to have.

@ajm-ska
Copy link
Collaborator

ajm-ska commented Sep 1, 2023

@appolloford I built your branch on AlmaLinux8 (GCC 8.5.0), but it is unable to run:

[2023-09-01 08:50:48.336] [CARTA] [info] Writing to the log file: /root/.carta/log/carta.log
[2023-09-01 08:50:48.336] [CARTA] [info] /root/carta-backend-url/build/carta_backend: Version 4.0.0-beta.1
[2023-09-01 08:50:48.337] [CARTA] [critical] basic_string::_M_construct null not valid

Which OS are you developing it on?
Maybe there is different behavior with the different GCC versions between OSs and our automated CI is not running your forked branch.

@kswang1029
Copy link
Contributor

kswang1029 commented Sep 1, 2023

@appolloford please file a feature request at https://github.com/CARTAvis/carta/issues then we can continue from there. This PR will be on hold due to our code freeze for release 4.0.

@appolloford
Copy link
Contributor Author

@ajm-asiaa It should be a problem from variable initialization. I made a new commit. Could you try it again?
@kswang1029 Sure. Is there a template I can refer to?

@kswang1029
Copy link
Contributor

@ajm-asiaa It should be a problem from variable initialization. I made a new commit. Could you try it again?
@kswang1029 Sure. Is there a template I can refer to?

Free form 🙂 try to be as comprehensive as possible. Thanks.

@veggiesaurus
Copy link
Collaborator

Just a couple of things:

  1. the controller itself is pretty much just a reverse proxy, takes no CPU resources
  2. the proccess themselves start up as the user, so you can track their usage. You can use a script to start up the backend process via a slurm/etc queue if needed, rather than directly.

However, I still think this PR is a good idea, and I realise that there will be situations (like yours) where the controller is not an option (although please make sure your users use tunnelling etc instead of VNC/x-forwarding when using CARTA!).

I'd rather make this a bit more generic. At the moment, we do something like http://localhost:3002/?token=asdasd2ad2. We can turn that into a template quite easily. Something like:

CARTA_TEMPLATE_URL = "http://{{host}}:{{port}/?token={{token}}" or if we want more details: {{protocol}}://{{host}}:{port}/?token={{token}}

The backend could just use the default template URL (as above) if none is provided, and fill in host port, token and so on. We could even add additional fields if we wanted (e.g. username).

in your case, it would be as simple as just using CARTA_TEMPLATE_URL=http://localhost:{{port}/cartafoo?token={token}, but I can see situations where other variations might be useful. e.g. https://{user}.myproxy-server/carta?token={token}

@appolloford
Copy link
Contributor Author

@veggiesaurus Thank you for the comments. I did not know that the controller works as a reverse proxy. Our plan was to integrate CARTA into Open OnDemand so I did not study controller carefully.
The idea of CARTA_TEMPLATE_URL looks great to me but I don't know how much information will be needed in a normal reverse proxy. Perhaps my colleague can comment more on this.

@veggiesaurus
Copy link
Collaborator

The idea of CARTA_TEMPLATE_URL looks great to me but I don't know how much information will be needed in a normal reverse proxy. Perhaps my colleague can comment more on this.

I think this is purely just a generalisation of your idea, so rather than just a prefix, we can adjust the URL in almost any way

@Micket
Copy link

Micket commented Sep 3, 2023

Hi, I'm the colleague who pushed @appolloford to open this PR

CARTA_TEMPLATE_URL = "http://{{host}}:{{port}/cartafoo?token={{token}}"

I think trying to specify the full URL here has problems. Mainly that you don't really get to choose all those parts freely at all.
Most of the urls that you need to be able to fetch from would have content interjected into the middle for anything to work:

http://{{host}}:{{port}}/some/custom/prefix/index.html?token={{token}}  # and any other html/js/svg static content
http://{{host}}:{{port}}/some/custom/prefix/api/database/preferences?token={{token}}
                                           |-- this part is determined elsewhere --|

as one can't get to decide the layout for the latter part of the path, and definitely not the query parameter (token) part.

And you also don't get to choose the port part either, and this part doesn't impact any of the websocket code since it's only the path part of the url that may impact routing. The rest would just be used in the printout from main (which any semi-automated system wouldn't look at anyway), so..

http://{{host}}:{{port}}/some/custom/prefix/index.html?token={{token}}
|--- doesn't matter ---| 

So you really only get to pick the prefix for the path segment, and in order to resolve the path to all the static content, need to know precisely the url prefix part /some/custom/prefix here in order to strip it off.
This url prefix part is the same as what jupyter (ServerApp.base_url) and matlab proxy (MWI_BASE_URL) does for example.

If the server could be configured to link all assets relatively, an approach using url rewrites in the proxy would also be doable, but I suspect that would just be more work (as far as i could tell from some tests, it doesn't do so right now)

@veggiesaurus
Copy link
Collaborator

veggiesaurus commented Sep 4, 2023

@Micket those are strong arguments 👍 I'm convinced 😇

Also, I think it's great that you're integrating this into Open On Demand 🚀

@confluence confluence self-assigned this Sep 5, 2023
@confluence
Copy link
Collaborator

I think that we're agreed that this is a good feature to add. I would like some changes to be made to the implementation (some of them should wait for some other PRs to be merged in), and there's a merge conflict that needs to be resolved. I am going to make a copy of @appolloford's branch in the main repo and take over development -- I will close this PR, but will make a new draft PR from the new branch.

@confluence
Copy link
Collaborator

confluence commented Nov 28, 2023

The new PR is #1338.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants