[lxc-devel] [PATCH 1/1] cgroup: recursively delete cgroups when asked
S.Çağlar Onur
caglar at 10ur.org
Mon Jan 13 17:09:12 UTC 2014
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>
More information about the lxc-devel
mailing list