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

Move -epmd_module and -erl_epmd_port into kernel parameters #8671

Merged
merged 8 commits into from
Sep 26, 2024

Conversation

jonatanklosko
Copy link
Contributor

This introduces epmd_module and erl_epmd_listen_port parameters for the kernel application. I updated the docs to deprecate -epmd_module and -erl_epmd_port, similarly to -connect_all in 1d0fe6f.

The kernel application already has a lot of parameters related to distribution, so I believe semantically this makes sense.

For context, in Livebook we started using a custom epmd_module. In Escript and Elixir releases, there are ways to set the -epmd_module flag, however there is no way to do that in development, other than setting ERL_AFLAGS env var. We already start distribution manually using net_kernel:start/2, so the idea is that now we could do application:set_env(kernel, epmd_module, Module) right before that.

@CLAassistant
Copy link

CLAassistant commented Jul 15, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

github-actions bot commented Jul 15, 2024

CT Test Results

    5 files    206 suites   1h 58m 18s ⏱️
3 078 tests 2 791 ✅ 287 💤 0 ❌
4 002 runs  3 657 ✅ 345 💤 0 ❌

Results for commit 1714f2f.

♻️ This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

// Erlang/OTP Github Action Bot

_:_ ->
case net_kernel:epmd_module() of
erl_epmd ->
case erl_epmd:listen_port_please(nonode, nohost) of
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note the fake nonode, nohost, those arguments are ignored by erl_epmd:listen_port_please/2, and this way we use the public API. Alternatively we could expose erl_epmd:erl_epmd_listen_port/0 and make it -doc false.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could also expose it as net_kernel:epmd_listen_port, to mirror net_kernel:epmd_module.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

epmd_module is a broader config, while erl_epmd_listen_port is specific to erl_epmd, as per the other comment.

@jonatanklosko
Copy link
Contributor Author

jonatanklosko commented Jul 16, 2024

A follow up question, if this change is accepted, would it be possible to have it in the next maintenance release? If yes, I will change the target branch, and then I can submit a separate PR altering the -erl_epmd_port 0 behaviour (if you agree that it's desired).

@jhogberg jhogberg added the stalled waiting for input by the Erlang/OTP team label Jul 17, 2024
@jhogberg
Copy link
Contributor

Thanks for the PR! I think we'll want to discuss this a bit more internally before merging, and most of us are on vacation right now. We'll revisit this later in the summer/early autumn. :-)

@jonatanklosko
Copy link
Contributor Author

@jhogberg sounds good, thanks for the heads up! :)

@rickard-green rickard-green added the team:VM Assigned to OTP team VM label Aug 5, 2024
@jhogberg jhogberg self-assigned this Aug 5, 2024
Copy link
Contributor

@kikofernandez kikofernandez left a comment

Choose a reason for hiding this comment

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

The comments are from the OTP team (not just me, so credit to them for this review)

Please, address the issues mentioned and feel free to rebase on top of maint.

lib/kernel/src/erl_epmd.erl Outdated Show resolved Hide resolved
erts/preloaded/src/init.erl Show resolved Hide resolved
lib/kernel/doc/kernel_app.md Outdated Show resolved Hide resolved
lib/kernel/src/erl_epmd.erl Show resolved Hide resolved
lib/kernel/src/erl_epmd.erl Outdated Show resolved Hide resolved
lib/kernel/src/net_kernel.erl Outdated Show resolved Hide resolved
lib/kernel/src/net_kernel.erl Outdated Show resolved Hide resolved
@jonatanklosko jonatanklosko changed the base branch from master to maint August 8, 2024 14:36
@kikofernandez kikofernandez removed the stalled waiting for input by the Erlang/OTP team label Aug 12, 2024
@jhogberg jhogberg added the testing currently being tested, tag is used by OTP internal CI label Aug 26, 2024
@garazdawi garazdawi removed the testing currently being tested, tag is used by OTP internal CI label Aug 26, 2024
@kikofernandez
Copy link
Contributor

If there is nothing left to fix, I will merge this next Monday (some of us are in a conference this week and I do not want to rush the merging before having a really final good look at it :) )

@jonatanklosko
Copy link
Contributor Author

@kikofernandez sounds good, thanks :)

@jonatanklosko
Copy link
Contributor Author

@kikofernandez kindly ping :)

@kikofernandez
Copy link
Contributor

We have a code freeze for 27.1. I will merge as soon as I am given the OK.
This change was not breaking backwards compatibility, so we can merge in maint and release with OTP 27.2

@kikofernandez kikofernandez added the testing currently being tested, tag is used by OTP internal CI label Sep 25, 2024
@kikofernandez
Copy link
Contributor

Merging tomorrow if no internal tests break

@kikofernandez kikofernandez merged commit 3e7f5d1 into erlang:maint Sep 26, 2024
19 checks passed
@jonatanklosko jonatanklosko deleted the jk-epmd-config branch September 26, 2024 08:45
@jonatanklosko
Copy link
Contributor Author

@kikofernandez thank you :)

@kikofernandez kikofernandez removed the testing currently being tested, tag is used by OTP internal CI label Sep 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team:VM Assigned to OTP team VM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants