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

Remove task/container overridden in DockerHostConfig #1048

Merged
merged 1 commit into from
Nov 6, 2017

Conversation

richardpen
Copy link

@richardpen richardpen commented Nov 2, 2017

In task/container overridden, it's copying the task/container struct
which has a lock, this could cause deadlock if some other go routine
acquired the lock right before copying and released the lock after the
copy. This commit moved the overridden part to ACS when unmarshal the
task from payload message.

Summary

Fix #1047

Implementation details

Removed the Overridden function, and added the same in TaskFromACS.

Testing

  • Builds on Linux (make release)
  • Builds on Windows (go build -out amazon-ecs-agent.exe ./agent)
  • Unit tests on Linux (make test) pass
  • Unit tests on Windows (go test -timeout=25s ./agent/...) pass
  • Integration tests on Linux (make run-integ-tests) pass
  • Integration tests on Windows (.\scripts\run-integ-tests.ps1) pass
  • Functional tests on Linux (make run-functional-tests) pass
  • Functional tests on Windows (.\scripts\run-functional-tests.ps1) pass

New tests cover the changes:
yes

Description for the changelog

Licensing

This contribution is under the terms of the Apache 2.0 License:

In task/container overridden, it's copying the task/container struct
which has a lock, this could cause deadlock if some other go routine
acquired the lock right before copying and released the lock after the
copy. This commit moved the overridden part to ACS when unmarshal the
task from payload message.
@samuelkarp
Copy link
Contributor

Can you add a test case that covers reading a task with overrides from the state file and unmarshalling it correctly so that the overrides are properly applied?

@samuelkarp samuelkarp added this to the 1.15.1 milestone Nov 6, 2017
@richardpen
Copy link
Author

@samuelkarp Actually the overrides field of the container won't be set in the agent today. We don't even need the code here in this PR, we shall remove this part of code when the model is updated.

@richardpen richardpen merged commit 0e9f2a8 into aws:dev Nov 6, 2017
@jahkeup jahkeup mentioned this pull request Nov 7, 2017
@nmeyerhans nmeyerhans mentioned this pull request Nov 10, 2017
@richardpen richardpen deleted the overridden branch November 21, 2017 01:03
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.

3 participants