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

Consume full task sync ACS responders from ecs-agent/ #3854

Merged
merged 1 commit into from
Aug 17, 2023

Conversation

danehlim
Copy link
Contributor

@danehlim danehlim commented Aug 17, 2023

Summary

Consume the task manifest responder and task stop verification ACK responder defined in ecs-agent module. These ACS message responders are used to support the full task sync feature.

Implementation details

Please note that, in the context of this PR, "ACS message responder"/"responder" is more or less synonymous with "ACS message handler"/"handler".

  • Create new file agent/acs/handler/task_manifest_responder.go and inside that file, implement TaskComparer and SequenceNumberAccessor interfaces defined in ecs-agent module. Some parts of the implementation are based off of functionality that exists in the current agent/acs/handler/task_manifest_handler.go file
  • Create new file agent/acs/handler/task_stop_verification_ack_responder.go and inside that file, implement TaskStopper interface defined in ecs-agent module. Some parts of the implementation are based off of functionality that exists in the current agent/acs/handler/task_manifest_handler.go file
  • Add tests for the above in new files agent/acs/handler/task_manifest_responder_test.go and agent/acs/handler/task_stop_verification_ack_responder_test.go. Some tests are based off of tests that exist in the current agent/acs/handler/task_manifest_handler_test.go file
  • Delete agent/acs/handler/task_manifest_handler.go and agent/acs/handler/task_manifest_handler_test.go
  • In agent/acs/handler/acs_handler.go:
    • Initialize task manifest responder and add its HandlerFunc function as a request handler to the ACS client, such that task manifest responder dictates what to do in response to receiving task manifest messages from ACS
    • Initialize task stop verification ACK responder and add its HandlerFunc function as a request handler to the ACS client, such that task stop verification ACK responder dictates what to do in response to receiving task stop verification ACKs from ACS
      • NOTE: On L274, you will notice we are passing nil as the respond function parameter. This is because although the task stop verification ACK responder in ecs-agent module currently takes in and contains a respond function parameter, it actually doesn't use it since we do not send anything back to ACS in response to task stop verification ACKs. I've left removing this non-necessity from the task stop verification ACK responder in the ecs-agent module out of scope of this pull request as this pull request is limited to and focuses on changes in the agent module
    • Remove ACS messages related to full task sync feature from pending ACKs logic since it is no longer applicable/used after consuming the task manifest and task stop verification ACK responders from ecs-agent module
      • Such logic is no longer applicable since there is no need to do multi-thread synchronization with channels with this responder (like we currently do in agent/acs/handler/task_manifest_handler.go) and the responders in the ecs-agent module factor out this non-necessity and are a lot cleaner and simpler to follow.
        For reference see the following PR for a similar change previously made to the heartbeat responder for the same reason: Fix flakey leak test for ACS Handler #3232
    • Remove pending ACKs logic altogether since there are no longer any responders that use it now

Testing

Unit, integration, and functional tests.

New tests cover the changes: yes

Description for the changelog

Consume full task sync ACS responders from ecs-agent/

Licensing

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@danehlim danehlim marked this pull request as ready for review August 17, 2023 17:07
@danehlim danehlim requested a review from a team as a code owner August 17, 2023 17:07
@danehlim danehlim merged commit 4a0b1b1 into aws:dev Aug 17, 2023
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants