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

ecs_service planning #1142

Open
8 of 10 tasks
markuman opened this issue May 10, 2022 · 10 comments
Open
8 of 10 tasks

ecs_service planning #1142

markuman opened this issue May 10, 2022 · 10 comments
Labels
easyfix Good for new comers and easy to start with contribution feature This issue/PR relates to a feature request good first issue

Comments

@markuman
Copy link
Member

markuman commented May 10, 2022 •
edited
Loading

Summary

ecs_service needs some care

the three most important ToDos

easy, good first issues

  • snake_dict parameter output of
    • ecs_service
    • ecs_service_info
  • service and name parameter/aliases in ecs_service and ecs_service_info
    • bring the modules closer to each other that they support the same parameter/aliases for the ecs service name
  • limit placement_constraints to max 10 (verify what docs told)

features

bugs
@markuman
Copy link
Member Author

bot_skip

@markuman
Copy link
Member Author

everyone who is bored can join

@markuman markuman mentioned this issue May 11, 2022
Merged
@jatorcasso
Copy link
Contributor

@markuman I can poke at a few of these after #1145 gets merged

@markuman markuman added good first issue easyfix Good for new comers and easy to start with contribution bug This issue/PR relates to a bug feature This issue/PR relates to a feature request labels May 19, 2022
softwarefactory-project-zuul bot pushed a commit that referenced this issue May 23, 2022
@markuman
fix ecs_cluster integration test (#1145)
35f4f59 
fix ecs_cluster integration test

SUMMARY

ecs_cluster: make ecs_cluster integration test work again - as it is

bugs I've hit and must be fixed to complete this challenge

ecs_taskdefinition: fix change detection of changing launch_type parameter
ecs_service:

compare of task_definition never works and results always in a changed task
change detect of health_check_grace_period_seconds was never implemented, but tested and failing, after the task_definition is compared correctly







ref: #1142
ISSUE TYPE


Bugfix Pull Request

COMPONENT NAME
ecs_taskdefinition
ecs_service
ADDITIONAL INFORMATION
basically the existing test tasks are not changed. just sorted and removed what was marked as fixme because it's simple not possible (changing network settings of a created service).

Reviewed-by: Alina Buzachis <None>
Reviewed-by: Joseph Torcasso <None>
Reviewed-by: Mark Chappell <None>
Reviewed-by: Markus Bergholz <git@osuv.de>
patchback bot pushed a commit that referenced this issue May 23, 2022
@markuman @patchback
fix ecs_cluster integration test (#1145)
15f87b8 
fix ecs_cluster integration test

SUMMARY

ecs_cluster: make ecs_cluster integration test work again - as it is

bugs I've hit and must be fixed to complete this challenge

ecs_taskdefinition: fix change detection of changing launch_type parameter
ecs_service:

compare of task_definition never works and results always in a changed task
change detect of health_check_grace_period_seconds was never implemented, but tested and failing, after the task_definition is compared correctly

ref: #1142
ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME
ecs_taskdefinition
ecs_service
ADDITIONAL INFORMATION
basically the existing test tasks are not changed. just sorted and removed what was marked as fixme because it's simple not possible (changing network settings of a created service).

Reviewed-by: Alina Buzachis <None>
Reviewed-by: Joseph Torcasso <None>
Reviewed-by: Mark Chappell <None>
Reviewed-by: Markus Bergholz <git@osuv.de>
(cherry picked from commit 35f4f59)
@patchback patchback bot mentioned this issue May 23, 2022
Merged
patchback bot pushed a commit that referenced this issue May 23, 2022
@markuman @patchback
fix ecs_cluster integration test (#1145)
5b4ef11 
fix ecs_cluster integration test

SUMMARY

ecs_cluster: make ecs_cluster integration test work again - as it is

bugs I've hit and must be fixed to complete this challenge

ecs_taskdefinition: fix change detection of changing launch_type parameter
ecs_service:

compare of task_definition never works and results always in a changed task
change detect of health_check_grace_period_seconds was never implemented, but tested and failing, after the task_definition is compared correctly

ref: #1142
ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME
ecs_taskdefinition
ecs_service
ADDITIONAL INFORMATION
basically the existing test tasks are not changed. just sorted and removed what was marked as fixme because it's simple not possible (changing network settings of a created service).

Reviewed-by: Alina Buzachis <None>
Reviewed-by: Joseph Torcasso <None>
Reviewed-by: Mark Chappell <None>
Reviewed-by: Markus Bergholz <git@osuv.de>
(cherry picked from commit 35f4f59)
@patchback patchback bot mentioned this issue May 23, 2022
Merged
softwarefactory-project-zuul bot pushed a commit that referenced this issue May 25, 2022
@patchback
fix ecs_cluster integration test (#1145) (#1154)
236f520 
[PR #1145/35f4f592 backport][stable-3] fix ecs_cluster integration test

This is a backport of PR #1145 as merged into main (35f4f59).
SUMMARY

ecs_cluster: make ecs_cluster integration test work again - as it is

bugs I've hit and must be fixed to complete this challenge

ecs_taskdefinition: fix change detection of changing launch_type parameter
ecs_service:

compare of task_definition never works and results always in a changed task
change detect of health_check_grace_period_seconds was never implemented, but tested and failing, after the task_definition is compared correctly







ref: #1142
ISSUE TYPE


Bugfix Pull Request

COMPONENT NAME
ecs_taskdefinition
ecs_service
ADDITIONAL INFORMATION
basically the existing test tasks are not changed. just sorted and removed what was marked as fixme because it's simple not possible (changing network settings of a created service).

Reviewed-by: Markus Bergholz <git@osuv.de>
softwarefactory-project-zuul bot pushed a commit that referenced this issue May 25, 2022
@patchback
fix ecs_cluster integration test (#1145) (#1153)
c36f8db 
[PR #1145/35f4f592 backport][stable-2] fix ecs_cluster integration test

This is a backport of PR #1145 as merged into main (35f4f59).
SUMMARY

ecs_cluster: make ecs_cluster integration test work again - as it is

bugs I've hit and must be fixed to complete this challenge

ecs_taskdefinition: fix change detection of changing launch_type parameter
ecs_service:

compare of task_definition never works and results always in a changed task
change detect of health_check_grace_period_seconds was never implemented, but tested and failing, after the task_definition is compared correctly







ref: #1142
ISSUE TYPE


Bugfix Pull Request

COMPONENT NAME
ecs_taskdefinition
ecs_service
ADDITIONAL INFORMATION
basically the existing test tasks are not changed. just sorted and removed what was marked as fixme because it's simple not possible (changing network settings of a created service).

Reviewed-by: Markus Bergholz <git@osuv.de>
@jatorcasso
Copy link
Contributor

jatorcasso commented May 25, 2022 •
edited
Loading

@markuman where are waiters needed here? are there specific test cases I can look at to validate the waiters do their job? I see for scaling down services

@markuman
Copy link
Member Author

markuman commented May 26, 2022 •
edited
Loading

@markuman where are waiters needed here? are there specific test cases I can look at to validate the waiters do their job? I see for scaling down services

yes. one case is when the service is state: absent, it must wait until the service is inactive.
it would make this retry https://github.com/ansible-collections/community.aws/blob/main/tests/integration/targets/ecs_cluster/tasks/main.yml#L983-L991 superfluous, because the cluster can only be removed when no service is available anymore.
wait: yes in this tasks inside the always block: https://github.com/ansible-collections/community.aws/blob/main/tests/integration/targets/ecs_cluster/tasks/main.yml#L877-L916

another case is when the desired_count changes. https://github.com/ansible-collections/community.aws/blob/main/tests/integration/targets/ecs_cluster/tasks/main.yml#L301-L319
or when the service is created. it must wait until the service is table: https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/ecs.html#ECS.Waiter.ServicesStable
this would also solves this PR: #91

@markuman
Copy link
Member Author

something else that might be need to be discussed is the format of the task_definition.

The input format is documented like this: https://github.com/ansible-collections/community.aws/blob/main/plugins/modules/ecs_service.py#L214

    task_definition: 'new_cluster-task:1'

But the output is returning just the arn of the task_definition:

    taskDefinition: arn:aws:ecs:eu-central-1:123456789:task-definition/new_cluster-task:1

Shall we support both as valid input formats?

@tremble
Copy link
Contributor

tremble commented May 30, 2022

Shall we support both as valid input formats?

Having dealt with all sorts of cross-account fun in the past few years, I'm a fan of supporting full ARNs in addition to just the names.

@markuman markuman mentioned this issue May 31, 2022
Merged
softwarefactory-project-zuul bot pushed a commit that referenced this issue Jun 1, 2022
@markuman
ecs_service and ecs_service_info: add name and service aliases (#1187)
029333b 
ecs_service and ecs_service_info: add name and service aliases

SUMMARY
while ecs_service is using name for the service name parameter, ecs_service_info is using service for the same purpose.
this PR adds just aliases to both modules, to use the same parameter to address the ecs service name.
ref #1142
ISSUE TYPE


Bugfix Pull Request
Docs Pull Request
Feature Pull Request

COMPONENT NAME
ecs_service
ecs_service_info

Reviewed-by: Alina Buzachis <None>
Reviewed-by: Joseph Torcasso <None>
Reviewed-by: Mark Chappell <None>
patchback bot pushed a commit that referenced this issue Jun 1, 2022
@markuman @patchback
ecs_service and ecs_service_info: add name and service aliases (#1187)
65e9b24 
ecs_service and ecs_service_info: add name and service aliases

SUMMARY
while ecs_service is using name for the service name parameter, ecs_service_info is using service for the same purpose.
this PR adds just aliases to both modules, to use the same parameter to address the ecs service name.
ref #1142
ISSUE TYPE

Bugfix Pull Request
Docs Pull Request
Feature Pull Request

COMPONENT NAME
ecs_service
ecs_service_info

Reviewed-by: Alina Buzachis <None>
Reviewed-by: Joseph Torcasso <None>
Reviewed-by: Mark Chappell <None>
(cherry picked from commit 029333b)
@patchback patchback bot mentioned this issue Jun 1, 2022
Merged
patchback bot pushed a commit that referenced this issue Jun 1, 2022
@markuman @patchback
ecs_service and ecs_service_info: add name and service aliases (#1187)
4f54e7d 
ecs_service and ecs_service_info: add name and service aliases

SUMMARY
while ecs_service is using name for the service name parameter, ecs_service_info is using service for the same purpose.
this PR adds just aliases to both modules, to use the same parameter to address the ecs service name.
ref #1142
ISSUE TYPE

Bugfix Pull Request
Docs Pull Request
Feature Pull Request

COMPONENT NAME
ecs_service
ecs_service_info

Reviewed-by: Alina Buzachis <None>
Reviewed-by: Joseph Torcasso <None>
Reviewed-by: Mark Chappell <None>
(cherry picked from commit 029333b)
@patchback patchback bot mentioned this issue Jun 1, 2022
Merged
softwarefactory-project-zuul bot pushed a commit that referenced this issue Jun 2, 2022
@patchback
ecs_service and ecs_service_info: add name and service aliases (#1187) (
b3308a3 
#1193)

[PR #1187/029333b3 backport][stable-3] ecs_service and ecs_service_info: add name and service aliases

This is a backport of PR #1187 as merged into main (029333b).
SUMMARY
while ecs_service is using name for the service name parameter, ecs_service_info is using service for the same purpose.
this PR adds just aliases to both modules, to use the same parameter to address the ecs service name.
ref #1142
ISSUE TYPE


Bugfix Pull Request
Docs Pull Request
Feature Pull Request

COMPONENT NAME
ecs_service
ecs_service_info

Reviewed-by: Alina Buzachis <None>
softwarefactory-project-zuul bot pushed a commit that referenced this issue Jun 2, 2022
@patchback
ecs_service and ecs_service_info: add name and service aliases (#1187) (
f23fad2 
#1192)

[PR #1187/029333b3 backport][stable-2] ecs_service and ecs_service_info: add name and service aliases

This is a backport of PR #1187 as merged into main (029333b).
SUMMARY
while ecs_service is using name for the service name parameter, ecs_service_info is using service for the same purpose.
this PR adds just aliases to both modules, to use the same parameter to address the ecs service name.
ref #1142
ISSUE TYPE


Bugfix Pull Request
Docs Pull Request
Feature Pull Request

COMPONENT NAME
ecs_service
ecs_service_info

Reviewed-by: Markus Bergholz <git@osuv.de>
@jatorcasso jatorcasso mentioned this issue Jun 3, 2022
Merged
@patchback patchback bot mentioned this issue Jun 29, 2022
Merged
softwarefactory-project-zuul bot pushed a commit that referenced this issue Jun 29, 2022
@patchback
ecs_* - add waiters (#1209) (#1274)
974c3f0 
[PR #1209/cff148f0 backport][stable-4] ecs_* - add waiters

This is a backport of PR #1209 as merged into main (cff148f).
Originally-Depends-On: #1212
SUMMARY

Add wait parameter to utilize boto3 waiters in ecs_service and ecs_task (ServicesInactive, TasksStopped, TasksRunning).
There's an additional waiter for ServicesStable but idempotence checked never failed locally so it seems redundant when creating a service.

Ref #1142
ISSUE TYPE

Feature Pull Request

COMPONENT NAME

ecs_service
ecs_task

ADDITIONAL INFORMATION
When testing the waiter for TasksRunning, tests failed on waiter error due to the container instance not being able to be created, not because of the waiter, so I commented out those tests for now.
In the ECS console:
Stopped reason CannotPullContainerError: inspect image has been retried 5 time(s): failed to resolve ref "docker.io/library/nginx:latest": failed to do request: Head https://registry-1.docker.io/v2/library/nginx/manifests/latest: dial tcp 34.237.244.67:443: i/o timeout

Reviewed-by: Mark Chappell <None>
@jatorcasso
Copy link
Contributor

@ingmarfjolla this could be a good first contribution for ya

@tremble tremble removed the bug This issue/PR relates to a bug label Jul 11, 2022
@alinabuzachis
Copy link
Contributor

@markuman Can you please check what's missing in here?

@markuman
Copy link
Member Author

@markuman Can you please check what's missing in here?

@alinabuzachis done. only two items are left. but they are more cosmetic in nature.

abikouo pushed a commit to abikouo/community.aws that referenced this issue Oct 24, 2023
@tremble
Update docs workflows to use intersphinx links instead of hard coding…
226ea7c 
… 'fake' module links (ansible-collections#1142)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
easyfix Good for new comers and easy to start with contribution feature This issue/PR relates to a feature request good first issue
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@tremble @markuman @alinabuzachis @jatorcasso