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

logging: Respect log-size-max immediately after open #468

Merged
merged 1 commit into from
Dec 15, 2023

Conversation

martinetd
Copy link
Contributor

When a container keeps restarting, conmon restarts and opens the log file with O_APPEND every single time, resetting bytes_written to 0 with a non-empty file.
If the container fails before writting log-size-max bytes then the log keeps growing indefinitely, potentially using up all the available space.

This should be kept in check: initialize bytes_written to the file's current size, so that it can be reset if required (it's possible that the file gets re-open before the first write if it was already above the limit, but that should almost never happen in practice)

In order to make this simpler make bytes written global variables


This bit us with podman recently with a bad container config and restart=on-failure; I thought it wasn't a problem before? but checking the code I don't see how this would be a new problem, so I probably failed to check this when adding the podman --log-opt max-size=xxx parameter...

Thanks!

@martinetd
Copy link
Contributor Author

The test that failed (not ok 233 accept signed image with restrictive policy) doesn't seem to have anything to do with logging:

# E1204 06:20:31.343653  129166 remote_image.go:180] "PullImage from image service failed" err="rpc error: code = Unknown desc = SignatureValidationFailed: Source image rejected: A signature was required, but no signature exists" image="quay.io/crio/signed"

I'll assume that's unrelated and ignore it unless told otherwise.

src/ctr_logging.c Outdated Show resolved Hide resolved
@haircommander
Copy link
Collaborator

couple of nits, feel free to ignore the test failures :)

@haircommander
Copy link
Collaborator

LGTM, @giuseppe PTAL

Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

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

LGTM

@giuseppe
Copy link
Member

giuseppe commented Dec 14, 2023

could you please rebase so the CI will (hopefully) pass?

When a container keeps restarting, conmon restarts and opens the log
file with O_APPEND every single time, resetting bytes_written to 0
with a non-empty file.
If the container fails before writting log-size-max bytes then the
log keeps growing indefinitely, potentially using up all the available
space.

This should be kept in check: initialize bytes_written to the file's
current size, so that it can be reset if required (it's possible that
the file gets re-open before the first write if it was already above the
limit, but that should almost never happen in practice)

In order to make this simpler make bytes written global variables

Signed-off-by: Dominique Martinet <[email protected]>
@rhatdan
Copy link
Member

rhatdan commented Dec 14, 2023

/lgtm
/approve

@martinetd
Copy link
Contributor Author

martinetd commented Dec 14, 2023

could you please rebase so the CI will (hopefully) pass?

It looks like that did the trick.

I don't suppose there are plans for a 2.1.10 a couple of days after the 2.1.9 is there? :)
(EDIT: looks like there will be one for the UAF fix, would be great if these two get in together)

@giuseppe giuseppe merged commit 5bd4abb into containers:main Dec 15, 2023
16 checks passed
@haircommander
Copy link
Collaborator

this is included in 2.1.10 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants