Merge lp:~tr3buchet/nova/xs_network_inject into lp:~hudson-openstack/nova/trunk

[Trey Morris]

added labels to networks for use in multi-nic
added writing network data to xenstore param-list
added call to agent to reset network
added reset_network call to openstack api

[Jay Pipes (jaypipes)]

Don't think I've ever seen a 5-space tabstop. Copy/paste error? :)

58 -# vim: tabstop=4 shiftwidth=4 softtabstop=4
59 +# vim: tabstop=5 shiftwidth=4 softtabstop=4

Please s/003_cactus/003_add_network_label_column/ for the migrate:

183 === added file 'nova/db/sqlalchemy/migrate_repo/versions/003_cactus.py'

We need >1 migration file for a release, and naming the migration file with something that describes the schema changes in it is a common practice.

258 def create_networks(self, context, cidr, num_networks, network_size,
259 - cidr_v6, *args, **kwargs):
260 + cidr_v6, label, *args, **kwargs):

Above, you have added label as a required argument to the network manager's create_networks() method, however:

1) I don't see where calls to create_networks() have been changed to pass in this newly-required label parameter
2) The label field of the Network model was added as an optional field, so it seems that perhaps the label param to create_networks() should have a default?

Other than those little things, the code looks good. Was there a plan for test code for this new functionality?

Cheers!
jay

review: Needs Fixing

[Trey Morris (tr3buchet)]

wow, for the life of me i can't figure out how that tabstop got set to 5.. must have been some rogue vim keystrokes or something

[Jay Pipes (jaypipes)]

Per our conversation on IRC, I now see you supplying the label='public' to the create_networks() function in nova-manage. You can disregard that comment from my review. Thanks!

[Trey Morris (tr3buchet)]

updates in place.

[Matt Dietz (cerberus)]

lgtm.

review: Approve

[Jay Pipes (jaypipes)]

nice work.

review: Approve

[Ed Leafe (ed-leafe)]

The new file "nova/db/sqlalchemy/migrate_repo/versions/003_add_label_to_networks.py" needs a correct copyright; NASA didn't create it in 2010. Change it to:
# Copyright 2011 OpenStack LLC

Other than that, lgtm.

[Sandy Walsh (sandy-walsh)]

Good job Trey!

Why does network_get_all_by_instance only check FixedIPs?

Not your fault, but too bad we can't get any unit tests at the Xen level ... where it's needed.

Trivial comment: doc strings should be proper sentences (caps and punctuation) and remove unnecessary whitespace. (compute.api)

I'll approve after I hear back from you.

[Trey Morris (tr3buchet)]

dabo: easy peasy
sandy: FixedIPs as opposed to both fixed and floating? caps and punctuation :(. englilish is getting into the code it seems

[Trey Morris (tr3buchet)]

still checking fixed IPs only as no one knows what floating IPs are. Also haven't heard back from anyone who should.

[Sandy Walsh (sandy-walsh)]

lgtm

review: Approve

[Ed Leafe (ed-leafe)]

kewl

review: Approve

=== modified file 'bin/nova-manage'
--- bin/nova-manage	2011-02-17 17:59:51 +0000
+++ bin/nova-manage	2011-02-17 19:12:16 +0000
@@ -472,8 +472,8 @@
     """Class for managing networks."""
 
     def create(self, fixed_range=None, num_networks=None,
-               network_size=None, vlan_start=None, vpn_start=None,
-               fixed_range_v6=None):
+               network_size=None, vlan_start=None,
+               vpn_start=None, fixed_range_v6=None, label='public'):
         """Creates fixed ips for host by range
         arguments: [fixed_range=FLAG], [num_networks=FLAG],
                    [network_size=FLAG], [vlan_start=FLAG],
@@ -495,9 +495,10 @@
                                     cidr=fixed_range,
                                     num_networks=int(num_networks),
                                     network_size=int(network_size),
+                                    vlan_start=int(vlan_start),
+                                    vpn_start=int(vpn_start),
                                     cidr_v6=fixed_range_v6,
-                                    vlan_start=int(vlan_start),
-                                    vpn_start=int(vpn_start))
+                                    label=label)
 
     def list(self):
         """List all created networks"""

=== modified file 'nova/api/openstack/__init__.py'
--- nova/api/openstack/__init__.py	2011-01-31 22:43:03 +0000
+++ nova/api/openstack/__init__.py	2011-02-17 19:12:16 +0000
@@ -79,6 +79,7 @@
             server_members["actions"] = "GET"
             server_members['suspend'] = 'POST'
             server_members['resume'] = 'POST'
+            server_members['reset_network'] = 'POST'
 
         mapper.resource("server", "servers", controller=servers.Controller(),
                         collection={'detail': 'GET'},

=== modified file 'nova/api/openstack/servers.py'
--- nova/api/openstack/servers.py	2011-01-26 16:16:16 +0000
+++ nova/api/openstack/servers.py	2011-02-17 19:12:16 +0000
@@ -249,6 +249,20 @@
             return faults.Fault(exc.HTTPUnprocessableEntity())
         return exc.HTTPAccepted()
 
+    def reset_network(self, req, id):
+        """
+        Reset networking on an instance (admin only).
+
+        """
+        context = req.environ['nova.context']
+        try:
+            self.compute_api.reset_network(context, id)
+        except:
+            readable = traceback.format_exc()
+            LOG.exception(_("Compute.api::reset_network %s"), readable)
+            return faults.Fault(exc.HTTPUnprocessableEntity())
+        return exc.HTTPAccepted()
+
     def pause(self, req, id):
         """ Permit Admins to Pause the server. """
         ctxt = req.environ['nova.context']

=== modified file 'nova/compute/api.py'
--- nova/compute/api.py	2011-02-15 18:19:52 +0000
+++ nova/compute/api.py	2011-02-17 19:12:16 +0000
@@ -466,6 +466,13 @@
         instance = self.get(context, instance_id)
         return instance['locked']
 
+    def reset_network(self, context, instance_id):
+        """
+        Reset networking on the instance.
+
+        """
+        self._cast_compute_message('reset_network', context, instance_id)
+
     def attach_volume(self, context, instance_id, volume_id, device):
         if not re.match("^/dev/[a-z]d[a-z]+$", device):
             raise exception.ApiError(_("Invalid device specified: %s. "

=== modified file 'nova/compute/manager.py'
--- nova/compute/manager.py	2011-02-09 10:08:15 +0000
+++ nova/compute/manager.py	2011-02-17 19:12:16 +0000
@@ -498,6 +498,18 @@
         instance_ref = self.db.instance_get(context, instance_id)
         return instance_ref['locked']
 
+    @checks_instance_lock
+    def reset_network(self, context, instance_id):
+        """
+        Reset networking on the instance.
+
+        """
+        context = context.elevated()
+        instance_ref = self.db.instance_get(context, instance_id)
+        LOG.debug(_('instance %s: reset network'), instance_id,
+                                                   context=context)
+        self.driver.reset_network(instance_ref)
+
     @exception.wrap_exception
     def get_console_output(self, context, instance_id):
         """Send the console output for an instance."""

=== modified file 'nova/db/api.py'
--- nova/db/api.py	2011-02-16 11:15:48 +0000
+++ nova/db/api.py	2011-02-17 19:12:16 +0000
@@ -293,6 +293,11 @@
     return IMPL.fixed_ip_get_by_address(context, address)
 
 
+def fixed_ip_get_all_by_instance(context, instance_id):
+    """Get fixed ips by instance or raise if none exist."""
+    return IMPL.fixed_ip_get_all_by_instance(context, instance_id)
+
+
 def fixed_ip_get_instance(context, address):
     """Get an instance for a fixed ip by address."""
     return IMPL.fixed_ip_get_instance(context, address)
@@ -521,6 +526,11 @@
     return IMPL.network_get_by_instance(context, instance_id)
 
 
+def network_get_all_by_instance(context, instance_id):
+    """Get all networks by instance id or raise if none exist."""
+    return IMPL.network_get_all_by_instance(context, instance_id)
+
+
 def network_get_index(context, network_id):
     """Get non-conflicting index for network."""
     return IMPL.network_get_index(context, network_id)

=== modified file 'nova/db/sqlalchemy/api.py'
--- nova/db/sqlalchemy/api.py	2011-02-17 14:14:45 +0000
+++ nova/db/sqlalchemy/api.py	2011-02-17 19:12:16 +0000
@@ -609,6 +609,17 @@
 
 
 @require_context
+def fixed_ip_get_all_by_instance(context, instance_id):
+    session = get_session()
+    rv = session.query(models.FixedIp).\
+                 filter_by(instance_id=instance_id).\
+                 filter_by(deleted=False)
+    if not rv:
+        raise exception.NotFound(_('No address for instance %s') % instance_id)
+    return rv
+
+
+@require_context
 def fixed_ip_get_instance_v6(context, address):
     session = get_session()
     mac = utils.to_mac(address)
@@ -1109,6 +1120,19 @@
 
 
 @require_admin_context
+def network_get_all_by_instance(_context, instance_id):
+    session = get_session()
+    rv = session.query(models.Network).\
+                 filter_by(deleted=False).\
+                 join(models.Network.fixed_ips).\
+                 filter_by(instance_id=instance_id).\
+                 filter_by(deleted=False)
+    if not rv:
+        raise exception.NotFound(_('No network for instance %s') % instance_id)
+    return rv
+
+
+@require_admin_context
 def network_set_host(context, network_id, host_id):
     session = get_session()
     with session.begin():

=== added file 'nova/db/sqlalchemy/migrate_repo/versions/003_add_label_to_networks.py'
--- nova/db/sqlalchemy/migrate_repo/versions/003_add_label_to_networks.py	1970-01-01 00:00:00 +0000
+++ nova/db/sqlalchemy/migrate_repo/versions/003_add_label_to_networks.py	2011-02-17 19:12:16 +0000
@@ -0,0 +1,51 @@
+# vim: tabstop=4 shiftwidth=4 softtabstop=4
+
+# Copyright 2011 OpenStack LLC
+# All Rights Reserved.
+#
+#    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.
+
+from sqlalchemy import *
+from migrate import *
+
+from nova import log as logging
+
+
+meta = MetaData()
+
+
+networks = Table('networks', meta,
+        Column('id', Integer(),  primary_key=True, nullable=False),
+        )
+
+
+#
+# New Tables
+#
+
+
+#
+# Tables to alter
+#
+
+networks_label = Column(
+        'label',
+        String(length=255, convert_unicode=False, assert_unicode=None,
+               unicode_error=None, _warn_on_bytestring=False))
+
+
+def upgrade(migrate_engine):
+    # Upgrade operations go here. Don't create your own engine;
+    # bind migrate_engine to your metadata
+    meta.bind = migrate_engine
+    networks.create_column(networks_label)

=== modified file 'nova/db/sqlalchemy/models.py'
--- nova/db/sqlalchemy/models.py	2011-02-01 16:09:34 +0000
+++ nova/db/sqlalchemy/models.py	2011-02-17 19:12:16 +0000
@@ -373,6 +373,7 @@
                                               "vpn_public_port"),
                       {'mysql_engine': 'InnoDB'})
     id = Column(Integer, primary_key=True)
+    label = Column(String(255))
 
     injected = Column(Boolean, default=False)
     cidr = Column(String(255), unique=True)

=== modified file 'nova/network/manager.py'
--- nova/network/manager.py	2011-02-09 20:49:25 +0000
+++ nova/network/manager.py	2011-02-17 19:12:16 +0000
@@ -331,11 +331,12 @@
         pass
 
     def create_networks(self, context, cidr, num_networks, network_size,
-                        cidr_v6, *args, **kwargs):
+                        cidr_v6, label, *args, **kwargs):
         """Create networks based on parameters."""
         fixed_net = IPy.IP(cidr)
         fixed_net_v6 = IPy.IP(cidr_v6)
         significant_bits_v6 = 64
+        count = 1
         for index in range(num_networks):
             start = index * network_size
             significant_bits = 32 - int(math.log(network_size, 2))
@@ -348,6 +349,11 @@
             net['gateway'] = str(project_net[1])
             net['broadcast'] = str(project_net.broadcast())
             net['dhcp_start'] = str(project_net[2])
+            if num_networks > 1:
+                net['label'] = "%s_%d" % (label, count)
+            else:
+                net['label'] = label
+            count += 1
 
             if(FLAGS.use_ipv6):
                 cidr_v6 = "%s/%s" % (fixed_net_v6[0], significant_bits_v6)

=== modified file 'nova/tests/api/openstack/test_servers.py'
--- nova/tests/api/openstack/test_servers.py	2011-01-27 18:48:00 +0000
+++ nova/tests/api/openstack/test_servers.py	2011-02-17 19:12:16 +0000
@@ -281,6 +281,18 @@
         res = req.get_response(fakes.wsgi_app())
         self.assertEqual(res.status_int, 202)
 
+    def test_server_reset_network(self):
+        FLAGS.allow_admin_api = True
+        body = dict(server=dict(
+            name='server_test', imageId=2, flavorId=2, metadata={},
+            personality={}))
+        req = webob.Request.blank('/v1.0/servers/1/reset_network')
+        req.method = 'POST'
+        req.content_type = 'application/json'
+        req.body = json.dumps(body)
+        res = req.get_response(fakes.wsgi_app())
+        self.assertEqual(res.status_int, 202)
+
     def test_server_diagnostics(self):
         req = webob.Request.blank("/v1.0/servers/1/diagnostics")
         req.method = "GET"

=== modified file 'nova/tests/test_xenapi.py'
--- nova/tests/test_xenapi.py	2011-02-09 10:08:15 +0000
+++ nova/tests/test_xenapi.py	2011-02-17 19:12:16 +0000
@@ -32,6 +32,7 @@
 from nova.virt.xenapi import fake as xenapi_fake
 from nova.virt.xenapi import volume_utils
 from nova.virt.xenapi.vmops import SimpleDH
+from nova.virt.xenapi.vmops import VMOps
 from nova.tests.db import fakes as db_fakes
 from nova.tests.xenapi import stubs
 from nova.tests.glance import stubs as glance_stubs
@@ -141,6 +142,10 @@
         self.stubs.UnsetAll()
 
 
+def reset_network(*args):
+    pass
+
+
 class XenAPIVMTestCase(test.TestCase):
     """
     Unit tests for VM operations
@@ -162,6 +167,7 @@
         stubs.stubout_session(self.stubs, stubs.FakeSessionForVMTests)
         stubs.stubout_get_this_vm_uuid(self.stubs)
         stubs.stubout_stream_disk(self.stubs)
+        self.stubs.Set(VMOps, 'reset_network', reset_network)
         glance_stubs.stubout_glance_client(self.stubs,
                                            glance_stubs.FakeGlance)
         self.conn = xenapi_conn.get_connection(False)

=== modified file 'nova/virt/xenapi/vmops.py'
--- nova/virt/xenapi/vmops.py	2011-02-15 17:30:19 +0000
+++ nova/virt/xenapi/vmops.py	2011-02-17 19:12:16 +0000
@@ -66,6 +66,7 @@
         if vm is not None:
             raise exception.Duplicate(_('Attempted to create'
             ' non-unique name %s') % instance.name)
+
         #ensure enough free memory is available
         if not VMHelper.ensure_free_mem(self._session, instance):
                 name = instance['name']
@@ -75,10 +76,6 @@
                                       instance['id'],
                                       power_state.SHUTDOWN)
                 return
-        bridge = db.network_get_by_instance(context.get_admin_context(),
-                                            instance['id'])['bridge']
-        network_ref = \
-            NetworkHelper.find_network_with_bridge(self._session, bridge)
 
         user = AuthManager().get_user(instance.user_id)
         project = AuthManager().get_project(instance.project_id)
@@ -107,9 +104,46 @@
                                           instance, kernel, ramdisk, pv_kernel)
         VMHelper.create_vbd(self._session, vm_ref, vdi_ref, 0, True)
 
-        if network_ref:
-            VMHelper.create_vif(self._session, vm_ref,
-                                network_ref, instance.mac_address)
+        # write network info
+        admin_context = context.get_admin_context()
+
+        # TODO(tr3buchet) - remove comment in multi-nic
+        # I've decided to go ahead and consider multiple IPs and networks
+        # at this stage even though they aren't implemented because these will
+        # be needed for multi-nic and there was no sense writing it for single
+        # network/single IP and then having to turn around and re-write it
+        IPs = db.fixed_ip_get_all_by_instance(admin_context, instance['id'])
+        for network in db.network_get_all_by_instance(admin_context,
+                                                      instance['id']):
+            network_IPs = [ip for ip in IPs if ip.network_id == network.id]
+
+            def ip_dict(ip):
+                return {'netmask': network['netmask'],
+                        'enabled': '1',
+                        'ip': ip.address}
+
+            mac_id = instance.mac_address.replace(':', '')
+            location = 'vm-data/networking/%s' % mac_id
+            mapping = {'label': network['label'],
+                       'gateway': network['gateway'],
+                       'mac': instance.mac_address,
+                       'dns': [network['dns']],
+                       'ips': [ip_dict(ip) for ip in network_IPs]}
+            self.write_to_param_xenstore(vm_ref, {location: mapping})
+
+            # TODO(tr3buchet) - remove comment in multi-nic
+            # this bit here about creating the vifs will be updated
+            # in multi-nic to handle multiple IPs on the same network
+            # and multiple networks
+            # for now it works as there is only one of each
+            bridge = network['bridge']
+            network_ref = \
+                NetworkHelper.find_network_with_bridge(self._session, bridge)
+
+            if network_ref:
+                VMHelper.create_vif(self._session, vm_ref,
+                                    network_ref, instance.mac_address)
+
         LOG.debug(_('Starting VM %s...'), vm_ref)
         self._session.call_xenapi('VM.start', vm_ref, False, False)
         instance_name = instance.name
@@ -117,6 +151,8 @@
                 % locals())
 
         # NOTE(armando): Do we really need to do this in virt?
+        # NOTE(tr3buchet): not sure but wherever we do it, we need to call
+        #                  reset_network afterwards
         timer = utils.LoopingCall(f=None)
 
         def _wait_for_boot():
@@ -137,6 +173,10 @@
                 timer.stop()
 
         timer.f = _wait_for_boot
+
+        # call reset networking
+        self.reset_network(instance)
+
         return timer.start(interval=0.5, now=True)
 
     def _get_vm_opaque_ref(self, instance_or_vm):
@@ -398,6 +438,14 @@
         # TODO: implement this!
         return 'http://fakeajaxconsole/fake_url'
 
+    def reset_network(self, instance):
+        """
+        Creates uuid arg to pass to make_agent_call and calls it.
+
+        """
+        args = {'id': str(uuid.uuid4())}
+        resp = self._make_agent_call('resetnetwork', instance, '', args)
+
     def list_from_xenstore(self, vm, path):
         """Runs the xenstore-ls command to get a listing of all records
         from 'path' downward. Returns a dict with the sub-paths as keys,

=== modified file 'nova/virt/xenapi_conn.py'
--- nova/virt/xenapi_conn.py	2011-01-25 23:20:55 +0000
+++ nova/virt/xenapi_conn.py	2011-02-17 19:12:16 +0000
@@ -188,6 +188,10 @@
         """resume the specified instance"""
         self._vmops.resume(instance, callback)
 
+    def reset_network(self, instance):
+        """reset networking for specified instance"""
+        self._vmops.reset_network(instance)
+
     def get_info(self, instance_id):
         """Return data about VM instance"""
         return self._vmops.get_info(instance_id)

=== modified file 'plugins/xenserver/xenapi/etc/xapi.d/plugins/agent'
--- plugins/xenserver/xenapi/etc/xapi.d/plugins/agent	2011-02-08 10:14:51 +0000
+++ plugins/xenserver/xenapi/etc/xapi.d/plugins/agent	2011-02-17 19:12:16 +0000
@@ -91,6 +91,18 @@
     return resp
 
 
+@jsonify
+def resetnetwork(self, arg_dict):
+    """
+    Writes a resquest to xenstore that tells the agent to reset networking.
+
+    """
+    arg_dict['value'] = json.dumps({'name': 'resetnetwork', 'value': ''})
+    request_id = arg_dict['id']
+    arg_dict['path'] = "data/host/%s" % request_id
+    xenstore.write_record(self, arg_dict)
+
+
 def _wait_for_agent(self, request_id, arg_dict):
     """Periodically checks xenstore for a response from the agent.
     The request is always written to 'data/host/{id}', and
@@ -124,4 +136,5 @@
 if __name__ == "__main__":
     XenAPIPlugin.dispatch(
         {"key_init": key_init,
-        "password": password})
+        "password": password,
+        "resetnetwork": resetnetwork})