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

[Trey Morris]

xenapi support for multi_nic. This is a phase of multi_nic which allows xenapi to work as is and with multi_nic. The other virt driver(s) need to be updated with the same support.

xenapi now supports multi_nic. This has to be merged before or at the same time as multi_nic. Driver receive network information from the compute worker for spawn, inject networking info, and create vifs. Will continue to work if network information is not passed in (for example if multi_nic is not yet implemented). After multi_nic a few of these shims can be deleted.

network info format:
list of (network model object, {network info dict}) pairs. The network info dict is going to be created identically to how mappings are currently created in nova/virt/xenapi/vmops.py in def inject_network_info

[Matt Dietz (cerberus)]

Looks ok to me. I also understand your use case from offline discussions.

review: Approve

[Rick Harris (rconradharris)]

Nice work, Trey. Here are my thoughts so far (mostly style stuff):

> 9 + def spawn(self, instance, network_info=None):

Not exposed from xenapi_conn.py?

Are all virt-drivers going to accept a network_info=None kwarg?

> 25 + if network_info is not None:
> 26 + self.inject_network_info(instance, network_info)
> 27 + self.create_vifs(instance, [nw for (nw, mapping) in network_info])
> 28 + else:
> 29 + # TODO(tr3buchet) - goes away with multi-nic
> 30 + networks = self.inject_network_info(instance)
> 31 + self.create_vifs(instance, networks)

It seems like we should be able to simplify all of that to:

    networks = self.inject_network_info(instance, network_info)
    self.create_vifs(instance, networks)

Assuming `inject_network_info` returns `networks` correctly for both the
network_info=None and network_info!=None cases.

> def inject_network_info(self, instance, network_info=None):

This is method is handling two separate cases 1) multi_nic and 2) no
multi_nice. Since it's pretty long, I think it's a good canidate to be broken
up into two smaller methods, something like:

    def inject_network_info(self, instance, network_info=None):
        if network_info:
            networks = self.inject_network_info_multi_nic(self, instance, network_info)
        else:
            networks = self.inject_network_info_default(self, instance)

        return networks

    def inject_network_info_multi_nic(self, instance, network_info):
        # multi-nic stuff here
        return networks

    def inject_network_info_default(self, instance):
        # old code here
        return networks

If possible, it's a good idea to have the inject_network_info stuff continue
to return `networks`; this is more consistent and allows you to make the
simplification noted above.

> 59 + self.write_to_param_xenstore(vm_ref, {location: mapping})

Syntax error. {'location': mapping}

> 61 + self.write_to_xenstore(vm_ref, location,
> 62 + mapping['location'])

Appears misaligned. Seems like we have to 'accepted' conventions here:

1. Line-up with the first arg
    self.write_to_xenstore(vm_ref, location,
                           mapping['location'])

2. +1 level hanging indent
    self.write_to_xenstore(vm_ref, location,
        mapping['location'])
OR

    self.write_to_xenstore(
        vm_ref, location, mapping['location'])

Looks like there's an opportunity to DRY up the code a bit. Looks like both
multi-nic and default both do something like

    for network in networks:
        try:
            self.write_to_xenstore(vm_ref, location,
                                           mapping['location'])
        except KeyError:
            # catch KeyError for domid if instance isn't running
            pass

There should be a way of extracting that out.

review: Needs Fixing

[Trey Morris (tr3buchet)]

refactoring!

[Trey Morris (tr3buchet)]

fixed. also merged qos and tested.

[Rick Harris (rconradharris)]

Really nice work here, Trey. I think this is a much cleaner patch than last time.

A couple of small nits:

> 127 + self.write_to_param_xenstore(vm_ref, {location: info})

location: => 'location':

> 201 + args, vm_ref)

Should probably dedent to match opening ( of function

> except KeyError:
> # catch KeyError for domid if instance isn't running
> pass

I'm unclear why/how a KeyError is raised in that case. Any info on that? (The catching of KeyError seems a little wonky in this case, that's why I bring it up.)

> 167 + device = 0
> 168 + for (network, info) in network_info:

Really minor, but we could eliminate the manual handling of the device counting-var by using `enumerate`:

  for device, (network, info) in enumerate(network_info):

Also might be clearer if `device_id`. Thoughts?

I'm a little worried that the code here has zero-test coverage, we've already had syntax errors sneak into both versions of the patch.

Would it be possible to:

a) Add a unit-test around _get_network_info, something simple to flex the code that generates the network_info mappings from instances

and

b) Add a unit-test around inject_network_info. For this we'd need to stub out the _make_plugin_call method.

You have some refactor-in-the-future TODOs in there, so in addition to flexing the code, these tests, if added, would probably help with the future refactoring work. Just a thought :)

[Trey Morris (tr3buchet)]

location is not supposed to be in quotes

This is just an opinion right?

KeyError is caught because when you attempt to write to the xenstore for an instance that has yet to be created there is no dom_id. This happens in the initial injection before boot and also in unittests.

I like the enumerate idea. playing with it now

in this refactor all code is covered by existing unittests

[Trey Morris (tr3buchet)]

sorry for blunt, in a hurry.

I love the enumerate. Your reviews are brilliant!

[Rick Harris (rconradharris)]

> location is not supposed to be in quotes

You're right. Not used to seeing dict-literals use variable-names for keys.

> This is just an opinion right?

Yep, it's my opinion that it's 'unclear' to me :)

The problem, as I see it, is that a domain missing from an instance *is not* a KeyError. It happens to trip a KeyError by virtue of how it's implemented, but, conceptually, I wouldn't expect that to be what _make_plugin_call raised.

I think ideally we'd have something like NoDomainForInstance exception. However, since Nova isn't yet being that specific with its exceptions, perhaps RuntimeError with a helpful msg would be appropriate. So,

  args = {'dom_id': vm_rec['domid'], 'path': path}

might become:

  if 'dom_id' not in vm_rec:
        raise RuntimeError(_('Instance %s does not have a domain') % instance_id)
  args = {'dom_id': vm_rec['domid'], 'path': path}

This is really minor, I'm raising this more for discussion than anything else, but it did cause some confusion for me while reviewing, so I thought it worth mentioning.

> I like the enumerate idea. playing with it now

Cool, enumerate is damn useful.

> in this refactor all code is covered by existing unittests

I'm not so sure about that. http://paste.openstack.org/show/948/

I changed `location` to `location1` (which is non-existent) and all of the tests still passed.

> sorry for blunt, in a hurry.

No worries!

[Rick Harris (rconradharris)]

We can work on unit-tests and KeyError issue in follow-up patches. For now, probably better to get this in sooner so multi-nic work can progress.

review: Approve

=== modified file 'nova/virt/xenapi/vm_utils.py'
--- nova/virt/xenapi/vm_utils.py	2011-03-17 19:34:45 +0000
+++ nova/virt/xenapi/vm_utils.py	2011-03-21 18:21:19 +0000
@@ -234,11 +234,11 @@
 
     @classmethod
     def create_vif(cls, session, vm_ref, network_ref, mac_address,
-                   dev="0", rxtx_cap=0):
+                   dev, rxtx_cap=0):
         """Create a VIF record.  Returns a Deferred that gives the new
         VIF reference."""
         vif_rec = {}
-        vif_rec['device'] = dev
+        vif_rec['device'] = str(dev)
         vif_rec['network'] = network_ref
         vif_rec['VM'] = vm_ref
         vif_rec['MAC'] = mac_address

=== modified file 'nova/virt/xenapi/vmops.py'
--- nova/virt/xenapi/vmops.py	2011-03-17 02:20:18 +0000
+++ nova/virt/xenapi/vmops.py	2011-03-21 18:21:19 +0000
@@ -81,11 +81,11 @@
                 instance.image_id, user, project, disk_image_type)
         return vdi_uuid
 
-    def spawn(self, instance):
+    def spawn(self, instance, network_info=None):
         vdi_uuid = self.create_disk(instance)
-        self._spawn_with_disk(instance, vdi_uuid=vdi_uuid)
+        self._spawn_with_disk(instance, vdi_uuid, network_info)
 
-    def _spawn_with_disk(self, instance, vdi_uuid):
+    def _spawn_with_disk(self, instance, vdi_uuid, network_info=None):
         """Create VM instance"""
         instance_name = instance.name
         vm_ref = VMHelper.lookup(self._session, instance_name)
@@ -129,8 +129,12 @@
                 vdi_ref=vdi_ref, userdevice=0, bootable=True)
 
         # inject_network_info and create vifs
-        networks = self.inject_network_info(instance)
-        self.create_vifs(instance, networks)
+        # TODO(tr3buchet) - check to make sure we have network info, otherwise
+        # create it now. This goes away once nova-multi-nic hits.
+        if network_info is None:
+            network_info = self._get_network_info(instance)
+        self.create_vifs(vm_ref, network_info)
+        self.inject_network_info(instance, vm_ref, network_info)
 
         LOG.debug(_('Starting VM %s...'), vm_ref)
         self._start(instance, vm_ref)
@@ -181,7 +185,7 @@
         timer.f = _wait_for_boot
 
         # call to reset network to configure network from xenstore
-        self.reset_network(instance)
+        self.reset_network(instance, vm_ref)
 
         return timer.start(interval=0.5, now=True)
 
@@ -685,24 +689,17 @@
         # TODO: implement this!
         return 'http://fakeajaxconsole/fake_url'
 
-    def inject_network_info(self, instance):
-        """
-        Generate the network info and make calls to place it into the
-        xenstore and the xenstore param list
-
-        """
-        # 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
-        vm_ref = self._get_vm_opaque_ref(instance.id)
-        logging.debug(_("injecting network info to xenstore for vm: |%s|"),
-                        vm_ref)
+    # TODO(tr3buchet) - remove this function after nova multi-nic
+    def _get_network_info(self, 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'])
+        IPs = db.fixed_ip_get_all_by_instance(admin_context,
+                                              instance['id'])
         networks = db.network_get_all_by_instance(admin_context,
                                                   instance['id'])
+        flavor = db.instance_type_get_by_name(admin_context,
+                                              instance['instance_type'])
+        network_info = []
         for network in networks:
             network_IPs = [ip for ip in IPs if ip.network_id == network.id]
 
@@ -719,67 +716,64 @@
                     "gateway": ip6.gatewayV6,
                     "enabled": "1"}
 
-            mac_id = instance.mac_address.replace(':', '')
-            location = 'vm-data/networking/%s' % mac_id
-            mapping = {
+            info = {
                 'label': network['label'],
                 'gateway': network['gateway'],
                 'mac': instance.mac_address,
+                'rxtx_cap': flavor['rxtx_cap'],
                 'dns': [network['dns']],
                 'ips': [ip_dict(ip) for ip in network_IPs],
                 'ip6s': [ip6_dict(ip) for ip in network_IPs]}
-
-            self.write_to_param_xenstore(vm_ref, {location: mapping})
-
+            network_info.append((network, info))
+        return network_info
+
+    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
+        """
+        logging.debug(_("injecting network info to xs for vm: |%s|"), vm_ref)
+
+        # this function raises if vm_ref is not a vm_opaque_ref
+        self._session.get_xenapi().VM.get_record(vm_ref)
+
+        for (network, info) in network_info:
+            location = 'vm-data/networking/%s' % info['mac'].replace(':', '')
+            self.write_to_param_xenstore(vm_ref, {location: info})
             try:
-                self.write_to_xenstore(vm_ref, location, mapping['location'])
+                # TODO(tr3buchet): fix function call after refactor
+                #self.write_to_xenstore(vm_ref, location, info)
+                self._make_plugin_call('xenstore.py', 'write_record', instance,
+                                       location, {'value': json.dumps(info)},
+                                       vm_ref)
             except KeyError:
                 # catch KeyError for domid if instance isn't running
                 pass
 
-        return networks
-
-    def create_vifs(self, instance, networks=None):
-        """
-        Creates vifs for an instance
-
-        """
-        vm_ref = self._get_vm_opaque_ref(instance['id'])
-        admin_context = context.get_admin_context()
-        flavor = db.instance_type_get_by_name(admin_context,
-                                              instance.instance_type)
+    def create_vifs(self, vm_ref, network_info):
+        """Creates vifs for an instance"""
         logging.debug(_("creating vif(s) for vm: |%s|"), vm_ref)
-        rxtx_cap = flavor['rxtx_cap']
-        if networks is None:
-            networks = db.network_get_all_by_instance(admin_context,
-                                                      instance['id'])
-        # 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
-        for network in networks:
+
+        # this function raises if vm_ref is not a vm_opaque_ref
+        self._session.get_xenapi().VM.get_record(vm_ref)
+
+        for device, (network, info) in enumerate(network_info):
+            mac_address = info['mac']
             bridge = network['bridge']
+            rxtx_cap = info.pop('rxtx_cap')
             network_ref = \
                 NetworkHelper.find_network_with_bridge(self._session, bridge)
 
-            if network_ref:
-                try:
-                    device = "1" if instance._rescue else "0"
-                except AttributeError:
-                    device = "0"
-
-                VMHelper.create_vif(self._session, vm_ref, network_ref,
-                                    instance.mac_address, device,
-                                    rxtx_cap=rxtx_cap)
-
-    def reset_network(self, instance):
-        """
-        Creates uuid arg to pass to make_agent_call and calls it.
-
-        """
+            VMHelper.create_vif(self._session, vm_ref, network_ref,
+                                mac_address, device, rxtx_cap)
+
+    def reset_network(self, instance, vm_ref):
+        """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)
+        # TODO(tr3buchet): fix function call after refactor
+        #resp = self._make_agent_call('resetnetwork', instance, '', args)
+        resp = self._make_plugin_call('agent', 'resetnetwork', instance, '',
+                                                               args, vm_ref)
 
     def list_from_xenstore(self, vm, path):
         """Runs the xenstore-ls command to get a listing of all records
@@ -820,25 +814,26 @@
         """
         self._make_xenstore_call('delete_record', vm, path)
 
-    def _make_xenstore_call(self, method, vm, path, addl_args={}):
+    def _make_xenstore_call(self, method, vm, path, addl_args=None):
         """Handles calls to the xenstore xenapi plugin."""
         return self._make_plugin_call('xenstore.py', method=method, vm=vm,
                 path=path, addl_args=addl_args)
 
-    def _make_agent_call(self, method, vm, path, addl_args={}):
+    def _make_agent_call(self, method, vm, path, addl_args=None):
         """Abstracts out the interaction with the agent xenapi plugin."""
         return self._make_plugin_call('agent', method=method, vm=vm,
                 path=path, addl_args=addl_args)
 
-    def _make_plugin_call(self, plugin, method, vm, path, addl_args={}):
+    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.
         Any errors raised by the plugin will in turn raise a RuntimeError here.
         """
         instance_id = vm.id
-        vm_ref = self._get_vm_opaque_ref(vm)
+        vm_ref = vm_ref or self._get_vm_opaque_ref(vm)
         vm_rec = self._session.get_xenapi().VM.get_record(vm_ref)
         args = {'dom_id': vm_rec['domid'], 'path': path}
-        args.update(addl_args)
+        args.update(addl_args or {})
         try:
             task = self._session.async_call_plugin(plugin, method, args)
             ret = self._session.wait_for_task(task, instance_id)