Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix --tail log on restart problem #13857

Merged
merged 1 commit into from
Apr 14, 2022
Merged

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented Apr 13, 2022

--tail=1 is not working f you restart a container with journald logging.

We see the exit status and then call into the logging a second time
causing all of the logs to print.

Removing the tail log on exited seems to fix the problem.

Fixes: #13098

Signed-off-by: Daniel J Walsh [email protected]

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 13, 2022
@nalind
Copy link
Member

nalind commented Apr 13, 2022

If it passes tests and doesn't bring back #12263, it's fine.

Copy link
Member

@edsantiago edsantiago left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly LGTM, with a few nits, and one big question: should the new test conform to the existing convention of testing under both k8s-file and journald?

Actually, two questions: regardless of the answer to the above, I think skip_if_journald_unavailable might be needed to avoid the usual mad panic when this makes it into RHEL-land.

@@ -30,6 +30,23 @@ load helpers
run_podman rm $cid
}

@test "podman logs - tail test" {
rand_string=$(random_string 40)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unused

run_podman restart $cid

run_podman logs --tail 1 $cid
is "$output" "test2" "logs should only show last line"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you rename this message? Maybe "logs, after restart, should still only show last line"? Distinct error messages make it sooooo much easier to track down the exact failure.

@edsantiago
Copy link
Member

New push addresses two of my nits. Can you, for posterity, at least comment on my other questions? I don't know where we stand these days on rootless RHEL journald.

@rhatdan
Copy link
Member Author

rhatdan commented Apr 13, 2022

I think we should test with both, if possible.

@edsantiago
Copy link
Member

I'm interpreting that as a request for code:

function _log_test_tail() {
    local driver=$1

    run_podman run -d --log-driver=$driver $IMAGE sh -c "echo test1; echo test2"
    cid="$output"

    run_podman logs --tail 1 $cid
    is "$output" "test2"  "logs should only show last line"

    run_podman restart $cid

    run_podman logs --tail 1 $cid
    is "$output" "test2"  "logs should only show last line after restart"

    run_podman rm $cid
}

@test "podman logs - tail test, k8s-file" {
    _log_test_tail k8s-file
}

@test "podman logs - tail test, journald" {
    # We can't use journald on RHEL as rootless: rhbz#1895105
    skip_if_journald_unavailable

    _log_test_tail journald
}

--tail=1 is not working f you restart a container with journald logging.

We see the exit status and then call into the logging a second time
causing all of the logs to print.

Removing the tail log on exited seems to fix the problem.

Fixes: containers#13098

Signed-off-by: Daniel J Walsh <[email protected]>
@rhatdan
Copy link
Member Author

rhatdan commented Apr 13, 2022

@edsantiago Added your code.

@edsantiago
Copy link
Member

LGTM assuming tests pass. FYI I ran new tests against main, and the k8s-file one passes root & rootless, journald fails root & rootless, which means both test and code are doing the right thing.

/lgtm
/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 13, 2022
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 13, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 13, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: edsantiago, rhatdan

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@rhatdan
Copy link
Member Author

rhatdan commented Apr 14, 2022

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 14, 2022
@openshift-merge-robot openshift-merge-robot merged commit 5771f82 into containers:main Apr 14, 2022
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 21, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[macOS] podman logs --tail # value not interpreted correctly when used against restarted containers
4 participants