Skip to content

[supervisor] Add port management #1909

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

Merged
merged 10 commits into from
Nov 4, 2020
Merged

[supervisor] Add port management #1909

merged 10 commits into from
Nov 4, 2020

Conversation

csweichel
Copy link
Contributor

@csweichel csweichel commented Sep 25, 2020

  • /werft ws-feature-flags=registry_facade
  • /werft https

This PR adds holistic port management to supervisor.

Until now Theia had multiple tasks when it comes to ports. For one, it was listening to ports being served in the workspace (moved to supervisor in #1827), it's listening for workspace instance status updates to determine which ports are exposed, and it combines those two sources to auto-expose ports when neccesary. It also offers a UI where users can expose ports publicly.

We're looking to move this functionality out of Theia and into supervisor. To this end, this PR

  • extends the supervisor port status API to support port exposure and served status
  • implements auto-exposure of served ports
  • starts the infrastructure in supervisor to combine the served/exposed ports state
  • implements a first version of an ExposedPorts source (by polling openPorts on the server)
  • introduces a comprehensive set of unit tests for that state merge.

This PR also changes the termination log behaviour of workspaces to include the last few lines of log output. This makes debugging supervisor crashes tenable.

Open Points

  • implement a ExposedPortsInterface that listens to workspace instance updates rather than polling. To this end one would first need to extend the Go Gitpod API support to provide access to incoming notifications.
  • make the Gitpod API websocket connection automatically reconnect if it looses connection (kind of like what gRPC connections do)
  • remove the old port handling code from Theia and consume the supervisor API instead
  • hook up into Code

How to test

  • start some workspace
  • create server.js file with the following content:
const http = require('http');

const requestListener = function (req, res) {
  res.writeHead(200);
  res.end('Hello, World!');
}

const server = http.createServer(requestListener);
server.listen(4040, 'localhost');
  • run node server.js to test that the localhost service gets served and exposed via the proxy as private
  • there should be a notification offering to open it in browser/preview or make public
  • try to toggle visibility and check that changes are reflected in the ports view and status bar
  • stop the script, check that view and status bar updated
  • now change the script to serve on '0.0.0.0', start the server and check everything again
  • update .gitpod.yml with a new record for the unexposed port yet, try different action and visibilities, each time change server.js to use such port and start the server
  • don't forget to try port ranges similarly to the previous point
  • check that auto resolving localhost URIs work:
    • in terminal echo localhost:5000 and then click on a link, given that 5000 is not exposed yet
  • additional points:

@csweichel csweichel requested a review from akosyakov September 25, 2020 12:48
@csweichel csweichel force-pushed the cw/supervisor-ports branch 2 times, most recently from 3b8a0c9 to fff5231 Compare October 12, 2020 12:00
@akosyakov akosyakov self-assigned this Oct 15, 2020
@akosyakov akosyakov force-pushed the cw/supervisor-ports branch 25 times, most recently from 7b11e2f to e0a316b Compare October 21, 2020 12:37
@akosyakov akosyakov force-pushed the cw/supervisor-ports branch from d19fed3 to da20152 Compare October 30, 2020 14:32
if config != nil {
ports = config.Ports
}
portConfigs, rangeConfigs := service.parser.parseInstanceConfigs(ports)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It took me a minute to understand this function. If I understand correctly, when there's no config, service.parser.parseInstanceConfigs(ports) will always return empty lists. In that case we could exit early returning false.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't think it would be correct, if the file was removed we should remove the previous state as well

@csweichel
Copy link
Contributor Author

I've tested the PR with

# bind globally
curl lama.sh | sh
curl lama.sh | LAMA_PORT=9264 sh

# bind to localhost
curl lama.sh | sh -s -- -l
curl lama.sh | LAMA_PORT=9264 sh -s -- -l

all worked as intended.

@akosyakov akosyakov force-pushed the cw/supervisor-ports branch 5 times, most recently from 1e8a032 to 975cb50 Compare November 3, 2020 11:04
@akosyakov akosyakov force-pushed the cw/supervisor-ports branch from 975cb50 to 6924012 Compare November 3, 2020 11:39
@akosyakov akosyakov force-pushed the cw/supervisor-ports branch from 6924012 to 47968aa Compare November 3, 2020 12:44
@akosyakov
Copy link
Member

@csweichel I've addressed all your comments and retested everything. Could you please have a look again?

@csweichel
Copy link
Contributor Author

csweichel commented Nov 3, 2020

Thank you for the changes.
I've re-tested the PR and everything works well.

As I'm the original author of the PR, I cannot approve it.
But from my end, this is ✅

Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed and tested by me and Chris.

@akosyakov akosyakov merged commit 966c8e8 into master Nov 4, 2020
@akosyakov akosyakov deleted the cw/supervisor-ports branch November 4, 2020 04:50
vhermecz added a commit to vhermecz/website that referenced this pull request Jan 6, 2021
csweichel pushed a commit to gitpod-io/retired-gatsby-website that referenced this pull request Jan 7, 2021
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.

2 participants