Fix a few problems in 'podman logs --tail' with journald driver #12067
Conversation
|
@hshiina thanks for the fixes! But looks like the podman logs tests are unhappy. |
hshiina
force-pushed
the
logs-journal-tail
branch
from
fc16671 to
173b969
Compare
test/e2e/logs_test.go
Outdated
| for _, log := range []string{"k8s-file", "journald", "json-file"} { | ||
|
|
There was a problem hiding this comment.
That is a good catch but the correct fix would be:
for _, log := range []string{"k8s-file", "journald", "json-file"} {
log := log
}This allows us to have one loop and have the log driver in the test name.
Having this loop inside the It() blocks would make it very hard to debug.
There was a problem hiding this comment.
Thank you for the advice!
| @@ -121,7 +121,7 @@ func (c *Container) readFromJournal(ctx context.Context, options *logs.LogOption | |||
| }() | |||
|
|
|||
| tailQueue := []*logs.LogLine{} // needed for options.Tail | |||
| doTail := options.Tail > 0 | |||
| doTail := options.Tail >= 0 | |||
There was a problem hiding this comment.
I am sure this should stay Tail > 0. If there are zero lines given we do not have to tail at all.
There was a problem hiding this comment.
The existing integration test expects '--tail 0' to output nothing and the other log drivers do so. I would like to make the journald driver consistent with the others now though this behavior may not be useful.
There was a problem hiding this comment.
OK, you are right. I thought the default value was 0 but it is -1.
hshiina
force-pushed
the
logs-journal-tail
branch
2 times, most recently
from
df8c1f9 to
53adcd2
Compare
|
Changes LGTM. I'm on the fence about the |
hshiina
force-pushed
the
logs-journal-tail
branch
from
53adcd2 to
2e73aaf
Compare
| @@ -38,8 +51,16 @@ var _ = Describe("Podman logs", func() { | |||
| }) | |||
|
|
|||
| for _, log := range []string{"k8s-file", "journald", "json-file"} { | |||
| log := log | |||
There was a problem hiding this comment.
Can you add a comment here explaining that this is important to move log var to the correct scope.
The following problems regarding `logs --tail` with the journald log driver are fixed: - One more line than a specified value is displayed. - '--tail 0' displays all lines while the other log drivers displays nothing. - Partial lines are not considered. - If the journald events backend is used and a container has exited, nothing is displayed. Integration tests that should have detected the bugs are also fixed. The tests are executed with json-file log driver three times without this fix. Signed-off-by: Hironori Shiina <[email protected]>
hshiina
force-pushed
the
logs-journal-tail
branch
from
2e73aaf to
c723e6b
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hshiina, Luap99 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
@containers/podman-maintainers PTAL |
|
/lgtm |
openshift-ci
bot
added
the
do-not-merge/hold
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
github-actions
bot
added
the
locked - please file new issue/PR
Signed-off-by: Hironori Shiina [email protected]
What this PR does / why we need it:
The following problems regarding
logs --tailwith the journald log driver are fixed:--tail 0displays all lines while the other log drivers displays nothing.Integration tests that should have detected the bugs are also fixed. The tests are executed with json-file log driver three times without this fix.
How to verify it
Run the fixed integration tests in
test/e2e/logs_test.go.Which issue(s) this PR fixes:
None
Special notes for your reviewer: