Conversation
a516fb4 to
5d14cec
Compare
5d14cec to
171a6c0
Compare
|
Rebased on top of recently rebased #3375 |
171a6c0 to
3c4e1f6
Compare
|
Rebased; no longer a draft! Also, this concludes what was started as #3316 |
|
@opencontainers/runc-maintainers PTAL |
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). |
8e195de to
6b98a38
Compare
66de915 to
91d4bb0
Compare
|
The first commit still has this paragraph in the commit message, which no longer appears to be the case;
|
@thaJeztah why do you think so? The first commit removes the logrus setup from TestMain, and |
91d4bb0 to
c739db2
Compare
c739db2 to
e377e20
Compare
e377e20 to
1143f1f
Compare
|
Rebased, reworked to make review easier (please review commit-by-commit). |
|
Let me simplify this even further. |
|
OK, I wanted to remove logging from the Go part of In short, this is a series of small commits that untangle some logic around |
libcontainer/init_linux.go
Outdated
| if werr := writeSync(pipe, procError); werr != nil { | ||
| fmt.Fprintln(os.Stderr, err) | ||
| if err := writeSync(pipe, procError); err != nil { | ||
| fmt.Fprintln(os.Stderr, retErr) |
There was a problem hiding this comment.
It's interesting that;
- there's no check for
retErrto be nil, so even if there's noretErr, we do aprintln retErris only printed ifwriteSyncorutils.WriteJSONfails (is that expected?)- The errors from
writeSyncandutils.WriteJSONare neither returned, nor printed 🤔
There was a problem hiding this comment.
Oh! I guess that's depending on;
// If init succeeds, it will not return, hence none of the defers will be called.
return containerInit(it, pipe, consoleSocket, fifofd, logPipeFd, mountFds)| if err := libcontainer.StartInitialization(); err != nil { | ||
| // as the error is sent back to the parent there is no need to log | ||
| // or write it to stderr because the parent process will handle this | ||
| os.Exit(1) |
There was a problem hiding this comment.
Looks like this is the code that was added in the first commit; perhaps it would make sense to move this commit (and the previous one) to the start to the list of commits.
There was a problem hiding this comment.
So, the first commit ("libct/int: remove logger from init") is a preparation for "init.go: move logger setup to StartInitialization". Logically, they belong together, but since test changes in libct/int are somewhat larger (they also involve a test case), I separate them out for easier review.
Now, the change in the first commit (""libct/int: remove logger from init") brings libcontainer/integration func init() in line with the main runc's func init. Later, we change StartInitialization to return error in some cases, so we remove the os.Exit(1).
All I can do is to rearrange the commits so these two are following each other:
- libct/int: remove logger from init
- init.go: move logger setup to StartInitialization
There was a problem hiding this comment.
Now, the code added is correct (when it is added), but since "libct/StartInitialization: amend docs, do not error" it needs to be fixed. Thus the two changes. Hope that makes sense.
|
It's hard to split changes to separate small commits, as they are all inter-dependent. But I did another try; PTAL. |
libcontainer/init_linux.go
Outdated
|
|
||
| // 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.
|
@thaJeztah @AkihiroSuda PTAL |
|
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>
|
Rebased. @lifubang @thaJeztah PTAL |
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.