VMware: fix the VNC port allocation

There is small chance for VNC port collisions with the current
implementation which choose the port number based on the MoRef id
of the VM.
This patch fixes this by running a query for all allocated ports
and then selects one which is not taken.

Change-Id: If7c3b14dd49ed05c5fde819c5a36d5608650cbbc
Closes-Bug: #1255609

nova/exception.py

@@ -909,6 +909,11 @@ class ConsoleTypeUnavailable(Invalid):
     msg_fmt = _("Unavailable console type %(console_type)s.")
 
 
+class ConsolePortRangeExhausted(NovaException):
+    msg_fmt = _("The console port range %(min_port)d-%(max_port)d is "
+                "exhausted.")
+
+
 class FlavorNotFound(NotFound):
     msg_fmt = _("Flavor %(flavor_id)s could not be found.")
 

nova/tests/virt/vmwareapi/test_driver_api.py

@@ -19,6 +19,7 @@
 Test suite for VMwareAPI.
 """
 
+import collections
 import contextlib
 import copy
 
@@ -1202,15 +1203,24 @@ class VMwareAPIVMTestCase(test.NoDBTestCase):
     def _test_get_vnc_console(self):
         self._create_vm()
         fake_vm = vmwareapi_fake._get_objects("VirtualMachine").objects[0]
-        fake_vm_id = int(fake_vm.obj.value.replace('vm-', ''))
+        OptionValue = collections.namedtuple('OptionValue', ['key', 'value'])
+        opt_val = OptionValue(key='', value=5906)
+        fake_vm.set(vm_util.VNC_CONFIG_KEY, opt_val)
         vnc_dict = self.conn.get_vnc_console(self.context, self.instance)
         self.assertEqual(vnc_dict['host'], self.vnc_host)
-        self.assertEqual(vnc_dict['port'], cfg.CONF.vmware.vnc_port +
-                         fake_vm_id % cfg.CONF.vmware.vnc_port_total)
+        self.assertEqual(vnc_dict['port'], 5906)
 
     def test_get_vnc_console(self):
         self._test_get_vnc_console()
 
+    def test_get_vnc_console_noport(self):
+        self._create_vm()
+        fake_vm = vmwareapi_fake._get_objects("VirtualMachine").objects[0]
+        self.assertRaises(exception.ConsoleTypeUnavailable,
+                          self.conn.get_vnc_console,
+                          self.context,
+                          self.instance)
+
     def test_host_ip_addr(self):
         self.assertEqual(self.conn.get_host_ip_addr(), "test_url")
 

nova/tests/virt/vmwareapi/test_vm_util.py

@@ -494,6 +494,31 @@ class VMwareVMUtilTestCase(test.NoDBTestCase):
         result = re.sub(r'\s+', '', repr(result))
         self.assertEqual(expected, result)
 
+    def _create_fake_vms(self):
+        fake_vms = fake.FakeRetrieveResult()
+        OptionValue = collections.namedtuple('OptionValue', ['key', 'value'])
+        for i in range(10):
+            vm = fake.ManagedObject()
+            opt_val = OptionValue(key='', value=5900 + i)
+            vm.set(vm_util.VNC_CONFIG_KEY, opt_val)
+            fake_vms.add_object(vm)
+        return fake_vms
+
+    def test_get_vnc_port(self):
+        fake_vms = self._create_fake_vms()
+        self.flags(vnc_port=5900, group='vmware')
+        self.flags(vnc_port_total=10000, group='vmware')
+        actual = vm_util.get_vnc_port(fake_session(fake_vms))
+        self.assertEqual(actual, 5910)
+
+    def test_get_vnc_port_exhausted(self):
+        fake_vms = self._create_fake_vms()
+        self.flags(vnc_port=5900, group='vmware')
+        self.flags(vnc_port_total=10, group='vmware')
+        self.assertRaises(exception.ConsolePortRangeExhausted,
+                          vm_util.get_vnc_port,
+                          fake_session(fake_vms))
+
     def test_get_all_cluster_refs_by_name_none(self):
         fake_objects = fake.FakeRetrieveResult()
         refs = vm_util.get_all_cluster_refs_by_name(fake_session(fake_objects),

nova/virt/vmwareapi/vm_util.py

@@ -22,12 +22,16 @@ import collections
 import copy
 import functools
 
+from oslo.config import cfg
+
 from nova import exception
 from nova.openstack.common.gettextutils import _
 from nova.openstack.common import log as logging
 from nova.openstack.common import units
+from nova import utils
 from nova.virt.vmwareapi import vim_util
 
+CONF = cfg.CONF
 LOG = logging.getLogger(__name__)
 # A cache for VM references. The key will be the VM name
 # and the value is the VM reference. The VM name is unique. This
@@ -77,6 +81,9 @@ def vm_ref_cache_from_name(func):
         return _vm_ref_cache(id, func, session, name)
     return wrapper
 
+# the config key which stores the VNC port
+VNC_CONFIG_KEY = 'config.extraConfig["RemoteDisplay.vnc.port"]'
+
 
 def build_datastore_path(datastore_name, path):
     """Build the datastore compliant path."""
@@ -685,6 +692,45 @@ def get_vnc_config_spec(client_factory, port):
     return virtual_machine_config_spec
 
 
+@utils.synchronized('vmware.get_vnc_port')
+def get_vnc_port(session):
+    """Return VNC port for an VM or None if there is no available port."""
+    min_port = CONF.vmware.vnc_port
+    port_total = CONF.vmware.vnc_port_total
+    allocated_ports = _get_allocated_vnc_ports(session)
+    max_port = min_port + port_total
+    for port in range(min_port, max_port):
+        if port not in allocated_ports:
+            return port
+    raise exception.ConsolePortRangeExhausted(min_port=min_port,
+                                              max_port=max_port)
+
+
+def _get_allocated_vnc_ports(session):
+    """Return an integer set of all allocated VNC ports."""
+    # TODO(rgerganov): bug #1256944
+    # The VNC port should be unique per host, not per vCenter
+    vnc_ports = set()
+    result = session._call_method(vim_util, "get_objects",
+                                  "VirtualMachine", [VNC_CONFIG_KEY])
+    while result:
+        for obj in result.objects:
+            if not hasattr(obj, 'propSet'):
+                continue
+            dynamic_prop = obj.propSet[0]
+            option_value = dynamic_prop.val
+            vnc_port = option_value.value
+            vnc_ports.add(int(vnc_port))
+        token = _get_token(result)
+        if token:
+            result = session._call_method(vim_util,
+                                          "continue_to_get_objects",
+                                          token)
+        else:
+            break
+    return vnc_ports
+
+
 def search_datastore_spec(client_factory, file_name):
     """Builds the datastore search spec."""
     search_spec = client_factory.create('ns0:HostDatastoreBrowserSearchSpec')

nova/virt/vmwareapi/vmops.py

@@ -333,7 +333,7 @@ class VMwareVMOps(object):
 
         # Set the vnc configuration of the instance, vnc port starts from 5900
         if CONF.vnc_enabled:
-            vnc_port = self._get_vnc_port(vm_ref)
+            vnc_port = vm_util.get_vnc_port(self._session)
             self._set_vnc_config(client_factory, instance, vnc_port)
 
         def _create_virtual_disk(folder, virtual_disk_path):
@@ -1445,9 +1445,17 @@ class VMwareVMOps(object):
     def get_vnc_console(self, instance):
         """Return connection info for a vnc console."""
         vm_ref = vm_util.get_vm_ref(self._session, instance)
+        opt_value = self._session._call_method(vim_util,
+                               'get_dynamic_property',
+                               vm_ref, 'VirtualMachine',
+                               vm_util.VNC_CONFIG_KEY)
+        if opt_value:
+            port = int(opt_value.value)
+        else:
+            raise exception.ConsoleTypeUnavailable(console_type='vnc')
 
         return {'host': CONF.vmware.host_ip,
-                'port': self._get_vnc_port(vm_ref),
+                'port': port,
                 'internal_access_path': None}
 
     def get_vnc_console_vcenter(self, instance):
@@ -1470,14 +1478,6 @@ class VMwareVMOps(object):
 
         return vnc_console
 
-    @staticmethod
-    def _get_vnc_port(vm_ref):
-        """Return VNC port for an VM."""
-        vm_id = int(vm_ref.value.replace('vm-', ''))
-        port = CONF.vmware.vnc_port + vm_id % CONF.vmware.vnc_port_total
-
-        return port
-
     @staticmethod
     def _get_machine_id_str(network_info):
         machine_id_str = ''