Merge lp:~blamar/nova/libvirt-cleanup-branch into lp:~hudson-openstack/nova/trunk

[Brian Lamar]

Moved libvirt flag defines to libvirt/__init__.py, and modified lazy loading of libvirt/libxml2/Cheetah to use nova.utils.optional_import.

Two small cleanups in libvirt:

  1) Moved flag defines to __init__.py
  2) Replaced lazy-loading of imports with utils.optional_import (new)

  One unrelated change is the addition of unittest2 to pip-requires. I'm
  using the unittest.skipIf method in test_libvirt.py and unittest2 has
  backported all Python 2.7 unittest improvements.

[Alex Meade (alex-meade)]

I like it

review: Approve

[Brian Waldon (bcwaldon)]

Good refactoring, Brian. one minor thing:

164: Can you rephrase this log line to not repeat the word 'import'? And should it be info or debug?

[Ed Leafe (ed-leafe)]

358 + """Retrieve a libvirt connection object.
359 +
360 + :param read_only: If True, the libvirt connection will be read only.
361 + :returns: `nova.virt.libvirt.connection:LibvirtConnection`
362 +
363 + """

Can this be reduced to a single line docstring? The param name makes its use obvious, and the return value type is what the docstring already stated.

Also, is there a reason not to change all uses of unittest to unittest2? Or is that planned as part of another merge?

review: Needs Information

[lp:~blamar/nova/libvirt-cleanup-branch]

1219. By Brian Lamar on 2011-06-29

review feedback from bcwaldon and ed leafe

[Brian Lamar (blamar)]

Shortened comment to single-line (you're very right, it was unnecessarily redundant!) and re-worded the log line for bcwaldon.

Ed, there is not a blueprint I'm aware of, so I made one. Great suggestion. I'm not going to actively pursue it now, but it will be something to point to when I do get the time or someone else feels like taking it on.

https://blueprints.launchpad.net/nova/+spec/nova-unittest2

[William Wolf (throughnothing)]

This looks good

review: Approve

[Ed Leafe (ed-leafe)]

ok, lgtm.

review: Approve

[Devin Carlen (devcamcar)]

lgtm

review: Approve

[OpenStack Infra (hudson-openstack)]

The attempt to merge lp:~blamar/nova/libvirt-cleanup-branch into lp:nova failed. Below is the output from the failed tests.

FloatingIpTest
    test_floating_ip_allocate OK 0.14
    test_floating_ip_associate OK 0.21
    test_floating_ip_disassociate OK 0.09
    test_floating_ip_release OK 0.09
    test_floating_ip_show OK 0.09
    test_floating_ips_list OK 0.09
    test_translate_floating_ip_view OK 0.04
FlavorsExtraSpecsTest
    test_create OK 0.04
    test_create_empty_body OK 0.04
    test_delete OK 0.04
    test_index OK 0.05
    test_index_no_data OK 0.04
    test_show OK 0.05
    test_show_spec_not_found OK 0.25
    test_update_item OK 0.05
    test_update_item_body_uri_mismatch OK 0.04
    test_update_item_empty_body OK 0.04
    test_update_item_too_many_keys OK 0.04
AccountsTest
    test_account_create OK 0.17
    test_account_delete OK 0.17
    test_account_update OK 0.18
    test_get_account OK 0.40
AdminAPITest
    test_admin_disabled OK 0.13
    test_admin_enabled OK 0.16
APITest
    test_exceptions_are_converted_to_faults OK 0.01
    test_malformed_json OK 0.05
    test_malformed_xml OK 0.06
Test
    test_authorize_project OK 0.09
    test_authorize_token OK 0.34
    test_authorize_user OK 0.05
    test_bad_project OK 0.09
    test_bad_token OK 0.05
    test_bad_user_bad_key OK 0.05
    test_bad_user_good_key OK 0.05
    test_no_user OK 0.06
    test_not_existing_project OK 0.08
    test_token_expiry OK 0.27
TestFunctional
    test_token_doesnotexist OK 0.06
    test_token_expiry OK 0.08
TestLimiter
    test_authorize_token OK 0.10
LimiterTest
    test_limiter_custom_max_limit ...

[lp:~blamar/nova/libvirt-cleanup-branch]

1220. By Brian Lamar on 2011-07-01

Merged trunk.

1221. By Brian Lamar on 2011-07-08

Merged trunk

[OpenStack Infra (hudson-openstack)]

The attempt to merge lp:~blamar/nova/libvirt-cleanup-branch into lp:nova failed. Below is the output from the failed tests.

FloatingIpTest
    test_floating_ip_allocate OK 0.29
    test_floating_ip_associate OK 0.10
    test_floating_ip_disassociate OK 0.09
    test_floating_ip_release OK 0.10
    test_floating_ip_show OK 0.10
    test_floating_ips_list OK 0.10
    test_translate_floating_ip_view OK 0.04
FixedIpTest
    test_add_fixed_ip OK 0.08
    test_add_fixed_ip_no_network OK 0.08
    test_remove_fixed_ip OK 0.08
    test_remove_fixed_ip_no_address OK 0.27
FlavorsExtraSpecsTest
    test_create OK 0.04
    test_create_empty_body OK 0.05
    test_delete OK 0.05
    test_index OK 0.05
    test_index_no_data OK 0.05
    test_show OK 0.05
    test_show_spec_not_found OK 0.04
    test_update_item OK 0.05
    test_update_item_body_uri_mismatch OK 0.05
    test_update_item_empty_body OK 0.05
    test_update_item_too_many_keys OK 0.06
AccountsTest
    test_account_create OK 0.36
    test_account_delete OK 0.17
    test_account_update OK 0.18
    test_get_account OK 0.18
AdminAPITest
    test_admin_disabled OK 0.15
    test_admin_enabled OK 0.42
APITest
    test_exceptions_are_converted_to_faults OK 0.03
    test_malformed_json OK 0.05
    test_malformed_xml OK 0.08
Test
    test_authorize_project OK 0.10
    test_authorize_token OK 0.10
    test_authorize_user OK 0.06
    test_bad_project OK 0.34
    test_bad_token OK 0.06
    test_bad_user_bad_key OK 0.06
    test_bad_user_good_key OK 0.06
    test_no_user OK 0.05
    test_not_existing_project OK 0.10
    test_token_expiry OK ...

[OpenStack Infra (hudson-openstack)]

The attempt to merge lp:~blamar/nova/libvirt-cleanup-branch into lp:nova failed. Below is the output from the failed tests.

FloatingIpTest
    test_floating_ip_allocate OK 0.29
    test_floating_ip_associate OK 0.10
    test_floating_ip_disassociate OK 0.10
    test_floating_ip_release OK 0.10
    test_floating_ip_show OK 0.10
    test_floating_ips_list OK 0.09
    test_translate_floating_ip_view OK 0.04
FixedIpTest
    test_add_fixed_ip OK 0.08
    test_add_fixed_ip_no_network OK 0.08
    test_remove_fixed_ip OK 0.28
    test_remove_fixed_ip_no_address OK 0.07
FlavorsExtraSpecsTest
    test_create OK 0.05
    test_create_empty_body OK 0.05
    test_delete OK 0.05
    test_index OK 0.05
    test_index_no_data OK 0.05
    test_show OK 0.05
    test_show_spec_not_found OK 0.05
    test_update_item OK 0.05
    test_update_item_body_uri_mismatch OK 0.05
    test_update_item_empty_body OK 0.05
    test_update_item_too_many_keys OK 0.05
AccountsTest
    test_account_create OK 0.39
    test_account_delete OK 0.17
    test_account_update OK 0.18
    test_get_account OK 0.18
AdminAPITest
    test_admin_disabled OK 0.14
    test_admin_enabled OK 0.47
APITest
    test_exceptions_are_converted_to_faults OK 0.01
    test_malformed_json OK 0.06
    test_malformed_xml OK 0.06
Test
    test_authorize_project OK 0.10
    test_authorize_token OK 0.10
    test_authorize_user OK 0.06
    test_bad_project OK 0.35
    test_bad_token OK 0.06
    test_bad_user_bad_key OK 0.06
    test_bad_user_good_key OK 0.06
    test_no_user OK 0.06
    test_not_existing_project OK 0.10
    test_token_expiry OK ...

[Monty Taylor (mordred)]

skipIf seems like an excellent way to describe conditional skipping... except that now it's throwing a SkipTest error.

The other tests which skip things use skip_test ... could you look in to fixing things so that skipIf does not throw a SkipTest error (or that the test harness understands that we skipped that on purpose)

OR

If unittest2 was added to get skipIf and skipIf is causing these problems - perhaps just use skip_test and remove the unittest2 need? (I mean, unless there is some grand scheme for migrating to unittest2 underway)

review: Needs Fixing

Unmerged revisions

1221. By Brian Lamar on 2011-07-08

Merged trunk

1220. By Brian Lamar on 2011-07-01

Merged trunk.

1219. By Brian Lamar on 2011-06-29

review feedback from bcwaldon and ed leafe

1218. By Brian Lamar on 2011-06-28

Two small cleanups in libvirt:

1) Moved flag defines to __init__.py
2) Replaced lazy-loading of imports with utils.optional_import (new)

One unrelated change is the addition of unittest2 to pip-requires. I'm
using the unittest.skipIf method in test_libvirt.py and unittest2 has
backported all Python 2.7 unittest improvements.

=== modified file 'nova/tests/test_libvirt.py'
--- nova/tests/test_libvirt.py	2011-06-28 16:20:02 +0000
+++ nova/tests/test_libvirt.py	2011-07-08 00:31:40 +0000
@@ -22,6 +22,7 @@
 import shutil
 import sys
 
+import unittest2 as unittest
 from xml.etree.ElementTree import fromstring as xml_to_tree
 from xml.dom.minidom import parseString as xml_to_dom
 
@@ -37,7 +38,12 @@
 from nova.virt.libvirt import connection
 from nova.virt.libvirt import firewall
 
-libvirt = None
+
+libvirt = utils.optional_import("libvirt")
+libxml2 = utils.optional_import("libxml2")
+Template = utils.optional_import("Cheetah.Template")
+
+
 FLAGS = flags.FLAGS
 flags.DECLARE('instances_path', 'nova.compute.manager')
 
@@ -147,7 +153,6 @@
 
     def setUp(self):
         super(LibvirtConnTestCase, self).setUp()
-        connection._late_load_cheetah()
         self.flags(fake_call=True)
         self.manager = manager.AuthManager()
 
@@ -188,20 +193,6 @@
                      'image_ref':     '123456',
                      'instance_type_id': '5'}  # m1.small
 
-    def lazy_load_library_exists(self):
-        """check if libvirt is available."""
-        # try to connect libvirt. if fail, skip test.
-        try:
-            import libvirt
-            import libxml2
-        except ImportError:
-            return False
-        global libvirt
-        libvirt = __import__('libvirt')
-        connection.libvirt = __import__('libvirt')
-        connection.libxml2 = __import__('libxml2')
-        return True
-
     def create_fake_libvirt_mock(self, **kwargs):
         """Defining mocks for LibvirtConnection(libvirt is not used)."""
 
@@ -337,9 +328,6 @@
         self._check_xml_and_container(instance_data)
 
     def test_snapshot(self):
-        if not self.lazy_load_library_exists():
-            return
-
         FLAGS.image_service = 'nova.image.fake.FakeImageService'
 
         # Start test
@@ -372,9 +360,6 @@
         self.assertEquals(snapshot['name'], snapshot_name)
 
     def test_snapshot_no_image_architecture(self):
-        if not self.lazy_load_library_exists():
-            return
-
         FLAGS.image_service = 'nova.image.fake.FakeImageService'
 
         # Start test
@@ -649,10 +634,6 @@
 
     def test_ensure_filtering_rules_for_instance_timeout(self):
         """ensure_filtering_fules_for_instance() finishes with timeout."""
-        # Skip if non-libvirt environment
-        if not self.lazy_load_library_exists():
-            return
-
         # Preparing mocks
         def fake_none(self):
             return
@@ -690,12 +671,9 @@
 
         db.instance_destroy(self.context, instance_ref['id'])
 
+    @unittest.skipIf(libvirt is None, "libvirt required for this test")
     def test_live_migration_raises_exception(self):
         """Confirms recover method is called when exceptions are raised."""
-        # Skip if non-libvirt environment
-        if not self.lazy_load_library_exists():
-            return
-
         # Preparing data
         self.compute = utils.import_object(FLAGS.compute_manager)
         instance_dict = {'host': 'fake', 'state': power_state.RUNNING,
@@ -741,10 +719,6 @@
 
     @test.skip_test("test needs rewrite: instance no longer has mac_address")
     def test_spawn_with_network_info(self):
-        # Skip if non-libvirt environment
-        if not self.lazy_load_library_exists():
-            return
-
         # Preparing mocks
         def fake_none(self, instance):
             return
@@ -829,20 +803,6 @@
         self.fw = firewall.IptablesFirewallDriver(
                       get_connection=lambda: self.fake_libvirt_connection)
 
-    def lazy_load_library_exists(self):
-        """check if libvirt is available."""
-        # try to connect libvirt. if fail, skip test.
-        try:
-            import libvirt
-            import libxml2
-        except ImportError:
-            return False
-        global libvirt
-        libvirt = __import__('libvirt')
-        connection.libvirt = __import__('libvirt')
-        connection.libxml2 = __import__('libxml2')
-        return True
-
     def tearDown(self):
         self.manager.delete_project(self.project)
         self.manager.delete_user(self.user)
@@ -1056,10 +1016,6 @@
 
     @test.skip_test("skip libvirt test project_get_network no longer exists")
     def test_unfilter_instance_undefines_nwfilter(self):
-        # Skip if non-libvirt environment
-        if not self.lazy_load_library_exists():
-            return
-
         admin_ctxt = context.get_admin_context()
 
         fakefilter = NWFilterFakes()

=== modified file 'nova/utils.py'
--- nova/utils.py	2011-06-30 19:20:59 +0000
+++ nova/utils.py	2011-07-08 00:31:40 +0000
@@ -75,6 +75,21 @@
         return cls()
 
 
+def optional_import(import_str):
+    """Attempt to import the given string, but fall back to returning None.
+
+    :param import_str: The name of the module to import.
+    :returns: The requested module, or None if unable to import.
+
+    """
+    try:
+        __import__(import_str)
+        return sys.modules[import_str]
+    except ImportError:
+        LOG.info(_("Unable to import optional module: %s") % import_str)
+        return None
+
+
 def vpn_ping(address, port, timeout=0.05, session_id=None):
     """Sends a vpn negotiation packet and returns the server session.
 

=== modified file 'nova/virt/libvirt/__init__.py'
--- nova/virt/libvirt/__init__.py	2011-04-22 16:47:09 +0000
+++ nova/virt/libvirt/__init__.py	2011-07-08 00:31:40 +0000
@@ -0,0 +1,85 @@
+# vim: tabstop=4 shiftwidth=4 softtabstop=4
+
+# Copyright 2011 OpenStack, LLC
+#
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+from nova import flags
+from nova import utils
+
+
+FLAGS = flags.FLAGS
+
+
+flags.DECLARE('live_migration_retry_count', 'nova.compute.manager')
+
+flags.DEFINE_string('rescue_image_id',
+                    'ami-rescue', 'Rescue ami image')
+
+flags.DEFINE_string('rescue_kernel_id',
+                    'aki-rescue', 'Rescue aki image')
+
+flags.DEFINE_string('rescue_ramdisk_id',
+                    'ari-rescue',
+                    'Rescue ari image')
+
+flags.DEFINE_string('libvirt_xml_template',
+                    utils.abspath('virt/libvirt.xml.template'),
+                    'Libvirt XML Template')
+
+flags.DEFINE_string('libvirt_type',
+                    'kvm',
+                    'Libvirt driver (kvm, lxc, qemu, uml, or xen)')
+
+flags.DEFINE_string('libvirt_uri',
+                    '',
+                    'Override the default libvirt URI set by libvirt_type')
+
+flags.DEFINE_string('ajaxterm_portrange',
+                    '10000-12000',
+                    'Range of ports that ajaxterm should randomly try to bind')
+
+flags.DEFINE_string('firewall_driver',
+                    'nova.virt.libvirt.firewall.IptablesFirewallDriver',
+                    'Firewall driver (defaults to iptables)')
+
+flags.DEFINE_string('cpuinfo_xml_template',
+                    utils.abspath('virt/cpuinfo.xml.template'),
+                    'CpuInfo XML Template (Used only live migration now)')
+
+flags.DEFINE_string('live_migration_uri',
+                    "qemu+tcp://%s/system",
+                    'Define protocol used by live_migration feature')
+
+flags.DEFINE_string('live_migration_flag',
+                    "VIR_MIGRATE_UNDEFINE_SOURCE, VIR_MIGRATE_PEER2PEER",
+                    'Define live migration behavior.')
+
+flags.DEFINE_string('qemu_img', 'qemu-img',
+                    'binary to use for qemu-img commands')
+
+flags.DEFINE_integer('live_migration_bandwidth', 0,
+                    'Define live migration behavior')
+
+flags.DEFINE_bool('start_guests_on_host_boot',
+                  False,
+                  'Whether to restart guests when the host reboots')
+
+flags.DEFINE_bool('allow_project_net_traffic',
+                  True,
+                  'Whether to allow in project network traffic')
+
+flags.DEFINE_bool('use_cow_images',
+                  True,
+                  'Whether to use cow images')
+

=== modified file 'nova/virt/libvirt/connection.py'
--- nova/virt/libvirt/connection.py	2011-07-07 18:30:47 +0000
+++ nova/virt/libvirt/connection.py	2011-07-08 00:31:40 +0000
@@ -17,23 +17,10 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
-"""
-A connection to a hypervisor through libvirt.
+"""Provides logic for connecting to a hypervisor through libvirt.
 
 Supports KVM, LXC, QEMU, UML, and XEN.
 
-**Related Flags**
-
-:libvirt_type:  Libvirt domain type.  Can be kvm, qemu, uml, xen
-                (default: kvm).
-:libvirt_uri:  Override for the default libvirt URI (depends on libvirt_type).
-:libvirt_xml_template:  Libvirt XML Template.
-:rescue_image_id:  Rescue ami image (default: ami-rescue).
-:rescue_kernel_id:  Rescue aki image (default: aki-rescue).
-:rescue_ramdisk_id:  Rescue ari image (default: ari-rescue).
-:injected_network_template:  Template file for injected network
-:allow_project_net_traffic:  Whether to allow in project network traffic
-
 """
 
 import hashlib
@@ -71,83 +58,27 @@
 from nova.virt.libvirt import netutils
 
 
-libvirt = None
-libxml2 = None
-Template = None
+libvirt = utils.optional_import("libvirt")
+libxml2 = utils.optional_import("libxml2")
+Template = utils.optional_import("Cheetah.Template")
 
 
 LOG = logging.getLogger('nova.virt.libvirt_conn')
-
-
 FLAGS = flags.FLAGS
-flags.DECLARE('live_migration_retry_count', 'nova.compute.manager')
-# TODO(vish): These flags should probably go into a shared location
-flags.DEFINE_string('rescue_image_id', 'ami-rescue', 'Rescue ami image')
-flags.DEFINE_string('rescue_kernel_id', 'aki-rescue', 'Rescue aki image')
-flags.DEFINE_string('rescue_ramdisk_id', 'ari-rescue', 'Rescue ari image')
-flags.DEFINE_string('libvirt_xml_template',
-                    utils.abspath('virt/libvirt.xml.template'),
-                    'Libvirt XML Template')
-flags.DEFINE_string('libvirt_type',
-                    'kvm',
-                    'Libvirt domain type (valid options are: '
-                    'kvm, lxc, qemu, uml, xen)')
-flags.DEFINE_string('libvirt_uri',
-                    '',
-                    'Override the default libvirt URI (which is dependent'
-                    ' on libvirt_type)')
-flags.DEFINE_bool('allow_project_net_traffic',
-                  True,
-                  'Whether to allow in project network traffic')
-flags.DEFINE_bool('use_cow_images',
-                  True,
-                  'Whether to use cow images')
-flags.DEFINE_string('ajaxterm_portrange',
-                    '10000-12000',
-                    'Range of ports that ajaxterm should randomly try to bind')
-flags.DEFINE_string('firewall_driver',
-                    'nova.virt.libvirt.firewall.IptablesFirewallDriver',
-                    'Firewall driver (defaults to iptables)')
-flags.DEFINE_string('cpuinfo_xml_template',
-                    utils.abspath('virt/cpuinfo.xml.template'),
-                    'CpuInfo XML Template (Used only live migration now)')
-flags.DEFINE_string('live_migration_uri',
-                    "qemu+tcp://%s/system",
-                    'Define protocol used by live_migration feature')
-flags.DEFINE_string('live_migration_flag',
-                    "VIR_MIGRATE_UNDEFINE_SOURCE, VIR_MIGRATE_PEER2PEER",
-                    'Define live migration behavior.')
-flags.DEFINE_integer('live_migration_bandwidth', 0,
-                    'Define live migration behavior')
-flags.DEFINE_string('qemu_img', 'qemu-img',
-                    'binary to use for qemu-img commands')
-flags.DEFINE_bool('start_guests_on_host_boot', False,
-                  'Whether to restart guests when the host reboots')
 
 
 def get_connection(read_only):
-    # These are loaded late so that there's no need to install these
-    # libraries when not using libvirt.
-    # Cheetah is separate because the unit tests want to load Cheetah,
-    # but not libvirt.
-    global libvirt
-    global libxml2
+    """Retrieve a libvirt connection object."""
     if libvirt is None:
-        libvirt = __import__('libvirt')
+        raise ImportError(_("Unable to import libvirt"))
     if libxml2 is None:
-        libxml2 = __import__('libxml2')
-    _late_load_cheetah()
+        raise ImportError(_("Unable to import libxml2"))
+    if Template is None:
+        raise ImportError(_("Unable to import Cheetah.Template"))
+
     return LibvirtConnection(read_only)
 
 
-def _late_load_cheetah():
-    global Template
-    if Template is None:
-        t = __import__('Cheetah.Template', globals(), locals(),
-                       ['Template'], -1)
-        Template = t.Template
-
-
 def _strip_dev(mount_path):
         return re.sub(r'^/dev/', '', mount_path)
 
@@ -896,7 +827,7 @@
             nets.append(net_info)
 
         if have_injected_networks:
-            net = str(Template(ifc_template,
+            net = str(Template.Template(ifc_template,
                                searchList=[{'interfaces': nets,
                                             'use_ipv6': FLAGS.use_ipv6}]))
 
@@ -1035,7 +966,7 @@
         LOG.debug(_('instance %s: starting toXML method'), instance['name'])
         xml_info = self._prepare_xml_info(instance, rescue, network_info,
                                           block_device_mapping)
-        xml = str(Template(self.libvirt_xml, searchList=[xml_info]))
+        xml = str(Template.Template(self.libvirt_xml, searchList=[xml_info]))
         LOG.debug(_('instance %s: finished toXML method'), instance['name'])
         return xml
 
@@ -1439,7 +1370,7 @@
 
         LOG.info(_('Instance launched has CPU info:\n%s') % cpu_info)
         dic = utils.loads(cpu_info)
-        xml = str(Template(self.cpuinfo_xml, searchList=dic))
+        xml = str(Template.Template(self.cpuinfo_xml, searchList=dic))
         LOG.info(_('to xml...\n:%s ' % xml))
 
         u = "http://libvirt.org/html/libvirt-libvirt.html#virCPUCompareResult"

=== modified file 'nova/virt/libvirt/firewall.py'
--- nova/virt/libvirt/firewall.py	2011-06-27 21:48:03 +0000
+++ nova/virt/libvirt/firewall.py	2011-07-08 00:31:40 +0000
@@ -32,11 +32,7 @@
 FLAGS = flags.FLAGS
 
 
-try:
-    import libvirt
-except ImportError:
-    LOG.warn(_("Libvirt module could not be loaded. NWFilterFirewall will "
-               "not work correctly."))
+libvirt = utils.optional_import("libvirt")
 
 
 class FirewallDriver(object):

=== modified file 'tools/pip-requires'
--- tools/pip-requires	2011-06-30 18:11:03 +0000
+++ tools/pip-requires	2011-07-08 00:31:40 +0000
@@ -33,3 +33,4 @@
 nosexcover
 GitPython
 paramiko
+unittest2