VMware: Fix memory leaks caused by caches

Using suds objects as keys for the DatastoreBrowser cache is incorrect
because they don't implement __eq__ and __hash__ for the VIM types.
This always results in cache miss and the cache grows with every spawn()
operation.

This patch fix this by using the 'value' property (which is string) of
the MoRef as key.

Change-Id: I2bcaf87e733d51055566aee41bb0a7e254027ba9
Closes-Bug: 1316433

nova/tests/virt/vmwareapi/test_imagecache.py

@@ -104,26 +104,18 @@ class ImageCacheManagerTestCase(test.NoDBTestCase):
         self.assertEqual(self._time, t)
 
     def test_get_ds_browser(self):
-        def fake_get_dynamic_property(vim, mobj, type, property_name):
-            self.assertEqual('Datastore', type)
-            self.assertEqual('browser', property_name)
-            self.fake_called += 1
-            return 'fake-ds-browser'
-
-        with contextlib.nested(
-            mock.patch.object(vim_util, 'get_dynamic_property',
-                              fake_get_dynamic_property)
-        ):
-            self.fake_called = 0
-            self.assertEqual({}, self._imagecache._ds_browser)
-            browser = self._imagecache._get_ds_browser('fake-ds-ref')
-            self.assertEqual('fake-ds-browser', browser)
-            self.assertEqual({'fake-ds-ref': 'fake-ds-browser'},
-                             self._imagecache._ds_browser)
-            self.assertEqual(1, self.fake_called)
-            browser = self._imagecache._get_ds_browser('fake-ds-ref')
-            self.assertEqual('fake-ds-browser', browser)
-            self.assertEqual(1, self.fake_called)
+        cache = self._imagecache._ds_browser
+        ds_browser = mock.Mock()
+        moref = fake.ManagedObjectReference('datastore-100')
+        self.assertIsNone(cache.get(moref.value))
+        mock_get_method = mock.Mock(return_value=ds_browser)
+        with mock.patch.object(vim_util, 'get_dynamic_property',
+                               mock_get_method):
+            ret = self._imagecache._get_ds_browser(moref)
+            mock_get_method.assert_called_once_with(mock.ANY, moref,
+                                                    'Datastore', 'browser')
+            self.assertIs(ds_browser, ret)
+            self.assertIs(ds_browser, cache.get(moref.value))
 
     def test_list_base_images(self):
         def fake_get_dynamic_property(vim, mobj, type, property_name):
@@ -140,7 +132,8 @@ class ImageCacheManagerTestCase(test.NoDBTestCase):
             mock.patch.object(ds_util, 'get_sub_folders',
                               fake_get_sub_folders)
         ) as (_get_dynamic, _get_sub_folders):
-            datastore = {'name': 'ds', 'ref': 'fake-ds-ref'}
+            fake_ds_ref = fake.ManagedObjectReference('fake-ds-ref')
+            datastore = {'name': 'ds', 'ref': fake_ds_ref}
             ds_path = ds_util.build_datastore_path(datastore['name'],
                                                    'base_folder')
             images = self._imagecache._list_datastore_images(

nova/tests/virt/vmwareapi/test_vmops.py

@@ -522,3 +522,17 @@ class VMwareVMOpsTestCase(test.NoDBTestCase):
             # we don't care what the log message is, we just want to make sure
             # our stub method is called which asserts the password is scrubbed
             self.assertTrue(debug_mock.called)
+
+    def test_get_ds_browser(self):
+        cache = self._vmops._datastore_browser_mapping
+        ds_browser = mock.Mock()
+        moref = vmwareapi_fake.ManagedObjectReference('datastore-100')
+        self.assertIsNone(cache.get(moref.value))
+        mock_call_method = mock.Mock(return_value=ds_browser)
+        with mock.patch.object(self._session, '_call_method',
+                               mock_call_method):
+            ret = self._vmops._get_ds_browser(moref)
+            mock_call_method.assert_called_once_with(vim_util,
+                'get_dynamic_property', moref, 'Datastore', 'browser')
+            self.assertIs(ds_browser, ret)
+            self.assertIs(ds_browser, cache.get(moref.value))

nova/virt/vmwareapi/imagecache.py

@@ -105,12 +105,12 @@ class ImageCacheManager(imagecache.ImageCacheManager):
         return timeutils.parse_strtime(ts, fmt=TIMESTAMP_FORMAT)
 
     def _get_ds_browser(self, ds_ref):
-        ds_browser = self._ds_browser.get(ds_ref)
+        ds_browser = self._ds_browser.get(ds_ref.value)
         if not ds_browser:
             ds_browser = vim_util.get_dynamic_property(
                     self._session._get_vim(), ds_ref,
                     "Datastore", "browser")
-            self._ds_browser[ds_ref] = ds_browser
+            self._ds_browser[ds_ref.value] = ds_browser
         return ds_browser
 
     def _list_datastore_images(self, ds_path, datastore):

nova/virt/vmwareapi/vmops.py

@@ -1387,12 +1387,12 @@ class VMwareVMOps(object):
                   instance=instance)
 
     def _get_ds_browser(self, ds_ref):
-        ds_browser = self._datastore_browser_mapping.get(ds_ref)
+        ds_browser = self._datastore_browser_mapping.get(ds_ref.value)
         if not ds_browser:
             ds_browser = self._session._call_method(
                 vim_util, "get_dynamic_property", ds_ref, "Datastore",
                 "browser")
-            self._datastore_browser_mapping[ds_ref] = ds_browser
+            self._datastore_browser_mapping[ds_ref.value] = ds_browser
         return ds_browser
 
     def get_datacenter_ref_and_name(self, ds_ref):