[lxc-devel] [PATCH 2/2] split cgroup handling into discrete backends

[Dwight Engen]

On Thu, 6 Feb 2014 11:30:35 -0600
Serge Hallyn <serge.hallyn at ubuntu.com> wrote:

> Quoting Dwight Engen (dwight.engen at oracle.com):
> > On Thu, 6 Feb 2014 10:49:22 -0600
> > Serge Hallyn <serge.hallyn at ubuntu.com> wrote:
> > 
> > > Quoting Dwight Engen (dwight.engen at oracle.com):
> > > > On Wed, 5 Feb 2014 23:34:16 -0600
> > > > Serge Hallyn <serge.hallyn at ubuntu.com> wrote:
> > > > 
> > > > > Quoting Serge Hallyn (serge.hallyn at ubuntu.com):
> > > > > > Quoting Dwight Engen (dwight.engen at oracle.com):
> > > > > > > - refactor cgroup into two backends, the classic cgfs
> > > > > > > driver and the new cgmanager. Instead of lxc_handler
> > > > > > > knowing about the internals of each, have it just store
> > > > > > > an opaque pointer to a struct that is private to each
> > > > > > > backend.
> > > > > > > 
> > > > > > > - rename a couple of cgroup functions for consistency:
> > > > > > > those that are considered an API (ie. exported by lxc.h)
> > > > > > > begin with lxc_ and those that are not are just cgroup_*
> > > > > > > 
> > > > > > > - made as many backend routines static as possible, only
> > > > > > > cg*_ops_init is exported
> > > > > > > 
> > > > > > > - made a nrtasks op which is needed by the utmp code for
> > > > > > > monitoring container shutdown, currently only implemented
> > > > > > > for the cgfs backend
> > > > > > 
> > > > > > Hi Dwight,
> > > > > > 
> > > > > > boy this diff got into some unfortunate strides (with
> > > > > > near-identical code being removed then inserted pages later)
> > > > > > making it harder to read than necessary :)
> > > > > > 
> > > > > > > Signed-off-by: Dwight Engen <dwight.engen at oracle.com>
> > > > > > 
> > > > > > Acked-by: Serge E. Hallyn <serge.hallyn at ubuntu.com>
> > > > > > 
> > > > > > (I'll apply after I run some tests)
> > > > > 
> > > > > Unfortunately, while containers started fine, lxc-attach and
> > > > > lxc-cgroup no longer seemed to work.  If it isn't obvious to
> > > > > you what the problem migth be, I'll look into it tomorrow.
> > > > 
> > > > Hmm, thats strange. I tested both of those with both backends.
> > > > I'll try to reproduce it here. I also tested the shutdown while
> > > > frozen case and at first I thought it was broken but then I
> > > > realized that you just have to wait for the timeout to occur
> > > > (which is the same as it used to be).
> > > 
> > > Works now, not sure what I did last night.
> > > 
> > > Pushed - thanks.
> > > 
> > > -serge
> > 
> > Hey Serge,
> > 
> > I do have one more question: do we expect that cgmanager will only
> > be used with a kernel that supports reboot(2) for container
> > shutdown? (ie. kernel commit 923c7538). Looks like this has been in
> > since 3.9.
> 
> No we expect it to work with older kernels (at least back to 3.2),
> else the code could have been quite a bit simpler in some places.
> 
> > I ask because I tested that the utmp watch stuff still works for
> > cgfs, but obviously it doesn't right now with cgmanager since the
> > nrtasks op isn't filled in.
> 
> Should be trivial to fill in, using GetTasks and counting the results.
> Do you think that a separate NrTasks dbus method would be worthwhile?

No I think using GetTasks and counting is fine. It looks to me like
what the utmp stuff really wants to know is just that the container has
shut down, and nrtasks is the way its doing that. I don't see that
systemd/machined has an equivalent interface for nrtasks but it does
emit a MachineRemoved dbus signal so maybe we could use that, at least
for the utmp case.

> For what it's worth, some other methods which I do plan to implement
> at some point but are lower priority for me (as I don't need them)
> include GetChildren (see list of child cgroups), get_eventfd (to get
> something to watch for changes), and remove_on_empty.  RemoveOnEmpty
> is waiting for a redisgned notify-on-release which Tejun has said to
> be working on right now, and likewise get_eventfd is waiting to see
> how that is re-designed.  Patches for all of these welcomem :)

Ah yep, now I see those in the xml under "still to add: low priority" :)

> -serge