Allow configuring Project Manager server host and port #6774

somebody1234 · 2023-05-19T07:14:20Z

Pull Request Description

Closes [/127.0.0.1:30535] Address in use #6730
- Changes config to allow environment variables to override server host and port
- Adds port scanning to Electron app to ensure the PM is started at a free port

Important Notes

SERVER_PORT=abcd enso.AppImage does NOT work. It would not be difficult to implement, but it probably needs discussion on how exactly it should be implemented - for example, SERVER_PORT is quite a generic name, should the Electron app pass though something like ENSO_PM_SERVER_PORT to the PM as SERVER_PORT instead?

⚠️ Port scanning is only implemented in the JS frontend. It is not implemented:

In Scala, because the JS/Rust code calling it needs to know the port as well. There shouldn't be any problems with adding port scanning though, if that's desired
In Rust, because I'm not sure parsing the host and port from a string is a good idea.
- (This also applies to JS, but it must work in JS, and port scanning is already a dependency there so it's quite a bit easier)
QA will need a new PM (sbt buildProjectManagerDistribution or ./run backend sbt -> buildProjectManagerDistribution), and the path must be supplied as: -engine.project-manager-path=path/to/new/pm/here

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

The documentation has been updated, if necessary.
Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
All code follows the
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
All code has been tested:
- Unit tests have been written where possible.
- If GUI codebase was changed, the GUI was tested when built using ./run ide build.

somebody1234 · 2023-05-21T22:14:51Z

i don't know whether this needs tests on the scala side

somebody1234 · 2023-06-19T09:04:47Z

So it seems like running two instances side-by-side by the same user should be unsupported - the Project Managers seem to want to acquire locks on the same files for compilation. So, when testing, instead a dummy HTTP server should be run on port 30535

xvcgreg · 2023-06-26T13:31:18Z

@PabloBuchu what's the status of this task?

somebody1234 · 2023-07-27T07:39:38Z

closing until this becomes an issue again

somebody1234 · 2023-08-09T06:47:42Z

i've done some basic testing only - by using ide watch with a http server already running at port 30535 (which makes it start the PM at port 30536):

✔️ PM is started at port 30536
✔️ opening new empty project works on local backend
✔️ opening new project from template works on local backend
✔️ opening project works on remote (cloud) backend
✔️ manually running PM at 30535 fails when there is another application running on that port
✔️ manually running PM at 30535 succeeds when there is no application running on that port
✔️ manually running PM without explicit port, defaults to 30535
✔️ http://localhost:8080/?engine.projectManagerUrl=ws://127.0.0.1:30535 errors when there is no WebSocket server at port 30535
✔️ http://localhost:8080/?engine.projectManagerUrl=ws://127.0.0.1:30536 doesn't error, and properly lists projects

4e6 · 2023-08-09T06:53:01Z

lib/scala/project-manager/src/main/scala/org/enso/projectmanager/boot/ProjectManager.scala

-        config.server.host,
-        config.server.port
+        processConfig.serverHost.getOrElse(config.server.host),
+        processConfig.serverPort.getOrElse(config.server.port)


Please add overrides in the config (instead or in addition to the cli flags)

enso/lib/scala/project-manager/src/main/resources/application.conf

Lines 35 to 36 in 59329bd

host = ${project-manager.network.interface}

port = 30535

The same way it is done for the network

enso/lib/scala/project-manager/src/main/resources/application.conf

Lines 23 to 32 in 59329bd

network {

interface = "127.0.0.1"

interface = ${?NETWORK_INTERFACE}

min-port = 49152

min-port = ${?NETWORK_MIN_PORT}

max-port = 65535

max-port = ${?NETWORK_MAX_PORT}

}

i.e the line

interface = ${?NETWORK_INTERFACE}

defines override of the interface value by the NETWORK_INTERFACE environment variable.

This way you can override config by providing the necessary environment variables without the need of adding extra CLI parameters.

somebody1234 · 2023-08-09T07:04:17Z

oh oops

(edit: the changes aren't finished, and the electron app will need to switch to provide envrironment variables instead, so the latest commit does NOT work)

…on app

somebody1234 · 2023-08-09T07:36:58Z

so it seems like all the CLI changes were unnecessary - not a bad thing though, since it means fewer things could go wrong

somebody1234 · 2023-08-09T07:40:46Z

i've done basic re-testing so there shouldn't be any major problems

JaroslavTulach

As far as I can say this is the right approach and I am looking forward to use it.

JaroslavTulach · 2023-08-10T08:26:19Z

lib/scala/project-manager/src/main/resources/application.conf

@@ -51,7 +54,7 @@ project-manager {

  storage {
    projects-root = ${user.home}/enso
-    projects-root=${?PROJECTS_ROOT}
+    projects-root = ${?PROJECTS_ROOT}


I cannot claim to be an expert on Scala and its .conf file. However I believe that the ${?SERVER_HOST} definition reads value from environment property, if it is set.

I am surprised we need so little changes in the project-manager code!

JaroslavTulach · 2023-08-10T08:27:00Z

app/ide-desktop/lib/client/src/index.ts

+                // eslint-disable-next-line @typescript-eslint/naming-convention
+                SERVER_HOST: this.projectManagerHost,
+                // eslint-disable-next-line @typescript-eslint/naming-convention
+                SERVER_PORT: `${this.projectManagerPort}`,


This code prepares the SERVER_HOST and SERVER_PORT environment variables for the project-manager.

JaroslavTulach · 2023-08-10T08:27:32Z

app/ide-desktop/lib/client/src/bin/project-manager.ts

    const binPath = pathOrPanic(args)
-    return await execFile(binPath, processArgs)
+    return await execFile(binPath, processArgs, { env })


Here the environment variables are passed to the project-manager process.

somebody1234 · 2023-08-18T11:18:16Z

@PabloBuchu this should be QAd (and code reviewed) at some point too btw

PabloBuchu · 2023-08-22T08:05:39Z

just tried build and run normally and it is trowing errors

@JaroslavTulach @4e6 could you do a QA of the part with the PM port?

somebody1234 · 2023-08-22T09:07:24Z

weird, i thought MutuallyExclusiveOptions wouldn't be an issue after being fixed elsewhere... maybe something related to the project manager is staying in the URL?

i did change the logic that decides whether to try loading index.js.gz in the fix for showLogs, i guess it still always needs to history.replaceState(null, '', new URL('.', originalUrl)) when opening cloud projects though

enso/app/ide-desktop/lib/dashboard/src/authentication/src/dashboard/components/editor.tsx

Lines 122 to 139 in a2946d2

    
           if (supportsLocalBackend) { 
        
               await runNewProject() 
        
           } else { 
        
               if (!initialized) { 
        
                   await Promise.all([ 
        
                       load.loadStyle(`${assetsRoot}style.css`), 
        
                       load.loadScript(`${assetsRoot}index.js.gz`), 
        
                   ]) 
        
                   setInitialized(true) 
        
               } 
        
               const originalUrl = window.location.href 
        
               // The URL query contains commandline options when running in the desktop, 
        
               // which will break the entrypoint for opening a fresh IDE instance. 
        
               history.replaceState(null, '', new URL('.', originalUrl)) 
        
               await runNewProject() 
        
               // Restore original URL so that initialization works correctly on refresh. 
        
               history.replaceState(null, '', originalUrl) 
        
           }

somebody1234 · 2023-08-22T10:18:18Z

pushed a commit that may fix it, unable to test due to issues with linux PM bundle

4e6 · 2023-08-22T15:06:17Z

I did the QA. It seems to work on my Linux machine

Started the project manager on a custom port env SERVER_PORT=30536 java -jar $JAVA_OPTS project-manager.jar -vv and then connected to it from the browser http://localhost:8080/?authentication=false&engine.projectManagerUrl=ws://127.0.0.1:30536
Built the IDE distribution with ./run --skip-version-check ide build --skip-wasm-opt and started it with ./dist/ide/enso-linux-2023.2.1-dev.AppImage -authentication=false --debug.dev-tools without errors in the console.

somebody1234 added 2 commits May 19, 2023 17:09

Add server host and port command-line arguments to Project Manager

68e6bf3

Merge branch 'develop' into wip/sb/project-manager-host-port

2d5d2f7

somebody1234 added 2 commits May 22, 2023 08:24

Add the new options to the CLI option parser

41c841c

Fix formatting

92c736a

somebody1234 force-pushed the wip/sb/project-manager-host-port branch from 8ee8af1 to 92c736a Compare May 21, 2023 22:28

somebody1234 added 9 commits May 22, 2023 17:35

Fix type error

20948b9

Pass project manager url; rename option; fix bugs

163cd4f

Properly pass CLI options to server

8c68309

Fix error

33283ea

Fix error

ae904df

Merge branch 'develop' into wip/sb/project-manager-host-port

809e7a4

Merge branch 'develop' into wip/sb/project-manager-host-port

a087654

Fix type error

95cf2ee

Fix bugs

61802b5

somebody1234 added the CI: No changelog needed Do not require a changelog entry for this PR. label May 25, 2023

somebody1234 mentioned this pull request May 25, 2023

Error executing EnsureCompiledJob when multiple projects are open at the same time #6857

Closed

Merge branch 'develop' into wip/sb/project-manager-host-port

e062da6

enso-bot bot mentioned this pull request May 30, 2023

Could not resolve engine version for Edition(Some(NaN) warnings #6806

Closed

Merge branch 'develop' into wip/sb/project-manager-host-port

e5d0b34

enso-bot bot mentioned this pull request Jun 19, 2023

Investigate reasons for Request timeouts during AttachVisualizationHandler #7005

Closed

PabloBuchu self-assigned this Jun 20, 2023

somebody1234 closed this Jul 27, 2023

somebody1234 reopened this Aug 9, 2023

somebody1234 added 2 commits August 9, 2023 15:08

Merge branch 'develop' into wip/sb/project-manager-host-port

5c1f5a9

Fix bug

479451f

somebody1234 marked this pull request as ready for review August 9, 2023 06:44

somebody1234 mentioned this pull request Aug 9, 2023

[/127.0.0.1:30535] Address in use #6730

Closed

Fix lint warning

e96174e

4e6 reviewed Aug 9, 2023

View reviewed changes

somebody1234 added 2 commits August 9, 2023 17:05

Remove CLI opts in favor of config overrides using environment variables

dc9f902

Use environment variables instead of command-line arguments in Electr…

e350ced

…on app

somebody1234 force-pushed the wip/sb/project-manager-host-port branch from 14f4542 to e350ced Compare August 9, 2023 07:21

Rename pmProcess back to process

73791a6

somebody1234 changed the title ~~Add server host and port command-line arguments to Project Manager~~ Allow configuring Project Manager server host and port Aug 9, 2023

somebody1234 requested a review from 4e6 August 9, 2023 07:36

4e6 approved these changes Aug 9, 2023

View reviewed changes

JaroslavTulach approved these changes Aug 10, 2023

View reviewed changes

Merge branch 'develop' into wip/sb/project-manager-host-port

088e404

enso-bot bot mentioned this pull request Aug 11, 2023

Unify & speed access to array-like objects behind ArrayXyzNode #6299

Closed

somebody1234 added 3 commits August 11, 2023 18:28

Merge branch 'develop' into wip/sb/project-manager-host-port

9509c7e

Fix IDE bug on cloud backend

8c7eae6

Merge branch 'develop' into wip/sb/project-manager-host-port

6ace1e6

Potential fix for MutuallyExclusiveOptions error

5955c7f

PabloBuchu added the CI: Ready to merge This PR is eligible for automatic merge label Aug 25, 2023

PabloBuchu approved these changes Aug 25, 2023

View reviewed changes

mergify bot merged commit aa2f72c into develop Aug 25, 2023
27 checks passed

mergify bot deleted the wip/sb/project-manager-host-port branch August 25, 2023 07:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow configuring Project Manager server host and port #6774

Allow configuring Project Manager server host and port #6774

somebody1234 commented May 19, 2023 •

edited

Loading

somebody1234 commented May 21, 2023 •

edited

Loading

somebody1234 commented Jun 19, 2023

xvcgreg commented Jun 26, 2023

somebody1234 commented Jul 27, 2023

somebody1234 commented Aug 9, 2023 •

edited

Loading

4e6 Aug 9, 2023

somebody1234 commented Aug 9, 2023 •

edited

Loading

somebody1234 commented Aug 9, 2023

somebody1234 commented Aug 9, 2023

JaroslavTulach left a comment

JaroslavTulach Aug 10, 2023

JaroslavTulach Aug 10, 2023

JaroslavTulach Aug 10, 2023

somebody1234 commented Aug 18, 2023 •

edited

Loading

PabloBuchu commented Aug 22, 2023

somebody1234 commented Aug 22, 2023

somebody1234 commented Aug 22, 2023

4e6 commented Aug 22, 2023

	network {
	interface = "127.0.0.1"
	interface = ${?NETWORK_INTERFACE}

	min-port = 49152
	min-port = ${?NETWORK_MIN_PORT}

	max-port = 65535
	max-port = ${?NETWORK_MAX_PORT}
	}

Allow configuring Project Manager server host and port #6774

Allow configuring Project Manager server host and port #6774

Conversation

somebody1234 commented May 19, 2023 • edited Loading

Pull Request Description

Important Notes

Checklist

somebody1234 commented May 21, 2023 • edited Loading

somebody1234 commented Jun 19, 2023

xvcgreg commented Jun 26, 2023

somebody1234 commented Jul 27, 2023

somebody1234 commented Aug 9, 2023 • edited Loading

4e6 Aug 9, 2023

Choose a reason for hiding this comment

somebody1234 commented Aug 9, 2023 • edited Loading

somebody1234 commented Aug 9, 2023

somebody1234 commented Aug 9, 2023

JaroslavTulach left a comment

Choose a reason for hiding this comment

JaroslavTulach Aug 10, 2023

Choose a reason for hiding this comment

JaroslavTulach Aug 10, 2023

Choose a reason for hiding this comment

JaroslavTulach Aug 10, 2023

Choose a reason for hiding this comment

somebody1234 commented Aug 18, 2023 • edited Loading

PabloBuchu commented Aug 22, 2023

somebody1234 commented Aug 22, 2023

somebody1234 commented Aug 22, 2023

4e6 commented Aug 22, 2023

somebody1234 commented May 19, 2023 •

edited

Loading

somebody1234 commented May 21, 2023 •

edited

Loading

somebody1234 commented Aug 9, 2023 •

edited

Loading

somebody1234 commented Aug 9, 2023 •

edited

Loading

somebody1234 commented Aug 18, 2023 •

edited

Loading