Move introspection server to ecs-agent #4470
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
To make the Agent Introspection Server available to the Fargate agent, the introspection server is now implemented in the shared ecs-agent library. The ECS agent introspection server is refactored to use this shared implementation.
aaithal
reviewed
Jan 16, 2025
|
|
||
| mockStateResolver := mock_utils.NewMockDockerStateResolver(ctrl) | ||
|
|
||
| server, _ := introspection.NewServer(&v1.AgentStateImpl{ |
There was a problem hiding this comment.
multiple occurrences - why do we ignore the error returned from this method?
|
|
||
| server := introspectionServerSetup(containerInstanceArn, dockerTaskEngine, cfg) | ||
| server, _ := introspection.NewServer( |
There was a problem hiding this comment.
We need to deal with the error returned here.
| for ndx, task := range allTasks { | ||
| containerMap, ok := agentState.ContainerMapByArn(task.Arn) | ||
| if !ok { | ||
| return nil, introspection.NewErrorNotFound(fmt.Sprintf("Container map for task %s not found", task.Arn)) |
There was a problem hiding this comment.
multiple occurrences - error strings should begin with lower case letters. See https://google.github.io/styleguide/go/decisions.html#error-strings
| func (as *AgentStateImpl) GetTaskMetadataByShortID(shortDockerID string) (*introspection.TaskResponse, error) { | ||
| agentState := as.TaskEngine.State() | ||
| tasks, _ := agentState.TaskByShortID(shortDockerID) | ||
| if len(tasks) == 0 { |
There was a problem hiding this comment.
you don't need this check. you just need to use the bool that
returns.| tasks, _ := agentState.TaskByShortID(shortDockerID) | ||
| if len(tasks) == 0 { | ||
| return nil, introspection.NewErrorNotFound(fmt.Sprintf("Task %s not found", shortDockerID)) | ||
| } else if len(tasks) > 1 { |
There was a problem hiding this comment.
Why do you need this check?
| } | ||
|
|
||
| // getErrorResponse returns an appropriate HTTP response status code and body for the error. | ||
| func getErrorResponse(err error) (int, string) { |
There was a problem hiding this comment.
Can we rename this as getHTTPErrorCode()? getErrorResponse is way too generic for what this method is doing.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
As part of the effort to bring the Agent Introspection Server to the Fargate Agent, this change moves the server's implementation to the shared
ecs-agentlibrary.Implementation details
The http server logic for the introspection server is moved to the shared
ecs-agentlibrary. This refactored server is responsible for handling http requests and generating responses. The server defines anAgentStateinterface which will be implemented in the ECS and Fargate agents. TheAgentStateimplementations will supply the server with the data it needs to construct responses. This change also includes the ECS agent's implementation ofAgentState.Testing
Unit and integration tests
New unit and integration tests were added to the
introspectionpackage inecs-agent. Where applicable, existing tests were updated. Existingunittests forintrospection_serverinagent/handlers/v1were moved to anintegrationtest file with the intent to:Manual tests
Outputs from the Agent introspection server were recorded from two test clusters: one using the current version of the ECS agent, and the other using a modified build with the shared introspection server implementation. E.g.,
/v1/metadataCurrent implementation
Shared implementation
New tests cover the changes: yes
Description for the changelog
Feature - Move agent introspection server to shared library
Additional Information
Does this PR include breaking model changes? If so, Have you added transformation functions?
No
Does this PR include the addition of new environment variables in the README?
No
Licensing
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.