Merge lp:~allenap/maas/node-networking-api-get-mac-addresses-and-interfaces into lp:~maas-committers/maas/trunk

[Gavin Panella]

The beginnings of maasserver.networking, a new API for working with networking in MAAS.

Right now this is a place to put code that relates to the new networking model in MAAS. The maze of code relating to the existing/old networking model can be left mostly alone, and later removed or refactored to use this API. Right now the only member of this API is NodeNetworking.

[Blake Rouse (blake-rouse)]

I am confused? Why not place this in InterfaceManager? That is new and is only use by the new model code. This really parts from the way we have implemented models.

review: Needs Information

[Blake Rouse (blake-rouse)]

Speaking of this InterfaceManager already has a get_interfaces_for_node. Which looks like it needs to be improved as your code gets all the nested interfaces. Makes me wonder if you would have placed this in the InterfaceManager you would have seen that it already exists.

[Mike Pontillo (mpontillo)]

I think we can make a good argument that models/node.py (at ~2300 lines) is too big, and responsible for a too much. (just scanning through, node.py is responsible for start/stop/release of nodes, node storage, node networking, and node DHCP mappings.) So I like the concept of a separate layer for networking, and I think we should consider whether other functionality should be moved out of the model. (Do we need a NodeStorage and NodeStateMachine class as well?)

There are never-ending arguments in software design about whether you want an "anemic" model (containing little functionality other than what is needed to map the object to a database row), so you can whatever objects you want and never worry about side effects during unit testing. Or if you want a rich model, where methods on the Node object could reach far beyond the Node, for example by creating related Interface, MACAddress, or even Subnet objects. (and thus adding side effects for those trying to unit test a Node in isolation!)

However, in this particular case (given a Node object) I think I should be able to say node.get_interfaces() and node.get_mac_addresses() without the need for an extra layer.

I spoke with you on IRC about this, and you said we could have the methods in Node turn around and call this interface. I think that would be fine (and especially useful as we move from the old model to the new model, as we could test the operations we need independently, and then switch over to the new model by changing the shim layer), but let's do that before we land this.

Also, I have some minor comments/questions below.

Sorry for the wall of text. I've been spending way too much time thinking about this MP. ;-)

review: Needs Fixing

[Gavin Panella (allenap)]

You're quite right, but it's Node.get_interfaces(). I've reimplemented it almost exactly! It has the recursive query too. Serves me right for wandering off on my own.

My original intent was to implement some of the new networking API where it's distinct from the old one. There's so much code in the models' modules, loose and on the managers and model classes themselves: I've long wanted to peel off some of the controller-like code and my temptation got the better of me today.

Part of my problem is that I've not been involved in this feature, during design or implementation, and I'm trying to pick it up in drips between other work. Separation was meant to make the thought process clearer.

Anyway, I'll rescind this and think again.

Unmerged revisions

4205. By Gavin Panella on 2015-08-18

Docstrings.

4204. By Gavin Panella on 2015-08-18

get_interfaces() works with nested interfaces.

4203. By Gavin Panella on 2015-08-18

NodeNetworking.get_mac_addresses() and NodeNetworking.get_interfaces().

4202. By Gavin Panella on 2015-08-18

Start of a maasserver/networking package.

=== added directory 'src/maasserver/networking'
=== added file 'src/maasserver/networking/__init__.py'
--- src/maasserver/networking/__init__.py	1970-01-01 00:00:00 +0000
+++ src/maasserver/networking/__init__.py	2015-08-18 17:11:19 +0000
@@ -0,0 +1,62 @@
+# Copyright 2012-2015 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""MAAS Networking."""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+str = None
+
+__metaclass__ = type
+__all__ = [
+    "NodeNetworking",
+]
+
+from textwrap import dedent
+
+from maasserver.models.interface import Interface
+
+
+class NodeNetworking:
+    """Query and modify a node's network configuration."""
+
+    def __init__(self, node):
+        super(NodeNetworking, self).__init__()
+        self.node = node
+
+    def get_mac_addresses(self):
+        """Obtain the set of MAC addresses for this node.
+
+        :return: A `QuerySet` for `MACAddress`.
+        """
+        return self.node.macaddress_set.all()
+
+    _get_interfaces_query = dedent("""\
+    WITH RECURSIVE interfaces(id) AS (
+      SELECT iface.id
+        FROM maasserver_interface iface, maasserver_macaddress mac
+       WHERE iface.mac_id = mac.id
+         AND mac.node_id = %s
+      UNION ALL
+      SELECT iface_rel.child_id
+        FROM maasserver_interfacerelationship iface_rel, interfaces
+       WHERE iface_rel.parent_id = interfaces.id
+    )
+    SELECT *
+      FROM maasserver_interface
+     WHERE id in (SELECT id FROM interfaces)
+    """)
+
+    def get_interfaces(self):
+        """Obtain the set of interfaces for this node.
+
+        This will (efficiently) recurse into all nested interfaces.
+
+        :return: A `QuerySet` for `Interface`.
+        """
+        return Interface.objects.raw(
+            self._get_interfaces_query, params=[self.node.id])
 
=== added directory 'src/maasserver/networking/tests'
=== added file 'src/maasserver/networking/tests/__init__.py'
=== added file 'src/maasserver/networking/tests/test_networking.py'
--- src/maasserver/networking/tests/test_networking.py	1970-01-01 00:00:00 +0000
+++ src/maasserver/networking/tests/test_networking.py	2015-08-18 17:11:19 +0000
@@ -0,0 +1,129 @@
+# Copyright 2012-2015 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for the `maasserver.networking` module itself."""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+str = None
+
+__metaclass__ = type
+__all__ = []
+
+from functools import partial
+
+from django.db.models.query import (
+    QuerySet,
+    RawQuerySet,
+)
+from maasserver.enum import INTERFACE_TYPE
+from maasserver.networking import NodeNetworking
+from maasserver.testing.factory import factory
+from maasserver.testing.testcase import MAASServerTestCase
+from testtools.matchers import (
+    Is,
+    IsInstance,
+)
+
+# Modify make_MACAddress() to not create an interface by default.
+make_MACAddress = partial(factory.make_MACAddress, iftype=None)
+make_Interface = factory.make_Interface
+
+
+class TestNodeNetworking(MAASServerTestCase):
+    """Tests for `NodeNetworking` class."""
+
+    def test__init(self):
+        node = factory.make_Node()
+        node_net = NodeNetworking(node)
+        self.assertThat(node_net.node, Is(node))
+
+    # MAC addresses.
+
+    def test__get_mac_addresses_returns_empty_when_no_mac_addrs(self):
+        node = factory.make_Node()
+        node_net = NodeNetworking(node)
+        self.assertItemsEqual(node_net.get_mac_addresses(), [])
+
+    def test__get_mac_addresses_returns_empty_when_no_mac_addrs_for_node(self):
+        node = factory.make_Node()
+        node_net = NodeNetworking(node)
+        mac_for_other_node = make_MACAddress()  # noqa
+        self.assertItemsEqual(node_net.get_mac_addresses(), [])
+
+    def test__get_mac_addresses_returns_mac_addrs_belonging_to_node(self):
+        node = factory.make_Node()
+        node_net = NodeNetworking(node)
+        mac_for_node_1 = make_MACAddress(node=node)
+        mac_for_node_2 = make_MACAddress(node=node)
+        mac_for_other_node = make_MACAddress()  # noqa
+        self.assertItemsEqual(
+            [mac_for_node_1, mac_for_node_2],
+            node_net.get_mac_addresses())
+
+    def test__get_mac_addresses_returns_query_set(self):
+        node = factory.make_Node()
+        node_net = NodeNetworking(node)
+        self.assertThat(node_net.get_mac_addresses(), IsInstance(QuerySet))
+
+    # Interfaces.
+
+    def test__get_interfaces_returns_empty_when_no_interfaces(self):
+        node = factory.make_Node()
+        node_net = NodeNetworking(node)
+        self.assertItemsEqual(node_net.get_interfaces(), [])
+
+    def test__get_interfaces_returns_empty_when_no_interfaces_for_node(self):
+        node = factory.make_Node()
+        node_net = NodeNetworking(node)
+        iface_for_other_node = (  # noqa
+            make_Interface(INTERFACE_TYPE.PHYSICAL))
+        self.assertItemsEqual(node_net.get_interfaces(), [])
+
+    def test__get_interfaces_returns_ifaces_belonging_to_node(self):
+        node = factory.make_Node()
+        node_net = NodeNetworking(node)
+
+        mac1 = make_MACAddress(node=node, iftype=None)
+        mac2 = make_MACAddress(node=node, iftype=None)
+
+        iface1 = make_Interface(INTERFACE_TYPE.PHYSICAL, mac=mac1)
+        iface2 = make_Interface(INTERFACE_TYPE.PHYSICAL, mac=mac2)
+
+        mac_for_other_node = make_MACAddress()  # noqa
+        iface_for_other_node = make_Interface(  # noqa
+            INTERFACE_TYPE.PHYSICAL, mac=mac_for_other_node)
+
+        self.assertItemsEqual([iface1, iface2], node_net.get_interfaces())
+
+    def test__get_interfaces_returns_nested_ifaces_belonging_to_node(self):
+        node = factory.make_Node()
+        node_net = NodeNetworking(node)
+
+        mac1 = make_MACAddress(node=node)
+        mac2 = make_MACAddress(node=node)
+        mac3 = make_MACAddress(node=node)
+
+        iface1 = make_Interface(INTERFACE_TYPE.PHYSICAL, mac=mac1)
+        iface2 = make_Interface(INTERFACE_TYPE.PHYSICAL, mac=mac2)
+        iface3 = make_Interface(INTERFACE_TYPE.PHYSICAL, mac=mac3)
+
+        bond_iface_for_node = make_Interface(
+            INTERFACE_TYPE.BOND, mac1, parents=[iface1, iface2])
+        vlan_iface_for_node = make_Interface(
+            INTERFACE_TYPE.VLAN, mac=None, parents=[iface3])
+        # INTERFACE_TYPE.ALIAS is not yet supported.
+
+        self.assertItemsEqual(
+            [iface1, iface2, iface3, bond_iface_for_node, vlan_iface_for_node],
+            node_net.get_interfaces())
+
+    def test__get_interfaces_returns_raw_query_set(self):
+        node = factory.make_Node()
+        node_net = NodeNetworking(node)
+        self.assertThat(node_net.get_interfaces(), IsInstance(RawQuerySet))
+        node_net.get_interfaces()