Merge "Fix cluster recovery and node recovery params"

api-ref/source/clusters.inc

@@ -1014,6 +1014,9 @@ include:
 - ``operation``: A string specifying the action to be performed for node
   recovery.
 
+- ``operation_params``: An optional dictionary specifying the key-value
+  arguments for the specific node recovery action.
+
 - ``check``: A boolean specifying whether the engine should check the actual
   statuses of cluster nodes before performing the recovery action. This
   parameter is added since microversion 1.6 and it defaults to False.

api-ref/source/nodes.inc

@@ -562,6 +562,9 @@ include:
 - ``operation``: A string specifying the action to be performed for node
   recovery.
 
+- ``operation_params``: An optional dictionary specifying the key-value
+  arguments for the specific node recovery action.
+
 - ``check``: A boolean specifying whether the engine should check the node's
   actual status before performing the recovery action. This parameter is added
   since microversion 1.6.

api-ref/source/samples/cluster-recover-request.json

@@ -1,6 +1,9 @@
 {
     "recover": {
-        "operation": "rebuild",
+        "operation": "reboot",
+        "operation_params": {
+            "type": "soft"
+        },
         "check": false
     }
 }

api-ref/source/samples/node-recover-request.json

@@ -1,6 +1,9 @@
 {
     "recover": {
-        "operation": "rebuild",
+        "operation": "reboot",
+        "operation_params": {
+            "type": "soft"
+        },
         "check": false
     }
 }

senlin/common/consts.py

@@ -344,3 +344,11 @@ CONFLICT_BYPASS_ACTIONS = [
 LOCK_BYPASS_ACTIONS = [
     CLUSTER_DELETE, NODE_DELETE, NODE_OPERATION,
 ]
+
+REBOOT_TYPE = 'type'
+
+REBOOT_TYPES = (
+    REBOOT_SOFT, REBOOT_HARD
+) = (
+    'SOFT', 'HARD'
+)

senlin/engine/actions/cluster_action.py

@@ -833,22 +833,11 @@ class ClusterAction(base.Action):
         """
         self.entity.do_recover(self.context)
 
-        # process data from health_policy
-        pd = self.data.get('health', None)
         inputs = {}
-        if pd:
+
-            check = self.data.get('check', False)
+        check = self.inputs.get('check', False)
-            recover_action = pd.get('recover_action', None)
+        inputs['operation'] = self.inputs.get('operation', None)
-            fencing = pd.get('fencing', None)
+        inputs['operation_params'] = self.inputs.get('operation_params', None)
-            if recover_action is not None:
-                inputs['operation'] = recover_action
-            if fencing is not None and 'COMPUTE' in fencing:
-                inputs['params'] = {'fence_compute': True}
-        else:
-            check = self.inputs.get('check', False)
-            recover_action = self.inputs.get('operation', None)
-            if recover_action is not None:
-                inputs['operation'] = recover_action
 
         children = []
         for node in self.entity.nodes:

senlin/engine/node.py

@@ -368,12 +368,11 @@ class Node(object):
         """
         options = action.inputs
 
-        operations = options.get('operation', [{'name': ''}])
+        operation = options.get('operation', None)
-        reboot_ops = [op for op in operations
+
-                      if op.get('name') == consts.RECOVER_REBOOT]
+        if (not self.physical_id and operation and
-        rebuild_ops = [op for op in operations
+                (operation.upper() == consts.RECOVER_REBOOT or
-                       if op.get('name') == consts.RECOVER_REBUILD]
+                 operation.upper() == consts.RECOVER_REBUILD)):
-        if not self.physical_id and (reboot_ops or rebuild_ops):
             # physical id is required for REBOOT or REBUILD operations
             LOG.warning('Recovery failed because node has no physical id'
                         ' was provided for reboot or rebuild operation.')

senlin/engine/service.py

@@ -1423,6 +1423,43 @@ class EngineService(service.Service):
 
         return {'action': action_id}
 
+    def _get_operation_params(self, params):
+        inputs = {}
+
+        if 'operation' in params:
+            op_name = params.pop('operation')
+            if not isinstance(op_name, six.string_types):
+                raise exception.BadRequest(
+                    msg="operation has to be a string")
+            if op_name.upper() not in consts.RECOVERY_ACTIONS:
+                msg = ("Operation value '{}' has to be one of the "
+                       "following: {}."
+                       ).format(op_name,
+                                ', '.join(consts.RECOVERY_ACTIONS))
+                raise exception.BadRequest(msg=msg)
+            inputs['operation'] = op_name
+
+            if 'operation_params' in params:
+                op_params = params.pop('operation_params')
+
+                if (op_name.upper() == consts.RECOVER_REBOOT):
+                    if not isinstance(op_params, dict):
+                        raise exception.BadRequest(
+                            msg="operation_params must be a map")
+
+                    if (consts.REBOOT_TYPE in op_params.keys() and
+                            op_params[consts.REBOOT_TYPE].upper()
+                            not in consts.REBOOT_TYPES):
+                        msg = ("Type field '{}' in operation_params has to be "
+                               "one of the following: {}.").format(
+                            op_params[consts.REBOOT_TYPE],
+                            ', '.join(consts.REBOOT_TYPES))
+                        raise exception.BadRequest(msg=msg)
+
+                inputs['operation_params'] = op_params
+
+        return inputs
+
     @request_context
     def cluster_recover(self, ctx, req):
         """Recover a cluster to a healthy status.
@@ -1441,8 +1478,7 @@ class EngineService(service.Service):
 
         inputs = {}
         if req.obj_attr_is_set('params') and req.params:
-            if 'operation' in req.params:
+            inputs = self._get_operation_params(req.params)
-                inputs['operation'] = req.params.pop('operation')
 
             if 'check' in req.params:
                 inputs['check'] = req.params.pop('check')
@@ -1455,9 +1491,6 @@ class EngineService(service.Service):
                 msg = _("Action parameter %s is not recognizable.") % keys
                 raise exception.BadRequest(msg=msg)
 
-        # TODO(anyone): should check if the 'params' attribute, if set,
-        # contains valid fields. This can be done by modeling the 'params'
-        # attribute into a separate object.
         params = {
             'name': 'cluster_recover_%s' % db_cluster.id[:8],
             'cause': consts.CAUSE_RPC,
@@ -1953,13 +1986,11 @@ class EngineService(service.Service):
             'inputs': {}
         }
         if req.obj_attr_is_set('params') and req.params:
+            kwargs['inputs'] = self._get_operation_params(req.params)
+
             if 'check' in req.params:
                 kwargs['inputs']['check'] = req.params.pop('check')
 
-            if 'operation' in req.params:
-                op_name = req.params.pop('operation')
-                kwargs['inputs']['operation'] = [{'name': op_name}]
-
             if 'delete_timeout' in req.params:
                 kwargs['inputs']['delete_timeout'] = req.params.pop(
                     'delete_timeout')

senlin/profiles/base.py

@@ -522,20 +522,19 @@ class Profile(object):
         :return status: True indicates successful recovery, False indicates
             failure.
         """
-        operation = options.pop('operation', None)
+        operation = options.get('operation', None)
-        force_recreate = options.pop('force_recreate', None)
+        force_recreate = options.get('force_recreate', None)
-        delete_timeout = options.pop('delete_timeout', None)
+        delete_timeout = options.get('delete_timeout', None)
 
-        # The operation is a list of action names with optional parameters
+        if operation.upper() != consts.RECOVER_RECREATE:
-        if operation and not isinstance(operation, six.string_types):
-            operation = operation[0]
-
-        if operation and operation['name'] != consts.RECOVER_RECREATE:
             LOG.error("Recover operation not supported: %s", operation)
             return None, False
 
-        extra_params = options.get('params', {})
+        extra_params = options.get('operation_params', None)
-        fence_compute = extra_params.get('fence_compute', False)
+        fence_compute = False
+        if extra_params:
+            fence_compute = extra_params.get('fence_compute', False)
+
         try:
             self.do_delete(obj, force=fence_compute, timeout=delete_timeout)
         except exc.EResourceDeletion as ex:

senlin/profiles/os/nova/server.py

@@ -248,8 +248,6 @@ class ServerProfile(base.Profile):
         'rescue', 'unrescue', 'evacuate', 'migrate',
     )
 
-    REBOOT_TYPE = 'type'
-    REBOOT_TYPES = (REBOOT_SOFT, REBOOT_HARD) = ('SOFT', 'HARD')
     ADMIN_PASSWORD = 'admin_pass'
     RESCUE_IMAGE = 'image_ref'
     EVACUATE_OPTIONS = (
@@ -262,11 +260,11 @@ class ServerProfile(base.Profile):
         OP_REBOOT: schema.Operation(
             _("Reboot the nova server."),
             schema={
-                REBOOT_TYPE: schema.StringParam(
+                consts.REBOOT_TYPE: schema.StringParam(
                     _("Type of reboot which can be 'SOFT' or 'HARD'."),
-                    default=REBOOT_SOFT,
+                    default=consts.REBOOT_SOFT,
                     constraints=[
-                        constraints.AllowedValues(REBOOT_TYPES),
+                        constraints.AllowedValues(consts.REBOOT_TYPES),
                     ]
                 )
             }
@@ -1604,29 +1602,32 @@ class ServerProfile(base.Profile):
         :return status: True indicates successful recovery, False indicates
             failure.
         """
-        operation = options.get('operation', None)
 
-        if operation and not isinstance(operation, six.string_types):
+        # default is recreate if not specified
-            operation = operation[0]
+        if 'operation' not in options or not options['operation']:
+            options['operation'] = consts.RECOVER_RECREATE
 
-        if operation is not None and 'name' in operation:
+        operation = options.get('operation')
-            op_name = operation['name']
-            if op_name.upper() != consts.RECOVER_RECREATE:
-                op_params = operation.get('params', {})
-                # nova recover operation always use hard reboot
-                # vm in error or stop status soft reboot can't succeed
-                if op_name.upper() == consts.RECOVER_REBOOT:
-                    if self.REBOOT_TYPE not in op_params:
-                        op_params[self.REBOOT_TYPE] = self.REBOOT_HARD
-                if op_name.lower() not in self.OP_NAMES:
-                    LOG.error("The operation '%s' is not supported",
-                              op_name)
-                    return obj.physical_id, False
 
-                method = getattr(self, "handle_" + op_name.lower())
+        if operation.upper() not in consts.RECOVERY_ACTIONS:
-                return method(obj, **op_params)
+            LOG.error("The operation '%s' is not supported",
+                      operation)
+            return obj.physical_id, False
 
-        return super(ServerProfile, self).do_recover(obj, **options)
+        op_params = options.get('operation_params', {})
+        if operation.upper() == consts.RECOVER_REBOOT:
+            # default to hard reboot if operation_params was not specified
+            if not isinstance(op_params, dict):
+                op_params = {}
+            if consts.REBOOT_TYPE not in op_params.keys():
+                op_params[consts.REBOOT_TYPE] = consts.REBOOT_HARD
+
+        if operation.upper() == consts.RECOVER_RECREATE:
+            # recreate is implemented in base class
+            return super(ServerProfile, self).do_recover(obj, **options)
+        else:
+            method = getattr(self, "handle_" + operation.lower())
+            return method(obj, **op_params)
 
     def handle_reboot(self, obj, **options):
         """Handler for the reboot operation."""
@@ -1634,9 +1635,9 @@ class ServerProfile(base.Profile):
             return None, False
 
         server_id = obj.physical_id
-        reboot_type = options.get(self.REBOOT_TYPE, self.REBOOT_SOFT)
+        reboot_type = options.get(consts.REBOOT_TYPE, consts.REBOOT_SOFT)
         if (not isinstance(reboot_type, six.string_types) or
-                reboot_type not in self.REBOOT_TYPES):
+                reboot_type.upper() not in consts.REBOOT_TYPES):
             return server_id, False
 
         nova_driver = self.compute(obj)

senlin/tests/unit/engine/actions/test_recover.py

@@ -68,60 +68,7 @@ class ClusterRecoverTest(base.SenlinTestCase):
             action.context, 'NODE_2', 'NODE_RECOVER',
             name='node_recover_NODE_2',
             cause=consts.CAUSE_DERIVED,
-            inputs={}
+            inputs={'operation': None, 'operation_params': None}
-        )
-        mock_dep.assert_called_once_with(action.context, ['NODE_RECOVER_ID'],
-                                         'CLUSTER_ACTION_ID')
-        mock_update.assert_called_once_with(action.context, 'NODE_RECOVER_ID',
-                                            {'status': 'READY'})
-        mock_start.assert_called_once_with()
-        mock_wait.assert_called_once_with()
-        cluster.eval_status.assert_called_once_with(
-            action.context, consts.CLUSTER_RECOVER)
-
-    @mock.patch.object(ao.Action, 'update')
-    @mock.patch.object(ab.Action, 'create')
-    @mock.patch.object(dobj.Dependency, 'create')
-    @mock.patch.object(dispatcher, 'start_action')
-    @mock.patch.object(ca.ClusterAction, '_wait_for_dependents')
-    def test_do_recover_with_data(self, mock_wait, mock_start,
-                                  mock_dep, mock_action, mock_update,
-                                  mock_load):
-        node1 = mock.Mock(id='NODE_1', cluster_id='FAKE_ID', status='ERROR')
-        cluster = mock.Mock(id='FAKE_ID', RECOVERING='RECOVERING',
-                            desired_capacity=2)
-        cluster.nodes = [node1]
-        cluster.do_recover.return_value = True
-        mock_load.return_value = cluster
-
-        action = ca.ClusterAction(cluster.id, 'CLUSTER_RECOVER', self.ctx)
-        action.id = 'CLUSTER_ACTION_ID'
-        action.data = {
-            'health': {
-                'recover_action': [{'name': 'REBOOT', 'params': None}],
-                'fencing': ['COMPUTE'],
-            }
-        }
-
-        mock_action.return_value = 'NODE_RECOVER_ID'
-        mock_wait.return_value = (action.RES_OK, 'Everything is Okay')
-
-        # do it
-        res_code, res_msg = action.do_recover()
-
-        # assertions
-        self.assertEqual(action.RES_OK, res_code)
-        self.assertEqual('Cluster recovery succeeded.', res_msg)
-
-        cluster.do_recover.assert_called_once_with(action.context)
-        mock_action.assert_called_once_with(
-            action.context, 'NODE_1', 'NODE_RECOVER',
-            name='node_recover_NODE_1',
-            cause=consts.CAUSE_DERIVED,
-            inputs={
-                'operation': [{'name': 'REBOOT', 'params': None}],
-                'params': {'fence_compute': True}
-            }
         )
         mock_dep.assert_called_once_with(action.context, ['NODE_RECOVER_ID'],
                                          'CLUSTER_ACTION_ID')
@@ -172,7 +119,8 @@ class ClusterRecoverTest(base.SenlinTestCase):
             name='node_recover_NODE_1',
             cause=consts.CAUSE_DERIVED,
             inputs={
-                'operation': consts.RECOVER_REBOOT
+                'operation': consts.RECOVER_REBOOT,
+                'operation_params': None
             }
         )
         mock_dep.assert_called_once_with(action.context, ['NODE_RECOVER_ID'],
@@ -221,8 +169,13 @@ class ClusterRecoverTest(base.SenlinTestCase):
         mock_load.return_value = cluster
         mock_action.return_value = 'NODE_ACTION_ID'
 
-        action = ca.ClusterAction('FAKE_CLUSTER', 'CLUSTER_REOVER', self.ctx)
+        action = ca.ClusterAction('FAKE_CLUSTER', 'CLUSTER_RECOVER', self.ctx)
         action.id = 'CLUSTER_ACTION_ID'
+        action.inputs = {
+            'operation': consts.RECOVER_RECREATE,
+            'check': False,
+            'check_capacity': False
+        }
 
         mock_wait.return_value = (action.RES_TIMEOUT, 'Timeout!')
 
@@ -237,7 +190,10 @@ class ClusterRecoverTest(base.SenlinTestCase):
             action.context, 'NODE_1', 'NODE_RECOVER',
             name='node_recover_NODE_1',
             cause=consts.CAUSE_DERIVED,
-            inputs={}
+            inputs={
+                'operation': consts.RECOVER_RECREATE,
+                'operation_params': None
+            }
         )
         mock_dep.assert_called_once_with(action.context, ['NODE_ACTION_ID'],
                                          'CLUSTER_ACTION_ID')
@@ -346,7 +302,8 @@ class ClusterRecoverTest(base.SenlinTestCase):
             action.context, 'NODE_2', 'NODE_RECOVER',
             name='node_recover_NODE_2',
             cause=consts.CAUSE_DERIVED,
-            inputs={}
+            inputs={'operation': None,
+                    'operation_params': None}
         )
         node_calls = [
             mock.call(self.ctx, node_id='NODE_1'),

senlin/tests/unit/engine/service/test_clusters.py

@@ -1512,6 +1512,74 @@ class ClusterTest(base.SenlinTestCase):
         )
         notify.assert_called_once_with()
 
+    @mock.patch.object(am.Action, 'create')
+    @mock.patch.object(co.Cluster, 'find')
+    @mock.patch.object(dispatcher, 'start_action')
+    def test_cluster_recover_rebuild(self, notify, mock_find, mock_action):
+        x_cluster = mock.Mock(id='CID')
+        mock_find.return_value = x_cluster
+        mock_action.return_value = 'ACTION_ID'
+        req = orco.ClusterRecoverRequest(identity='C1',
+                                         params={'operation': 'REBUILD'})
+
+        result = self.eng.cluster_recover(self.ctx, req.obj_to_primitive())
+
+        self.assertEqual({'action': 'ACTION_ID'}, result)
+        mock_find.assert_called_once_with(self.ctx, 'C1')
+        mock_action.assert_called_once_with(
+            self.ctx, 'CID', consts.CLUSTER_RECOVER,
+            name='cluster_recover_CID',
+            cause=consts.CAUSE_RPC,
+            status=am.Action.READY,
+            inputs={'operation': 'REBUILD'},
+        )
+        notify.assert_called_once_with()
+
+    @mock.patch.object(am.Action, 'create')
+    @mock.patch.object(co.Cluster, 'find')
+    @mock.patch.object(dispatcher, 'start_action')
+    def test_cluster_recover_reboot(self, notify, mock_find, mock_action):
+        x_cluster = mock.Mock(id='CID')
+        mock_find.return_value = x_cluster
+        mock_action.return_value = 'ACTION_ID'
+        req = orco.ClusterRecoverRequest(identity='C1',
+                                         params={'operation': 'REBOOT'})
+
+        result = self.eng.cluster_recover(self.ctx, req.obj_to_primitive())
+
+        self.assertEqual({'action': 'ACTION_ID'}, result)
+        mock_find.assert_called_once_with(self.ctx, 'C1')
+        mock_action.assert_called_once_with(
+            self.ctx, 'CID', consts.CLUSTER_RECOVER,
+            name='cluster_recover_CID',
+            cause=consts.CAUSE_RPC,
+            status=am.Action.READY,
+            inputs={'operation': 'REBOOT'},
+        )
+        notify.assert_called_once_with()
+
+    @mock.patch.object(am.Action, 'create')
+    @mock.patch.object(co.Cluster, 'find')
+    @mock.patch.object(dispatcher, 'start_action')
+    def test_cluster_recover_default(self, notify, mock_find, mock_action):
+        x_cluster = mock.Mock(id='CID')
+        mock_find.return_value = x_cluster
+        mock_action.return_value = 'ACTION_ID'
+        req = orco.ClusterRecoverRequest(identity='C1')
+
+        result = self.eng.cluster_recover(self.ctx, req.obj_to_primitive())
+
+        self.assertEqual({'action': 'ACTION_ID'}, result)
+        mock_find.assert_called_once_with(self.ctx, 'C1')
+        mock_action.assert_called_once_with(
+            self.ctx, 'CID', consts.CLUSTER_RECOVER,
+            name='cluster_recover_CID',
+            cause=consts.CAUSE_RPC,
+            status=am.Action.READY,
+            inputs={}
+        )
+        notify.assert_called_once_with()
+
     @mock.patch.object(co.Cluster, 'find')
     def test_cluster_recover_cluster_not_found(self, mock_find):
         mock_find.side_effect = exc.ResourceNotFound(type='cluster',
@@ -1544,6 +1612,46 @@ class ClusterTest(base.SenlinTestCase):
                          six.text_type(ex.exc_info[1]))
         mock_find.assert_called_once_with(self.ctx, 'Bogus')
 
+    @mock.patch.object(co.Cluster, 'find')
+    def test_cluster_recover_invalid_operation(self, mock_find):
+        x_cluster = mock.Mock(id='CID')
+        mock_find.return_value = x_cluster
+
+        req = orco.ClusterRecoverRequest(identity='Bogus',
+                                         params={'operation': 'fake'})
+
+        ex = self.assertRaises(rpc.ExpectedException,
+                               self.eng.cluster_recover,
+                               self.ctx, req.obj_to_primitive())
+
+        self.assertEqual(exc.BadRequest, ex.exc_info[0])
+        self.assertEqual("Operation value 'fake' has to be one of the "
+                         "following: REBOOT, REBUILD, RECREATE.",
+                         six.text_type(ex.exc_info[1]))
+        mock_find.assert_called_once_with(self.ctx, 'Bogus')
+
+    @mock.patch.object(co.Cluster, 'find')
+    def test_cluster_recover_invalid_operation_params(self, mock_find):
+        x_cluster = mock.Mock(id='CID')
+        mock_find.return_value = x_cluster
+
+        req = orco.ClusterRecoverRequest(
+            identity='Bogus',
+            params={'operation': 'reboot',
+                    'operation_params': {'type': 'blah'}
+                    }
+        )
+
+        ex = self.assertRaises(rpc.ExpectedException,
+                               self.eng.cluster_recover,
+                               self.ctx, req.obj_to_primitive())
+
+        self.assertEqual(exc.BadRequest, ex.exc_info[0])
+        self.assertEqual("Type field 'blah' in operation_params has to be one "
+                         "of the following: SOFT, HARD.",
+                         six.text_type(ex.exc_info[1]))
+        mock_find.assert_called_once_with(self.ctx, 'Bogus')
+
     @mock.patch.object(am.Action, 'create')
     @mock.patch.object(co.Cluster, 'find')
     @mock.patch.object(dispatcher, 'start_action')

senlin/tests/unit/engine/service/test_nodes.py

@@ -934,7 +934,7 @@ class NodeTest(base.SenlinTestCase):
         mock_find.return_value = mock.Mock(id='12345678AB')
         mock_action.return_value = 'ACTION_ID'
 
-        params = {'operation': 'some_action'}
+        params = {'operation': 'REBOOT'}
         req = orno.NodeRecoverRequest(identity='FAKE_NODE', params=params)
         result = self.eng.node_recover(self.ctx, req.obj_to_primitive())
 
@@ -945,7 +945,7 @@ class NodeTest(base.SenlinTestCase):
             name='node_recover_12345678',
             cause=consts.CAUSE_RPC,
             status=action_mod.Action.READY,
-            inputs={'operation': [{'name': 'some_action'}]})
+            inputs={'operation': 'REBOOT'})
         mock_start.assert_called_once_with()
 
     @mock.patch.object(dispatcher, 'start_action')
@@ -955,7 +955,7 @@ class NodeTest(base.SenlinTestCase):
         mock_find.return_value = mock.Mock(id='12345678AB')
         mock_action.return_value = 'ACTION_ID'
 
-        params = {'check': True, 'operation': 'some_action'}
+        params = {'check': True, 'operation': 'REBUILD'}
         req = orno.NodeRecoverRequest(identity='FAKE_NODE', params=params)
         result = self.eng.node_recover(self.ctx, req.obj_to_primitive())
 
@@ -966,7 +966,7 @@ class NodeTest(base.SenlinTestCase):
             name='node_recover_12345678',
             cause=consts.CAUSE_RPC,
             status=action_mod.Action.READY,
-            inputs={'check': True, 'operation': [{'name': 'some_action'}]})
+            inputs={'check': True, 'operation': 'REBUILD'})
         mock_start.assert_called_once_with()
 
     @mock.patch.object(dispatcher, 'start_action')
@@ -977,7 +977,7 @@ class NodeTest(base.SenlinTestCase):
         mock_find.return_value = mock.Mock(id='12345678AB')
         mock_action.return_value = 'ACTION_ID'
 
-        params = {'delete_timeout': 20, 'operation': 'some_action'}
+        params = {'delete_timeout': 20, 'operation': 'RECREATE'}
         req = orno.NodeRecoverRequest(identity='FAKE_NODE', params=params)
         result = self.eng.node_recover(self.ctx, req.obj_to_primitive())
 
@@ -989,7 +989,7 @@ class NodeTest(base.SenlinTestCase):
             cause=consts.CAUSE_RPC,
             status=action_mod.Action.READY,
             inputs={'delete_timeout': 20,
-                    'operation': [{'name': 'some_action'}]})
+                    'operation': 'RECREATE'})
         mock_start.assert_called_once_with()
 
     @mock.patch.object(dispatcher, 'start_action')
@@ -1000,7 +1000,8 @@ class NodeTest(base.SenlinTestCase):
         mock_find.return_value = mock.Mock(id='12345678AB')
         mock_action.return_value = 'ACTION_ID'
 
-        params = {'force_recreate': True, 'operation': 'some_action'}
+        params = {'force_recreate': True, 'operation': 'reboot',
+                  'operation_params': {'type': 'soft'}}
         req = orno.NodeRecoverRequest(identity='FAKE_NODE', params=params)
         result = self.eng.node_recover(self.ctx, req.obj_to_primitive())
 
@@ -1012,7 +1013,8 @@ class NodeTest(base.SenlinTestCase):
             cause=consts.CAUSE_RPC,
             status=action_mod.Action.READY,
             inputs={'force_recreate': True,
-                    'operation': [{'name': 'some_action'}]})
+                    'operation': 'reboot',
+                    'operation_params': {'type': 'soft'}})
         mock_start.assert_called_once_with()
 
     @mock.patch.object(no.Node, 'find')
@@ -1031,7 +1033,7 @@ class NodeTest(base.SenlinTestCase):
 
     @mock.patch.object(action_mod.Action, 'create')
     @mock.patch.object(no.Node, 'find')
-    def test_node_recover_invalid_operation(self, mock_find, mock_action):
+    def test_node_recover_unknown_operation(self, mock_find, mock_action):
         mock_find.return_value = mock.Mock(id='12345678AB')
         mock_action.return_value = 'ACTION_ID'
         params = {'bogus': 'illegal'}
@@ -1047,6 +1049,47 @@ class NodeTest(base.SenlinTestCase):
         mock_find.assert_called_once_with(self.ctx, 'FAKE_NODE')
         self.assertEqual(0, mock_action.call_count)
 
+    @mock.patch.object(action_mod.Action, 'create')
+    @mock.patch.object(no.Node, 'find')
+    def test_node_recover_invalid_operation(self, mock_find, mock_action):
+        mock_find.return_value = mock.Mock(id='12345678AB')
+        mock_action.return_value = 'ACTION_ID'
+        params = {'force_recreate': True, 'operation': 'blah',
+                  'operation_params': {'type': 'soft'}}
+        req = orno.NodeRecoverRequest(identity='FAKE_NODE', params=params)
+
+        ex = self.assertRaises(rpc.ExpectedException,
+                               self.eng.node_recover,
+                               self.ctx, req.obj_to_primitive())
+
+        self.assertEqual(exc.BadRequest, ex.exc_info[0])
+        self.assertEqual("Operation value 'blah' has to be one of the "
+                         "following: REBOOT, REBUILD, RECREATE.",
+                         six.text_type(ex.exc_info[1]))
+        mock_find.assert_called_once_with(self.ctx, 'FAKE_NODE')
+        self.assertEqual(0, mock_action.call_count)
+
+    @mock.patch.object(action_mod.Action, 'create')
+    @mock.patch.object(no.Node, 'find')
+    def test_node_recover_invalid_operation_params(self, mock_find,
+                                                   mock_action):
+        mock_find.return_value = mock.Mock(id='12345678AB')
+        mock_action.return_value = 'ACTION_ID'
+        params = {'force_recreate': True, 'operation': 'REBOOT',
+                  'operation_params': {'type': 'blah'}}
+        req = orno.NodeRecoverRequest(identity='FAKE_NODE', params=params)
+
+        ex = self.assertRaises(rpc.ExpectedException,
+                               self.eng.node_recover,
+                               self.ctx, req.obj_to_primitive())
+
+        self.assertEqual(exc.BadRequest, ex.exc_info[0])
+        self.assertEqual("Type field 'blah' in operation_params has to be one "
+                         "of the following: SOFT, HARD.",
+                         six.text_type(ex.exc_info[1]))
+        mock_find.assert_called_once_with(self.ctx, 'FAKE_NODE')
+        self.assertEqual(0, mock_action.call_count)
+
     @mock.patch.object(dispatcher, 'start_action')
     @mock.patch.object(action_mod.Action, 'create')
     @mock.patch.object(node_mod.Node, 'load')

senlin/tests/unit/engine/test_node.py

@@ -629,7 +629,7 @@ class TestNode(base.SenlinTestCase):
         mock_recover.return_value = new_id, True
         mock_status.side_effect = set_status
         action = mock.Mock()
-        action.inputs = {'operation': [{'SWIM': 1, 'DANCE': 2}]}
+        action.inputs = {'operation': 'SWIM'}
 
         res = node.do_recover(self.context, action)
 
@@ -690,7 +690,7 @@ class TestNode(base.SenlinTestCase):
         def set_status(*args, **kwargs):
             if args[1] == 'ACTIVE':
                 node.physical_id = new_id
-                node.data = {'recovery': 'RECREATE'}
+                node.data = {'recovery': 'recreate'}
 
         node = nodem.Node('node1', PROFILE_ID, '')
         node.physical_id = 'd94d6333-82e6-4f87-b7ab-b786776df9d1'
@@ -734,44 +734,7 @@ class TestNode(base.SenlinTestCase):
         mock_check = self.patchobject(pb.Profile, 'check_object')
         mock_check.return_value = False
         action = mock.Mock(
-            outputs={}, inputs={'operation': [{'name': 'RECREATE'}],
+            outputs={}, inputs={'operation': 'RECREATE',
-                                'check': True})
-
-        res = node.do_recover(self.context, action)
-
-        self.assertTrue(res)
-        mock_check.assert_called_once_with(self.context, node)
-        mock_recover.assert_called_once_with(
-            self.context, node, **action.inputs)
-        self.assertEqual('node1', node.name)
-        self.assertEqual(new_id, node.physical_id)
-        self.assertEqual(PROFILE_ID, node.profile_id)
-        mock_status.assert_has_calls([
-            mock.call(self.context, 'RECOVERING',
-                      reason='Recovery in progress'),
-            mock.call(self.context, consts.NS_ACTIVE,
-                      reason='Recovery succeeded',
-                      physical_id=new_id,
-                      data={'recovery': 'RECREATE'})])
-
-    @mock.patch.object(nodem.Node, 'set_status')
-    @mock.patch.object(pb.Profile, 'recover_object')
-    def test_node_recover_mult_rebuild(self, mock_recover, mock_status):
-        def set_status(*args, **kwargs):
-            if args[1] == 'ACTIVE':
-                node.physical_id = new_id
-                node.data = {'recovery': 'RECREATE'}
-
-        node = nodem.Node('node1', PROFILE_ID, '', id='fake')
-        node.physical_id = 'd94d6333-82e6-4f87-b7ab-b786776df9d1'
-        new_id = '166db83b-b4a4-49ef-96a8-6c0fdd882d1a'
-        mock_recover.return_value = new_id, True
-        mock_status.side_effect = set_status
-        mock_check = self.patchobject(pb.Profile, 'check_object')
-        mock_check.return_value = False
-        action = mock.Mock(
-            outputs={}, inputs={'operation': [{'name': 'REBOOT'},
-                                              {'name': 'REBUILD'}],
                                 'check': True})
 
         res = node.do_recover(self.context, action)
@@ -811,7 +774,7 @@ class TestNode(base.SenlinTestCase):
             id=node.physical_id,
             reason='Boom!'
         )
-        action = mock.Mock(inputs={'operation': [{'boom': 1}],
+        action = mock.Mock(inputs={'operation': 'boom',
                                    'check': True})
 
         res = node.do_recover(self.context, action)
@@ -837,7 +800,7 @@ class TestNode(base.SenlinTestCase):
         node = nodem.Node('node1', PROFILE_ID, None)
         node.physical_id = 'd94d6333-82e6-4f87-b7ab-b786776df9d1'
         mock_recover.return_value = node.physical_id, None
-        action = mock.Mock(inputs={'operation': [{'name': 'RECREATE'}]})
+        action = mock.Mock(inputs={'operation': 'RECREATE'})
 
         res = node.do_recover(self.context, action)
 
@@ -850,7 +813,7 @@ class TestNode(base.SenlinTestCase):
 
     def test_node_recover_no_physical_id_reboot_op(self):
         node = nodem.Node('node1', PROFILE_ID, None)
-        action = mock.Mock(inputs={'operation': [{'name': 'REBOOT'}]})
+        action = mock.Mock(inputs={'operation': 'REBOOT'})
 
         res = node.do_recover(self.context, action)
 
@@ -858,7 +821,7 @@ class TestNode(base.SenlinTestCase):
 
     def test_node_recover_no_physical_id_rebuild_op(self):
         node = nodem.Node('node1', PROFILE_ID, None)
-        action = mock.Mock(inputs={'operation': [{'name': 'REBUILD'}]})
+        action = mock.Mock(inputs={'operation': 'REBUILD'})
 
         res = node.do_recover(self.context, action)
 
@@ -915,7 +878,7 @@ class TestNode(base.SenlinTestCase):
         mock_check = self.patchobject(pb.Profile, 'check_object')
         mock_check.return_value = False
         action = mock.Mock(
-            outputs={}, inputs={'operation': [{'name': 'RECREATE'}],
+            outputs={}, inputs={'operation': 'RECREATE',
                                 'check': True})
 
         res = node.do_recover(self.context, action)
@@ -940,7 +903,18 @@ class TestNode(base.SenlinTestCase):
         node = nodem.Node('node1', PROFILE_ID, None)
         node.physical_id = 'd94d6333-82e6-4f87-b7ab-b786776df9d1'
         action = mock.Mock(
-            outputs={}, inputs={'operation': [{'name': 'foo'}]})
+            outputs={}, inputs={'operation': 'foo'})
+
+        res = node.do_recover(self.context, action)
+        self.assertEqual({}, action.outputs)
+        self.assertFalse(res)
+
+    @mock.patch.object(nodem.Node, 'set_status')
+    def test_node_recover_operation_not_string(self, mock_set_status):
+        node = nodem.Node('node1', PROFILE_ID, None)
+        node.physical_id = 'd94d6333-82e6-4f87-b7ab-b786776df9d1'
+        action = mock.Mock(
+            outputs={}, inputs={'operation': 'foo'})
 
         res = node.do_recover(self.context, action)
         self.assertEqual({}, action.outputs)

senlin/tests/unit/profiles/test_nova_server.py

@@ -1521,7 +1521,7 @@ class TestNovaServerBasic(base.SenlinTestCase):
         profile = server.ServerProfile('t', self.spec)
         node_obj = mock.Mock(physical_id='FAKE_ID')
 
-        res = profile.do_recover(node_obj, operation=[{'name': 'REBUILD'}])
+        res = profile.do_recover(node_obj, operation='REBUILD')
 
         self.assertEqual(mock_rebuild.return_value, res)
         mock_rebuild.assert_called_once_with(node_obj)
@@ -1531,7 +1531,7 @@ class TestNovaServerBasic(base.SenlinTestCase):
         profile = server.ServerProfile('t', self.spec)
         node_obj = mock.Mock(physical_id='FAKE_ID')
 
-        res = profile.do_recover(node_obj, operation=[{'name': 'REBUILD'}])
+        res = profile.do_recover(node_obj, operation='REBUILD')
 
         self.assertEqual(mock_rebuild.return_value, res)
         mock_rebuild.assert_called_once_with(node_obj)
@@ -1541,7 +1541,7 @@ class TestNovaServerBasic(base.SenlinTestCase):
         profile = server.ServerProfile('t', self.spec)
         node_obj = mock.Mock(physical_id='FAKE_ID')
 
-        res = profile.do_recover(node_obj, operation=[{'name': 'REBOOT'}])
+        res = profile.do_recover(node_obj, operation='REBOOT')
 
         self.assertTrue(res)
         self.assertEqual(mock_reboot.return_value, res)
@@ -1553,7 +1553,7 @@ class TestNovaServerBasic(base.SenlinTestCase):
         node_obj = mock.Mock(physical_id='FAKE_ID')
 
         res, status = profile.do_recover(node_obj,
-                                         operation=[{'name': 'BLAHBLAH'}])
+                                         operation='BLAHBLAH')
 
         self.assertFalse(status)
 
@@ -1562,11 +1562,11 @@ class TestNovaServerBasic(base.SenlinTestCase):
         profile = server.ServerProfile('t', self.spec)
         node_obj = mock.Mock(physical_id='FAKE_ID')
 
-        res = profile.do_recover(node_obj, operation=[{'name': 'RECREATE'}])
+        res = profile.do_recover(node_obj, operation='RECREATE')
 
         self.assertEqual(mock_base_recover.return_value, res)
         mock_base_recover.assert_called_once_with(
-            node_obj, operation=[{'name': 'RECREATE'}])
+            node_obj, operation='RECREATE')
 
     def test_handle_reboot(self):
         obj = mock.Mock(physical_id='FAKE_ID')

senlin/tests/unit/profiles/test_profile_base.py

@@ -16,6 +16,7 @@ import mock
 from oslo_context import context as oslo_ctx
 import six
 
+from senlin.common import consts
 from senlin.common import context as senlin_ctx
 from senlin.common import exception
 from senlin.common import schema
@@ -837,11 +838,12 @@ class TestProfileBase(base.SenlinTestCase):
         self.patchobject(profile, 'do_create', return_value=True)
         self.patchobject(profile, 'do_delete', return_value=True)
 
-        res, status = profile.do_recover(mock.Mock())
+        res, status = profile.do_recover(mock.Mock(),
+                                         operation=consts.RECOVER_RECREATE)
         self.assertTrue(status)
 
         res, status = profile.do_recover(
-            mock.Mock(), operation=[{'name': 'bar'}])
+            mock.Mock(), operation='bar')
         self.assertFalse(status)
 
     def test_do_recover_with_fencing(self):
@@ -851,10 +853,11 @@ class TestProfileBase(base.SenlinTestCase):
         obj = mock.Mock()
 
         res = profile.do_recover(obj, ignore_missing=True,
-                                 params={"fence_compute": True})
+                                 params={"fence_compute": True},
+                                 operation=consts.RECOVER_RECREATE)
 
         self.assertTrue(res)
-        profile.do_delete.assert_called_once_with(obj, force=True,
+        profile.do_delete.assert_called_once_with(obj, force=False,
                                                   timeout=None)
         profile.do_create.assert_called_once_with(obj)
 
@@ -864,7 +867,8 @@ class TestProfileBase(base.SenlinTestCase):
         self.patchobject(profile, 'do_delete', return_value=True)
         obj = mock.Mock()
 
-        res = profile.do_recover(obj, ignore_missing=True, delete_timeout=5)
+        res = profile.do_recover(obj, ignore_missing=True, delete_timeout=5,
+                                 operation=consts.RECOVER_RECREATE)
 
         self.assertTrue(res)
         profile.do_delete.assert_called_once_with(obj, force=False,
@@ -877,7 +881,8 @@ class TestProfileBase(base.SenlinTestCase):
         self.patchobject(profile, 'do_delete', return_value=True)
         obj = mock.Mock()
 
-        res = profile.do_recover(obj, ignore_missing=True, force_recreate=True)
+        res = profile.do_recover(obj, ignore_missing=True, force_recreate=True,
+                                 operation=consts.RECOVER_RECREATE)
 
         self.assertTrue(res)
         profile.do_delete.assert_called_once_with(obj, force=False,
@@ -892,7 +897,8 @@ class TestProfileBase(base.SenlinTestCase):
         self.patchobject(profile, 'do_delete', side_effect=err)
         obj = mock.Mock()
 
-        res = profile.do_recover(obj, ignore_missing=True, force_recreate=True)
+        res = profile.do_recover(obj, ignore_missing=True, force_recreate=True,
+                                 operation=consts.RECOVER_RECREATE)
         self.assertTrue(res)
         profile.do_delete.assert_called_once_with(obj, force=False,
                                                   timeout=None)
@@ -903,7 +909,7 @@ class TestProfileBase(base.SenlinTestCase):
         err = exception.EResourceDeletion(type='STACK', id='ID',
                                           message='BANG')
         self.patchobject(profile, 'do_delete', side_effect=err)
-        operation = [{"name": "RECREATE"}]
+        operation = "RECREATE"
 
         ex = self.assertRaises(exception.EResourceOperation,
                                profile.do_recover,
@@ -917,7 +923,7 @@ class TestProfileBase(base.SenlinTestCase):
         profile = self._create_profile('test-profile')
         self.patchobject(profile, 'do_delete', return_value=True)
         self.patchobject(profile, 'do_create', return_value=True)
-        operation = [{"name": "RECREATE"}]
+        operation = "RECREATE"
         res = profile.do_recover(mock.Mock(), operation=operation)
 
         self.assertTrue(res)
@@ -927,7 +933,7 @@ class TestProfileBase(base.SenlinTestCase):
         err = exception.EResourceDeletion(type='STACK', id='ID',
                                           message='BANG')
         self.patchobject(profile, 'do_delete', side_effect=err)
-        operation = [{"name": "RECREATE"}]
+        operation = "RECREATE"
 
         ex = self.assertRaises(exception.EResourceOperation,
                                profile.do_recover,
@@ -941,7 +947,7 @@ class TestProfileBase(base.SenlinTestCase):
         self.patchobject(profile, 'do_delete', return_value=True)
         err = exception.EResourceCreation(type='STACK', message='BANNG')
         self.patchobject(profile, 'do_create', side_effect=err)
-        operation = [{"name": "RECREATE"}]
+        operation = "RECREATE"
 
         ex = self.assertRaises(exception.EResourceOperation,
                                profile.do_recover,