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 common ENI attachment functionality #3685

Merged
merged 2 commits into from
May 12, 2023

Conversation

danehlim
Copy link
Contributor

@danehlim danehlim commented May 9, 2023

Summary

Refactor functionality common to handling/responding to attach instance ENI and attach task ENI messages from ACS and other associated code.

Prerequisite: Move arn and ttime utils from agent/utils to the ecs-agent module. These utils will be used by functionality being moved to ecs-agent module as part of this pull request.

Implementation details

This pull request has been split into two commits for easier readability of separation between the prerequisite changes and main changes.

--- COMMIT 1 (click here to view associated changes) ---

  1. [Prerequisite] Move arn and ttime utils to ecs-agent module
    • agent/utils/arn.go -> ecs-agent/utils/arn/arn.go
    • agent/utils/ttime/generate_mocks.go -> ecs-agent/utils/ttime/generate_mocks.go
    • agent/utils/ttime/mocks/time_mocks.go -> ecs-agent/utils/ttime/mocks/time_mocks.go
    • agent/utils/ttime/ttime.go -> ecs-agent/utils/ttime/ttime.go
    • Update dependent imports accordingly based off of the above modifications

--- COMMIT 2 (click here to view associated changes) ---

  1. Refactor agent/api/eni and move to ecs-agent/
    • agent/api/eni/eni.go -> ecs-agent/api/eni/eni.go
      • Use github.com/aws/amazon-ecs-agent/ecs-agent/logger instead of github.com/cihub/seelog for logging
    • agent/api/eni/eni_test.go -> ecs-agent/api/eni/eni_test.go
    • agent/api/eni/enistatus.go -> ecs-agent/api/status/attachment_status.go
      • Update statuses to be more general and extensible and allow for use with other resource attachments (i.e., not specific to ENI attachments)
    • agent/api/eni/enistatus_test.go -> ecs-agent/api/status/attachment_status_test.go
      • Update tests to use general attachment statuses
    • agent/api/eni/eniattachment.go -> ecs-agent/api/eni/eniattachment.go
      • Define new AttachmentInfo struct in ecs-agent/api/attachmentinfo/attachment_info.go file, which contains general fields that can be common to other resource attachments (i.e., not specific to ENI attachments)
      • Remove overlapping fields between AttachmentInfo and ENIAttachment structs from ENIAttachment struct, and then have ENIAttachment struct embed the AttachmentInfo struct
    • agent/api/eni/eniattachment_test.go -> ecs-agent/api/eni/eniattachment_test.go
      • Update tests to use general attachment statuses and embedded AttachmentInfo struct
  2. Update files previously dependent on agent/api/eni to account for previous implementation detail's changes
  3. Define new ENIHandler interface in ecs-agent/acs/session/attach_eni_common.go to abstract Agent state when handling ENI attachments
  4. Refactor attach ENI functionality in agent/acs/handler
    • agent/acs/handler/attach_eni_handler_common.go
      • Change existing ackTimeoutHandler struct to eniHandler struct and refactor handleENIAttachment() and addENIAttachmentToState() functions into eniHandler struct methods, such that eniHandler struct implements ENIHandler interface
    • Update the following files to make use of ENIHandler interface and/or eniHandler struct
      • agent/acs/handler/acs_handler.go
      • agent/acs/handler/attach_eni_handler_common_test.go
      • agent/acs/handler/attach_instance_eni_handler.go
      • agent/acs/handler/attach_instance_eni_handler_test.go
      • agent/acs/handler/attach_task_eni_handler.go
      • agent/acs/handler/attach_task_eni_handler_test.go

Testing

Unit, integration, and functional tests.

Description for the changelog

Refactor common ENI attachment functionality

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 May 9, 2023 23:23
@danehlim danehlim requested a review from a team as a code owner May 9, 2023 23:23
sparrc
sparrc previously approved these changes May 10, 2023
sparrc
sparrc previously approved these changes May 11, 2023
@danehlim danehlim dismissed stale reviews from YashdalfTheGray and sparrc via e741926 May 11, 2023 20:34
@danehlim danehlim force-pushed the attach-eni-common branch from eb5e733 to e741926 Compare May 11, 2023 20:34
Copy link
Contributor

@YashdalfTheGray YashdalfTheGray left a comment

Choose a reason for hiding this comment

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

Approving after rebase

@danehlim danehlim merged commit 947efd8 into aws:dev May 12, 2023
@ubhattacharjya ubhattacharjya mentioned this pull request May 24, 2023
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.

5 participants