Fix container logs -n line truncation and four performance defects - #2023
Fix container logs -n line truncation and four performance defects#2023gnavadev wants to merge 3 commits into
container logs -n line truncation and four performance defects#2023Conversation
| return [] | ||
| } | ||
|
|
||
| let lines = text.components(separatedBy: .newlines).filter { !$0.isEmpty } |
There was a problem hiding this comment.
Is dropping empty lines intentional? It changes the original log output and differs from the behavior of the standard tail command, which preserves blank lines.
There was a problem hiding this comment.
It's part of the original behavior. That's the same components(separatedBy: .newlines) + filter { !$0.isEmpty } this PR deletes from ContainerLogs.swift and MachineLogs.swift
That said, you're right that it's wrong. I'll make sure to change it.
There was a problem hiding this comment.
Checked the docs first. -n is documented only as "Number of lines to show from the end of the logs" and -f as "Follow log output", neither mentions filtering, and there was no comment on the line. The project does document this kind of filtering when it's intended, e.g. --env-file is described as "ignores # comments and blank lines" in four places. So I've treated it as a bug.
Blank lines are now preserved on both paths, matching tail and matching what container logs without -n already did.
There was a problem hiding this comment.
Checked the docs first. -n is documented only as "Number of lines to show from the end of the logs" and -f as "Follow log output", neither mentions filtering, and there was no comment on the line. The project does document this kind of filtering when it's intended, e.g. --env-file is described as "ignores # comments and blank lines" in four places. So I've treated it as a bug.
Blank lines are now preserved on both paths, matching tail and matching what container logs without -n already did.
@gnavadev Understood
Type of Change
Motivation and Context
Fixes #2022.
Five defects, all pre-existing on
main. No documented flag, output format, or APIshape changes.
1.
container logs -ntruncated the first line, and was O(n²).The tail loop read backwards in 1 KiB chunks, prepending each to a
Databuffer andre-decoding and re-splitting the entire accumulated buffer every iteration. It also
stopped as soon as it held
nlines, so the fragment left at a chunk boundary wasprinted as though complete.
It now counts line starts only in the bytes just read, joins once at the end, and reads
until it holds more than
nlines so the fragment is always discarded.ContainerLogs.swiftandMachineLogs.swiftcontained byte-for-byte identical 82-linecopies of this, so both carried the same bug. They are now one
LogTailerhelper in theshared
ContainerCommandsmodule.2.
Globbercompiled every pattern twice per call: once viatry Regex(...)purelyto validate and discard, and again inside
range(of:options:.regularExpression), forevery entry visited during a build-context walk. Patterns are now cached by glob
component. Validation still runs through
Regexon the cache-miss path so invalid globskeep throwing, and matching uses a cached
NSRegularExpression, the same enginerange(of:options:)already used underneath.3.
container system dfblocked unrelated operations. BothContainersService.calculateDiskUsage()andVolumesService.calculateDiskUsage()held asingle global
AsyncLockacross a synchronous recursive filesystem walk. Both nowsnapshot state under the lock, release it, and size bundles in a
nonisolatedasynchelper, which keeps the walk off the actor's executor where it already ran.
4.
BuildFSSyncusedSequence.contains(where:)against aSetinstead of its O(1)contains(_:), once per entry per directory during tar creation. The store is now adictionary keyed by relative path, which also removes a hand-written
Hashableconformance that hashed on only one of the struct's three fields, plus an unused field.
5.
container statssampled serially. Both samples now fan out through a task groupkeyed by container ID. Failure handling is unchanged: a failed first sample drops the
container, a failed second keeps the first.
Testing
Truncation fix verified end to end. Same container, same log file:
Added
Tests/ContainerCommandsTests/LogTailerTests.swiftcovering blank lines, a missingtrailing newline, lines longer than the 1 KiB read chunk, and multi-byte UTF-8 straddling
a chunk boundary. Extracting
lastLines(fh:n:)fromtail(...)is what made thistestable; the previous code only printed.
Full unit suite passes (576 tests, 71 suites). Item 2 is covered by the existing
GlobberTests, including the invalid-pattern cases asserting malformed globs stillthrow. Items 3 to 5 have no unit coverage and were verified by manual exercise of
container system df,container stats, andcontainer build.No documentation update:
docs/command-reference.mddocuments-nas "number of linesto show from the end of the logs", which is what the fixed code now does faithfully.
Follow-up from review: blank lines are now preserved on both the
-nand-fpaths. Fixing the follow path also required buffering partial lines across reads,
which fixes fragments being printed as complete lines and reads being dropped when
a chunk splits a multi-byte character.