network: use pyroute2 to manage default routes #1569
Conversation
dbungert
force-pushed
the
lp-2004659-has-network-try2
branch
from
c0297fa
to
9c185bd
Compare
subiquitycore/controllers/network.py
Outdated
|
|
||
| def _analyze_routes(self, routes): | ||
| return any(route['table'] == 254 and not route['dst'] | ||
| for route in routes) |
There was a problem hiding this comment.
I think we should consider rejecting routes with metrics > 20000 here, to filter out connections that have not passed NM's connectivity check (unless there is a expectation that such routes might be created by some other mechanism? It seems very unlikely in the context of subiquity though?)
There was a problem hiding this comment.
I think we should, yes. These high-metric routes can stick around. I think it sensible to filter them. Implemented.
dbungert
force-pushed
the
lp-2004659-has-network-try2
branch
from
9c185bd
to
e36fedd
Compare
Sure, we are already changing the type so I think changing the name is appropriate. Implemented. |
The existing event based method of watching for has_network has a flaw. The incoming route_change events from probert do not distinguish routes on the same interface but a different metric, so if 2 routes on one interface appear, we only get one event. Then if one of those routes is removed, we will inappropriately remove this route from the default_routes list. Aside from the code watching the event stream, the set of default routes is an elaborate boolean value. Simplify the code by passing around a boolean, and when we get a route_change event, use that to go looking again at the list of default routes. LP: #2004659
Network manager can create routes at metric aka priority above 20000. These can stick around if they are not the best choice, or they may disappear quickly. Do not consider one of these routes as a valid default route for has_network purposes.
dbungert
force-pushed
the
lp-2004659-has-network-try2
branch
from
e36fedd
to
69bb830
Compare
| @@ -246,8 +246,7 @@ async def apply_autoinstall_config(self, context): | |||
| self.apply_config(context) | |||
| with context.child("wait_for_apply"): | |||
| await self.apply_config_task.wait() | |||
| self.model.has_network = bool( | |||
| self.network_event_receiver.default_routes) | |||
| self.model.has_network = self.network_event_receiver.has_default_route | |||
There was a problem hiding this comment.
100% nothing to do with this branch but I think this line is pointless given that configured does the same and is called immediately after apply_autoinstall_config?
The existing event based method of watching for has_network has a flaw.
The incoming route_change events from probert do not distinguish routes on the same interface but a different metric, so if 2 routes on one interface appear, we only get one event. Then if one of those routes is removed, we will inappropriately remove this route from the default_routes list.
Aside from the code watching the event stream, the set of default routes is an elaborate boolean value.
Simplify the code by passing around a boolean, and when we get a route_change event, use that to go looking again at the list of default routes.
LP: #2004659