Fix bug where pause container was not always cleaned up #2824
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
angelcar
force-pushed
the
angelcar_pause_container_cleanup_fix
branch
4 times, most recently
from
8f97f8b
to
23db43c
Compare
sharanyad
reviewed
Mar 2, 2021
| // checkTearDownPauseContainer idempotently tears down the pause container network when the pause container's known | ||
| //or desired status is stopped. | ||
| func (engine *DockerTaskEngine) checkTearDownPauseContainer(task *apitask.Task) { | ||
| for _, container := range task.Containers { |
There was a problem hiding this comment.
instead of iterating through all containers, can we check if task is not awsvpc enabled (GetENI == nil) and return quickly?
There was a problem hiding this comment.
is it time or readability that would be improved? In terms of time, there can be a max of 10 containers so we would gain a few nanoseconds :p
There was a problem hiding this comment.
as a practice, just like to return quickly wherever possible :) also, containers count could be increased in the future
There was a problem hiding this comment.
Fair enough. changed.
angelcar
force-pushed
the
angelcar_pause_container_cleanup_fix
branch
from
23db43c
to
4229491
Compare
sharanyad
approved these changes
Mar 2, 2021
mythri-garaga
approved these changes
Mar 2, 2021
shubham2892
approved these changes
Mar 2, 2021
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
This fixes a bug that can prevent the pause container network to be cleaned up when ECS Agent is restarted due to
dockerdrestarts. Failing to clean the pause container network might cause issues for future tasks to start.When
dockerdis restarted, all running containers (along with ECS Agent) are stopped. Afterdockerdand ECS Agent restart successfully, the latter proceeds to stop all the tasks that were running previous to the restart since it realizes the respective containers are no longer running. When this happens, the pause container network is not cleaned up since the workflow is different to the normal operations.Implementation details
Do a check after the managed tasks is stopped to see if the pause container still needs to be cleaned up. Most of the times the pause container has already been cleaned up by the time the managed task stops, but as mentioned above, there are scenarios where this might not happen.
Since now we are invoking container cleanup in more than one place (previously it only happened in
stopContainermethod), the operation was made idempotent by storing a flag in the container to indicate whether that container was torn down.For now that flag is only used for the pause container, but might be useful for other containers that need explicit teardown in the future.
Testing
There are a number of unit and integration tests that check for proper pause container cleanup. Those tests should still pass since the previous logic is not modified and a few new assertions are added to check if the new tear down flag was set.
In addition, manually tested that pause container network is cleaned under normal circumstances as well as after docker restarts.
New tests cover the changes: yes
Description for the changelog
Fix bug where pause container was not always cleaned up
Licensing
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.