-
Notifications
You must be signed in to change notification settings - Fork 619
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
test: add functional test for agent introspection endpoint #1485
test: add functional test for agent introspection endpoint #1485
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just few questions, and thanks for writing such a thorough functional test.
"image": "amazon/amazon-ecs-agent-introspection-validator:make", | ||
"name": "agent-introspection-validator", | ||
"memory": 50, | ||
"healthCheck": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this test need to have a healthcheck?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this is unnecessary, I will remove it.
"startPeriod": 1 | ||
}, | ||
"logConfiguration": { | ||
"logDriver": "awslogs", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are we configuring this task to use awslogs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the logs in misc/agent-introspection-validator/agent-introspection-validator.go
needs be to be uploaded to Cloud Watch.
|
||
// ContainerResponse is the schema for the container response JSON object | ||
type ContainerResponse struct { | ||
DockerID string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just making a note since i asked you offline, this should be DockerId
to match the original API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I was planning to update it when the fix PR is rebased, will add a TODO in the PR description.
dda680a
to
1097ea3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor comment, but lgtm
return nil, err | ||
} | ||
|
||
fmt.Printf("Received tasks metadata: %s \n", string(body)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should these fmt.Printfs be logging statements?
seelog.Infof("Received tasks metadata: %s \n", string(body)) | ||
|
||
var tasksMetadata TasksResponse | ||
err = json.Unmarshal(body, &tasksMetadata) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unmarshal could also work with case insensitive, this won't capture the bug, example: https://play.golang.org/p/iH8nsK-UJ_L
4465113
to
fbd2072
Compare
@richardpen I have refactored accordingly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly looks good, minor comments
func TestAgentIntrospectionValidator(t *testing.T) { | ||
// The Docker version was 17.03.1-ce when we added changes to agent | ||
// introspection endpoint feature for the last time. | ||
RequireDockerVersion(t, ">=17.03.1-ce") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this required? The introspection API shouldn't have dependency on the docker version.
require.NoError(t, err, "Error waiting for task to transition to STOPPED") | ||
exitCode, _ := task.ContainerExitcode("agent-introspection-validator") | ||
|
||
assert.Equal(t, 42, exitCode, fmt.Sprintf("Expected exit code of 42; got %d", exitCode)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: you can just use assert.Equalf
to get rid of the fmt.Sprintf
.
containerName, actualContainerName) | ||
} | ||
|
||
if containerMetadataMap["DockerID"] == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be DockerId
?
faa8ed8
to
a15382f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, please make sure all tests pass.
a15382f
to
8fb0a25
Compare
Summary
Add functional test for agent introspection endpoint.
Implementation details
The
AgentIntrospectionValidator
basically mimicsTaskMetadataValidator
:host
mode, so it's similar to the case when you ssh into the container instance and query the introspection endpoint.Testing
make release
)go build -out amazon-ecs-agent.exe ./agent
)make test
) passgo test -timeout=25s ./agent/...
) passmake run-integ-tests
) pass.\scripts\run-integ-tests.ps1
) passmake run-functional-tests
) pass.\scripts\run-functional-tests.ps1
) passDescription for the changelog
Licensing
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.