(*initProcess).start: rm second Apply

[kolyshkin]

Commit df3fa11 adds a second call to cgroupManager.Apply() method in
order to add a second pid to the cgroup.

While I admit I don't understand the reason why it was added
(even after reading #1916, #1184, and #1884), it seems that
going through all the steps (configure cgroup parameters, figure
out paths to all controllers, create a systemd unit) twice is excessive
and should not be done.

More to say, even merely adding the child pid to the same cgroup seems
redundant, as we added the parent pid to the cgroup before sending the
data to the child (runc init process), and it waits for the data before
doing clone(), so its children will be in the same cgroup anyway.

Needs a very careful review.

[kolyshkin]

The thing I do not understand is why Apply() is called twice here:

runc/libcontainer/process_linux.go

Lines 322 to 358 in dbe5aca

	// Do this before syncing with child so that no children can escape the
	// cgroup. We don't need to worry about not doing this and not being root
	// because we'd be using the rootless cgroup manager in that case.
	if err := p.manager.Apply(p.pid()); err != nil {
	return newSystemErrorWithCause(err, "applying cgroup configuration for process")
	}
	if p.intelRdtManager != nil {
	if err := p.intelRdtManager.Apply(p.pid()); err != nil {
	return newSystemErrorWithCause(err, "applying Intel RDT configuration for process")
	}
	}
	if _, err := io.Copy(p.messageSockPair.parent, p.bootstrapData); err != nil {
	return newSystemErrorWithCause(err, "copying bootstrap data to pipe")
	}
	childPid, err := p.getChildPid()
	if err != nil {
	return newSystemErrorWithCause(err, "getting the final child's pid from pipe")
	}
	// Save the standard descriptor names before the container process
	// can potentially move them (e.g., via dup2()). If we don't do this now,
	// we won't know at checkpoint time which file descriptor to look up.
	fds, err := getPipeFds(childPid)
	if err != nil {
	return newSystemErrorWithCausef(err, "getting pipe fds for pid %d", childPid)
	}
	p.setExternalDescriptors(fds)
	// Do this before syncing with child so that no children
	// can escape the cgroup
	if err := p.manager.Apply(childPid); err != nil {
	return newSystemErrorWithCause(err, "applying cgroup configuration for process")
	}
	if p.intelRdtManager != nil {
	if err := p.intelRdtManager.Apply(childPid); err != nil {
	return newSystemErrorWithCause(err, "applying Intel RDT configuration for process")
	}
	}

Presumably it is to fix #1884, and I might buy into adding the second pid, but can't understand why we need to say create the systemd slice for the second time, set all limits for the second time etc.

[cyphar]

I noticed the double-Apply recently and I agree that it doesn't make any sense to me. In fact, I'm fairly sure that setting the PID isn't necessary either -- we Apply once before we even send the config data to the runc init process so it hasn't gotten to the point of all the forking garbage in nsexec.c (meaning that all the container processes are in the container). Note that p.pid() is the PID of the runc init process.

In fact, unless I'm misunderstanding the code (which is quite possible -- I didn't get much sleep), all of the CREATECGROUPNS synchronisation is not necessary because we implicitly already put the container process inside the target cgroup before any children were forked. Though I might be wrong about this part -- #1884 was an issue with the old setup (where we only did Apply(p.pid()) without the CREATECGROUPNS synchronisation).

[kolyshkin]

I'm fairly sure that setting the PID isn't necessary either -- we Apply once before we even send the config data to the runc init process so it hasn't gotten to the point of all the forking garbage in nsexec.c.

Looks like it is. Trying to remove the second Apply().

[kolyshkin]

I'm fairly sure that setting the PID isn't necessary either -- we Apply once before we even send the config data to the runc init process so it hasn't gotten to the point of all the forking garbage in nsexec.c.

Looks like it is.

Hmm, OTOH it was the same was at the time of commit df3fa11, and even at the time of commit 69663f0 (mentioned in #1884) so again I am not sure what am I missing here :(

BTW, #1884 (comment) says

When playing with runc code, it appears that the error disappears if we retry running
cgroups.EnterPid on failure (which might indicate this is a transient/race issue).

So, my current (and rather weak) theory is that the second Apply() was just a (wrong, but working) workaround for an issue that is actually fixed later by #1950.

[kolyshkin]

OK, I found out that #1184 was not adding the second Apply() but then #1916 (which is a carry of #1184) did. Again, I am not sure why, maybe it was an error during rebase (there was definitely an error of adding a second duplicate defer statement to that function, see discussion in #1916 (comment), fixed in #2238).

[kolyshkin]

I read the code through a few times and am pretty confident the second apply is useless.

@mrunalp @giuseppe @cyphar @AkihiroSuda PTAL

[mrunalp]

@AkihiroSuda @cyphar ptal.

[cyphar]

I agree that the second Allow isn't necessary. As I said, I'm also fairly sure that the CREATECGROUPNS synchronisation is also unnecessary -- but I'll submit a follow-up PR which removes that.