microsoft / dotnet-framework-docker

The repo for the official docker images for .NET Framework on Windows Server Core.
https://hub.docker.com/_/microsoft-dotnet-framework
MIT License
710 stars 335 forks source link

Apply patch for supporting container limits #842

Closed mthalman closed 3 years ago

mthalman commented 3 years ago

Applies the patch for 4.8's support of container limits (i.e. Docker's --cpus and --memory options).

cguedel commented 2 years ago

Is there more information about this, also regarding what KB9008392 does? There's only two hits on Google for this KB number, both pointing to this issue/the commit.

Reason I'm asking is because we see a kind of strange behavior when this patch is applied, but maybe we're understanding something wrong.

Whenever we start powershell processes using something like this in the container

$i=0; while($True) { start-process powershell; $i++; echo $i }

on the host, there's increased "committed memory" (as shown in taskmgr). For each powershell process, we see the commit increasing by about 500mb. We see the same with our own .net assemblies, but each process (we run multiple in the container) is ending up with a commit size of over 2GB.

We think it's related to the commit size, that we saw docker/moby crash due to an out of memory condition and are now looking for solutions/workarounds and stumbled upon this.

If we remove the application of the patch (basically reverting this merge request to the Dockerfile), the commit size increases with each powershell instance by a few MB, which seems more reasonable.

mthalman commented 2 years ago

@cguedel - You can see the announcement which explains what this patch provides.

If we remove the application of the patch (basically reverting this merge request to the Dockerfile), the commit size increases with each powershell instance by a few MB, which seems more reasonable.

Just to confirm, you've removed this section of the Dockerfile and the issue goes away?

https://github.com/microsoft/dotnet-framework-docker/blob/35c0730b85686a74eefa33b6e4026fe92affae32/src/runtime/4.8/windowsservercore-ltsc2019/Dockerfile#L17-L23

cc @mangod9

cguedel commented 2 years ago

@mthalman I see, thanks for the pointers.

Yes, if we remove this part from the Dockerfile, the issue (I'm not even sure if it is one) goes away.

cguedel commented 2 years ago

Sorry, actually we remove this:

https://github.com/microsoft/dotnet-framework-docker/blob/35c0730b85686a74eefa33b6e4026fe92affae32/src/runtime/4.8/windowsservercore-ltsc2019/Dockerfile#L25-L31

mthalman commented 2 years ago

Sorry, actually we remove this:

Ah, right. That's what I meant to select.

mangod9 commented 2 years ago

Hi @cguedel, are you able to share a repro for this issue? If I understand correctly, you are spawning multiple processes and each one shows a committed memory of 2gb immediately on startup?

cguedel commented 2 years ago

@mangod9 See here https://gist.github.com/cguedel/c8c097787ea06a2e96e61a85a6b41b3a

Basically this builds two containers, one identical to the current Dockerfile here in the repo, one without the patch. Then it spawns 100 powershell processes. Afterwards it sorts them by paged memory size and outputs the first two.

Note: I have also observed this to be a smaller issue on system with lower memory. I.e. if the host only has 8GB RAM, instead of ~500MB for each process, I only see about 200MB (also for our own exe [which I cannot share]).

Does this help?

mangod9 commented 2 years ago

Thanks for the info @cguedel. As part of the changes to enable better container support the feature added functionality to use upto 75% of the available container memory as the HeapHardLimit. This might lead to higher memory utilization if the container is running multiple managed processes within it. However in most cases the GC should be able to utilize the maximum resources but in most cases you shouldnt be observing any failures, since it will be aggressive in cleaning up memory when running low.

You could use the newer COMPlus_GCHeapHardLimit setting to reduce the max heap for each managed process if required.

@maoni0 as fyi.

mangod9 commented 2 years ago

Hi @cguedel, following up here whether the workaround would work for you?

cguedel commented 2 years ago

Hey @mangod9, I can confirm that setting this environment variable changes things. However, we're not quite sure which value we should set this to, to get the same behavior as before?

mangod9 commented 2 years ago

You could set it based on how many managed processes you expect to run within the container, so each process will get at most COMPlus_GCHeapHardLimit of heap space. So for example if you are running 10 processes within the container the limit could be 1/10th of the container memory restriction.

That said we dont expect that leaving it as is might cause failures just that the memory utilization might be higher and if the container does get into low memory situations the GC will do aggressive collections.

mthalman commented 2 years ago

I've posted an announcement with details on this issue: https://github.com/microsoft/dotnet-framework-docker/issues/863