[lxc-devel] [PATCH 1/1] cgroup: recursively delete cgroups when asked
Dwight Engen
dwight.engen at oracle.com
Mon Jan 13 22:09:08 UTC 2014
On Mon, 13 Jan 2014 14:53:40 -0500
"Michael H. Warfield" <mhw at WittsEnd.com> wrote:
> On Mon, 2014-01-13 at 12:38 -0500, Dwight Engen wrote:
> > On Mon, 13 Jan 2014 12:19:25 -0500
> > S.Çağlar Onur <caglar at 10ur.org> wrote:
>
> > > OK I think we need following
> > >
> > > diff --git a/src/lxc/confile.c b/src/lxc/confile.c
> > > index 50d09da..4208b0e 100644
> > > --- a/src/lxc/confile.c
> > > +++ b/src/lxc/confile.c
> > > @@ -2209,7 +2209,7 @@ void write_config(FILE *fout, struct
> > > lxc_conf *c) fprintf(fout, "lxc.seccomp = %s\n", c->seccomp);
> > > if (c->kmsg == 0)
> > > fprintf(fout, "lxc.kmsg = 0\n");
> > > - if (c->autodev)
> > > + if (c->autodev == 1)
> > > fprintf(fout, "lxc.autodev = 1\n");
> > > if (c->loglevel != LXC_LOG_PRIORITY_NOTSET)
> > > fprintf(fout, "lxc.loglevel = %s\n",
> > > lxc_log_priority_to_string(c->loglevel));
>
> > Hi Çağlar, sorry for messing that one up. It looks like autodev is
> > initialized to -1 which must be why the if check is catching. Other
> > places in conf.c are comparing for > 0 so maybe we should use that
> > instead of == 1?
>
> That's some of my stuff from months ago. That's to enable
> autodetection of certain conditions where we need to set the autodev
> default to be enabled. Serge, Stéphane, and I had been discussing
> how to reasonable detect when we should enable autodev by default and
> that settled in on whether systemd was in the readlink dereference
> for init. But we (I) also wanted an explicit enable / disable which
> resulted in the autodev option being a trinary of -1/0/1.
Okay, that makes sense. So does that mean that we should actually be
saving out the value when it is 0 and 1? ie. anything but the default
of -1? If so then my fix was still not quite right and the check needs
to be if (c->autodev != -1).
> Regards,
> Mike
>
> > > On Mon, Jan 13, 2014 at 12:09 PM, S.Çağlar Onur <caglar at 10ur.org>
> > > wrote:
> > > > On Mon, Jan 13, 2014 at 12:05 PM, S.Çağlar Onur
> > > > <caglar at 10ur.org> wrote:
> > > >> Hi Serge,
> > > >>
> > > >> On Mon, Jan 13, 2014 at 11:24 AM, Serge Hallyn
> > > >> <serge.hallyn at ubuntu.com> wrote:
> > > >>> Quoting S.Çağlar Onur (caglar at 10ur.org):
> > > >>>> Hey there,
> > > >>>>
> > > >>>> On Sun, Jan 12, 2014 at 7:24 PM, Stéphane Graber
> > > >>>> <stgraber at ubuntu.com> wrote:
> > > >>>> > On Sat, Jan 11, 2014 at 12:14:26AM -0600, Serge Hallyn
> > > >>>> > wrote:
> > > >>>> >> Currently when a container is shut down, lxc walks the
> > > >>>> >> set of all cgroup paths it created, in reverse order, and
> > > >>>> >> tries to remove them. This doesn't suffice if the
> > > >>>> >> container has also created new cgroups.
> > > >>>> >>
> > > >>>> >> It'd be impolite to recursively remove all the cgroup
> > > >>>> >> paths we created, since this can include '/lxc' and
> > > >>>> >> thereunder all other containers started since.
> > > >>>> >>
> > > >>>> >> This patch changes container shutdown to only delete the
> > > >>>> >> container's own path, but do so recursively. Note that
> > > >>>> >> if we fail during startup, the container won't have
> > > >>>> >> created any cgroup paths so it the old way works fine.
> > > >>>> >>
> > > >>>> >> Signed-off-by: Serge Hallyn <serge.hallyn at ubuntu.com>
> > > >>>> >
> > > >>>> > Acked-by: Stéphane Graber <stgraber at ubuntu.com>
> > > >>>>
> > > >>>> This patch started to cause my add_remove_device_node tests
> > > >>>> to fail (rmdir started to return EBUSY), following seems to
> > > >>>> solve it but I'm not sure whether it's a fix or just a
> > > >>>> workaround. Any ideas what might be triggering that?
> > > >>>
> > > >>> Are you sure it was that patch? You can drop my patch and
> > > >>> your test succeeds again?
> > > >>
> > > >> Hmm you are right, after a git bisect it pointed following
> > > >
> > > > [hit send too early]
> > > >
> > > > and yes reverting that commit fixes the issue
> > > >
> > > > [caglar at qp:~] for i in seq 100; do sudo
> > > > lxc-test-device-add-remove; done lxc_container: rmdir failed
> > > > Removing /dev/network_latency from the container
> > > > (device_add_remove_test) failed...
> > > > lxc_container: Container /var/lib/lxc:device_add_remove_test
> > > > already exists Creating the container (device_add_remove_test)
> > > > failed... [caglar at qp:~] sudo lxc-stop -n device_add_remove_test
> > > > [caglar at qp:~] sudo lxc-destroy -n device_add_remove_test
> > > >
> > > > [revert/install etc.]
> > > >
> > > > [caglar at qp:~] for i in seq 100; do sudo
> > > > lxc-test-device-add-remove; done [caglar at qp:~]
> > > >
> > > >
> > > >> df2d4205073d3f57543951ca7ffabf891b230634 is the first bad
> > > >> commit commit df2d4205073d3f57543951ca7ffabf891b230634
> > > >> Author: Dwight Engen <dwight.engen at oracle.com>
> > > >> Date: Thu Jan 9 15:36:13 2014 -0500
> > > >>
> > > >> ensure all config items are duplicated on
> > > >> clone/write_config
> > > >>
> > > >> Since previously I had found a config item that wasn't
> > > >> being propagated by lxc-clone, I went through all the config
> > > >> items and made sure that: a) Each item is documented in
> > > >> lxc.conf b) Each item is written out by write_config
> > > >>
> > > >> The only one that isn't is lxc.include, which by its nature
> > > >> only pulls in other config item types.
> > > >>
> > > >> Signed-off-by: Dwight Engen <dwight.engen at oracle.com>
> > > >> Acked-by: Serge E. Hallyn <serge.hallyn at ubuntu.com>
> > > >>
> > > >>>> diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c
> > > >>>> index 48fef74..80e68fb 100644
> > > >>>> --- a/src/lxc/lxccontainer.c
> > > >>>> +++ b/src/lxc/lxccontainer.c
> > > >>>> @@ -2934,7 +2934,7 @@ static bool
> > > >>>> add_remove_device_node(struct lxc_container *c, const char
> > > >>>> *src_path ERROR("unlink failed");
> > > >>>> goto out;
> > > >>>> }
> > > >>>> - if (rmdir(directory_path) < 0 && errno !=
> > > >>>> ENOTEMPTY) {
> > > >>>> + if (rmdir(directory_path) < 0 && errno !=
> > > >>>> ENOTEMPTY && errno != EBUSY) {
> > > >>>> ERROR("rmdir failed");
> > > >>>> goto out;
> > > >>>> }
> > > >>>>
> > > >>>>
> > > >>>> >> ---
> > > >>>> >> src/lxc/cgroup.c | 101
> > > >>>> >> ++++++++++++++++++++++++++++++++++++++++++++++++------- 1
> > > >>>> >> file changed, 88 insertions(+), 13 deletions(-)
> > > >>>> >>
> > > >>>> >> diff --git a/src/lxc/cgroup.c b/src/lxc/cgroup.c
> > > >>>> >> index b1196b4..be43c73 100644
> > > >>>> >> --- a/src/lxc/cgroup.c
> > > >>>> >> +++ b/src/lxc/cgroup.c
> > > >>>> >> @@ -68,9 +68,9 @@ static char
> > > >>>> >> **subsystems_from_mount_options(const char *mount_options,
> > > >>>> >> char **ke static void lxc_cgroup_mount_point_free(struct
> > > >>>> >> cgroup_mount_point *mp); static void
> > > >>>> >> lxc_cgroup_hierarchy_free(struct cgroup_hierarchy *h);
> > > >>>> >> static bool is_valid_cgroup(const char *name); -static int
> > > >>>> >> create_or_remove_cgroup(bool remove, struct
> > > >>>> >> cgroup_mount_point *mp, const char *path); +static int
> > > >>>> >> create_or_remove_cgroup(bool remove, struct
> > > >>>> >> cgroup_mount_point *mp, const char *path, int recurse);
> > > >>>> >> static int create_cgroup(struct cgroup_mount_point *mp,
> > > >>>> >> const char *path); -static int remove_cgroup(struct
> > > >>>> >> cgroup_mount_point *mp, const char *path); +static int
> > > >>>> >> remove_cgroup(struct cgroup_mount_point *mp, const char
> > > >>>> >> *path, bool recurse); static char
> > > >>>> >> *cgroup_to_absolute_path(struct cgroup_mount_point *mp,
> > > >>>> >> const char *path, const char *suffix); static struct
> > > >>>> >> cgroup_process_info *find_info_for_subsystem(struct
> > > >>>> >> cgroup_process_info *info, const char *subsystem); static
> > > >>>> >> int do_cgroup_get(const char *cgroup_path, const char
> > > >>>> >> *sub_filename, char *value, size_t len); @@ -81,6 +81,75
> > > >>>> >> @@ static int cgroup_recursive_task_count(const char
> > > >>>> >> *cgroup_path); static int count_lines(const char *fn);
> > > >>>> >> static int handle_cgroup_settings(struct
> > > >>>> >> cgroup_mount_point *mp, char *cgroup_path);
> > > >>>> >>
> > > >>>> >> +static int cgroup_rmdir(char *dirname)
> > > >>>> >> +{
> > > >>>> >> + struct dirent dirent, *direntp;
> > > >>>> >> + int saved_errno = 0;
> > > >>>> >> + DIR *dir;
> > > >>>> >> + int ret, failed=0;
> > > >>>> >> + char pathname[MAXPATHLEN];
> > > >>>> >> +
> > > >>>> >> + dir = opendir(dirname);
> > > >>>> >> + if (!dir) {
> > > >>>> >> + ERROR("%s: failed to open %s", __func__,
> > > >>>> >> dirname);
> > > >>>> >> + return -1;
> > > >>>> >> + }
> > > >>>> >> +
> > > >>>> >> + while (!readdir_r(dir, &dirent, &direntp)) {
> > > >>>> >> + struct stat mystat;
> > > >>>> >> + int rc;
> > > >>>> >> +
> > > >>>> >> + if (!direntp)
> > > >>>> >> + break;
> > > >>>> >> +
> > > >>>> >> + if (!strcmp(direntp->d_name, ".") ||
> > > >>>> >> + !strcmp(direntp->d_name, ".."))
> > > >>>> >> + continue;
> > > >>>> >> +
> > > >>>> >> + rc = snprintf(pathname, MAXPATHLEN, "%s/%s",
> > > >>>> >> dirname, direntp->d_name);
> > > >>>> >> + if (rc < 0 || rc >= MAXPATHLEN) {
> > > >>>> >> + ERROR("pathname too long");
> > > >>>> >> + failed=1;
> > > >>>> >> + if (!saved_errno)
> > > >>>> >> + saved_errno = -ENOMEM;
> > > >>>> >> + continue;
> > > >>>> >> + }
> > > >>>> >> + ret = lstat(pathname, &mystat);
> > > >>>> >> + if (ret) {
> > > >>>> >> + SYSERROR("%s: failed to stat %s",
> > > >>>> >> __func__, pathname);
> > > >>>> >> + failed=1;
> > > >>>> >> + if (!saved_errno)
> > > >>>> >> + saved_errno = errno;
> > > >>>> >> + continue;
> > > >>>> >> + }
> > > >>>> >> + if (S_ISDIR(mystat.st_mode)) {
> > > >>>> >> + if (cgroup_rmdir(pathname) < 0) {
> > > >>>> >> + if (!saved_errno)
> > > >>>> >> + saved_errno = errno;
> > > >>>> >> + failed=1;
> > > >>>> >> + }
> > > >>>> >> + }
> > > >>>> >> + }
> > > >>>> >> +
> > > >>>> >> + if (rmdir(dirname) < 0) {
> > > >>>> >> + SYSERROR("%s: failed to delete %s",
> > > >>>> >> __func__, dirname);
> > > >>>> >> + if (!saved_errno)
> > > >>>> >> + saved_errno = errno;
> > > >>>> >> + failed=1;
> > > >>>> >> + }
> > > >>>> >> +
> > > >>>> >> + ret = closedir(dir);
> > > >>>> >> + if (ret) {
> > > >>>> >> + SYSERROR("%s: failed to close directory %s",
> > > >>>> >> __func__, dirname);
> > > >>>> >> + if (!saved_errno)
> > > >>>> >> + saved_errno = errno;
> > > >>>> >> + failed=1;
> > > >>>> >> + }
> > > >>>> >> +
> > > >>>> >> + errno = saved_errno;
> > > >>>> >> + return failed ? -1 : 0;
> > > >>>> >> +}
> > > >>>> >> +
> > > >>>> >> struct cgroup_meta_data *lxc_cgroup_load_meta()
> > > >>>> >> {
> > > >>>> >> const char *cgroup_use = NULL;
> > > >>>> >> @@ -745,7 +814,7 @@ extern struct cgroup_process_info
> > > >>>> >> *lxc_cgroup_create(const char *name, const cha
> > > >>>> >> * In that case, remove the cgroup from all
> > > >>>> >> previous hierarchies */
> > > >>>> >> for (j = 0, info_ptr = base_info; j < i &&
> > > >>>> >> info_ptr; info_ptr = info_ptr->next, j++) {
> > > >>>> >> - r =
> > > >>>> >> remove_cgroup(info_ptr->designated_mount_point,
> > > >>>> >> info_ptr->created_paths[info_ptr->created_paths_count -
> > > >>>> >> 1]);
> > > >>>> >> + r =
> > > >>>> >> remove_cgroup(info_ptr->designated_mount_point,
> > > >>>> >> info_ptr->created_paths[info_ptr->created_paths_count -
> > > >>>> >> 1], false); if (r < 0) WARN("could not clean up cgroup we
> > > >>>> >> created when trying to create container");
> > > >>>> >> free(info_ptr->created_paths[info_ptr->created_paths_count
> > > >>>> >> - 1]); @@ -1039,8 +1108,7 @@ void
> > > >>>> >> lxc_cgroup_process_info_free_and_remove(struct
> > > >>>> >> cgroup_process_info *info) if (!info) return; next =
> > > >>>> >> info->next;
> > > >>>> >> - for (pp = info->created_paths; pp && *pp; pp++);
> > > >>>> >> - for ((void)(pp && --pp); info->created_paths && pp
> > > >>>> >> >= info->created_paths; --pp) {
> > > >>>> >> + {
> > > >>>> >> struct cgroup_mount_point *mp =
> > > >>>> >> info->designated_mount_point; if (!mp)
> > > >>>> >> mp =
> > > >>>> >> lxc_cgroup_find_mount_point(info->hierarchy,
> > > >>>> >> info->cgroup_path, true); @@ -1049,7 +1117,10 @@ void
> > > >>>> >> lxc_cgroup_process_info_free_and_remove(struct
> > > >>>> >> cgroup_process_info *info)
> > > >>>> >> * '/lxc' cgroup in this container
> > > >>>> >> but another container
> > > >>>> >> * is still running (for example)
> > > >>>> >> */
> > > >>>> >> - (void)remove_cgroup(mp, *pp);
> > > >>>> >> + (void)remove_cgroup(mp,
> > > >>>> >> info->cgroup_path, true);
> > > >>>> >> + }
> > > >>>> >> + for (pp = info->created_paths; pp && *pp; pp++);
> > > >>>> >> + for ((void)(pp && --pp); info->created_paths && pp
> > > >>>> >> >= info->created_paths; --pp) { free(*pp);
> > > >>>> >> }
> > > >>>> >> free(info->created_paths);
> > > >>>> >> @@ -1609,7 +1680,7 @@ bool is_valid_cgroup(const char
> > > >>>> >> *name) return strcmp(name, ".") != 0 && strcmp(name,
> > > >>>> >> "..") != 0; }
> > > >>>> >>
> > > >>>> >> -int create_or_remove_cgroup(bool do_remove, struct
> > > >>>> >> cgroup_mount_point *mp, const char *path) +int
> > > >>>> >> create_or_remove_cgroup(bool do_remove, struct
> > > >>>> >> cgroup_mount_point *mp, const char *path, int recurse)
> > > >>>> >> { int r, saved_errno = 0; char *buf =
> > > >>>> >> cgroup_to_absolute_path(mp, path, NULL); @@ -1617,9
> > > >>>> >> +1688,13 @@ int create_or_remove_cgroup(bool do_remove,
> > > >>>> >> struct cgroup_mount_point *mp, const return -1;
> > > >>>> >>
> > > >>>> >> /* create or remove directory */
> > > >>>> >> - r = do_remove ?
> > > >>>> >> - rmdir(buf) :
> > > >>>> >> - mkdir(buf, 0777);
> > > >>>> >> + if (do_remove) {
> > > >>>> >> + if (recurse)
> > > >>>> >> + r = cgroup_rmdir(buf);
> > > >>>> >> + else
> > > >>>> >> + r = rmdir(buf);
> > > >>>> >> + } else
> > > >>>> >> + r = mkdir(buf, 0777);
> > > >>>> >> saved_errno = errno;
> > > >>>> >> free(buf);
> > > >>>> >> errno = saved_errno;
> > > >>>> >> @@ -1628,12 +1703,12 @@ int create_or_remove_cgroup(bool
> > > >>>> >> do_remove, struct cgroup_mount_point *mp, const
> > > >>>> >>
> > > >>>> >> int create_cgroup(struct cgroup_mount_point *mp, const
> > > >>>> >> char *path) {
> > > >>>> >> - return create_or_remove_cgroup(false, mp, path);
> > > >>>> >> + return create_or_remove_cgroup(false, mp, path,
> > > >>>> >> false); }
> > > >>>> >>
> > > >>>> >> -int remove_cgroup(struct cgroup_mount_point *mp, const
> > > >>>> >> char *path) +int remove_cgroup(struct cgroup_mount_point
> > > >>>> >> *mp, const char *path, bool recurse) {
> > > >>>> >> - return create_or_remove_cgroup(true, mp, path);
> > > >>>> >> + return create_or_remove_cgroup(true, mp, path,
> > > >>>> >> recurse); }
> > > >>>> >>
> > > >>>> >> char *cgroup_to_absolute_path(struct cgroup_mount_point
> > > >>>> >> *mp, const char *path, const char *suffix) --
> > > >>>> >> 1.8.5.2
> > > >>>> >>
> > > >>>> >> _______________________________________________
> > > >>>> >> lxc-devel mailing list
> > > >>>> >> lxc-devel at lists.linuxcontainers.org
> > > >>>> >> http://lists.linuxcontainers.org/listinfo/lxc-devel
> > > >>>> >
> > > >>>> > --
> > > >>>> > Stéphane Graber
> > > >>>> > Ubuntu developer
> > > >>>> > http://www.ubuntu.com
> > > >>>> >
> > > >>>> > _______________________________________________
> > > >>>> > lxc-devel mailing list
> > > >>>> > lxc-devel at lists.linuxcontainers.org
> > > >>>> > http://lists.linuxcontainers.org/listinfo/lxc-devel
> > > >>>> >
> > > >>>>
> > > >>>>
> > > >>>>
> > > >>>> --
> > > >>>> S.Çağlar Onur <caglar at 10ur.org>
> > > >>>> _______________________________________________
> > > >>>> lxc-devel mailing list
> > > >>>> lxc-devel at lists.linuxcontainers.org
> > > >>>> http://lists.linuxcontainers.org/listinfo/lxc-devel
> > > >>> _______________________________________________
> > > >>> lxc-devel mailing list
> > > >>> lxc-devel at lists.linuxcontainers.org
> > > >>> http://lists.linuxcontainers.org/listinfo/lxc-devel
> > > >>
> > > >>
> > > >>
> > > >> --
> > > >> S.Çağlar Onur <caglar at 10ur.org>
> > > >
> > > >
> > > >
> > > > --
> > > > S.Çağlar Onur <caglar at 10ur.org>
> > >
> > >
> > >
> >
> > _______________________________________________
> > lxc-devel mailing list
> > lxc-devel at lists.linuxcontainers.org
> > http://lists.linuxcontainers.org/listinfo/lxc-devel
> >
>
More information about the lxc-devel
mailing list