LCOW: adds memory limit for linux containers #35955

bruegth · 2018-01-08T15:45:38Z

- What I did
LCOW: adds memory limit to linux containers
Because if omitted a Hyper-V container is limited to 1GB and couldn't changed.

- How I did it
copy and paste from windows container creation

- How to verify it
run meminfo in linux container under windows

- Description for the changelog

LCOW: Adds memory limit for linux containers

- A picture of a cute animal (not mandatory but encouraged)

GordonTheTurtle · 2018-01-08T15:45:39Z

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "lcow-adds-memory-limit-to-linux-containers" git@github.com:bruegth/moby.git somewhere
$ cd somewhere
$ git commit --amend -s --no-edit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

Signed-off-by: Thomas Brueggemann <thomas.brueggemann@wuh-group.com>

yongtang · 2018-01-09T17:26:10Z

/cc @jhowardmsft

lowenna · 2018-01-09T20:43:04Z

libcontainerd/client_local_windows.go

@@ -418,6 +418,13 @@ func (c *client) createLinux(id string, spec *specs.Spec, runtimeOptions interfa
 		}
 		configuration.NetworkSharedContainerName = spec.Windows.Network.NetworkSharedContainerName
 	}
+
+	// Add memory limit (if omittet the memory will be limited to default (1GB) in Hyper-V isolation)
+	if spec.Windows.Resources.Memory != nil {


These don't need to be nested - the second if won't be evaluated if the first is false.

I'm not sure I follow. The first is required in case specs.Windows.Resources.Memory is nil, right?

lowenna · 2018-01-09T20:43:20Z

@johnstep PTAL

johnstep · 2018-01-09T22:16:32Z

libcontainerd/client_local_windows.go

@@ -418,6 +418,13 @@ func (c *client) createLinux(id string, spec *specs.Spec, runtimeOptions interfa
 		}
 		configuration.NetworkSharedContainerName = spec.Windows.Network.NetworkSharedContainerName
 	}
+
+	// Add memory limit (if omittet the memory will be limited to default (1GB) in Hyper-V isolation)


s/omittet/omitted/

johnstep · 2018-01-09T22:26:51Z

libcontainerd/client_local_windows.go

@@ -418,6 +418,13 @@ func (c *client) createLinux(id string, spec *specs.Spec, runtimeOptions interfa
 		}
 		configuration.NetworkSharedContainerName = spec.Windows.Network.NetworkSharedContainerName
 	}
+
+	// Add memory limit (if omittet the memory will be limited to default (1GB) in Hyper-V isolation)
+	if spec.Windows.Resources.Memory != nil {


I'm not sure I follow. The first is required in case specs.Windows.Resources.Memory is nil, right?

thaJeztah · 2018-01-10T08:58:31Z

libcontainerd/client_local_windows.go

+
+	// Add memory limit (if omittet the memory will be limited to default (1GB) in Hyper-V isolation)
+	if spec.Windows.Resources.Memory != nil {
+		if spec.Windows.Resources.Memory.Limit != nil {


I'm super confused; spec.Windows.Resources controls resources for Linux containers? I'm worried that we'll now be duplicating all functionality that's already covered by standard Linux containers;

moby/vendor/github.com/opencontainers/runtime-spec/specs-go/config.go

Lines 22 to 27 in 4f5c47a

// Linux is platform-specific configuration for Linux based containers.

Linux *Linux `json:"linux,omitempty" platform:"linux"`

// Solaris is platform-specific configuration for Solaris based containers.

Solaris *Solaris `json:"solaris,omitempty" platform:"solaris"`

// Windows is platform-specific configuration for Windows based containers.

Windows *Windows `json:"windows,omitempty" platform:"windows"`

I think the distinction Windows / Linux is not so well chosen, possibly HCS / Docker (+plattform specific subset) would be better.
Conversely, the HCS is also a subset of the docker config.

https://github.com/Microsoft/hcsshim/blob/master/interface.go#L67
https://docs.docker.com/engine/api/v1.35/#operation/ContainerCreate

Point is that Linux containers take a whole different set of options for Memory constraints;

moby/vendor/github.com/opencontainers/runtime-spec/specs-go/config.go

Lines 273 to 289 in 4f5c47a

// LinuxMemory for Linux cgroup 'memory' resource management

type LinuxMemory struct {

// Memory limit (in bytes).

Limit *int64 `json:"limit,omitempty"`

// Memory reservation or soft_limit (in bytes).

Reservation *int64 `json:"reservation,omitempty"`

// Total memory limit (memory + swap).

Swap *int64 `json:"swap,omitempty"`

// Kernel memory limit (in bytes).

Kernel *int64 `json:"kernel,omitempty"`

// Kernel memory limit for tcp (in bytes)

KernelTCP *int64 `json:"kernelTCP,omitempty"`

// How aggressive the kernel will swap memory pages.

Swappiness *uint64 `json:"swappiness,omitempty"`

// DisableOOMKiller disables the OOM killer for out of memory conditions

DisableOOMKiller *bool `json:"disableOOMKiller,omitempty"`

}

IIUC, this will now map the Memory-constraints for the container to the Hyper-V VM for the container (not the container itself)

Yes that is correct.
HyperV Isolation mode reserves the memory for the container and the container itself has a limit by container config.
So in HyperV Isolation the container count is limit by physical memory.

Signed-off-by: Thomas Brueggemann <thomas.brueggemann@wuh-group.com>

tiborvass · 2018-01-18T19:40:03Z

I think @thaJeztah's concerns about LCOW vs Windows are valid: isn't the whole point of LCOW to abstract the Linux API implementation and let userspace programs behave the same as on Linux? In which case why don't we reuse the Linux configs?

Those questions have to be answered, but let's unblock this PR.

LGTM

lowenna · 2018-01-19T02:35:36Z

@tiborvass I had an offline conversation with @johnstep, but there's a good reason here. The short answer is that on Windows, there really are two sets of "resources" to run an LCOW

Some are required to configure the utility VM itself, such as layer paths, defined in OCI under type Windows struct..., the same as for WCOW. In addition, this struct defines resources of the utility VM which is the WindowsResources element of the Windows struct.

Beyond that is configuration of the container inside the utility VM. That's where the Linux side of the OCI spec comes in (the LinuxResources OCI struct), and are applied inside the utility VM, as opposed to being properties of the utility VM itself. This category has not yet been implemented - in effect, the container inside the utility VM runs 'unbounded' currently. Obviously as we move forward and in RS4/5 can run multiple containers inside a UVM, then these resources are critical.

Does that help clear up your question?

tiborvass · 2018-01-19T16:08:31Z

@jhowardmsft almost! How do the windows resources get set, and shouldn't the linux resources be set instead? IOW, docker run --memory 2gb can only set one of the two. My understanding is that the windows resource is more useful to be set because if it's not set, HCS will set it to the default 1Gb which is annoying, and it makes no sense to set 2GB limit for the linux container, when the VM is set to 1Gb.

So is the solution to only set the windows ressources for the ones that VMs also have, and set the linux ressources only for the very cgroupy things?

johnstep · 2018-01-23T19:08:46Z

Please note that, while the code changes look fine, this should not be merged until the discussion above is completed, and there are still outstanding questions. Thanks.

razzeee · 2018-02-07T10:58:09Z

Would it be possible to get the discussion going again, as it seems to have have stopped for some reason :/

fizxmike · 2018-04-25T01:48:24Z

67E6

Yes... please continue the conversation. I need this feature, and I'm sure others do too. (Unless someone here knows a different workaround for "... HCS [setting] the default 1Gb which is annoying"?)

libcontainerd/client_local_windows.go

richardgavel · 2018-07-18T13:52:45Z

Can we resolve this PR? Only being able to create a Linux container that support a hard coded ~1 GB of memory doesn't help much. I am trying to create a Linux Saltstack master + Windows minion combination and LCOW makes that combination of platform types much easier. However, Salt needs more than the default allocated memory.

lowenna · 2018-07-18T22:31:14Z

I think in the HCS v1 schema, this is pretty much the best we can do. In the v2 schema, it's far more flexible with clients in control of both the utility VM side and the container side.

notanaverageman · 2018-07-19T06:57:56Z

This PR would end in nil errors with only these changes. I had opened another pull request for both CPU and Memory limits on LCOW. #37296
I have been using it without a problem for more than a month.

olljanat · 2018-12-22T09:48:09Z

@bruegth CI fails to error:

18:23:50 libcontainerd/client_local_windows.go:1::warning: file is not gofmted with -s (gofmt)
18:23:50 libcontainerd/client_local_windows.go:1::warning: file is not goimported (goimports)
18:23:51 Build step 'Execute shell' marked build as failure

And there is reviews above which needs to be addressed.

bruegth · 2019-01-07T14:43:27Z

@johnstep and @thaJeztah I suggest to close this PR and go further with #37296

olljanat · 2019-01-10T15:42:37Z

@johnstep and @thaJeztah I suggest to close this PR and go further with #37296

Closing...

GordonTheTurtle added dco/no Automatically set by a bot when one of the commits lacks proper signature status/0-triage labels Jan 8, 2018

adds memory limit

8f14b28

Signed-off-by: Thomas Brueggemann <thomas.brueggemann@wuh-group.com>

GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label Jan 8, 2018

yongtang added status/2-code-review and removed status/0-triage labels Jan 9, 2018

lowenna reviewed Jan 9, 2018

View reviewed changes

johnstep suggested changes Jan 9, 2018

View reviewed changes

thaJeztah reviewed Jan 10, 2018

View reviewed changes

fixes comment spelling

7540aa5

Signed-off-by: Thomas Brueggemann <thomas.brueggemann@wuh-group.com>

thaJeztah assigned johnstep Jan 11, 2018

tiborvass added the rebuild/janky label Jan 18, 2018

GordonTheTurtle removed the rebuild/janky label Jan 18, 2018

tiborvass added the rebuild/z label Jan 18, 2018

GordonTheTurtle removed the rebuild/z label Jan 18, 2018

tiborvass added rebuild/experimental labels Jan 18, 2018

GordonTheTurtle removed rebuild/experimental labels Jan 18, 2018

johnstep approved these changes Jan 23, 2018

View reviewed changes

fizxmike mentioned this pull request Apr 25, 2018

docker doesn't use memory or CPU options microsoft/opengcs#145

Closed

fizxmike mentioned this pull request Apr 25, 2018

Windows image build Docker runs out of memory on Windows 10 #31604

Closed

ghost reviewed May 5, 2018

View reviewed changes

libcontainerd/client_local_windows.go Show resolved Hide resolved

thaJeztah added the area/lcow Issues and PR's related to the experimental LCOW feature label Oct 30, 2018

derek bot added the rebuild/* label Dec 21, 2018

GordonTheTurtle removed the rebuild/* label Dec 21, 2018

derek bot added the status/failing-ci Indicates that the PR in its current state fails the test suite label Dec 22, 2018

derek bot closed this Jan 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

LCOW: adds memory limit for linux containers #35955

LCOW: adds memory limit for linux containers #35955

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

8000 Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

	// Linux is platform-specific configuration for Linux based containers.
	Linux *Linux `json:"linux,omitempty" platform:"linux"`
	// Solaris is platform-specific configuration for Solaris based containers.
	Solaris *Solaris `json:"solaris,omitempty" platform:"solaris"`
	// Windows is platform-specific configuration for Windows based containers.
	Windows *Windows `json:"windows,omitempty" platform:"windows"`

	// LinuxMemory for Linux cgroup 'memory' resource management
	type LinuxMemory struct {
	// Memory limit (in bytes).
	Limit *int64 `json:"limit,omitempty"`
	// Memory reservation or soft_limit (in bytes).
	Reservation *int64 `json:"reservation,omitempty"`
	// Total memory limit (memory + swap).
	Swap *int64 `json:"swap,omitempty"`
	// Kernel memory limit (in bytes).
	Kernel *int64 `json:"kernel,omitempty"`
	// Kernel memory limit for tcp (in bytes)
	KernelTCP *int64 `json:"kernelTCP,omitempty"`
	// How aggressive the kernel will swap memory pages.
	Swappiness *uint64 `json:"swappiness,omitempty"`
	// DisableOOMKiller disables the OOM killer for out of memory conditions
	DisableOOMKiller *bool `json:"disableOOMKiller,omitempty"`
	}

LCOW: adds memory limit for linux containers #35955

LCOW: adds memory limit for linux containers #35955

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

8000 Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!