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

runtime: Change 'destroys state' to 'removes container' #213

Conversation

wking
Copy link
Contributor

@wking wking commented Oct 6, 2015

The runtime hasn't destroyed any of it's internal state before calling
the post-stop hooks (it still knows the state JSON it's going to pipe
to the hooks, and could still have the original configs in memory if
it wanted to keep that around). What's changed by the time we have
the post-stop hooks (and the reason to keep an explicit container ID
around) is that the kernel state (namespaces and cgroups) might be
gone by the time the post-stop hooks are run (any PID namespace that
was created by the container process will certainly be gone by then).
This commit updates the wording to more clearly mean "after removing
namespaces and cgroups" and to avoid folks interpreting it as "after
releasing some internal-to-the-runtime-process memory".

This line landed as part of #87, and the only review so far seems to
have been an “it's” → “its” typo fix.

If the “removes the container” phrasing bothers people for some
reason, I'm happy to reroll. I'm also happy to put the “destroys its
own state” phrasing back in if someone can clarify what state it was
talking about and it turns out that there is some important
internal-to-the-runtime entity that is being destroyed by post-stop
time (in which case I'd like to add additional wording to clarify that
entity in the spec).

The runtime hasn't destroyed any of it's internal state before calling
the post-stop hooks (it still knows the state JSON it's going to pipe
to the hooks, and could still have the original configs in memory if
it wanted to keep that around).  What's changed by the time we have
the post-stop hooks (and the reason to keep an explicit container ID
around) is that the *kernel* state (namespaces and cgroups) might be
gone by the time the post-stop hooks are run (any PID namespace that
was created by the container process will certainly be gone by then).
This commit updates the wording to more clearly mean "after removing
namespaces and cgroups" and to avoid folks interpreting it as "after
releasing some internal-to-the-runtime-process memory".

Signed-off-by: W. Trevor King <wking@tremily.us>
@crosbymichael
Copy link
Member

The runtime does not remove the container.

@wking
Copy link
Contributor Author

wking commented Oct 7, 2015

On Wed, Oct 07, 2015 at 11:57:24AM -0700, Michael Crosby wrote:

The runtime does not remove the container.

Who removes the container? With container defined as “the device
cgroup” (see 1), it looks like runC is removing it here [2,3].

@crosbymichael
Copy link
Member

Not one person said a container is defined as the device cgroup. You keep make up your own glossary and terms and try to insert them into the spec and this just adds confusion for everyone reading it. Cgroups are just one of the features in the linux kernel used to create a container.

@wking
Copy link
Contributor Author

wking commented Oct 7, 2015

On Wed, Oct 07, 2015 at 01:08:40PM -0700, Michael Crosby wrote:

Not one person said a container is defined as the device cgroup.
You keep make up your own glossary and terms and try to insert them
into the spec and this just adds confusion for everyone reading it.

I agree that landing a glossary (#107 or otherwise) would help a lot.

Cgroups are just one of the features in the linux kernel used to
create a container.

And the device cgroup is how runC currently decides what processes to
kill during cleanup [1,2,3] in the absence of a PID namespace. So the
union of PID namespace and device cgroup seems to define “container
processes”.

The runtime doesn't remove the PID namespace (that happens
automatically on container-process death), but it does remove the
device cgroup and other persistent portions of the container. Besides
the kernel's cleanup (PID namespace, etc.) and the runtime's cleanup
(device cgroup, etc.), is anyone else removing portions of the
container?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants