Implement sync_power_state periodic task

Implement a periodic task in the ConductorManager which will check all
the existing, non-locked nodes and verify that their current power
states correspond to state stored in DB. If they differ, for now, log a
warning and update the DB using current power state.

This is a step towards keeping powered-off-nodes off.

Also, this removes the unused db.api.get_nodes method
and adds a new get_nodeinfo_list method.

Change-Id: I609fdbd7db6620de617b44f83c7ec35350f9178c
Blueprint: keep-powered-off-nodes-off

6 files changed, 159 insertions, 7 deletions

diff --git a/etc/ironic/ironic.conf.sample b/etc/ironic/ironic.conf.sample
index a7cba89ad..69701c336 100644
--- a/etc/ironic/ironic.conf.sample
+++ b/etc/ironic/ironic.conf.sample
@@ -431,6 +431,10 @@
 # value)
 #heartbeat_timeout=60
 
+# Interval between syncing the node power state to the
+# database, in seconds. (integer value)
+#sync_power_state_interval=60
+
 
 [database]
 
diff --git a/ironic/conductor/manager.py b/ironic/conductor/manager.py
index 01b0e34be..57dc952e1 100644
--- a/ironic/conductor/manager.py
+++ b/ironic/conductor/manager.py
@@ -65,6 +65,10 @@ conductor_opts = [
         cfg.IntOpt('heartbeat_timeout',
                    default=60,
                    help='Maximum time since the last check-in of a conductor'),
+        cfg.IntOpt('sync_power_state_interval',
+                   default=60,
+                   help='Interval between syncing the node power state to the '
+                        'database, in seconds.'),
 ]
 
 CONF = cfg.CONF
@@ -373,3 +377,33 @@ class ConductorManager(service.PeriodicService):
     @periodic_task.periodic_task(spacing=CONF.conductor.heartbeat_interval)
     def _conductor_service_record_keepalive(self, context):
         self.dbapi.touch_conductor(self.host)
+
+    @periodic_task.periodic_task(
+            spacing=CONF.conductor.sync_power_state_interval)
+    def _sync_power_states(self, context):
+        # TODO(deva): add filter by conductor<->instance mapping
+        filters = {'reserved': False}
+        node_list = self.dbapi.get_nodeinfo_list(filters=filters)
+        for node_id, in node_list:
+            try:
+                with task_manager.acquire(context, node_id) as task:
+                    node = task.node
+                    power_state = task.driver.power.get_power_state(task, node)
+                    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)
+
+            except (exception.NodeLocked, exception.NodeNotFound):
+                # NOTE(deva): if an instance is deleted during sync,
+                #             or locked by another process,
+                #             silently ignore it and continue
+                continue
diff --git a/ironic/db/api.py b/ironic/db/api.py
index 8a142cc9a..a35531e05 100644
--- a/ironic/db/api.py
+++ b/ironic/db/api.py
@@ -42,10 +42,23 @@ class Connection(object):
         """Constructor."""
 
     @abc.abstractmethod
-    def get_nodes(self, columns):
-        """Return a list of dicts of all nodes.
-
-        :param columns: List of columns to return.
+    def get_nodeinfo_list(self, columns=None, filters=None, limit=None,
+                          marker=None, sort_key=None, sort_dir=None):
+        """Return a list of the specified columns for all nodes that match
+        the specified filters.
+
+        :param columns: List of column names to return.
+                        Defaults to 'id' column when columns == None.
+        :param filters: Filters to apply. Defaults to None.
+                        'associated': True | False
+                        'reserved': True | False
+        :param limit: Maximum number of nodes to return.
+        :param marker: the last item of the previous page; we return the next
+                       result set.
+        :param sort_key: Attribute by which results should be sorted.
+        :param sort_dir: direction in which results should be sorted.
+                         (asc, desc)
+        :returns: A list of tuples of the specified columns.
         """
 
     @abc.abstractmethod
diff --git a/ironic/db/sqlalchemy/api.py b/ironic/db/sqlalchemy/api.py
index 36c108ba2..b8066cb84 100644
--- a/ironic/db/sqlalchemy/api.py
+++ b/ironic/db/sqlalchemy/api.py
@@ -178,9 +178,32 @@ class Connection(api.Connection):
     def __init__(self):
         pass
 
-    @objects.objectify(objects.Node)
-    def get_nodes(self, columns):
-        pass
+    def get_nodeinfo_list(self, columns=None, filters=None, limit=None,
+                          marker=None, sort_key=None, sort_dir=None):
+        # list-ify columns and filters default values because it is bad form
+        # to include a mutable list in function definitions.
+        if filters is None:
+            filters = []
+        if columns is None:
+            columns = [models.Node.id]
+        else:
+            columns = [getattr(models.Node, c) for c in columns]
+
+        query = model_query(*columns, base_model=models.Node)
+        if 'associated' in filters:
+            if filters['associated']:
+                query = query.filter(models.Node.instance_uuid != None)
+            else:
+                query = query.filter(models.Node.instance_uuid == None)
+        if 'reserved' in filters:
+            if filters['reserved']:
+                query = query.filter(models.Node.reservation != None)
+            else:
+                query = query.filter(models.Node.reservation == None)
+        if 'driver' in filters:
+            query = query.filter(models.Node.driver == filters['driver'])
+        return _paginate_query(models.Node, limit, marker,
+                               sort_key, sort_dir, query)
 
     @objects.objectify(objects.Node)
     def get_node_list(self, limit=None, marker=None,
diff --git a/ironic/tests/conductor/test_manager.py b/ironic/tests/conductor/test_manager.py
index 6ff6593f7..296417523 100644
--- a/ironic/tests/conductor/test_manager.py
+++ b/ironic/tests/conductor/test_manager.py
@@ -25,6 +25,7 @@ from oslo.config import cfg
 from ironic.common import driver_factory
 from ironic.common import exception
 from ironic.common import states
+from ironic.common import utils as ironic_utils
 from ironic.conductor import manager
 from ironic.conductor import task_manager
 from ironic.db import api as dbapi
@@ -78,6 +79,72 @@ class ManagerTestCase(base.DbTestCase):
             self.service._conductor_service_record_keepalive(self.context)
             mock_touch.assert_called_once_with('test-host')
 
+    def test__sync_power_state_no_sync(self):
+        self.service.start()
+        n = utils.get_test_node(driver='fake', power_state='fake-power')
+        self.dbapi.create_node(n)
+        with mock.patch.object(self.driver.power,
+                               'get_power_state') as get_power_mock:
+            get_power_mock.return_value = 'fake-power'
+            self.service._sync_power_states(self.context)
+            get_power_mock.assert_called_once_with(mock.ANY, mock.ANY)
+        node = self.dbapi.get_node(n['id'])
+        self.assertEqual(node['power_state'], 'fake-power')
+
+    def test__sync_power_state_do_sync(self):
+        self.service.start()
+        n = utils.get_test_node(driver='fake', power_state='fake-power')
+        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)
+        node = self.dbapi.get_node(n['id'])
+        self.assertEqual(node['power_state'], states.POWER_ON)
+
+    def test__sync_power_state_node_locked(self):
+        self.service.start()
+        n = utils.get_test_node(driver='fake', power_state='fake-power')
+        self.dbapi.create_node(n)
+        self.dbapi.reserve_nodes('fake-reserve', [n['id']])
+        with mock.patch.object(self.driver.power,
+                               'get_power_state') as get_power_mock:
+            self.service._sync_power_states(self.context)
+            self.assertFalse(get_power_mock.called)
+        node = self.dbapi.get_node(n['id'])
+        self.assertEqual('fake-power', node['power_state'])
+
+    def test__sync_power_state_multiple_nodes(self):
+        self.service.start()
+
+        # create three nodes
+        nodes = []
+        for i in range(0, 3):
+            n = utils.get_test_node(id=i, uuid=ironic_utils.generate_uuid(),
+                    driver='fake', power_state=states.POWER_OFF)
+            self.dbapi.create_node(n)
+            nodes.append(n['uuid'])
+
+        # lock the first node
+        self.dbapi.reserve_nodes('fake-reserve', [nodes[0]])
+
+        with mock.patch.object(self.driver.power,
+                               'get_power_state') as get_power_mock:
+            get_power_mock.return_value = states.POWER_ON
+            with mock.patch.object(self.dbapi,
+                                   'get_nodeinfo_list') as get_fnl_mock:
+                # delete the second node
+                self.dbapi.destroy_node(nodes[1])
+                get_fnl_mock.return_value = [[n] for n in nodes]
+                self.service._sync_power_states(self.context)
+            # check that get_power only called once, which updated third node
+            get_power_mock.assert_called_once_with(mock.ANY, mock.ANY)
+        n1 = self.dbapi.get_node(nodes[0])
+        n3 = self.dbapi.get_node(nodes[2])
+        self.assertEqual(n1['power_state'], states.POWER_OFF)
+        self.assertEqual(n3['power_state'], states.POWER_ON)
+
     def test_get_power_state(self):
         n = utils.get_test_node(driver='fake')
         self.dbapi.create_node(n)
diff --git a/ironic/tests/db/test_nodes.py b/ironic/tests/db/test_nodes.py
index b8e450a6e..450d13994 100644
--- a/ironic/tests/db/test_nodes.py
+++ b/ironic/tests/db/test_nodes.py
@@ -112,6 +112,17 @@ class DbNodeTestCase(base.DbTestCase):
         self.assertRaises(exception.InvalidIdentity,
                           self.dbapi.get_node, 'not-a-uuid')
 
+    def test_get_nodeinfo_list_defaults(self):
+        for i in range(1, 6):
+            n = utils.get_test_node(id=i, uuid=ironic_utils.generate_uuid())
+            self.dbapi.create_node(n)
+        res = [i[0] for i in self.dbapi.get_nodeinfo_list()]
+        self.assertEqual(sorted(res), sorted(xrange(1, 6)))
+
+    # TODO(deva): add more tests for get_nodeinfo_list
+    #             - check each filter works
+    #             - check variable column return works
+
     def test_get_node_list(self):
         uuids = []
         for i in range(1, 6):