[native] Replace fixed test worker port with ephemeral ports

[czentgr]

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]

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]

@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]

@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]

@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]

@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.

[czentgr]

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

[aditi-pandit]

@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]

@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]

Both @czentgr and @tdcmeehan hit this.
In Velox, we have PortUtil class https://github.com/facebookincubator/velox/blob/main/velox/exec/tests/utils/PortUtil.h that returns a free port. 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.

[majetideepak]

@amitkdutta Do you have any feedback on this change? Would you like anyone from Meta to review this?

[aditi-pandit]

@majetideepak @tdcmeehan : I liked the direction Christian had with the change. Was only hesitating since I hadn't heard more complaints about it. But if others are in agreement, then I'm alright as well.

[aditi-pandit]

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

[czentgr]

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.

[steveburnett]

LGTM! (docs)

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

[aditi-pandit]

Thanks @czentgr

[aditi-pandit]

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

[czentgr]

@amitkdutta @spershin Can you please take a look?