Release cluster lock on failed policy check

When policy check fails, the cluster lock will not be released because
the the policy check status is incorrectly set to ok.  This patch fixes
it so the cluster lock is released on failed policy checks.

Change-Id: I6f964d16e243476730e1fd5a628fa6a4870a0c6f
Closes-Bug: #1903568

senlin/engine/actions/base.py

@@ -496,21 +496,6 @@ class Action(object):
     def is_resumed(self):
         return self._check_signal() == self.SIG_RESUME
 
-    def _check_result(self, name):
-        """Check policy status and generate event.
-
-        :param name: Name of policy checked
-        :return: True if the policy checking can be continued, or False if the
-                 policy checking should be aborted.
-        """
-        reason = self.data['reason']
-        if self.data['status'] == policy_mod.CHECK_OK:
-            return True
-
-        self.data['reason'] = ("Failed policy '%(name)s': %(reason)s"
-                               ) % {'name': name, 'reason': reason}
-        return False
-
     def policy_check(self, cluster_id, target):
         """Check all policies attached to cluster and give result.
 
@@ -526,9 +511,10 @@ class Action(object):
         bindings = cpo.ClusterPolicy.get_all(self.context, cluster_id,
                                              sort='priority',
                                              filters={'enabled': True})
+
         # default values
-        self.data['status'] = policy_mod.CHECK_OK
+        self.data['status'] = policy_mod.CHECK_NONE
-        self.data['reason'] = 'Completed policy checking.'
+        self.data['reason'] = ''
 
         for pb in bindings:
             policy = policy_mod.Policy.load(self.context, pb.policy_id)
@@ -545,12 +531,22 @@ class Action(object):
             else:  # target == 'AFTER'
                 method = getattr(policy, 'post_op', None)
 
+            # call policy check function
+            # it will set result in data['status']
             if method is not None:
                 method(cluster_id, self)
 
-            res = self._check_result(policy.name)
+            # stop policy check is one of them fails
-            if res is False:
+            if self.data['status'] == policy_mod.CHECK_ERROR:
+                reason = self.data['reason']
+                self.data['reason'] = ("Failed policy '%(name)s': %(reason)s"
+                                       ) % {'name': policy.name,
+                                            'reason': reason}
                 return
+
+        self.data['status'] = policy_mod.CHECK_OK
+        self.data['reason'] = 'Completed policy checking.'
+
         return
 
     @staticmethod

senlin/policies/base.py

@@ -25,9 +25,9 @@ from senlin.objects import credential as co
 from senlin.objects import policy as po
 
 CHECK_RESULTS = (
-    CHECK_OK, CHECK_ERROR,
+    CHECK_OK, CHECK_ERROR, CHECK_NONE
 ) = (
-    'OK', 'ERROR',
+    'OK', 'ERROR', 'NONE'
 )
 
 

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

@@ -1024,29 +1024,6 @@ class ActionPolicyCheckTest(base.SenlinTestCase):
         self.assertEqual(0, mock_pre_op.call_count)
         self.assertEqual(0, mock_post_op.call_count)
 
-    def test_check_result_true(self):
-        cluster_id = CLUSTER_ID
-        action = ab.Action(cluster_id, 'OBJECT_ACTION', self.ctx)
-        action.data['status'] = policy_mod.CHECK_OK
-        action.data['reason'] = "Completed policy checking."
-
-        res = action._check_result('FAKE_POLICY_NAME')
-
-        self.assertTrue(res)
-
-    def test_check_result_false(self):
-        cluster_id = CLUSTER_ID
-        action = ab.Action(cluster_id, 'OBJECT_ACTION', self.ctx)
-        action.data['status'] = policy_mod.CHECK_ERROR
-        reason = ("Policy '%s' cooldown is still in progress." %
-                  'FAKE_POLICY_2')
-        action.data['reason'] = reason
-
-        res = action._check_result('FAKE_POLICY_NAME')
-        reason = ("Failed policy '%(name)s': %(reason)s"
-                  ) % {'name': 'FAKE_POLICY_NAME', 'reason': reason}
-        self.assertFalse(res)
-
     @mock.patch.object(cpo.ClusterPolicy, 'get_all')
     @mock.patch.object(policy_mod.Policy, 'load')
     def test_policy_check_pre_op(self, mock_load, mock_load_all):
@@ -1112,8 +1089,7 @@ class ActionPolicyCheckTest(base.SenlinTestCase):
 
     @mock.patch.object(cpo.ClusterPolicy, 'get_all')
     @mock.patch.object(policy_mod.Policy, 'load')
-    @mock.patch.object(ab.Action, '_check_result')
+    def test_policy_check_abort_in_middle(self, mock_load,
-    def test_policy_check_abort_in_middle(self, mock_check, mock_load,
                                           mock_load_all):
         cluster_id = CLUSTER_ID
         # Note: both policies are mocked
@@ -1124,6 +1100,13 @@ class ActionPolicyCheckTest(base.SenlinTestCase):
                             TARGET=[('AFTER', 'OBJECT_ACTION')])
         policy2.name = 'P2'
         action = ab.Action(cluster_id, 'OBJECT_ACTION', self.ctx)
+        action.data = mock.MagicMock()
+
+        # mock action.data to return error for the first policy call
+        # (i.e. after policy1 post_op method has been called).
+        # this should stop the policy check and prevent the
+        # the policy2 post_op method from being called.
+        action.data.__getitem__.side_effect = [policy_mod.CHECK_ERROR, '']
 
         # Note: policy binding is created but not stored
         pb1 = self._create_cp_binding(cluster_id, policy1.id)
@@ -1131,7 +1114,6 @@ class ActionPolicyCheckTest(base.SenlinTestCase):
         mock_load_all.return_value = [pb1, pb2]
         # mock return value for two calls
         mock_load.side_effect = [policy1, policy2]
-        mock_check.side_effect = [False, True]
 
         res = action.policy_check(cluster_id, 'AFTER')
 

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

@@ -637,6 +637,40 @@ class NodeActionTest(base.SenlinTestCase):
                                              lock.NODE_SCOPE)
         mock_check.assert_called_once_with('FAKE_CLUSTER', 'BEFORE')
 
+    @mock.patch.object(lock, 'cluster_lock_acquire')
+    @mock.patch.object(lock, 'cluster_lock_release')
+    @mock.patch.object(base_action.Action, 'policy_check')
+    def test_execute_policy_check_exception(self, mock_check, mock_release,
+                                            mock_acquire, mock_load):
+        node = mock.Mock()
+        node.id = 'NID'
+        node.cluster_id = 'FAKE_CLUSTER'
+        mock_load.return_value = node
+
+        action = node_action.NodeAction('NODE_ID', 'NODE_FLY', self.ctx,
+                                        cause='RPC Request')
+        action.id = 'ACTION_ID'
+        action.data = {
+            'status': policy_mod.CHECK_NONE,
+            'reason': ''
+        }
+        mock_acquire.return_value = action.id
+
+        mock_check.side_effect = Exception('error')
+
+        res_code, res_msg = action.execute()
+
+        reason = 'Policy check: '
+        self.assertEqual(action.RES_ERROR, res_code)
+        self.assertEqual(reason, res_msg)
+        mock_load.assert_called_once_with(action.context, node_id='NODE_ID')
+        mock_acquire.assert_called_once_with(self.ctx, 'FAKE_CLUSTER',
+                                             'ACTION_ID', None,
+                                             lock.NODE_SCOPE, False)
+        mock_release.assert_called_once_with('FAKE_CLUSTER', 'ACTION_ID',
+                                             lock.NODE_SCOPE)
+        mock_check.assert_called_once_with('FAKE_CLUSTER', 'BEFORE')
+
     @mock.patch.object(lock, 'cluster_lock_acquire')
     @mock.patch.object(lock, 'cluster_lock_release')
     @mock.patch.object(lock, 'node_lock_acquire')