Remove kmem Initialization check while setting memory configuration #962

dubstack · 2016-07-23T00:16:22Z

I suggest that we modify the logic for setting the kmem.limit_on_bytes to be more deterministic and
remove the kmemInitialized check.

If kernel memory is enabled we limit the cgroup during cgroup creation ie. in apply(). This will enable kernel memory accounting on the cgroup ( http://lxr.free-electrons.com/source/Documentation/cgroups/memory.txt?v=3.9#L284 ).
We limit the cgroup by writing some arbitrary limiting value(say 1) so that the kernel memory is accounted, and then we remove the limitation by writing -1.

Once accounting is enabled we should be able to set the kernel memory limit to configuration value in the Set method.

The only scenario in which this would fail is if someone straight out calls Set() on a cgroup which was not created by libcontainer, in which case, if there are already process attached to the cgroup setting kernel memory limit would return an Ebusy signal and we can error out.

cc @mrunalp @derekwaynecarr @cyphar @vishh @hqhq PTAL

mlaventure · 2016-07-24T17:36:12Z

Are we happy for it to fail if joining an existing cgroup with a limit already sets? On this PR, the error message is not very explicit from a user perspective as it will just be:

process_linux.go:258: applying cgroup configuration for process caused "failed to write 1 to memory.kmem.limit_in_bytes: write /sys/fs/cgroup/memory/xxx/memory.kmem.limit_in_bytes: device or resource busy"

I think adding a more explanatory message to the returned error on EBUSY may be a good idea.

WDYT?

hqhq · 2016-07-25T02:19:52Z

Yeah, that's what I concerned in #841 (comment)

dubstack · 2016-07-25T18:42:46Z

@mlaventure @hqhq Please have a look at my comment here: #841 (comment)

dubstack · 2016-07-25T18:46:42Z

libcontainer/cgroups/fs/memory.go

-		// We have to set kernel memory here, as we can't change it once
-		// processes have been attached to the cgroup.
-		if err := s.SetKernelMemory(path, d.config); err != nil {
+		// We have to limit the kernel memory here as it won't be accounted at all


We need to have a check here for determining if kmem is enabled on the system.
@hqhq @vishh Suggestions on how to check for the same?

Check if memory.kmem.limit_in_bytes file is there?

On Mon, Jul 25, 2016 at 11:46 AM, Buddha Prakash [email protected]
wrote:

In libcontainer/cgroups/fs/memory.go
#962 (comment):

@@ -34,9 +32,15 @@ func (s *MemoryGroup) Apply(d *cgroupData) (err error) {
return err
}
}

// We have to set kernel memory here, as we can't change it once

// processes have been attached to the cgroup.

if err := s.SetKernelMemory(path, d.config); err != nil {

// We have to limit the kernel memory here as it won't be accounted at all

We need to have a check here for determining if kmem is enabled on the
system.
@hqhq https://github.com/hqhq @vishh https://github.com/vishh
Suggestions on how to check for the same?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/opencontainers/runc/pull/962/files/5106b565da1c9af39f279edb0235eb1da06d703d#r72121173,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AGvIKEIF-LQ6vcR1xObXWdqCQ_kmAS4aks5qZQSdgaJpZM4JTOyV
.

Alternative of opencontainers#962 Fixes: opencontainers#841 Signed-off-by: Qiang Huang <[email protected]>

dubstack · 2016-07-28T00:00:49Z

Updated the patch to add a more specific error message when kernel memory limit write fails with an EBUSY.
@mlaventure @hqhq @cyphar @vishh PTAL.

mlaventure · 2016-07-28T00:39:32Z

LGTM (IANAM).

vishh · 2016-07-28T21:52:45Z

libcontainer/cgroups/fs/memory.go

-			return err
+
+		// Check if kernel memory is enabled
+		if _, err := os.Stat(filepath.Join(path, cgroupKernelMemoryLimit)); err == nil {


nit use cgroups.PathExists(...)

Why not move this check to SetKernelMemory(...) method?

The idea was we SetKernelMemory() only if kernel memory is enabled. So have the check and then call the method.

But having the check inside the method would be more cleaner. I will make that change.

dubstack · 2016-07-29T00:35:17Z

@cyphar @hqhq @mrunalp @vishh Addressed all comments. PTAL. This is blocking work in Kubernetes. Can we get this quickly reviewed and merged?

hqhq · 2016-07-29T03:47:16Z

libcontainer/cgroups/fs/memory.go

+		if pathErr, ok := err.(*os.PathError); ok {
+			if errNo, ok := pathErr.Err.(syscall.Errno); ok {
+				if errNo == syscall.EBUSY {
+					return fmt.Errorf("Cannot set %s after tasks have joined this cgroup", cgroupKernelMemoryLimit)


EBUSY could also be returned if there are child cgroups, we need to change error message a bit to cover that.

Don't forget to change the above comments.

And error messages start with lower case in runc.

Right! Thanks

dubstack · 2016-07-29T22:26:18Z

@cyphar @hqhq @mrunalp @mlaventure I have updated the logic in systemd to be same as that of cgroup fs.

@mrunalp @derekwaynecarr Can you please have a look and maybe test it on a systemd system.

hqhq · 2016-07-30T01:09:31Z

libcontainer/cgroups/fs/memory.go

+		if pathErr, ok := err.(*os.PathError); ok {
+			if errNo, ok := pathErr.Err.(syscall.Errno); ok {
+				if errNo == syscall.EBUSY {
+					return fmt.Errorf("failed to set %s, because either tasks have already joined this cgroupe or it has chilren", cgroupKernelMemoryLimit)


s/cgroupe/cgroup
s/chilren/children

Thanks! I will make the change!

Signed-off-by: Buddha Prakash <[email protected]>

dubstack · 2016-08-01T16:48:34Z

@cyphar @mrunalp @mlaventure friendly ping!

vishh · 2016-08-01T18:05:12Z

This LGTM. Thanks for the cleanup @dubstack !!

@opencontainers/runc-maintainers Can we get a quick review on this? This PR is important.

mrunalp · 2016-08-01T18:14:44Z

Testing this with systemd.

mrunalp · 2016-08-01T18:19:33Z

@hmeng-19 Could you test this PR on RHEL 7?

haiyanmeng · 2016-08-01T19:08:25Z

@mrunalp , I tested this on RHEL7.
All the bats tests finished successfully.
All the unit tests except for TestInitJoinNetworkAndUser (as mentioned in #915) also finished successfully.

mrunalp · 2016-08-01T19:48:34Z

@hmeng-19 Thanks for testing.

mrunalp · 2016-08-01T19:48:38Z

LGTM

haiyanmeng · 2016-08-01T19:51:38Z

@mrunalp , no problem.
To make it easy for others to test this PR, here is the testing command I used on RHEL7:

First, I created a basic config.json:

ocitools generate --tty --output=config.json \
--uidmappings=0:0:1024 --gidmappings=0:0:1024 \
--cgroups-path user.slice:hmeng:test2222 --mount-cgroups ro

Then add kernel memory limit into config.json:

        "memory": {
            "kernel": 50000000
        },

Finally, start the container and check the kernel memory limit:

runc --systemd-cgroup run 1234
runc run 1234

hqhq · 2016-08-02T02:04:01Z

LGTM

Follow up: opencontainers#962 Signed-off-by: Qiang Huang <[email protected]>

hqhq · 2016-08-02T02:12:16Z

@dubstack Sorry for not being specific, we should do more cleanups for GetLongBit, I've opened a follow up PR #968 for not blocking this.

GordonTheTurtle added the status/0-triage label Jul 23, 2016

dubstack force-pushed the fix_kmem_limits branch 2 times, most recently from a5e95f8 to 1a323f1 Compare July 23, 2016 01:30

dubstack force-pushed the fix_kmem_limits branch from 1a323f1 to 5106b56 Compare July 25, 2016 18:25

dubstack reviewed Jul 25, 2016
View reviewed changes

hqhq added a commit to hqhq/runc that referenced this pull request Jul 26, 2016

Use root kmem to detect if kmem limit have been initialized

f1accd6

Alternative of opencontainers#962 Fixes: opencontainers#841 Signed-off-by: Qiang Huang <[email protected]>

hqhq mentioned this pull request Jul 26, 2016

Use root kmem to detect if kmem limit have been initialized #965

Closed

dubstack force-pushed the fix_kmem_limits branch from 5106b56 to b2fe403 Compare July 27, 2016 23:49

vishh reviewed Jul 28, 2016
View reviewed changes

dubstack mentioned this pull request Jul 28, 2016

Bump Libcontainer to Latest Head kubernetes/kubernetes#29755

Closed

dubstack force-pushed the fix_kmem_limits branch from b2fe403 to bb706f9 Compare July 29, 2016 00:29

hqhq reviewed Jul 29, 2016
View reviewed changes

GordonTheTurtle added the dco/no label Jul 29, 2016

dubstack force-pushed the fix_kmem_limits branch from e742564 to 91825b2 Compare July 29, 2016 22:22

GordonTheTurtle removed the dco/no label Jul 29, 2016

hqhq reviewed Jul 30, 2016
View reviewed changes

Remove kmem Initialization check

fcd966f

Signed-off-by: Buddha Prakash <[email protected]>

dubstack force-pushed the fix_kmem_limits branch from 91825b2 to fcd966f Compare August 1, 2016 16:47

hqhq merged commit 50f0a2b into opencontainers:master Aug 2, 2016

hqhq added a commit to hqhq/runc that referenced this pull request Aug 2, 2016

Cleanup GetLongBit

777ac05

Follow up: opencontainers#962 Signed-off-by: Qiang Huang <[email protected]>

hqhq mentioned this pull request Aug 2, 2016

Cleanup GetLongBit #968

Merged

dubstack deleted the fix_kmem_limits branch August 2, 2016 18:15

cyphar mentioned this pull request Aug 3, 2016

sysconfig_notcgo does not implement GetLongBit #841

Closed

hqhq mentioned this pull request Oct 31, 2016

"sudo runc run <container_id>" generates error #953

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove kmem Initialization check while setting memory configuration #962

Remove kmem Initialization check while setting memory configuration #962

dubstack commented Jul 23, 2016 •

edited

Loading

mlaventure commented Jul 24, 2016

hqhq commented Jul 25, 2016

dubstack commented Jul 25, 2016

dubstack Jul 25, 2016

vishh Jul 25, 2016

dubstack commented Jul 28, 2016

mlaventure commented Jul 28, 2016

vishh Jul 28, 2016

vishh Jul 28, 2016

dubstack Jul 29, 2016

dubstack commented Jul 29, 2016

hqhq Jul 29, 2016

hqhq Jul 29, 2016

hqhq Jul 29, 2016

dubstack Jul 29, 2016

dubstack commented Jul 29, 2016

hqhq Jul 30, 2016

dubstack Aug 1, 2016

dubstack commented Aug 1, 2016

vishh commented Aug 1, 2016

mrunalp commented Aug 1, 2016

mrunalp commented Aug 1, 2016

haiyanmeng commented Aug 1, 2016

mrunalp commented Aug 1, 2016

mrunalp commented Aug 1, 2016 •

edited by caniszczyk

Loading

haiyanmeng commented Aug 1, 2016

hqhq commented Aug 2, 2016 •

edited

Loading

hqhq commented Aug 2, 2016

Remove kmem Initialization check while setting memory configuration #962

Remove kmem Initialization check while setting memory configuration #962

Conversation

dubstack commented Jul 23, 2016 • edited Loading

mlaventure commented Jul 24, 2016

hqhq commented Jul 25, 2016

dubstack commented Jul 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dubstack commented Jul 28, 2016

mlaventure commented Jul 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dubstack commented Jul 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dubstack commented Jul 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dubstack commented Aug 1, 2016

vishh commented Aug 1, 2016

mrunalp commented Aug 1, 2016

mrunalp commented Aug 1, 2016

haiyanmeng commented Aug 1, 2016

mrunalp commented Aug 1, 2016

mrunalp commented Aug 1, 2016 • edited by caniszczyk Loading

haiyanmeng commented Aug 1, 2016

hqhq commented Aug 2, 2016 • edited Loading

hqhq commented Aug 2, 2016

dubstack commented Jul 23, 2016 •

edited

Loading

mrunalp commented Aug 1, 2016 •

edited by caniszczyk

Loading

hqhq commented Aug 2, 2016 •

edited

Loading