Add test suite for LogReader implementations
#43231
Closed
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
Whether or not the logger has been closed is a property of the logger, and only of concern to its log reading implementation, not log watchers. The loggers and their reader implementations can communicate as they see fit. A single channel per logger which is closed when the logger is closed is plenty sufficient to broadcast the state to log readers, with no extra bookeeping or synchronization required. Signed-off-by: Cory Snider <csnider@mirantis.com>
Add a comprehensive test suite for validating the behavior of any LogReader. Test the current LogFile-based implementations against it. Fix bugs which were unconvered by the test suite. The json-file driver appended a newline character to log messages with PLogMetaData.Last set, but the local driver did not. Alter the behavior of the local driver to match that of the json-file driver. The LogFile follower would stop immediately upon the producer closing. The close signal would race the file watcher; if a message were to be logged and the logger immediately closed, the follower could miss that last message if the close signal (formerly ProducerGone) was to win the race. Add logic to perform one more round of reading when the producer is closed to catch up on any final logs. The asynchronous startup of the log-reading goroutine made the follow-tail tests nondeterministic. The Log calls in the tests which were supposed to happen after the reader started reading would sometimes execute before the reader, throwing off the counts. Tweak the ReadLogs implementation so that the order of operations is deterministic. Signed-off-by: Cory Snider <csnider@mirantis.com>
The followLogs function handled closing the file, decoder and watcher values until after follow.mainLoop returned. However, it closed the initial values assigned before entering mainLoop. A few different situations may arise inside mainLoop which are handled by closing and replacing one or more of these values in the follow struct. If such a situation were to occur, the replaced values would dangle, potentially leaking file descriptors until (hopefully) the garbage collector invokes their finalizers. Signed-off-by: Cory Snider <csnider@mirantis.com>
The file descriptor is not used for anything, and as it is closed asynchronously when the watch is removed, only serves to cause issues deleting and renaming watched files on Windows. Signed-off-by: Cory Snider <csnider@mirantis.com>
|
The fixes included in this PR, while sufficient to get the tests to pass in CI, merely substitutes the known issues following logs on Windows with a different set of issues. Closing in favor of #43294. |
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.
- What I did
I wrote a comprehensive unit test suite for interface
logger.LogReaderwhich verifies that any implementation behaves like the json-file driver. I tested both the json-file and local drivers and fixed some bugs which the tests hit. Some of the fixes improve the situation for #39274 as the daemon no longer logs any logging-driver errors anddocker logs --followno longer exits early, but the container logs are still not followed across rotations on Windows Server 2022. The log writing is no longer broken by watching the logs, at least: interrupting the command and runningdocker logsagain prints the most recent logs.This PR will be followed up by a rewrite of the journald driver's LogReader implementation (#43219) which passes this test suite and fixes all the known bugs listed in #38859, including the "impossible to implement entirely correct" ones.
- Description for the changelog
locallog driver- A picture of a cute animal (not mandatory but encouraged)