Handling Pausing from freezer state

[rajasec]

@crosbymichael @mrunalp
With current runc implementation, container pause is handled from the config file, Added the contents to handle from the freezer subsystem and its freezer.state from the respective container.
This is to handle #217 which has been raised

Signed-off-by: Rajasekaran [email protected]

[wking]

As it stands with 48dd4af, here's the current mapping from
freezer.state text 1 to Go constants:

• THAWED → Running
• FREEZING → Running
• FROZEN → Paused
• No freezer subsystem → Running

I'd suggest we update our Go constants to reflect the kernel's values:

• THAWED → Thawed
• FREEZING → Freezing
• FROZEN → Frozen
• No freezer subsystem → Thawed

so folks can distinguish between THAWED and FREEZING. But I'm not
sure how other OSes handle this sort of state, so if we're trying to
strike a portable balance between their models, maybe Running / Paused
is the best we can do? In that case, lumping FREEZING and THAWED
together is probably a lesser evil than lumping FREEZING and FROZEN
together, because trying to freeze a FREEZING cgroup is idempotent
1, but trying to walk a FREEZING cgroup as if it were FROZEN could
give you an inconsistent picture.

[rajasec]

@wking @crosbymichael @LK4D4
I agree with your point on consistency with respect to states and kernel values. Do you want me to update in this PR or in different PR ?

[wking]

On Fri, Aug 21, 2015 at 10:25:26AM -0700, Rajasekaran wrote:

I agree with your point on consistency with respect to states and
kernel values. Do you want me to update in this PR or in different
PR ?

I'm not a maintainer (just adding my perspective as a user), so I
can't comment on how to split this among PRs.

Maybe this sort of model discussion should happen on the mailing list
1? If 48dd4af is how Docker currently handles the mapping, then
it's probably fine to leave this PR as it stands until there's
something in the spec defining the intended semantics.

[crosbymichael]

@rajasec this looks good as a way to find out if the container is frozen.

Lets factor out this code into a separate function and make sure that we have a specific error type if a system does not support the freezer cgroup. We don't want to the state method to fail if a system does not support freezer. So make sure you check for an IsNotExist error and ignore it in here.

[rajasec]

@crosbymichael
Added the separate function to handle the freezer. Ensured, state method will not fail if system does not supporting freezer.
Latest commit 86c1d6f

[rajasec]

@crosbymichael @wking
Here is the latest commit as per your comments.
b73aef5

[wking]

On Sat, Aug 22, 2015 at 12:14:09AM -0700, Rajasekaran wrote:

Here is the latest commit as per your comments.
b73aef5

You can probably drop everything after your first s-o-b in your commit
message.

I've filed a fixup PR against this branch (rajasec#1) with my
suggestion for dropping the racy Stat.

[wking]

On Sat, Aug 22, 2015 at 10:34:14AM -0700, W. Trevor King wrote:

I've filed a fixup PR against this branch (rajasec#1) with my
suggestion for dropping the racy Stat.

I also added another fixup to that PR (wking/runc@6f3b1d2ea3) that
handles the case where the cgroups path mapping doesn't contain a
"freezer" key. With your initial 48dd4af, you had a ‘path != ""’
check that might have been doing the same thing. The GetPaths() docs
aren't clear 1, but I'd expect unavailable cgroups to just not be
listed in the mapping, rather than having a key with an empty-string
value. That's how the filesystem-based manager works anyway 2.
It's unlikely that the empty string is a valid cgroup path, but it
seems better to use Go's builtin missing-key syntax than assuming that
the default string is always due to a missing-key lookup.

[rajasec]

@wking @crosbymichael
I'm not checking for paths != "", With my last commit I changed to stat of the file ( whether subsystem exists). As empty path can also be valid cgroup path.
we can check cgroups.IsNotFound function would give whether subsystem exists.

[wking]

On Sat, Aug 22, 2015 at 08:17:04PM -0700, Rajasekaran wrote:

I'm not checking for paths != "", With my last commit I changed to
stat of the file ( whether subsystem exists).

Right, in 1 I was just referencing your original commit.

As empty path can also be valid cgroup path. we can check
cgroups.IsNotFound function would give whether subsystem exists.

The explicit !ok check in wking/runc@6f3b1d2 (part of my fixup
rajasec#1) is distinguishing between “we don't have a freezer
group” (in which case, there's no need to een attempt to open a file)
and “we have a freezer group at ” (in which case I try to open
/freezer.state, without a Stat call because of
wking/runc@c07c601f, also in rajasec#1).

With your current b73aef5, you don't distinguish between “we don't
have a freezer group” and “we have a freezer group with an
empty-string path”. Conflating those two is probably not a big deal
(as I said in 1), but I still think that my !ok check is the better
way to go (if nothing else, it saves a syscall in the no-freezer-group
case).

[rajasec]

@crosbymichael
Your thoughts.. Do I need to make any further updates

[crosbymichael]

I think you are missing a return 0, err here

[rajasec]

@crosbymichael
2 things

1. If the freezer subsystem is not available part of container creation, we don't want to fail in this case. Container status will be running
2. func TestGetContainerState(t *testing.T) part of unit testing uses the container only with memory subsystem, To handle this situation too, we return running. Via this test only, it helped me to figure out container being created without cgroup freezer.

Mainly for the first point to handle system without freezer subsystem.

Even I thought that the same thing, it seems little odd for return Running.

[wking]

On Fri, Aug 28, 2015 at 08:49:17PM -0700, Rajasekaran wrote:

1. If the freezer subsystem is not available part of container
   creation, we don't want to fail in this case. Container status
   will be running
2. func TestGetContainerState(t *testing.T) part of unit testing
   uses the container only with memory subsystem, To handle this
   situation too, we return running. Via this test only, it helped
   me to figure out container being created without cgroup freezer.

(2) is just an instance of (1), so the comment can be just:

if os.IsNotExist(err) {
// there is no freezer subsystem, so the application must be running
return Running, nil

[rajasec]

@crosbymichael
Added the comment and committed the file.

[hqhq]

Maybe we should check both FROZEN and FREEZING here, we have pausing defined but never used, it should be returned here.

[wking]

On Mon, Sep 07, 2015 at 12:01:35AM -0700, Qiang Huang wrote:

Maybe we should check both FROZEN and FREEZING here, we have
pausing defined but never used, it should be returned here.

Previous thoughts on model state settings in [1,2,3]. But if we
already have state constants for each of Linux's three settings, I
think it makes sense to use them (at least until the runtime-agnostic
spec has something to say about freezing).

[rajasec]

@hqhq
Checkpointed is already checked before entering this function ( currentStatus function had the immediate check on checkpoint. At the last checkFreezer is called.

[rajasec]

@crosbymichael
For checking the pausing state from CLI level, would this change required part of runc ?

[crosbymichael]

@rajasec i cherry picked your commit into #311 so now we have robust freezer checking for the states. Thanks!