-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Conversation
Please add this PR description to the commit itself. |
There was a problem hiding this 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.
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. |
Yes, too many things need to consider.
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. |
f5ad1bd
to
de88a01
Compare
There was a problem hiding this 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
.
8dc27ee
to
e04ae8c
Compare
There was a problem hiding this 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?
e04ae8c
to
708d002
Compare
708d002
to
fd2e81a
Compare
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. |
fd2e81a
to
f70dc84
Compare
@opencontainers/runc-maintainers Can we merge this, then we can continue work on moving some c code to go. |
Introduce a common parent struct `containerProcess`, let both `initProcess` and `setnsProcess` are inherited from it. Signed-off-by: lfbzhm <lifubang@acmcoder.com>
f70dc84
to
171c414
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rebased to re-run CI on latest git HEAD.
Still LGTM
Because the
initProcess
andsetnsProcess
have many same fields, and they are all in a big fileprocess_linux.go
, so let's do some refactor to this file:Introduce a common parent struct
containerProcess
, let bothinitProcess
andsetnsProcess
are inherited from it;MoveinitProcess
andsetnsProcess
definition and implementation out ofprocess_linux.go
, let them in their own seperate files:init_process_linux.go
andsetns_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
andsetnsProcess
can be treated ascontainerProcess
, we can only need to write some logic forcontainerProcess
, and then could be used for bothinitProcess
andsetnsProcess
.