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

added address resolution fix for running in docker containers #11944

Merged
merged 3 commits into from
Nov 24, 2020
Merged

added address resolution fix for running in docker containers #11944

merged 3 commits into from
Nov 24, 2020

Conversation

roireshef
Copy link
Contributor

This PR fixes #11943
which is a bug in address resolution when running in a docker container

Related issue number

#11943

Checks

  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

@dHannasch dHannasch mentioned this pull request Nov 11, 2020
Merged
6 tasks
dHannasch
dHannasch reviewed Nov 12, 2020
View reviewed changes
python/ray/_private/services.py
@@ -186,7 +186,8 @@ def get_address_info_from_redis_helper(redis_address,
client_node_ip_address = client_info["NodeManagerAddress"]
if (client_node_ip_address == node_ip_address
or (client_node_ip_address == "127.0.0.1"
and redis_ip_address == get_node_ip_address())):
and redis_ip_address == get_node_ip_address())
Copy link
Contributor

@dHannasch dHannasch Nov 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Come to think of it, do we actually want get_node_ip_address(redis_address) here rather than leaving the argument as the default 8.8.8.8? If the GlobalState will be reporting the IP addresses that it sees from the perspective of redis_address (which might not always be the same as the view from 8.8.8.8 Google DNS)?

ijrsvt
ijrsvt previously requested changes Nov 12, 2020
View reviewed changes
Copy link
Contributor

@ijrsvt ijrsvt left a comment

Choose a reason for hiding this comment

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

Could you also make a change in this file:

ray/java/runtime/src/main/java/io/ray/runtime/RayNativeRuntime.java

Line 94 in 62c7ab5

// Keep this method logic in sync with `services.get_address_info_from_redis_helper`

@ijrsvt ijrsvt assigned ericl and ijrsvt and unassigned ijrsvt Nov 12, 2020
@roireshef
Copy link
Contributor Author

roireshef commented Nov 12, 2020 •
edited
Loading

@ijrsvt In response to #11944 (review)
I modified the Java version as well, but it's been few good years since I coded Java and I don't have a Java IDE on my laptop, so I did that in a text editor. Please verify correctness...

@ericl
Copy link
Contributor

ericl commented Nov 13, 2020

Looks good, but seems to be causing Java test failures still.

@ericl ericl added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Nov 13, 2020
@roireshef
Copy link
Contributor Author

Looks good, but seems to be causing Java test failures still.

I noticed the IF conditions do not match exactly between python and Java, even before my change. Can this be related?

@roireshef
Copy link
Contributor Author

@ericl, @fyrestone, @rkooo567 - could any of you guys assist here? I'm not sure how to proceed further...

@ericl
Copy link
Contributor

ericl commented Nov 19, 2020

@raulchen do you know someone familiar with the Java failure in this case?

@dHannasch dHannasch mentioned this pull request Nov 20, 2020
Closed
6 tasks
@ericl ericl removed the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Nov 23, 2020
@ericl
Copy link
Contributor

ericl commented Nov 23, 2020

Reverting the Java change for now.

@ericl ericl merged commit 888357d into ray-project:master Nov 24, 2020
ConeyLiu
ConeyLiu reviewed May 25, 2021
View reviewed changes
python/ray/_private/services.py
@@ -186,7 +186,8 @@ def get_address_info_from_redis_helper(redis_address,
client_node_ip_address = client_info["NodeManagerAddress"]
if (client_node_ip_address == node_ip_address
or (client_node_ip_address == "127.0.0.1"
and redis_ip_address == get_node_ip_address())):
and redis_ip_address == get_node_ip_address())
or client_node_ip_address == redis_ip_address):
Copy link
Contributor

Choose a reason for hiding this comment

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

If we call ray.init from worker node (not head node), this will return the wrong address info. We should return the given worker node address info not the head node address info. @roireshef @ericl

This was referenced May 25, 2021
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ConeyLiu ConeyLiu ConeyLiu left review comments

@dHannasch dHannasch dHannasch left review comments

@ericl ericl ericl approved these changes

@ijrsvt ijrsvt ijrsvt left review comments

Assignees

@ericl ericl

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Failure to connect to Redis when running in a docker container
5 participants
@roireshef @ericl @ConeyLiu @dHannasch @ijrsvt