Change reconcile/container update order on init and waitForHostResources/emitCurrentStatus order #3747
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
prateekchaudhry
force-pushed
the
taskAccountingAgentRestart
branch
from
a770215
to
dddabb1
Compare
Yiyuanzzz
previously approved these changes
Jun 12, 2023
singholt
reviewed
Jun 12, 2023
yinyic
reviewed
Jun 13, 2023
| // Wait here until enough resources are available on host for the task to progress | ||
| // - Waits until host resource manager succesfully 'consume's task resources and returns | ||
| // - For tasks which have crossed this stage before (on agent restarts), resources are pre-consumed - returns immediately | ||
| // (resources are later 'release'd on Stopped task emitTaskEvent call) | ||
| mtask.waitForHostResources() |
There was a problem hiding this comment.
Maybe we can add a small optimization here to skip invoking waitForHostResources if task known status is not STOPPED? Since we know in that case the host resources will be released immediately anyway.
There was a problem hiding this comment.
Yes let me add that. Although for this particular 'restart case', resources would be detected as 'STOPPED', and waitForHostResources behavior is resources are pre-consumed - returns immediately i.e. without queueing.
yinyic
previously approved these changes
Jun 13, 2023
singholt
previously approved these changes
Jun 13, 2023
Yiyuanzzz
previously approved these changes
Jun 13, 2023
yinyic
previously approved these changes
Jun 13, 2023
prateekchaudhry
dismissed stale reviews from yinyic, Yiyuanzzz, and singholt
via
495ad7f
singholt
approved these changes
Jun 13, 2023
Yiyuanzzz
approved these changes
Jun 13, 2023
prateekchaudhry
merged commit abf9bd1
into
aws:feature/task-resource-accounting
35 checks passed
prateekchaudhry
added a commit
that referenced
this pull request
Jun 21, 2023
…ces/emitCurrentStatus order (#3747)
prateekchaudhry
added a commit
that referenced
this pull request
Jun 22, 2023
…ces/emitCurrentStatus order (#3747)
Merged
prateekchaudhry
added a commit
that referenced
this pull request
Jul 12, 2023
…ces/emitCurrentStatus order (#3747)
prateekchaudhry
added a commit
to prateekchaudhry/amazon-ecs-agent
that referenced
this pull request
Jul 12, 2023
prateekchaudhry
added a commit
that referenced
this pull request
Jul 12, 2023
* Revert "Revert "host resource manager initialization"" This reverts commit dafb967. * Revert "Revert "Add method to get host resources reserved for a task (#3706)"" This reverts commit 8d824db. * Revert "Revert "Add host resource manager methods (#3700)"" This reverts commit bec1303. * Revert "Revert "Remove task serialization and use host resource manager for task resources (#3723)"" This reverts commit cb54139. * Revert "Revert "add integ tests for task accounting (#3741)"" This reverts commit 61ad010. * Revert "Revert "Change reconcile/container update order on init and waitForHostResources/emitCurrentStatus order (#3747)"" This reverts commit 60a3f42. * Revert "Revert "Dont consume host resources for tasks getting STOPPED while waiting in waitingTasksQueue (#3750)"" This reverts commit 8943792.
Realmonia
pushed a commit
that referenced
this pull request
Jul 20, 2023
* Revert reverted changes for task resource accounting (#3796) * Revert "Revert "host resource manager initialization"" This reverts commit dafb967. * Revert "Revert "Add method to get host resources reserved for a task (#3706)"" This reverts commit 8d824db. * Revert "Revert "Add host resource manager methods (#3700)"" This reverts commit bec1303. * Revert "Revert "Remove task serialization and use host resource manager for task resources (#3723)"" This reverts commit cb54139. * Revert "Revert "add integ tests for task accounting (#3741)"" This reverts commit 61ad010. * Revert "Revert "Change reconcile/container update order on init and waitForHostResources/emitCurrentStatus order (#3747)"" This reverts commit 60a3f42. * Revert "Revert "Dont consume host resources for tasks getting STOPPED while waiting in waitingTasksQueue (#3750)"" This reverts commit 8943792. * fix memory resource accounting for multiple containers in single task (#3782) * fix memory resource accounting for multiple containers * change unit tests for multiple containers, add unit test for awsvpc
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 PR fixes a bug figured out in task resource accounting on Agent Restarts. The issue is when Agent is down. a container stops (repro by
docker stop <container_id>) and agent comes back up, task resources are not released by host resource manager.Problems in current orders of calls are :
reconcileHostResources(see Remove task serialization and use host resource manager for task resources #3723) is being called after stopped containers are accounted for in task engine. So if a container stops while the agent is stopped, reconcileHostResources may not pre-'consume' resources for a task, because it's overseeTask would still need to be run for cleanup if it's status has changed to stop. As a result,reconcileHostResourcesshould be called beforefilterTasksToStartUnsafewhich updates the container and task statuses. This order is updated with comments.emitCurrentStatusalso does anemitTaskEventcall - which releases 'consumed' resources in host resource manager (seeManagement of host resourcesin summary in Remove task serialization and use host resource manager for task resources #3723). AndwaitForHostResources()again re-allocates resources, leading to persistent accounted for resources in host resource manager in these cases (when tasks stop when agent is down). Changing it's order afterwaitForHostResources()call and updating comments.This PR also makes a change in
host_resource_managerandToHostResourcesintask.goto dereference ports from[]*stringto[]stringduring logging for more understandable debug logging. This change results in outputting actual port values instead of lvalues of the ports which might not be always relevant. See PORTS_TCP and PORTS_UDP after the change here[Debug] logger=structured msg="Consumed resources after task consume call" CPU=512 MEMORY=768 PORTS_TCP=[22 23] PORTS_UDP=[1000 1001] GPU= 1 taskArn="arn:aws:ecs:us-east-1:<aws_account_id>:task/cluster-name/11111"Related Containers Roadmap Issue
aws/containers-roadmap#325
Testing
With debug logs, verified agent restarts function properly and resources are accounted and released properly for following scenarios :
Description for the changelog
Fix Agent restarts and ports logging with task resource accounting
Licensing
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.