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

Fix GetNodeIP to get the first ip #982

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix GetNodeIP to get the first ip #982

wants to merge 2 commits into from

Conversation

lrq619
Copy link
Collaborator

@lrq619 lrq619 commented May 8, 2024

Summary

Change the bash command for getting node's private ip address
Now it would get the first ip address starting with 10.

Implementation Notes ⚒️

Change bash command implementation to:

ip route | awk '{print $(NF)}' | awk '/^10\..*/ {print; exit}'

External Dependencies 🍀

  • N/A

Breaking API Changes ⚠️

  • N/A

Simply specify none (N/A) if not applicable.

@lrq619 lrq619 requested a review from leokondrashov May 8, 2024 08:12
@lrq619 lrq619 added the bug Something isn't working label May 8, 2024
@leokondrashov
Copy link
Contributor

Did you check it for operation in the case of two addresses in the subnet?

We should maybe try to let the user choose the IP out of the available ones, as was proposed in the issue.

@lrq619
Copy link
Collaborator Author

lrq619 commented May 9, 2024

Did you check it for operation in the case of two addresses in the subnet?

We should maybe try to let the user choose the IP out of the available ones, as was proposed in the issue.

Tested by replacing ip route to cat route_ip.txt, and change the contents in the txt file to simulate enviroments with multiple ip starting from 10.

Enabling user choosing NodeIP is good, but I haven't come up with any ideas.
Most naive one would be pop out a prompt and let user input an ip_address, but I don't think adding such interaction operation would be good in a setup script.It would block there if no input is sent, and this setup script sometimes need to be run by automatical workflows like github actions instead of real person)

Do you have any suggestion?

@lrq619
Copy link
Collaborator Author

lrq619 commented May 9, 2024

@JooyoungPark73 I think the gvisor runner is failing again, could you please restart it? Thank you

@yulinzou
Copy link
Contributor

yulinzou commented May 17, 2024

Solving this issue, added error handling when ip route | awk '{print $(NF)}' | awk '/^10\..*/ {print; exit}' return empty IP, will fallback to get from hostname -I

Signed-off-by: lrq619 <lrq619@outlook.com>
Signed-off-by: Zou Yulin <yulin.zou@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't consider the situation where there are two IPs starting with '10'.
3 participants