Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Container CPU limits not supported by .NET runtime in process isolation mode -- Windows containers #46889

Closed
richlander opened this issue Jan 12, 2021 · 11 comments · Fixed by #52690
Assignees
Milestone

Comments

@richlander
Copy link
Member

richlander commented Jan 12, 2021

We intend to support container limits on both Linux and Windows. It appears that we don't have support for process-isolated containers on Windows. That should be resolved.

In particular, CPUs are not being honored for process-isolated Windows containers. They should behave the same way as Linux containers. We need to fix that. Let's start with .NET 6.0, and then look at backporting needs.

Update -- The new behavior is defined at #53149. CPU and memory limits are now honored for Windows process-isolated containers. Previously, only memory-limits were honored.

Command used in each example:

docker run --rm -p 8080:80 -m 80 --cpus 1 mcr.microsoft.com/dotnet/samples:aspnetapp

Windows process-isolated behavior:

image

Note: My local machine is on insider builds, and so doesn't support process-isolated containers of any kind. This example is from a Windows Server 2019 Azure VM. The examples below are from my local machine. That's why they look different.

Windows Hyper-V-isolated behavior:

image

Linux behavior:

image

@richlander richlander changed the title Container limits not supported in process isolation mode in Windows Container limits not supported by .NET runtime in process isolation mode -- Windows containers Jan 12, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Jan 12, 2021
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@mangod9 mangod9 added this to the 6.0.0 milestone Jan 13, 2021
@mangod9 mangod9 removed the untriaged New issue has not been triaged by the area owner label Jan 13, 2021
@mangod9
Copy link
Member

mangod9 commented Jan 26, 2021

@kouvel @janvorli since we were discussing this today: #47427, which might be relevant to this

@AntonLapounov
Copy link
Member

both CPUs and memory are not being honored for process-isolated Windows containers

I am not sure what "not being honored" means here. It is the operating system's job to allocate resources and honor any CPU and memory limits. For Process.MaxWorkingSet we return the value reported by GetProcessWorkingSetSizeEx. The default "soft" limit in Windows is 345 pages or 1,413,120 bytes. Are you suggesting we should return something different? Why? What is the use case here?

@jkotas
Copy link
Member

jkotas commented Apr 26, 2021

The repro steps are not demonstrating the problem that this issue is meant to be about. Process.MaxWorkingSet is not enforced hard limit on Windows by default. It is a soft target for how much memory the process should get when the memory is in a short supply (ie completely useless value) by default.

The repro steps should be displaying GCMemoryInfo.TotalAvailableMemoryBytes. GCMemoryInfo.TotalAvailableMemoryBytes captures the hard limit infered from the environment that the GC will try to stay under.

@AntonLapounov
Copy link
Member

I think GCToOSInterface::GetPhysicalMemoryLimit already takes all memory limits into account, including the ones enforced by the Job:

if ((limit_info.BasicLimitInformation.LimitFlags & JOB_OBJECT_LIMIT_JOB_MEMORY) != 0)
job_memory_limit = limit_info.JobMemoryLimit;
if ((limit_info.BasicLimitInformation.LimitFlags & JOB_OBJECT_LIMIT_PROCESS_MEMORY) != 0)
job_process_memory_limit = limit_info.ProcessMemoryLimit;
if ((limit_info.BasicLimitInformation.LimitFlags & JOB_OBJECT_LIMIT_WORKINGSET) != 0)
job_workingset_limit = limit_info.BasicLimitInformation.MaximumWorkingSetSize;
if ((job_memory_limit != (size_t)UINTPTR_MAX) ||
(job_process_memory_limit != (size_t)UINTPTR_MAX) ||
(job_workingset_limit != (size_t)UINTPTR_MAX))
{
job_physical_memory_limit = min (job_memory_limit, job_process_memory_limit);
job_physical_memory_limit = min (job_physical_memory_limit, job_workingset_limit);
The calculated value is stored in gc_heap::total_physical_mem:

gc_heap::total_physical_mem = GCToOSInterface::GetPhysicalMemoryLimit (&gc_heap::is_restricted_physical_mem);

and returned by GCMemoryInfo.TotalAvailableMemoryBytes:

*totalAvailableMemoryBytes = gc_heap::heap_hard_limit != 0 ? gc_heap::heap_hard_limit : gc_heap::total_physical_mem;

@jkotas
Copy link
Member

jkotas commented Apr 26, 2021

Agree. It would be useful to validate that it actually works for an actual process-isolated container.

We are missing the CPU limits though (https://docs.microsoft.com/en-us/windows/win32/api/winnt/ns-winnt-jobobject_cpu_rate_control_information).

@AntonLapounov
Copy link
Member

It would be useful to validate that it actually works for an actual process-isolated container.

Here is what I see with 80 MB memory and 1.0 CPU quota limits:

  • --isolation=hyperv
NUMBER_OF_PROCESSORS: 1
Environment.ProcessorCount: 1
GC.GetGCMemoryInfo().TotalAvailableMemoryBytes: 62,914,560
  • --isolation=process
NUMBER_OF_PROCESSORS: 16
Environment.ProcessorCount: 16
GC.GetGCMemoryInfo().TotalAvailableMemoryBytes: 62,914,560

The memory limit is respected and Environment.ProcessorCount works in full accordance with its present specification, returning "the number of processors on the current machine". Since process isolation does not use a VM, no machine with one processor is created.

If it has been decided to hijack the Environment.ProcessorCount property to return not "the number of processors on the current machine", but some different number like "the rounded up CPU quota for the current process", why has the documentation not been updated to reflect that? Could we please write down the new definition for this property and create a documentation change pull request by clicking "Edit" on the documentation page?

@richlander
Copy link
Member Author

The memory limit is respected and Environment.ProcessorCount works in full accordance with its present specification,

I don't think that's true. Our cgroup behavior does something different. That's what I'm asking for Windows process-isolated containers.

@AntonLapounov
Copy link
Member

I don't think that's true.

What exactly is not true? I have provided results for both isolation modes on Windows above.

@richlander
Copy link
Member Author

I made a PR to docs: dotnet/dotnet-api-docs#6668.

The result of this change does not report the total number of processors on the machine. That's what I meant.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label May 13, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label May 14, 2021
@richlander
Copy link
Member Author

See #53149 for a demonstration of this change.

@richlander richlander changed the title Container limits not supported by .NET runtime in process isolation mode -- Windows containers Container CPU limits not supported by .NET runtime in process isolation mode -- Windows containers May 25, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jun 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants