From 10c46619479e41e85b14343bd32159efda32775b Mon Sep 17 00:00:00 2001 From: Chris Behrens Date: Tue, 11 Jun 2013 19:00:02 +0000 Subject: Fix race conditions with xenstore xenapi code updates xenstore using _add_to_param_xenstore(), which has to ensure a key doesn't exist before it can add the same key. However, if you have 2 calls at the same time, this is racey. Fixes bug 1190026 Change-Id: I545b7a9fab0bc4c3749bc75387ed3f6ff27a512f --- nova/tests/virt/xenapi/test_xenapi.py | 18 +++++++++------ nova/virt/xenapi/vmops.py | 43 ++++++++++++++++++++++------------- 2 files changed, 38 insertions(+), 23 deletions(-) diff --git a/nova/tests/virt/xenapi/test_xenapi.py b/nova/tests/virt/xenapi/test_xenapi.py index bc166655d..f54af7465 100644 --- a/nova/tests/virt/xenapi/test_xenapi.py +++ b/nova/tests/virt/xenapi/test_xenapi.py @@ -3299,7 +3299,7 @@ class XenAPIInjectMetadataTestCase(stubs.XenAPITestBase): self.xenstore = dict(persist={}, ephem={}) def fake_get_vm_opaque_ref(inst, instance): - self.assertEqual(instance, 'instance') + self.assertEqual(instance, {'uuid': 'fake'}) return 'vm_ref' def fake_add_to_param_xenstore(inst, vm_ref, key, val): @@ -3312,12 +3312,12 @@ class XenAPIInjectMetadataTestCase(stubs.XenAPITestBase): del self.xenstore['persist'][key] def fake_write_to_xenstore(inst, instance, path, value, vm_ref=None): - self.assertEqual(instance, 'instance') + self.assertEqual(instance, {'uuid': 'fake'}) self.assertEqual(vm_ref, 'vm_ref') self.xenstore['ephem'][path] = jsonutils.dumps(value) def fake_delete_from_xenstore(inst, instance, path, vm_ref=None): - self.assertEqual(instance, 'instance') + self.assertEqual(instance, {'uuid': 'fake'}) self.assertEqual(vm_ref, 'vm_ref') if path in self.xenstore['ephem']: del self.xenstore['ephem'][path] @@ -3346,7 +3346,8 @@ class XenAPIInjectMetadataTestCase(stubs.XenAPITestBase): # Check xenstore key sanitizing system_metadata=[{'key': 'sys_a', 'value': 1}, {'key': 'sys_b', 'value': 2}, - {'key': 'sys_c', 'value': 3}]) + {'key': 'sys_c', 'value': 3}], + uuid='fake') self.conn._vmops.inject_instance_metadata(instance, 'vm_ref') self.assertEqual(self.xenstore, { @@ -3363,6 +3364,7 @@ class XenAPIInjectMetadataTestCase(stubs.XenAPITestBase): def test_change_instance_metadata_add(self): # Test XenStore key sanitizing here, too. diff = {'test.key': ['+', 4]} + instance = {'uuid': 'fake'} self.xenstore = { 'persist': { 'vm-data/user-metadata/a': '1', @@ -3376,7 +3378,7 @@ class XenAPIInjectMetadataTestCase(stubs.XenAPITestBase): }, } - self.conn._vmops.change_instance_metadata('instance', diff) + self.conn._vmops.change_instance_metadata(instance, diff) self.assertEqual(self.xenstore, { 'persist': { @@ -3395,6 +3397,7 @@ class XenAPIInjectMetadataTestCase(stubs.XenAPITestBase): def test_change_instance_metadata_update(self): diff = dict(b=['+', 4]) + instance = {'uuid': 'fake'} self.xenstore = { 'persist': { 'vm-data/user-metadata/a': '1', @@ -3408,7 +3411,7 @@ class XenAPIInjectMetadataTestCase(stubs.XenAPITestBase): }, } - self.conn._vmops.change_instance_metadata('instance', diff) + self.conn._vmops.change_instance_metadata(instance, diff) self.assertEqual(self.xenstore, { 'persist': { @@ -3425,6 +3428,7 @@ class XenAPIInjectMetadataTestCase(stubs.XenAPITestBase): def test_change_instance_metadata_delete(self): diff = dict(b=['-']) + instance = {'uuid': 'fake'} self.xenstore = { 'persist': { 'vm-data/user-metadata/a': '1', @@ -3438,7 +3442,7 @@ class XenAPIInjectMetadataTestCase(stubs.XenAPITestBase): }, } - self.conn._vmops.change_instance_metadata('instance', diff) + self.conn._vmops.change_instance_metadata(instance, diff) self.assertEqual(self.xenstore, { 'persist': { diff --git a/nova/virt/xenapi/vmops.py b/nova/virt/xenapi/vmops.py index e178d08fd..feffc55ac 100644 --- a/nova/virt/xenapi/vmops.py +++ b/nova/virt/xenapi/vmops.py @@ -1097,6 +1097,7 @@ class VMOps(object): def inject_instance_metadata(self, instance, vm_ref): """Inject instance metadata into xenstore.""" + @utils.synchronized('xenstore-' + instance['uuid']) def store_meta(topdir, data_list): for item in data_list: key = self._sanitize_xenstore_key(item['key']) @@ -1110,9 +1111,8 @@ class VMOps(object): def change_instance_metadata(self, instance, diff): """Apply changes to instance metadata to xenstore.""" vm_ref = self._get_vm_opaque_ref(instance) - for key, change in diff.items(): - key = self._sanitize_xenstore_key(key) - location = 'vm-data/user-metadata/%s' % key + + def process_change(location, change): if change[0] == '-': self._remove_from_param_xenstore(vm_ref, location) try: @@ -1131,6 +1131,14 @@ class VMOps(object): # catch KeyError for domid if instance isn't running pass + @utils.synchronized('xenstore-' + instance['uuid']) + def update_meta(): + for key, change in diff.items(): + key = self._sanitize_xenstore_key(key) + location = 'vm-data/user-metadata/%s' % key + process_change(location, change) + update_meta() + def _find_root_vdi_ref(self, vm_ref): """Find and return the root vdi ref for a VM.""" if not vm_ref: @@ -1524,19 +1532,22 @@ class VMOps(object): vm_ref = vm_ref or self._get_vm_opaque_ref(instance) LOG.debug(_("Injecting network info to xenstore"), instance=instance) - for vif in network_info: - xs_data = self._vif_xenstore_data(vif) - location = ('vm-data/networking/%s' % - vif['address'].replace(':', '')) - self._add_to_param_xenstore(vm_ref, - location, - jsonutils.dumps(xs_data)) - try: - self._write_to_xenstore(instance, location, xs_data, - vm_ref=vm_ref) - except KeyError: - # catch KeyError for domid if instance isn't running - pass + @utils.synchronized('xenstore-' + instance['uuid']) + def update_nwinfo(): + for vif in network_info: + xs_data = self._vif_xenstore_data(vif) + location = ('vm-data/networking/%s' % + vif['address'].replace(':', '')) + self._add_to_param_xenstore(vm_ref, + location, + jsonutils.dumps(xs_data)) + try: + self._write_to_xenstore(instance, location, xs_data, + vm_ref=vm_ref) + except KeyError: + # catch KeyError for domid if instance isn't running + pass + update_nwinfo() def _create_vifs(self, vm_ref, instance, network_info): """Creates vifs for an instance.""" -- cgit