Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

(17.32.0) DEVOPS-9370: use old capacity value when scaling down #41

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

(17.32.0) DEVOPS-9370: use old capacity value when scaling down #41

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 12 additions & 20 deletions License2Deploy/rolling_deploy.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ def __init__(self,
self.health_wait = health_wait
self.only_new_wait = only_new_wait
self.existing_instance_ids = []
self.old_desired_capacity = 2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where is this default of 2 coming from vs None

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default can be anything because that field will be assigned to at the beginning of deployment. However a test case is going to fail if set it to None.

self.new_desired_capacity = None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a dead gobal variable as you've refactored to only be used within method scope

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not dead. new_desired_capacity is used by code (line 202). We need this field to hold the value the capacity is temporarily increased to.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since it's only used in the one function, calculate_autoscale_desired_instance_count, it doesn't need to be declared in the init. we can just declare it in the function w/o self.


def get_ami_id_state(self, ami_id):
Expand Down Expand Up @@ -123,24 +124,19 @@ def get_lb(self):
def calculate_autoscale_desired_instance_count(self, group_name, desired_state):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't this be done as such and avoid the class variable:

  def calculate_autoscale_desired_instance_count(self, group_name, desired_state):
    ''' Search via specific autoscale group name to return modified desired instance count '''
    try:
      new_desired_capacity = self.old_desired_capacity * 2
      if desired_state == 'increase':
        self.old_desired_capacity = int(self.get_group_info(group_name)[0].desired_capacity)
        logging.info("Current desired count was changed from {0} to {1}".format(self.old_desired_capacity, new_desired_capacity))
        return new_desired_capacity
      elif desired_state == 'decrease':
        logging.info("Current desired count was changed from {0} to {1}".format(new_desired_capacity, self.old_desired_capacity))
        return self.old_desired_capacity
      else:
        raise Exception("Please make sure the desired_state is set to either increase or decrease")
      return None #not sure this is required
    except Exception as e:
      logging.error(e)
      exit(self.exit_error_code)

''' Search via specific autoscale group name to return modified desired instance count '''
try:
cur_count = int(self.get_group_info(group_name)[0].desired_capacity)
if desired_state == 'increase':
new_count = self.double_autoscale_instance_count(cur_count)
self.old_desired_capacity = int(self.get_group_info(group_name)[0].desired_capacity)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should moved back outside of the try block.
if you want to cache the value do an if None: set the value, the reason being is that if this method is called todecrease without being called to increase first it will currently set itself to 2 , correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. As mentioned above, 2 was set there just to satisfy a test case. It could be set to 1000 as long as a number greater than 2. It won't matter because this line right here will set it to the original desired capacity of the ASG. It is 2 for dnbi, perhaps 3 for owl? or 5 for whatever. It is the ASG capacity that we later want to revert to.

self.new_desired_capacity = self.old_desired_capacity * 2
logging.info("Current desired count was changed from {0} to {1}".format(self.old_desired_capacity, self.new_desired_capacity))
elif desired_state == 'decrease':
new_count = self.decrease_autoscale_instance_count(cur_count)
logging.info("Current desired count was changed from {0} to {1}".format(cur_count, new_count))
return new_count
logging.info("Current desired count was changed from {0} to {1}".format(self.new_desired_capacity, self.old_desired_capacity))
self.new_desired_capacity = self.old_desired_capacity
else:
raise Exception("Please make sure the desired_state is set to either increase or decrease")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

put back the value that was passed in the error message

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should also be a ValueError

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, this is to satisfy a test case. We can change the test but it is really not related to the issue we are attacking here.

return self.new_desired_capacity
except Exception as e:
logging.error("Please make sure the desired_state is set to either increase or decrease: {0}".format(e))
logging.error(e)
exit(self.exit_error_code)

def double_autoscale_instance_count(self, count):
''' Multiply current count by 2 '''
return count * 2

def decrease_autoscale_instance_count(self, count):
''' Divide current count in half '''
return count / 2

def set_autoscale_instance_desired_count(self, new_count, group_name):
''' Increase desired count by double '''
Expand Down Expand Up @@ -191,16 +187,13 @@ def get_instance_ids_by_requested_build_tag(self, id_list, build):
and 'BUILD' in inst.tags
and inst.tags['BUILD'] == str(build)]

if len(new_instances) < self.get_new_instances_count():
if len(new_instances) < self.old_desired_capacity:
raise Exception('Not all new instances with build number "{0}" are in the group'.format(self.build_number))
else:
ip_dict = self.get_instance_ip_addrs(new_instances)
logging.info("New Instance List with IP Addresses: {0}".format(ip_dict))
return new_instances

def get_new_instances_count(self):
return self.new_desired_capacity / 2

def wait_for_new_instances(self, instance_ids, retry=10, wait_time=30):
''' Monitor new instances that come up and wait until they are ready '''
for instance in instance_ids:
Expand Down Expand Up @@ -363,8 +356,7 @@ def deploy(self): # pragma: no cover
if not self.force_redeploy and self.is_redeploy():
self.stop_deploy('You are attempting to redeploy the same build. Please pass the force_redeploy flag if a redeploy is desired')
self.disable_project_cloudwatch_alarms()
self.new_desired_capacity = self.calculate_autoscale_desired_instance_count(group_name, 'increase')
self.set_autoscale_instance_desired_count(self.new_desired_capacity, group_name)
self.set_autoscale_instance_desired_count(self.calculate_autoscale_desired_instance_count(group_name, 'increase'), group_name)
self.launch_new_instances(group_name)
self.set_autoscale_instance_desired_count(self.calculate_autoscale_desired_instance_count(group_name, 'decrease'), group_name)
self.confirm_lb_has_only_new_instances()
Expand Down
8 changes: 1 addition & 7 deletions tests/rolling_deploy_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,7 @@ def test_calculate_autoscale_desired_instance_count(self):
increase = self.rolling_deploy.calculate_autoscale_desired_instance_count(self.GMS_AUTOSCALING_GROUP_STG, 'increase')
decrease = self.rolling_deploy.calculate_autoscale_desired_instance_count(self.GMS_AUTOSCALING_GROUP_STG, 'decrease')
self.assertEqual(increase, 4)
self.assertEqual(decrease, 1)
self.assertEqual(decrease, 2)

@mock_autoscaling_deprecated
def test_calculate_autoscale_desired_instance_count_failure(self):
Expand Down Expand Up @@ -384,12 +384,6 @@ def test_wait_for_new_instances_failure(self):
def test_set_autoscale_instance_desired_count_failure(self):
self.assertRaises(SystemExit, lambda: self.rolling_deploy.set_autoscale_instance_desired_count(4, self.GMS_AUTOSCALING_GROUP_STG))

def test_double_autoscale_instance_count(self):
self.assertEqual(self.rolling_deploy.double_autoscale_instance_count(2), 4)

def test_decrease_autoscale_instance_count(self):
self.assertEqual(self.rolling_deploy.decrease_autoscale_instance_count(4), 2)

def main():
unittest.main()

Expand Down