acs: add an empty task in TaskStateChange for unrecognized tasks #1467
Conversation
|
@haikuoliu: was there a bug report for this? or how did we discover this gap? mostly just curious. |
|
@haikuoliu: please add a unit test to cover this case |
36e6c06 to
aad3e69
Compare
|
@adnxn I have added unit test for this. |
| wait := &sync.WaitGroup{} | ||
| wait.Add(1) | ||
|
|
||
| mockECSACSClient.EXPECT().SubmitTaskStateChange(gomock.Any()).Do(func(x interface{}) { |
There was a problem hiding this comment.
please check the Task field is not nil inside the mock call, probably something like this.
aad3e69 to
17809f3
Compare
| // The real task cannot be extracted from payload message, so we send an empty task. | ||
| // This is necessary because the task handler will not send an event whose | ||
| // Task is nil. | ||
| Task: &apitask.Task{}, |
There was a problem hiding this comment.
Can you do a nil check here, as the task isn't always nil here?
There was a problem hiding this comment.
Clarified offline.
| mockECSACSClient.EXPECT().SubmitTaskStateChange(gomock.Any()).Do(func(change api.TaskStateChange) { | ||
| assert.NotNil(t, change.Task) | ||
| wait.Done() | ||
| // Don't return, verify timeout happens. |
There was a problem hiding this comment.
If I understand this correctly, this test ensures that the handler deals with the empty task correctly. Are there other test cases we should be hitting? For example, does this handler code path get touched if the task is recognized but nill?
There was a problem hiding this comment.
This test is to ensure that the SubmitTaskStateChange will be hit when there are unrecognized tasks. I added a not nil check here is because we manually create a non-nil task and pass it along the code path.
The unrecognized task means that the task cannot be converted to apitask.Task or some of the fields are not in correct format, so if it's a nil task then it should be a unrecognized task.
| wait.Add(1) | ||
|
|
||
| mockECSACSClient.EXPECT().SubmitTaskStateChange(gomock.Any()).Do(func(change api.TaskStateChange) { | ||
| assert.NotNil(t, change.Task) |
There was a problem hiding this comment.
Do we allow unrecognized tasks to run and complete? If so, how do we know that the task state changes after this are handled ok?
i.e. Is the empty task passed on to another component? And if so, we we creating a new untested code path by creating the empty task?
There was a problem hiding this comment.
We don't allow unrecognized tasks to run or complete. If it's unrecognized, we simply drop it and tell backend that this task is stopped because it's unrecognized.
To stop a task, we don't even need to specify a task struct when calling SubmitTaskStateChange API, The only purpose I added an empty task here is to make this task change not dropped by Agent, so that it will be sent to backend via SubmitTaskStateChange API.
17809f3 to
0974f62
Compare
b961fc1 to
3f82094
Compare
3f82094 to
66a0c5f
Compare
Summary
Add an empty task in TaskStateChange for unrecognized tasks
This fixes a bug where unrecognized task cannot be stopped
Implementation details
Currently when Agent receives a task from ACS, if it's malformed and Agent cannot recognize it, it will be handled by handleUnrecognizedTask function. The
handleUnrecognizedTaskintends to send a task change event to stop the task, but it fails to do so, because it returns aTaskStateChangewithout aTaskin it, and this will be discarded by taskHandler, as it should not be sent according to taskShouldBeSentSo this fix is to add an empty task in the result of
handleUnrecognizedTaskso that the task change will not be discarded.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
Bug - Fixed a bug where unrecognized task cannot be stopped
Licensing
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.