From 8ad764a9de0a833d1e5828b5cd6846b7eb28b52a Mon Sep 17 00:00:00 2001 From: Michael Davies Date: Thu, 20 Feb 2014 17:08:58 +1030 Subject: [PATCH] Add option to sync node power state from DB Previously, during _sync_node_power_state, when a node's power state did not match the expected state, the DB was always updated and a warning logged. This patch causes the node's power state to instead be set to the expected value, by default, and adds a CONF option to enable the old behavior. Implements bp:keep-powered-off-nodes-off Co-Authored-By: Michael Davies Change-Id: I1f5af0061dbe8c57b8bd3f8a65b215aadd549551 --- etc/ironic/ironic.conf.sample | 6 ++ ironic/conductor/manager.py | 82 ++++++++++++++++++-------- ironic/tests/conductor/test_manager.py | 68 ++++++++++++++++++++- 3 files changed, 129 insertions(+), 27 deletions(-) diff --git a/etc/ironic/ironic.conf.sample b/etc/ironic/ironic.conf.sample index 0d012014c6..a23f2b99af 100644 --- a/etc/ironic/ironic.conf.sample +++ b/etc/ironic/ironic.conf.sample @@ -497,6 +497,12 @@ # 0 - unlimited. (integer value) #deploy_callback_timeout=1800 +# During sync_power_state, should the hardware power state be +# set to the state recorded in the database (True) or should +# the database be updated based on the hardware state (False). +# (boolean value) +#force_power_state_during_sync=true + [database] diff --git a/ironic/conductor/manager.py b/ironic/conductor/manager.py index 5b01a27af5..18f0e7fe5f 100644 --- a/ironic/conductor/manager.py +++ b/ironic/conductor/manager.py @@ -93,12 +93,68 @@ conductor_opts = [ default=1800, help='Timeout (seconds) for waiting callback from deploy ' 'ramdisk. 0 - unlimited.'), + cfg.BoolOpt('force_power_state_during_sync', + default=True, + help='During sync_power_state, should the hardware power ' + 'state be set to the state recorded in the database ' + '(True) or should the database be updated based on ' + 'the hardware state (False).'), ] CONF = cfg.CONF CONF.register_opts(conductor_opts, 'conductor') +def _do_sync_power_state(task): + node = task.node + + try: + power_state = task.driver.power.get_power_state(task, node) + except Exception as e: + # TODO(rloo): change to IronicException, after + # https://bugs.launchpad.net/ironic/+bug/1267693 + LOG.warning(_("During sync_power_state, could not get power state for " + "node %(node)s. Error: %(err)s."), + {'node': node.uuid, 'err': e}) + return + + if node.power_state is None: + LOG.info(_("During sync_power_state, node %(node)s has no previous " + "known state. Recording current state '%(state)s'."), + {'node': node.uuid, 'state': power_state}) + node.power_state = power_state + node.save(task.context) + + if power_state == node.power_state: + return + + if CONF.conductor.force_power_state_during_sync: + LOG.warning(_("During sync_power_state, node %(node)s state " + "'%(actual)s' does not match expected state. " + "Changing hardware state to '%(state)s'."), + {'node': node.uuid, 'actual': power_state, + 'state': node.power_state}) + try: + # node_power_action will update the node record + # so don't do that again here. + utils.node_power_action(task, task.node, + node.power_state) + except Exception as e: + # TODO(rloo): change to IronicException after + # https://bugs.launchpad.net/ironic/+bug/1267693 + LOG.error(_("Failed to change power state of node %(node)s " + "to '%(state)s'."), {'node': node.uuid, + 'state': node.power_state}) + else: + LOG.warning(_("During sync_power_state, node %(node)s state " + "does not match expected state '%(state)s'. " + "Updating recorded state to '%(actual)s'."), + {'node': node.uuid, 'actual': power_state, + 'state': node.power_state}) + node.power_state = power_state + node.save(task.context) + + class ConductorManager(service.PeriodicService): """Ironic Conductor service main class.""" @@ -395,31 +451,7 @@ class ConductorManager(service.PeriodicService): continue with task_manager.acquire(context, node_id) as task: - node = task.node - - try: - power_state = task.driver.power.get_power_state(task, - node) - except Exception as e: - #TODO(rloo): change to IronicException, after - # https://bugs.launchpad.net/ironic/+bug/1267693 - LOG.debug(_("During sync_power_state, could not get " - "power state for node %(node)s. Error: %(err)s.") % - {'node': node.uuid, 'err': e}) - continue - - if power_state != node['power_state']: - # NOTE(deva): don't log a warning the first time we - # sync a node's power state - if node['power_state'] is not None: - LOG.warning(_("During sync_power_state, node " - "%(node)s out of sync. Expected: %(old)s. " - "Actual: %(new)s. Updating DB.") % - {'node': node['uuid'], - 'old': node['power_state'], - 'new': power_state}) - node['power_state'] = power_state - node.save(context) + _do_sync_power_state(task) except exception.NodeNotFound: LOG.info(_("During sync_power_state, node %(node)s was not " diff --git a/ironic/tests/conductor/test_manager.py b/ironic/tests/conductor/test_manager.py index 78cc2db90a..0c68f4cab8 100644 --- a/ironic/tests/conductor/test_manager.py +++ b/ironic/tests/conductor/test_manager.py @@ -98,9 +98,9 @@ class ManagerTestCase(base.DbTestCase): node = self.dbapi.get_node(n['id']) self.assertEqual(node['power_state'], 'fake-power') - def test__sync_power_state_do_sync(self): + def test__sync_power_state_not_set(self): self.service.start() - n = utils.get_test_node(driver='fake', power_state='fake-power') + n = utils.get_test_node(driver='fake', power_state=None) self.dbapi.create_node(n) with mock.patch.object(self.driver.power, 'get_power_state') as get_power_mock: @@ -110,6 +110,68 @@ class ManagerTestCase(base.DbTestCase): node = self.dbapi.get_node(n['id']) self.assertEqual(node['power_state'], states.POWER_ON) + @mock.patch.object(objects.node.Node, 'save') + def test__sync_power_state_unchanged(self, save_mock): + self.service.start() + n = utils.get_test_node(driver='fake', power_state=states.POWER_ON) + self.dbapi.create_node(n) + with mock.patch.object(self.driver.power, + 'get_power_state') as get_power_mock: + get_power_mock.return_value = states.POWER_ON + self.service._sync_power_states(self.context) + get_power_mock.assert_called_once_with(mock.ANY, mock.ANY) + self.assertFalse(save_mock.called) + + @mock.patch.object(conductor_utils, 'node_power_action') + @mock.patch.object(objects.node.Node, 'save') + def test__sync_power_state_changed_sync(self, save_mock, npa_mock): + self.service.start() + self.config(force_power_state_during_sync=True, group='conductor') + n = utils.get_test_node(driver='fake', power_state=states.POWER_ON) + self.dbapi.create_node(n) + with mock.patch.object(self.driver.power, + 'get_power_state') as get_power_mock: + get_power_mock.return_value = states.POWER_OFF + self.service._sync_power_states(self.context) + get_power_mock.assert_called_once_with(mock.ANY, mock.ANY) + npa_mock.assert_called_once_with(mock.ANY, mock.ANY, + states.POWER_ON) + self.assertFalse(save_mock.called) + + @mock.patch.object(conductor_utils, 'node_power_action') + @mock.patch.object(objects.node.Node, 'save') + def test__sync_power_state_changed_failed(self, save_mock, npa_mock): + self.service.start() + self.config(force_power_state_during_sync=True, group='conductor') + n = utils.get_test_node(driver='fake', power_state=states.POWER_ON) + self.dbapi.create_node(n) + with mock.patch.object(self.driver.power, + 'get_power_state') as get_power_mock: + get_power_mock.return_value = states.POWER_OFF + npa_mock.side_effect = exception.IronicException('test') + # we are testing that this does NOT raise an exception + # so don't assertRaises here + self.service._sync_power_states(self.context) + get_power_mock.assert_called_once_with(mock.ANY, mock.ANY) + npa_mock.assert_called_once_with(mock.ANY, mock.ANY, + states.POWER_ON) + self.assertFalse(save_mock.called) + + @mock.patch.object(conductor_utils, 'node_power_action') + @mock.patch.object(objects.node.Node, 'save') + def test__sync_power_state_changed_save(self, save_mock, npa_mock): + self.service.start() + self.config(force_power_state_during_sync=False, group='conductor') + n = utils.get_test_node(driver='fake', power_state=states.POWER_OFF) + self.dbapi.create_node(n) + with mock.patch.object(self.driver.power, + 'get_power_state') as get_power_mock: + get_power_mock.return_value = states.POWER_ON + self.service._sync_power_states(self.context) + get_power_mock.assert_called_once_with(mock.ANY, mock.ANY) + self.assertFalse(npa_mock.called) + self.assertTrue(save_mock.called) + def test__sync_power_state_node_locked(self): self.service.start() n = utils.get_test_node(driver='fake', power_state='fake-power') @@ -124,6 +186,7 @@ class ManagerTestCase(base.DbTestCase): def test__sync_power_state_multiple_nodes(self): self.service.start() + self.config(force_power_state_during_sync=False, group='conductor') # create three nodes nodes = [] @@ -156,6 +219,7 @@ class ManagerTestCase(base.DbTestCase): def test__sync_power_state_node_no_power_state(self): self.service.start() + self.config(force_power_state_during_sync=False, group='conductor') # create three nodes nodes = []