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

Refactor ACS payload message handling #3840

Merged
merged 1 commit into from
Aug 10, 2023
Merged

Conversation

danehlim
Copy link
Contributor

@danehlim danehlim commented Aug 4, 2023

Summary

Refactor ACS payload message handling, update other associated code, and improve/update tests.

Implementation details

  1. Refactor ACS payload message handling
    • agent/acs/handler/payload_handler.go -> ecs-agent/acs/session/payload_responder.go (partial)
      • Components of the responder that do not interact with Agent state belong in this file
      • Refactor payloadRequestHandler struct and existing code into payloadResponder struct, such that payloadResponder struct implements the wsclient.RequestResponder interface
      • Define and use new validatePayloadMessagefunction to do some ACS payload message validation upfront
      • Handle the payload message using the ProcessMessage method of the responder's PayloadMessageHandler
      • Use github.com/aws/amazon-ecs-agent/ecs-agent/logger instead of github.com/cihub/seelog for logging
      • Use errors.Errorf in favor of fmt.Errorf
      • Rename method handleUnrecognizedTask to more suitable name handleInvalidTask
    • agent/acs/handler/payload_handler.go -> agent/acs/handler/payload_responder.go (partial)
      • Components of the responder that interact with Agent state belong in this file
      • Refactor functionality from such components mentioned above into payloadMessageHandler struct such that payloadMessageHandler struct implements the PayloadMessageHandler interface defined in ecs-agent/acs/session/payload_responder.go
      • Use github.com/aws/amazon-ecs-agent/ecs-agent/logger instead of github.com/cihub/seelog for logging
  2. Update other associated files
    • agent/acs/handler/acs_handler.go
      • Initialize payload responder and add its HandlerFunc function as a request handler to the ACS client, such that payload responder dictates what to do in response to receiving payload messages from ACS
      • Remove payload messages from pending ACKs logic and payloadMessageBufferSize constant since they are no longer applicable/used after the refactor of ACS payload message handling
        • 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). As part of the refactor, we factor out this non-necessity and thus methods such as start(), stop(), clearAcks(), etc. can be removed. The resulting responder is 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
    • ecs-agent/acs/session/generate_mocks.go and ecs-agent/acs/session/mocks/session_mock.go
      • Generate new mocks for the PayloadMessageHandler interface to be used in testing
    • ecs-agent/acs/logger/field/constants.go
      • Create and use logger field for logging credentials ID
      • Create and use logger field for logging task version
  3. Improve/update tests, most notably:
    • Update existing tests accordingly based off of above changes to refactor ACS payload message handling in the following files:
      • agent/acs/handler/payload_handler_test.go -> ecs-agent/acs/session/payload_responder.go (partial)
        • Payload responder tests that only need to interact with stateless components (e.g., message validation) belong in this file
      • agent/acs/handler/payload_handler_test.go -> agent/acs/handler/payload_responder_test.go (partial)
        • Payload responder tests that need to directly interact with Agent state belong in this file
      • agent/acs/handler/acs_handler_test.go
    • Update existing tests in agent/acs/handler/payload_handler_test.go accordingly based off of above changes to agent/acs/handler/payload_handler.go
    • General cleanup/quality of life updates to tests (e.g., use table testing when possible, eliminate redundant initialization of test payloadMessage variables for each payload responder test that requires one)
    • Use testconst.MessageID test constant defined in ecs-agent/acs/session/testconst/test_const.go in favor of payloadMessageId test constant currently defined in agent/acs/handler/payload_handler_test.go

Testing

Unit, integration, and functional tests.

New tests cover the changes: yes

Manually started and stopped task using a custom Agent with the changes from this pull request built into it. Both operations completed in a reasonable amount of time.

Description for the changelog

Refactor ACS payload message handling

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 4, 2023 20:44
@danehlim danehlim requested a review from a team as a code owner August 4, 2023 20:44
agent/acs/handler/payload_responder.go Show resolved Hide resolved
agent/acs/handler/payload_responder.go Show resolved Hide resolved
agent/acs/handler/payload_responder.go Outdated Show resolved Hide resolved
agent/acs/handler/payload_responder.go Outdated Show resolved Hide resolved
agent/acs/handler/payload_responder.go Show resolved Hide resolved
agent/acs/handler/payload_responder.go Outdated Show resolved Hide resolved
ecs-agent/acs/session/payload_responder.go Outdated Show resolved Hide resolved
ecs-agent/acs/session/payload_responder.go Outdated Show resolved Hide resolved
agent/acs/handler/payload_responder.go Outdated Show resolved Hide resolved
ecs-agent/acs/session/payload_responder.go Show resolved Hide resolved
ecs-agent/acs/session/payload_responder.go Outdated Show resolved Hide resolved
ecs-agent/acs/session/payload_responder.go Show resolved Hide resolved
ecs-agent/acs/session/payload_responder.go Show resolved Hide resolved
ecs-agent/acs/session/payload_responder.go Outdated Show resolved Hide resolved
ecs-agent/acs/session/payload_responder.go Show resolved Hide resolved
agent/acs/handler/payload_responder.go Show resolved Hide resolved
@RichaGangwar RichaGangwar self-requested a review August 10, 2023 00:22
Copy link
Contributor

@RichaGangwar RichaGangwar left a comment

Choose a reason for hiding this comment

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

Looks good to me. :)

@danehlim danehlim merged commit d46df70 into aws:dev Aug 10, 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

4 participants