Adjustments to container logging to address concerns from #262 #264
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
The `WRITEV_BUFFER_N_IOV` constant is not part of the `ctr_logging` interface, and is only used internal to `src/ctr_logging.c`.
Address a number of conditions flagged in issue containers#262. The general thrust of this commit is to move closer to a one-to-one, `read(2)` system call to `writev(2)` system call ratio by creating a memory buffer for reading sized to match the stdio pipes. The commit also allocates a maximum number of I/O vectors sized to the read buffer size, calculating 2 I/O vectors per line, defaulting to a target line size of 25 bytes (defined as the constant, `AVG_LOG_LINE_TARGET`). The commit also now allocates the I/O vectors and read buffer memory using `mmap()` so that we don't cross page boundaries for I/O operations. As a result, the commit removes the `STDIO_BUF_SIZE` constance, using `fcntl(F_GETPIPE_SZ)` calls to size all buffers for reading from pipes. The `stdpipe_t` value is now passed to `write_to_remote_consoles()` so that we do not have to play games with pre-/post- byte allocations to buffers.
We don't bother allocating I/O vectors for the journald case, and we make it much more efficient by using a pre-allocated stack variable using `memcpy` only, instead of `strdup()`.
|
I opened cri-o/cri-o#5013 to test these changes with the kubernetes e2e and cri-o |
|
Seems to be some packaging issues: Also fmt test is not happy. |
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.
There are two commits in this PR:
Move
WRITEV_BUFFER_N_IOVtosrc/ctr_logging.cThe
WRITEV_BUFFER_N_IOVconstant is not part of thectr_logginginterface, and is only used internal tosrc/ctr_logging.c.Match
read(2)buffer size to pipe size.Address a number of conditions flagged in issue #262.
The general thrust of this commit is to move closer to a one-to-one,
read(2)system call towritev(2)system call ratio by creating a memory buffer for reading sized to match the stdio pipes.The commit also allocates a maximum number of I/O vectors sized to the read buffer size, calculating 2 I/O vectors per line, defaulting to a target line size of 25 bytes (defined as the constant,
AVG_LOG_LINE_TARGET).The commit also now allocates the I/O vectors and read buffer memory using
mmap()so that we don't cross page boundaries for I/O operations.As a result, the commit removes the
STDIO_BUF_SIZEconstance, usingfcntl(F_GETPIPE_SZ)calls to size all buffers for reading from pipes.The
stdpipe_tvalue is now passed towrite_to_remote_consoles()so that we do not have to play games with pre-/post- byte allocations to buffers.