Merge lp:~citrix-openstack/nova/xenapi-vlan-network-manager into lp:~hudson-openstack/nova/trunk

[Salvatore Orlando]

Added VLAN networking support for XenAPI

The branch proposed for merged implements the xenapi-vlan-network-manager blueprint (https://blueprints.launchpad.net/nova/+spec/xenapi-vlan-network-manager).

It extends VLAN network manager supports to compute nodes running xenapi.
To do so, we create a xenapi specific network driver, which implements only ensure_vlan_bridge.
This way, when the compute node invokes NetworkManager.setup_compute_network, the appropriate VLAN configuration is performed using XenAPI.

Note: the network node is not affected at all by this change, and it should therefore be executed using the linux_net driver as usual. The compute node should be executed with the xenapi_net driver instead.

[Matt Dietz (cerberus)]

I merged with trunk and got a conflict in network/manager.py

<<<<<<< TREE
flags.DEFINE_string('vlan_interface', 'eth0',
                    'network device for vlans')
flags.DEFINE_integer('num_networks', 1000, 'Number of networks to support')
=======
flags.DEFINE_integer('num_networks', 1, 'Number of networks to support')
>>>>>>> MERGE-SOURCE

Also had a unit-test error in one of the test files, so the rest of the XenAPI tests aren't currently running. Forget a bzr add?

======================================================================
ERROR: Failure: ImportError (cannot import name fake_utils)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/local/lib/python2.6/dist-packages/nose/loader.py", line 390, in loadTestsFromName
    addr.filename, addr.module)
  File "/usr/local/lib/python2.6/dist-packages/nose/importer.py", line 39, in importFromPath
    return self.importFromDir(dir_path, fqname)
  File "/usr/local/lib/python2.6/dist-packages/nose/importer.py", line 86, in importFromDir
    mod = load_module(part_fqname, fh, filename, desc)
  File "/root/code/nova/xenapi-vlan-network-manager/nova/tests/test_xenapi.py", line 41, in <module>
    from nova.tests import fake_utils
ImportError: cannot import name fake_utils

----------------------------------------------------------------------

36) Shouldn't this be the citrix copyright?

623-632) this seems really odd to me. What's an example of how we would actually fail into the exception block?

Tiny nits:

116 + "Expected %(vlan_num)d") % locals())

You might want a space before Expected or it will run up against the period from the previous sentence

review: Needs Fixing

[Salvatore Orlando (salvatore-orlando)]

Hi Matt,
thanks for reviewing this branch

> I merged with trunk and got a conflict in network/manager.py
>
> <<<<<<< TREE
> flags.DEFINE_string('vlan_interface', 'eth0',
> 'network device for vlans')
> flags.DEFINE_integer('num_networks', 1000, 'Number of networks to support')
> =======
> flags.DEFINE_integer('num_networks', 1, 'Number of networks to support')
> >>>>>>> MERGE-SOURCE
>

I guess the conflict has arisen from a recent merge (the one for solving a bug in nova-manage). I will merge my branch with trunk, and push again...

> Also had a unit-test error in one of the test files, so the rest of the XenAPI
> tests aren't currently running. Forget a bzr add?

...and when pushing, this time I will not forget to add fake_utils.py!

>
> ======================================================================
> ERROR: Failure: ImportError (cannot import name fake_utils)
> ----------------------------------------------------------------------
> Traceback (most recent call last):
> File "/usr/local/lib/python2.6/dist-packages/nose/loader.py", line 390, in
> loadTestsFromName
> addr.filename, addr.module)
> File "/usr/local/lib/python2.6/dist-packages/nose/importer.py", line 39, in
> importFromPath
> return self.importFromDir(dir_path, fqname)
> File "/usr/local/lib/python2.6/dist-packages/nose/importer.py", line 86, in
> importFromDir
> mod = load_module(part_fqname, fh, filename, desc)
> File "/root/code/nova/xenapi-vlan-network-
> manager/nova/tests/test_xenapi.py", line 41, in <module>
> from nova.tests import fake_utils
> ImportError: cannot import name fake_utils
>
> ----------------------------------------------------------------------
>
> 36) Shouldn't this be the citrix copyright?
>

Yes, it should. Shame on me...

> 623-632) this seems really odd to me. What's an example of how we would
> actually fail into the exception block?
>

I agree this try/except block is quite weird.
The main reason is that find_network_with_bridge expects to find a network on the Xen backend whose attribute 'bridge' matches the 'bridge' column in the nova db or the FLAGS.flat_network_bridge flag.
However, while this is true for flat networking (in which we tipically set the flag to xenbr0 or something like that), this does not work anymore with VLAN networks. In XenAPI names for bridges associated with VLAN networks are auto-generated. For instance, for VLAN 111 it will not be possible to call the bridge br111, but a name like xapiXX (where XX is an integer) will be generated.
To solve this problem, when we create a VLAN network in XenAPI, we set its 'name' attribute to the value of the 'bridge' column in the name db. We then lookup for the appropriate network by looking at the 'name' attribute instead of the 'bridge' one.

The try/except block first tries to identify the network by bridge, and then, if it fails, tries with the 'name' attribute. I agree however that this does not look good at all. I will therefore shift the burden of the bridge/name lookup into NetworkHelper, implementing a method that looks for both properties and then returns the first network for which either the 'bridge' or the 'nam...

[Salvatore Orlando (salvatore-orlando)]

The branch has been updated and a new round of review is kindly asked.
Matt's comments have been addressed as follows:
- resolved conflicts with trunk (this does not exclude new conflicts in the future, though!)
- added missing fake_utils.py file
- changed copyright header in xenapi_net.py
- removed exception block in create_vif, changed find_network_with_bridge to deal with both bridge and name_label attributes
- Fixed missing whitespace problem with debug line

Also, the unit test for spawning VM on VLAN network has been slightly changed to reflect changes in the code

[lp:~citrix-openstack/nova/xenapi-vlan-network-manager]

689. By Salvatore Orlando on 2011-03-23

removed excess debug line

690. By Salvatore Orlando on 2011-03-23

merge trunk

[Matt Dietz (cerberus)]

lgtm.

review: Approve

[lp:~citrix-openstack/nova/xenapi-vlan-network-manager]

691. By Salvatore Orlando on 2011-03-24

merge trunk

692. By Salvatore Orlando on 2011-03-24

merge trunk

693. By Salvatore Orlando on 2011-03-25

merge trunk

[Salvatore Orlando (salvatore-orlando)]

Requesting FF exception for this branch, as it has already 1 approve vote and there's no major dispute on the way it is implemented.

The branch touches two areas:
- VM spawn procedure for xenapi backend (thoroughly tested)
- Network driver for xenapi backend (new area - a test has been added in test_xenapi which offers good code coverage)

Benefit:
- this branch will enable support the Vlan Network manager on XenAPI backends. This support has also been implemented for VSphere backends in a branch that was merged yesterday.

Risk assessment:
- No risk for network manager, as its code has not been touched at all. The xenapi network driver will be used only by the compute node for setting up VLANs on xen hosts. This also means that there cannot be any impact on other capabilities of the Network Manager, such as VPN access or Floating IPs.
- Since this is a new feature, and the associated code is executed only when a compute node uses XenAPI and the VLan manager, there is no risk of breaking libvirt, hyperv, or mware installations, or XenAPI installations which use the flat manager

[lp:~citrix-openstack/nova/xenapi-vlan-network-manager]

694. By Salvatore Orlando on 2011-03-25

merge trunk

[Thierry Carrez (ttx)]

This is also a bit large, but relatively self-contained. It needs to get merged soon, FFe granted for merging by Tuesday evening.

review: Approve (ffe)

[Trey Morris (tr3buchet)]

Looks good. Unless I'm missing something multi-nic and this should get along just fine unless you want to have vlans on multiple interfaces, in which case setting the vlan_interface with a flag isnt'a good idea. The flat_network_bridge flag is going away with multi-nic, perhaps vlan_interface will need to go away too?

approve for now but we should probably discuss.

review: Approve

[Trey Morris (tr3buchet)]

214 + def fake_network_get_all_by_instance(context, instance_id):
215 + l = []
216 + #even instance numbers are on vlan networks
217 + if instance_id % 2 == 0:
218 + l.append(FakeModel(vlan_network_fields))
219 + else:
220 + l.append(FakeModel(flat_network_fields))
221 + return l

missed this... please don't use 'l' as a variable name (lower case L).

review: Needs Fixing

[Trey Morris (tr3buchet)]

as this is down to the wire...
if I'm asleep or not around when 'l' is fixed, as that's all my approval is hinged upon, feel free to approve out from under me or whatever to get it in

[Salvatore Orlando (salvatore-orlando)]

> as this is down to the wire...
> if I'm asleep or not around when 'l' is fixed, as that's all my approval is
> hinged upon, feel free to approve out from under me or whatever to get it in

Hi Trey, I'm going to update the 'l' thing right now.

As regards vlan_interface, its semantics are probably different from flat_network_bridge.
flat_network_bridge tells (or least it used to tell!) nova-compute which bridge should be used for VIFs. Of course, with multiple VIFs per VM, this does not make sense anymore.
vlan_interface instead represents the physical interface on the hypervisor we will use for creating the bridge for the vlan network.
In libvirt this means using vconfig to create a vlan on vlan_interface, create a new bridge, and then attach the vlan interface to this bridge; the virtual interface is then attached to newly created bridge.
In xenapi instead this means using xapi to create a network, retrieve the PIF associated with vlan_interface and then create a VLAN object using the network and the PIF. The VIF is then attached to the newly created network.

Summarizing I think that after multi-nic we will still need vlan_interface. What we will need to discuss is probably the way in which we create these bridges. Currently the nova-compute node does that, probably we want to let the network service do all networking-related arrangements.

[lp:~citrix-openstack/nova/xenapi-vlan-network-manager]

695. By Salvatore Orlando on 2011-03-25

merge trunk
addressing Trey's comments

[Salvatore Orlando (salvatore-orlando)]

Branch updated.
Lowercase 'L' not used anymore.
Merge with trunk (rev 892)

[termie (termie)]

- please put a space between the # and the comment in all cases, preferably with Sentence case
- please end your docstrings with periods in all cases
- line 87: please put the beginning of the dictionary as the same line as the opening curly brace and indent the next lines to match
- line 93: please us parens to wrap the multiline expression rather than escaping the newline, as in:

expr = (foo == BLAH
        and bar < 3)

- line 104: please place one argument per line, not two, or place them all on the same line on the next line and double indent
- line 114: please use != instead of <>
- please adjust all docstrings to match the formatting described in HACKING
- line 362: you have a commented out line that should probably be removed
- in xenapi tests you want 'from nova import log as logging' rather than as LOG, and you want it in the correct place in the import list, as described in HACKING
- line 454: please indent this line to match the other arguments
- line 619: the newline does not need to be escaped
- line 656: please put the beginning of the dict contents on teh first line and indent the rest of the contents to match it
- same for 676 and 684
- line 699: the correct format is # TODO(salvatore-orlando): the contents
- line 740: parens instead of escaped newlines, please
- same for line 777

review: Needs Fixing

[lp:~citrix-openstack/nova/xenapi-vlan-network-manager]

696. By Armando Migliaccio on 2011-03-28

addressed termies review (first round)

697. By Armando Migliaccio on 2011-03-28

addressed termie's review (second round)

698. By Armando Migliaccio on 2011-03-28

addressed termies review (third round)

[Armando Migliaccio (armando-migliaccio)]

Folks,

thanks very much for the review. This last round was regarding coding style. I have addressed issues raised by Termie, whereas Salvatore addressed Trey's.

I guess we are ready to roll now.

Cheers,
Armando

[lp:~citrix-openstack/nova/xenapi-vlan-network-manager]

699. By Armando Migliaccio on 2011-03-28

merge trunk

[Trey Morris (tr3buchet)]

looks good to me

review: Approve

[lp:~citrix-openstack/nova/xenapi-vlan-network-manager]

700. By Armando Migliaccio on 2011-03-28

merge trunk

[termie (termie)]

thanks for the style cleanups :) there were still a few that didn't quite make it:

- single-line docstrings should be on a single line:

"""Single line docstring."""

not

"""
Single line docstring.
"""

- Multiline docstrings consist of a one-line summary on the first line, followed by a newline, followed by a more detailed description, such as:

"""Single line summary.

More detailed description.

"""

(We have the extra newline at the end to be nice to emacs users).

- you need some extra newlines at the top of nova/network/xenapi_network between the imports and LOG and between LOG and FLAGS
- when importing import only modules, not objects directly, for example instead of

+from nova.virt.xenapi.network_utils import NetworkHelper

you would want

from nova.virt.xenapi import network_utils

and then later on in your code you use network_utils.NetworkHelper

I agree this gets a little verbose do to the current code style in the xenapi tools, that code currently has a bit of a java fragrance, but this is the python way (we don't use static methods or factories nearly as much as java).

- when you define _CLASSES in fake.py you don't need to escape the newline (remove the backslash)

- in _create_local_sr a common way of dealing with long lines in that situation is to move everything to the next line and double indent, rather than escaping a new line, for example:

sr_ref = _create_object(
        'SR',
        {'name_label': 'Local storage',
         'type': 'lvm',
         'content_type': 'user',
         'shared': False,
         'physical_size': str(1 << 30),
         'physical_utilisation': str(0),
         'virtual_allocation': str(0),
         'other_config': {
                 'i18n-original-value-name_label': 'Local storage',
                 'i18n-key': 'local-storage'},
         'VDIs': []})

review: Needs Fixing

[Armando Migliaccio (armando-migliaccio)]

thanks for your thorough review. Please see my comments inline

> thanks for the style cleanups :) there were still a few that didn't quite make
> it:
>
> - single-line docstrings should be on a single line:
>
> """Single line docstring."""
>
> not
>
> """
> Single line docstring.
> """
>
> - Multiline docstrings consist of a one-line summary on the first line,
> followed by a newline, followed by a more detailed description, such as:
>
> """Single line summary.
>
> More detailed description.
>
> """
>
> (We have the extra newline at the end to be nice to emacs users).
>
>

I wish I didn't edit those in the first place as they weren't part of the initial diff...I was just acting politely ;)

> - you need some extra newlines at the top of nova/network/xenapi_network
> between the imports and LOG and between LOG and FLAGS

What do you mean? linux_net.py in the same folder has got the same number of lines. Can you clarify this please?

> - when importing import only modules, not objects directly, for example
> instead of
>
> +from nova.virt.xenapi.network_utils import NetworkHelper
>
> you would want
>
> from nova.virt.xenapi import network_utils
>
> and then later on in your code you use network_utils.NetworkHelper

I am not sure this rule has been applied consistently throughout the code, but I'll oblige. On a more general note, it'd be nice to have a summarized set of Coding Style Guidelines on the Wiki (some sort of checklist other that http://www.python.org/dev/peps/pep-0008/, which is way too long). This may help and reduce the number of review rounds

>
> I agree this gets a little verbose do to the current code style in the xenapi
> tools, that code currently has a bit of a java fragrance, but this is the
> python way (we don't use static methods or factories nearly as much as java).
>
> - when you define _CLASSES in fake.py you don't need to escape the newline
> (remove the backslash)
>
> - in _create_local_sr a common way of dealing with long lines in that
> situation is to move everything to the next line and double indent, rather
> than escaping a new line, for example:
>
> sr_ref = _create_object(
> 'SR',
> {'name_label': 'Local storage',
> 'type': 'lvm',
> 'content_type': 'user',
> 'shared': False,
> 'physical_size': str(1 << 30),
> 'physical_utilisation': str(0),
> 'virtual_allocation': str(0),
> 'other_config': {
> 'i18n-original-value-name_label': 'Local storage',
> 'i18n-key': 'local-storage'},
> 'VDIs': []})

but then the newline for the inner dictionary is okay?

[lp:~citrix-openstack/nova/xenapi-vlan-network-manager]

701. By Armando Migliaccio on 2011-03-28

style fixes

702. By Armando Migliaccio on 2011-03-28

merge trunk

703. By Armando Migliaccio on 2011-03-28

fix docstrings

[Armando Migliaccio (armando-migliaccio)]

okay, I hope I caught them all!

Cheers,
Armando

[lp:~citrix-openstack/nova/xenapi-vlan-network-manager]

704. By Armando Migliaccio on 2011-03-29

merge trunk

705. By Armando Migliaccio on 2011-03-29

sorted pep8 errors that were introduced during previous fixes

=== modified file 'nova/network/linux_net.py'
--- nova/network/linux_net.py	2011-03-24 18:51:38 +0000
+++ nova/network/linux_net.py	2011-03-29 10:28:19 +0000
@@ -44,13 +44,10 @@
 flags.DEFINE_string('dhcp_domain',
                     'novalocal',
                     'domain to use for building the hostnames')
-
 flags.DEFINE_string('networks_path', '$state_path/networks',
                     'Location to keep network config files')
 flags.DEFINE_string('public_interface', 'eth0',
                     'Interface for public IP addresses')
-flags.DEFINE_string('vlan_interface', 'eth0',
-                    'network device for vlans')
 flags.DEFINE_string('dhcpbridge', _bin_file('nova-dhcpbridge'),
                         'location of nova-dhcpbridge')
 flags.DEFINE_string('routing_source_ip', '$my_ip',

=== modified file 'nova/network/manager.py'
--- nova/network/manager.py	2011-03-24 19:04:24 +0000
+++ nova/network/manager.py	2011-03-29 10:28:19 +0000
@@ -73,6 +73,8 @@
 flags.DEFINE_string('flat_network_dhcp_start', '10.0.0.2',
                     'Dhcp start for FlatDhcp')
 flags.DEFINE_integer('vlan_start', 100, 'First VLAN for private networks')
+flags.DEFINE_string('vlan_interface', 'eth0',
+                    'network device for vlans')
 flags.DEFINE_integer('num_networks', 1, 'Number of networks to support')
 flags.DEFINE_string('vpn_ip', '$my_ip',
                     'Public IP for the cloudpipe VPN servers')

=== added file 'nova/network/xenapi_net.py'
--- nova/network/xenapi_net.py	1970-01-01 00:00:00 +0000
+++ nova/network/xenapi_net.py	2011-03-29 10:28:19 +0000
@@ -0,0 +1,85 @@
+# vim: tabstop=4 shiftwidth=4 softtabstop=4
+
+# Copyright (c) 2011 Citrix Systems, Inc.
+# Copyright 2011 OpenStack LLC.
+#
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+"""
+Implements vlans, bridges, and iptables rules using linux utilities.
+"""
+
+import os
+
+from nova import db
+from nova import exception
+from nova import flags
+from nova import log as logging
+from nova import utils
+from nova.virt.xenapi_conn import XenAPISession
+from nova.virt.xenapi import network_utils
+
+LOG = logging.getLogger("nova.xenapi_net")
+
+FLAGS = flags.FLAGS
+
+
+def ensure_vlan_bridge(vlan_num, bridge, net_attrs=None):
+    """Create a vlan and bridge unless they already exist."""
+    # Open xenapi session
+    LOG.debug("ENTERING ensure_vlan_bridge in xenapi net")
+    url = FLAGS.xenapi_connection_url
+    username = FLAGS.xenapi_connection_username
+    password = FLAGS.xenapi_connection_password
+    session = XenAPISession(url, username, password)
+    # Check whether bridge already exists
+    # Retrieve network whose name_label is "bridge"
+    network_ref = network_utils.NetworkHelper.find_network_with_name_label(
+                                                                       session,
+                                                                       bridge)
+    if network_ref == None:
+        # If bridge does not exists
+        # 1 - create network
+        description = "network for nova bridge %s" % bridge
+        network_rec = {'name_label': bridge,
+                       'name_description': description,
+                       'other_config': {}}
+        network_ref = session.call_xenapi('network.create', network_rec)
+        # 2 - find PIF for VLAN
+        expr = 'field "device" = "%s" and \
+                field "VLAN" = "-1"' % FLAGS.vlan_interface
+        pifs = session.call_xenapi('PIF.get_all_records_where', expr)
+        pif_ref = None
+        # Multiple PIF are ok: we are dealing with a pool
+        if len(pifs) == 0:
+            raise Exception(
+                  _('Found no PIF for device %s') % FLAGS.vlan_interface)
+        # 3 - create vlan for network
+        for pif_ref in pifs.keys():
+            session.call_xenapi('VLAN.create',
+                                pif_ref,
+                                str(vlan_num),
+                                network_ref)
+    else:
+        # Check VLAN tag is appropriate
+        network_rec = session.call_xenapi('network.get_record', network_ref)
+        # Retrieve PIFs from network
+        for pif_ref in network_rec['PIFs']:
+            # Retrieve VLAN from PIF
+            pif_rec = session.call_xenapi('PIF.get_record', pif_ref)
+            pif_vlan = int(pif_rec['VLAN'])
+            # Raise an exception if VLAN != vlan_num
+            if pif_vlan != vlan_num:
+                raise Exception(_("PIF %(pif_rec['uuid'])s for network "
+                                  "%(bridge)s has VLAN id %(pif_vlan)d. "
+                                  "Expected %(vlan_num)d") % locals())

=== modified file 'nova/tests/db/fakes.py'
--- nova/tests/db/fakes.py	2011-03-25 01:23:33 +0000
+++ nova/tests/db/fakes.py	2011-03-29 10:28:19 +0000
@@ -25,7 +25,7 @@
 
 
 def stub_out_db_instance_api(stubs, injected=True):
-    """ Stubs out the db API for creating Instances """
+    """Stubs out the db API for creating Instances."""
 
     INSTANCE_TYPES = {
         'm1.tiny': dict(memory_mb=512,
@@ -56,27 +56,39 @@
                  flavorid=5,
                  rxtx_cap=5)}
 
-    network_fields = {
-        'id': 'test',
-        'bridge': 'xenbr0',
-        'label': 'test_network',
-        'netmask': '255.255.255.0',
-        'cidr_v6': 'fe80::a00:0/120',
-        'netmask_v6': '120',
-        'gateway': '10.0.0.1',
-        'gateway_v6': 'fe80::a00:1',
-        'broadcast': '10.0.0.255',
-        'dns': '10.0.0.2',
-        'ra_server': None,
-        'injected': injected}
-
-    fixed_ip_fields = {
-        'address': '10.0.0.3',
-        'address_v6': 'fe80::a00:3',
-        'network_id': 'test'}
+    flat_network_fields = {'id': 'fake_flat',
+                           'bridge': 'xenbr0',
+                           'label': 'fake_flat_network',
+                           'netmask': '255.255.255.0',
+                           'cidr_v6': 'fe80::a00:0/120',
+                           'netmask_v6': '120',
+                           'gateway': '10.0.0.1',
+                           'gateway_v6': 'fe80::a00:1',
+                           'broadcast': '10.0.0.255',
+                           'dns': '10.0.0.2',
+                           'ra_server': None,
+                           'injected': injected}
+
+    vlan_network_fields = {'id': 'fake_vlan',
+                           'bridge': 'br111',
+                           'label': 'fake_vlan_network',
+                           'netmask': '255.255.255.0',
+                           'cidr_v6': 'fe80::a00:0/120',
+                           'netmask_v6': '120',
+                           'gateway': '10.0.0.1',
+                           'gateway_v6': 'fe80::a00:1',
+                           'broadcast': '10.0.0.255',
+                           'dns': '10.0.0.2',
+                           'ra_server': None,
+                           'vlan': 111,
+                           'injected': False}
+
+    fixed_ip_fields = {'address': '10.0.0.3',
+                       'address_v6': 'fe80::a00:3',
+                       'network_id': 'fake_flat'}
 
     class FakeModel(object):
-        """ Stubs out for model """
+        """Stubs out for model."""
         def __init__(self, values):
             self.values = values
 
@@ -96,10 +108,19 @@
         return INSTANCE_TYPES[name]
 
     def fake_network_get_by_instance(context, instance_id):
+        # Even instance numbers are on vlan networks
+        if instance_id % 2 == 0:
+            return FakeModel(vlan_network_fields)
+        else:
+            return FakeModel(flat_network_fields)
         return FakeModel(network_fields)
 
     def fake_network_get_all_by_instance(context, instance_id):
-        return [FakeModel(network_fields)]
+        # Even instance numbers are on vlan networks
+        if instance_id % 2 == 0:
+            return [FakeModel(vlan_network_fields)]
+        else:
+            return [FakeModel(flat_network_fields)]
 
     def fake_instance_get_fixed_address(context, instance_id):
         return FakeModel(fixed_ip_fields).address
@@ -111,6 +132,8 @@
         return [FakeModel(fixed_ip_fields)]
 
     stubs.Set(db, 'network_get_by_instance', fake_network_get_by_instance)
+    stubs.Set(db, 'network_get_all_by_instance',
+              fake_network_get_all_by_instance)
     stubs.Set(db, 'instance_type_get_all', fake_instance_type_get_all)
     stubs.Set(db, 'instance_type_get_by_name', fake_instance_type_get_by_name)
     stubs.Set(db, 'instance_get_fixed_address',

=== modified file 'nova/tests/fake_utils.py'
--- nova/tests/fake_utils.py	2011-03-24 02:01:46 +0000
+++ nova/tests/fake_utils.py	2011-03-29 10:28:19 +0000
@@ -14,8 +14,7 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
-"""This modules stubs out functions in nova.utils
-"""
+"""This modules stubs out functions in nova.utils."""
 
 import re
 import types
@@ -42,21 +41,25 @@
 
 
 def fake_execute_set_repliers(repliers):
-    """Allows the client to configure replies to commands"""
+    """Allows the client to configure replies to commands."""
     global _fake_execute_repliers
     _fake_execute_repliers = repliers
 
 
 def fake_execute_default_reply_handler(*ignore_args, **ignore_kwargs):
-    """A reply handler for commands that haven't been added to the reply
-    list.  Returns empty strings for stdout and stderr
+    """A reply handler for commands that haven't been added to the reply list.
+
+    Returns empty strings for stdout and stderr.
+
     """
     return '', ''
 
 
 def fake_execute(*cmd_parts, **kwargs):
-    """This function stubs out execute, optionally executing
-    a preconfigued function to return expected data
+    """This function stubs out execute.
+
+    It optionally executes a preconfigued function to return expected data.
+
     """
     global _fake_execute_repliers
 

=== modified file 'nova/tests/test_xenapi.py'
--- nova/tests/test_xenapi.py	2011-03-25 00:16:53 +0000
+++ nova/tests/test_xenapi.py	2011-03-29 10:28:19 +0000
@@ -14,9 +14,7 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
-"""
-Test suite for XenAPI
-"""
+"""Test suite for XenAPI."""
 
 import functools
 import os
@@ -65,9 +63,7 @@
 
 
 class XenAPIVolumeTestCase(test.TestCase):
-    """
-    Unit tests for Volume operations
-    """
+    """Unit tests for Volume operations."""
     def setUp(self):
         super(XenAPIVolumeTestCase, self).setUp()
         self.stubs = stubout.StubOutForTesting()
@@ -101,7 +97,7 @@
         return db.volume_create(self.context, vol)
 
     def test_create_iscsi_storage(self):
-        """ This shows how to test helper classes' methods """
+        """This shows how to test helper classes' methods."""
         stubs.stubout_session(self.stubs, stubs.FakeSessionForVolumeTests)
         session = xenapi_conn.XenAPISession('test_url', 'root', 'test_pass')
         helper = volume_utils.VolumeHelper
@@ -116,7 +112,7 @@
         db.volume_destroy(context.get_admin_context(), vol['id'])
 
     def test_parse_volume_info_raise_exception(self):
-        """ This shows how to test helper classes' methods """
+        """This shows how to test helper classes' methods."""
         stubs.stubout_session(self.stubs, stubs.FakeSessionForVolumeTests)
         session = xenapi_conn.XenAPISession('test_url', 'root', 'test_pass')
         helper = volume_utils.VolumeHelper
@@ -130,7 +126,7 @@
         db.volume_destroy(context.get_admin_context(), vol['id'])
 
     def test_attach_volume(self):
-        """ This shows how to test Ops classes' methods """
+        """This shows how to test Ops classes' methods."""
         stubs.stubout_session(self.stubs, stubs.FakeSessionForVolumeTests)
         conn = xenapi_conn.get_connection(False)
         volume = self._create_volume()
@@ -149,7 +145,7 @@
         check()
 
     def test_attach_volume_raise_exception(self):
-        """ This shows how to test when exceptions are raised """
+        """This shows how to test when exceptions are raised."""
         stubs.stubout_session(self.stubs,
                               stubs.FakeSessionForVolumeFailedTests)
         conn = xenapi_conn.get_connection(False)
@@ -172,9 +168,7 @@
 
 
 class XenAPIVMTestCase(test.TestCase):
-    """
-    Unit tests for VM operations
-    """
+    """Unit tests for VM operations."""
     def setUp(self):
         super(XenAPIVMTestCase, self).setUp()
         self.manager = manager.AuthManager()
@@ -188,6 +182,7 @@
                    instance_name_template='%d')
         xenapi_fake.reset()
         xenapi_fake.create_local_srs()
+        xenapi_fake.create_local_pifs()
         db_fakes.stub_out_db_instance_api(self.stubs)
         xenapi_fake.create_network('fake', FLAGS.flat_network_bridge)
         stubs.stubout_session(self.stubs, stubs.FakeSessionForVMTests)
@@ -247,12 +242,12 @@
 
         check()
 
-    def create_vm_record(self, conn, os_type):
+    def create_vm_record(self, conn, os_type, instance_id=1):
         instances = conn.list_instances()
-        self.assertEquals(instances, ['1'])
+        self.assertEquals(instances, [str(instance_id)])
 
         # Get Nova record for VM
-        vm_info = conn.get_info(1)
+        vm_info = conn.get_info(instance_id)
         # Get XenAPI record for VM
         vms = [rec for ref, rec
                in xenapi_fake.get_all_records('VM').iteritems()
@@ -286,19 +281,19 @@
             key = 'vm-data/networking/aabbccddeeff'
             xenstore_value = xenstore_data[key]
             tcpip_data = ast.literal_eval(xenstore_value)
-            self.assertEquals(tcpip_data, {
-                'label': 'test_network',
-                'broadcast': '10.0.0.255',
-                'ips': [{'ip': '10.0.0.3',
-                         'netmask':'255.255.255.0',
-                         'enabled':'1'}],
-                'ip6s': [{'ip': 'fe80::a8bb:ccff:fedd:eeff',
-                          'netmask': '120',
-                          'enabled': '1',
-                          'gateway': 'fe80::a00:1'}],
-                'mac': 'aa:bb:cc:dd:ee:ff',
-                'dns': ['10.0.0.2'],
-                'gateway': '10.0.0.1'})
+            self.assertEquals(tcpip_data,
+                              {'label': 'fake_flat_network',
+                               'broadcast': '10.0.0.255',
+                               'ips': [{'ip': '10.0.0.3',
+                                        'netmask':'255.255.255.0',
+                                        'enabled':'1'}],
+                                'ip6s': [{'ip': 'fe80::a8bb:ccff:fedd:eeff',
+                                          'netmask': '120',
+                                          'enabled': '1',
+                                          'gateway': 'fe80::a00:1'}],
+                                'mac': 'aa:bb:cc:dd:ee:ff',
+                                'dns': ['10.0.0.2'],
+                                'gateway': '10.0.0.1'})
 
     def check_vm_params_for_windows(self):
         self.assertEquals(self.vm['platform']['nx'], 'true')
@@ -334,9 +329,9 @@
 
     def _test_spawn(self, image_id, kernel_id, ramdisk_id,
                     instance_type="m1.large", os_type="linux",
-                    check_injection=False):
+                    instance_id=1, check_injection=False):
         stubs.stubout_loopingcall_start(self.stubs)
-        values = {'id': 1,
+        values = {'id': instance_id,
                   'project_id': self.project.id,
                   'user_id': self.user.id,
                   'image_id': image_id,
@@ -347,7 +342,7 @@
                   'os_type': os_type}
         instance = db.instance_create(self.context, values)
         self.conn.spawn(instance)
-        self.create_vm_record(self.conn, os_type)
+        self.create_vm_record(self.conn, os_type, instance_id)
         self.check_vm_record(self.conn, check_injection)
 
     def test_spawn_not_enough_memory(self):
@@ -468,6 +463,28 @@
         # guest agent is detected
         self.assertFalse(self._tee_executed)
 
+    def test_spawn_vlanmanager(self):
+        self.flags(xenapi_image_service='glance',
+                   network_manager='nova.network.manager.VlanManager',
+                   network_driver='nova.network.xenapi_net',
+                   vlan_interface='fake0')
+        # Reset network table
+        xenapi_fake.reset_table('network')
+        # Instance id = 2 will use vlan network (see db/fakes.py)
+        fake_instance_id = 2
+        network_bk = self.network
+        # Ensure we use xenapi_net driver
+        self.network = utils.import_object(FLAGS.network_manager)
+        self.network.setup_compute_network(None, fake_instance_id)
+        self._test_spawn(glance_stubs.FakeGlance.IMAGE_MACHINE,
+                         glance_stubs.FakeGlance.IMAGE_KERNEL,
+                         glance_stubs.FakeGlance.IMAGE_RAMDISK,
+                         instance_id=fake_instance_id)
+        # TODO(salvatore-orlando): a complete test here would require
+        # a check for making sure the bridge for the VM's VIF is
+        # consistent with bridge specified in nova db
+        self.network = network_bk
+
     def test_spawn_with_network_qos(self):
         self._create_instance()
         for vif_ref in xenapi_fake.get_all('VIF'):
@@ -497,7 +514,7 @@
         self.stubs.UnsetAll()
 
     def _create_instance(self):
-        """Creates and spawns a test instance"""
+        """Creates and spawns a test instance."""
         stubs.stubout_loopingcall_start(self.stubs)
         values = {
             'id': 1,
@@ -515,9 +532,7 @@
 
 
 class XenAPIDiffieHellmanTestCase(test.TestCase):
-    """
-    Unit tests for Diffie-Hellman code
-    """
+    """Unit tests for Diffie-Hellman code."""
     def setUp(self):
         super(XenAPIDiffieHellmanTestCase, self).setUp()
         self.alice = SimpleDH()
@@ -541,9 +556,7 @@
 
 
 class XenAPIMigrateInstance(test.TestCase):
-    """
-    Unit test for verifying migration-related actions
-    """
+    """Unit test for verifying migration-related actions."""
 
     def setUp(self):
         super(XenAPIMigrateInstance, self).setUp()
@@ -598,9 +611,7 @@
 
 
 class XenAPIDetermineDiskImageTestCase(test.TestCase):
-    """
-    Unit tests for code that detects the ImageType
-    """
+    """Unit tests for code that detects the ImageType."""
     def setUp(self):
         super(XenAPIDetermineDiskImageTestCase, self).setUp()
         glance_stubs.stubout_glance_client(self.stubs,
@@ -619,9 +630,7 @@
         self.assertEqual(disk_type, dt)
 
     def test_instance_disk(self):
-        """
-        If a kernel is specified then the image type is DISK (aka machine)
-        """
+        """If a kernel is specified, the image type is DISK (aka machine)."""
         FLAGS.xenapi_image_service = 'objectstore'
         self.fake_instance.image_id = glance_stubs.FakeGlance.IMAGE_MACHINE
         self.fake_instance.kernel_id = glance_stubs.FakeGlance.IMAGE_KERNEL

=== modified file 'nova/virt/xenapi/fake.py'
--- nova/virt/xenapi/fake.py	2011-03-24 02:01:46 +0000
+++ nova/virt/xenapi/fake.py	2011-03-29 10:28:19 +0000
@@ -60,8 +60,8 @@
 from nova import log as logging
 
 
-_CLASSES = ['host', 'network', 'session', 'SR', 'VBD',\
-            'PBD', 'VDI', 'VIF', 'VM', 'task']
+_CLASSES = ['host', 'network', 'session', 'SR', 'VBD',
+            'PBD', 'VDI', 'VIF', 'PIF', 'VM', 'VLAN', 'task']
 
 _db_content = {}
 
@@ -78,30 +78,36 @@
     for c in _CLASSES:
         _db_content[c] = {}
     create_host('fake')
-    create_vm('fake', 'Running', is_a_template=False, is_control_domain=True)
+    create_vm('fake',
+              'Running',
+              is_a_template=False,
+              is_control_domain=True)
+
+
+def reset_table(table):
+    if not table in _CLASSES:
+        return
+    _db_content[table] = {}
 
 
 def create_host(name_label):
-    return _create_object('host', {
-        'name_label': name_label,
-        })
+    return _create_object('host',
+                          {'name_label': name_label})
 
 
 def create_network(name_label, bridge):
-    return _create_object('network', {
-        'name_label': name_label,
-        'bridge': bridge,
-        })
+    return _create_object('network',
+                          {'name_label': name_label,
+                           'bridge': bridge})
 
 
 def create_vm(name_label, status,
               is_a_template=False, is_control_domain=False):
-    return _create_object('VM', {
-        'name_label': name_label,
-        'power-state': status,
-        'is_a_template': is_a_template,
-        'is_control_domain': is_control_domain,
-        })
+    return _create_object('VM',
+                          {'name_label': name_label,
+                           'power-state': status,
+                           'is_a_template': is_a_template,
+                           'is_control_domain': is_control_domain})
 
 
 def destroy_vm(vm_ref):
@@ -123,27 +129,24 @@
 
 
 def create_vdi(name_label, read_only, sr_ref, sharable):
-    return _create_object('VDI', {
-        'name_label': name_label,
-        'read_only': read_only,
-        'SR': sr_ref,
-        'type': '',
-        'name_description': '',
-        'sharable': sharable,
-        'other_config': {},
-        'location': '',
-        'xenstore_data': '',
-        'sm_config': {},
-        'VBDs': {},
-        })
+    return _create_object('VDI',
+                          {'name_label': name_label,
+                           'read_only': read_only,
+                           'SR': sr_ref,
+                           'type': '',
+                           'name_description': '',
+                           'sharable': sharable,
+                           'other_config': {},
+                           'location': '',
+                           'xenstore_data': '',
+                           'sm_config': {},
+                           'VBDs': {}})
 
 
 def create_vbd(vm_ref, vdi_ref):
-    vbd_rec = {
-        'VM': vm_ref,
-        'VDI': vdi_ref,
-        'currently_attached': False,
-        }
+    vbd_rec = {'VM': vm_ref,
+               'VDI': vdi_ref,
+               'currently_attached': False}
     vbd_ref = _create_object('VBD', vbd_rec)
     after_VBD_create(vbd_ref, vbd_rec)
     return vbd_ref
@@ -169,19 +172,24 @@
 
 
 def create_pbd(config, host_ref, sr_ref, attached):
-    return _create_object('PBD', {
-        'device-config': config,
-        'host': host_ref,
-        'SR': sr_ref,
-        'currently-attached': attached,
-        })
+    return _create_object('PBD',
+                          {'device-config': config,
+                           'host': host_ref,
+                           'SR': sr_ref,
+                           'currently-attached': attached})
 
 
 def create_task(name_label):
-    return _create_object('task', {
-        'name_label': name_label,
-        'status': 'pending',
-        })
+    return _create_object('task',
+                          {'name_label': name_label,
+                           'status': 'pending'})
+
+
+def create_local_pifs():
+    """Adds a PIF for each to the local database with VLAN=-1.
+       Do this one per host."""
+    for host_ref in _db_content['host'].keys():
+        _create_local_pif(host_ref)
 
 
 def create_local_srs():
@@ -192,25 +200,34 @@
 
 
 def _create_local_sr(host_ref):
-    sr_ref = _create_object('SR', {
-        'name_label': 'Local storage',
-        'type': 'lvm',
-        'content_type': 'user',
-        'shared': False,
-        'physical_size': str(1 << 30),
-        'physical_utilisation': str(0),
-        'virtual_allocation': str(0),
-        'other_config': {
-            'i18n-original-value-name_label': 'Local storage',
-            'i18n-key': 'local-storage',
-            },
-        'VDIs': []
-        })
+    sr_ref = _create_object(
+             'SR',
+             {'name_label': 'Local storage',
+              'type': 'lvm',
+              'content_type': 'user',
+              'shared': False,
+              'physical_size': str(1 << 30),
+              'physical_utilisation': str(0),
+              'virtual_allocation': str(0),
+              'other_config': {
+                     'i18n-original-value-name_label': 'Local storage',
+                     'i18n-key': 'local-storage'},
+              'VDIs': []})
     pbd_ref = create_pbd('', host_ref, sr_ref, True)
     _db_content['SR'][sr_ref]['PBDs'] = [pbd_ref]
     return sr_ref
 
 
+def _create_local_pif(host_ref):
+    pif_ref = _create_object('PIF',
+                             {'name-label': 'Fake PIF',
+                              'MAC': '00:11:22:33:44:55',
+                              'physical': True,
+                              'VLAN': -1,
+                              'device': 'fake0',
+                              'host_uuid': host_ref})
+
+
 def _create_object(table, obj):
     ref = str(uuid.uuid4())
     obj['uuid'] = str(uuid.uuid4())
@@ -234,6 +251,21 @@
     return sr_ref
 
 
+def _create_vlan(pif_ref, vlan_num, network_ref):
+    pif_rec = get_record('PIF', pif_ref)
+    vlan_pif_ref = _create_object('PIF',
+                                  {'name-label': 'Fake VLAN PIF',
+                                   'MAC': '00:11:22:33:44:55',
+                                   'physical': True,
+                                   'VLAN': vlan_num,
+                                   'device': pif_rec['device'],
+                                   'host_uuid': pif_rec['host_uuid']})
+    return _create_object('VLAN',
+                          {'tagged-pif': pif_ref,
+                           'untagged-pif': vlan_pif_ref,
+                           'tag': vlan_num})
+
+
 def get_all(table):
     return _db_content[table].keys()
 
@@ -292,6 +324,10 @@
         rec['currently_attached'] = False
         rec['device'] = ''
 
+    def PIF_get_all_records_where(self, _1, _2):
+        # TODO (salvatore-orlando): filter table on _2
+        return _db_content['PIF']
+
     def VM_get_xenstore_data(self, _1, vm_ref):
         return _db_content['VM'][vm_ref].get('xenstore_data', '')
 
@@ -302,7 +338,7 @@
         db_ref['xenstore_data'][key] = None
 
     def network_get_all_records_where(self, _1, _2):
-        # TODO (salvatore-orlando):filter table on _2
+        # TODO (salvatore-orlando): filter table on _2
         return _db_content['network']
 
     def VM_add_to_xenstore_data(self, _1, vm_ref, key, value):
@@ -318,6 +354,9 @@
     def host_call_plugin(*args):
         return 'herp'
 
+    def network_get_all_records_where(self, _1, filter):
+        return self.xenapi.network.get_all_records()
+
     def xenapi_request(self, methodname, params):
         if methodname.startswith('login'):
             self._login(methodname, params)
@@ -337,10 +376,9 @@
 
     def _login(self, method, params):
         self._session = str(uuid.uuid4())
-        _db_content['session'][self._session] = {
-            'uuid': str(uuid.uuid4()),
-            'this_host': _db_content['host'].keys()[0],
-            }
+        _db_content['session'][self._session] = \
+                                {'uuid': str(uuid.uuid4()),
+                                 'this_host': _db_content['host'].keys()[0]}
 
     def _logout(self):
         s = self._session
@@ -456,12 +494,16 @@
     def _create(self, name, params):
         self._check_session(params)
         is_sr_create = name == 'SR.create'
+        is_vlan_create = name == 'VLAN.create'
         # Storage Repositories have a different API
-        expected = is_sr_create and 10 or 2
+        expected = is_sr_create and 10 or is_vlan_create and 4 or 2
         self._check_arg_count(params, expected)
         (cls, _) = name.split('.')
         ref = is_sr_create and \
-            _create_sr(cls, params) or _create_object(cls, params[1])
+              _create_sr(cls, params) or \
+              is_vlan_create and \
+              _create_vlan(params[1], params[2], params[3]) or \
+              _create_object(cls, params[1])
 
         # Call hook to provide any fixups needed (ex. creating backrefs)
         after_hook = 'after_%s_create' % cls

=== modified file 'nova/virt/xenapi/network_utils.py'
--- nova/virt/xenapi/network_utils.py	2010-12-23 03:35:41 +0000
+++ nova/virt/xenapi/network_utils.py	2011-03-29 10:28:19 +0000
@@ -28,11 +28,26 @@
     """
     The class that wraps the helper methods together.
     """
+    @classmethod
+    def find_network_with_name_label(cls, session, name_label):
+        networks = session.call_xenapi('network.get_by_name_label', name_label)
+        if len(networks) == 1:
+            return networks[0]
+        elif len(networks) > 1:
+            raise Exception(_('Found non-unique network'
+                              ' for name_label %s') % name_label)
+        else:
+            return None
 
     @classmethod
     def find_network_with_bridge(cls, session, bridge):
-        """Return the network on which the bridge is attached, if found."""
-        expr = 'field "bridge" = "%s"' % bridge
+        """
+        Return the network on which the bridge is attached, if found.
+        The bridge is defined in the nova db and can be found either in the
+        'bridge' or 'name_label' fields of the XenAPI network record.
+        """
+        expr = 'field "name__label" = "%s" or ' \
+               'field "bridge" = "%s"' % (bridge, bridge)
         networks = session.call_xenapi('network.get_all_records_where', expr)
         if len(networks) == 1:
             return networks.keys()[0]

=== modified file 'nova/virt/xenapi/vmops.py'
--- nova/virt/xenapi/vmops.py	2011-03-25 00:16:53 +0000
+++ nova/virt/xenapi/vmops.py	2011-03-29 10:28:19 +0000
@@ -58,7 +58,7 @@
         VMHelper.XenAPI = self.XenAPI
 
     def list_instances(self):
-        """List VM instances"""
+        """List VM instances."""
         # TODO(justinsb): Should we just always use the details method?
         #  Seems to be the same number of API calls..
         vm_refs = []
@@ -69,7 +69,7 @@
         return vm_refs
 
     def list_instances_detail(self):
-        """List VM instances, returning InstanceInfo objects"""
+        """List VM instances, returning InstanceInfo objects."""
         instance_infos = []
         for vm_ref in self._session.get_xenapi().VM.get_all():
             vm_rec = self._session.get_xenapi().VM.get_record(vm_ref)
@@ -119,11 +119,11 @@
         self._spawn(instance, vm_ref)
 
     def spawn_rescue(self, instance):
-        """Spawn a rescue instance"""
+        """Spawn a rescue instance."""
         self.spawn(instance)
 
     def _create_vm(self, instance, vdi_uuid, network_info=None):
-        """Create VM instance"""
+        """Create VM instance."""
         instance_name = instance.name
         vm_ref = VMHelper.lookup(self._session, instance_name)
         if vm_ref is not None:
@@ -180,7 +180,7 @@
         return vm_ref
 
     def _spawn(self, instance, vm_ref):
-        """Spawn a new instance"""
+        """Spawn a new instance."""
         LOG.debug(_('Starting VM %s...'), vm_ref)
         self._start(instance, vm_ref)
         instance_name = instance.name
@@ -236,7 +236,8 @@
         return timer.start(interval=0.5, now=True)
 
     def _get_vm_opaque_ref(self, instance_or_vm):
-        """Refactored out the common code of many methods that receive either
+        """
+        Refactored out the common code of many methods that receive either
         a vm name or a vm instance, and want a vm instance in return.
         """
         # if instance_or_vm is a string it must be opaque ref or instance name
@@ -264,21 +265,21 @@
         return vm_ref
 
     def _acquire_bootlock(self, vm):
-        """Prevent an instance from booting"""
+        """Prevent an instance from booting."""
         self._session.call_xenapi(
             "VM.set_blocked_operations",
             vm,
             {"start": ""})
 
     def _release_bootlock(self, vm):
-        """Allow an instance to boot"""
+        """Allow an instance to boot."""
         self._session.call_xenapi(
             "VM.remove_from_blocked_operations",
             vm,
             "start")
 
     def snapshot(self, instance, image_id):
-        """Create snapshot from a running VM instance
+        """Create snapshot from a running VM instance.
 
         :param instance: instance to be snapshotted
         :param image_id: id of image to upload to
@@ -298,6 +299,7 @@
         3. Push-to-glance: Once coalesced, we call a plugin on the XenServer
             that will bundle the VHDs together and then push the bundle into
             Glance.
+
         """
         template_vm_ref = None
         try:
@@ -330,11 +332,12 @@
             return
 
     def migrate_disk_and_power_off(self, instance, dest):
-        """Copies a VHD from one host machine to another
-
-        :param instance: the instance that owns the VHD in question
-        :param dest: the destination host machine
-        :param disk_type: values are 'primary' or 'cow'
+        """Copies a VHD from one host machine to another.
+
+        :param instance: the instance that owns the VHD in question.
+        :param dest: the destination host machine.
+        :param disk_type: values are 'primary' or 'cow'.
+
         """
         vm_ref = VMHelper.lookup(self._session, instance.name)
 
@@ -383,7 +386,7 @@
         return {'base_copy': base_copy_uuid, 'cow': cow_uuid}
 
     def link_disks(self, instance, base_copy_uuid, cow_uuid):
-        """Links the base copy VHD to the COW via the XAPI plugin"""
+        """Links the base copy VHD to the COW via the XAPI plugin."""
         vm_ref = VMHelper.lookup(self._session, instance.name)
         new_base_copy_uuid = str(uuid.uuid4())
         new_cow_uuid = str(uuid.uuid4())
@@ -404,7 +407,7 @@
         return new_cow_uuid
 
     def resize_instance(self, instance, vdi_uuid):
-        """Resize a running instance by changing it's RAM and disk size """
+        """Resize a running instance by changing it's RAM and disk size."""
         #TODO(mdietz): this will need to be adjusted for swap later
         #The new disk size must be in bytes
 
@@ -418,18 +421,20 @@
         LOG.debug(_("Resize instance %s complete") % (instance.name))
 
     def reboot(self, instance):
-        """Reboot VM instance"""
+        """Reboot VM instance."""
         vm_ref = self._get_vm_opaque_ref(instance)
         task = self._session.call_xenapi('Async.VM.clean_reboot', vm_ref)
         self._session.wait_for_task(task, instance.id)
 
     def set_admin_password(self, instance, new_pass):
-        """Set the root/admin password on the VM instance. This is done via
-        an agent running on the VM. Communication between nova and the agent
-        is done via writing xenstore records. Since communication is done over
-        the XenAPI RPC calls, we need to encrypt the password. We're using a
-        simple Diffie-Hellman class instead of the more advanced one in
-        M2Crypto for compatibility with the agent code.
+        """Set the root/admin password on the VM instance.
+
+        This is done via an agent running on the VM. Communication between nova
+        and the agent is done via writing xenstore records. Since communication
+        is done over the XenAPI RPC calls, we need to encrypt the password.
+        We're using a simple Diffie-Hellman class instead of the more advanced
+        one in M2Crypto for compatibility with the agent code.
+
         """
         # Need to uniquely identify this request.
         transaction_id = str(uuid.uuid4())
@@ -462,11 +467,14 @@
         return resp_dict['message']
 
     def inject_file(self, instance, path, contents):
-        """Write a file to the VM instance. The path to which it is to be
-        written and the contents of the file need to be supplied; both will
-        be base64-encoded to prevent errors with non-ASCII characters being
-        transmitted. If the agent does not support file injection, or the user
-        has disabled it, a NotImplementedError will be raised.
+        """Write a file to the VM instance.
+
+        The path to which it is to be written and the contents of the file
+        need to be supplied; both will be base64-encoded to prevent errors
+        with non-ASCII characters being transmitted. If the agent does not
+        support file injection, or the user has disabled it, a
+        NotImplementedError will be raised.
+
         """
         # Files/paths must be base64-encoded for transmission to agent
         b64_path = base64.b64encode(path)
@@ -487,7 +495,7 @@
         return resp_dict['message']
 
     def _shutdown(self, instance, vm_ref, hard=True):
-        """Shutdown an instance"""
+        """Shutdown an instance."""
         state = self.get_info(instance['name'])['state']
         if state == power_state.SHUTDOWN:
             instance_name = instance.name
@@ -511,11 +519,11 @@
             LOG.exception(exc)
 
     def _shutdown_rescue(self, rescue_vm_ref):
-        """Shutdown a rescue instance"""
+        """Shutdown a rescue instance."""
         self._session.call_xenapi("Async.VM.hard_shutdown", rescue_vm_ref)
 
     def _destroy_vdis(self, instance, vm_ref):
-        """Destroys all VDIs associated with a VM"""
+        """Destroys all VDIs associated with a VM."""
         instance_id = instance.id
         LOG.debug(_("Destroying VDIs for Instance %(instance_id)s")
                   % locals())
@@ -532,7 +540,7 @@
                 LOG.exception(exc)
 
     def _destroy_rescue_vdis(self, rescue_vm_ref):
-        """Destroys all VDIs associated with a rescued VM"""
+        """Destroys all VDIs associated with a rescued VM."""
         vdi_refs = VMHelper.lookup_vm_vdis(self._session, rescue_vm_ref)
         for vdi_ref in vdi_refs:
             try:
@@ -541,7 +549,7 @@
                 continue
 
     def _destroy_rescue_vbds(self, rescue_vm_ref):
-        """Destroys all VBDs tied to a rescue VM"""
+        """Destroys all VBDs tied to a rescue VM."""
         vbd_refs = self._session.get_xenapi().VM.get_VBDs(rescue_vm_ref)
         for vbd_ref in vbd_refs:
             vbd_rec = self._session.get_xenapi().VBD.get_record(vbd_ref)
@@ -550,8 +558,7 @@
                 VMHelper.destroy_vbd(self._session, vbd_ref)
 
     def _destroy_kernel_ramdisk(self, instance, vm_ref):
-        """
-        Three situations can occur:
+        """Three situations can occur:
 
             1. We have neither a ramdisk nor a kernel, in which case we are a
                RAW image and can omit this step
@@ -561,6 +568,7 @@
 
             3. We have both, in which case we safely remove both the kernel
                and the ramdisk.
+
         """
         instance_id = instance.id
         if not instance.kernel_id and not instance.ramdisk_id:
@@ -589,7 +597,7 @@
         LOG.debug(_("kernel/ramdisk files removed"))
 
     def _destroy_vm(self, instance, vm_ref):
-        """Destroys a VM record"""
+        """Destroys a VM record."""
         instance_id = instance.id
         try:
             task = self._session.call_xenapi('Async.VM.destroy', vm_ref)
@@ -600,7 +608,7 @@
         LOG.debug(_("Instance %(instance_id)s VM destroyed") % locals())
 
     def _destroy_rescue_instance(self, rescue_vm_ref):
-        """Destroy a rescue instance"""
+        """Destroy a rescue instance."""
         self._destroy_rescue_vbds(rescue_vm_ref)
         self._shutdown_rescue(rescue_vm_ref)
         self._destroy_rescue_vdis(rescue_vm_ref)
@@ -608,11 +616,11 @@
         self._session.call_xenapi("Async.VM.destroy", rescue_vm_ref)
 
     def destroy(self, instance):
-        """
-        Destroy VM instance
+        """Destroy VM instance.
 
         This is the method exposed by xenapi_conn.destroy(). The rest of the
         destroy_* methods are internal.
+
         """
         instance_id = instance.id
         LOG.info(_("Destroying VM for Instance %(instance_id)s") % locals())
@@ -621,13 +629,13 @@
 
     def _destroy(self, instance, vm_ref, shutdown=True,
                  destroy_kernel_ramdisk=True):
-        """
-        Destroys VM instance by performing:
-
-            1. A shutdown if requested
-            2. Destroying associated VDIs
-            3. Destroying kernel and ramdisk files (if necessary)
-            4. Destroying that actual VM record
+        """Destroys VM instance by performing:
+
+            1. A shutdown if requested.
+            2. Destroying associated VDIs.
+            3. Destroying kernel and ramdisk files (if necessary).
+            4. Destroying that actual VM record.
+
         """
         if vm_ref is None:
             LOG.warning(_("VM is not present, skipping destroy..."))
@@ -650,35 +658,36 @@
         callback(ret)
 
     def pause(self, instance, callback):
-        """Pause VM instance"""
+        """Pause VM instance."""
         vm_ref = self._get_vm_opaque_ref(instance)
         task = self._session.call_xenapi('Async.VM.pause', vm_ref)
         self._wait_with_callback(instance.id, task, callback)
 
     def unpause(self, instance, callback):
-        """Unpause VM instance"""
+        """Unpause VM instance."""
         vm_ref = self._get_vm_opaque_ref(instance)
         task = self._session.call_xenapi('Async.VM.unpause', vm_ref)
         self._wait_with_callback(instance.id, task, callback)
 
     def suspend(self, instance, callback):
-        """suspend the specified instance"""
+        """Suspend the specified instance."""
         vm_ref = self._get_vm_opaque_ref(instance)
         task = self._session.call_xenapi('Async.VM.suspend', vm_ref)
         self._wait_with_callback(instance.id, task, callback)
 
     def resume(self, instance, callback):
-        """resume the specified instance"""
+        """Resume the specified instance."""
         vm_ref = self._get_vm_opaque_ref(instance)
         task = self._session.call_xenapi('Async.VM.resume', vm_ref, False,
                                          True)
         self._wait_with_callback(instance.id, task, callback)
 
     def rescue(self, instance, callback):
-        """Rescue the specified instance
-            - shutdown the instance VM
-            - set 'bootlock' to prevent the instance from starting in rescue
-            - spawn a rescue VM (the vm name-label will be instance-N-rescue)
+        """Rescue the specified instance.
+
+            - shutdown the instance VM.
+            - set 'bootlock' to prevent the instance from starting in rescue.
+            - spawn a rescue VM (the vm name-label will be instance-N-rescue).
 
         """
         rescue_vm_ref = VMHelper.lookup(self._session,
@@ -702,10 +711,11 @@
         self._session.call_xenapi("Async.VBD.plug", rescue_vbd_ref)
 
     def unrescue(self, instance, callback):
-        """Unrescue the specified instance
-            - unplug the instance VM's disk from the rescue VM
-            - teardown the rescue VM
-            - release the bootlock to allow the instance VM to start
+        """Unrescue the specified instance.
+
+            - unplug the instance VM's disk from the rescue VM.
+            - teardown the rescue VM.
+            - release the bootlock to allow the instance VM to start.
 
         """
         rescue_vm_ref = VMHelper.lookup(self._session,
@@ -723,9 +733,11 @@
         self._start(instance, original_vm_ref)
 
     def poll_rescued_instances(self, timeout):
-        """Look for expirable rescued instances
+        """Look for expirable rescued instances.
+
             - forcibly exit rescue mode for any instances that have been
               in rescue mode for >= the provided timeout
+
         """
         last_ran = self.poll_rescue_last_ran
         if not last_ran:
@@ -761,30 +773,30 @@
                                       False)
 
     def get_info(self, instance):
-        """Return data about VM instance"""
+        """Return data about VM instance."""
         vm_ref = self._get_vm_opaque_ref(instance)
         vm_rec = self._session.get_xenapi().VM.get_record(vm_ref)
         return VMHelper.compile_info(vm_rec)
 
     def get_diagnostics(self, instance):
-        """Return data about VM diagnostics"""
+        """Return data about VM diagnostics."""
         vm_ref = self._get_vm_opaque_ref(instance)
         vm_rec = self._session.get_xenapi().VM.get_record(vm_ref)
         return VMHelper.compile_diagnostics(self._session, vm_rec)
 
     def get_console_output(self, instance):
-        """Return snapshot of console"""
+        """Return snapshot of console."""
         # TODO: implement this to fix pylint!
         return 'FAKE CONSOLE OUTPUT of instance'
 
     def get_ajax_console(self, instance):
-        """Return link to instance's ajax console"""
+        """Return link to instance's ajax console."""
         # TODO: implement this!
         return 'http://fakeajaxconsole/fake_url'
 
     # TODO(tr3buchet) - remove this function after nova multi-nic
     def _get_network_info(self, instance):
-        """creates network info list for instance"""
+        """Creates network info list for instance."""
         admin_context = context.get_admin_context()
         IPs = db.fixed_ip_get_all_by_instance(admin_context,
                                               instance['id'])
@@ -826,7 +838,7 @@
     def inject_network_info(self, instance, vm_ref, network_info):
         """
         Generate the network info and make calls to place it into the
-        xenstore and the xenstore param list
+        xenstore and the xenstore param list.
         """
         logging.debug(_("injecting network info to xs for vm: |%s|"), vm_ref)
 
@@ -847,7 +859,7 @@
                 pass
 
     def create_vifs(self, vm_ref, network_info):
-        """Creates vifs for an instance"""
+        """Creates vifs for an instance."""
         logging.debug(_("creating vif(s) for vm: |%s|"), vm_ref)
 
         # this function raises if vm_ref is not a vm_opaque_ref
@@ -858,8 +870,8 @@
             bridge = network['bridge']
             rxtx_cap = info.pop('rxtx_cap')
             network_ref = \
-                NetworkHelper.find_network_with_bridge(self._session, bridge)
-
+                NetworkHelper.find_network_with_bridge(self._session,
+                                                       bridge)
             VMHelper.create_vif(self._session, vm_ref, network_ref,
                                 mac_address, device, rxtx_cap)
 
@@ -872,7 +884,8 @@
                                                                args, vm_ref)
 
     def list_from_xenstore(self, vm, path):
-        """Runs the xenstore-ls command to get a listing of all records
+        """
+        Runs the xenstore-ls command to get a listing of all records
         from 'path' downward. Returns a dict with the sub-paths as keys,
         and the value stored in those paths as values. If nothing is
         found at that path, returns None.
@@ -881,7 +894,8 @@
         return json.loads(ret)
 
     def read_from_xenstore(self, vm, path):
-        """Returns the value stored in the xenstore record for the given VM
+        """
+        Returns the value stored in the xenstore record for the given VM
         at the specified location. A XenAPIPlugin.PluginError will be raised
         if any error is encountered in the read process.
         """
@@ -897,7 +911,8 @@
         return ret
 
     def write_to_xenstore(self, vm, path, value):
-        """Writes the passed value to the xenstore record for the given VM
+        """
+        Writes the passed value to the xenstore record for the given VM
         at the specified location. A XenAPIPlugin.PluginError will be raised
         if any error is encountered in the write process.
         """
@@ -905,7 +920,8 @@
                 {'value': json.dumps(value)})
 
     def clear_xenstore(self, vm, path):
-        """Deletes the VM's xenstore record for the specified path.
+        """
+        Deletes the VM's xenstore record for the specified path.
         If there is no such record, the request is ignored.
         """
         self._make_xenstore_call('delete_record', vm, path)
@@ -922,7 +938,8 @@
 
     def _make_plugin_call(self, plugin, method, vm, path, addl_args=None,
                                                           vm_ref=None):
-        """Abstracts out the process of calling a method of a xenapi plugin.
+        """
+        Abstracts out the process of calling a method of a xenapi plugin.
         Any errors raised by the plugin will in turn raise a RuntimeError here.
         """
         instance_id = vm.id
@@ -952,7 +969,8 @@
         return ret
 
     def add_to_xenstore(self, vm, path, key, value):
-        """Adds the passed key/value pair to the xenstore record for
+        """
+        Adds the passed key/value pair to the xenstore record for
         the given VM at the specified location. A XenAPIPlugin.PluginError
         will be raised if any error is encountered in the write process.
         """
@@ -965,7 +983,8 @@
         self.write_to_xenstore(vm, path, current)
 
     def remove_from_xenstore(self, vm, path, key_or_keys):
-        """Takes either a single key or a list of keys and removes
+        """
+        Takes either a single key or a list of keys and removes
         them from the xenstoreirecord data for the given VM.
         If the key doesn't exist, the request is ignored.
         """
@@ -992,7 +1011,8 @@
     ###### names to distinguish them. (dabo)
     ########################################################################
     def read_partial_from_param_xenstore(self, instance_or_vm, key_prefix):
-        """Returns a dict of all the keys in the xenstore parameter record
+        """
+        Returns a dict of all the keys in the xenstore parameter record
         for the given instance that begin with the key_prefix.
         """
         data = self.read_from_param_xenstore(instance_or_vm)
@@ -1003,7 +1023,8 @@
         return data
 
     def read_from_param_xenstore(self, instance_or_vm, keys=None):
-        """Returns the xenstore parameter record data for the specified VM
+        """
+        Returns the xenstore parameter record data for the specified VM
         instance as a dict. Accepts an optional key or list of keys; if a
         value for 'keys' is passed, the returned dict is filtered to only
         return the values for those keys.
@@ -1025,9 +1046,11 @@
         return ret
 
     def add_to_param_xenstore(self, instance_or_vm, key, val):
-        """Takes a key/value pair and adds it to the xenstore parameter
+        """
+        Takes a key/value pair and adds it to the xenstore parameter
         record for the given vm instance. If the key exists in xenstore,
-        it is overwritten"""
+        it is overwritten
+        """
         vm_ref = self._get_vm_opaque_ref(instance_or_vm)
         self.remove_from_param_xenstore(instance_or_vm, key)
         jsonval = json.dumps(val)
@@ -1035,7 +1058,8 @@
                                           (vm_ref, key, jsonval))
 
     def write_to_param_xenstore(self, instance_or_vm, mapping):
-        """Takes a dict and writes each key/value pair to the xenstore
+        """
+        Takes a dict and writes each key/value pair to the xenstore
         parameter record for the given vm instance. Any existing data for
         those keys is overwritten.
         """
@@ -1043,7 +1067,8 @@
             self.add_to_param_xenstore(instance_or_vm, k, v)
 
     def remove_from_param_xenstore(self, instance_or_vm, key_or_keys):
-        """Takes either a single key or a list of keys and removes
+        """
+        Takes either a single key or a list of keys and removes
         them from the xenstore parameter record data for the given VM.
         If the key doesn't exist, the request is ignored.
         """
@@ -1069,7 +1094,8 @@
 
 
 class SimpleDH(object):
-    """This class wraps all the functionality needed to implement
+    """
+    This class wraps all the functionality needed to implement
     basic Diffie-Hellman-Merkle key exchange in Python. It features
     intelligent defaults for the prime and base numbers needed for the
     calculation, while allowing you to supply your own. It requires that
@@ -1078,7 +1104,8 @@
     is not available, a RuntimeError will be raised.
     """
     def __init__(self, prime=None, base=None, secret=None):
-        """You can specify the values for prime and base if you wish;
+        """
+        You can specify the values for prime and base if you wish;
         otherwise, reasonable default values will be used.
         """
         if prime is None: