From f0fbae7c115f4da2d1f01d15ba3fb5b1e2401073 Mon Sep 17 00:00:00 2001 From: Duc Truong Date: Thu, 5 Nov 2020 01:25:20 +0000 Subject: [PATCH] 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 | 34 ++++++++----------- senlin/policies/base.py | 4 +-- .../unit/engine/actions/test_action_base.py | 34 +++++-------------- .../unit/engine/actions/test_node_action.py | 34 +++++++++++++++++++ 4 files changed, 59 insertions(+), 47 deletions(-) diff --git a/senlin/engine/actions/base.py b/senlin/engine/actions/base.py index 82b41f024..8ddf9efc0 100755 --- a/senlin/engine/actions/base.py +++ b/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['reason'] = 'Completed policy checking.' + self.data['status'] = policy_mod.CHECK_NONE + 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) - if res is False: + # stop policy check is one of them fails + 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 diff --git a/senlin/policies/base.py b/senlin/policies/base.py index 5e95331dd..dd285f11b 100644 --- a/senlin/policies/base.py +++ b/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' ) diff --git a/senlin/tests/unit/engine/actions/test_action_base.py b/senlin/tests/unit/engine/actions/test_action_base.py index 9d6df4f1c..4267e3ea8 100755 --- a/senlin/tests/unit/engine/actions/test_action_base.py +++ b/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_check, mock_load, + def test_policy_check_abort_in_middle(self, 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') diff --git a/senlin/tests/unit/engine/actions/test_node_action.py b/senlin/tests/unit/engine/actions/test_node_action.py index c841ef047..88406ba2e 100644 --- a/senlin/tests/unit/engine/actions/test_node_action.py +++ b/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')