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 init and setns process #4312

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lifubang
Copy link
Member

@lifubang lifubang commented Jun 6, 2024

Because the initProcess and setnsProcess have many same fields, and they are all in a big file process_linux.go, so let's do some refactor to this file:

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

  2. Move initProcess and setnsProcess definition and implementation out of process_linux.go, let them in their own seperate files: init_process_linux.go and setns_process_linux.go, it will make more clear when reading and changing the code.
    EDIT: the second refactor could be refactored after the release-1.1 is EOL.

This refactor doesn't change any logic, and it's the first step of #4309 . After this refactor, both initProcess and setnsProcess can be treated as containerProcess, we can only need to write some logic for containerProcess, and then could be used for both initProcess and setnsProcess.

@kolyshkin
Copy link
Contributor

Please add this PR description to the commit itself.

Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

I'd like to merge #4283 before this one because otherwise we can't do a revert.

Or maybe you can make a smaller refactor, without splitting process_linux into two files.

@kolyshkin
Copy link
Contributor

Or maybe you can make a smaller refactor, without splitting process_linux into two files.

In fact, that would be better because when a lot of functions are moved from a file to a file, it makes it much harder to dig in git history.

@lifubang
Copy link
Member Author

lifubang commented Jun 7, 2024

I'd like to merge #4283 before this one because otherwise we can't do a revert.

Yes, too many things need to consider.

Or maybe you can make a smaller refactor, without splitting process_linux into two files.

Yes, there is another problem, if we split this file, any changes about this file can't be backported to release-1.1 in the future.

@lifubang lifubang force-pushed the refactor-process branch 2 times, most recently from f5ad1bd to de88a01 Compare June 7, 2024 12:30
Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

So, I was hoping that unifying this can help to deduplicate some methods: pid, externalDescriptors, forwardChildLogs, terminate, startTime, signal etc., if those are to be made methods of containerProcess.

@lifubang lifubang force-pushed the refactor-process branch 3 times, most recently from 8dc27ee to e04ae8c Compare June 27, 2024 10:21
@lifubang lifubang requested a review from kolyshkin June 27, 2024 10:22
Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

This looks better!

Yet perhaps it makes sense to merge it after the 1.2.0 release, as we're already at rc2 now. WDYT?

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

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

Yet perhaps it makes sense to merge it after the 1.2.0 release, as we're already at rc2 now. WDYT?

I have no strong idea. I think this is a code refactor to let the code become more readable and reasonable, it doesn't change any logic.

@lifubang lifubang added the kind/refactor refactoring label Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants