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.

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

ecs_taskdefinition: ensure cast to integer and idempotency fix #574

Merged
merged 4 commits into from
Aug 11, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions changelogs/fragments/574-ecs_taskdefinition-improvement.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
bugfixes:
- ecs_taskdefinition - ensure cast to integer (https://github.com/ansible-collections/community.aws/pull/574).
- ecs_taskdefinition - fix idempotency (https://github.com/ansible-collections/community.aws/pull/574).
71 changes: 53 additions & 18 deletions plugins/modules/ecs_taskdefinition.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,10 @@
short_description: register a task definition in ecs
description:
- Registers or deregisters task definitions in the Amazon Web Services (AWS) EC2 Container Service (ECS).
author: Mark Chance (@Java1Guy)
author:
- Mark Chance (@Java1Guy)
- Alina Buzachis (@alinabuzachis)
requirements: [ json, botocore, boto3 ]
options:
state:
description:
Expand Down Expand Up @@ -189,7 +192,7 @@
linuxParameters:
description: Linux-specific modifications that are applied to the container, such as Linux kernel capabilities.
required: False
type: list
type: dict
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like it's going from a list of dicts to a dict, is this not a breaking change? (Although I guess we can do this with 2.0.0

Copy link
Member

Choose a reason for hiding this comment

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

(Imho) it never worked as a list. I guess it was one of the transformation errors when elements where introduced.

suboptions:
capabilities:
description:
Expand Down Expand Up @@ -410,6 +413,8 @@
description: The type of the ulimit.
type: str
required: False
choices: ['core', 'cpu', 'data', 'fsize', 'locks', 'memlock', 'msgqueue', 'nice', 'nofile', 'nproc', 'rss',
'rtprio', 'rttime', 'sigpending', 'stack']
softLimit:
description: The soft limit for the ulimit type.
type: int
Expand Down Expand Up @@ -642,9 +647,9 @@
except ImportError:
pass # caught by AnsibleAWSModule

from ansible.module_utils._text import to_text

from ansible_collections.amazon.aws.plugins.module_utils.core import AnsibleAWSModule
from ansible_collections.amazon.aws.plugins.module_utils.ec2 import AWSRetry


class EcsTaskManager:
Expand All @@ -653,21 +658,21 @@ class EcsTaskManager:
def __init__(self, module):
self.module = module

self.ecs = module.client('ecs')
self.ecs = module.client('ecs', AWSRetry.jittered_backoff())

def describe_task(self, task_name):
try:
response = self.ecs.describe_task_definition(taskDefinition=task_name)
response = self.ecs.describe_task_definition(aws_retry=True, taskDefinition=task_name)
return response['taskDefinition']
except botocore.exceptions.ClientError:
except (botocore.exceptions.ClientError, botocore.exceptions.BotoCoreError) as e:
return None

def register_task(self, family, task_role_arn, execution_role_arn, network_mode, container_definitions, volumes, launch_type, cpu, memory):
validated_containers = []

# Ensures the number parameters are int as required by boto
for container in container_definitions:
for param in ('memory', 'cpu', 'memoryReservation'):
for param in ('memory', 'cpu', 'memoryReservation', 'startTimeout', 'stopTimeout'):
if param in container:
container[param] = int(container[param])

Expand All @@ -681,6 +686,23 @@ def register_task(self, family, task_role_arn, execution_role_arn, network_mode,
self.module.fail_json(msg="In awsvpc network mode, host port must be set to the same as "
"container port or not be set")

if 'linuxParameters' in container:
for linux_param in container.get('linuxParameters'):
if linux_param == 'tmpfs':
for tmpfs_param in container['linuxParameters']['tmpfs']:
if 'size' in tmpfs_param:
tmpfs_param['size'] = int(tmpfs_param['size'])

for param in ('maxSwap', 'swappiness', 'sharedMemorySize'):
if param in linux_param:
container['linuxParameters'][param] = int(container['linuxParameters'][param])
Comment on lines +696 to +698
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
for param in ('maxSwap', 'swappiness', 'sharedMemorySize'):
if param in linux_param:
container['linuxParameters'][param] = int(container['linuxParameters'][param])
if linux_param in ('maxSwap', 'swappiness', 'sharedMemorySize'):`
container['linuxParameters'][param] = int(container['linuxParameters'][param])

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@markuman Thank you for reviewing. I used the for loop here because I need to cast to int all these parameters ('maxSwap', 'swappiness', 'sharedMemorySize'). The user can use all together.


if 'ulimits' in container:
for limits_mapping in container['ulimits']:
for limit in ('softLimit', 'hardLimit'):
if limit in limits_mapping:
limits_mapping[limit] = int(limits_mapping[limit])
Comment on lines +701 to +704
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
for limits_mapping in container['ulimits']:
for limit in ('softLimit', 'hardLimit'):
if limit in limits_mapping:
limits_mapping[limit] = int(limits_mapping[limit])
for limits_mapping in container['ulimits']:
for limit in ('softLimit', 'hardLimit'):
if limit == limits_mapping:
limits_mapping[limit] = int(limits_mapping[limit])

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@markuman limits_mapping is a dict, for this reason I used if limit in limits_mapping.


validated_containers.append(container)

params = dict(
Expand All @@ -701,8 +723,8 @@ def register_task(self, family, task_role_arn, execution_role_arn, network_mode,
params['executionRoleArn'] = execution_role_arn

try:
response = self.ecs.register_task_definition(**params)
except botocore.exceptions.ClientError as e:
response = self.ecs.register_task_definition(aws_retry=True, **params)
except (botocore.exceptions.ClientError, botocore.exceptions.BotoCoreError) as e:
self.module.fail_json_aws(e, msg="Failed to register task")

return response['taskDefinition']
Expand Down Expand Up @@ -795,28 +817,31 @@ def main():
module.fail_json(msg='links parameter is not supported if network mode is awsvpc.')

for environment in container.get('environment', []):
environment['value'] = to_text(environment['value'])
environment['value'] = environment['value']

for environment_file in container.get('environmentFiles', []):
if environment_file['type'] != 's3':
module.fail_json(msg='The only supported value for environmentFiles is s3.')

for linux_param in container.get('linuxParameters', {}):
if linux_param.get('devices') and launch_type == 'FARGATE':
if linux_param == 'maxSwap' and launch_type == 'FARGATE':
module.fail_json(msg='devices parameter is not supported with the FARGATE launch type.')

if linux_param.get('maxSwap') and launch_type == 'FARGATE':
if linux_param == 'maxSwap' and launch_type == 'FARGATE':
module.fail_json(msg='maxSwap parameter is not supported with the FARGATE launch type.')
elif linux_param.get('maxSwap') and linux_param['maxSwap'] < 0:
elif linux_param == 'maxSwap' and int(container['linuxParameters']['maxSwap']) < 0:
Copy link
Member

Choose a reason for hiding this comment

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

What happen if key maxSwap does not exist? Is it possible? I've no idea.

Copy link
Member

Choose a reason for hiding this comment

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

same for swappiness

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@markuman maxSwap and swappiness are optional and they can be specified within linuxParameters. If this is what you meant. This flow here is only executed when the parameters are specified.

module.fail_json(msg='Accepted values for maxSwap are 0 or any positive integer.')

if linux_param.get('swappiness') and (linux_param['swappiness'] < 0 or linux_param['swappiness'] > 100):
if (
linux_param == 'swappiness' and
(int(container['linuxParameters']['swappiness']) < 0 or int(container['linuxParameters']['swappiness']) > 100)
):
module.fail_json(msg='Accepted values for swappiness are whole numbers between 0 and 100.')

if linux_param.get('sharedMemorySize') and launch_type == 'FARGATE':
if linux_param == 'sharedMemorySize' and launch_type == 'FARGATE':
module.fail_json(msg='sharedMemorySize parameter is not supported with the FARGATE launch type.')

if linux_param.get('tmpfs') and launch_type == 'FARGATE':
if linux_param == 'tmpfs' and launch_type == 'FARGATE':
module.fail_json(msg='tmpfs parameter is not supported with the FARGATE launch type.')

if container.get('hostname') and network_mode == 'awsvpc':
Expand Down Expand Up @@ -862,14 +887,24 @@ def _right_has_values_of_left(left, right):

for list_val in left_list:
if list_val not in right_list:
return False
# if list_val is the port mapping, the key 'protocol' may be absent (but defaults to 'tcp')
# fill in that default if absent and see if it is in right_list then
if isinstance(list_val, dict) and not list_val.get('protocol'):
modified_list_val = dict(list_val)
modified_list_val.update(protocol='tcp')
if modified_list_val in right_list:
continue
else:
return False

# Make sure right doesn't have anything that left doesn't
for k, v in right.items():
if v and k not in left:
return False
# 'essential' defaults to True when not specified
if k == 'essential' and v is True:
pass
else:
return False

return True

Expand Down
5 changes: 5 additions & 0 deletions tests/integration/targets/ecs_cluster/defaults/main.yml
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ ecs_service_name: "{{ resource_prefix }}-service"
ecs_task_image_path: nginx
ecs_task_name: "{{ resource_prefix }}-task"
ecs_task_memory: 128
target_swap_mb: 0
target_swappiness: 80
ecs_task_containers:
- name: "{{ ecs_task_name }}"
image: "{{ ecs_task_image_path }}"
Expand All @@ -15,6 +17,9 @@ ecs_task_containers:
portMappings:
- containerPort: "{{ ecs_task_container_port }}"
hostPort: "{{ ecs_task_host_port|default(0) }}"
linuxParameters:
maxSwap: "{{ target_swap_mb }}"
swappiness: "{{ target_swappiness }}"
mountPoints: "{{ ecs_task_mount_points|default([]) }}"
ecs_service_deployment_configuration:
minimum_healthy_percent: 0
Expand Down
26 changes: 11 additions & 15 deletions tests/integration/targets/ecs_cluster/tasks/full_test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@

- name: create subnets
ec2_vpc_subnet:
az: '{{ ec2_region }}{{ item.zone }}'
az: '{{ aws_region }}{{ item.zone }}'
tags:
Name: '{{ resource_prefix }}_ecs_cluster-subnet-{{ item.zone }}'
vpc_id: '{{ setup_vpc.vpc.id }}'
Expand Down Expand Up @@ -116,7 +116,7 @@
- name: provision ec2 instance to create an image
ec2_instance:
key_name: '{{ ec2_keypair|default(setup_key.key.name) }}'
instance_type: t2.micro
instance_type: t3.micro
state: present
image_id: '{{ ecs_image_id }}'
wait: yes
Expand Down Expand Up @@ -156,7 +156,7 @@
state: present
scheme: internal
security_groups: '{{ setup_sg.group_id }}'
subnets: "{{ setup_subnet.results | community.general.json_query('[].subnet.id') }}"
subnets: "{{ setup_subnet.results | map(attribute='subnet.id') | list }}"
listeners:
- Protocol: HTTP
Port: 80
Expand Down Expand Up @@ -187,8 +187,6 @@
assert:
that:
- not ecs_task_definition_again.changed
# FIXME: task definition should not change, will need #26752 or equivalent
ignore_errors: yes

- name: obtain ECS task definition facts
ecs_taskdefinition_info:
Expand Down Expand Up @@ -281,7 +279,7 @@
containerName: "{{ ecs_task_name }}"
containerPort: "{{ ecs_task_container_port }}"
network_configuration:
subnets: "{{ setup_subnet.results | community.general.json_query('[].subnet.id') }}"
subnets: "{{ setup_subnet.results | map(attribute='subnet.id') | list }}"
security_groups:
- '{{ setup_sg.group_id }}'
register: ecs_service_network_without_awsvpc_task
Expand Down Expand Up @@ -389,7 +387,7 @@
containerName: "{{ ecs_task_name }}"
containerPort: "{{ ecs_task_container_port }}"
network_configuration:
subnets: "{{ setup_subnet.results | community.general.json_query('[].subnet.id') }}"
subnets: "{{ setup_subnet.results | map(attribute='subnet.id') | list }}"
security_groups:
- '{{ setup_sg.group_id }}'
register: create_ecs_service_with_vpc
Expand Down Expand Up @@ -423,7 +421,7 @@
containerName: "{{ ecs_task_name }}"
containerPort: "{{ ecs_task_container_port }}"
network_configuration:
subnets: "{{ setup_subnet.results | community.general.json_query('[].subnet.id') }}"
subnets: "{{ setup_subnet.results | map(attribute='subnet.id') | list }}"
security_groups:
- "{{ resource_prefix }}-ecs-vpc-test-sg"
register: update_ecs_service_with_vpc
Expand Down Expand Up @@ -674,7 +672,7 @@
deployment_configuration: "{{ ecs_service_deployment_configuration }}"
launch_type: FARGATE
network_configuration:
subnets: "{{ setup_subnet.results | community.general.json_query('[].subnet.id') }}"
subnets: "{{ setup_subnet.results | map(attribute='subnet.id') | list }}"
security_groups:
- '{{ setup_sg.group_id }}'
assign_public_ip: true
Expand All @@ -693,7 +691,7 @@
launch_type: FARGATE
count: 1
network_configuration:
subnets: "{{ setup_subnet.results | community.general.json_query('[].subnet.id') }}"
subnets: "{{ setup_subnet.results | map(attribute='subnet.id') | list }}"
security_groups:
- '{{ setup_sg.group_id }}'
assign_public_ip: true
Expand Down Expand Up @@ -725,7 +723,7 @@
tag_key: tag_value
tag_key2: tag_value2
network_configuration:
subnets: "{{ setup_subnet.results | community.general.json_query('[].subnet.id') }}"
subnets: "{{ setup_subnet.results | map(attribute='subnet.id') | list }}"
security_groups:
- '{{ setup_sg.group_id }}'
assign_public_ip: true
Expand All @@ -752,7 +750,7 @@
tag_key: tag_value
tag_key2: tag_value2
network_configuration:
subnets: "{{ setup_subnet.results | community.general.json_query('[].subnet.id') }}"
subnets: "{{ setup_subnet.results | map(attribute='subnet.id') | list }}"
security_groups:
- '{{ setup_sg.group_id }}'
assign_public_ip: true
Expand All @@ -767,7 +765,7 @@
launch_type: FARGATE
count: 1
network_configuration:
subnets: "{{ setup_subnet.results | community.general.json_query('[].subnet.id') }}"
subnets: "{{ setup_subnet.results | map(attribute='subnet.id') | list }}"
security_groups:
- '{{ setup_sg.group_id }}'
assign_public_ip: false
Expand Down Expand Up @@ -857,8 +855,6 @@
ignore_errors: yes
register: ecs_service_scale_down



- name: scale down scheduling_strategy service
ecs_service:
name: "{{ ecs_service_name }}-replica"
Expand Down