Merge "Driver's validation during node update process implemented"

ironic/common/exception.py

@@ -229,6 +229,10 @@ class DiskNotFound(NotFound):
     message = _("No disk at %(location)s")
 
 
+class DriverNotFound(NotFound):
+    message = _("Failed to load driver %(driver_name)s.")
+
+
 class ImageNotFound(NotFound):
     message = _("Image %(image_id)s could not be found.")
 

ironic/conductor/manager.py

@@ -102,9 +102,10 @@ class ConductorManager(service.PeriodicService):
             raise exception.IronicException(_(
                 "Invalid method call: update_node can not change node state."))
 
-        with task_manager.acquire(node_id, shared=False) as task:
+        driver_name = node_obj.get('driver') if 'driver' in delta else None
-            # NOTE(deva): invalid driver parameters should not be saved
+        with task_manager.acquire(node_id,
-            #             don't proceed if these checks fail
+                                  shared=False,
+                                  driver_name=driver_name) as task:
             if 'driver_info' in delta:
                 task.driver.deploy.validate(node_obj)
                 task.driver.power.validate(node_obj)

ironic/conductor/resource_manager.py

@@ -55,7 +55,7 @@ class NodeManager(object):
     #             once, the first time NodeManager.__init__ is called.
     _driver_factory = None
 
-    def __init__(self, id, t):
+    def __init__(self, id, t, driver_name=None):
         if not NodeManager._driver_factory:
             NodeManager._init_driver_factory()
 
@@ -66,12 +66,9 @@ class NodeManager(object):
         self.node = db.get_node(id)
         self.ports = db.get_ports_by_node(id)
 
-        driver_name = self.node.get('driver')
+        # Select new driver's name if defined or select already defined in db.
-        try:
+        driver_name = driver_name or self.node.get('driver')
-            self.driver = NodeManager._driver_factory[driver_name].obj
+        self.driver = self.load_driver(driver_name)
-        except KeyError:
-            raise exception.IronicException(_(
-                "Failed to load driver %s.") % driver_name)
 
     # NOTE(deva): Use lockutils to avoid a potential race in eventlet
     #             that might try to create two driver factories.
@@ -89,13 +86,13 @@ class NodeManager(object):
 
     @classmethod
     @lockutils.synchronized(RESOURCE_MANAGER_SEMAPHORE, 'ironic-')
-    def acquire(cls, id, t):
+    def acquire(cls, id, t, new_driver=None):
         """Acquire a NodeManager and associate to a TaskManager."""
         n = cls._nodes.get(id)
         if n:
             n.task_refs.append(t)
         else:
-            n = cls(id, t)
+            n = cls(id, t, new_driver)
             cls._nodes[id] = n
         return n
 
@@ -120,3 +117,17 @@ class NodeManager(object):
         # Delete the resource when no TaskManager references it.
         if len(n.task_refs) == 0:
             del(cls._nodes[id])
+
+    def load_driver(self, driver_name):
+        """Find a driver based on driver_name and return a driver object.
+
+        :param driver_name: The name of the driver.
+        :returns: A driver object.
+        :raises: DriverNotFound if any driver is not found.
+        """
+        try:
+            driver = NodeManager._driver_factory[driver_name]
+        except KeyError:
+            raise exception.DriverNotFound(driver_name=driver_name)
+
+        return driver.obj

ironic/conductor/task_manager.py

@@ -95,7 +95,7 @@ def require_exclusive_lock(f):
 
 
 @contextlib.contextmanager
-def acquire(node_ids, shared=False):
+def acquire(node_ids, shared=False, driver_name=None):
     """Context manager for acquiring a lock on one or more Nodes.
 
     Acquire a lock atomically on a non-empty set of nodes. The lock
@@ -106,6 +106,7 @@ def acquire(node_ids, shared=False):
     :param node_ids: A list of ids or uuids of nodes to lock.
     :param shared: Boolean indicating whether to take a shared or exclusive
                    lock. Default: False.
+    :param driver_name: Name of Driver. Default: None.
     :returns: An instance of :class:`TaskManager`.
 
     """
@@ -120,7 +121,8 @@ def acquire(node_ids, shared=False):
         if not shared:
             t.dbapi.reserve_nodes(CONF.host, node_ids)
         for id in node_ids:
-            t.resources.append(resource_manager.NodeManager.acquire(id, t))
+            t.resources.append(resource_manager.NodeManager.acquire(
+                                                        id, t, driver_name))
         yield t
     finally:
         for id in [r.id for r in t.resources]:
@@ -154,3 +156,12 @@ class TaskManager(object):
         else:
             raise AttributeError(_("Multi-node TaskManager "
                                    "has no attribute 'driver'"))
+
+    @property
+    def node_manager(self):
+        """Special accessor for single-node manager."""
+        if len(self.resources) == 1:
+            return self.resources[0]
+        else:
+            raise AttributeError(_("Multi-node TaskManager "
+                "can't select single node manager from the list"))

ironic/tests/conductor/test_manager.py

@@ -17,7 +17,6 @@
 #    under the License.
 
 """Test class for Ironic ManagerService."""
-
 import mox
 
 from ironic.common import exception
@@ -95,9 +94,10 @@ class ManagerTestCase(base.DbTestCase):
         self.assertEqual(res['extra'], {'test': 'one'})
 
     def test_update_node_invalid_state(self):
-        ndict = utils.get_test_node(driver='fake', extra={'test': 'one'},
+        ndict = utils.get_test_node(driver='fake',
-                                instance_uuid=None,
+                                    extra={'test': 'one'},
-                                power_state=states.POWER_ON)
+                                    instance_uuid=None,
+                                    power_state=states.POWER_ON)
         node = self.dbapi.create_node(ndict)
 
         # check that it fails because state is POWER_ON
@@ -111,6 +111,26 @@ class ManagerTestCase(base.DbTestCase):
         res = objects.Node.get_by_uuid(self.context, node['uuid'])
         self.assertEqual(res['instance_uuid'], None)
 
+    def test_update_node_invalid_driver(self):
+        existing_driver = 'fake'
+        wrong_driver = 'wrong-driver'
+        ndict = utils.get_test_node(driver=existing_driver,
+                                    extra={'test': 'one'},
+                                    instance_uuid=None,
+                                    task_state=states.POWER_ON)
+        node = self.dbapi.create_node(ndict)
+        # check that it fails because driver not found
+        node['driver'] = wrong_driver
+        node['driver_info'] = {}
+        self.assertRaises(exception.DriverNotFound,
+                          self.service.update_node,
+                          self.context,
+                          node)
+
+        # verify change did not happen
+        res = objects.Node.get_by_uuid(self.context, node['uuid'])
+        self.assertEqual(res['driver'], existing_driver)
+
     def test_update_node_invalid_driver_info(self):
         # TODO(deva)
         pass

ironic/tests/conductor/test_node_manager.py

@@ -0,0 +1,152 @@
+# vim: tabstop=4 shiftwidth=4 softtabstop=4
+# coding=utf-8
+
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+import contextlib
+import mock
+
+from ironic.common import exception
+from ironic.conductor import resource_manager
+from ironic.tests import base
+
+
+class NodeManagerTestCase(base.TestCase):
+
+    def setUp(self):
+        super(NodeManagerTestCase, self).setUp()
+        self.existing_driver_name = 'some existing driver'
+        self.existing_driver_name_2 = 'some other existing driver'
+        self.non_existing_driver_name = "non existing driver"
+        self.existing_driver = mock.MagicMock()
+        self.existing_driver_2 = mock.MagicMock()
+        self.test_id = 1
+        self.test_task = mock.MagicMock()
+        self.test_node = {'driver': self.existing_driver_name, }
+        self.test_ports = mock.MagicMock()
+        self.driver_factory = {
+            self.existing_driver_name: self.existing_driver,
+            self.existing_driver_name_2: self.existing_driver_2, }
+        db_keys = {
+            "get_node.return_value": self.test_node,
+            "get_ports_by_node.return_value": self.test_ports, }
+        self.dbapi = mock.MagicMock()
+        self.dbapi.get_instance.return_value = mock.MagicMock(**db_keys)
+
+    def _fake_init(*args, **kwargs):
+        return None
+
+    def test_node_manager_init_id(self):
+        NodeManager = resource_manager.NodeManager
+        with contextlib.nested(
+                mock.patch.object(NodeManager,
+                                  '_driver_factory',
+                                  new=self.driver_factory),
+                mock.patch("ironic.conductor.resource_manager.dbapi",
+                           self.dbapi)):
+            new_nm = NodeManager(id=self.test_id, t=self.test_task)
+            self.assertEqual(new_nm.id, self.test_id)
+
+    def test_node_manager_init_task(self):
+        NodeManager = resource_manager.NodeManager
+        with contextlib.nested(
+                mock.patch.object(NodeManager,
+                                  '_driver_factory',
+                                  new=self.driver_factory),
+                mock.patch("ironic.conductor.resource_manager.dbapi",
+                           self.dbapi)
+                ):
+            new_nm = NodeManager(id=self.test_id, t=self.test_task)
+            self.assertEqual(new_nm.task_refs, [self.test_task])
+
+    def test_node_manager_init_node(self):
+        NodeManager = resource_manager.NodeManager
+        with contextlib.nested(
+                mock.patch.object(NodeManager,
+                                  '_driver_factory',
+                                  new=self.driver_factory),
+                mock.patch("ironic.conductor.resource_manager.dbapi",
+                           self.dbapi)
+                ):
+            new_nm = NodeManager(id=self.test_id, t=self.test_task)
+            self.assertEqual(new_nm.node, self.test_node)
+
+    def test_node_manager_init_ports(self):
+        NodeManager = resource_manager.NodeManager
+        with contextlib.nested(
+                mock.patch.object(NodeManager,
+                                  '_driver_factory',
+                                  new=self.driver_factory),
+                mock.patch("ironic.conductor.resource_manager.dbapi",
+                           self.dbapi)
+                ):
+            new_nm = NodeManager(id=self.test_id, t=self.test_task)
+            self.assertEqual(new_nm.ports, self.test_ports)
+
+    def test_node_manager_init_driver(self):
+        NodeManager = resource_manager.NodeManager
+        with contextlib.nested(
+                mock.patch.object(NodeManager,
+                                  '_driver_factory',
+                                  new=self.driver_factory),
+                mock.patch("ironic.conductor.resource_manager.dbapi",
+                           self.dbapi)
+                ):
+            new_nm = NodeManager(id=self.test_id, t=self.test_task)
+            self.assertEqual(new_nm.driver, self.existing_driver.obj)
+
+    def test_node_manager_init_new_driver(self):
+        NodeManager = resource_manager.NodeManager
+        with contextlib.nested(
+                mock.patch.object(NodeManager,
+                                  '_driver_factory',
+                                  new=self.driver_factory),
+                mock.patch("ironic.conductor.resource_manager.dbapi",
+                           self.dbapi)
+                ):
+            new_nm = NodeManager(id=self.test_id,
+                                 t=self.test_task,
+                                 driver_name=self.existing_driver_name_2)
+            self.assertEqual(new_nm.driver, self.existing_driver_2.obj)
+
+    def test_load_existing_driver(self):
+        NodeManager = resource_manager.NodeManager
+        with contextlib.nested(
+                mock.patch.object(NodeManager,
+                                  '_driver_factory',
+                                  new=self.driver_factory),
+                mock.patch.object(NodeManager,
+                                  '__init__',
+                                  new=self._fake_init)
+                ):
+            node_manager = NodeManager()
+            self.assertEqual(node_manager.load_driver(
+                                self.existing_driver_name
+                                ),
+                            self.existing_driver.obj
+                            )
+
+    def test_load_non_existing_driver(self):
+        NodeManager = resource_manager.NodeManager
+        with contextlib.nested(
+                mock.patch.object(NodeManager,
+                                  '_driver_factory',
+                                  new=self.driver_factory),
+                mock.patch.object(NodeManager,
+                                  '__init__',
+                                  new=self._fake_init)
+                ):
+            node_manager = NodeManager()
+            self.assertRaises(exception.DriverNotFound,
+                              node_manager.load_driver,
+                              self.non_existing_driver_name)

ironic/tests/conductor/test_task_manager.py

@@ -162,6 +162,7 @@ class ExclusiveLockDecoratorTestCase(base.DbTestCase):
         with task_manager.acquire(self.uuids) as task:
             self.assertEqual(task.node, task.resources[0].node)
             self.assertEqual(task.driver, task.resources[0].driver)
+            self.assertEqual(task.node_manager, task.resources[0])
 
     def test_one_node_per_task_properties_fail(self):
         self.uuids.append(create_fake_node(456))
@@ -172,5 +173,9 @@ class ExclusiveLockDecoratorTestCase(base.DbTestCase):
             def get_driver():
                 return task.driver
 
+            def get_node_manager():
+                return task.node_manager
+
             self.assertRaises(AttributeError, get_node)
             self.assertRaises(AttributeError, get_driver)
+            self.assertRaises(AttributeError, get_node_manager)