refactor: move some c code to go #4309

lifubang · 2024-06-05T10:53:46Z

As mentioned in #3951 , we want to move c code to golang, there are many hard works to do.

This PR has done the first step, move all the stage-0 c code and some of the stage-2 c code to go code, because they are not related to namespaces, and could be implemented by golang.

This refactor brings one benifit, it reduces one process clone when start/run/create a container. But because the stage-1 c code is hard to move to go code, so it brings a Complexity for libct/nsenter, for example, it's hard to write unit tests for nsenter.

Welcome more suggestions.

libcontainer/nsenter/nsexec.c

libcontainer/process_linux.go

libcontainer/system/linux.go

libcontainer/utils/utils.go

libcontainer/container_setup.go

libcontainer/process_linux.go

AkihiroSuda · 2024-09-04T21:50:46Z

Needs rebase

AkihiroSuda · 2024-09-04T21:51:29Z

Maybe this should be postponed to v1.3, to avoid having regression in v1.2?

libcontainer/configs/config.go

lifubang · 2024-09-30T06:11:56Z

Needs rebase

Done.
Welcome more reviews before we can merge.
@opencontainers/runc-maintainers

I'll do rebase again once #4312 merged.

rata · 2024-09-30T10:09:57Z

@lifubang I like the idea of the PR, but I'd prefer to merge this just after the 1.2.0 final release. I'd like to focus on bug-fixing now, to do the final release.

This is non-trivial at all and has several semantic changes, there might be follow-up PRs to fix edge cases, and I really prefer to not put all of that on the hot-path to release 1.2.0. I think doing a 1.3.0 release in a few months after 1.2.0 is completely fine (indeed it was the original plan for 1.2, to be a few months after 1.1), if we have this PR (and probably we will have a few of the others that are around too), this makes sense to me as a new release.

What do you think?

lifubang · 2024-09-30T10:53:43Z

What do you think?

Agree.
We are indeed focusing on the bug fix of 1.2.0.
What I think is that this PR needs more careful review. If we have some free time, we can spend on it.

rata · 2024-09-30T11:02:36Z

I'll focus on this after the 1.2 release. Thanks for working on this! :)

rata · 2025-01-21T18:48:03Z

@lifubang are you aiming to merge this for 1.3? There seems to be a lot of conflicts. I haven't tried to review yet, let me know and I can try to review this. It seems like a complicated PR, though, so it will take some time :)

kolyshkin

Left a few more nits. In general this is a nice development which needs to be finished.

Note the first commit from this PR is already merged (as commit 171c414, PR #4312). This, I guess, is the reason for most of the conflicts.

@lifubang are you going to keep working on that?

libcontainer/container_setup.go

libcontainer/init_linux.go

lifubang · 2025-02-11T01:29:40Z

@lifubang are you going to keep working on that?

Yes, I'll work on it after my children's winter vacation ended in the next week.

lifubang · 2025-02-20T09:18:36Z

@rata @kolyshkin @cyphar @AkihiroSuda PTAL

lifubang · 2025-02-20T10:30:06Z

I don't want to rebase the branch, because I want to test it in ubuntu-20.04(It has been removed in #4634).

rata

@lifubang thanks for working on this! It's really a great and needed improvement.

I'll try to take a closer look later. Do you think it might be possible to split this in more commits? Like instead of moving all of stage-0 and most of stage-2, can we have like one commit moving the plumbing to go and some basic thing, another moving the write of userns mappings, etc.?

If this is not too complicated to do (it might be), I think it can help code review a lot. Specially to find bugs, as that is simpler when looking at smaller chunks that just need to do the same in go than in C.

rata · 2025-02-20T11:29:17Z

libcontainer/nsenter/nsexec.c

-				bail("failed to close sync_child_pipe[0] fd");
-
-			/* For debugging. */
-			prctl(PR_SET_NAME, (unsigned long)"runc:[2:INIT]", 0, 0, 0);


I'd keep this. It's way simpler the init stage now, but still things can go wrong (specially since we are doing big changes) and it is useful to have this IMHO.

👍 make sense.

rata · 2025-02-20T11:37:27Z

libcontainer/nsenter/nsexec.c

Just curious, is there any non async-signal-safe fuction remaining here, as defined in man signal-safety, after the fork/clone?

rata · 2025-02-20T11:42:54Z

@lifubang can you rebase and revert the removal of ubuntu here? We can just remove that commit before merging. But that way we will review what is going to be merged (not sure if the conflicts will make a big change in this PR or not).

rata · 2025-02-20T11:59:47Z

I moved this to the 1.3.0-rc.1 milestone. Let's aim to have it by then 🤞 (I think rc.1 didn't exist yet when this was added to 1.3.0), I guess we all agree but if anyone disagrees please let me know :)

rata · 2025-02-20T12:02:22Z

At some point it might be interesting to see if this has a perf implication too (we want it anyways, but it might be better on that regard too :)). @kolyshkin added some functions to test an exec. I don't remember if they will cover this well, but worth taking a look.

As we want tom move some code from c to go, we should implement them in golang first, for example: UpdateSetgroups, TryMappingTool, UpdateUidmap, UpdateGidmap, UpdateTimeNsOffsets, and UpdateOomScoreAdj. Signed-off-by: lifubang <[email protected]>

Signed-off-by: lifubang <[email protected]>

rata

@lifubang You didn't say it's ready for review again, but tests are green and you did split the commits. Although it seems commit 4 and 5 have the same title and you might be working on them?

I wrote some comments before realizing you might be working on it still. I'll leave them in case they are useful. Please do ask for review again once it is ready:)

rata · 2025-02-21T15:30:04Z

libcontainer/init_linux.go

+		return os.NewSyscallError("setgid", err)
+	}
+
+	if !config.Config.RootlessEUID && requiresRootOrMappingTool(config.Config.GIDMappings) {


Why is requiresRootOrMappingTool() equivalent to config.is_setgroup?

rata · 2025-02-21T15:33:29Z

libcontainer/process_linux.go

 	_ = c.syncSockChild.Close()
 	_ = c.logPipeChild.Close()
 }

 func (c *processComm) closeParent() {
 	_ = c.initSockParent.Close()
+	_ = c.stage1SockChild.Close()


typo here? Do we want to close the parent here, I guess?

rata · 2025-02-21T15:35:55Z

libcontainer/process_linux.go


-	// Ignore the error in case the child has already been reaped for any reason
-	_, _ = firstChildProcess.Wait()


Why is this removed?

rata · 2025-02-21T15:42:19Z

libcontainer/nsenter/nsexec.c

-/* XXX: This is ugly. */
-static int syncfd = -1;


This is an example that could be a different commit, IMHO. It can be of the last ones, if that helps to avoid breaking compilation/tests. But if you split this out in another commit and just mention that you move to function X now that YY happens and we don't need it here anymore.

If I want to review in detail, it's hard when logically/semantically unrelated changes are mixed together. Because I go and check if this is sound, but checking that can be several steps, and then I have to do backtracking and come back to this point and continue reviewing. It's way more likely I'll miss more stuff if I review this way.

On the other hand, if this was a different commit and you call out in the commit msg why is this possible now, then it will be very easy to review.

rata · 2025-02-21T15:44:17Z

libcontainer/container_setup_linux.go

+	var buf [bufSize]byte
+	native := nl.NativeEndian()
+	native.PutUint32(buf[:], uint32(msg))
+	if _, err := unix.Write(int(f.Fd()), buf[:]); err != nil {


I guess we need to handle when it returns EINTR now that this is go. This applies to all functions that can return EINTR and are using the unix or syscall package.

rata · 2025-02-21T15:45:22Z

libcontainer/nsenter/log.c

@@ -58,7 +58,7 @@ void write_log(int level, const char *format, ...)
 		if (stage == NULL)
 			goto out;
 	} else {
-		ret = asprintf(&stage, "nsexec-%d", current_stage);
+		ret = asprintf(&stage, "nsexec-%d", current_stage + 1);


Why is this?

rata · 2025-02-21T16:05:42Z

This is a big and major change, it will be great if more eyes can have a look. Maybe @cyphar @AkihiroSuda could you have a look?

cyphar · 2025-02-25T02:34:38Z

I've taken a look at this a few times and I'm not sure how I feel about it. Yes, it removes C code but I'm not really sure whether or not it's an improvement overall -- I'll need to read through it a few more times...

I had a few crazy ideas for removing all of the C code (such as putting the runc init into PTRACE_TRACEME mode and then setting up things before executing the Go init) which could theoretically remove a lot of the stuff in Go (once Go supports joining namespaces) but I suspect there will be bits we won't be able to move without doing CRIU-style parasitic code injection.

I'm going to move the milestone to 1.4.0 since it seems unlikely we'll be able to get it in before -rc1, and ultimately this is an internal cleanup rather than a key feature.

lifubang · 2025-02-25T10:21:42Z

I've taken a look at this a few times and I'm not sure how I feel about it. Yes, it removes C code but I'm not really sure whether or not it's an improvement overall -- I'll need to read through it a few more times...

Haha, it seems like that I saw your puzzled face. When I was refactoring these code, I usually wanted to say: "Though it lacks substance, it still brings joy".
Like my description in the PR:
"This PR has done the first step, move all the stage-0 c code and some of the stage-2 c code to go code, because they are not related to namespaces, and could be implemented by golang."

So what I think is to reduce the quantity of functions in nsexec.c, then it maybe help to do the next refactor steps.

I'm going to move the milestone to 1.4.0 since it seems unlikely we'll be able to get it in before -rc1, and ultimately this is an internal cleanup rather than a key feature.

I agree.

rata · 2025-02-25T12:13:37Z

For me, one very interesting thing is to remove the async-safe-safe issues we currently have.

@cyphar just curious, what makes you doubt? Is it more like a gut feeling and not yet clear? I'm of course fine moving to 1.4, not asking due to that, but out of curiosity :)

lifubang force-pushed the refactor-c-to-go branch 4 times, most recently from bbab957 to c534efe Compare June 5, 2024 11:19