Fixing container start when cgroup subsystem not found

[rajasec]

With current runc implementation, if the cgroup subsystem is not mounted in the host, subsequent directories are not created inside the container. But Set interface is called to write to respective subsystem attributes ( if the runtime.json had the right values). In that case container failed to start.

Example
In runtime.json, I've added the cpu shares and blkio related parameters
After unmounting the cpu and blkio subsystem from host, the container failed to start due to following errors
FATA[0000] Container start failed: [8] System error: no such directory for cpu.shares

As directories are not created if subsystem is not mounted, Let us try not to set the values for the respective subsystems ( if not mounted) and gracefully start the container instead of failing

Signed-off-by: Rajasekaran rajasec79@gmail.com

[mrunalp]

I don't think that we should silently continue on if some subsystems aren't mounted. Failing is better.

[crosbymichael]

Yes, we had this same discussion in the past when this repository was libcontainer. I can try to find the discussion again for history but there are some subsystems that are optional but we want a hard fail because if you don't have some of these cgroups then we cannot guarantee some of the aspects of a container.

[rajasec]

@crosbymichael @mrunalp
Thanks for your quick review on this PR.

As per your comment, we want hard fail,
we should fail at the beginning itself when subsystem is not found or mounted.

I meant to say when subsystem is not mounted or not available, don't fail during set operation, fail during Join operation, where it returns cgroup subsystem not found.
dir, err := d.join("blkio")
here itself we can fail cgroups.IsNotFound(err)
We don't need to allow to set and then fail.

With current implementation, memory subsystem is handled this way too(silently continues). is it different than other subsystems ?

Moreover with current implementation
Failure will happen only if we set some appropriate values in runtime.json for not mounted cgroup subsystems, otherwise we wont see the failure during start of container.

If it is 0 or null, container start ignores during set operations ( for not mounted cgroup subsystems)

[crosbymichael]

@rajasec is there a reason why you are hitting this issue? What is the root cause?

[rajasec]

@crosbymichael
Problem:
As ubuntu kernel using the deadline scheduler as default, blkio setting was not effective inside the container ( as blkio works only for CFQ), So I'was unmounting the unnecessary file system(blkio) to be mounted. During that operations I have removed/unmounted cpu too ( instead of blkio)
But the problem started from here.
Since I use to specify cpu shares ( non-zero value) in my runtime.json, container failed to start due to cpu shares
Gave the following error
FATA[0000] Container start failed: [8] System error: no such directory for cpu.shares

Root cause
cpu.go
dir, err := d.join("cpu")
if err != nil && !cgroups.IsNotFound(err) {
return err
}
After executing above code, err gave the "mount point for cpu not found" and cgroups.IsNotFound (err) also gives for proper value ( as the subsystem is not found)
since we are negating with cgroups.IsNotFound(err) this error got ignored.
This error got ignored& bypassed, even though the subsystem is not found.

So it went ahead to set the values
if err := s.Set(dir, d.c); err != nil {
return err

it failed during setting up of cpu.shares, because the directory or mountpoint does not exists.

This PR handles, if specific subsystem is not found in the host, Don't try to set the values instead return back ( similar to memory subsystem)
it is not about gracefully starting the container.

[hqhq]

@rajasec @crosbymichael The story was:

• We did things like this PR does before in fix some cgroups issues docker-archive/libcontainer#437
• Then it was reverted by Revert "cgroups: only return path when subsystem really mounted docker-archive/libcontainer#474 because we want it fail hard when a user specifies an option and we are unable to fulfill the request
• The revert broke something, so we did it again in another way in don't fail when subsystem not mounted docker-archive/libcontainer#476 , that's mostly what we have now.

[hqhq]

@rajasec You are right memory subsystem is different from other optional subsystems, that's wrong, I opened a PR #343 to fix this.

[crosbymichael]

@hqhq what should we do with this PR then?

[hqhq]

@crosbymichael The main problem @rajasec got in this PR is intended, the other problem he mentioned in the comments is fixed by #343 , so I think we can close this PR then.

[rajasec]

@hqhq
Before we close this PR, one thing I would need few confirmation

I agree that we need hard failure.

But if subsystem is not found or not mounted, my question is why we go and fail in set operation instead we can fail at join operation itself ? in that case , we don't even allow the code to do set of values.

Also cgroups.IsNotFound(err) function is not having much meaning, as we always negate when subsystem not found

[hqhq]

But if subsystem is not found or not mounted, my question is why we go and fail in set operation instead we can fail at join operation itself ? in that case , we don't even allow the code to do set of values.

I agree fail at set is not perfect, if you want to fail at join or right after join, you need a function to get if that subsystem requested some valid values, something like function memoryAssigned I added in #343 , that doesn't seems necessary, so I think it's hard to tell which one is better. Fail at set is the simplest, it didn't add much code or complex. But I'm open with other options, you are always welcome to fire a PR, let's see if we can make it better :)

Also cgroups.IsNotFound(err) function is not having much meaning, as we always negate when subsystem not found

Sorry I don't get it, what do you mean "we always negate when subsystem not found"? If subsystem is not found and we are not using it (by assigning any valid values in runtime.json), we don't negate it.

[rajasec]

@hqhq @crosbymichael
As you are saying the changes are much intentional, I'm closing this PR.
Thanks for your reviews.