From 197febdb01f93a39c8225d7a5a25da1a4eb6d963 Mon Sep 17 00:00:00 2001 From: huangtianhua Date: Mon, 16 Jan 2017 16:07:15 +0800 Subject: [PATCH] Improve resource.NoActionRequired Log the proper reasons (scaling activity or cooldown) when scaling is not allowed. Change-Id: I3a9ed8fe7bff8696551470a9fac0b1a2831eb95c --- heat/engine/resource.py | 9 +++-- .../aws/autoscaling/autoscaling_group.py | 7 +--- .../openstack/heat/scaling_policy.py | 7 +--- heat/scaling/cooldown.py | 33 +++++++++++++++---- .../autoscaling/test_heat_scaling_group.py | 20 ++++------- .../autoscaling/test_heat_scaling_policy.py | 33 ++++++++++--------- heat/tests/autoscaling/test_scaling_group.py | 20 ++++------- heat/tests/autoscaling/test_scaling_policy.py | 31 +++++++++-------- 8 files changed, 84 insertions(+), 76 deletions(-) diff --git a/heat/engine/resource.py b/heat/engine/resource.py index a5b369bb2f..e16dc74428 100644 --- a/heat/engine/resource.py +++ b/heat/engine/resource.py @@ -71,7 +71,11 @@ class NoActionRequired(Exception): Resource subclasses should raise this exception from handle_signal() to suppress recording of an event corresponding to the signal. """ - pass + def __init__(self, res_name='Unknown', reason=''): + msg = (_("The resource %(res)s could not perform " + "scaling action: %(reason)s") % + {'res': res_name, 'reason': reason}) + super(Exception, self).__init__(six.text_type(msg)) class PollDelay(Exception): @@ -2162,7 +2166,8 @@ class Resource(object): pass except Exception as ex: LOG.info(_LI('signal %(name)s : %(msg)s'), - {'name': six.text_type(self), 'msg': ex}, + {'name': six.text_type(self), + 'msg': six.text_type(ex)}, exc_info=True) failure = exception.ResourceFailure(ex, self) raise failure diff --git a/heat/engine/resources/aws/autoscaling/autoscaling_group.py b/heat/engine/resources/aws/autoscaling/autoscaling_group.py index 9c4b26048f..48c092e5be 100644 --- a/heat/engine/resources/aws/autoscaling/autoscaling_group.py +++ b/heat/engine/resources/aws/autoscaling/autoscaling_group.py @@ -295,12 +295,7 @@ class AutoScalingGroup(instgrp.InstanceGroup, cooldown.CooldownMixin): "as there is no change in capacity.") % self.name) raise resource.NoActionRequired - if not self._is_scaling_allowed(): - LOG.info(_LI("%(name)s NOT performing scaling adjustment, " - "cooldown %(cooldown)s") % { - 'name': self.name, - 'cooldown': self.properties[self.COOLDOWN]}) - raise resource.NoActionRequired + self._check_scaling_allowed() # send a notification before, on-error and on-success. notif = { diff --git a/heat/engine/resources/openstack/heat/scaling_policy.py b/heat/engine/resources/openstack/heat/scaling_policy.py index 56f6834af2..21d1f58436 100644 --- a/heat/engine/resources/openstack/heat/scaling_policy.py +++ b/heat/engine/resources/openstack/heat/scaling_policy.py @@ -174,12 +174,7 @@ class AutoScalingPolicy(signal_responder.SignalResponder, ) % {'alarm': self.name, 'group': asgn_id}) - if not self._is_scaling_allowed(): - LOG.info(_LI("%(name)s NOT performing scaling action, " - "cooldown %(cooldown)s") % { - 'name': self.name, - 'cooldown': self.properties[self.COOLDOWN]}) - raise resource.NoActionRequired + self._check_scaling_allowed() LOG.info(_LI('%(name)s alarm, adjusting group %(group)s with id ' '%(asgn_id)s by %(filter)s') % { diff --git a/heat/scaling/cooldown.py b/heat/scaling/cooldown.py index 315b543c3a..2fe2b88c50 100644 --- a/heat/scaling/cooldown.py +++ b/heat/scaling/cooldown.py @@ -11,10 +11,17 @@ # License for the specific language governing permissions and limitations # under the License. +from oslo_log import log as logging + from heat.common import exception +from heat.common.i18n import _ +from heat.common.i18n import _LI +from heat.engine import resource from oslo_utils import timeutils import six +LOG = logging.getLogger(__name__) + class CooldownMixin(object): """Utility class to encapsulate Cooldown related logic. @@ -23,10 +30,14 @@ class CooldownMixin(object): This logic includes both cooldown timestamp comparing and scaling in progress checking. """ - def _is_scaling_allowed(self): + def _check_scaling_allowed(self): metadata = self.metadata_get() if metadata.get('scaling_in_progress'): - return False + LOG.info(_LI("Can not perform scaling action: resource %s " + "is already in scaling.") % self.name) + reason = _('due to scaling activity') + raise resource.NoActionRequired(res_name=self.name, + reason=reason) try: # Negative values don't make sense, so they are clamped to zero cooldown = max(0, self.properties[self.COOLDOWN]) @@ -40,12 +51,10 @@ class CooldownMixin(object): # Note: this is for supporting old version cooldown logic if metadata: last_adjust = next(six.iterkeys(metadata)) - if not timeutils.is_older_than(last_adjust, cooldown): - return False + self._cooldown_check(cooldown, last_adjust) else: last_adjust = next(six.iterkeys(metadata['cooldown'])) - if not timeutils.is_older_than(last_adjust, cooldown): - return False + self._cooldown_check(cooldown, last_adjust) except ValueError: # occurs when metadata has only {scaling_in_progress: False} pass @@ -54,7 +63,17 @@ class CooldownMixin(object): # after the scaling operation completes metadata['scaling_in_progress'] = True self.metadata_set(metadata) - return True + + def _cooldown_check(self, cooldown, last_adjust): + if not timeutils.is_older_than(last_adjust, cooldown): + LOG.info(_LI("Can not perform scaling action: " + "resource %(name)s is in cooldown (%(cooldown)s).") % + {'name': self.name, + 'cooldown': cooldown}) + reason = _('due to cooldown, ' + 'cooldown %s') % cooldown + raise resource.NoActionRequired( + res_name=self.name, reason=reason) def _finished_scaling(self, cooldown_reason, size_changed=True): # If we wanted to implement the AutoScaling API like AWS does, diff --git a/heat/tests/autoscaling/test_heat_scaling_group.py b/heat/tests/autoscaling/test_heat_scaling_group.py index 0e04029d0f..5f913adae1 100644 --- a/heat/tests/autoscaling/test_heat_scaling_group.py +++ b/heat/tests/autoscaling/test_heat_scaling_group.py @@ -110,10 +110,9 @@ class TestGroupAdjust(common.HeatTestCase): self.assertIsNone(self.group.validate()) def test_scaling_policy_cooldown_toosoon(self): - """If _is_scaling_allowed() returns False don't progress.""" dont_call = self.patchobject(self.group, 'resize') - self.patchobject(self.group, '_is_scaling_allowed', - return_value=False) + self.patchobject(self.group, '_check_scaling_allowed', + side_effect=resource.NoActionRequired) self.assertRaises(resource.NoActionRequired, self.group.adjust, 1) self.assertEqual([], dont_call.call_args_list) @@ -137,8 +136,7 @@ class TestGroupAdjust(common.HeatTestCase): resize = self.patchobject(self.group, 'resize') finished_scaling = self.patchobject(self.group, '_finished_scaling') notify = self.patch('heat.engine.notification.autoscaling.send') - self.patchobject(self.group, '_is_scaling_allowed', - return_value=True) + self.patchobject(self.group, '_check_scaling_allowed') self.group.adjust(33, adjustment_type='PercentChangeInCapacity', min_adjustment_step=2) @@ -169,8 +167,7 @@ class TestGroupAdjust(common.HeatTestCase): resize = self.patchobject(self.group, 'resize') finished_scaling = self.patchobject(self.group, '_finished_scaling') notify = self.patch('heat.engine.notification.autoscaling.send') - self.patchobject(self.group, '_is_scaling_allowed', - return_value=True) + self.patchobject(self.group, '_check_scaling_allowed') self.group.adjust(-33, adjustment_type='PercentChangeInCapacity', min_adjustment_step=2) @@ -201,8 +198,7 @@ class TestGroupAdjust(common.HeatTestCase): resize = self.patchobject(self.group, 'resize') finished_scaling = self.patchobject(self.group, '_finished_scaling') notify = self.patch('heat.engine.notification.autoscaling.send') - self.patchobject(self.group, '_is_scaling_allowed', - return_value=True) + self.patchobject(self.group, '_check_scaling_allowed') self.group.adjust(1) expected_notifies = [ @@ -231,8 +227,7 @@ class TestGroupAdjust(common.HeatTestCase): self.patchobject(self.group, 'resize', side_effect=ValueError('test error')) notify = self.patch('heat.engine.notification.autoscaling.send') - self.patchobject(self.group, '_is_scaling_allowed', - return_value=True) + self.patchobject(self.group, '_check_scaling_allowed') self.patchobject(self.group, '_finished_scaling') self.assertRaises(ValueError, self.group.adjust, 1) @@ -261,8 +256,7 @@ class TestGroupAdjust(common.HeatTestCase): self.patchobject(self.group, 'resize', side_effect=ValueError('test error')) notify = self.patch('heat.engine.notification.autoscaling.send') - self.patchobject(self.group, '_is_scaling_allowed', - return_value=True) + self.patchobject(self.group, '_check_scaling_allowed') self.patchobject(self.group, '_finished_scaling') self.assertRaises(ValueError, self.group.adjust, diff --git a/heat/tests/autoscaling/test_heat_scaling_policy.py b/heat/tests/autoscaling/test_heat_scaling_policy.py index 0477d9fd84..13869b571d 100644 --- a/heat/tests/autoscaling/test_heat_scaling_policy.py +++ b/heat/tests/autoscaling/test_heat_scaling_policy.py @@ -84,8 +84,8 @@ class TestAutoScalingPolicy(common.HeatTestCase): self.patchobject(group, 'adjust', side_effect=resource.NoActionRequired()) mock_fin_scaling = self.patchobject(up_policy, '_finished_scaling') - with mock.patch.object(up_policy, '_is_scaling_allowed', - return_value=True) as mock_isa: + with mock.patch.object(up_policy, + '_check_scaling_allowed') as mock_isa: self.assertRaises(resource.NoActionRequired, up_policy.handle_signal) mock_isa.assert_called_once_with() @@ -100,15 +100,14 @@ class TestAutoScalingPolicy(common.HeatTestCase): group = stack['my-group'] self.patchobject(group, 'adjust') mock_fin_scaling = self.patchobject(up_policy, '_finished_scaling') - with mock.patch.object(up_policy, '_is_scaling_allowed', - return_value=True) as mock_isa: + with mock.patch.object(up_policy, + '_check_scaling_allowed') as mock_isa: self.assertIsNone(up_policy.handle_signal()) mock_isa.assert_called_once_with() mock_fin_scaling.assert_called_once_with('change_in_capacity : 1', size_changed=True) def test_scaling_policy_cooldown_toosoon(self): - """If _is_scaling_allowed() returns False don't progress.""" t = template_format.parse(as_template) stack = utils.parse_stack(t, params=as_params) pol = self.create_scaling_policy(t, stack, 'my-policy') @@ -117,8 +116,9 @@ class TestAutoScalingPolicy(common.HeatTestCase): with mock.patch.object(group, 'adjust', side_effect=AssertionError) as dont_call: - with mock.patch.object(pol, '_is_scaling_allowed', - return_value=False) as mock_cip: + with mock.patch.object( + pol, '_check_scaling_allowed', + side_effect=resource.NoActionRequired) as mock_cip: self.assertRaises(resource.NoActionRequired, pol.handle_signal, details=test) mock_cip.assert_called_once_with() @@ -132,8 +132,7 @@ class TestAutoScalingPolicy(common.HeatTestCase): group = self.patchobject(pol.stack, 'resource_by_refid').return_value group.name = 'fluffy' - with mock.patch.object(pol, '_is_scaling_allowed', - return_value=True) as mock_isa: + with mock.patch.object(pol, '_check_scaling_allowed') as mock_isa: pol.handle_signal(details=test) mock_isa.assert_called_once_with() group.adjust.assert_called_once_with(1, 'change_in_capacity', None) @@ -176,7 +175,9 @@ class TestCooldownMixin(common.HeatTestCase): previous_meta = {'cooldown': { now.isoformat(): 'change_in_capacity : 1'}} self.patchobject(pol, 'metadata_get', return_value=previous_meta) - self.assertFalse(pol._is_scaling_allowed()) + ex = self.assertRaises(resource.NoActionRequired, + pol._check_scaling_allowed) + self.assertIn('due to cooldown', six.text_type(ex)) def test_cooldown_is_in_progress_scaling_unfinished(self): t = template_format.parse(as_template) @@ -185,7 +186,9 @@ class TestCooldownMixin(common.HeatTestCase): previous_meta = {'scaling_in_progress': True} self.patchobject(pol, 'metadata_get', return_value=previous_meta) - self.assertFalse(pol._is_scaling_allowed()) + ex = self.assertRaises(resource.NoActionRequired, + pol._check_scaling_allowed) + self.assertIn('due to scaling activity', six.text_type(ex)) def test_cooldown_not_in_progress(self): t = template_format.parse(as_template) @@ -200,7 +203,7 @@ class TestCooldownMixin(common.HeatTestCase): 'scaling_in_progress': False } self.patchobject(pol, 'metadata_get', return_value=previous_meta) - self.assertTrue(pol._is_scaling_allowed()) + self.assertIsNone(pol._check_scaling_allowed()) def test_scaling_policy_cooldown_zero(self): t = template_format.parse(as_template) @@ -216,7 +219,7 @@ class TestCooldownMixin(common.HeatTestCase): previous_meta = {'cooldown': { now.isoformat(): 'change_in_capacity : 1'}} self.patchobject(pol, 'metadata_get', return_value=previous_meta) - self.assertTrue(pol._is_scaling_allowed()) + self.assertIsNone(pol._check_scaling_allowed()) def test_scaling_policy_cooldown_none(self): t = template_format.parse(as_template) @@ -233,7 +236,7 @@ class TestCooldownMixin(common.HeatTestCase): previous_meta = {'cooldown': { now.isoformat(): 'change_in_capacity : 1'}} self.patchobject(pol, 'metadata_get', return_value=previous_meta) - self.assertTrue(pol._is_scaling_allowed()) + self.assertIsNone(pol._check_scaling_allowed()) def test_no_cooldown_no_scaling_in_progress(self): t = template_format.parse(as_template) @@ -243,7 +246,7 @@ class TestCooldownMixin(common.HeatTestCase): # no cooldown entry in the metadata previous_meta = {'scaling_in_progress': False} self.patchobject(pol, 'metadata_get', return_value=previous_meta) - self.assertTrue(pol._is_scaling_allowed()) + self.assertIsNone(pol._check_scaling_allowed()) def test_metadata_is_written(self): t = template_format.parse(as_template) diff --git a/heat/tests/autoscaling/test_scaling_group.py b/heat/tests/autoscaling/test_scaling_group.py index 9d9bceb056..233cf5076f 100644 --- a/heat/tests/autoscaling/test_scaling_group.py +++ b/heat/tests/autoscaling/test_scaling_group.py @@ -294,10 +294,9 @@ class TestGroupAdjust(common.HeatTestCase): self.assertIsNone(self.group.validate()) def test_scaling_policy_cooldown_toosoon(self): - """If _is_scaling_allowed() returns False don't progress.""" dont_call = self.patchobject(self.group, 'resize') - self.patchobject(self.group, '_is_scaling_allowed', - return_value=False) + self.patchobject(self.group, '_check_scaling_allowed', + side_effect=resource.NoActionRequired) self.assertRaises(resource.NoActionRequired, self.group.adjust, 1) self.assertEqual([], dont_call.call_args_list) @@ -335,8 +334,7 @@ class TestGroupAdjust(common.HeatTestCase): resize = self.patchobject(self.group, 'resize') finished_scaling = self.patchobject(self.group, '_finished_scaling') notify = self.patch('heat.engine.notification.autoscaling.send') - self.patchobject(self.group, '_is_scaling_allowed', - return_value=True) + self.patchobject(self.group, '_check_scaling_allowed') self.group.adjust(33, adjustment_type='PercentChangeInCapacity', min_adjustment_step=2) @@ -367,8 +365,7 @@ class TestGroupAdjust(common.HeatTestCase): resize = self.patchobject(self.group, 'resize') finished_scaling = self.patchobject(self.group, '_finished_scaling') notify = self.patch('heat.engine.notification.autoscaling.send') - self.patchobject(self.group, '_is_scaling_allowed', - return_value=True) + self.patchobject(self.group, '_check_scaling_allowed') self.group.adjust(-33, adjustment_type='PercentChangeInCapacity', min_adjustment_step=2) @@ -399,8 +396,7 @@ class TestGroupAdjust(common.HeatTestCase): resize = self.patchobject(self.group, 'resize') finished_scaling = self.patchobject(self.group, '_finished_scaling') notify = self.patch('heat.engine.notification.autoscaling.send') - self.patchobject(self.group, '_is_scaling_allowed', - return_value=True) + self.patchobject(self.group, '_check_scaling_allowed') self.group.adjust(1) expected_notifies = [ @@ -429,8 +425,7 @@ class TestGroupAdjust(common.HeatTestCase): self.patchobject(self.group, 'resize', side_effect=ValueError('test error')) notify = self.patch('heat.engine.notification.autoscaling.send') - self.patchobject(self.group, '_is_scaling_allowed', - return_value=True) + self.patchobject(self.group, '_check_scaling_allowed') self.patchobject(self.group, '_finished_scaling') self.assertRaises(ValueError, self.group.adjust, 1) @@ -459,8 +454,7 @@ class TestGroupAdjust(common.HeatTestCase): self.patchobject(self.group, 'resize', side_effect=ValueError('test error')) notify = self.patch('heat.engine.notification.autoscaling.send') - self.patchobject(self.group, '_is_scaling_allowed', - return_value=True) + self.patchobject(self.group, '_check_scaling_allowed') self.patchobject(self.group, '_finished_scaling') self.assertRaises(ValueError, self.group.adjust, diff --git a/heat/tests/autoscaling/test_scaling_policy.py b/heat/tests/autoscaling/test_scaling_policy.py index 2b8500fe8e..6a839ba982 100644 --- a/heat/tests/autoscaling/test_scaling_policy.py +++ b/heat/tests/autoscaling/test_scaling_policy.py @@ -87,8 +87,8 @@ class TestAutoScalingPolicy(common.HeatTestCase): self.patchobject(group, 'adjust', side_effect=resource.NoActionRequired()) mock_fin_scaling = self.patchobject(up_policy, '_finished_scaling') - with mock.patch.object(up_policy, '_is_scaling_allowed', - return_value=True) as mock_isa: + with mock.patch.object(up_policy, + '_check_scaling_allowed') as mock_isa: self.assertRaises(resource.NoActionRequired, up_policy.handle_signal) mock_isa.assert_called_once_with() @@ -103,15 +103,14 @@ class TestAutoScalingPolicy(common.HeatTestCase): group = stack['WebServerGroup'] self.patchobject(group, 'adjust') mock_fin_scaling = self.patchobject(up_policy, '_finished_scaling') - with mock.patch.object(up_policy, '_is_scaling_allowed', - return_value=True) as mock_isa: + with mock.patch.object(up_policy, + '_check_scaling_allowed') as mock_isa: self.assertIsNone(up_policy.handle_signal()) mock_isa.assert_called_once_with() mock_fin_scaling.assert_called_once_with('ChangeInCapacity : 1', size_changed=True) def test_scaling_policy_cooldown_toosoon(self): - """If _is_scaling_allowed() returns False don't progress.""" t = template_format.parse(as_template) stack = utils.parse_stack(t, params=as_params) pol = self.create_scaling_policy(t, stack, 'WebServerScaleUpPolicy') @@ -120,8 +119,9 @@ class TestAutoScalingPolicy(common.HeatTestCase): with mock.patch.object(group, 'adjust', side_effect=AssertionError) as dont_call: - with mock.patch.object(pol, '_is_scaling_allowed', - return_value=False) as mock_isa: + with mock.patch.object( + pol, '_check_scaling_allowed', + side_effect=resource.NoActionRequired) as mock_isa: self.assertRaises(resource.NoActionRequired, pol.handle_signal, details=test) mock_isa.assert_called_once_with() @@ -135,8 +135,7 @@ class TestAutoScalingPolicy(common.HeatTestCase): group = self.patchobject(pol.stack, 'resource_by_refid').return_value group.name = 'fluffy' - with mock.patch.object(pol, '_is_scaling_allowed', - return_value=True) as mock_isa: + with mock.patch.object(pol, '_check_scaling_allowed') as mock_isa: pol.handle_signal(details=test) mock_isa.assert_called_once_with() group.adjust.assert_called_once_with(1, 'ChangeInCapacity', None) @@ -187,7 +186,9 @@ class TestCooldownMixin(common.HeatTestCase): previous_meta = {'cooldown': { now.isoformat(): 'ChangeInCapacity : 1'}} self.patchobject(pol, 'metadata_get', return_value=previous_meta) - self.assertFalse(pol._is_scaling_allowed()) + ex = self.assertRaises(resource.NoActionRequired, + pol._check_scaling_allowed) + self.assertIn('due to cooldown', six.text_type(ex)) def test_cooldown_is_in_progress_scaling_unfinished(self): t = template_format.parse(as_template) @@ -196,7 +197,9 @@ class TestCooldownMixin(common.HeatTestCase): previous_meta = {'scaling_in_progress': True} self.patchobject(pol, 'metadata_get', return_value=previous_meta) - self.assertFalse(pol._is_scaling_allowed()) + ex = self.assertRaises(resource.NoActionRequired, + pol._check_scaling_allowed) + self.assertIn('due to scaling activity', six.text_type(ex)) def test_cooldown_not_in_progress(self): t = template_format.parse(as_template) @@ -211,7 +214,7 @@ class TestCooldownMixin(common.HeatTestCase): 'scaling_in_progress': False } self.patchobject(pol, 'metadata_get', return_value=previous_meta) - self.assertTrue(pol._is_scaling_allowed()) + self.assertIsNone(pol._check_scaling_allowed()) def test_scaling_policy_cooldown_zero(self): t = template_format.parse(as_template) @@ -226,7 +229,7 @@ class TestCooldownMixin(common.HeatTestCase): now = timeutils.utcnow() previous_meta = {now.isoformat(): 'ChangeInCapacity : 1'} self.patchobject(pol, 'metadata_get', return_value=previous_meta) - self.assertTrue(pol._is_scaling_allowed()) + self.assertIsNone(pol._check_scaling_allowed()) def test_scaling_policy_cooldown_none(self): t = template_format.parse(as_template) @@ -242,7 +245,7 @@ class TestCooldownMixin(common.HeatTestCase): now = timeutils.utcnow() previous_meta = {now.isoformat(): 'ChangeInCapacity : 1'} self.patchobject(pol, 'metadata_get', return_value=previous_meta) - self.assertTrue(pol._is_scaling_allowed()) + self.assertIsNone(pol._check_scaling_allowed()) def test_metadata_is_written(self): t = template_format.parse(as_template)