Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: move some c code to go #4309

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

lifubang
Copy link
Member

@lifubang lifubang commented Jun 5, 2024

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-1 c code and some of the stage-2 c code to go code, because they are not related to namespaces, they should 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.

@lifubang lifubang force-pushed the refactor-c-to-go branch 4 times, most recently from bbab957 to c534efe Compare June 5, 2024 11:19
@@ -76,6 +79,10 @@ func newProcessComm() (*processComm, error) {
if err != nil {
return nil, fmt.Errorf("unable to create init pipe: %w", err)
}
comm.stage1SockParent, comm.stage1SockChild, err = utils.NewSockPair("stage1")
if err != nil {
return nil, fmt.Errorf("unable to create init pipe: %w", err)
Copy link

@YanzhaoLi YanzhaoLi Jul 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although it's stage-1, do we need another name instead of init pipe for this log?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, thanks.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed! Thanks. Welcome more reviews!

@AkihiroSuda
Copy link
Member

Needs rebase

@AkihiroSuda AkihiroSuda added the kind/refactor refactoring label Sep 4, 2024
@AkihiroSuda
Copy link
Member

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

@lifubang lifubang added this to the 1.3.0 milestone Sep 5, 2024
@@ -27,6 +27,11 @@ type IDMap struct {
Size int64 `json:"size"`
}

// ToString is to serize the IDMap to a string.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A typo?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, fixed.
Welcome more reviews!

Introduce a common parent struct `containerProcess`,
let both `initProcess` and `setnsProcess` are inherited
from it.

Signed-off-by: lfbzhm <lifubang@acmcoder.com>
Move all the stage-1 c code and some of the stage-2
c code to go code, because they are not related to
namespaces, they should be implemented by golang.

Signed-off-by: lifubang <lifubang@acmcoder.com>
@lifubang
Copy link
Member Author

Needs rebase

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

I'll do rebase again once #4312 merged.

@rata
Copy link
Member

rata commented Sep 30, 2024

@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
Copy link
Member Author

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
Copy link
Member

rata commented Sep 30, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/refactor refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants