fix memory resource accounting for multiple containers in single task #3782
Conversation
| @@ -4983,6 +4983,35 @@ func TestToHostResources(t *testing.T) { | |||
| }, | |||
| } | |||
|
|
|||
| // A combination of containers with different configs with memory sourcing from different sources | |||
| testTask5 := &Task{ | |||
There was a problem hiding this comment.
This test may not be able to catch the gap that we are fixing, since the test uses a single hostConfig for all containers, In that case, the existing implementation would've worked fine. We should create at least two hostConfigs with different memoryReservation, such that the existing implementation would fail
There was a problem hiding this comment.
Ack - additionally, existing implementation fails due to payload Agent receives does not have all the HostConfig fields. So the unit test and it's hostConfig need to be defined in such a way as well to actually repro the case with failure for existing impl and success for new.
If we define HostConfig with a struct in unit test, the zero value (=int64(0)) gets used instead which does not create actual scenario, in which the MemoryReservation field itself might not exist. So both implementations succeed in such case.
Pushed changes to correct this.
prateekchaudhry
force-pushed
the
taskAccountingMemoryAccountingFix
branch
5 times, most recently
from
1c95f25
to
b737dcf
Compare
| // To parse memory reservation / soft limit | ||
| hostConfig := &dockercontainer.HostConfig{} | ||
|
|
||
| for _, c := range task.Containers { | ||
| // To parse memory reservation / soft limit | ||
| hostConfig := &dockercontainer.HostConfig{} | ||
|
|
There was a problem hiding this comment.
Can you update in description about how this is causing the problem? i.e. json.Unmarshal will not override previously assigned value if the field is not specified in the new json source.
prateekchaudhry
changed the title
prateekchaudhry
force-pushed
the
feature/task-resource-accounting
branch
from
9aa7d55
to
058983a
Compare
prateekchaudhry
dismissed
Realmonia’s stale review
The merge-base changed after approval.
prateekchaudhry
force-pushed
the
feature/task-resource-accounting
branch
from
058983a
to
1e72259
Compare
prateekchaudhry
force-pushed
the
taskAccountingMemoryAccountingFix
branch
from
b737dcf
to
3f80e5c
Compare
|
prateekchaudhry
merged commit 2b0ff57
into
aws:feature/task-resource-accounting
…aws#3782) * fix memory resource accounting for multiple containers * change unit tests for multiple containers, add unit test for awsvpc
* 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
This PR fixes how task memory is accounted when task level memory limit is not specified in task definition. The existing implementation fails to account correctly in cases where containers with
MemoryReservationfield present in HostConfig might occur before containers without such field in thetask.Containersslice. In such cases,MemoryReservationof previous container 'persists', as the object into which the HostConfig is being unmarshalled is declared outside of the loop which iterates over the containers.Main cause of this change is that the ACS payload agent receives has HostConfig like
{\"NetworkMode\":\"bridge\",\"CapAdd\":[],\"CapDrop\":[]}with missing fields in HostConfig struct. That is, if fields likeMemoryReservationare not specified, they are not present in the payload, andjson.Unmarshaldoes not make any changes to the object. Using a new object for every container ensures this persistence does not happen.Implementation details
Fix:
&dockercontainer.HostConfig{}object for each container when mapping task to host resourcesrawHostConfig*in related unit tests have been modified to be directly defined using a string instead of marshaling a HostConfig object. This is necessary, because if we use a HostConfig object, it will use the zero value (int64(0)) of fields likeMemoryReservationif they are not specifiedRelated Containers Roadmap Issue
aws/containers-roadmap#325
Testing
Verified -
MemoryReservationfollowed by containers withmemoryfail for existing implementation - and pass with the fix -testTask5hereNew tests cover the changes:
Yes
Description for the changelog
Fix memory resource accounting for multiple containers in single task
Licensing
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.