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: Add more steps to the lifecycle docs #207

Closed
wants to merge 3 commits into from

Conversation

wking
Copy link
Contributor

@wking wking commented Sep 30, 2015

And explain how the handling differs when an application joins an
existing container vs. creating a new container. This is spun off
from the mailing list thread now that that's settled down.

This write-up is just about Linux containers, since I don't understand
the other systems well enough to do them justice. I'm happy to roll
in notes for other platforms if folks know what to write ;).

I've pulled the hook-environment change I made in this post out
into a separate commit, and I'm happy to push that as a separate PR if
it makes review easier.

A typical lifecyle progresses like this:

1. There is no container or running application
2. A user tells the runtime to start a container+application
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

start a container for an application.. ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A user tells the runtime to start a container Don't use the word application, its just a container

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think it makes sense to leave out application altogether.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Wed, Sep 30, 2015 at 11:52:37AM -0700, Mrunal Patel wrote:

+2. A user tells the runtime to start a container+application

start a container for an application.. ?

Without explicit create and start steps, it's “to create a container
and launch an application inside that container”, but that seemed too
long. Should I just replace “container+application” with “container
and application”?

On IRC on 2015-09-28:

  11:28 < jojy_mesos> i guess i was expecting an additional attribute
    for hooks that specifies whether the hooks runs in the host
    namespace or in the container's namespace
  11:28 < lk4d4> they all run in host namespaces

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

wking commented Sep 30, 2015

I've rebased on the new master and added 80c1729 since #202 landed. I'm not sure what the intended behavior isif the application dies while we're running post-start hooks. In 80c1729 I've gone with “we terminate any in-progress post-start hook and skip any that hadn't been called yet”, but “terminate” is vague (TERM? KILL? TERM and KILL after 5 seconds if necessary?). @LK4D4, do you want something different than the 80c1729 phrasing?

@wking wking force-pushed the lifecycle branch 2 times, most recently from d421f51 to e0ec87b Compare September 30, 2015 22:43
@justincormack
Copy link
Contributor

Is this supposed to be illustrative or normative? It might be clearer to insert normative language where useful.

Destroying the "container process" (might be a better term, its usually going to be container pid 1 but I suppose you might not have a pid namespace) will often implicitly destroy the container, so I would allow for that to happen, so hooks after that may not be able to see container any more.

@wking
Copy link
Contributor Author

wking commented Oct 1, 2015

On Thu, Oct 01, 2015 at 02:23:34AM -0700, Justin Cormack wrote:

Is this supposed to be descriptive or normative?

I'd rather have the summaries in the enumerated list be informative
(so you can cut a few corners there), but the sections they link to be
normative (so everyone is on the same page for what's really
happening).

Destroying the "container process" (might be a better term, its
usually going to be container pid 1 but I suppose you might not have
a pid namespace) will often implicitly destroy the container, so I
would allow for that to happen, so hooks after that may not be able
to see container any more.

I don't see “destroy” in the language through e0ec87b. Are you
suggesting that language for:

  1. The runtime terminates any other processes in the container

This termination is qualified by the “Joining existing containers”
section, that clarifies the “other processes in the container” means
“other processes in any cgroup you created” (at least on Linux). I'm
happy to roll in changes that make that more clear (maybe in 2
itself?), if you have suggestions for the text you'd like to see.

This would also be easier if we landed a glossary like that proposed
in #107, although that doesn't currently equate “container processes”
with “processes in a unified-hierarchy cgroup” like we do in this PR.

And explain how the handling differs when a new start-call joins an
existing container vs. creating a new container.  This is spun off
from the mailing list thread now that that's settled down [1].

This write-up is just about Linux containers, since I don't understand
the other systems well enough to do them justice.

[1]: https://groups.google.com/a/opencontainers.org/forum/#!topic/dev/frUXLljXy8Y
     Message-ID: <20150915034717.GO18018@odin.tremily.us>

Signed-off-by: W. Trevor King <wking@tremily.us>
These just landed via opencontainers#202.  I'm not entirely clear on how we intend
to handle "the application died before we finished executing post-stop
hooks", but I've made my best guess here.

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

wking commented Oct 6, 2015

I did a bit more digging in runC's cleanup implementation yesterday,
and here's what it's currently doing:

  1. If the container process was the first in a PID namespace, it's
    death will automatically cleanup any other processes in that
    namespace. Otherwise, the current runC code kills the other
    processes in the cgroup [1,2]. The ‘Freeze’ function requires a
    freezer cgroup 3, but the killer carries on if it fails 4. The
    ‘GetPids’ function looks at the device cgroup 5. The ‘Kill’ call
    sends a SIGKILL [6,7,8,9]. Wait for the killed processes to exit
    10.
  2. Remove any created cgroups 11
  3. Remove /run/opencontainer/containers/{container-id} 12.
  4. Run the post-stop hooks 13.

I'm not sure when persistent namespaces (e.g. network namespaces) are
removed.

That's a bit different from the cleanup logic currently proposed here:

a. I suggest running the post-stop hooks before cleaning up the
container.
b. I suggest removing the state.json file first (with the just-pushed
e0ec87b717a999) to be the inverse of the container-creation
process, where state.json is written last.
c. I require a unified hierarchy, which I thought was our intention
after 429f936 (Adding cgroups path to the Spec, 2015-09-02, #137)
14. The unified hierarchy is still experimental 15, but it
makes handling containers easier because we don't have to create a
PID namespace or device cgroup before we can easily define “the
process belonging to the container”. With the unified hierarchy
(‘mount -t cgroup -o __DEVEL__sane_behavior cgroup $MOUNT_POINT’),
you can add processes to a cgroup that doesn't use any controllers,
which is what you'd want if someone left linux.devices and
linux.resources out of their runtime.json. I don't know of a good
way to do this with the split hierarchy, although the devices
controller seems like a reasonable choice for a canonical
hierarchy, since you can join one without configuring it and keep
the default matching the parent 16.

I think this PR's current approach to (a) and (b) makes sense, and it
would be easy to update runC to match. I'm less clear on (c), since I
like the unified-hierarchy approach, but am cautious about relying on
an experimental kernel interface. Maybe we should relax our
unified-hierarchy phrasing and explicitly mention the device
controller as one that must be created and whose cgroup.procs defines
the set of container processes.

@@ -12,27 +12,28 @@ Presently there are `Prestart`, `Poststart` and `Poststop`.
Hooks allow one to run code before/after various lifecycle events of the container.
Hooks MUST be called in the listed order.
The state of the container is passed to the hooks over stdin, so the hooks could get the information they need to do their work.
All hooks execute in the host environment (e.g. the same namespace, cgroups, etc. that apply to the host process).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is incorrect. They are forked from the runtime process so they inherit from the runtime, not all host processes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Wed, Oct 07, 2015 at 11:36:22AM -0700, Michael Crosby wrote:

+All hooks execute in the host environment (e.g. the same
namespace, cgroups, etc. that apply to the host process).

This is incorrect. They are forked from the runtime process so they
inherit from the runtime, not all host processes.

There are at least two runtime processes here. There is a process
launched with (for example) ‘runC start’, which I've been referring to
as the “host process”. That process forks and the child ends up in
the container, eventually running the config.json process binary. I'd
been referring to the latter as the “container process”. Do you
prefer alternative names for those two processes?

@vishh
Copy link
Contributor

vishh commented Oct 12, 2015

I find this PR introducing concepts to the Spec that are implementation details of runC. Input from folks working on implementing the Spec in non-Linux OSes will definitely help.

@wking
Copy link
Contributor Author

wking commented Oct 12, 2015

On Mon, Oct 12, 2015 at 03:50:19PM -0700, Vish Kannan wrote:

I find this PR introducing concepts to the Spec that are
implementation details of runC. Input from folks working on
implementing the Spec in non-Linux OSes will definitely help.

The initial comment for this PR points out that it's just about Linux
containers. And as I said there, I'm happy to roll in notes for other
platforms if folks know what to write.

And the goal on Linux is to document what runC does now 1, since
that's something that is easier to agree on.

So +1 to feedback like “on Windows, container creation will need
language like …” or “in commit abc, runC is actually cleaning up
containers by doing …”. But I think feedback like “I think we should
use the unified-hierarchy 2” should be spun off to the list (it's
mentioned as an option in 3) and not handled here.

@vishh
Copy link
Contributor

vishh commented Oct 12, 2015

@wking: I suggest closing this PR and starting an email thread to solicit feedback from other members.
I hope the existing comments gives you an idea as to what is required to come up with a working proposal.

@wking
Copy link
Contributor Author

wking commented Oct 13, 2015

On Mon, Oct 12, 2015 at 04:57:21PM -0700, Vish Kannan wrote:

@wking: I suggest closing this PR and starting an email thread to
solicit feedback from other members. I hope the existing comments
gives you an idea as to what is required to come up with a working
proposal.

Email thread exists [1](also linked in this PR's original post). I
moved it to a PR after that thread settled down and I got clearance in
a meeting (see action items in 2). I'm happy to continue discussion
in the thread if that's easier for everyone else, but I'm still not
sure what changes you're looking for.

@crosbymichael
Copy link
Member

I think there is still a lot of discussion needed on the lifecycle but this is not an accurate base to start from and will end up being more work if we iterate on this PR instead of discussing first and starting fresh.

We need to make this detailed but having some of the people who work on runtimes comment as wells as abstract where this works cross platform. We also need to figure out why we are defining the lifecycle and the types of issues it solves by having this defined or else it will be targeted at a something that is not useful for a consumer.

@wking
Copy link
Contributor Author

wking commented Oct 13, 2015

On Mon, Oct 12, 2015 at 05:16:15PM -0700, Michael Crosby wrote:

I think there is still a lot of discussion needed on the lifecycle
but this is not an accurate base to start from and will end up being
more work if we iterate on this PR instead of discussing first and
starting fresh.

Fair enough. @mrunalp volunteered to write one up 1, and this
attempt was because he hadn't gotten around it yet 2. I'd even be
fine just copying over runC's libcontainer/SPEC.md and then iterating
on that in more tightly-focused PRs.

We also need to figure out why we are defining the lifecycle and the
types of issues it solves by having this defined or else it will be
targeted at a something that is not useful for a consumer.

I think this is pretty clear. For example, it's not currently clear
whether we can rely on the ‘pid’ entry being there in the pre-start
and/or post-stop hooks, what portions of the existing container are
available for pre-start hooks to work with (see #115), what gets
cleaned up when a container exits, what “container processes” even
means 3, what “standard operations” means 4, or what a runtime API
should look like 5. All of these are difficult discussions to have
without specs for what we think a container lifecycle looks like.

@wking wking mentioned this pull request Mar 18, 2016
wking added a commit to wking/nmbug-oci that referenced this pull request Jul 26, 2017
My pull request was rejected on 2015-10-12 [1], but Mrunal filed an
alternative which landed on 2015-12-09 [2].

[1]: opencontainers/runtime-spec#207 (comment)
[2]: opencontainers/runtime-spec#231 (comment)
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.

5 participants