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

Update ElasticManager constructor auto IP config #190

Merged
merged 2 commits into from
Jun 30, 2023
Merged

Update ElasticManager constructor auto IP config #190

merged 2 commits into from
Jun 30, 2023

Conversation

mikeingold
Copy link
Contributor

Ref discussion at JuliaLang/julia#189

This PR updates the ElasticManager constructor to enable support for automatic IP address detection on Windows (and potentially other platforms that aren't macOS or Linux). Prior to this change, a custom get_private_ip() function is used to detect the platform and execute system cmds to look for the machine's IP address. These changes obsolete that function in favor of a call to Sockets.getipaddr(), a utility that was not available until the v1.2 release of Julia.

It's possible for Sockets.getipaddr() to throw an error if no network interfaces are detected, so I've implemented a try-catch to generate a helpful error message, suggesting the IP address be specified manually. This message is largely derived from the prior get_private_ip() error message.

I tested this modified branch on an up-to-date Windows 11 machine and verified that it correctly resolved the local IP address with addr=:auto, where the prior version would simply throw an error that the feature is only supported on Mac/Linux. I've also tested it on an up-to-date Mac Mini M1 and it also correctly identified the local IP address and seems to work fine.

Pros:

  • Enables support for ElasticManagers automatic IP address option in Windows and possibly other platforms that Julia supports.
  • Reduces the potential for bugs/errors that could be caused by running cmds and parsing the returns.

Cons:

  • Requires the compat minimum for ClusterManagers.jl to be bumped from 1 to at least 1.2.

@kescobo
Copy link
Collaborator

kescobo commented Jun 12, 2023

As I said in the issue, I'm definitely in favor of this. Presumably anyone in a restrictive with enough environment that they can't update Julia also isn't going to be updating this package to take advantage of this change. If there are no objections, I'll merge at the end of the week (please feel free to bump if/when I forget)

@mikeingold
Copy link
Contributor Author

Just realized that I should also have updated the README to clarify that this is no longer OS-specific functionality. Working it now...

@mikeingold
Copy link
Contributor Author

Done.

@Moelf
Copy link
Collaborator

Moelf commented Jun 16, 2023

btw regarding the interface issue, I think Julia needs to fix this:

@kescobo
Copy link
Collaborator

kescobo commented Jun 16, 2023

@Moelf Does that affect whether or not to merge this, or just an upstream fix that would make this unnecessary?

@Moelf
Copy link
Collaborator

Moelf commented Jun 16, 2023

this PR uses getipaddr() but I'm saying that function can return result that is within the range of reserved private IP, and we should probably filter it to make this PR more robust

@mikeingold
Copy link
Contributor Author

I'm trying to understand the linked issue, but it doesn't seem like I'm running any systems/configurations susceptible to it. The prior solution was a get_private_ip() function that runs an external shell Cmd, e.g. hostname -i in Linux, and appears to simply accept the first IP returned. The Sockets.getipaddr version farms out to Sockets.getipaddrs, which seems to attempt the same thing but by instead using a ccall to libuv(?). It seems like the only stage at which filtering is even possible is within one of these external functions since they return only the single IP. Barring that, the only obvious solutions seem to be: (1) spinning our own variant, which was the status quo, or (2) contribute improvements to the Sockets functions and then merge some version of this PR. Without access to a system that experiences the referenced issue, I'm not sure how much help I can offer with the latter.

@Moelf
Copy link
Collaborator

Moelf commented Jun 20, 2023

I don't mind merging this PR

@mikeingold
Copy link
Contributor Author

@kescobo I'm good with moving forward on the merge if no one else objects.

@kescobo kescobo merged commit c665258 into JuliaParallel:master Jun 30, 2023
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.

3 participants