runc
runc copied to clipboard
opencontainers
refactor: move some c code to go
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.
Maybe this should be postponed to v1.3, to avoid having regression in v1.2?
Needs rebase
Done. Welcome more reviews before we can merge. @opencontainers/runc-maintainers
I'll do rebase again once #4312 merged.
@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?
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.
@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 :)
@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.
I don't want to rebase the branch, because I want to test it in ubuntu-20.04(It has been removed in #4634).
@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).
I moved this to the 1.3.0-rc.1 milestone. Let's aim to have it by then :crossed_fingers: (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 :)
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.
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?
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.
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.
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 :)