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

Allow user to change network interface prioprity #1836

Merged
merged 12 commits into from
Jul 4, 2023

Conversation

patrickelectric
Copy link
Member

@patrickelectric patrickelectric commented Jun 28, 2023

Adds a globe icon to show that internet access is available:

image

The same icon can be used to control network interface priority.

image

So.. The problem is that network priority is done by network route and not network and that makes things more confuse to show to the user.

root@blueos:~# route -n
Kernel IP routing table
Destination     Gateway         Genmask         Flags Metric Ref    Use Iface
0.0.0.0         192.168.0.1     0.0.0.0         UG    202    0        0 eth0
0.0.0.0         192.168.86.1    0.0.0.0         UG    304    0        0 wlan0
172.18.0.0      0.0.0.0         255.255.0.0     U     0      0        0 docker0
192.168.0.0     0.0.0.0         255.255.255.0   U     202    0        0 eth0
192.168.2.0     0.0.0.0         255.255.255.0   U     0      0        0 eth0
192.168.3.0     0.0.0.0         255.255.255.0   U     0      0        0 usb0
192.168.86.0    0.0.0.0         255.255.255.0   U     304    0        0 wlan0
192.168.196.0   0.0.0.0         255.255.255.0   U     0      0        0 ztyxasoevg

So, a single interface can have multiple destinations with different priorities.
We could do the same thing as ifmetric and change the network priority for all routes.

root@blueos:~# ifmetric eth0 303
root@blueos:~# route -n
Kernel IP routing table
Destination     Gateway         Genmask         Flags Metric Ref    Use Iface
0.0.0.0         192.168.0.1     0.0.0.0         UG    303    0        0 eth0
0.0.0.0         192.168.86.1    0.0.0.0         UG    304    0        0 wlan0
172.18.0.0      0.0.0.0         255.255.0.0     U     0      0        0 docker0
192.168.0.0     0.0.0.0         255.255.255.0   U     303    0        0 eth0
192.168.2.0     0.0.0.0         255.255.255.0   U     303    0        0 eth0
192.168.3.0     0.0.0.0         255.255.255.0   U     0      0        0 usb0
192.168.86.0    0.0.0.0         255.255.255.0   U     304    0        0 wlan0
192.168.196.0   0.0.0.0         255.255.255.0   U     0      0        0 ztyxasoevg

Is that correct ? How should we display the order based on the original version ?

In the end I'm changing the priority of the entire interface to the highest value + 1;

Fix #1539
Fix #1057

@rafaellehmkuhl
Copy link
Member

Does the wireless interface appear on the Ethernet menu after this change?

@patrickelectric
Copy link
Member Author

Does the wireless interface appear on the Ethernet menu after this change?

No, there's a new endpoint for all interfaces now.

@ES-Alexander ES-Alexander added the docs-needed Change needs to be documented label Jun 29, 2023
@patrickelectric patrickelectric force-pushed the internet-icon branch 2 times, most recently from 5c55bfa to 66b0783 Compare June 29, 2023 17:15
Copy link
Member

@joaoantoniocardoso joaoantoniocardoso left a comment

Choose a reason for hiding this comment

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

Some observations:

  • Sometimes "Apply" does nothing to the route metrics.
  • When "Apply" works, it always changes the value of the routes (maybe not all routes), even if the order is kept the same.
  • Sometimes the order of the list is changed to the inverse of what is applied: I switch wlan0 to the second, then if I open the dialogue again, it's the first.
  • The colors are very confusing to me, I have to think a lot to mentally keep track of what's happening when I've switched the position of two items. I recommend not using different colors, as the meaning should be the same as the written position. I think my little brain just expects the colors to mean each interface, not each position.
  • Ipv6 metrics are not following Ipv4 metrics changes.

core/tools/cable_guy/bootstrap.sh Outdated Show resolved Hide resolved
@patrickelectric patrickelectric force-pushed the internet-icon branch 7 times, most recently from 7c5a910 to 3f29107 Compare July 3, 2023 23:12
Copy link
Member

@joaoantoniocardoso joaoantoniocardoso left a comment

Choose a reason for hiding this comment

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

From the previous observations, only one is still unfixed:

  • Ipv6 metrics are not following Ipv4 metrics changes.

@patrickelectric
Copy link
Member Author

From the previous observations, only one is still unfixed:

  • Ipv6 metrics are not following Ipv4 metrics changes.

It should be working now, isn't it ?

@joaoantoniocardoso
Copy link
Member

From the previous observations, only one is still unfixed:

  • Ipv6 metrics are not following Ipv4 metrics changes.

It should be working now, isn't it ?

nope, Ipv6 metrics are kept unchanged :(

@patrickelectric
Copy link
Member Author

patrickelectric commented Jul 4, 2023

From the previous observations, only one is still unfixed:

  • Ipv6 metrics are not following Ipv4 metrics changes.

It should be working now, isn't it ?

nope, Ipv6 metrics are kept unchanged :(

I believe that is a problem with the ifmetric and not with this implementation, since we do not support IPv6 around our code base and this already fix the main issue, it should not be a holder.

Copy link
Member

@joaoantoniocardoso joaoantoniocardoso left a comment

Choose a reason for hiding this comment

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

From the previous observations, only one is still unfixed:

  • Ipv6 metrics are not following Ipv4 metrics changes.

It should be working now, isn't it ?

nope, Ipv6 metrics are kept unchanged :(

I believe that is a problem with the ifmetric and not with this implementation, since we do not support IPv6 around our code base and this already fix the main issue, it should not be a holder.

I see, then just so we are aware, #1539 might not be solved for all the cases.

@joaoantoniocardoso
Copy link
Member

Just to be clear, the metrics changes can be done using ip, ifmetric is just a helper for ipv4.

@joaoantoniocardoso
Copy link
Member

And there's this fork, apparently with ipv6 support.

@patrickelectric patrickelectric merged commit 1e76676 into bluerobotics:master Jul 4, 2023
@ES-Alexander ES-Alexander added docs-complete Change documentation has been completed and removed docs-needed Change needs to be documented labels Aug 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-complete Change documentation has been completed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow changing network priority Tell user if he has internet connection or not (somewhere)
4 participants