Merge lp:~hazmat/pyjuju/local-network into lp:pyjuju

[Kapil Thangavelu]

[William Reade (fwereade)]

[0]

+ networks[name] = (state == "active") and True or False

Isn't that equivalent to:

+ networks[name] = (state == "active")

?

[1]

Nice tests; s/nonexistant/nonexistent/.

[2]

+def start_network(name, subnet):

I feel this function could be rearranged a little for clarity.

[3]

FAILED (skips=5, failures=3, errors=92, successes=1346)

Many due to "'FileStorage' object has no attribute 'get_url'", as noted in https://code.launchpad.net/~hazmat/ensemble/lib-files/+merge/73444, but it doesn't look like that's all of them.

review: Needs Fixing

[Gustavo Niemeyer (niemeyer)]

Very nice, +1 with some simple bits only.

[1]

+ subprocess.check_call(["virsh", "net-start", name])

It occurred to me that an error on these calls will pretty
often crash the whole application, because we won't catch
the exception coming out of it. Should we be trying to be
more careful about handling error situations and reporting
them instead of letting them go through, or are such errors
indeed supposed to terminate the app?

[2]

+def list_networks():

Having some sample output of the program in a comment would
make the logic much easier to follow.

[3]

+ networks_raw = filter(
+ None, [n.strip() for n in output.split("\n")][2:])
+ for name, state, auto in [n.split() for n in networks_raw]:
+ networks[name] = (state == "active") and True or False

I agree regarding clarity. E.g.:

# The first two lines contain headers
output_lines = output.splitlines()[2:]
for line in output_lines:
    if not line.strip():
        continue
    name, state, auto = line.split()
    networks[name] = (state == "active")

[4]

+def get_network_attributes(name):

Sample output would be useful as well. Would give a better idea
of what the code is looking after (e.g. why e.g. the ip is
processed the way it is).

review: Approve

[Kapil Thangavelu (hazmat)]

Excerpts from Gustavo Niemeyer's message of Mon Sep 05 19:09:24 UTC 2011:
> Review: Approve
> Very nice, +1 with some simple bits only.
>
> [1]
>
> + subprocess.check_call(["virsh", "net-start", name])
>
> It occurred to me that an error on these calls will pretty
> often crash the whole application, because we won't catch
> the exception coming out of it. Should we be trying to be
> more careful about handling error situations and reporting
> them instead of letting them go through, or are such errors
> indeed supposed to terminate the app?

Indeed its terminal error. The network is done as part of the initial bootstrap
command. A failure here is terminal to any further usage of the environment, and
it seems appropriate that this bubbles up directly to the user on the cli.
i could go ahead and reuse the _cmd from the juju.lib.lxc work to give some
additional contextual information.

[2-4] Addressed, sample output included in docstrings, and additional clarity to
the parsing.

[Benjamin Saller (bcsaller)]

The code in this branch looks fine. As we've talked about my issue is that we don't need to do all the heavy lifting that this branch currently does. Currently libvirt-bin does much/all of the networking setup and doing this by hand isn't really needed.

That said the code here may enable easier changes and better control in the future. So +1 with that reservations.

review: Approve

[Kapil Thangavelu (hazmat)]

Excerpts from Benjamin Saller's message of Fri Sep 16 21:11:25 UTC 2011:
> Review: Approve
>
> The code in this branch looks fine. As we've talked about my issue is that we don't need to do all the heavy lifting that this branch currently does. Currently libvirt-bin does much/all of the networking setup and doing this by hand isn't really needed.
>
> That said the code here may enable easier changes and better control in the future. So +1 with that reservations.

Thanks. just to be clear this code defers all the heavy lifting to libvirt, its
just provides an abstraction over managing/inspecting libvirt networking
features from ensemble.. err juju ;-) I agree this is currently doing more than we
need as we're relying on the libvirt default network for the lxc provider,
instead of creating our own.

=== added file 'juju/providers/lxc/network.py'
--- juju/providers/lxc/network.py	1970-01-01 00:00:00 +0000
+++ juju/providers/lxc/network.py	2011-09-16 21:06:26 +0000
@@ -0,0 +1,151 @@
+
+import subprocess
+import tempfile
+from xml.etree import ElementTree
+
+
+from twisted.internet.defer import inlineCallbacks, returnValue
+from twisted.internet.threads import deferToThread
+
+
+libvirt_network_template = """\
+<network>
+  <name>%(name)s</name>
+  <bridge name="vbr-%(name)s-%%d" />
+  <forward/>
+  <ip address="192.168.%(subnet)d.1" netmask="255.255.255.0">
+    <dhcp>
+      <range start="192.168.%(subnet)d.2" end="192.168.%(subnet)d.254" />
+    </dhcp>
+  </ip>
+</network>
+"""
+
+
+class Network(object):
+    """ Setup a bridge network with forwarding and dnsmasq for the environment.
+
+    Utilizes libvirt's networking subsystem to actualize configuration.
+    """
+
+    def __init__(self, name, subnet=131):
+        """
+        :param name: Name of the libvirt network
+        :param subnet: 192.168.x subnet to utilize
+        """
+        self._name = name
+        self._subnet = subnet
+
+    def start(self):
+        """Start the network.
+        """
+        return deferToThread(start_network, self._name, self._subnet)
+
+    def stop(self):
+        """Stop the network.
+        """
+        return deferToThread(stop_network, self._name)
+
+    @inlineCallbacks
+    def is_running(self):
+        """Returns True if the network is currently active, False otherwise.
+        """
+        networks = yield deferToThread(list_networks)
+        returnValue(bool(networks.get(self._name, False)))
+
+    def get_attributes(self):
+        """Return attributes of the network as a dictionary.
+
+        The network name, starting ip address, and bridge name are returned
+        """
+        return deferToThread(get_network_attributes, self._name)
+
+
+def start_network(name, subnet):
+    """Install a libvirt network for the environment.
+
+    Alternatively do the nat and bridge by hand ala
+    http://blog.mudy.info/2010/07/linux-container-on-amazon-ec2-server/
+    """
+    networks = list_networks()
+    if name in networks:
+        if networks[name]:
+            return
+        else:
+            subprocess.check_call(["virsh", "net-start", name])
+            return
+
+    libvirt_network = libvirt_network_template % (
+        dict(name=name, subnet=subnet))
+    network_file = tempfile.NamedTemporaryFile()
+    network_file.write(libvirt_network)
+    network_file.flush()
+
+    subprocess.check_call(["virsh", "net-define", network_file.name])
+    network_file.close()
+    subprocess.check_call(["virsh", "net-start", name])
+
+
+def list_networks():
+    """Return a dictionary of network name to active status bools.
+
+    Sample virsh net-list output::
+
+Name                 State      Autostart
+-----------------------------------------
+default              active     yes
+juju-test        inactive   no
+foobar               inactive   no
+
+    Parsing the above would return::
+    {"default": True, "juju-test": False, "foobar": False}
+    """
+    output = subprocess.check_output(["virsh", "net-list", "--all"])
+    networks = {}
+    # Take the header off and normalize whitespace.
+    net_lines = [n.strip() for n in output.splitlines()[2:]]
+    for line in net_lines:
+        if not line:
+            continue
+        name, state, auto = line.split()
+        networks[name] = state == "active"
+    return networks
+
+
+def get_network_attributes(name):
+    """Return core attributes of a libvirt network.
+
+    :param name: Name of the libvirt network.
+
+    returns a dictionary containing the name, ip,, and bridge name.
+
+    Sample virsh net-dumpxml output::
+<network>
+  <name>default</name>
+  <uuid>56fb682c-26a8-3645-5e47-b77ce33eefc6</uuid>
+  <forward mode='nat'/>
+  <bridge name='virbr0' stp='on' delay='0' />
+  <ip address='192.168.122.1' netmask='255.255.255.0'>
+    <dhcp>
+      <range start='192.168.122.2' end='192.168.122.254' />
+    </dhcp>
+  </ip>
+</network>
+    """
+    output = subprocess.check_output(["virsh", "net-dumpxml", name])
+    attrs = {}
+    etree = ElementTree.fromstring(output)
+    attrs["name"] = etree.findtext("name")
+    attrs["ip"] = etree.find("ip").attrib
+    attrs["bridge"] = etree.find("bridge").attrib["name"]
+    return attrs
+
+
+def stop_network(name):
+    """Stop the named libvirt network.
+    """
+    networks = list_networks()
+    status = networks.get(name)
+    if not status:
+        return
+    subprocess.check_call(["virsh", "net-stop", name])

=== added file 'juju/providers/lxc/tests/test_network.py'
--- juju/providers/lxc/tests/test_network.py	1970-01-01 00:00:00 +0000
+++ juju/providers/lxc/tests/test_network.py	2011-09-16 21:06:26 +0000
@@ -0,0 +1,211 @@
+import subprocess
+
+from twisted.internet.defer import inlineCallbacks
+
+from juju.providers.lxc.network import Network
+from juju.lib.testing import TestCase
+from juju.lib.mocker import MATCH
+
+# Captured raw output from virsh net-list
+network_list_default_output = """\
+Name                 State      Autostart
+-----------------------------------------
+default              active     yes       
+
+"""
+
+# Parameterized output for test case
+network_list_output_template = """\
+Name                 State      Autostart
+-----------------------------------------
+default              active     yes
+%(name)s             %(state)s  %(autostart)s
+
+"""
+
+network_dump_xml_default_output = """\
+<network>
+  <name>default</name>
+  <uuid>56fb682c-26a8-3645-5e47-b77ce33eefc6</uuid>
+  <forward mode='nat'/>
+  <bridge name='virbr0' stp='on' delay='0' />
+  <ip address='192.168.122.1' netmask='255.255.255.0'>
+    <dhcp>
+      <range start='192.168.122.2' end='192.168.122.254' />
+    </dhcp>
+  </ip>
+</network>
+"""
+
+
+class NetworkTestCase(TestCase):
+
+    # Mocked tests against the network class
+
+    @inlineCallbacks
+    def test_get_network_attributes(self):
+        network_dumpxml_mock = self.mocker.mock()
+        self.patch(subprocess,  "check_output", network_dumpxml_mock)
+        network_dumpxml_mock(["virsh", "net-dumpxml", "default"])
+        self.mocker.result(network_dump_xml_default_output)
+        self.mocker.replay()
+
+        network = Network("default")
+        attributes = yield network.get_attributes()
+        self.assertEqual(
+            dict(name="default",
+                 ip=dict(address="192.168.122.1", netmask="255.255.255.0"),
+                 bridge="virbr0"),
+            attributes)
+
+    @inlineCallbacks
+    def test_start(self):
+        network_list_mock = self.mocker.mock()
+        network_start_mock = self.mocker.mock()
+
+        def check_definition(args):
+            cmd, subcommand, template_path = args
+            network_definition = open(template_path).read()
+            self.assertIn(
+                '<name>foobar</name>', network_definition)
+            self.assertIn(
+                '<bridge name="vbr-foobar-%d" />', network_definition)
+            self.assertIn(
+                '<range start="192.168.131.2" end="192.168.131.254" />',
+                network_definition)
+            self.assertIn(
+                '<ip address="192.168.131.1" netmask="255.255.255.0">',
+                network_definition)
+            return True
+
+        self.patch(subprocess,  "check_output", network_list_mock)
+        self.patch(subprocess, "check_call", network_start_mock)
+
+        network_list_mock(["virsh", "net-list", "--all"])
+        self.mocker.result(network_list_default_output)
+
+        network_start_mock(MATCH(check_definition))
+        network_start_mock(["virsh", "net-start", "foobar"])
+        self.mocker.replay()
+
+        network = Network("foobar")
+        yield network.start()
+
+    @inlineCallbacks
+    def test_start_stoppped_network(self):
+        network_list_mock = self.mocker.mock()
+        self.patch(subprocess, "check_output", network_list_mock)
+        network_list_mock(["virsh", "net-list", "--all"])
+
+        self.mocker.result(
+            network_list_output_template % dict(
+                name="juju", state="inactive", autostart="no"))
+
+        network_start_mock = self.mocker.mock()
+        self.patch(subprocess, "check_call", network_start_mock)
+        network_start_mock(["virsh", "net-start", "juju"])
+        self.mocker.replay()
+
+        network = Network("juju")
+        yield network.start()
+
+    @inlineCallbacks
+    def test_start_started_network(self):
+        network_list_mock = self.mocker.mock()
+        self.patch(subprocess, "check_output", network_list_mock)
+        network_list_mock(["virsh", "net-list", "--all"])
+
+        self.mocker.result(
+            network_list_output_template % dict(
+                name="zoo", state="active", autostart="no"))
+
+        # patch to catch any errant calls
+        network_start_mock = self.mocker.mock()
+        self.patch(subprocess, "check_call", network_start_mock)
+        self.mocker.replay()
+        network = Network("zoo")
+        yield network.start()
+
+    @inlineCallbacks
+    def test_stop(self):
+        network_list_mock = self.mocker.mock()
+        network_stop_mock = self.mocker.mock()
+        self.patch(subprocess, "check_output", network_list_mock)
+        self.patch(subprocess, "check_call", network_stop_mock)
+
+        network_list_mock(["virsh", "net-list", "--all"])
+        self.mocker.result(
+            network_list_output_template % dict(
+                name="zoo", state="active", autostart="no"))
+
+        network_stop_mock(["virsh", "net-stop", "zoo"])
+        self.mocker.replay()
+
+        network = Network("zoo")
+        yield network.stop()
+
+    @inlineCallbacks
+    def test_stop_stopped_network(self):
+        network_list_mock = self.mocker.mock()
+        self.patch(subprocess, "check_output", network_list_mock)
+        network_list_mock(["virsh", "net-list", "--all"])
+
+        self.mocker.result(
+            network_list_output_template % dict(
+                name="zoo", state="inactive", autostart="no"))
+
+        # patch to catch any errant calls
+        network_stop_mock = self.mocker.mock()
+        self.patch(subprocess, "check_call", network_stop_mock)
+        self.mocker.replay()
+
+        network = Network("zoo")
+        yield network.stop()
+
+    @inlineCallbacks
+    def test_stop_nonexistent_network(self):
+        network_list_mock = self.mocker.mock()
+        self.patch(subprocess, "check_output", network_list_mock)
+        network_list_mock(["virsh", "net-list", "--all"])
+
+        self.mocker.result(
+            network_list_output_template % dict(
+                name="zoo", state="inactive", autostart="no"))
+
+        # patch to catch any errant calls
+        network_stop_mock = self.mocker.mock()
+        self.patch(subprocess, "check_call", network_stop_mock)
+        self.mocker.replay()
+
+        network = Network("zebra")
+        yield network.stop()
+
+    @inlineCallbacks
+    def test_is_running(self):
+        network_list_mock = self.mocker.mock()
+        self.patch(subprocess,  "check_output", network_list_mock)
+
+        # Network on
+        network_list_mock(["virsh", "net-list", "--all"])
+        self.mocker.result(
+            network_list_output_template % (
+                dict(name="foobar", state="active", autostart="off")))
+
+        # Network off
+        network_list_mock(["virsh", "net-list", "--all"])
+        self.mocker.result(
+            network_list_output_template % (
+                dict(name="foobar", state="inactive", autostart="off")))
+
+        # Non existent
+        network_list_mock(["virsh", "net-list", "--all"])
+        self.mocker.result(
+            network_list_output_template % (
+                dict(name="magic", state="inactive", autostart="off")))
+
+        self.mocker.replay()
+
+        network = Network("foobar")
+        self.assertTrue((yield network.is_running()))
+        self.assertFalse((yield network.is_running()))
+        self.assertFalse((yield network.is_running()))