Addressed issues raised by Rick Harris' review

12 files changed, 243 insertions, 204 deletions

diff --git a/nova/network/vmwareapi_net.py b/nova/network/vmwareapi_net.py
index f1232dada..93e6584f0 100644
--- a/nova/network/vmwareapi_net.py
+++ b/nova/network/vmwareapi_net.py
@@ -25,7 +25,7 @@ from nova import flags
 from nova import log as logging
 from nova import utils
 from nova.virt.vmwareapi_conn import VMWareAPISession
-from nova.virt.vmwareapi.network_utils import NetworkHelper
+from nova.virt.vmwareapi import network_utils
 
 LOG = logging.getLogger("nova.network.vmwareapi_net")
 
@@ -50,13 +50,13 @@ def ensure_vlan_bridge(vlan_num, bridge, net_attrs=None):
                                FLAGS.vmwareapi_api_retry_count)
     vlan_interface = FLAGS.vlan_interface
     # Check if the vlan_interface physical network adapter exists on the host
-    if not NetworkHelper.check_if_vlan_interface_exists(session,
+    if not network_utils.check_if_vlan_interface_exists(session,
                                                         vlan_interface):
         raise exception.NotFound(_("There is no physical network adapter with "
                           "the name %s on the ESX host") % vlan_interface)
 
     # Get the vSwitch associated with the Physical Adapter
-    vswitch_associated = NetworkHelper.get_vswitch_for_vlan_interface(
+    vswitch_associated = network_utils.get_vswitch_for_vlan_interface(
                                         session, vlan_interface)
     if vswitch_associated is None:
         raise exception.NotFound(_("There is no virtual switch associated "
@@ -64,16 +64,16 @@ def ensure_vlan_bridge(vlan_num, bridge, net_attrs=None):
             vlan_interface)
     # Check whether bridge already exists and retrieve the the ref of the
     # network whose name_label is "bridge"
-    network_ref = NetworkHelper.get_network_with_the_name(session, bridge)
-    if network_ref == None:
+    network_ref = network_utils.get_network_with_the_name(session, bridge)
+    if network_ref is None:
         # Create a port group on the vSwitch associated with the vlan_interface
         # corresponding physical network adapter on the ESX host
-        NetworkHelper.create_port_group(session, bridge, vswitch_associated,
+        network_utils.create_port_group(session, bridge, vswitch_associated,
                                 vlan_num)
     else:
         # Get the vlan id and vswitch corresponding to the port group
         pg_vlanid, pg_vswitch = \
-            NetworkHelper.get_vlanid_and_vswitch_for_portgroup(session, bridge)
+            network_utils.get_vlanid_and_vswitch_for_portgroup(session, bridge)
 
         # Check if the vsiwtch associated is proper
         if pg_vswitch != vswitch_associated:
diff --git a/nova/tests/test_vmwareapi.py b/nova/tests/test_vmwareapi.py
index b31ac11f1..22b66010a 100644
--- a/nova/tests/test_vmwareapi.py
+++ b/nova/tests/test_vmwareapi.py
@@ -59,8 +59,7 @@ class VMWareAPIVMTestCase(test.TestCase):
                                            glance_stubs.FakeGlance)

         self.conn = vmwareapi_conn.get_connection(False)

 

-    def _create_vm(self):

-        """Create and spawn the VM."""

+    def _create_instance_in_the_db(self):

         values = {'name': 1,

                   'id': 1,

                   'project_id': self.project.id,

@@ -72,6 +71,10 @@ class VMWareAPIVMTestCase(test.TestCase):
                   'mac_address': 'aa:bb:cc:dd:ee:ff',

                   }

         self.instance = db.instance_create(values)

+

+    def _create_vm(self):

+        """Create and spawn the VM."""

+        self._create_instance_in_the_db()

         self.type_data = db.instance_type_get_by_name(None, 'm1.large')

         self.conn.spawn(self.instance)

         self._check_vm_record()

@@ -139,6 +142,11 @@ class VMWareAPIVMTestCase(test.TestCase):
         info = self.conn.get_info(1)

         self._check_vm_info(info, power_state.RUNNING)

 

+    def test_snapshot_non_existent(self):

+        self._create_instance_in_the_db()

+        self.assertRaises(Exception, self.conn.snapshot, self.instance,

+                          "Test-Snapshot")

+

     def test_reboot(self):

         self._create_vm()

         info = self.conn.get_info(1)

@@ -147,6 +155,19 @@ class VMWareAPIVMTestCase(test.TestCase):
         info = self.conn.get_info(1)

         self._check_vm_info(info, power_state.RUNNING)

 

+    def test_reboot_non_existent(self):

+        self._create_instance_in_the_db()

+        self.assertRaises(Exception, self.conn.reboot, self.instance)

+

+    def test_reboot_not_poweredon(self):

+        self._create_vm()

+        info = self.conn.get_info(1)

+        self._check_vm_info(info, power_state.RUNNING)

+        self.conn.suspend(self.instance, self.dummy_callback_handler)

+        info = self.conn.get_info(1)

+        self._check_vm_info(info, power_state.PAUSED)

+        self.assertRaises(Exception, self.conn.reboot, self.instance)

+

     def test_suspend(self):

         self._create_vm()

         info = self.conn.get_info(1)

@@ -155,6 +176,11 @@ class VMWareAPIVMTestCase(test.TestCase):
         info = self.conn.get_info(1)

         self._check_vm_info(info, power_state.PAUSED)

 

+    def test_suspend_non_existent(self):

+        self._create_instance_in_the_db()

+        self.assertRaises(Exception, self.conn.suspend, self.instance,

+                          self.dummy_callback_handler)

+

     def test_resume(self):

         self._create_vm()

         info = self.conn.get_info(1)

@@ -166,6 +192,18 @@ class VMWareAPIVMTestCase(test.TestCase):
         info = self.conn.get_info(1)

         self._check_vm_info(info, power_state.RUNNING)

 

+    def test_resume_non_existent(self):

+        self._create_instance_in_the_db()

+        self.assertRaises(Exception, self.conn.resume, self.instance,

+                          self.dummy_callback_handler)

+

+    def test_resume_not_suspended(self):

+        self._create_vm()

+        info = self.conn.get_info(1)

+        self._check_vm_info(info, power_state.RUNNING)

+        self.assertRaises(Exception, self.conn.resume, self.instance,

+                          self.dummy_callback_handler)

+

     def test_get_info(self):

         self._create_vm()

         info = self.conn.get_info(1)

@@ -176,10 +214,14 @@ class VMWareAPIVMTestCase(test.TestCase):
         info = self.conn.get_info(1)

         self._check_vm_info(info, power_state.RUNNING)

         instances = self.conn.list_instances()

-        self.assertTrue(len(instances) == 1)

+        self.assertEquals(len(instances), 1)

         self.conn.destroy(self.instance)

         instances = self.conn.list_instances()

-        self.assertTrue(len(instances) == 0)

+        self.assertEquals(len(instances), 0)

+

+    def test_destroy_non_existent(self):

+        self._create_instance_in_the_db()

+        self.assertEquals(self.conn.destroy(self.instance), None)

 

     def test_pause(self):

         pass

diff --git a/nova/virt/vmwareapi/error_util.py b/nova/virt/vmwareapi/error_util.py
index f14cafed5..53fa8f24d 100644
--- a/nova/virt/vmwareapi/error_util.py
+++ b/nova/virt/vmwareapi/error_util.py
@@ -41,7 +41,7 @@ class SessionOverLoadException(VimException):
 

 

 class VimAttributeError(VimException):

-    """VIM Attribute Error."""

+    """VI Attribute Error."""

     pass

 

 

@@ -57,15 +57,15 @@ class VimFaultException(Exception):
         return str(self.exception_obj)

 

 

-class FaultCheckers:

+class FaultCheckers(object):

     """

     Methods for fault checking of SOAP response. Per Method error handlers

     for which we desire error checking are defined. SOAP faults are

     embedded in the SOAP messages as properties and not as SOAP faults.

     """

 

-    @classmethod

-    def retrieveproperties_fault_checker(self, resp_obj):

+    @staticmethod

+    def retrieveproperties_fault_checker(resp_obj):

         """

         Checks the RetrieveProperties response for errors. Certain faults

         are sent as part of the SOAP body as property of missingSet.

diff --git a/nova/virt/vmwareapi/fake.py b/nova/virt/vmwareapi/fake.py
index 3afb46590..4bb467fa9 100644
--- a/nova/virt/vmwareapi/fake.py
+++ b/nova/virt/vmwareapi/fake.py
@@ -68,16 +68,16 @@ def cleanup():
         _db_content[c] = {}

 

 

-def _create_object(table, obj):

+def _create_object(table, table_obj):

     """Create an object in the db."""

-    _db_content[table][obj.obj] = obj

+    _db_content[table][table_obj.obj] = table_obj

 

 

-def _get_objects(type):

+def _get_objects(obj_type):

     """Get objects of the type."""

     lst_objs = []

-    for key in _db_content[type]:

-        lst_objs.append(_db_content[type][key])

+    for key in _db_content[obj_type]:

+        lst_objs.append(_db_content[obj_type][key])

     return lst_objs

 

 

@@ -88,19 +88,13 @@ class Prop(object):
         self.name = None

         self.val = None

 

-    def setVal(self, val):

-        self.val = val

-

-    def setName(self, name):

-        self.name = name

-

 

 class ManagedObject(object):

     """Managed Data Object base class."""

 

     def __init__(self, name="ManagedObject", obj_ref=None):

         """Sets the obj property which acts as a reference to the object."""

-        object.__setattr__(self, 'objName', name)

+        super(ManagedObject, self).__setattr__('objName', name)

         if obj_ref is None:

             obj_ref = str(uuid.uuid4())

         object.__setattr__(self, 'obj', obj_ref)

@@ -127,8 +121,8 @@ class ManagedObject(object):
                 prop.val = val

                 return

         elem = Prop()

-        elem.setName(attr)

-        elem.setVal(val)

+        elem.name = attr

+        elem.val = val

         self.propSet.append(elem)

 

     def __getattr__(self, attr):

@@ -143,15 +137,7 @@ class ManagedObject(object):
 

 class DataObject(object):

     """Data object base class."""

-

-    def __init__(self):

-        pass

-

-    def __getattr__(self, attr):

-        return object.__getattribute__(self, attr)

-

-    def __setattr__(self, attr, value):

-        object.__setattr__(self, attr, value)

+    pass

 

 

 class VirtualDisk(DataObject):

@@ -160,30 +146,24 @@ class VirtualDisk(DataObject):
     __class__.__name__ to 'VirtualDisk'. Refer place where __class__.__name__

     is used in the code.

     """

-

-    def __init__(self):

-        DataObject.__init__(self)

+    pass

 

 

 class VirtualDiskFlatVer2BackingInfo(DataObject):

     """VirtualDiskFlatVer2BackingInfo class."""

-

-    def __init__(self):

-        DataObject.__init__(self)

+    pass

 

 

 class VirtualLsiLogicController(DataObject):

     """VirtualLsiLogicController class."""

-

-    def __init__(self):

-        DataObject.__init__(self)

+    pass

 

 

 class VirtualMachine(ManagedObject):

     """Virtual Machine class."""

 

     def __init__(self, **kwargs):

-        ManagedObject.__init__(self, "VirtualMachine")

+        super(VirtualMachine, self).__init__("VirtualMachine")

         self.set("name", kwargs.get("name"))

         self.set("runtime.connectionState",

                  kwargs.get("conn_state", "connected"))

@@ -224,7 +204,7 @@ class VirtualMachine(ManagedObject):
             controller.key = controller_key

 

             self.set("config.hardware.device", [disk, controller])

-        except Exception:

+        except AttributeError:

             # Case of Reconfig of VM to set extra params

             self.set("config.extraConfig", val.extraConfig)

 

@@ -233,7 +213,7 @@ class Network(ManagedObject):
     """Network class."""

 

     def __init__(self):

-        ManagedObject.__init__(self, "Network")

+        super(Network, self).__init__("Network")

         self.set("summary.name", "vmnet0")

 

 

@@ -241,7 +221,7 @@ class ResourcePool(ManagedObject):
     """Resource Pool class."""

 

     def __init__(self):

-        ManagedObject.__init__(self, "ResourcePool")

+        super(ResourcePool, self).__init__("ResourcePool")

         self.set("name", "ResPool")

 

 

@@ -249,7 +229,7 @@ class Datastore(ManagedObject):
     """Datastore class."""

 

     def __init__(self):

-        ManagedObject.__init__(self, "Datastore")

+        super(Datastore, self).__init__("Datastore")

         self.set("summary.type", "VMFS")

         self.set("summary.name", "fake-ds")

 

@@ -258,7 +238,7 @@ class HostNetworkSystem(ManagedObject):
     """HostNetworkSystem class."""

 

     def __init__(self):

-        ManagedObject.__init__(self, "HostNetworkSystem")

+        super(HostNetworkSystem, self).__init__("HostNetworkSystem")

         self.set("name", "networkSystem")

 

         pnic_do = DataObject()

@@ -274,7 +254,7 @@ class HostSystem(ManagedObject):
     """Host System class."""

 

     def __init__(self):

-        ManagedObject.__init__(self, "HostSystem")

+        super(HostSystem, self).__init__("HostSystem")

         self.set("name", "ha-host")

         if _db_content.get("HostNetworkSystem", None) is None:

             create_host_network_system()

@@ -341,7 +321,7 @@ class Datacenter(ManagedObject):
     """Datacenter class."""

 

     def __init__(self):

-        ManagedObject.__init__(self, "Datacenter")

+        super(Datacenter, self).__init__("Datacenter")

         self.set("name", "ha-datacenter")

         self.set("vmFolder", "vm_folder_ref")

         if _db_content.get("Network", None) is None:

@@ -356,7 +336,7 @@ class Task(ManagedObject):
     """Task class."""

 

     def __init__(self, task_name, state="running"):

-        ManagedObject.__init__(self, "Task")

+        super(Task, self).__init__("Task")

         info = DataObject

         info.name = task_name

         info.state = state

@@ -406,7 +386,7 @@ def _add_file(file_path):
 

 def _remove_file(file_path):

     """Removes a file reference from the db."""

-    if _db_content.get("files", None) is None:

+    if _db_content.get("files") is None:

         raise exception.NotFound(_("No files have been added yet"))

     # Check if the remove is for a single file object or for a folder

     if file_path.find(".vmdk") != -1:

@@ -418,10 +398,9 @@ def _remove_file(file_path):
         # Removes the files in the folder and the folder too from the db

         for file in _db_content.get("files"):

             if file.find(file_path) != -1:

-                try:

-                    _db_content.get("files").remove(file)

-                except Exception:

-                    pass

+                lst_files = _db_content.get("files")

+                if lst_files and lst_files.count(file):

+                    lst_files.remove(file)

 

 

 def fake_fetch_image(image, instance, **kwargs):

@@ -457,9 +436,6 @@ def _get_vm_mdo(vm_ref):
 class FakeFactory(object):

     """Fake factory class for the suds client."""

 

-    def __init__(self):

-        pass

-

     def create(self, obj_name):

         """Creates a namespace object."""

         return DataObject()

@@ -661,7 +637,8 @@ class FakeVim(object):
                         for prop in properties:

                             temp_mdo.set(prop, mdo.get(prop))

                         lst_ret_objs.append(temp_mdo)

-            except Exception:

+            except Exception, exc:

+                LOG.exception(exc)

                 continue

         return lst_ret_objs

 

diff --git a/nova/virt/vmwareapi/io_util.py b/nova/virt/vmwareapi/io_util.py
index 7f321c1e7..2ec773b7b 100644
--- a/nova/virt/vmwareapi/io_util.py
+++ b/nova/virt/vmwareapi/io_util.py
@@ -82,8 +82,8 @@ class GlanceWriteThread(object):
             """Function to do the image data transfer through an update

             and thereon checks if the state is 'active'."""

             self.glance_client.update_image(self.image_id,

-                                     image_meta=self.image_meta,

-                                     image_data=self.input)

+                                            image_meta=self.image_meta,

+                                            image_data=self.input)

             self._running = True

             while self._running:

                 try:

diff --git a/nova/virt/vmwareapi/network_utils.py b/nova/virt/vmwareapi/network_utils.py
index 9232adab6..e77842535 100644
--- a/nova/virt/vmwareapi/network_utils.py
+++ b/nova/virt/vmwareapi/network_utils.py
@@ -28,123 +28,122 @@ from nova.virt.vmwareapi import vm_util
 LOG = logging.getLogger("nova.virt.vmwareapi.network_utils")

 

 

-class NetworkHelper:

-

-    @classmethod

-    def get_network_with_the_name(cls, session, network_name="vmnet0"):

-        """

-        Gets reference to the network whose name is passed as the

-        argument.

-        """

-        hostsystems = session._call_method(vim_util, "get_objects",

-                    "HostSystem", ["network"])

-        vm_networks_ret = hostsystems[0].propSet[0].val

-        # Meaning there are no networks on the host. suds responds with a ""

-        # in the parent property field rather than a [] in the

-        # ManagedObjectRefernce property field of the parent

-        if not vm_networks_ret:

-            return None

-        vm_networks = vm_networks_ret.ManagedObjectReference

-        networks = session._call_method(vim_util,

-                           "get_properties_for_a_collection_of_objects",

-                           "Network", vm_networks, ["summary.name"])

-        for network in networks:

-            if network.propSet[0].val == network_name:

-                return network.obj

+def get_network_with_the_name(session, network_name="vmnet0"):

+    """

+    Gets reference to the network whose name is passed as the

+    argument.

+    """

+    hostsystems = session._call_method(vim_util, "get_objects",

+                "HostSystem", ["network"])

+    vm_networks_ret = hostsystems[0].propSet[0].val

+    # Meaning there are no networks on the host. suds responds with a ""

+    # in the parent property field rather than a [] in the

+    # ManagedObjectRefernce property field of the parent

+    if not vm_networks_ret:

         return None

+    vm_networks = vm_networks_ret.ManagedObjectReference

+    networks = session._call_method(vim_util,

+                       "get_properties_for_a_collection_of_objects",

+                       "Network", vm_networks, ["summary.name"])

+    for network in networks:

+        if network.propSet[0].val == network_name:

+            return network.obj

+    return None

+

+

+def get_vswitch_for_vlan_interface(session, vlan_interface):

+    """

+    Gets the vswitch associated with the physical network adapter

+    with the name supplied.

+    """

+    # Get the list of vSwicthes on the Host System

+    host_mor = session._call_method(vim_util, "get_objects",

+         "HostSystem")[0].obj

+    vswitches_ret = session._call_method(vim_util,

+                "get_dynamic_property", host_mor,

+                "HostSystem", "config.network.vswitch")

+    # Meaning there are no vSwitches on the host. Shouldn't be the case,

+    # but just doing code check

+    if not vswitches_ret:

+        return

+    vswitches = vswitches_ret.HostVirtualSwitch

+    # Get the vSwitch associated with the network adapter

+    for elem in vswitches:

+        try:

+            for nic_elem in elem.pnic:

+                if str(nic_elem).split('-')[-1].find(vlan_interface) != -1:

+                    return elem.name

+        # Catching Attribute error as a vSwitch may not be associated with a

+        # physical NIC.

+        except AttributeError:

+            pass

 

-    @classmethod

-    def get_vswitch_for_vlan_interface(cls, session, vlan_interface):

-        """

-        Gets the vswitch associated with the physical network adapter

-        with the name supplied.

-        """

-        # Get the list of vSwicthes on the Host System

-        host_mor = session._call_method(vim_util, "get_objects",

-             "HostSystem")[0].obj

-        vswitches_ret = session._call_method(vim_util,

-                    "get_dynamic_property", host_mor,

-                    "HostSystem", "config.network.vswitch")

-        # Meaning there are no vSwitches on the host. Shouldn't be the case,

-        # but just doing code check

-        if not vswitches_ret:

-            return

-        vswitches = vswitches_ret.HostVirtualSwitch

-        # Get the vSwitch associated with the network adapter

-        for elem in vswitches:

-            try:

-                for nic_elem in elem.pnic:

-                    if str(nic_elem).split('-')[-1].find(vlan_interface) != -1:

-                        return elem.name

-            except Exception:

-                pass

 

-    @classmethod

-    def check_if_vlan_interface_exists(cls, session, vlan_interface):

-        """Checks if the vlan_inteface exists on the esx host."""

-        host_net_system_mor = session._call_method(vim_util, "get_objects",

-             "HostSystem", ["configManager.networkSystem"])[0].propSet[0].val

-        physical_nics_ret = session._call_method(vim_util,

-                    "get_dynamic_property", host_net_system_mor,

-                    "HostNetworkSystem", "networkInfo.pnic")

-        # Meaning there are no physical nics on the host

-        if not physical_nics_ret:

-            return False

-        physical_nics = physical_nics_ret.PhysicalNic

-        for pnic in physical_nics:

-            if vlan_interface == pnic.device:

-                return True

+def check_if_vlan_interface_exists(session, vlan_interface):

+    """Checks if the vlan_inteface exists on the esx host."""

+    host_net_system_mor = session._call_method(vim_util, "get_objects",

+         "HostSystem", ["configManager.networkSystem"])[0].propSet[0].val

+    physical_nics_ret = session._call_method(vim_util,

+                "get_dynamic_property", host_net_system_mor,

+                "HostNetworkSystem", "networkInfo.pnic")

+    # Meaning there are no physical nics on the host

+    if not physical_nics_ret:

         return False

+    physical_nics = physical_nics_ret.PhysicalNic

+    for pnic in physical_nics:

+        if vlan_interface == pnic.device:

+            return True

+    return False

 

-    @classmethod

-    def get_vlanid_and_vswitch_for_portgroup(cls, session, pg_name):

-        """Get the vlan id and vswicth associated with the port group."""

-        host_mor = session._call_method(vim_util, "get_objects",

-             "HostSystem")[0].obj

-        port_grps_on_host_ret = session._call_method(vim_util,

-                    "get_dynamic_property", host_mor,

-                    "HostSystem", "config.network.portgroup")

-        if not port_grps_on_host_ret:

-            excep = ("ESX SOAP server returned an empty port group "

-                    "for the host system in its response")

-            LOG.exception(excep)

-            raise exception.Error(_(excep))

-        port_grps_on_host = port_grps_on_host_ret.HostPortGroup

-        for p_gp in port_grps_on_host:

-            if p_gp.spec.name == pg_name:

-                p_grp_vswitch_name = p_gp.vswitch.split("-")[-1]

-                return p_gp.spec.vlanId, p_grp_vswitch_name

 

-    @classmethod

-    def create_port_group(cls, session, pg_name, vswitch_name, vlan_id=0):

-        """

-        Creates a port group on the host system with the vlan tags

-        supplied. VLAN id 0 means no vlan id association.

-        """

-        client_factory = session._get_vim().client.factory

-        add_prt_grp_spec = vm_util.get_add_vswitch_port_group_spec(

-                        client_factory,

-                        vswitch_name,

-                        pg_name,

-                        vlan_id)

-        host_mor = session._call_method(vim_util, "get_objects",

-             "HostSystem")[0].obj

-        network_system_mor = session._call_method(vim_util,

-            "get_dynamic_property", host_mor,

-            "HostSystem", "configManager.networkSystem")

-        LOG.debug(_("Creating Port Group with name %s on "

-                    "the ESX host") % pg_name)

-        try:

-            session._call_method(session._get_vim(),

-                    "AddPortGroup", network_system_mor,

-                    portgrp=add_prt_grp_spec)

-        except error_util.VimFaultException, exc:

-            # There can be a race condition when two instances try

-            # adding port groups at the same time. One succeeds, then

-            # the other one will get an exception. Since we are

-            # concerned with the port group being created, which is done

-            # by the other call, we can ignore the exception.

-            if error_util.FAULT_ALREADY_EXISTS not in exc.fault_list:

-                raise exception.Error(exc)

-        LOG.debug(_("Created Port Group with name %s on "

-                    "the ESX host") % pg_name)

+def get_vlanid_and_vswitch_for_portgroup(session, pg_name):

+    """Get the vlan id and vswicth associated with the port group."""

+    host_mor = session._call_method(vim_util, "get_objects",

+         "HostSystem")[0].obj

+    port_grps_on_host_ret = session._call_method(vim_util,

+                "get_dynamic_property", host_mor,

+                "HostSystem", "config.network.portgroup")

+    if not port_grps_on_host_ret:

+        excep = ("ESX SOAP server returned an empty port group "

+                "for the host system in its response")

+        LOG.exception(excep)

+        raise exception.Error(_(excep))

+    port_grps_on_host = port_grps_on_host_ret.HostPortGroup

+    for p_gp in port_grps_on_host:

+        if p_gp.spec.name == pg_name:

+            p_grp_vswitch_name = p_gp.vswitch.split("-")[-1]

+            return p_gp.spec.vlanId, p_grp_vswitch_name

+

+

+def create_port_group(session, pg_name, vswitch_name, vlan_id=0):

+    """

+    Creates a port group on the host system with the vlan tags

+    supplied. VLAN id 0 means no vlan id association.

+    """

+    client_factory = session._get_vim().client.factory

+    add_prt_grp_spec = vm_util.get_add_vswitch_port_group_spec(

+                    client_factory,

+                    vswitch_name,

+                    pg_name,

+                    vlan_id)

+    host_mor = session._call_method(vim_util, "get_objects",

+         "HostSystem")[0].obj

+    network_system_mor = session._call_method(vim_util,

+        "get_dynamic_property", host_mor,

+        "HostSystem", "configManager.networkSystem")

+    LOG.debug(_("Creating Port Group with name %s on "

+                "the ESX host") % pg_name)

+    try:

+        session._call_method(session._get_vim(),

+                "AddPortGroup", network_system_mor,

+                portgrp=add_prt_grp_spec)

+    except error_util.VimFaultException, exc:

+        # There can be a race condition when two instances try

+        # adding port groups at the same time. One succeeds, then

+        # the other one will get an exception. Since we are

+        # concerned with the port group being created, which is done

+        # by the other call, we can ignore the exception.

+        if error_util.FAULT_ALREADY_EXISTS not in exc.fault_list:

+            raise exception.Error(exc)

+    LOG.debug(_("Created Port Group with name %s on "

+                "the ESX host") % pg_name)

diff --git a/nova/virt/vmwareapi/read_write_util.py b/nova/virt/vmwareapi/read_write_util.py
index 237fd44dc..84f4942eb 100644
--- a/nova/virt/vmwareapi/read_write_util.py
+++ b/nova/virt/vmwareapi/read_write_util.py
@@ -15,7 +15,7 @@
 #    License for the specific language governing permissions and limitations

 #    under the License.

 

-"""Classes to handle image files.

+"""Classes to handle image files

 

 Collection of classes to handle image upload/download to/from Image service

 (like Glance image storage and retrieval service) from/to ESX/ESXi server.

@@ -43,7 +43,7 @@ USER_AGENT = "OpenStack-ESX-Adapter"
 

 try:

     READ_CHUNKSIZE = client.BaseClient.CHUNKSIZE

-except:

+except AttributeError:

     READ_CHUNKSIZE = 65536

 

 

@@ -91,8 +91,8 @@ class VMwareHTTPFile(object):
         """Close the file handle."""

         try:

             self.file_handle.close()

-        except Exception:

-            pass

+        except Exception, exc:

+            LOG.exception(exc)

 

     def __del__(self):

         """Close the file handle on garbage collection."""

diff --git a/nova/virt/vmwareapi/vim.py b/nova/virt/vmwareapi/vim.py
index 61a0dd2b3..ba14f1512 100644
--- a/nova/virt/vmwareapi/vim.py
+++ b/nova/virt/vmwareapi/vim.py
@@ -16,7 +16,7 @@
 #    under the License.

 

 """

-Classes that facilitate SOAP calls for VMware VI API.

+Classes for making VMware VI SOAP calls.

 """

 

 import httplib

@@ -80,7 +80,7 @@ class Vim:
         wsdl_url = FLAGS.vmwareapi_wsdl_loc

         if wsdl_url is None:

             raise Exception(_("Must specify vmwareapi_wsdl_loc"))

-        # Use this when VMware fixes their faulty wsdl

+        # TODO(sateesh): Use this when VMware fixes their faulty wsdl

         #wsdl_url = '%s://%s/sdk/vimService.wsdl' % (self._protocol,

         #        self._host_name)

         url = '%s://%s/sdk' % (self._protocol, self._host_name)

diff --git a/nova/virt/vmwareapi/vim_util.py b/nova/virt/vmwareapi/vim_util.py
index a0088cb6d..11214231c 100644
--- a/nova/virt/vmwareapi/vim_util.py
+++ b/nova/virt/vmwareapi/vim_util.py
@@ -27,11 +27,12 @@ def build_selection_spec(client_factory, name):
     return sel_spec

 

 

-def build_traversal_spec(client_factory, name, type, path, skip, select_set):

+def build_traversal_spec(client_factory, name, spec_type, path, skip,

+                         select_set):

     """Builds the traversal spec object."""

     traversal_spec = client_factory.create('ns0:TraversalSpec')

     traversal_spec.name = name

-    traversal_spec.type = type

+    traversal_spec.type = spec_type

     traversal_spec.path = path

     traversal_spec.skip = skip

     traversal_spec.selectSet = select_set

@@ -131,7 +132,7 @@ def get_object_properties(vim, collector, mobj, type, properties):
         usecoll = vim.get_service_content().propertyCollector

     property_filter_spec = client_factory.create('ns0:PropertyFilterSpec')

     property_spec = client_factory.create('ns0:PropertySpec')

-    property_spec.all = (properties == None or len(properties) == 0)

+    property_spec.all = (properties is None or len(properties) == 0)

     property_spec.pathSet = properties

     property_spec.type = type

     object_spec = client_factory.create('ns0:ObjectSpec')

@@ -170,10 +171,10 @@ def get_objects(vim, type, properties_to_collect=["name"], all=False):
                                 specSet=[property_filter_spec])

 

 

-def get_prop_spec(client_factory, type, properties):

+def get_prop_spec(client_factory, spec_type, properties):

     """Builds the Property Spec Object."""

     prop_spec = client_factory.create('ns0:PropertySpec')

-    prop_spec.type = type

+    prop_spec.type = spec_type

     prop_spec.pathSet = properties

     return prop_spec

 

diff --git a/nova/virt/vmwareapi/vm_util.py b/nova/virt/vmwareapi/vm_util.py
index f2ef8d2d7..a2fa7600c 100644
--- a/nova/virt/vmwareapi/vm_util.py
+++ b/nova/virt/vmwareapi/vm_util.py
@@ -126,8 +126,8 @@ def create_network_spec(client_factory, network_name, mac_address):
     return network_spec

 

 

-def get_vmdk_attach_config_spec(client_factory,

-                disksize, file_path, adapter_type="lsiLogic"):

+def get_vmdk_attach_config_spec(client_factory, disksize, file_path,

+                                adapter_type="lsiLogic"):

     """Builds the vmdk attach config spec."""

     config_spec = client_factory.create('ns0:VirtualMachineConfigSpec')

 

@@ -198,8 +198,8 @@ def get_vmdk_create_spec(client_factory, size_in_kb, adapter_type="lsiLogic"):
     return create_vmdk_spec

 

 

-def create_virtual_disk_spec(client_factory,

-                disksize, controller_key, file_path=None):

+def create_virtual_disk_spec(client_factory, disksize, controller_key,

+                             file_path=None):

     """

     Builds spec for the creation of a new/ attaching of an already existing

     Virtual Disk to the VM.

@@ -288,7 +288,7 @@ def get_machine_id_change_spec(client_factory, mac, ip_addr, netmask, gateway):
 

 

 def get_add_vswitch_port_group_spec(client_factory, vswitch_name,

-                                           port_group_name, vlan_id):

+                                    port_group_name, vlan_id):

     """Builds the virtual switch port group add spec."""

     vswitch_port_group_spec = client_factory.create('ns0:HostPortGroupSpec')

     vswitch_port_group_spec.name = port_group_name

diff --git a/nova/virt/vmwareapi/vmops.py b/nova/virt/vmwareapi/vmops.py
index e09b89e39..cf6c88bbd 100644
--- a/nova/virt/vmwareapi/vmops.py
+++ b/nova/virt/vmwareapi/vmops.py
@@ -35,7 +35,7 @@ from nova.compute import power_state
 from nova.virt.vmwareapi import vim_util

 from nova.virt.vmwareapi import vm_util

 from nova.virt.vmwareapi import vmware_images

-from nova.virt.vmwareapi.network_utils import NetworkHelper

+from nova.virt.vmwareapi import network_utils

 

 FLAGS = flags.FLAGS

 LOG = logging.getLogger("nova.virt.vmwareapi.vmops")

@@ -113,7 +113,7 @@ class VMWareVMOps(object):
 

         def _check_if_network_bridge_exists():

             network_ref = \

-                NetworkHelper.get_network_with_the_name(self._session,

+                network_utils.get_network_with_the_name(self._session,

                                                         net_name)

             if network_ref is None:

                 raise exception.NotFound(_("Network with the name '%s' doesn't"

@@ -590,8 +590,8 @@ class VMWareVMOps(object):
                              "got this exception while deleting"

                              " the VM contents from the disk: %s")

                              % str(excep))

-        except Exception, e:

-            LOG.exception(e)

+        except Exception, exc:

+            LOG.exception(exc)

 

     def pause(self, instance, callback):

         """Pause a VM instance."""

diff --git a/nova/virt/vmwareapi_conn.py b/nova/virt/vmwareapi_conn.py
index 414b8731d..87c3fa299 100644
--- a/nova/virt/vmwareapi_conn.py
+++ b/nova/virt/vmwareapi_conn.py
@@ -229,7 +229,12 @@ class VMWareAPISession(object):
                                 self.vim.get_service_content().sessionManager,

                                 sessionId=[self._session_id])

                     except Exception, excep:

-                        LOG.exception(excep)

+                        # This exception is something we can live with. It is

+                        # just an extra caution on our side. The session may

+                        # have been cleared. We could have made a call to

+                        # SessionIsActive, but that is an overhead because we

+                        # anyway would have to call TerminateSession.

+                        LOG.debug(excep)

                 self._session_id = session.key

                 return

             except Exception, excep:

@@ -243,8 +248,10 @@ class VMWareAPISession(object):
         # ESX host

         try:

             self.vim.Logout(self.vim.get_service_content().sessionManager)

-        except Exception:

-            pass

+        except Exception, excep:

+            # It is just cautionary on our part to do a logout in del just

+            # to ensure that the session is not left active.

+            LOG.debug(excep)

 

     def _is_vim_object(self, module):

         """Check if the module is a VIM Object instance."""

@@ -258,6 +265,7 @@ class VMWareAPISession(object):
         args = list(args)

         retry_count = 0

         exc = None

+        last_fault_list = []

         while True:

             try:

                 if not self._is_vim_object(module):

@@ -279,6 +287,18 @@ class VMWareAPISession(object):
                 # and then proceeding ahead with the call.

                 exc = excep

                 if error_util.FAULT_NOT_AUTHENTICATED in excep.fault_list:

+                    # Because of the idle session returning an empty

+                    # RetrievePropertiesResponse and also the same is returned

+                    # when there is say empty answer to the query for

+                    # VMs on the host ( as in no VMs on the host), we have no

+                    # way to differentiate.

+                    # So if the previous response was also am empty response

+                    # and after creating a new session, we get the same empty

+                    # response, then we are sure of the response being supposed

+                    # to be empty.

+                    if error_util.FAULT_NOT_AUTHENTICATED in last_fault_list:

+                        return []

+                    last_fault_list = excep.fault_list

                     self._create_session()

                 else:

                     # No re-trying for errors for API call has gone through