Merge lp:~vishvananda/nova/network-refactor into lp:~hudson-openstack/nova/trunk

[Vish Ishaya]

Improves pep8 compliance and pylint score in network code.

Mostly a whole bunch of pep and pylint cleanup, but also fixes the ordering of commands on ip release so that ips are reused.

[Jay Pipes (jaypipes)]

This is excellent work, Vish. In particular, I really like how you cleaned up (and made more explicit and descriptive) the way that the BaseNetwork and its subclasses provide information on reserved IP addresses. The way I had done it before was sloppy :)

Nice job.

review: Approve

[Jesse Andrews (anotherjesse)]

lgtm, we are using this already inside nebula

review: Approve

=== modified file 'bin/nova-dhcpbridge'
--- bin/nova-dhcpbridge	2010-08-08 02:51:17 +0000
+++ bin/nova-dhcpbridge	2010-08-10 22:55:54 +0000
@@ -56,7 +56,7 @@
 
 
 def del_lease(_mac, ip, _hostname, _interface):
-    """Remove the leased IP from the databases."""
+    """Called when a lease expires."""
     if FLAGS.fake_rabbit:
         service.VlanNetworkService().release_ip(ip)
     else:
@@ -70,8 +70,9 @@
     net = model.get_network_by_interface(interface)
     res = ""
     for host_name in net.hosts:
-        res += "%s\n" % linux_net.hostDHCP(net, host_name,
-                                           net.hosts[host_name])
+        res += "%s\n" % linux_net.host_dhcp(net,
+                                            host_name,
+                                            net.hosts[host_name])
     return res
 
 

=== modified file 'nova/endpoint/cloud.py'
--- nova/endpoint/cloud.py	2010-08-08 18:40:03 +0000
+++ nova/endpoint/cloud.py	2010-08-10 22:55:54 +0000
@@ -103,7 +103,7 @@
         result = {}
         for instance in self.instdir.all:
             if instance['project_id'] == project_id:
-                line = '%s slots=%d' % (instance['private_dns_name'], 
+                line = '%s slots=%d' % (instance['private_dns_name'],
                     INSTANCE_TYPES[instance['instance_type']]['vcpus'])
                 if instance['key_name'] in result:
                     result[instance['key_name']].append(line)
@@ -423,7 +423,7 @@
             i['key_name'] = instance.get('key_name', None)
             if context.user.is_admin():
                 i['key_name'] = '%s (%s, %s)' % (i['key_name'],
-                    instance.get('project_id', None), 
+                    instance.get('project_id', None),
                     instance.get('node_name', ''))
             i['product_codes_set'] = self._convert_to_set(
                 instance.get('product_codes', None), 'product_code')
@@ -560,15 +560,15 @@
         # TODO: Get the real security group of launch in here
         security_group = "default"
         for num in range(int(kwargs['max_count'])):
-            vpn = False
+            is_vpn = False
             if image_id  == FLAGS.vpn_image_id:
-                vpn = True
+                is_vpn = True
             allocate_result = yield rpc.call(network_topic,
                      {"method": "allocate_fixed_ip",
                       "args": {"user_id": context.user.id,
                                "project_id": context.project.id,
                                "security_group": security_group,
-                               "vpn": vpn}})
+                               "is_vpn": is_vpn}})
             allocate_data = allocate_result['result']
             inst = self.instdir.new()
             inst['image_id'] = image_id

=== modified file 'nova/network/exception.py'
--- nova/network/exception.py	2010-08-03 21:31:47 +0000
+++ nova/network/exception.py	2010-08-10 22:55:54 +0000
@@ -24,17 +24,25 @@
 
 
 class NoMoreAddresses(Error):
+    """No More Addresses are available in the network"""
     pass
 
+
 class AddressNotAllocated(Error):
+    """The specified address has not been allocated"""
     pass
 
+
 class AddressAlreadyAssociated(Error):
+    """The specified address has already been associated"""
     pass
 
+
 class AddressNotAssociated(Error):
+    """The specified address is not associated"""
     pass
 
+
 class NotValidNetworkSize(Error):
+    """The network size is not valid"""
     pass
-

=== modified file 'nova/network/linux_net.py'
--- nova/network/linux_net.py	2010-08-03 21:31:47 +0000
+++ nova/network/linux_net.py	2010-08-10 22:55:54 +0000
@@ -15,85 +15,102 @@
 #    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 logging
 import signal
 import os
-import subprocess
 
 # todo(ja): does the definition of network_path belong here?
 
+from nova import flags
 from nova import utils
 
-from nova import flags
-FLAGS=flags.FLAGS
+FLAGS = flags.FLAGS
 
 flags.DEFINE_string('dhcpbridge_flagfile',
                     '/etc/nova/nova-dhcpbridge.conf',
                     'location of flagfile for dhcpbridge')
 
+
 def execute(cmd, addl_env=None):
+    """Wrapper around utils.execute for fake_network"""
     if FLAGS.fake_network:
-        logging.debug("FAKE NET: %s" % cmd)
+        logging.debug("FAKE NET: %s", cmd)
         return "fake", 0
     else:
         return utils.execute(cmd, addl_env=addl_env)
 
+
 def runthis(desc, cmd):
+    """Wrapper around utils.runthis for fake_network"""
     if FLAGS.fake_network:
         return execute(cmd)
     else:
-        return utils.runthis(desc,cmd)
-
-def Popen(cmd):
-    if FLAGS.fake_network:
-        execute(' '.join(cmd))
-    else:
-        subprocess.Popen(cmd)
+        return utils.runthis(desc, cmd)
 
 
 def device_exists(device):
-    (out, err) = execute("ifconfig %s" % device)
+    """Check if ethernet device exists"""
+    (_out, err) = execute("ifconfig %s" % device)
     return not err
 
+
 def confirm_rule(cmd):
+    """Delete and re-add iptables rule"""
     execute("sudo iptables --delete %s" % (cmd))
     execute("sudo iptables -I %s" % (cmd))
 
+
 def remove_rule(cmd):
+    """Remove iptables rule"""
     execute("sudo iptables --delete %s" % (cmd))
 
-def bind_public_ip(ip, interface):
-    runthis("Binding IP to interface: %s", "sudo ip addr add %s dev %s" % (ip, interface))
-
-def unbind_public_ip(ip, interface):
-    runthis("Binding IP to interface: %s", "sudo ip addr del %s dev %s" % (ip, interface))
+
+def bind_public_ip(public_ip, interface):
+    """Bind ip to an interface"""
+    runthis("Binding IP to interface: %s",
+            "sudo ip addr add %s dev %s" % (public_ip, interface))
+
+
+def unbind_public_ip(public_ip, interface):
+    """Unbind a public ip from an interface"""
+    runthis("Binding IP to interface: %s",
+            "sudo ip addr del %s dev %s" % (public_ip, interface))
+
 
 def vlan_create(net):
-    """ create a vlan on on a bridge device unless vlan already exists """
+    """Create a vlan on on a bridge device unless vlan already exists"""
     if not device_exists("vlan%s" % net['vlan']):
         logging.debug("Starting VLAN inteface for %s network", (net['vlan']))
         execute("sudo vconfig set_name_type VLAN_PLUS_VID_NO_PAD")
         execute("sudo vconfig add %s %s" % (FLAGS.bridge_dev, net['vlan']))
         execute("sudo ifconfig vlan%s up" % (net['vlan']))
 
+
 def bridge_create(net):
-    """ create a bridge on a vlan unless it already exists """
+    """Create a bridge on a vlan unless it already exists"""
     if not device_exists(net['bridge_name']):
         logging.debug("Starting Bridge inteface for %s network", (net['vlan']))
         execute("sudo brctl addbr %s" % (net['bridge_name']))
         execute("sudo brctl setfd %s 0" % (net.bridge_name))
         # execute("sudo brctl setageing %s 10" % (net.bridge_name))
         execute("sudo brctl stp %s off" % (net['bridge_name']))
-        execute("sudo brctl addif %s vlan%s" % (net['bridge_name'], net['vlan']))
+        execute("sudo brctl addif %s vlan%s" % (net['bridge_name'],
+                                                net['vlan']))
         if net.bridge_gets_ip:
             execute("sudo ifconfig %s %s broadcast %s netmask %s up" % \
                 (net['bridge_name'], net.gateway, net.broadcast, net.netmask))
-            confirm_rule("FORWARD --in-interface %s -j ACCEPT" % (net['bridge_name']))
+            confirm_rule("FORWARD --in-interface %s -j ACCEPT" %
+                         (net['bridge_name']))
         else:
             execute("sudo ifconfig %s up" % net['bridge_name'])
 
-def dnsmasq_cmd(net):
+
+def _dnsmasq_cmd(net):
+    """Builds dnsmasq command"""
     cmd = ['sudo -E dnsmasq',
         ' --strict-order',
         ' --bind-interfaces',
@@ -101,42 +118,48 @@
         ' --pid-file=%s' % dhcp_file(net['vlan'], 'pid'),
         ' --listen-address=%s' % net.dhcp_listen_address,
         ' --except-interface=lo',
-        ' --dhcp-range=%s,static,600s' % (net.dhcp_range_start),
+        ' --dhcp-range=%s,static,600s' % net.dhcp_range_start,
         ' --dhcp-hostsfile=%s' % dhcp_file(net['vlan'], 'conf'),
         ' --dhcp-script=%s' % bin_file('nova-dhcpbridge'),
         ' --leasefile-ro']
     return ''.join(cmd)
 
-def hostDHCP(network, host, mac):
-    idx = host.split(".")[-1] # Logically, the idx of instances they've launched in this net
+
+def host_dhcp(network, host, mac):
+    """Return a host string for a network, host, and mac"""
+    # Logically, the idx of instances they've launched in this net
+    idx = host.split(".")[-1]
     return "%s,%s-%s-%s.novalocal,%s" % \
         (mac, network['user_id'], network['vlan'], idx, host)
 
-# todo(ja): if the system has restarted or pid numbers have wrapped
+
+# TODO(ja): if the system has restarted or pid numbers have wrapped
 #           then you cannot be certain that the pid refers to the
 #           dnsmasq.  As well, sending a HUP only reloads the hostfile,
 #           so any configuration options (like dchp-range, vlan, ...)
 #           aren't reloaded
 def start_dnsmasq(network):
-    """ (re)starts a dnsmasq server for a given network
+    """(Re)starts a dnsmasq server for a given network
 
     if a dnsmasq instance is already running then send a HUP
     signal causing it to reload, otherwise spawn a new instance
     """
     with open(dhcp_file(network['vlan'], 'conf'), 'w') as f:
         for host_name in network.hosts:
-            f.write("%s\n" % hostDHCP(network, host_name, network.hosts[host_name]))
+            f.write("%s\n" % host_dhcp(network,
+                                      host_name,
+                                      network.hosts[host_name]))
 
     pid = dnsmasq_pid_for(network)
 
     # if dnsmasq is already running, then tell it to reload
     if pid:
-        # todo(ja): use "/proc/%d/cmdline" % (pid) to determine if pid refers
+        # TODO(ja): use "/proc/%d/cmdline" % (pid) to determine if pid refers
         #           correct dnsmasq process
         try:
             os.kill(pid, signal.SIGHUP)
-        except Exception, e:
-            logging.debug("Hupping dnsmasq threw %s", e)
+        except Exception as exc:  # pylint: disable=W0703
+            logging.debug("Hupping dnsmasq threw %s", exc)
 
     # otherwise delete the existing leases file and start dnsmasq
     lease_file = dhcp_file(network['vlan'], 'leases')
@@ -146,31 +169,37 @@
     # FLAGFILE and DNSMASQ_INTERFACE in env
     env = {'FLAGFILE': FLAGS.dhcpbridge_flagfile,
            'DNSMASQ_INTERFACE': network['bridge_name']}
-    execute(dnsmasq_cmd(network), addl_env=env)
+    execute(_dnsmasq_cmd(network), addl_env=env)
+
 
 def stop_dnsmasq(network):
-    """ stops the dnsmasq instance for a given network """
+    """Stops the dnsmasq instance for a given network"""
     pid = dnsmasq_pid_for(network)
 
     if pid:
         try:
             os.kill(pid, signal.SIGTERM)
-        except Exception, e:
-            logging.debug("Killing dnsmasq threw %s", e)
+        except Exception as exc:  # pylint: disable=W0703
+            logging.debug("Killing dnsmasq threw %s", exc)
+
 
 def dhcp_file(vlan, kind):
-    """ return path to a pid, leases or conf file for a vlan """
+    """Return path to a pid, leases or conf file for a vlan"""
 
     return os.path.abspath("%s/nova-%s.%s" % (FLAGS.networks_path, vlan, kind))
 
+
 def bin_file(script):
+    """Return the absolute path to scipt in the bin directory"""
     return os.path.abspath(os.path.join(__file__, "../../../bin", script))
 
+
 def dnsmasq_pid_for(network):
-    """ the pid for prior dnsmasq instance for a vlan,
-    returns None if no pid file exists
-
-    if machine has rebooted pid might be incorrect (caller should check)
+    """Returns he pid for prior dnsmasq instance for a vlan
+
+    Returns None if no pid file exists
+
+    If machine has rebooted pid might be incorrect (caller should check)
     """
 
     pid_file = dhcp_file(network['vlan'], 'pid')
@@ -178,4 +207,3 @@
     if os.path.exists(pid_file):
         with open(pid_file, 'r') as f:
             return int(f.read())
-

=== modified file 'nova/network/model.py'
--- nova/network/model.py	2010-08-08 20:20:50 +0000
+++ nova/network/model.py	2010-08-10 22:55:54 +0000
@@ -57,7 +57,8 @@
 
 
 class Vlan(datastore.BasicModel):
-    def __init__(self, project, vlan):
+    """Tracks vlans assigned to project it the datastore"""
+    def __init__(self, project, vlan):  # pylint: disable=W0231
         """
         Since we don't want to try and find a vlan by its identifier,
         but by a project id, we don't call super-init.
@@ -67,10 +68,12 @@
 
     @property
     def identifier(self):
+        """Datastore identifier"""
         return "%s:%s" % (self.project_id, self.vlan_id)
 
     @classmethod
     def create(cls, project, vlan):
+        """Create a Vlan object"""
         instance = cls(project, vlan)
         instance.save()
         return instance
@@ -78,6 +81,7 @@
     @classmethod
     @datastore.absorb_connection_error
     def lookup(cls, project):
+        """Returns object by project if it exists in datastore or None"""
         set_name = cls._redis_set_name(cls.__name__)
         vlan = datastore.Redis.instance().hget(set_name, project)
         if vlan:
@@ -88,19 +92,19 @@
     @classmethod
     @datastore.absorb_connection_error
     def dict_by_project(cls):
-        """a hash of project:vlan"""
+        """A hash of project:vlan"""
         set_name = cls._redis_set_name(cls.__name__)
-        return datastore.Redis.instance().hgetall(set_name)
+        return datastore.Redis.instance().hgetall(set_name) or {}
 
     @classmethod
     @datastore.absorb_connection_error
     def dict_by_vlan(cls):
-        """a hash of vlan:project"""
+        """A hash of vlan:project"""
         set_name = cls._redis_set_name(cls.__name__)
         retvals = {}
-        hashset = datastore.Redis.instance().hgetall(set_name)
-        for val in hashset.keys():
-            retvals[hashset[val]] = val
+        hashset = datastore.Redis.instance().hgetall(set_name) or {}
+        for (key, val) in hashset.iteritems():
+            retvals[val] = key
         return retvals
 
     @classmethod
@@ -119,40 +123,47 @@
         default way of saving into "vlan:ID" and adding to a set of "vlans".
         """
         set_name = self._redis_set_name(self.__class__.__name__)
-        datastore.Redis.instance().hset(set_name, self.project_id, self.vlan_id)
+        datastore.Redis.instance().hset(set_name,
+                                        self.project_id,
+                                        self.vlan_id)
 
     @datastore.absorb_connection_error
     def destroy(self):
+        """Removes the object from the datastore"""
         set_name = self._redis_set_name(self.__class__.__name__)
         datastore.Redis.instance().hdel(set_name, self.project_id)
 
     def subnet(self):
+        """Returns a string containing the subnet"""
         vlan = int(self.vlan_id)
         network = IPy.IP(FLAGS.private_range)
-        start = (vlan-FLAGS.vlan_start) * FLAGS.network_size
+        start = (vlan - FLAGS.vlan_start) * FLAGS.network_size
         # minus one for the gateway.
         return "%s-%s" % (network[start],
                           network[start + FLAGS.network_size - 1])
 
+
 # CLEANUP:
 # TODO(ja): Save the IPs at the top of each subnet for cloudpipe vpn clients
-# TODO(ja): does vlanpool "keeper" need to know the min/max - 
+# TODO(ja): does vlanpool "keeper" need to know the min/max -
 #           shouldn't FLAGS always win?
-# TODO(joshua): Save the IPs at the top of each subnet for cloudpipe vpn clients
-
 class BaseNetwork(datastore.BasicModel):
+    """Implements basic logic for allocating ips in a network"""
     override_type = 'network'
-    NUM_STATIC_IPS = 3 # Network, Gateway, and CloudPipe
 
     @property
     def identifier(self):
+        """Datastore identifier"""
         return self.network_id
 
     def default_state(self):
+        """Default values for new objects"""
         return {'network_id': self.network_id, 'network_str': self.network_str}
 
     @classmethod
+    # pylint: disable=R0913
     def create(cls, user_id, project_id, security_group, vlan, network_str):
+        """Create a BaseNetwork object"""
         network_id = "%s:%s" % (project_id, security_group)
         net = cls(network_id, network_str)
         net['user_id'] = user_id
@@ -170,93 +181,124 @@
 
     @property
     def network(self):
+        """Returns a string representing the network"""
         return IPy.IP(self['network_str'])
 
     @property
     def netmask(self):
+        """Returns the netmask of this network"""
         return self.network.netmask()
 
     @property
     def gateway(self):
+        """Returns the network gateway address"""
         return self.network[1]
 
     @property
     def broadcast(self):
+        """Returns the network broadcast address"""
         return self.network.broadcast()
 
     @property
     def bridge_name(self):
+        """Returns the bridge associated with this network"""
         return "br%s" % (self["vlan"])
 
     @property
     def user(self):
+        """Returns the user associated with this network"""
         return manager.AuthManager().get_user(self['user_id'])
 
     @property
     def project(self):
+        """Returns the project associated with this network"""
         return manager.AuthManager().get_project(self['project_id'])
 
     @property
     def _hosts_key(self):
+        """Datastore key where hosts are stored"""
         return "network:%s:hosts" % (self['network_str'])
 
     @property
     def hosts(self):
+        """Returns a hash of all hosts allocated in this network"""
         return datastore.Redis.instance().hgetall(self._hosts_key) or {}
 
     def _add_host(self, _user_id, _project_id, host, target):
+        """Add a host to the datastore"""
         datastore.Redis.instance().hset(self._hosts_key, host, target)
 
     def _rem_host(self, host):
+        """Remove a host from the datastore"""
         datastore.Redis.instance().hdel(self._hosts_key, host)
 
     @property
     def assigned(self):
+        """Returns a list of all assigned keys"""
         return datastore.Redis.instance().hkeys(self._hosts_key)
 
     @property
     def available(self):
-        # the .2 address is always CloudPipe
-        # and the top <n> are for vpn clients
-        num_ips = self.num_static_ips
-        num_clients = FLAGS.cnt_vpn_clients
-        for idx in range(num_ips, len(self.network)-(1 + num_clients)):
+        """Returns a list of all available addresses in the network"""
+        for idx in range(self.num_bottom_reserved_ips,
+                         len(self.network) - self.num_top_reserved_ips):
             address = str(self.network[idx])
             if not address in self.hosts.keys():
                 yield address
 
     @property
-    def num_static_ips(self):
-        return BaseNetwork.NUM_STATIC_IPS
+    def num_bottom_reserved_ips(self):
+        """Returns number of ips reserved at the bottom of the range"""
+        return 2  # Network, Gateway
+
+    @property
+    def num_top_reserved_ips(self):
+        """Returns number of ips reserved at the top of the range"""
+        return 1  # Broadcast
 
     def allocate_ip(self, user_id, project_id, mac):
+        """Allocates an ip to a mac address"""
         for address in self.available:
-            logging.debug("Allocating IP %s to %s" % (address, project_id))
+            logging.debug("Allocating IP %s to %s", address, project_id)
             self._add_host(user_id, project_id, address, mac)
             self.express(address=address)
             return address
         raise exception.NoMoreAddresses("Project %s with network %s" %
-                                                (project_id, str(self.network)))
+                                        (project_id, str(self.network)))
 
     def lease_ip(self, ip_str):
-        logging.debug("Leasing allocated IP %s" % (ip_str))
+        """Called when DHCP lease is activated"""
+        logging.debug("Leasing allocated IP %s", ip_str)
 
     def release_ip(self, ip_str):
+        """Called when DHCP lease expires
+
+        Removes the ip from the assigned list"""
         if not ip_str in self.assigned:
             raise exception.AddressNotAllocated()
+        self._rem_host(ip_str)
         self.deexpress(address=ip_str)
-        self._rem_host(ip_str)
+        logging.debug("Releasing IP %s", ip_str)
 
     def deallocate_ip(self, ip_str):
-        # Do nothing for now, cleanup on ip release
-        pass
+        """Deallocates an allocated ip"""
+        # NOTE(vish): Perhaps we should put the ip into an intermediate
+        #             state, so we know that we are pending waiting for
+        #             dnsmasq to confirm that it has been released.
+        logging.debug("Deallocating allocated IP %s", ip_str)
 
     def list_addresses(self):
+        """List all allocated addresses"""
         for address in self.hosts:
             yield address
 
-    def express(self, address=None): pass
-    def deexpress(self, address=None): pass
+    def express(self, address=None):
+        """Set up network.  Implemented in subclasses"""
+        pass
+
+    def deexpress(self, address=None):
+        """Tear down network.  Implemented in subclasses"""
+        pass
 
 
 class BridgedNetwork(BaseNetwork):
@@ -280,7 +322,11 @@
     override_type = 'network'
 
     @classmethod
-    def get_network_for_project(cls, user_id, project_id, security_group):
+    def get_network_for_project(cls,
+                                user_id,
+                                project_id,
+                                security_group='default'):
+        """Returns network for a given project"""
         vlan = get_vlan_for_project(project_id)
         network_str = vlan.subnet()
         return cls.create(user_id, project_id, security_group, vlan.vlan_id,
@@ -296,30 +342,36 @@
         linux_net.vlan_create(self)
         linux_net.bridge_create(self)
 
+
 class DHCPNetwork(BridgedNetwork):
-    """
-    properties:
-        dhcp_listen_address: the ip of the gateway / dhcp host
-        dhcp_range_start: the first ip to give out
-        dhcp_range_end: the last ip to give out
-    """
+    """Network supporting DHCP"""
     bridge_gets_ip = True
     override_type = 'network'
 
     def __init__(self, *args, **kwargs):
         super(DHCPNetwork, self).__init__(*args, **kwargs)
-        # logging.debug("Initing DHCPNetwork object...")
-        self.dhcp_listen_address = self.network[1]
-        self.dhcp_range_start = self.network[3]
-        self.dhcp_range_end = self.network[-(1 + FLAGS.cnt_vpn_clients)]
-        try:
+        if not(os.path.exists(FLAGS.networks_path)):
             os.makedirs(FLAGS.networks_path)
-        # NOTE(todd): I guess this is a lazy way to not have to check if the
-        #             directory exists, but shouldn't we be smarter about
-        #             telling the difference between existing directory and
-        #             permission denied? (Errno 17 vs 13, OSError)
-        except Exception, err:
-            pass
+
+    @property
+    def num_bottom_reserved_ips(self):
+        # For cloudpipe
+        return super(DHCPNetwork, self).num_bottom_reserved_ips + 1
+
+    @property
+    def num_top_reserved_ips(self):
+        return super(DHCPNetwork, self).num_top_reserved_ips + \
+                FLAGS.cnt_vpn_clients
+
+    @property
+    def dhcp_listen_address(self):
+        """Address where dhcp server should listen"""
+        return self.gateway
+
+    @property
+    def dhcp_range_start(self):
+        """Starting address dhcp server should use"""
+        return self.network[self.num_bottom_reserved_ips]
 
     def express(self, address=None):
         super(DHCPNetwork, self).express(address=address)
@@ -329,15 +381,17 @@
             linux_net.start_dnsmasq(self)
         else:
             logging.debug("Not launching dnsmasq: no hosts.")
-        self.express_cloudpipe()
+        self.express_vpn()
 
     def allocate_vpn_ip(self, user_id, project_id, mac):
+        """Allocates the reserved ip to a vpn instance"""
         address = str(self.network[2])
         self._add_host(user_id, project_id, address, mac)
         self.express(address=address)
         return address
 
-    def express_cloudpipe(self):
+    def express_vpn(self):
+        """Sets up routing rules for vpn"""
         private_ip = str(self.network[2])
         linux_net.confirm_rule("FORWARD -d %s -p udp --dport 1194 -j ACCEPT"
                                % (private_ip, ))
@@ -353,7 +407,9 @@
         else:
             linux_net.start_dnsmasq(self)
 
+
 class PublicAddress(datastore.BasicModel):
+    """Represents an elastic ip in the datastore"""
     override_type = "address"
 
     def __init__(self, address):
@@ -369,6 +425,7 @@
 
     @classmethod
     def create(cls, user_id, project_id, address):
+        """Creates a PublicAddress object"""
         addr = cls(address)
         addr['user_id'] = user_id
         addr['project_id'] = project_id
@@ -379,35 +436,34 @@
 
 
 DEFAULT_PORTS = [("tcp", 80), ("tcp", 22), ("udp", 1194), ("tcp", 443)]
+
+
 class PublicNetworkController(BaseNetwork):
+    """Handles elastic ips"""
     override_type = 'network'
 
     def __init__(self, *args, **kwargs):
         network_id = "public:default"
-        super(PublicNetworkController, self).__init__(network_id, 
-            FLAGS.public_range)
+        super(PublicNetworkController, self).__init__(network_id,
+            FLAGS.public_range, *args, **kwargs)
         self['user_id'] = "public"
         self['project_id'] = "public"
-        self["create_time"] = time.strftime('%Y-%m-%dT%H:%M:%SZ', time.gmtime())
+        self["create_time"] = time.strftime('%Y-%m-%dT%H:%M:%SZ',
+                                            time.gmtime())
         self["vlan"] = FLAGS.public_vlan
         self.save()
         self.express()
 
     @property
-    def available(self):
-        for idx in range(2, len(self.network)-1):
-            address = str(self.network[idx])
-            if not address in self.hosts.keys():
-                yield address
-
-    @property
     def host_objs(self):
+        """Returns assigned addresses as PublicAddress objects"""
         for address in self.assigned:
             yield PublicAddress(address)
 
-    def get_host(self, host):
-        if host in self.assigned:
-            return PublicAddress(host)
+    def get_host(self, public_ip):
+        """Returns a specific public ip as PublicAddress object"""
+        if public_ip in self.assigned:
+            return PublicAddress(public_ip)
         return None
 
     def _add_host(self, user_id, project_id, host, _target):
@@ -423,9 +479,10 @@
         self.release_ip(ip_str)
 
     def associate_address(self, public_ip, private_ip, instance_id):
+        """Associates a public ip to a private ip and instance id"""
         if not public_ip in self.assigned:
             raise exception.AddressNotAllocated()
-        # TODO(joshua): Keep an index going both ways
+        # TODO(josh): Keep an index going both ways
         for addr in self.host_objs:
             if addr.get('private_ip', None) == private_ip:
                 raise exception.AddressAlreadyAssociated()
@@ -438,6 +495,7 @@
         self.express(address=public_ip)
 
     def disassociate_address(self, public_ip):
+        """Disassociates a public ip with its private ip"""
         if not public_ip in self.assigned:
             raise exception.AddressNotAllocated()
         addr = self.get_host(public_ip)
@@ -453,7 +511,7 @@
         if address:
             addresses = [self.get_host(address)]
         for addr in addresses:
-            if addr.get('private_ip','available') == 'available':
+            if addr.get('private_ip', 'available') == 'available':
                 continue
             public_ip = addr['address']
             private_ip = addr['private_ip']
@@ -462,7 +520,7 @@
                                    % (public_ip, private_ip))
             linux_net.confirm_rule("POSTROUTING -t nat -s %s -j SNAT --to %s"
                                    % (private_ip, public_ip))
-            # TODO: Get these from the secgroup datastore entries
+            # TODO(joshua): Get these from the secgroup datastore entries
             linux_net.confirm_rule("FORWARD -d %s -p icmp -j ACCEPT"
                                    % (private_ip))
             for (protocol, port) in DEFAULT_PORTS:
@@ -485,19 +543,18 @@
                                   % (private_ip, protocol, port))
 
 
-# FIXME(todd): does this present a race condition, or is there some piece of
-#              architecture that mitigates it (only one queue listener per net)?
+# FIXME(todd): does this present a race condition, or is there some
+#              piece of architecture that mitigates it (only one queue
+#              listener per net)?
 def get_vlan_for_project(project_id):
-    """
-    Allocate vlan IDs to individual users.
-    """
+    """Allocate vlan IDs to individual users"""
     vlan = Vlan.lookup(project_id)
     if vlan:
         return vlan
     known_vlans = Vlan.dict_by_vlan()
     for vnum in range(FLAGS.vlan_start, FLAGS.vlan_end):
         vstr = str(vnum)
-        if not known_vlans.has_key(vstr):
+        if not vstr in known_vlans:
             return Vlan.create(project_id, vnum)
         old_project_id = known_vlans[vstr]
         if not manager.AuthManager().get_project(old_project_id):
@@ -521,8 +578,9 @@
                 return Vlan.create(project_id, vnum)
     raise exception.AddressNotAllocated("Out of VLANs")
 
+
 def get_project_network(project_id, security_group='default'):
-    """ get a project's private network, allocating one if needed """
+    """Gets a project's private network, allocating one if needed"""
     project = manager.AuthManager().get_project(project_id)
     if not project:
         raise nova_exception.NotFound("Project %s doesn't exist." % project_id)
@@ -533,28 +591,29 @@
 
 
 def get_network_by_address(address):
+    """Gets the network for a given private ip"""
     # TODO(vish): This is completely the wrong way to do this, but
     #             I'm getting the network binary working before I
     #             tackle doing this the right way.
-    logging.debug("Get Network By Address: %s" % address)
+    logging.debug("Get Network By Address: %s", address)
     for project in manager.AuthManager().get_projects():
         net = get_project_network(project.id)
         if address in net.assigned:
-            logging.debug("Found %s in %s" % (address, project.id))
+            logging.debug("Found %s in %s", address, project.id)
             return net
     raise exception.AddressNotAllocated()
 
 
 def get_network_by_interface(iface, security_group='default'):
+    """Gets the network for a given interface"""
     vlan = iface.rpartition("br")[2]
     project_id = Vlan.dict_by_vlan().get(vlan)
     return get_project_network(project_id, security_group)
 
 
-
 def get_public_ip_for_instance(instance_id):
-    # FIXME: this should be a lookup - iteration won't scale
+    """Gets the public ip for a given instance"""
+    # FIXME(josh): this should be a lookup - iteration won't scale
     for address_record in PublicAddress.all():
         if address_record.get('instance_id', 'available') == instance_id:
             return address_record['address']
-

=== modified file 'nova/network/service.py'
--- nova/network/service.py	2010-08-05 19:29:50 +0000
+++ nova/network/service.py	2010-08-10 22:55:54 +0000
@@ -17,7 +17,7 @@
 #    under the License.
 
 """
-Network Nodes are responsible for allocating ips and setting up network
+Network Hosts are responsible for allocating ips and setting up network
 """
 
 from nova import datastore
@@ -38,7 +38,7 @@
 flags.DEFINE_string('flat_network_bridge', 'br100',
                     'Bridge for simple network instances')
 flags.DEFINE_list('flat_network_ips',
-                  ['192.168.0.2','192.168.0.3','192.168.0.4'],
+                  ['192.168.0.2', '192.168.0.3', '192.168.0.4'],
                   'Available ips for simple network')
 flags.DEFINE_string('flat_network_network', '192.168.0.0',
                        'Network for simple network')
@@ -51,26 +51,34 @@
 flags.DEFINE_string('flat_network_dns', '8.8.4.4',
                        'Dns for simple network')
 
+
 def type_to_class(network_type):
+    """Convert a network_type string into an actual Python class"""
     if network_type == 'flat':
         return FlatNetworkService
-    elif network_type  == 'vlan':
+    elif network_type == 'vlan':
         return VlanNetworkService
     raise NotFound("Couldn't find %s network type" % network_type)
 
 
 def setup_compute_network(network_type, user_id, project_id, security_group):
+    """Sets up the network on a compute host"""
     srv = type_to_class(network_type)
-    srv.setup_compute_network(network_type, user_id, project_id, security_group)
+    srv.setup_compute_network(network_type,
+                              user_id,
+                              project_id,
+                              security_group)
 
 
 def get_host_for_project(project_id):
+    """Get host allocated to project from datastore"""
     redis = datastore.Redis.instance()
     return redis.get(_host_key(project_id))
 
 
 def _host_key(project_id):
-    return "network_host:%s" % project_id
+    """Returns redis host key for network"""
+    return "networkhost:%s" % project_id
 
 
 class BaseNetworkService(service.Service):
@@ -80,6 +88,7 @@
     """
     def __init__(self, *args, **kwargs):
         self.network = model.PublicNetworkController()
+        super(BaseNetworkService, self).__init__(*args, **kwargs)
 
     def set_network_host(self, user_id, project_id, *args, **kwargs):
         """Safely sets the host of the projects network"""
@@ -109,7 +118,7 @@
         pass
 
     @classmethod
-    def setup_compute_network(self, user_id, project_id, security_group,
+    def setup_compute_network(cls, user_id, project_id, security_group,
                               *args, **kwargs):
         """Sets up matching network for compute hosts"""
         raise NotImplementedError()
@@ -138,7 +147,7 @@
     """Basic network where no vlans are used"""
 
     @classmethod
-    def setup_compute_network(self, user_id, project_id, security_group,
+    def setup_compute_network(cls, user_id, project_id, security_group,
                               *args, **kwargs):
         """Network is created manually"""
         pass
@@ -175,26 +184,28 @@
         """Returns an ip to the pool"""
         datastore.Redis.instance().sadd('ips', fixed_ip)
 
+
 class VlanNetworkService(BaseNetworkService):
     """Vlan network with dhcp"""
     # NOTE(vish): A lot of the interactions with network/model.py can be
     #             simplified and improved.  Also there it may be useful
     #             to support vlans separately from dhcp, instead of having
     #             both of them together in this class.
+    # pylint: disable=W0221
     def allocate_fixed_ip(self, user_id, project_id,
                           security_group='default',
-                          vpn=False, *args, **kwargs):
-        """Gets a fixed ip from the pool """
+                          is_vpn=False, *args, **kwargs):
+        """Gets a fixed ip from the pool"""
         mac = utils.generate_mac()
         net = model.get_project_network(project_id)
-        if vpn:
+        if is_vpn:
             fixed_ip = net.allocate_vpn_ip(user_id, project_id, mac)
         else:
             fixed_ip = net.allocate_ip(user_id, project_id, mac)
         return {'network_type': FLAGS.network_type,
                 'bridge_name': net['bridge_name'],
                 'mac_address': mac,
-                'private_dns_name' : fixed_ip}
+                'private_dns_name': fixed_ip}
 
     def deallocate_fixed_ip(self, fixed_ip,
                             *args, **kwargs):
@@ -202,9 +213,11 @@
         return model.get_network_by_address(fixed_ip).deallocate_ip(fixed_ip)
 
     def lease_ip(self, address):
+        """Called by bridge when ip is leased"""
         return model.get_network_by_address(address).lease_ip(address)
 
     def release_ip(self, address):
+        """Called by bridge when ip is released"""
         return model.get_network_by_address(address).release_ip(address)
 
     def restart_nets(self):
@@ -218,7 +231,7 @@
         vpn.NetworkData.create(project_id)
 
     @classmethod
-    def setup_compute_network(self, user_id, project_id, security_group,
+    def setup_compute_network(cls, user_id, project_id, security_group,
                               *args, **kwargs):
         """Sets up matching network for compute hosts"""
         # NOTE(vish): Use BridgedNetwork instead of DHCPNetwork because

=== modified file 'nova/network/vpn.py'
--- nova/network/vpn.py	2010-08-05 19:29:50 +0000
+++ nova/network/vpn.py	2010-08-10 22:55:54 +0000
@@ -33,7 +33,9 @@
 flags.DEFINE_integer('vpn_end_port', 2000,
                     'End port for the cloudpipe VPN servers')
 
+
 class NoMorePorts(exception.Error):
+    """No ports available to allocate for the given ip"""
     pass
 
 
@@ -67,34 +69,44 @@
         return network_data
 
     @classmethod
-    def find_free_port_for_ip(cls, ip):
+    def find_free_port_for_ip(cls, vpn_ip):
         """Finds a free port for a given ip from the redis set"""
         # TODO(vish): these redis commands should be generalized and
         #             placed into a base class. Conceptually, it is
         #             similar to an association, but we are just
         #             storing a set of values instead of keys that
         #             should be turned into objects.
-        redis = datastore.Redis.instance()
-        key = 'ip:%s:ports' % ip
+        cls._ensure_set_exists(vpn_ip)
+
+        port = datastore.Redis.instance().spop(cls._redis_ports_key(vpn_ip))
+        if not port:
+            raise NoMorePorts()
+        return port
+
+    @classmethod
+    def _redis_ports_key(cls, vpn_ip):
+        """Key that ports are stored under in redis"""
+        return 'ip:%s:ports' % vpn_ip
+
+    @classmethod
+    def _ensure_set_exists(cls, vpn_ip):
+        """Creates the set of ports for the ip if it doesn't already exist"""
         # TODO(vish): these ports should be allocated through an admin
         #             command instead of a flag
-        if (not redis.exists(key) and
-            not redis.exists(cls._redis_association_name('ip', ip))):
+        redis = datastore.Redis.instance()
+        if (not redis.exists(cls._redis_ports_key(vpn_ip)) and
+            not redis.exists(cls._redis_association_name('ip', vpn_ip))):
             for i in range(FLAGS.vpn_start_port, FLAGS.vpn_end_port + 1):
-                redis.sadd(key, i)
-
-        port = redis.spop(key)
-        if not port:
-            raise NoMorePorts()
-        return port
+                redis.sadd(cls._redis_ports_key(vpn_ip), i)
 
     @classmethod
-    def num_ports_for_ip(cls, ip):
+    def num_ports_for_ip(cls, vpn_ip):
         """Calculates the number of free ports for a given ip"""
-        return datastore.Redis.instance().scard('ip:%s:ports' % ip)
+        cls._ensure_set_exists(vpn_ip)
+        return datastore.Redis.instance().scard('ip:%s:ports' % vpn_ip)
 
     @property
-    def ip(self):
+    def ip(self):  # pylint: disable=C0103
         """The ip assigned to the project"""
         return self['ip']
 
@@ -113,4 +125,3 @@
         self.unassociate_with('ip', self.ip)
         datastore.Redis.instance().sadd('ip:%s:ports' % self.ip, self.port)
         super(NetworkData, self).destroy()
-

=== modified file 'nova/tests/network_unittest.py'
--- nova/tests/network_unittest.py	2010-08-06 21:31:03 +0000
+++ nova/tests/network_unittest.py	2010-08-10 22:55:54 +0000
@@ -15,7 +15,9 @@
 #    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
 #    License for the specific language governing permissions and limitations
 #    under the License.
-
+"""
+Unit Tests for network code
+"""
 import IPy
 import os
 import logging
@@ -31,8 +33,10 @@
 
 FLAGS = flags.FLAGS
 
+
 class NetworkTestCase(test.TrialTestCase):
-    def setUp(self):
+    """Test cases for network code"""
+    def setUp(self):  # pylint: disable=C0103
         super(NetworkTestCase, self).setUp()
         # NOTE(vish): if you change these flags, make sure to change the
         #             flags in the corresponding section in nova-dhcpbridge
@@ -43,7 +47,6 @@
                    network_size=32)
         logging.getLogger().setLevel(logging.DEBUG)
         self.manager = manager.AuthManager()
-        self.dnsmasq = FakeDNSMasq()
         self.user = self.manager.create_user('netuser', 'netuser', 'netuser')
         self.projects = []
         self.projects.append(self.manager.create_project('netuser',
@@ -54,47 +57,49 @@
             self.projects.append(self.manager.create_project(name,
                                                              'netuser',
                                                              name))
-        self.network = model.PublicNetworkController()
+            vpn.NetworkData.create(self.projects[i].id)
         self.service = service.VlanNetworkService()
 
-    def tearDown(self):
+    def tearDown(self):  # pylint: disable=C0103
         super(NetworkTestCase, self).tearDown()
         for project in self.projects:
             self.manager.delete_project(project)
         self.manager.delete_user(self.user)
 
     def test_public_network_allocation(self):
+        """Makes sure that we can allocaate a public ip"""
         pubnet = IPy.IP(flags.FLAGS.public_range)
-        address = self.network.allocate_ip(self.user.id, self.projects[0].id, "public")
+        address = self.service.allocate_elastic_ip(self.user.id,
+                                           self.projects[0].id)
         self.assertTrue(IPy.IP(address) in pubnet)
-        self.assertTrue(IPy.IP(address) in self.network.network)
 
     def test_allocate_deallocate_fixed_ip(self):
-        result  = yield self.service.allocate_fixed_ip(
+        """Makes sure that we can allocate and deallocate a fixed ip"""
+        result = self.service.allocate_fixed_ip(
                 self.user.id, self.projects[0].id)
         address = result['private_dns_name']
         mac = result['mac_address']
-        logging.debug("Was allocated %s" % (address))
         net = model.get_project_network(self.projects[0].id, "default")
         self.assertEqual(True, is_in_project(address, self.projects[0].id))
         hostname = "test-host"
-        self.dnsmasq.issue_ip(mac, address, hostname, net.bridge_name)
-        rv = self.service.deallocate_fixed_ip(address)
+        issue_ip(mac, address, hostname, net.bridge_name)
+        self.service.deallocate_fixed_ip(address)
 
         # Doesn't go away until it's dhcp released
         self.assertEqual(True, is_in_project(address, self.projects[0].id))
 
-        self.dnsmasq.release_ip(mac, address, hostname, net.bridge_name)
+        release_ip(mac, address, hostname, net.bridge_name)
         self.assertEqual(False, is_in_project(address, self.projects[0].id))
 
-    def test_range_allocation(self):
-        hostname = "test-host"
-        result = yield self.service.allocate_fixed_ip(
-                    self.user.id, self.projects[0].id)
+    def test_side_effects(self):
+        """Ensures allocating and releasing has no side effects"""
+        hostname = "side-effect-host"
+        result = self.service.allocate_fixed_ip(self.user.id,
+                                                self.projects[0].id)
         mac = result['mac_address']
         address = result['private_dns_name']
-        result = yield self.service.allocate_fixed_ip(
-                self.user, self.projects[1].id)
+        result = self.service.allocate_fixed_ip(self.user,
+                                                self.projects[1].id)
         secondmac = result['mac_address']
         secondaddress = result['private_dns_name']
 
@@ -102,66 +107,75 @@
         secondnet = model.get_project_network(self.projects[1].id, "default")
 
         self.assertEqual(True, is_in_project(address, self.projects[0].id))
-        self.assertEqual(True, is_in_project(secondaddress, self.projects[1].id))
+        self.assertEqual(True, is_in_project(secondaddress,
+                                             self.projects[1].id))
         self.assertEqual(False, is_in_project(address, self.projects[1].id))
 
         # Addresses are allocated before they're issued
-        self.dnsmasq.issue_ip(mac, address, hostname, net.bridge_name)
-        self.dnsmasq.issue_ip(secondmac, secondaddress,
-                                hostname, secondnet.bridge_name)
+        issue_ip(mac, address, hostname, net.bridge_name)
+        issue_ip(secondmac, secondaddress, hostname, secondnet.bridge_name)
 
-        rv = self.service.deallocate_fixed_ip(address)
-        self.dnsmasq.release_ip(mac, address, hostname, net.bridge_name)
+        self.service.deallocate_fixed_ip(address)
+        release_ip(mac, address, hostname, net.bridge_name)
         self.assertEqual(False, is_in_project(address, self.projects[0].id))
 
         # First address release shouldn't affect the second
-        self.assertEqual(True, is_in_project(secondaddress, self.projects[1].id))
+        self.assertEqual(True, is_in_project(secondaddress,
+                                             self.projects[1].id))
 
-        rv = self.service.deallocate_fixed_ip(secondaddress)
-        self.dnsmasq.release_ip(secondmac, secondaddress,
-                                hostname, secondnet.bridge_name)
-        self.assertEqual(False, is_in_project(secondaddress, self.projects[1].id))
+        self.service.deallocate_fixed_ip(secondaddress)
+        release_ip(secondmac, secondaddress, hostname, secondnet.bridge_name)
+        self.assertEqual(False, is_in_project(secondaddress,
+                                              self.projects[1].id))
 
     def test_subnet_edge(self):
-        result = yield self.service.allocate_fixed_ip(self.user.id,
+        """Makes sure that private ips don't overlap"""
+        result = self.service.allocate_fixed_ip(self.user.id,
                                                        self.projects[0].id)
         firstaddress = result['private_dns_name']
         hostname = "toomany-hosts"
-        for i in range(1,5):
+        for i in range(1, 5):
             project_id = self.projects[i].id
-            result = yield self.service.allocate_fixed_ip(
+            result = self.service.allocate_fixed_ip(
                     self.user, project_id)
             mac = result['mac_address']
             address = result['private_dns_name']
-            result = yield self.service.allocate_fixed_ip(
+            result = self.service.allocate_fixed_ip(
                     self.user, project_id)
             mac2 = result['mac_address']
             address2 = result['private_dns_name']
-            result = yield self.service.allocate_fixed_ip(
+            result = self.service.allocate_fixed_ip(
                    self.user, project_id)
             mac3 = result['mac_address']
             address3 = result['private_dns_name']
-            self.assertEqual(False, is_in_project(address, self.projects[0].id))
-            self.assertEqual(False, is_in_project(address2, self.projects[0].id))
-            self.assertEqual(False, is_in_project(address3, self.projects[0].id))
-            rv = self.service.deallocate_fixed_ip(address)
-            rv = self.service.deallocate_fixed_ip(address2)
-            rv = self.service.deallocate_fixed_ip(address3)
             net = model.get_project_network(project_id, "default")
-            self.dnsmasq.release_ip(mac, address, hostname, net.bridge_name)
-            self.dnsmasq.release_ip(mac2, address2, hostname, net.bridge_name)
-            self.dnsmasq.release_ip(mac3, address3, hostname, net.bridge_name)
+            issue_ip(mac, address, hostname, net.bridge_name)
+            issue_ip(mac2, address2, hostname, net.bridge_name)
+            issue_ip(mac3, address3, hostname, net.bridge_name)
+            self.assertEqual(False, is_in_project(address,
+                                                  self.projects[0].id))
+            self.assertEqual(False, is_in_project(address2,
+                                                  self.projects[0].id))
+            self.assertEqual(False, is_in_project(address3,
+                                                  self.projects[0].id))
+            self.service.deallocate_fixed_ip(address)
+            self.service.deallocate_fixed_ip(address2)
+            self.service.deallocate_fixed_ip(address3)
+            release_ip(mac, address, hostname, net.bridge_name)
+            release_ip(mac2, address2, hostname, net.bridge_name)
+            release_ip(mac3, address3, hostname, net.bridge_name)
         net = model.get_project_network(self.projects[0].id, "default")
-        rv = self.service.deallocate_fixed_ip(firstaddress)
-        self.dnsmasq.release_ip(mac, firstaddress, hostname, net.bridge_name)
+        self.service.deallocate_fixed_ip(firstaddress)
+        release_ip(mac, firstaddress, hostname, net.bridge_name)
 
-    def test_212_vpn_ip_and_port_looks_valid(self):
-        vpn.NetworkData.create(self.projects[0].id)
+    def test_vpn_ip_and_port_looks_valid(self):
+        """Ensure the vpn ip and port are reasonable"""
         self.assert_(self.projects[0].vpn_ip)
         self.assert_(self.projects[0].vpn_port >= FLAGS.vpn_start_port)
         self.assert_(self.projects[0].vpn_port <= FLAGS.vpn_end_port)
 
     def test_too_many_vpns(self):
+        """Ensure error is raised if we run out of vpn ports"""
         vpns = []
         for i in xrange(vpn.NetworkData.num_ports_for_ip(FLAGS.vpn_ip)):
             vpns.append(vpn.NetworkData.create("vpnuser%s" % i))
@@ -169,84 +183,101 @@
         for network_datum in vpns:
             network_datum.destroy()
 
-    def test_release_before_deallocate(self):
-        pass
-
-    def test_deallocate_before_issued(self):
-        pass
-
-    def test_too_many_addresses(self):
-        """
-        Here, we test that a proper NoMoreAddresses exception is raised.
-
-        However, the number of available IP addresses depends on the test
+    def test_ips_are_reused(self):
+        """Makes sure that ip addresses that are deallocated get reused"""
+        result = self.service.allocate_fixed_ip(
+                    self.user.id, self.projects[0].id)
+        mac = result['mac_address']
+        address = result['private_dns_name']
+
+        hostname = "reuse-host"
+        net = model.get_project_network(self.projects[0].id, "default")
+
+        issue_ip(mac, address, hostname, net.bridge_name)
+        self.service.deallocate_fixed_ip(address)
+        release_ip(mac, address, hostname, net.bridge_name)
+
+        result = self.service.allocate_fixed_ip(
+                self.user, self.projects[0].id)
+        secondmac = result['mac_address']
+        secondaddress = result['private_dns_name']
+        self.assertEqual(address, secondaddress)
+        self.service.deallocate_fixed_ip(secondaddress)
+        issue_ip(secondmac, secondaddress, hostname, net.bridge_name)
+        release_ip(secondmac, secondaddress, hostname, net.bridge_name)
+
+    def test_available_ips(self):
+        """Make sure the number of available ips for the network is correct
+
+        The number of available IP addresses depends on the test
         environment's setup.
 
         Network size is set in test fixture's setUp method.
 
-        There are FLAGS.cnt_vpn_clients addresses reserved for VPN (NUM_RESERVED_VPN_IPS)
-
-        And there are NUM_STATIC_IPS that are always reserved by Nova for the necessary
-        services (gateway, CloudPipe, etc)
-
-        So we should get flags.network_size - (NUM_STATIC_IPS +
-                                               NUM_PREALLOCATED_IPS +
-                                               NUM_RESERVED_VPN_IPS)
-        usable addresses
+        There are ips reserved at the bottom and top of the range.
+        services (network, gateway, CloudPipe, broadcast)
         """
         net = model.get_project_network(self.projects[0].id, "default")
-
-        # Determine expected number of available IP addresses
-        num_static_ips = net.num_static_ips
         num_preallocated_ips = len(net.hosts.keys())
-        num_reserved_vpn_ips = flags.FLAGS.cnt_vpn_clients
-        num_available_ips = flags.FLAGS.network_size - (num_static_ips +
-                                                        num_preallocated_ips +
-                                                        num_reserved_vpn_ips)
+        net_size = flags.FLAGS.network_size
+        num_available_ips = net_size - (net.num_bottom_reserved_ips +
+                                        num_preallocated_ips +
+                                        net.num_top_reserved_ips)
+        self.assertEqual(num_available_ips, len(list(net.available)))
+
+    def test_too_many_addresses(self):
+        """Test for a NoMoreAddresses exception when all fixed ips are used.
+        """
+        net = model.get_project_network(self.projects[0].id, "default")
 
         hostname = "toomany-hosts"
         macs = {}
         addresses = {}
-        for i in range(0, (num_available_ips - 1)):
-            result = yield self.service.allocate_fixed_ip(self.user.id, self.projects[0].id)
+        # Number of availaible ips is len of the available list
+        num_available_ips = len(list(net.available))
+        for i in range(num_available_ips):
+            result = self.service.allocate_fixed_ip(self.user.id,
+                                                    self.projects[0].id)
             macs[i] = result['mac_address']
             addresses[i] = result['private_dns_name']
-            self.dnsmasq.issue_ip(macs[i], addresses[i], hostname, net.bridge_name)
-
-        self.assertFailure(self.service.allocate_fixed_ip(self.user.id, self.projects[0].id), NoMoreAddresses)
-
-        for i in range(0, (num_available_ips - 1)):
-            rv = self.service.deallocate_fixed_ip(addresses[i])
-            self.dnsmasq.release_ip(macs[i], addresses[i], hostname, net.bridge_name)
+            issue_ip(macs[i], addresses[i], hostname, net.bridge_name)
+
+        self.assertEqual(len(list(net.available)), 0)
+        self.assertRaises(NoMoreAddresses, self.service.allocate_fixed_ip,
+                          self.user.id, self.projects[0].id)
+
+        for i in range(len(addresses)):
+            self.service.deallocate_fixed_ip(addresses[i])
+            release_ip(macs[i], addresses[i], hostname, net.bridge_name)
+        self.assertEqual(len(list(net.available)), num_available_ips)
+
 
 def is_in_project(address, project_id):
+    """Returns true if address is in specified project"""
     return address in model.get_project_network(project_id).list_addresses()
 
-def _get_project_addresses(project_id):
-    project_addresses = []
-    for addr in model.get_project_network(project_id).list_addresses():
-        project_addresses.append(addr)
-    return project_addresses
 
 def binpath(script):
+    """Returns the absolute path to a script in bin"""
     return os.path.abspath(os.path.join(__file__, "../../../bin", script))
 
-class FakeDNSMasq(object):
-    def issue_ip(self, mac, ip, hostname, interface):
-        cmd = "%s add %s %s %s" % (binpath('nova-dhcpbridge'),
-                                   mac, ip, hostname)
-        env = {'DNSMASQ_INTERFACE': interface,
-               'TESTING' : '1',
-               'FLAGFILE' : FLAGS.dhcpbridge_flagfile}
-        (out, err) = utils.execute(cmd, addl_env=env)
-        logging.debug("ISSUE_IP: %s, %s " % (out, err))
-
-    def release_ip(self, mac, ip, hostname, interface):
-        cmd = "%s del %s %s %s" % (binpath('nova-dhcpbridge'),
-                                   mac, ip, hostname)
-        env = {'DNSMASQ_INTERFACE': interface,
-               'TESTING' : '1',
-               'FLAGFILE' : FLAGS.dhcpbridge_flagfile}
-        (out, err) = utils.execute(cmd, addl_env=env)
-        logging.debug("RELEASE_IP: %s, %s " % (out, err))
-
+
+def issue_ip(mac, private_ip, hostname, interface):
+    """Run add command on dhcpbridge"""
+    cmd = "%s add %s %s %s" % (binpath('nova-dhcpbridge'),
+                               mac, private_ip, hostname)
+    env = {'DNSMASQ_INTERFACE': interface,
+           'TESTING': '1',
+           'FLAGFILE': FLAGS.dhcpbridge_flagfile}
+    (out, err) = utils.execute(cmd, addl_env=env)
+    logging.debug("ISSUE_IP: %s, %s ", out, err)
+
+def release_ip(mac, private_ip, hostname, interface):
+    """Run del command on dhcpbridge"""
+    cmd = "%s del %s %s %s" % (binpath('nova-dhcpbridge'),
+                               mac, private_ip, hostname)
+    env = {'DNSMASQ_INTERFACE': interface,
+           'TESTING': '1',
+           'FLAGFILE': FLAGS.dhcpbridge_flagfile}
+    (out, err) = utils.execute(cmd, addl_env=env)
+    logging.debug("RELEASE_IP: %s, %s ", out, err)