From c89a17bfe31a668f76b02304fd7478d75f09bcc3 Mon Sep 17 00:00:00 2001 From: rabi Date: Mon, 24 Apr 2017 18:48:20 +0530 Subject: [PATCH] Refactor CooldownMixin The present way of comparing timestamp in metadata + cooldown with current timestamp can be wrong, if the policy cooldown is updated in between scaling activity. Set metadata with the expected cooldown end timestmap rather than the current timestamp when scaling is finished and compare it with current timestamp. Change-Id: I50ddba4c8da3c6ec8b34ebb4768017cb1a446f5f --- .../aws/autoscaling/autoscaling_group.py | 24 +++--- .../openstack/heat/scaling_policy.py | 9 ++- heat/scaling/cooldown.py | 81 +++++++++++-------- .../autoscaling/test_heat_scaling_group.py | 5 +- .../autoscaling/test_heat_scaling_policy.py | 48 ++++++----- heat/tests/autoscaling/test_scaling_group.py | 5 +- heat/tests/autoscaling/test_scaling_policy.py | 33 ++++---- 7 files changed, 114 insertions(+), 91 deletions(-) diff --git a/heat/engine/resources/aws/autoscaling/autoscaling_group.py b/heat/engine/resources/aws/autoscaling/autoscaling_group.py index e7613ecf41..481cca7cdd 100644 --- a/heat/engine/resources/aws/autoscaling/autoscaling_group.py +++ b/heat/engine/resources/aws/autoscaling/autoscaling_group.py @@ -223,19 +223,21 @@ class AutoScalingGroup(cooldown.CooldownMixin, instgrp.InstanceGroup): def check_create_complete(self, task): """Update cooldown timestamp after create succeeds.""" done = super(AutoScalingGroup, self).check_create_complete(task) + cooldown = self.properties[self.COOLDOWN] if done: - self._finished_scaling( - "%s : %s" % (sc_util.CFN_EXACT_CAPACITY, - grouputils.get_size(self))) + self._finished_scaling(cooldown, + "%s : %s" % (sc_util.CFN_EXACT_CAPACITY, + grouputils.get_size(self))) return done def check_update_complete(self, cookie): """Update the cooldown timestamp after update succeeds.""" done = super(AutoScalingGroup, self).check_update_complete(cookie) + cooldown = self.properties[self.COOLDOWN] if done: - self._finished_scaling( - "%s : %s" % (sc_util.CFN_EXACT_CAPACITY, - grouputils.get_size(self))) + self._finished_scaling(cooldown, + "%s : %s" % (sc_util.CFN_EXACT_CAPACITY, + grouputils.get_size(self))) return done def _get_new_capacity(self, capacity, @@ -284,7 +286,7 @@ class AutoScalingGroup(cooldown.CooldownMixin, instgrp.InstanceGroup): def adjust(self, adjustment, adjustment_type=sc_util.CFN_CHANGE_IN_CAPACITY, - min_adjustment_step=None): + min_adjustment_step=None, cooldown=None): """Adjust the size of the scaling group if the cooldown permits.""" if self.status != self.COMPLETE: LOG.info("%s NOT performing scaling adjustment, " @@ -300,7 +302,10 @@ class AutoScalingGroup(cooldown.CooldownMixin, instgrp.InstanceGroup): "as there is no change in capacity.", self.name) raise resource.NoActionRequired - self._check_scaling_allowed() + if cooldown is None: + cooldown = self.properties[self.COOLDOWN] + + self._check_scaling_allowed(cooldown) # send a notification before, on-error and on-success. notif = { @@ -342,7 +347,8 @@ class AutoScalingGroup(cooldown.CooldownMixin, instgrp.InstanceGroup): "group %s.", self.name) raise finally: - self._finished_scaling("%s : %s" % (adjustment_type, adjustment), + self._finished_scaling(cooldown, + "%s : %s" % (adjustment_type, adjustment), size_changed=size_changed) def _tags(self): diff --git a/heat/engine/resources/openstack/heat/scaling_policy.py b/heat/engine/resources/openstack/heat/scaling_policy.py index 90738f6885..aa40e7b1de 100644 --- a/heat/engine/resources/openstack/heat/scaling_policy.py +++ b/heat/engine/resources/openstack/heat/scaling_policy.py @@ -172,7 +172,7 @@ class AutoScalingPolicy(cooldown.CooldownMixin, ) % {'alarm': self.name, 'group': asgn_id}) - self._check_scaling_allowed() + self._check_scaling_allowed(self.properties[self.COOLDOWN]) LOG.info('%(name)s alarm, adjusting group %(group)s with id ' '%(asgn_id)s by %(filter)s', @@ -196,9 +196,10 @@ class AutoScalingPolicy(cooldown.CooldownMixin, {'name': self.name, 'group': group.name}) raise finally: - self._finished_scaling("%s : %s" % ( - self.properties[self.ADJUSTMENT_TYPE], - self.properties[self.SCALING_ADJUSTMENT]), + self._finished_scaling( + self.properties[self.COOLDOWN], + "%s : %s" % (self.properties[self.ADJUSTMENT_TYPE], + self.properties[self.SCALING_ADJUSTMENT]), size_changed=size_changed) def _resolve_attribute(self, name): diff --git a/heat/scaling/cooldown.py b/heat/scaling/cooldown.py index e3724856a3..5fbab16b2b 100644 --- a/heat/scaling/cooldown.py +++ b/heat/scaling/cooldown.py @@ -11,11 +11,12 @@ # License for the specific language governing permissions and limitations # under the License. -from oslo_log import log as logging +import datetime from heat.common import exception from heat.common.i18n import _ from heat.engine import resource +from oslo_log import log as logging from oslo_utils import timeutils import six @@ -29,7 +30,12 @@ class CooldownMixin(object): This logic includes both cooldown timestamp comparing and scaling in progress checking. """ - def _check_scaling_allowed(self): + def _sanitize_cooldown(self, cooldown): + if cooldown is None: + return 0 + return max(0, cooldown) + + def _check_scaling_allowed(self, cooldown): metadata = self.metadata_get() if metadata.get('scaling_in_progress'): LOG.info("Can not perform scaling action: resource %s " @@ -37,51 +43,58 @@ class CooldownMixin(object): 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]) - except TypeError: - # If not specified, it will be None, same as cooldown == 0 - cooldown = 0 + cooldown = self._sanitize_cooldown(cooldown) + # if both cooldown and cooldown_end not in metadata + if all(k not in metadata for k in ('cooldown', 'cooldown_end')): + # Note: this is for supporting old version cooldown checking + metadata.pop('scaling_in_progress', None) + if metadata and cooldown != 0: + last_adjust = next(six.iterkeys(metadata)) + if not timeutils.is_older_than(last_adjust, cooldown): + self._log_and_raise_no_action(cooldown) - if cooldown != 0: - try: - if 'cooldown' not in metadata: - # Note: this is for supporting old version cooldown logic - if metadata: - last_adjust = next(six.iterkeys(metadata)) - self._cooldown_check(cooldown, last_adjust) - else: - last_adjust = next(six.iterkeys(metadata['cooldown'])) - self._cooldown_check(cooldown, last_adjust) - except ValueError: - # occurs when metadata has only {scaling_in_progress: False} - pass + elif 'cooldown_end' in metadata: + cooldown_end = next(six.iterkeys(metadata['cooldown_end'])) + now = timeutils.utcnow().isoformat() + if now < cooldown_end: + self._log_and_raise_no_action(cooldown) + + elif cooldown != 0: + # Note: this is also for supporting old version cooldown checking + last_adjust = next(six.iterkeys(metadata['cooldown'])) + if not timeutils.is_older_than(last_adjust, cooldown): + self._log_and_raise_no_action(cooldown) # Assumes _finished_scaling is called # after the scaling operation completes metadata['scaling_in_progress'] = True self.metadata_set(metadata) - def _cooldown_check(self, cooldown, last_adjust): - if not timeutils.is_older_than(last_adjust, cooldown): - LOG.info("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 _log_and_raise_no_action(self, cooldown): + LOG.info("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): + def _finished_scaling(self, cooldown, + cooldown_reason, size_changed=True): # If we wanted to implement the AutoScaling API like AWS does, # we could maintain event history here, but since we only need # the latest event for cooldown, just store that for now metadata = self.metadata_get() if size_changed: - now = timeutils.utcnow().isoformat() - metadata['cooldown'] = {now: cooldown_reason} + cooldown = self._sanitize_cooldown(cooldown) + cooldown_end = (timeutils.utcnow() + datetime.timedelta( + seconds=cooldown)).isoformat() + if 'cooldown_end' in metadata: + cooldown_end = max( + next(six.iterkeys(metadata['cooldown_end'])), + cooldown_end) + metadata['cooldown_end'] = {cooldown_end: cooldown_reason} metadata['scaling_in_progress'] = False try: self.metadata_set(metadata) diff --git a/heat/tests/autoscaling/test_heat_scaling_group.py b/heat/tests/autoscaling/test_heat_scaling_group.py index e98a0753b1..50e7517b26 100644 --- a/heat/tests/autoscaling/test_heat_scaling_group.py +++ b/heat/tests/autoscaling/test_heat_scaling_group.py @@ -159,6 +159,7 @@ class TestGroupAdjust(common.HeatTestCase): self.assertEqual(expected_notifies, notify.call_args_list) resize.assert_called_once_with(3) finished_scaling.assert_called_once_with( + None, 'PercentChangeInCapacity : 33', size_changed=True) @@ -190,6 +191,7 @@ class TestGroupAdjust(common.HeatTestCase): self.assertEqual(expected_notifies, notify.call_args_list) resize.assert_called_once_with(1) finished_scaling.assert_called_once_with( + None, 'PercentChangeInCapacity : -33', size_changed=True) @@ -218,7 +220,8 @@ class TestGroupAdjust(common.HeatTestCase): self.assertEqual(expected_notifies, notify.call_args_list) resize.assert_called_once_with(1) - finished_scaling.assert_called_once_with('ChangeInCapacity : 1', + finished_scaling.assert_called_once_with(None, + 'ChangeInCapacity : 1', size_changed=True) grouputils.get_size.assert_called_once_with(self.group) diff --git a/heat/tests/autoscaling/test_heat_scaling_policy.py b/heat/tests/autoscaling/test_heat_scaling_policy.py index 65fb7695c9..05a4d4fa75 100644 --- a/heat/tests/autoscaling/test_heat_scaling_policy.py +++ b/heat/tests/autoscaling/test_heat_scaling_policy.py @@ -89,8 +89,9 @@ class TestAutoScalingPolicy(common.HeatTestCase): '_check_scaling_allowed') as mock_isa: self.assertRaises(resource.NoActionRequired, up_policy.handle_signal) - mock_isa.assert_called_once_with() - mock_fin_scaling.assert_called_once_with('change_in_capacity : 1', + mock_isa.assert_called_once_with(60) + mock_fin_scaling.assert_called_once_with(60, + 'change_in_capacity : 1', size_changed=False) def test_scaling_policy_adjust_size_changed(self): @@ -104,8 +105,9 @@ class TestAutoScalingPolicy(common.HeatTestCase): 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', + mock_isa.assert_called_once_with(60) + mock_fin_scaling.assert_called_once_with(60, + 'change_in_capacity : 1', size_changed=True) def test_scaling_policy_cooldown_toosoon(self): @@ -122,7 +124,7 @@ class TestAutoScalingPolicy(common.HeatTestCase): side_effect=resource.NoActionRequired) as mock_cip: self.assertRaises(resource.NoActionRequired, pol.handle_signal, details=test) - mock_cip.assert_called_once_with() + mock_cip.assert_called_once_with(60) self.assertEqual([], dont_call.call_args_list) def test_policy_metadata_reset(self): @@ -146,7 +148,7 @@ class TestAutoScalingPolicy(common.HeatTestCase): group.name = 'fluffy' with mock.patch.object(pol, '_check_scaling_allowed') as mock_isa: pol.handle_signal(details=test) - mock_isa.assert_called_once_with() + mock_isa.assert_called_once_with(60) group.adjust.assert_called_once_with(1, 'change_in_capacity', None) def test_scaling_policy_refid(self): @@ -183,12 +185,14 @@ class TestCooldownMixin(common.HeatTestCase): stack = utils.parse_stack(t, params=as_params) pol = self.create_scaling_policy(t, stack, 'my-policy') + properties = t['resources']['my-policy']['properties'] now = timeutils.utcnow() previous_meta = {'cooldown': { now.isoformat(): 'change_in_capacity : 1'}} self.patchobject(pol, 'metadata_get', return_value=previous_meta) ex = self.assertRaises(resource.NoActionRequired, - pol._check_scaling_allowed) + pol._check_scaling_allowed, + properties['cooldown']) self.assertIn('due to cooldown', six.text_type(ex)) def test_cooldown_is_in_progress_scaling_unfinished(self): @@ -196,10 +200,12 @@ class TestCooldownMixin(common.HeatTestCase): stack = utils.parse_stack(t, params=as_params) pol = self.create_scaling_policy(t, stack, 'my-policy') + properties = t['resources']['my-policy']['properties'] previous_meta = {'scaling_in_progress': True} self.patchobject(pol, 'metadata_get', return_value=previous_meta) ex = self.assertRaises(resource.NoActionRequired, - pol._check_scaling_allowed) + pol._check_scaling_allowed, + properties['cooldown']) self.assertIn('due to scaling activity', six.text_type(ex)) def test_cooldown_not_in_progress(self): @@ -215,15 +221,11 @@ class TestCooldownMixin(common.HeatTestCase): 'scaling_in_progress': False } self.patchobject(pol, 'metadata_get', return_value=previous_meta) - self.assertIsNone(pol._check_scaling_allowed()) + self.assertIsNone(pol._check_scaling_allowed(60)) def test_scaling_policy_cooldown_zero(self): t = template_format.parse(as_template) - # Create the scaling policy (with cooldown=0) and scale up one - properties = t['resources']['my-policy']['properties'] - properties['cooldown'] = '0' - stack = utils.parse_stack(t, params=as_params) pol = self.create_scaling_policy(t, stack, 'my-policy') @@ -231,16 +233,11 @@ class TestCooldownMixin(common.HeatTestCase): previous_meta = {'cooldown': { now.isoformat(): 'change_in_capacity : 1'}} self.patchobject(pol, 'metadata_get', return_value=previous_meta) - self.assertIsNone(pol._check_scaling_allowed()) + self.assertIsNone(pol._check_scaling_allowed(0)) def test_scaling_policy_cooldown_none(self): t = template_format.parse(as_template) - # Create the scaling policy no cooldown property, should behave the - # same as when cooldown==0 - properties = t['resources']['my-policy']['properties'] - del properties['cooldown'] - stack = utils.parse_stack(t, params=as_params) pol = self.create_scaling_policy(t, stack, 'my-policy') @@ -248,17 +245,18 @@ class TestCooldownMixin(common.HeatTestCase): previous_meta = {'cooldown': { now.isoformat(): 'change_in_capacity : 1'}} self.patchobject(pol, 'metadata_get', return_value=previous_meta) - self.assertIsNone(pol._check_scaling_allowed()) + self.assertIsNone(pol._check_scaling_allowed(None)) def test_no_cooldown_no_scaling_in_progress(self): t = template_format.parse(as_template) stack = utils.parse_stack(t, params=as_params) pol = self.create_scaling_policy(t, stack, 'my-policy') - # no cooldown entry in the metadata - previous_meta = {'scaling_in_progress': False} + awhile_ago = timeutils.utcnow() - datetime.timedelta(seconds=100) + previous_meta = {'scaling_in_progress': False, + awhile_ago.isoformat(): 'change_in_capacity : 1'} self.patchobject(pol, 'metadata_get', return_value=previous_meta) - self.assertIsNone(pol._check_scaling_allowed()) + self.assertIsNone(pol._check_scaling_allowed(60)) def test_metadata_is_written(self): t = template_format.parse(as_template) @@ -269,9 +267,9 @@ class TestCooldownMixin(common.HeatTestCase): reason = 'cool as' meta_set = self.patchobject(pol, 'metadata_set') self.patchobject(timeutils, 'utcnow', return_value=nowish) - pol._finished_scaling(reason, size_changed=True) + pol._finished_scaling(0, reason, size_changed=True) meta_set.assert_called_once_with( - {'cooldown': {nowish.isoformat(): reason}, + {'cooldown_end': {nowish.isoformat(): reason}, 'scaling_in_progress': False}) diff --git a/heat/tests/autoscaling/test_scaling_group.py b/heat/tests/autoscaling/test_scaling_group.py index 233cf5076f..122eec1843 100644 --- a/heat/tests/autoscaling/test_scaling_group.py +++ b/heat/tests/autoscaling/test_scaling_group.py @@ -357,6 +357,7 @@ class TestGroupAdjust(common.HeatTestCase): self.assertEqual(expected_notifies, notify.call_args_list) resize.assert_called_once_with(3) finished_scaling.assert_called_once_with( + None, 'PercentChangeInCapacity : 33', size_changed=True) @@ -388,6 +389,7 @@ class TestGroupAdjust(common.HeatTestCase): self.assertEqual(expected_notifies, notify.call_args_list) resize.assert_called_once_with(3) finished_scaling.assert_called_once_with( + None, 'PercentChangeInCapacity : -33', size_changed=True) @@ -416,7 +418,8 @@ class TestGroupAdjust(common.HeatTestCase): self.assertEqual(expected_notifies, notify.call_args_list) resize.assert_called_once_with(1) - finished_scaling.assert_called_once_with('ChangeInCapacity : 1', + finished_scaling.assert_called_once_with(None, + 'ChangeInCapacity : 1', size_changed=True) grouputils.get_size.assert_called_once_with(self.group) diff --git a/heat/tests/autoscaling/test_scaling_policy.py b/heat/tests/autoscaling/test_scaling_policy.py index dff0fe536a..fc59606343 100644 --- a/heat/tests/autoscaling/test_scaling_policy.py +++ b/heat/tests/autoscaling/test_scaling_policy.py @@ -92,8 +92,9 @@ class TestAutoScalingPolicy(common.HeatTestCase): '_check_scaling_allowed') as mock_isa: self.assertRaises(resource.NoActionRequired, up_policy.handle_signal) - mock_isa.assert_called_once_with() - mock_fin_scaling.assert_called_once_with('ChangeInCapacity : 1', + mock_isa.assert_called_once_with(60) + mock_fin_scaling.assert_called_once_with(60, + 'ChangeInCapacity : 1', size_changed=False) def test_scaling_policy_adjust_size_changed(self): @@ -107,8 +108,9 @@ class TestAutoScalingPolicy(common.HeatTestCase): 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', + mock_isa.assert_called_once_with(60) + mock_fin_scaling.assert_called_once_with(60, + 'ChangeInCapacity : 1', size_changed=True) def test_scaling_policy_cooldown_toosoon(self): @@ -125,7 +127,7 @@ class TestAutoScalingPolicy(common.HeatTestCase): side_effect=resource.NoActionRequired) as mock_isa: self.assertRaises(resource.NoActionRequired, pol.handle_signal, details=test) - mock_isa.assert_called_once_with() + mock_isa.assert_called_once_with(60) self.assertEqual([], dont_call.call_args_list) def test_scaling_policy_cooldown_ok(self): @@ -138,7 +140,7 @@ class TestAutoScalingPolicy(common.HeatTestCase): group.name = 'fluffy' with mock.patch.object(pol, '_check_scaling_allowed') as mock_isa: pol.handle_signal(details=test) - mock_isa.assert_called_once_with() + mock_isa.assert_called_once_with(60) group.adjust.assert_called_once_with(1, 'ChangeInCapacity', None) @mock.patch.object(aws_sp.AWSScalingPolicy, '_get_ec2_signed_url') @@ -188,7 +190,8 @@ class TestCooldownMixin(common.HeatTestCase): now.isoformat(): 'ChangeInCapacity : 1'}} self.patchobject(pol, 'metadata_get', return_value=previous_meta) ex = self.assertRaises(resource.NoActionRequired, - pol._check_scaling_allowed) + pol._check_scaling_allowed, + 60) self.assertIn('due to cooldown', six.text_type(ex)) def test_cooldown_is_in_progress_scaling_unfinished(self): @@ -199,7 +202,7 @@ class TestCooldownMixin(common.HeatTestCase): previous_meta = {'scaling_in_progress': True} self.patchobject(pol, 'metadata_get', return_value=previous_meta) ex = self.assertRaises(resource.NoActionRequired, - pol._check_scaling_allowed) + pol._check_scaling_allowed, 60) self.assertIn('due to scaling activity', six.text_type(ex)) def test_cooldown_not_in_progress(self): @@ -215,22 +218,18 @@ class TestCooldownMixin(common.HeatTestCase): 'scaling_in_progress': False } self.patchobject(pol, 'metadata_get', return_value=previous_meta) - self.assertIsNone(pol._check_scaling_allowed()) + self.assertIsNone(pol._check_scaling_allowed(60)) def test_scaling_policy_cooldown_zero(self): t = template_format.parse(as_template) - # Create the scaling policy (with Cooldown=0) and scale up one - properties = t['Resources']['WebServerScaleUpPolicy']['Properties'] - properties['Cooldown'] = '0' - stack = utils.parse_stack(t, params=as_params) pol = self.create_scaling_policy(t, stack, 'WebServerScaleUpPolicy') now = timeutils.utcnow() previous_meta = {now.isoformat(): 'ChangeInCapacity : 1'} self.patchobject(pol, 'metadata_get', return_value=previous_meta) - self.assertIsNone(pol._check_scaling_allowed()) + self.assertIsNone(pol._check_scaling_allowed(0)) def test_scaling_policy_cooldown_none(self): t = template_format.parse(as_template) @@ -246,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.assertIsNone(pol._check_scaling_allowed()) + self.assertIsNone(pol._check_scaling_allowed(None)) def test_metadata_is_written(self): t = template_format.parse(as_template) @@ -257,9 +256,9 @@ class TestCooldownMixin(common.HeatTestCase): reason = 'cool as' meta_set = self.patchobject(pol, 'metadata_set') self.patchobject(timeutils, 'utcnow', return_value=nowish) - pol._finished_scaling(reason) + pol._finished_scaling(0, reason) meta_set.assert_called_once_with( - {'cooldown': {nowish.isoformat(): reason}, + {'cooldown_end': {nowish.isoformat(): reason}, 'scaling_in_progress': False})