From 5ada538559142a1cf26ac30c05d238b12084bf72 Mon Sep 17 00:00:00 2001 From: Eric Fried Date: Tue, 14 Feb 2017 15:30:10 -0600 Subject: [PATCH] Synchronize power ops When separate threads are issuing power on/off requests on a VM (possibly through spawns, deletes, power_on, power_off, reboot, etc.), timing issues can arise when threads are retrieving and acting on the VM state. This change set synchronizes power on/off operations per instance to avoid this. A side-effect of this change is that the PowerOn Task no longer requires the lpar_wrap from prior Tasks. Change-Id: Ia21a3f98f51f338115d3728702881bc747f67b9f Closes-Bug: #1664720 --- .../tests/virt/powervm/test_driver.py | 45 +++------ nova_powervm/tests/virt/powervm/test_vm.py | 53 +++++++--- nova_powervm/virt/powervm/driver.py | 8 -- nova_powervm/virt/powervm/tasks/storage.py | 4 +- nova_powervm/virt/powervm/tasks/vm.py | 17 +--- nova_powervm/virt/powervm/vm.py | 99 ++++++++++--------- 6 files changed, 107 insertions(+), 119 deletions(-) diff --git a/nova_powervm/tests/virt/powervm/test_driver.py b/nova_powervm/tests/virt/powervm/test_driver.py index 8346e6f5..cf5a1fee 100644 --- a/nova_powervm/tests/virt/powervm/test_driver.py +++ b/nova_powervm/tests/virt/powervm/test_driver.py @@ -331,9 +331,8 @@ class TestPowerVMDriver(test.TestCase): self.crt_lpar.assert_called_with( self.apt, self.drv.host_wrapper, self.inst, self.inst.get_flavor(), nvram=None, slot_mgr=self.slot_mgr) - mock_pwron.assert_called_once_with( - self.apt, self.inst, entry=self.crt_lpar.return_value, - opts='fake-opts') + mock_pwron.assert_called_once_with(self.apt, self.inst, + opts='fake-opts') mock_sanitize.assert_not_called() # Assert that tasks that are not supposed to be called are not called self.assertFalse(mock_conn_vol.called) @@ -375,9 +374,7 @@ class TestPowerVMDriver(test.TestCase): self.assertTrue(self.validate_vopt.called) # Power on was called - mock_pwron.assert_called_once_with( - self.apt, self.inst, entry=self.crt_lpar.return_value, - opts=mock_opts) + mock_pwron.assert_called_once_with(self.apt, self.inst, opts=mock_opts) mock_opts.remove_optical.assert_called_with('fake-name', time=60) mock_sanitize.assert_called_with( self.inst.name, prefix='cfg_', suffix='.iso', max_len=37) @@ -427,9 +424,7 @@ class TestPowerVMDriver(test.TestCase): self.inst, self.inst.get_flavor(), nvram=None, slot_mgr=self.slot_mgr) # Power on was called - mock_pwron.assert_called_once_with( - self.apt, self.inst, entry=self.crt_lpar.return_value, - opts=mock.ANY) + mock_pwron.assert_called_once_with(self.apt, self.inst, opts=mock.ANY) # Check that the connect volume was called self.assertEqual(2, self.vol_drv.connect_volume.call_count) @@ -492,9 +487,7 @@ class TestPowerVMDriver(test.TestCase): self.inst, self.inst.get_flavor(), nvram=None, slot_mgr=self.slot_mgr) # Power on was called - mock_pwron.assert_called_once_with( - self.apt, self.inst, entry=self.crt_lpar.return_value, - opts=mock.ANY) + mock_pwron.assert_called_once_with(self.apt, self.inst, opts=mock.ANY) # Check that the connect volume was called self.assertEqual(2, self.vol_drv.connect_volume.call_count) @@ -542,9 +535,7 @@ class TestPowerVMDriver(test.TestCase): self.inst, self.inst.get_flavor(), nvram=None, slot_mgr=self.slot_mgr) # Power on was called - mock_pwron.assert_called_once_with( - self.apt, self.inst, entry=self.crt_lpar.return_value, - opts=mock.ANY) + mock_pwron.assert_called_once_with(self.apt, self.inst, opts=mock.ANY) # Check that the connect volume was called self.assertEqual(2, self.vol_drv.connect_volume.call_count) @@ -663,9 +654,7 @@ class TestPowerVMDriver(test.TestCase): self.assertEqual(2, self.vol_drv.connect_volume.call_count) # Power on was called - mock_pwron.assert_called_once_with( - self.apt, self.inst, entry=self.crt_lpar.return_value, - opts=mock.ANY) + mock_pwron.assert_called_once_with(self.apt, self.inst, opts=mock.ANY) # Validate the rollbacks were called self.assertEqual(2, self.vol_drv.disconnect_volume.call_count) @@ -716,9 +705,8 @@ class TestPowerVMDriver(test.TestCase): self.assertTrue(mock_update_lod_src.called) # Power on was called - mock_pwron.assert_called_once_with( - self.apt, self.inst_ibmi, entry=self.crt_lpar.return_value, - opts=mock.ANY) + mock_pwron.assert_called_once_with(self.apt, self.inst_ibmi, + opts=mock.ANY) # Check that the connect volume was called self.assertEqual(2, self.vol_drv.connect_volume.call_count) @@ -771,9 +759,8 @@ class TestPowerVMDriver(test.TestCase): self.apt, self.drv.host_wrapper, self.inst_ibmi, self.inst_ibmi.get_flavor(), nvram=None, slot_mgr=self.slot_mgr) self.assertTrue(mock_update_lod_src.called) - mock_pwron.assert_called_once_with( - self.apt, self.inst_ibmi, entry=self.crt_lpar.return_value, - opts=mock.ANY) + mock_pwron.assert_called_once_with(self.apt, self.inst_ibmi, + opts=mock.ANY) # Assert that tasks that are not supposed to be called are not called self.assertFalse(mock_conn_vol.called) self.assertFalse(mock_crt_cfg_drv.called) @@ -1404,7 +1391,6 @@ class TestPowerVMDriver(test.TestCase): 'connect_vol_*', 'save_bdm_fake_vol2', 'fake', - 'get_vm', 'pwr_vm', ] with fx.DriverTaskFlow() as taskflow_fix: @@ -1432,7 +1418,6 @@ class TestPowerVMDriver(test.TestCase): 'connect_vol_*', 'save_bdm_fake_vol2', 'fake', - 'get_vm', 'pwr_vm', ] with fx.DriverTaskFlow() as taskflow_fix: @@ -1515,9 +1500,7 @@ class TestPowerVMDriver(test.TestCase): force_immediate=False) self.assertTrue(mock_disk_dvr.create_disk_from_image.called) self.assertTrue(mock_disk_dvr.connect_disk.called) - mock_pwron.assert_called_once_with( - self.apt, self.inst, entry=self.get_inst_wrap.return_value, - opts=mock.ANY) + mock_pwron.assert_called_once_with(self.apt, self.inst, opts=mock.ANY) self.assertEqual('PowerOn(bootmode=sms)', str(mock_pwron.call_args[1]['opts'])) @@ -1533,9 +1516,7 @@ class TestPowerVMDriver(test.TestCase): self.apt, self.inst, force_immediate=False) self.assertTrue(mock_disk_dvr.disconnect_image_disk.called) self.assertTrue(mock_disk_dvr.delete_disks.called) - mock_pwron.assert_called_once_with( - self.apt, self.inst, entry=self.get_inst_wrap.return_value, - opts=None) + mock_pwron.assert_called_once_with(self.apt, self.inst, opts=None) @mock.patch('nova_powervm.virt.powervm.driver.LOG') def test_log_op(self, mock_log): diff --git a/nova_powervm/tests/virt/powervm/test_vm.py b/nova_powervm/tests/virt/powervm/test_vm.py index 1fc00c7e..1858695c 100644 --- a/nova_powervm/tests/virt/powervm/test_vm.py +++ b/nova_powervm/tests/virt/powervm/test_vm.py @@ -503,14 +503,19 @@ class TestVM(test.TestCase): self.assertDictEqual(attrs, {'env': 'OS400'}) @mock.patch('pypowervm.tasks.power.power_on') - def test_power_on(self, mock_power_on): + @mock.patch('oslo_concurrency.lockutils.lock') + @mock.patch('nova_powervm.virt.powervm.vm.get_instance_wrapper') + def test_power_on(self, mock_wrap, mock_lock, mock_power_on): instance = objects.Instance(**powervm.TEST_INSTANCE) entry = mock.Mock(state=pvm_bp.LPARState.NOT_ACTIVATED) + mock_wrap.return_value = entry - self.assertTrue(vm.power_on(None, instance, entry=entry, opts='opts')) + self.assertTrue(vm.power_on(None, instance, opts='opts')) mock_power_on.assert_called_once_with(entry, None, add_parms='opts') + mock_lock.assert_called_once_with('power_%s' % instance.uuid) mock_power_on.reset_mock() + mock_lock.reset_mock() stop_states = [ pvm_bp.LPARState.RUNNING, pvm_bp.LPARState.STARTING, @@ -519,18 +524,23 @@ class TestVM(test.TestCase): pvm_bp.LPARState.SUSPENDING] for stop_state in stop_states: - entry = mock.Mock(state=stop_state) - self.assertFalse(vm.power_on(None, instance, entry=entry)) + entry.state = stop_state + self.assertFalse(vm.power_on(None, instance)) mock_power_on.assert_not_called() + mock_lock.assert_called_once_with('power_%s' % instance.uuid) + mock_lock.reset_mock() @mock.patch('pypowervm.tasks.power.power_off') - def test_power_off(self, mock_power_off): + @mock.patch('oslo_concurrency.lockutils.lock') + @mock.patch('nova_powervm.virt.powervm.vm.get_instance_wrapper') + def test_power_off(self, mock_wrap, mock_lock, mock_power_off): instance = objects.Instance(**powervm.TEST_INSTANCE) + entry = mock.Mock(state=pvm_bp.LPARState.NOT_ACTIVATED) + mock_wrap.return_value = entry - self.assertFalse(vm.power_off( - None, instance, - entry=mock.Mock(state=pvm_bp.LPARState.NOT_ACTIVATED))) + self.assertFalse(vm.power_off(None, instance)) mock_power_off.assert_not_called() + mock_lock.assert_called_once_with('power_%s' % instance.uuid) stop_states = [ pvm_bp.LPARState.RUNNING, pvm_bp.LPARState.STARTING, @@ -538,35 +548,41 @@ class TestVM(test.TestCase): pvm_bp.LPARState.ERROR, pvm_bp.LPARState.RESUMING, pvm_bp.LPARState.SUSPENDING] for stop_state in stop_states: - entry = mock.Mock(state=stop_state) + entry.state = stop_state mock_power_off.reset_mock() - self.assertTrue(vm.power_off(None, instance, entry=entry)) + mock_lock.reset_mock() + self.assertTrue(vm.power_off(None, instance)) mock_power_off.assert_called_once_with( entry, None, force_immediate=power.Force.ON_FAILURE, add_parms=None) + mock_lock.assert_called_once_with('power_%s' % instance.uuid) mock_power_off.reset_mock() + mock_lock.reset_mock() self.assertTrue(vm.power_off( - None, instance, entry=entry, force_immediate=True, timeout=5)) + None, instance, force_immediate=True, timeout=5)) mock_power_off.assert_called_once_with( entry, None, force_immediate=True, add_parms=None, timeout=5) + mock_lock.assert_called_once_with('power_%s' % instance.uuid) @mock.patch('pypowervm.tasks.power.power_off') - def test_power_off_negative(self, mock_power_off): + @mock.patch('nova_powervm.virt.powervm.vm.get_instance_wrapper') + def test_power_off_negative(self, mock_wrap, mock_power_off): """Negative tests.""" instance = objects.Instance(**powervm.TEST_INSTANCE) + mock_wrap.return_value = mock.Mock(state=pvm_bp.LPARState.RUNNING) # Raise the expected pypowervm exception mock_power_off.side_effect = pvm_exc.VMPowerOffFailure( reason='Something bad.', lpar_nm='TheLPAR') # We should get a valid Nova exception that the compute manager expects self.assertRaises(exception.InstancePowerOffFailure, - vm.power_off, None, instance, - mock.Mock(state=pvm_bp.LPARState.RUNNING)) + vm.power_off, None, instance) + @mock.patch('oslo_concurrency.lockutils.lock') @mock.patch('nova_powervm.virt.powervm.vm.get_instance_wrapper') @mock.patch('pypowervm.tasks.power.power_on') @mock.patch('pypowervm.tasks.power.power_off') - def test_reboot(self, mock_pwroff, mock_pwron, mock_giw): + def test_reboot(self, mock_pwroff, mock_pwron, mock_giw, mock_lock): entry = mock.Mock() inst = mock.Mock(uuid='uuid') mock_giw.return_value = entry @@ -576,8 +592,10 @@ class TestVM(test.TestCase): vm.reboot('adapter', inst, True) mock_pwron.assert_called_once_with(entry, None) mock_pwroff.assert_not_called() + mock_lock.assert_called_once_with('power_uuid') mock_pwron.reset_mock() + mock_lock.reset_mock() # VM is in an active state entry.state = pvm_bp.LPARState.RUNNING @@ -587,8 +605,10 @@ class TestVM(test.TestCase): add_parms=mock.ANY) self.assertEqual('PowerOff(restart=true)', str(mock_pwroff.call_args[1]['add_parms'])) + mock_lock.assert_called_once_with('power_uuid') mock_pwroff.reset_mock() + mock_lock.reset_mock() # Same, but soft vm.reboot('adapter', inst, False) @@ -597,8 +617,10 @@ class TestVM(test.TestCase): add_parms=mock.ANY) self.assertEqual('PowerOff(restart=true)', str(mock_pwroff.call_args[1]['add_parms'])) + mock_lock.assert_called_once_with('power_uuid') mock_pwroff.reset_mock() + mock_lock.reset_mock() # Exception path mock_pwroff.side_effect = Exception() @@ -609,6 +631,7 @@ class TestVM(test.TestCase): add_parms=mock.ANY) self.assertEqual('PowerOff(restart=true)', str(mock_pwroff.call_args[1]['add_parms'])) + mock_lock.assert_called_once_with('power_uuid') @mock.patch('nova_powervm.virt.powervm.vm.get_pvm_uuid') @mock.patch('nova_powervm.virt.powervm.vm.get_vm_qp') diff --git a/nova_powervm/virt/powervm/driver.py b/nova_powervm/virt/powervm/driver.py index 29d99e47..304057df 100644 --- a/nova_powervm/virt/powervm/driver.py +++ b/nova_powervm/virt/powervm/driver.py @@ -878,9 +878,6 @@ class PowerVMDriver(driver.ComputeDriver): # Define the flow flow = tf_lf.Flow("rescue") - # Get the LPAR Wrapper - flow.add(tf_vm.Get(self.adapter, self.host_uuid, instance)) - # Power Off the LPAR flow.add(tf_vm.PowerOff(self.adapter, instance)) @@ -913,9 +910,6 @@ class PowerVMDriver(driver.ComputeDriver): # Define the flow flow = tf_lf.Flow("unrescue") - # Get the LPAR Wrapper - flow.add(tf_vm.Get(self.adapter, self.host_uuid, instance)) - # Power Off the LPAR flow.add(tf_vm.PowerOff(self.adapter, instance)) @@ -1327,8 +1321,6 @@ class PowerVMDriver(driver.ComputeDriver): flow.add(stg_ftsk) if power_on: - # Get the lpar wrapper (required by power-on), then power-on - flow.add(tf_vm.Get(self.adapter, self.host_uuid, instance)) flow.add(tf_vm.PowerOn(self.adapter, instance)) try: diff --git a/nova_powervm/virt/powervm/tasks/storage.py b/nova_powervm/virt/powervm/tasks/storage.py index 27d8a225..df5c219c 100644 --- a/nova_powervm/virt/powervm/tasks/storage.py +++ b/nova_powervm/virt/powervm/tasks/storage.py @@ -179,8 +179,6 @@ class ConnectDisk(pvm_task.PowerVMTask): def __init__(self, disk_dvr, context, instance, stg_ftsk=None): """Create the Task for the connect disk to instance method. - Requires LPAR info through requirement of lpar_wrap. - Requires disk info through requirement of disk_dev_info (provided by crt_disk_from_img) @@ -200,7 +198,7 @@ class ConnectDisk(pvm_task.PowerVMTask): self.context = context self.stg_ftsk = stg_ftsk - def execute_impl(self, disk_dev_info,): + def execute_impl(self, disk_dev_info): self.disk_dvr.connect_disk(self.context, self.instance, disk_dev_info, stg_ftsk=self.stg_ftsk) diff --git a/nova_powervm/virt/powervm/tasks/vm.py b/nova_powervm/virt/powervm/tasks/vm.py index 862f8990..497d98b1 100644 --- a/nova_powervm/virt/powervm/tasks/vm.py +++ b/nova_powervm/virt/powervm/tasks/vm.py @@ -197,25 +197,19 @@ class PowerOn(pvm_task.PowerVMTask): def __init__(self, adapter, instance, pwr_opts=None): """Create the Task for the power on of the LPAR. - Obtains LPAR info through requirement of lpar_wrap (provided by - tf_crt_vm). - :param adapter: The pypowervm adapter. - :param host_uuid: The host UUID. :param instance: The nova instance. :param pwr_opts: Additional parameters for the pypowervm PowerOn Job. """ - super(PowerOn, self).__init__( - instance, 'pwr_vm', requires=['lpar_wrap']) + super(PowerOn, self).__init__(instance, 'pwr_vm') self.adapter = adapter self.instance = instance self.pwr_opts = pwr_opts - def execute_impl(self, lpar_wrap): - vm.power_on(self.adapter, self.instance, entry=lpar_wrap, - opts=self.pwr_opts) + def execute_impl(self): + vm.power_on(self.adapter, self.instance, opts=self.pwr_opts) - def revert_impl(self, lpar_wrap, result, flow_failures): + def revert_impl(self, result, flow_failures): LOG.warning(_LW('Powering off instance: %s'), self.instance.name) if isinstance(result, task_fail.Failure): @@ -223,8 +217,7 @@ class PowerOn(pvm_task.PowerVMTask): LOG.debug('Power on failed. Not performing power off.') return - vm.power_off(self.adapter, self.instance, entry=lpar_wrap, - force_immediate=True) + vm.power_off(self.adapter, self.instance, force_immediate=True) class PowerOff(pvm_task.PowerVMTask): diff --git a/nova_powervm/virt/powervm/vm.py b/nova_powervm/virt/powervm/vm.py index 88a48395..3c60e105 100644 --- a/nova_powervm/virt/powervm/vm.py +++ b/nova_powervm/virt/powervm/vm.py @@ -14,6 +14,7 @@ # License for the specific language governing permissions and limitations # under the License. +from oslo_concurrency import lockutils from oslo_log import log as logging from oslo_serialization import jsonutils import re @@ -650,39 +651,36 @@ def dlt_lpar(adapter, lpar_uuid): raise -def power_on(adapter, instance, entry=None, opts=None): +def power_on(adapter, instance, opts=None): """Powers on a VM. :param adapter: A pypowervm.adapter.Adapter. :param instance: The nova instance to power on. - :param entry: (Optional) The pypowervm wrapper for the entry. If not - provided, will be looked up. :param opts: (Optional) Additional parameters to the pypowervm power_on method. See that method's docstring for details. :return: True if the instance was powered on. False if it was not in a startable state. :raises: InstancePowerOnFailure """ - if entry is None: + # Synchronize power-on and power-off ops on a given instance + with lockutils.lock('power_%s' % instance.uuid): entry = get_instance_wrapper(adapter, instance) - # Get the current state and see if we can start the VM - if entry.state in POWERVM_STARTABLE_STATE: - # Now start the lpar - power.power_on(entry, None, add_parms=opts) - return True + # Get the current state and see if we can start the VM + if entry.state in POWERVM_STARTABLE_STATE: + # Now start the lpar + power.power_on(entry, None, add_parms=opts) + return True return False -def power_off(adapter, instance, entry=None, opts=None, force_immediate=False, +def power_off(adapter, instance, opts=None, force_immediate=False, timeout=None): """Powers off a VM. :param adapter: A pypowervm.adapter.Adapter. :param instance: The nova instance to power off. - :param entry: (Optional) The pypowervm wrapper for the entry. If not - provided, will be looked up. :param opts: (Optional) Additional parameters to the pypowervm power_off method. See that method's docstring for details. :param force_immediate: (Optional, Default False) Should it be immediately @@ -694,32 +692,33 @@ def power_off(adapter, instance, entry=None, opts=None, force_immediate=False, stoppable state. :raises: InstancePowerOffFailure """ - if entry is None: + # Synchronize power-on and power-off ops on a given instance + with lockutils.lock('power_%s' % instance.uuid): entry = get_instance_wrapper(adapter, instance) - # Get the current state and see if we can stop the VM - LOG.debug("Powering off request for instance %(inst)s which is in " - "state %(state)s. Force Immediate Flag: %(force)s.", - {'inst': instance.name, 'state': entry.state, - 'force': force_immediate}) - if entry.state in POWERVM_STOPABLE_STATE: - # Now stop the lpar - try: - LOG.debug("Power off executing for instance %(inst)s.", + # Get the current state and see if we can stop the VM + LOG.debug("Powering off request for instance %(inst)s which is in " + "state %(state)s. Force Immediate Flag: %(force)s.", + {'inst': instance.name, 'state': entry.state, + 'force': force_immediate}) + if entry.state in POWERVM_STOPABLE_STATE: + # Now stop the lpar + try: + LOG.debug("Power off executing for instance %(inst)s.", + {'inst': instance.name}) + kwargs = {'timeout': timeout} if timeout else {} + force_flag = (power.Force.TRUE if force_immediate + else power.Force.ON_FAILURE) + power.power_off(entry, None, force_immediate=force_flag, + add_parms=opts, **kwargs) + except Exception as e: + LOG.exception(e) + raise exception.InstancePowerOffFailure( + reason=six.text_type(e)) + return True + else: + LOG.debug("Power off not required for instance %(inst)s.", {'inst': instance.name}) - kwargs = {'timeout': timeout} if timeout else {} - force_flag = (power.Force.TRUE if force_immediate - else power.Force.ON_FAILURE) - power.power_off(entry, None, force_immediate=force_flag, - add_parms=opts, **kwargs) - except Exception as e: - LOG.exception(e) - raise exception.InstancePowerOffFailure( - reason=six.text_type(e)) - return True - else: - LOG.debug("Power off not required for instance %(inst)s.", - {'inst': instance.name}) return False @@ -732,20 +731,22 @@ def reboot(adapter, instance, hard): :param hard: Boolean True if hard reboot, False otherwise. :raises: InstanceRebootFailure """ - try: - entry = get_instance_wrapper(adapter, instance) - if entry.state != pvm_bp.LPARState.NOT_ACTIVATED: - power.power_off(entry, None, force_immediate=hard, - add_parms=popts.PowerOffOpts().restart()) - else: - # pypowervm does NOT throw an exception if "already down". - # Any other exception from pypowervm is a legitimate failure; - # let it raise up. - # If we get here, pypowervm thinks the instance is down. - power.power_on(entry, None) - except Exception as e: - LOG.exception(e) - raise exception.InstanceRebootFailure(reason=six.text_type(e)) + # Synchronize power-on and power-off ops on a given instance + with lockutils.lock('power_%s' % instance.uuid): + try: + entry = get_instance_wrapper(adapter, instance) + if entry.state != pvm_bp.LPARState.NOT_ACTIVATED: + power.power_off(entry, None, force_immediate=hard, + add_parms=popts.PowerOffOpts().restart()) + else: + # pypowervm does NOT throw an exception if "already down". + # Any other exception from pypowervm is a legitimate failure; + # let it raise up. + # If we get here, pypowervm thinks the instance is down. + power.power_on(entry, None) + except Exception as e: + LOG.exception(e) + raise exception.InstanceRebootFailure(reason=six.text_type(e)) def get_pvm_uuid(instance):