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

Merged
merged 2 commits into from
Jun 3, 2024
Merged

Fix GetNodeIP to get the first ip #982

merged 2 commits into from
Jun 3, 2024

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 lrq619 linked an issue May 9, 2024 that may be closed by this pull request
@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>
@lrq619
Copy link
Collaborator Author

lrq619 commented Jun 3, 2024

@leokondrashov Hi, could you please review this branch so we can get this branch merged?

Copy link
Contributor

@leokondrashov leokondrashov left a comment

Choose a reason for hiding this comment

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

The command works correctly on CloudLab and CCDS cloud. Can merge

@lrq619 lrq619 merged commit 4dccf25 into main Jun 3, 2024
23 checks passed
@lrq619 lrq619 deleted the fix_get_node_ip branch June 3, 2024 06:49
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