[CRIU] [PATCH] cg: don't try to copy special props to cpuset:/

[Tycho Andersen]

On Fri, Oct 03, 2014 at 07:02:03PM +0400, Pavel Emelyanov wrote:
> On 10/03/2014 05:30 PM, Tycho Andersen wrote:
> > On Fri, Oct 03, 2014 at 12:47:39PM +0400, Pavel Emelyanov wrote:
> >> On 10/03/2014 03:40 AM, Tycho Andersen wrote:
> >>> We can't write to cpuset's root cg (indeed, we didn't create it and it has
> >>> stuff in it), so ignore this special case.
> >>
> >> Wait a second. I though we should avoid writing the "special props"
> >> to all directories that we did *not* created on restore. Is this
> >> assumption correct?
> > 
> > Yes, that's what we should do (and you're right that this patch
> > doesn't guarantee that). However, that does mean that users could set
> > their cgroups up in a way that the restore can't succeed.
> 
> But isn't it the user's problem? Let's make CRIU's behavior simple and
> predictable -- if the directory exists, CRIU assumes that it's already
> set up properly and just doesn't touch one. Does this make sense you?

Yes, agreed.

> > Another issue is that if the user migrates to a node with fewer CPUs,
> > the restore will fail. e.g. if they have a cpuset.cpus == 0-3, and
> > they migrate to a machine with only two cores (cpuset.cpus == 0-1), we
> > will try to write 0-3 in and fail.
> 
> I agree.
>
> BTW, won't we hit the similar issue with e.g. memory? If we migrate from
> large-RAM box with big memory limit to small-RAM box and set this big mem
> limit so that it effectively covers all the new box's RAM? I suppose these
> two are the problems of different level and we shouldn't teach CRIU to
> solve them in the fully automatic way.

Yes, I agree. We can leave it up to higher level tools that know about
these things. What is a good interface to expose here? It seems like a
cli for rewriting would be painful, maybe it is better to expose the
.proto for cgroup and have the higher level tools just rewrite the
images before restore. Another option would be to have something like
--override-hardware-related-cgroups which just ignores the properties
for these special cgroups.

Tycho

> > I will think about a better way to do this patch. In the meantime, the
> > first hunk of this diff needs to be applied since it just fixes a
> > typo. I will resend that.
> 
> Thanks! :)
> 
> > Tycho
> > 
> >>> Signed-off-by: Tycho Andersen <tycho.andersen at canonical.com>
> >>> ---
> >>>  cgroup.c | 13 ++++++++++++-
> >>>  1 file changed, 12 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/cgroup.c b/cgroup.c
> >>> index 29e373d..36e3487 100644
> >>> --- a/cgroup.c
> >>> +++ b/cgroup.c
> >>> @@ -886,7 +886,7 @@ static int find_value(const char *controller, const char *path, const char *prop
> >>>  			*pos = 0;
> >>>  			snprintf(prefix, PATH_MAX, "%s/%s", controller, my_path);
> >>>  		} else {
> >>> -			snprintf(fpath, PATH_MAX, "%s", controller);
> >>> +			snprintf(prefix, PATH_MAX, "%s", controller);
> >>>  		}
> >>>  		snprintf(fpath, PATH_MAX, "%s/%s", prefix, prop);
> >>>  
> >>> @@ -964,6 +964,17 @@ static int copy_special_cg_props(const char *controller, char *path)
> >>>  			if (find_value(controller, path, prop, buf, prefix, missing_path) < 0)
> >>>  				return -1;
> >>>  
> >>> +			/*
> >>> +			 * If the path that was missing is "/", we should skip
> >>> +			 * copying this property, since cpuset won't allow us
> >>> +			 * to write to it because there are already processes
> >>> +			 * there (e.g. init). The root cg had no restrictions
> >>> +			 * on cpuset resources here, so ignoring this and
> >>> +			 * writing nothing is safe.
> >>> +			 */
> >>> +			if (strcmp(missing_path, "/") == 0)
> >>> +				continue;
> >>> +
> >>>  			if (copy_recursive(prefix, missing_path, prop, buf) < 0) {
> >>>  				pr_err("copying prop %s failed\n", prop);
> >>>  				return -1;
> >>>
> >>
> > .
> > 
>