init simplification #3385
Merged
init simplification #3385
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kolyshkin
force-pushed
the
init-logger-setup
branch
from
a516fb4 to
5d14cec
Compare
kolyshkin
force-pushed
the
init-logger-setup
branch
from
5d14cec to
171a6c0
Compare
|
Rebased on top of recently rebased #3375 |
kolyshkin
force-pushed
the
init-logger-setup
branch
from
171a6c0 to
3c4e1f6
Compare
|
Rebased; no longer a draft! Also, this concludes what was started as #3316 |
kolyshkin
commented
Mar 27, 2022
|
@opencontainers/runc-maintainers PTAL |
kolyshkin
force-pushed
the
init-logger-setup
branch
from
3c4e1f6 to
8e195de
Compare
|
Rebased; @opencontainers/runc-maintainers PTAL (this concludes a very long and heavy refactoring, the new code is easier to follow and understand). |
kolyshkin
force-pushed
the
init-logger-setup
branch
from
8e195de to
6b98a38
Compare
kolyshkin
force-pushed
the
init-logger-setup
branch
2 times, most recently
from
66de915 to
91d4bb0
Compare
|
The first commit still has this paragraph in the commit message, which no longer appears to be the case;
|
thaJeztah
reviewed
Jul 2, 2022
@thaJeztah why do you think so? The first commit removes the logrus setup from TestMain, and |
kolyshkin
force-pushed
the
init-logger-setup
branch
from
91d4bb0 to
c739db2
Compare
kolyshkin
force-pushed
the
init-logger-setup
branch
from
c739db2 to
e377e20
Compare
kolyshkin
force-pushed
the
init-logger-setup
branch
from
e377e20 to
1143f1f
Compare
kolyshkin
force-pushed
the
init-logger-setup
branch
from
3ae62ab to
bd0c83c
Compare
kolyshkin
force-pushed
the
init-logger-setup
branch
2 times, most recently
from
40e27c4 to
5aae844
Compare
kolyshkin
force-pushed
the
init-logger-setup
branch
from
5aae844 to
f0ce911
Compare
kolyshkin
commented
May 22, 2023
libcontainer/init_linux.go
Outdated
| @@ -149,7 +161,7 @@ func StartInitialization() (retErr error) { | |||
| }() | |||
|
|
|||
| // If init succeeds, it will not return, hence none of the defers will be called. | |||
| return containerInit(it, pipe, consoleSocket, fifofd, logPipeFd, mountFds) | |||
| return containerInit(it, pipe, consoleSocket, fifofd, logFD, mountFds) | |||
There was a problem hiding this comment.
OK, we now use 3 different ways to say "FD", but I'm pretty sure the last one is the best one.
There was a problem hiding this comment.
I personally prefer Fd. I can write up a PR to unify this.
kolyshkin
force-pushed
the
init-logger-setup
branch
from
f0ce911 to
159d2d9
Compare
|
@thaJeztah @AkihiroSuda PTAL |
AkihiroSuda
approved these changes
Jun 6, 2023
kolyshkin
force-pushed
the
init-logger-setup
branch
from
159d2d9 to
612c251
Compare
kolyshkin
force-pushed
the
init-logger-setup
branch
from
612c251 to
87249f6
Compare
|
Needs rebase |
Currently, logrus is used from the Go part of runc init, mostly for a few debug messages (see setns_init_linux.go and standard_init_linux.go), and a single warning (see rootfs_linux.go). This means logrus is part of init implementation, and thus, its setup belongs to StartInitialization(). Move the code there. As a nice side effect, now we don't have to convert _LIBCONTAINER_LOGPIPE twice. Note that since this initialization is now also called from libct/int tests, which do not set _LIBCONTAINER_LOGLEVEL, let's make _LIBCONTAINER_LOGLEVEL optional. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This commit does two things: 1. Consolidate StartInitialization calling logic into Init(). 2. Fix init error handling logic. The main issues at hand are: - the "unable to convert _LIBCONTAINER_INITPIPE" error from StartInitialization is never shown; - errors from WriteSync and WriteJSON are never shown; - the StartInit calling code is triplicated; - using panic is questionable. Generally, our goals are: - if there's any error, do our best to show it; - but only show each error once; - simplify the code, unify init implementations. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
kolyshkin
force-pushed
the
init-logger-setup
branch
from
87249f6 to
883aef7
Compare
|
Rebased. @lifubang @thaJeztah PTAL |
lifubang
approved these changes
Aug 5, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Based on and currently includes #3375. Draft until that one is merged.Based on and currently includes #3854. Draft until that one is merged.This moves logging setup out of
func init()and intoStartInitialization()(where it should belong);This concludes the spring cleaning started in #3354 (promise). Loosely based on parts of #3316.