Add method to get host resources reserved for a task #3706
Conversation
prateekchaudhry
force-pushed
the
taskResourceMethod
branch
4 times, most recently
from
e564ea1
to
8ad9759
Compare
prateekchaudhry
force-pushed
the
taskResourceMethod
branch
from
8ad9759
to
7a045e4
Compare
| for _, c := range task.Containers { | ||
| if c.DockerConfig.HostConfig != nil { | ||
| err := json.Unmarshal([]byte(*c.DockerConfig.HostConfig), hostConfig) | ||
| if err != nil || hostConfig.MemoryReservation <= 0 { |
There was a problem hiding this comment.
Just out of curious, is there a case that MemoryReservation actually less than 0? How did we know it's unit with unmarshal?
There was a problem hiding this comment.
I don't see a scenario where it will be less than 0. Units are mentioned in comments here, also verified by manual runs.
There was a problem hiding this comment.
int64 implies that this is a signed integer (ie could be negative). You might also add another check to validate that the HostConfig is an int.
There was a problem hiding this comment.
MemoryReservation is the soft limit we are concerned with here, which is part of the HostConfig. I believe we are already checking if it is zero or negative by the check here hostConfig.MemoryReservation <= 0, so should be good imo.
There was a problem hiding this comment.
And that MemoryReservation field in the HostConfig is zero-initialized by default so we'll always have it if we don't have a null HostConfig. Resolving.
There was a problem hiding this comment.
Yes that's right, thanks!
agent/api/task/task.go
Outdated
| // * GPU | ||
| // - Return num of gpus requested (len of GPUIDs field) | ||
| // | ||
| // TODO remove ToHostResources is used |
There was a problem hiding this comment.
nit: not sure what this TODO tells us.
There was a problem hiding this comment.
remove when ToHostResources is used, when got lost somewhere, will update it
| @@ -4802,3 +4803,196 @@ func TestInitializeAndGetCredentialSpecResource(t *testing.T) { | |||
| _, ok := task.GetCredentialSpecResource() | |||
| assert.True(t, ok) | |||
| } | |||
|
|
|||
| func getTestTaskResourceMap(cpu int64, mem int64, ports []*string, portsUdp []*string, numGPUs int64) map[string]*ecs.Resource { | |||
There was a problem hiding this comment.
this is a well-written test case 🥇
This reverts commit 67dcd21.
* 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.
* 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
Summary
Adding a method to
ToHostResourcesforTaskstruct. This returns resources inmap[string]*ecs.Resourcetype which can later directly be consumed byHostResourceManager(See related PRs #3700, #3684 which setup this) to account for resources for tasks scheduled to be run/running.There is a conversion logic on how to interpret different task resources at task/container levels when accounting for a task. This has been written down in comments above
ToHostResources, and the function basically implements that in code.Resources to account for : cpu, memory, ports(tcp/udp) and gpu
Both
HostResourceManagermethods andToHostResourceswill be used in a future change which will remove serialization and instead wire these up together in task_manager/docker_task_engine for starting and stopping tasks.Related Containers Roadmap Issue
aws/containers-roadmap#325
Testing
Unit tests to test for basic resource conversion logic have been added and tested with.
New tests cover the changes: Yes
Description for the changelog
Add method to get host resources reserved for a task
Licensing
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.