Skip to content

Commit

Permalink
network: use pyroute2 to manage default routes
Browse files Browse the repository at this point in the history
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
  • Loading branch information
dbungert committed Feb 18, 2023
1 parent 4266a5e commit c0297fa
Show file tree
Hide file tree
Showing 10 changed files with 157 additions and 39 deletions.
1 change: 1 addition & 0 deletions apt-deps.txt
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ python3-pyflakes
python3-pytest
python3-pytest-xdist
python3-pyudev
python3-pyroute2
python3-requests
python3-requests-unixsocket
python3-setuptools
Expand Down
1 change: 1 addition & 0 deletions snapcraft.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ parts:
- python3-minimal
- python3-oauthlib
- python3-pkg-resources
- python3-pyroute2
- python3-pyrsistent
- python3-pyudev
- python3-requests
Expand Down
6 changes: 3 additions & 3 deletions subiquity/client/controllers/network.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
import os
import shutil
import tempfile
from typing import List, Optional
from typing import Optional

from aiohttp import web

Expand Down Expand Up @@ -74,9 +74,9 @@ async def update_link_POST(self, act: LinkAction,
if act == LinkAction.DEL:
self.view.del_link(info)

async def route_watch_POST(self, routes: List[int]) -> None:
async def route_watch_POST(self, has_default_route: bool) -> None:
if self.view is not None:
self.view.update_default_routes(routes)
self.view.update_default_routes(has_default_route)

async def apply_starting_POST(self) -> None:
if self.view is not None:
Expand Down
2 changes: 1 addition & 1 deletion subiquity/common/apidef.py
Original file line number Diff line number Diff line change
Expand Up @@ -444,7 +444,7 @@ class update_link:
def POST(act: LinkAction, info: Payload[NetDevInfo]) -> None: ...

class route_watch:
def POST(routes: List[int]) -> None: ...
def POST(has_default_route: bool) -> None: ...

class apply_starting:
def POST() -> None: ...
Expand Down
1 change: 0 additions & 1 deletion subiquity/server/controllers/mirror.py
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,6 @@ async def find_and_elect_candidate_mirror(self, context):
pass

if not self.app.base_model.network.has_network:
log.debug("Skipping mirror check since network is not available.")
return

# Try each mirror one after another.
Expand Down
14 changes: 6 additions & 8 deletions subiquity/server/controllers/network.py
Original file line number Diff line number Diff line change
Expand Up @@ -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_routes

async def _apply_config(self, *, context=None, silent=False):
try:
Expand Down Expand Up @@ -284,8 +283,7 @@ async def GET(self) -> NetworkStatus:
wlan_support_install_state=self.wlan_support_install_state())

async def configured(self):
self.model.has_network = bool(
self.network_event_receiver.default_routes)
self.model.has_network = self.network_event_receiver.has_default_routes
self.model.needs_wpasupplicant = (
self.wlan_support_install_state() == WLANSupportInstallState.DONE)
await super().configured()
Expand All @@ -308,7 +306,7 @@ async def subscription_PUT(self, socket_path: str) -> None:
run_bg_task(
self._call_client(
client, conn, lock, "route_watch",
self.network_event_receiver.default_routes))
self.network_event_receiver.has_default_routes))

async def subscription_DELETE(self, socket_path: str) -> None:
if socket_path not in self.clients:
Expand Down Expand Up @@ -347,9 +345,9 @@ def apply_error(self, stage):
super().apply_error(stage)
self._call_clients("apply_error", stage)

def update_default_routes(self, routes):
super().update_default_routes(routes)
self._call_clients("route_watch", routes)
def update_default_routes(self, has_default_routes):
super().update_default_routes(has_default_routes)
self._call_clients("route_watch", has_default_routes)

def _send_update(self, act, dev):
with self.context.child(
Expand Down
64 changes: 41 additions & 23 deletions subiquitycore/controllers/network.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import subprocess
from typing import Optional

import pyroute2
import yaml

from probert.network import IFF_UP, NetworkEventReceiver
Expand Down Expand Up @@ -56,7 +57,7 @@ class SubiquityNetworkEventReceiver(NetworkEventReceiver):
def __init__(self, controller):
self.controller = controller
self.model = controller.model
self.default_routes = set()
self.has_default_routes = False

def new_link(self, ifindex, link):
netdev = self.model.new_link(ifindex, link)
Expand All @@ -65,9 +66,8 @@ def new_link(self, ifindex, link):

def del_link(self, ifindex):
netdev = self.model.del_link(ifindex)
if ifindex in self.default_routes:
self.default_routes.remove(ifindex)
self.controller.update_default_routes(self.default_routes)
self.probe_default_routes()
self.controller.update_default_routes(self.has_default_routes)
if netdev is not None:
self.controller.del_link(netdev)

Expand All @@ -76,9 +76,9 @@ def update_link(self, ifindex):
if netdev is None:
return
flags = getattr(netdev.info, "flags", 0)
if not (flags & IFF_UP) and ifindex in self.default_routes:
self.default_routes.remove(ifindex)
self.controller.update_default_routes(self.default_routes)
if not (flags & IFF_UP):
self.probe_default_routes()
self.controller.update_default_routes(self.has_default_routes)
self.controller.update_link(netdev)

def route_change(self, action, data):
Expand All @@ -87,13 +87,31 @@ def route_change(self, action, data):
return
if data['table'] != 254:
return
ifindex = data['ifindex']
if action == "NEW" or action == "CHANGE":
self.default_routes.add(ifindex)
elif action == "DEL" and ifindex in self.default_routes:
self.default_routes.remove(ifindex)
log.debug('default routes %s', self.default_routes)
self.controller.update_default_routes(self.default_routes)
self.probe_default_routes()
self.controller.update_default_routes(self.has_default_routes)

def _analyze_routes(self, routes):
return any(route['table'] == 254 and not route['dst']
for route in routes)

def probe_default_routes(self):
with pyroute2.NDB() as ndb:
self.has_default_routes = self._analyze_routes(ndb.routes)
log.debug('default routes %s', self.has_default_routes)

@staticmethod
def create(controller: BaseController, dry_run: bool) \
-> "SubiquityNetworkEventReceiver":
if dry_run:
return DryRunSubiquityNetworkEventReceiver(controller)
else:
return SubiquityNetworkEventReceiver(controller)


class DryRunSubiquityNetworkEventReceiver(SubiquityNetworkEventReceiver):
def probe_default_routes(self):
self.has_default_routes = True
log.debug('dryrun default routes %s', self.has_default_routes)


default_netplan = '''
Expand Down Expand Up @@ -149,7 +167,8 @@ def __init__(self, app):
self.parse_netplan_configs()

self._watching = False
self.network_event_receiver = SubiquityNetworkEventReceiver(self)
self.network_event_receiver = \
SubiquityNetworkEventReceiver.create(self, self.opts.dry_run)

def parse_netplan_configs(self):
self.model.parse_netplan_configs(self.root)
Expand Down Expand Up @@ -485,8 +504,8 @@ def apply_error(self, stage):
pass

@abc.abstractmethod
def update_default_routes(self, routes):
if routes:
def update_default_routes(self, has_default_routes):
if has_default_routes:
self.app.hub.broadcast(CoreChannels.NETWORK_UP)

@abc.abstractmethod
Expand Down Expand Up @@ -613,16 +632,15 @@ def make_ui(self):
self.apply_config(silent=True)
self.view_shown = True
self.view.update_default_routes(
self.network_event_receiver.default_routes)
self.network_event_receiver.has_default_routes)
return self.view

def end_ui(self):
self.view = None

def done(self):
log.debug("NetworkController.done next_screen")
self.model.has_network = bool(
self.network_event_receiver.default_routes)
self.model.has_network = self.network_event_receiver.has_default_routes
self.app.next_screen()

def cancel(self):
Expand All @@ -643,10 +661,10 @@ def apply_error(self, stage):
if self.view is not None:
self.view.show_network_error(stage)

def update_default_routes(self, routes):
super().update_default_routes(routes)
def update_default_routes(self, has_default_routes):
super().update_default_routes(has_default_routes)
if self.view:
self.view.update_default_routes(routes)
self.view.update_default_routes(has_default_routes)

def new_link(self, netdev):
super().new_link(netdev)
Expand Down
14 changes: 14 additions & 0 deletions subiquitycore/controllers/tests/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
# Copyright 2015 Canonical, Ltd.
#
# This program is free software: you can redistribute it and/or modify
# it under the terms of the GNU Affero General Public License as
# published by the Free Software Foundation, either version 3 of the
# License, or (at your option) any later version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
# GNU Affero General Public License for more details.
#
# You should have received a copy of the GNU Affero General Public License
# along with this program. If not, see <http://www.gnu.org/licenses/>.
87 changes: 87 additions & 0 deletions subiquitycore/controllers/tests/test_network.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
# Copyright 2023 Canonical, Ltd.
#
# This program is free software: you can redistribute it and/or modify
# it under the terms of the GNU Affero General Public License as
# published by the Free Software Foundation, either version 3 of the
# License, or (at your option) any later version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
# GNU Affero General Public License for more details.
#
# You should have received a copy of the GNU Affero General Public License
# along with this program. If not, see <http://www.gnu.org/licenses/>.

import unittest
from unittest.mock import Mock

from subiquitycore.controllers.network import SubiquityNetworkEventReceiver


class TestRoutes(unittest.IsolatedAsyncioTestCase):
def setUp(self):
self.er = SubiquityNetworkEventReceiver(Mock())

def test_empty(self):
self.assertFalse(self.er._analyze_routes([]))

def test_one_good(self):
routes = [{
"target": "localhost",
"tflags": 0,
"table": 254,
"ifname": "ens3",
"dst": "",
"dst_len": 0,
"gateway": "10.0.2.2"
}]

self.assertTrue(self.er._analyze_routes(routes))

def test_one_good_one_other(self):
routes = [{
"target": "localhost",
"tflags": 0,
"table": 254,
"ifname": "ens3",
"dst": "",
"dst_len": 0,
"gateway": "10.0.2.2"
}, {
"target": "localhost",
"tflags": 0,
"table": 254,
"ifname": "ens3",
"dst": "10.0.2.0",
"dst_len": 24,
"gateway": None
}]

self.assertTrue(self.er._analyze_routes(routes))

def test_one_other(self):
routes = [{
"target": "localhost",
"tflags": 0,
"table": 254,
"ifname": "ens3",
"dst": "10.0.2.0",
"dst_len": 24,
"gateway": None
}]

self.assertFalse(self.er._analyze_routes(routes))

def test_wrong_table(self):
routes = [{
"target": "localhost",
"tflags": 0,
"table": 255,
"ifname": "ens3",
"dst": "",
"dst_len": 0,
"gateway": "10.0.2.2"
}]

self.assertFalse(self.er._analyze_routes(routes))
6 changes: 3 additions & 3 deletions subiquitycore/ui/views/network.py
Original file line number Diff line number Diff line change
Expand Up @@ -324,9 +324,9 @@ def _action(self, sender, action, netdev_table):
dev_info = netdev_table.dev_info
meth("{}/{}".format(dev_info.name, action.name), dev_info)

def update_default_routes(self, routes):
log.debug('view route_watcher %s', routes)
if routes:
def update_default_routes(self, has_default_routes):
log.debug('view route_watcher %s', has_default_routes)
if has_default_routes:
label = _("Done")
else:
label = _("Continue without network")
Expand Down

0 comments on commit c0297fa

Please sign in to comment.