From 047bd6f651c50c1e28030cdae3115d3a39116b1f Mon Sep 17 00:00:00 2001 From: moe Date: Wed, 4 Feb 2015 05:56:19 +0100 Subject: [PATCH 1/3] Fixes #744. The following cases work for me now: - Create new ASG with tags - Update tags on ASG (create/change/delete) In short, the module should now work as expected wrt tagging. The previous code did not work at all with latest boto for me (serialization errors) and the logic was buggy anyway; e.g. removed tags would never get deleted from ec2. --- cloud/amazon/ec2_asg.py | 78 +++++++++++++++++++++++------------------ 1 file changed, 43 insertions(+), 35 deletions(-) diff --git a/cloud/amazon/ec2_asg.py b/cloud/amazon/ec2_asg.py index 6e5d3508cb8..c20f2d500d8 100644 --- a/cloud/amazon/ec2_asg.py +++ b/cloud/amazon/ec2_asg.py @@ -219,6 +219,25 @@ def enforce_required_arguments(module): def get_properties(autoscaling_group): properties = dict((attr, getattr(autoscaling_group, attr)) for attr in ASG_ATTRIBUTES) + + # + # Ansible output is serialized to JSON but our + # tag list is sometimes a list of Tag-objects + # (as received by Boto). + # + # Since we can not easily teach python to serialize + # such a list we replace it with a dict-representation + # in those cases. + # + # Yes, this is an ugly hack. But at least it works, + # unlike the even uglier hack that was here previously... + # + if 'tags' in properties and properties['tags'] is list: + serializable_tags = {} + for tag in properties['tags']: + serializable_tags[tag.key] = [tag.value, tag.propagate_at_launch] + properties['tags'] = serializable_tags + properties['healthy_instances'] = 0 properties['in_service_instances'] = 0 properties['unhealthy_instances'] = 0 @@ -281,18 +300,12 @@ def create_autoscaling_group(connection, module): asg_tags = [] for tag in set_tags: - if tag.has_key('key') and tag.has_key('value'): # this block is to support depricated form - asg_tags.append(Tag(key=tag.get('key'), - value=tag.get('value'), - propagate_at_launch=bool(tag.get('propagate_at_launch', True)), - resource_id=group_name)) - else: - for k,v in tag.iteritems(): - if k !='propagate_at_launch': - asg_tags.append(Tag(key=k, - value=v, - propagate_at_launch=bool(tag.get('propagate_at_launch', True)), - resource_id=group_name)) + for k,v in tag.iteritems(): + if k !='propagate_at_launch': + asg_tags.append(Tag(key=k, + value=v, + propagate_at_launch=bool(tag.get('propagate_at_launch', True)), + resource_id=group_name)) if not as_groups: if not vpc_zone_identifier and not availability_zones: @@ -344,30 +357,25 @@ def create_autoscaling_group(connection, module): setattr(as_group, attr, module_attr) if len(set_tags) > 0: - existing_tags = as_group.tags - existing_tag_map = dict((tag.key, tag) for tag in existing_tags) - for tag in set_tags: - if tag.has_key('key') and tag.has_key('value'): # this is to support deprecated method - if 'key' not in tag: - continue - if ( not tag['key'] in existing_tag_map or - existing_tag_map[tag['key']].value != tag['value'] or - ('propagate_at_launch' in tag and - existing_tag_map[tag['key']].propagate_at_launch != tag['propagate_at_launch']) ): - changed = True - continue - else: - for k,v in tag.iteritems(): - if k !='propagate_at_launch': - if ( not k in existing_tag_map or - existing_tag_map[k].value != v or - ('propagate_at_launch' in tag and - existing_tag_map[k].propagate_at_launch != tag['propagate_at_launch']) ): - changed = True - continue - if changed: + have_tags = {} + want_tags = {} + + for tag in asg_tags: + want_tags[tag.key] = [tag.value, tag.propagate_at_launch] + + for tag in as_group.tags: + have_tags[tag.key] = [tag.value, tag.propagate_at_launch] + dead_tags = [] + if not tag.key in want_tags: + changed = True + dead_tags.append(tag) + + if dead_tags != []: + connection.delete_tags(dead_tags) + + if have_tags != want_tags: + changed = True connection.create_or_update_tags(asg_tags) - as_group.tags = asg_tags # handle loadbalancers separately because None != [] load_balancers = module.params.get('load_balancers') or [] From 0d0205ad54b805635ee32723f167e1b8123252ef Mon Sep 17 00:00:00 2001 From: moe Date: Wed, 4 Feb 2015 06:13:52 +0100 Subject: [PATCH 2/3] Woops, make collect/delete loop more efficient. --- cloud/amazon/ec2_asg.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cloud/amazon/ec2_asg.py b/cloud/amazon/ec2_asg.py index c20f2d500d8..ac5e501bbee 100644 --- a/cloud/amazon/ec2_asg.py +++ b/cloud/amazon/ec2_asg.py @@ -363,15 +363,15 @@ def create_autoscaling_group(connection, module): for tag in asg_tags: want_tags[tag.key] = [tag.value, tag.propagate_at_launch] + dead_tags = [] for tag in as_group.tags: have_tags[tag.key] = [tag.value, tag.propagate_at_launch] - dead_tags = [] if not tag.key in want_tags: changed = True dead_tags.append(tag) - if dead_tags != []: - connection.delete_tags(dead_tags) + if dead_tags != []: + connection.delete_tags(dead_tags) if have_tags != want_tags: changed = True From 5399f3744f0302ba9e5e076f792de8f74936577d Mon Sep 17 00:00:00 2001 From: Toshio Kuratomi Date: Fri, 20 Mar 2015 09:52:19 -0700 Subject: [PATCH 3/3] Fix review comments from @bcoca in #745 --- cloud/amazon/ec2_asg.py | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/cloud/amazon/ec2_asg.py b/cloud/amazon/ec2_asg.py index 92025c2b23c..97d89d4c987 100644 --- a/cloud/amazon/ec2_asg.py +++ b/cloud/amazon/ec2_asg.py @@ -225,19 +225,10 @@ def enforce_required_arguments(module): def get_properties(autoscaling_group): properties = dict((attr, getattr(autoscaling_group, attr)) for attr in ASG_ATTRIBUTES) - # - # Ansible output is serialized to JSON but our - # tag list is sometimes a list of Tag-objects - # (as received by Boto). - # - # Since we can not easily teach python to serialize - # such a list we replace it with a dict-representation - # in those cases. - # - # Yes, this is an ugly hack. But at least it works, - # unlike the even uglier hack that was here previously... - # - if 'tags' in properties and properties['tags'] is list: + # Ugly hack to make this JSON-serializable. We take a list of boto Tag + # objects and replace them with a dict-representation. Needed because the + # tags are included in ansible's return value (which is jsonified) + if 'tags' in properties and isinstance(properties['tags'], list): serializable_tags = {} for tag in properties['tags']: serializable_tags[tag.key] = [tag.value, tag.propagate_at_launch]