fabfuel / ecs-deploy

Powerful CLI tool to simplify Amazon ECS deployments, rollbacks & scaling
Other
843 stars 145 forks source link

Introduction of task_cpu and task_memory is breaking deploy in 1.13 #204

Closed gdesmarais-ctx closed 2 years ago

gdesmarais-ctx commented 2 years ago

It looks like the addition of the two cli options task-cpu and task-memory in https://github.com/fabfuel/ecs-deploy/blob/84c1b5d527ddced1a1189b1cf0b960dd6cc87585/ecs_deploy/cli.py (commit https://github.com/fabfuel/ecs-deploy/commit/84c1b5d527ddced1a1189b1cf0b960dd6cc87585) might be setting up for failure if you don't specify those values.

When running (env vars are same as previous versions)

  ecs deploy "${_ecs_cluster_name}" "${_ecs_service_name}" --tag "$_deployment_env" --timeout -1

With ecs-deploy==1.13.0 results in the following errors:

Invalid type for parameter cpu, value: None, type: <class 'NoneType'>, valid types: <class 'str'>
Invalid type for parameter memory, value: None, type: <class 'NoneType'>, valid types: <class 'str'>

Full stack trace at bottom of this.

I believe the issue is that the cli options will default to none https://github.com/fabfuel/ecs-deploy/blob/84c1b5d527ddced1a1189b1cf0b960dd6cc87585/ecs_deploy/cli.py#L183-184

@click.option('--task-cpu', type=int, help='Overwrites the cpu value for a task: <cpu>')
@click.option('--task-memory', type=int, help='Overwrites the memory value for a task: <memory>')

and then the cli sets the values in the new task definition like: https://github.com/fabfuel/ecs-deploy/blob/84c1b5d527ddced1a1189b1cf0b960dd6cc87585/ecs_deploy/cli.py#L243-244

        td.set_cpu(**{key: value for (key, value) in cpu})
        td.set_memory(**{key: value for (key, value) in memory})

If you don't pass the parameters, the values are None, which seems to then cause the task definition to be invalid. I expect the right solution is to wrap the two set_task_* lines in something like

if task_cpu:
   td.set_task_cpu(task_cpu)

Full stack trace:


Deploying service cdex_uat_svc on the ecs cluster celsius-apps-uat
960 | Deploying based on task definition: celsius-apps-uat-EcsCdexTaskDefinition-wTQqptAGJrSC:35
961 |  
962 | Updating task definition
963 | Changed image of container "cdex-uat" to: "386834949250.dkr.ecr.us-east-1.amazonaws.com/datasciences-dashboard:uat" (was: "386834949250.dkr.ecr.us-east-1.amazonaws.com/datasciences-dashboard:uat")
964 |  
965 | Creating new task definition revision
966 | Traceback (most recent call last):
967 | File "/root/.pyenv/versions/3.7.10/bin/ecs", line 8, in <module>
968 | sys.exit(ecs())
969 | File "/root/.pyenv/versions/3.7.10/lib/python3.7/site-packages/click/core.py", line 829, in __call__
970 | return self.main(*args, **kwargs)
971 | File "/root/.pyenv/versions/3.7.10/lib/python3.7/site-packages/click/core.py", line 782, in main
972 | rv = self.invoke(ctx)
973 | File "/root/.pyenv/versions/3.7.10/lib/python3.7/site-packages/click/core.py", line 1259, in invoke
974 | return _process_result(sub_ctx.command.invoke(sub_ctx))
975 | File "/root/.pyenv/versions/3.7.10/lib/python3.7/site-packages/click/core.py", line 1066, in invoke
976 | return ctx.invoke(self.callback, **ctx.params)
977 | File "/root/.pyenv/versions/3.7.10/lib/python3.7/site-packages/click/core.py", line 610, in invoke
978 | return callback(*args, **kwargs)
979 | File "/root/.pyenv/versions/3.7.10/lib/python3.7/site-packages/ecs_deploy/cli.py", line 139, in deploy
980 | new_td = create_task_definition(deployment, td)
981 | File "/root/.pyenv/versions/3.7.10/lib/python3.7/site-packages/ecs_deploy/cli.py", line 580, in create_task_definition
982 | new_td = action.update_task_definition(task_definition)
983 | File "/root/.pyenv/versions/3.7.10/lib/python3.7/site-packages/ecs_deploy/ecs.py", line 1321, in update_task_definition
984 | memory=task_definition.memory
985 | File "/root/.pyenv/versions/3.7.10/lib/python3.7/site-packages/ecs_deploy/ecs.py", line 98, in register_task_definition
986 | **additional_properties
987 | File "/root/.pyenv/versions/3.7.10/lib/python3.7/site-packages/botocore/client.py", line 395, in _api_call
988 | return self._make_api_call(operation_name, kwargs)
989 | File "/root/.pyenv/versions/3.7.10/lib/python3.7/site-packages/botocore/client.py", line 696, in _make_api_call
990 | api_params, operation_model, context=request_context)
991 | File "/root/.pyenv/versions/3.7.10/lib/python3.7/site-packages/botocore/client.py", line 746, in _convert_to_request_dict
992 | api_params, operation_model)
993 | File "/root/.pyenv/versions/3.7.10/lib/python3.7/site-packages/botocore/validate.py", line 360, in serialize_to_request
994 | raise ParamValidationError(report=report.generate_report())
995 | botocore.exceptions.ParamValidationError: Parameter validation failed:
996 | Invalid type for parameter cpu, value: None, type: <class 'NoneType'>, valid types: <class 'str'>
997 | Invalid type for parameter memory, value: None, type: <class 'NoneType'>, valid types: <class 'str'>
998
jliocsar commented 2 years ago

We're having the same issue on two different apps too.

fabfuel commented 2 years ago

I'm looking into It already, I can reproduce it now.

luizfm commented 2 years ago

We're facing the same problem here. At least, if you don't want to set those cli options, you can run your deploy using the last version before update: ecs-deploy==1.12.1

Don't forget to do that for every environment that you have for the project like: staging and production.

henriquesalvaro commented 2 years ago

Same thing for the addition of the runtimePlatform option, btw. Added in this commit I believe: https://github.com/fabfuel/ecs-deploy/commit/b285a1facfd62b727538921410285a33fc994869

jliocsar commented 2 years ago

Thank you for the quick fix @fabfuel :love_you_gesture:

fabfuel commented 2 years ago

I have pushed a hotfix to PyPI: 1.13.1

It solves the problem, when an old task definition has no cpu, memory or runtimePlatform defined. Thanks for the detailed error report @gdesmarais-ctx! And thanks for the additional hint @henriquesalvaro

Hope, that everything is back to normal now?! 😅