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.

Sign up for GitHub

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

Jump to bottom

[native] Replace fixed test worker port with ephemeral ports #22748

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[native] Replace fixed test worker port with ephemeral ports #22748

wants to merge 1 commit into from
+78 −42

Conversation

czentgr
Copy link
Contributor

@czentgr czentgr commented May 14, 2024 •
edited
Loading

Previously the listener ports for the native works in the E2E tests was hard coded to be 1234 + worker count.
The change looks in the OS for an available ephemeral port and uses this value when spawning the native workers.

Description

Motivation and Context

On my Mac I encountered problems running the E2E native tests. The worker was up and running and listened on the port. Yet for some reason the HTTP request timed out. The connection was set up but there was no response.

Connection could occur but each request was eaten. Changing the port to a different port resolved the issue.

$ curl -v http://127.0.0.1:1234/v1/info 
*   Trying 127.0.0.1:1234...
* Connected to 127.0.0.1 (127.0.0.1) port 1234
> GET /v1/info HTTP/1.1
> Host: 127.0.0.1:1234
> User-Agent: curl/8.4.0
> Accept: */*
>
^C

I20240514 17:32:42.203320 52237991 PrestoServer.cpp:260] [PRESTO_STARTUP] Starting server at :::1234 (127.0.0.1)

presto_se 52290 czentgr   44u    IPv6 0x630812116eb7c8e3       0t0      TCP *:search-agent (LISTEN)

and in the logs

2024-05-14T17:25:15.854-0500    WARN    node-state-poller-0     com.facebook.presto.metadata.HttpRemoteNodeState        Node state update request to http://127.0.0.1:1234/v1/info/state has not returned in 10.05s
...
2024-05-14T17:26:36.941-0500    WARN    UpdateResponseHandler-20240514_222407_00000_zb5hw.0.0.0.0-572   com.facebook.presto.server.RequestErrorTracker  Error updating task 20240514_222407_00000_zb5hw.0.0.0.0: java.util.concurrent.TimeoutException: Total timeout 10000 ms elapsed: http://127.0.0.1:1234/v1/task/20240514_222407_00000_zb5hw.0.0.0.0

continuously until the test case fails.

Impact

Test Plan

Contributor checklist

  • Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== NO RELEASE NOTE ==

@ZacBlanco
Copy link
Contributor

ZacBlanco commented May 14, 2024 •
edited
Loading

Just want to leave my $.02 here -- We had similar problems at my old job and we had attempted this type of solution to grab free worker ports. Ultimately it ended up being more reliable to pick a fixed port number that is usually not used by the OS for our E2E integration tests. This type of port selection didn't work because after releasing the socket back to the OS, we found a race condition occurred quite often where we would get assigned the same port back to back or in close succession before the port is actually allocated to the new server's socket.

I think in this case, since we don't launch workers in parallel, we probably won't run into this situation as often, but I do have a PR which parallelizes the launching that would probably cause issues (#22212). I think a better solution would let the worker bind to port 0, and then query the process internally for its assigned port once the socket is returned by the OS to the worker.

@czentgr
Copy link
Contributor Author

czentgr commented May 15, 2024

@ZacBlanco Thank you for your comment.
Yes, my assumption here is that the workers are sequentially launched (hence the comment in the code). If this occurs in parallel then this won't work reliably.

I also thought of the prestissimo side. We don't need to define a fixed port in the config. The worker will tell the coordinator how to reach it during the announcement. But not sure what would be needed for the HttpServer - we pass in the http/https config. I would need to look into it a bit more.

@czentgr czentgr changed the title [native] Replace fixed worker port with ephemeral ports [WIP][native] Replace fixed worker port with ephemeral ports May 15, 2024
@czentgr czentgr force-pushed the cz_auto_worker_port branch 4 times, most recently from 92c4482 to 751b140 Compare May 16, 2024 22:55
@czentgr czentgr changed the title [WIP][native] Replace fixed worker port with ephemeral ports [native] Replace fixed worker port with ephemeral ports Jun 7, 2024
@czentgr
Copy link
Contributor Author

czentgr commented Jun 7, 2024

@aditi-pandit FYI. I was thinking to add something to the C++ documentation about the http-server.port and http-server.https.port when they are set to 0. Doesn't look like anything exists in the Java side of the doc for these.

@aditi-pandit
Copy link
Contributor

@aditi-pandit FYI. I was thinking to add something to the C++ documentation about the http-server.port and http-server.https.port when they are set to 0. Doesn't look like anything exists in the Java side of the doc for these.

@czentgr : Didn't entirely follow the reference in the question ? What do you want to document about http-server.portandhttp-server.https.port` ?

@czentgr czentgr force-pushed the cz_auto_worker_port branch 3 times, most recently from f424676 to f835b46 Compare June 20, 2024 13:48
@czentgr czentgr marked this pull request as ready for review July 2, 2024 15:02
@czentgr czentgr requested a review from a team as a code owner July 2, 2024 15:02
@majetideepak majetideepak self-assigned this Jul 2, 2024
@czentgr czentgr force-pushed the cz_auto_worker_port branch 2 times, most recently from c1066d6 to eac7520 Compare July 18, 2024 15:35
@czentgr
Copy link
Contributor Author

czentgr commented Jul 18, 2024

@aditi-pandit I was asking about the documentation to say that you can set the port options in the config to 0 and the OS will pick a port automatically.
There is no currently no documentation on what happens if the port is set to 0 or if it is allowed to be set to 0.

@tdcmeehan tdcmeehan self-assigned this Sep 10, 2024
@czentgr
Copy link
Contributor Author

czentgr commented Sep 10, 2024

@majetideepak @aditi-pandit Do you think this can gain traction?

@aditi-pandit
Copy link
Contributor

@czentgr : I like the direction you are proposing in the fix. Though I don't really hear any complaints about the current behavior either. So I feel that we can skip it.

@tdcmeehan
Copy link
Contributor

@aditi-pandit I found myself in need of this behavior--just for example, running E2E tests and a query runner simultaneously would fail without picking random ports. If there's no objections, I'd like to get this change in.

majetideepak
majetideepak previously approved these changes Sep 12, 2024
View reviewed changes
aditi-pandit
aditi-pandit reviewed Sep 12, 2024
View reviewed changes
Copy link
Contributor

@aditi-pandit aditi-pandit left a comment

Choose a reason for hiding this comment

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

@czentgr : Please add documentation for the ephemeral port configuration.

@czentgr
Copy link
Contributor Author

czentgr commented Sep 12, 2024

But @czentgr mentioned that using the ephemeral port approach is more robust as the PortUtil approach can still have a race condition where different instances can get the same port but only one succeeds in using it.

We could use the PortUtil utility. If the user specifies http.port=0 and/or https.port=0 we could then call the port utility to provide two ports (these are also from the ephemeral pool) and use these for the announcer and task uri. But it is possible that these are in use by the time the http server comes up. The http server itself passes the port number (0) to the OS when creating the listener and the OS also picks an ephemeral port. It does the same thing the utility does with respect to how the ephemeral port is assigned except there is no time gap. This does then necessitate the change to defer creation of the announce message body and task uri where the ports are used to after the http server has been started.
in addition, the PortUtil is not usually built as part of Velox. The testing tools need to be enabled which is usually not how presto_server is built. So some refactoring at the Velox side would be necessary as well.

@czentgr czentgr changed the title [native] Replace fixed worker port with ephemeral ports [native] Replace fixed test worker port with ephemeral ports Sep 12, 2024
steveburnett
steveburnett previously approved these changes Sep 12, 2024
View reviewed changes
Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

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

LGTM! (docs)

Pull branch, local doc build, looks good. Thanks!

majetideepak
majetideepak previously approved these changes Sep 16, 2024
View reviewed changes
aditi-pandit
aditi-pandit previously approved these changes Sep 17, 2024
View reviewed changes
Copy link
Contributor

@aditi-pandit aditi-pandit left a comment

Choose a reason for hiding this comment

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

Thanks @czentgr

@aditi-pandit
Copy link
Contributor

@amitkdutta @spershin : Would be great to get Meta approval as well.

@czentgr
Copy link
Contributor Author

czentgr commented Oct 15, 2024

@amitkdutta @spershin Can you please take a look?

@amitkdutta
Copy link
Contributor

CC: @tangjiangling @xiaoxmeng

@czentgr
Copy link
Contributor Author

czentgr commented Nov 14, 2024

@amitkdutta @tangjiangling @xiaoxmeng Do you think you have comments on this. Or do you think we can proceed?

xiaoxmeng
xiaoxmeng reviewed Nov 22, 2024
View reviewed changes
Copy link
Contributor

@xiaoxmeng xiaoxmeng left a comment

Choose a reason for hiding this comment

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

@czentgr thanks for the change % comments.

presto-native-execution/presto_cpp/main/PrestoServer.cpp Outdated
} else {
taskUri = fmt::format(kTaskUriFormat, kHttp, address_, httpPort);
}
auto setTaskUri = [&](bool useHttps, int port) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move this to where it is first used in the code? thanks!

nit: s/setTaskUri/setTaskUriCb or setTaskUriFn/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm moving this (and the other lambda) before the httpServer start.

I'm thinking of moving the httpServer lambda into it's own function too if we have to move this inside because things get a bit less readable if we put these (longer) lambdas into the server lamdba as well.

presto-native-execution/presto_cpp/main/PrestoServer.cpp Outdated
uint64_t heartbeatFrequencyMs = systemConfig->heartbeatFrequencyMs();
if (heartbeatFrequencyMs > 0) {
heartbeatManager_ = std::make_unique<PeriodicHeartbeatManager>(
auto startAnnouncerAndHeartbeatManager = [&](bool useHttps, int port) {
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

nit: s/startAnnouncerAndHeartbeatManager/startAnnouncerAndHeartbeatManagerCb/

presto-native-execution/presto_cpp/main/PrestoServer.cpp Outdated
break;
}
if (coordinatorDiscoverer_ != nullptr) {
VELOX_CHECK(
Copy link
Contributor

Choose a reason for hiding this comment

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

VELOX_CHECK_NOT_NULL

presto-native-execution/presto_cpp/main/PrestoServer.cpp
setTaskUri(httpsPort.has_value(), address.address.getPort());
break;
}
if (coordinatorDiscoverer_ != nullptr) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Leave an empty line?

presto-native-execution/presto_cpp/main/PrestoServer.cpp Outdated
"The announcer is expected to have been created but wasn't.");
const auto heartbeatFrequencyMs = systemConfig->heartbeatFrequencyMs();
if (heartbeatFrequencyMs > 0) {
VELOX_CHECK(
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

presto-native-execution/presto_cpp/main/PrestoServer.cpp Outdated
uint64_t heartbeatFrequencyMs = systemConfig->heartbeatFrequencyMs();
if (heartbeatFrequencyMs > 0) {
heartbeatManager_ = std::make_unique<PeriodicHeartbeatManager>(
auto startAnnouncerAndHeartbeatManager = [&](bool useHttps, int port) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we need this change to support ephemeral port? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The announcer constructs the announcement message with the port for the coordinator when it is instantiated. This is the port then used by the coordinator to communicate to the worker. That means we have to construct the announcer after we know what port we need to use in the announcement message.

Similarly, the HeartbeatManager sets the address and port of the listener in the header of a request via HTTP_HEADER_HOST. And since the port is determined on listener start (if not specified) we have to construct it later and provide the known port at that time.

@facebook-github-bot
Copy link
Collaborator

@amitkdutta has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@amitkdutta
Copy link
Contributor

Same comments as @xiaoxmeng. Othewise imported internally to run e2e tests, looks good to me.

@czentgr
[native] Replace fixed test worker port with ephemeral ports
8f0eb84 
Previously the listener ports for the native works in the E2E tests
was hard coded to be 1234 + worker count.
The change looks in the OS for an available ephemeral port
and uses this value when spawning the native workers.

The native worker must is deferring some configuration until the
port selection by the OS is known.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xiaoxmeng xiaoxmeng xiaoxmeng left review comments

@aditi-pandit aditi-pandit aditi-pandit left review comments

@steveburnett steveburnett steveburnett left review comments

@majetideepak majetideepak majetideepak left review comments

@elharo elharo Awaiting requested review from elharo elharo is a code owner

@presto-oss presto-oss Awaiting requested review from presto-oss presto-oss is a code owner automatically assigned from prestodb/committers

@amitkdutta amitkdutta Awaiting requested review from amitkdutta

At least 1 approving review is required to merge this pull request.

Assignees

@majetideepak majetideepak

@tdcmeehan tdcmeehan

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@czentgr @ZacBlanco @aditi-pandit @tdcmeehan @majetideepak @amitkdutta @facebook-github-bot @steveburnett @xiaoxmeng