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

add log-global-size-max option to limit the total output #342

Merged
merged 3 commits into from
Jun 14, 2022

Conversation

haircommander
Copy link
Collaborator

@haircommander
Copy link
Collaborator Author

@rhatdan
Copy link
Member

rhatdan commented Jun 8, 2022

Does global add any value to the option. Would --logs-size-max work?

@haircommander
Copy link
Collaborator Author

Does global add any value to the option. Would --logs-size-max work?

we already have a log-size-max for the log rotation fields. the global is to differentiate the two

@rhatdan
Copy link
Member

rhatdan commented Jun 9, 2022

--logs-size-max
versus --log-size-max The s to differentiate the difference.

@haircommander
Copy link
Collaborator Author

ahh good idea. Unfortunately we've already shipped cri-o with the flag (and patches to conmon in openshift 🙈 ) so it would end up being tricky to change it now

@rhatdan
Copy link
Member

rhatdan commented Jun 9, 2022

Ok stick to your wacky option :^(, it is not really used by Humans anyways.

@haircommander haircommander merged commit 99eac3e into containers:main Jun 14, 2022
hswong3i added a commit to alvistack/containers-conmon that referenced this pull request Jun 16, 2022
containers#342 add support for
`log-global-size-max` for
cri-o/cri-o@a4080bb,
but incorrectly name the CLI option as `log-size-global-max`.

Signed-off-by: Wong Hoi Sing Edison <[email protected]>
hswong3i added a commit to alvistack/containers-conmon that referenced this pull request Jun 16, 2022
…e-max`

containers#342 add support for
`log-global-size-max` for
cri-o/cri-o@a4080bb,
but incorrectly name the CLI option as `log-size-global-max`.

Signed-off-by: Wong Hoi Sing Edison <[email protected]>
haircommander pushed a commit that referenced this pull request Jun 16, 2022
#342 add support for
`log-global-size-max` for
cri-o/cri-o@a4080bb,
but incorrectly name the CLI option as `log-size-global-max`.

Signed-off-by: Wong Hoi Sing Edison <[email protected]>
@giuseppe
Copy link
Member

giuseppe commented Jul 4, 2024

@haircommander would it be a breaking change for CRI-O if we change the behavior of --log-global-size-max to delete the oldest log file instead of stopping writing so that we could reuse it for containers/podman#23191?

@giuseppe
Copy link
Member

giuseppe commented Jul 4, 2024

probably it is still cleaner to add a new option to specify the maximum number of files to keep around

@haircommander
Copy link
Collaborator Author

I'm not actually sure. It's used to limit the amount of output on exec probe requests to mitigate a CVE, and we never really concretely promised any behavior on which output would be returned if the exec probe exceeded the max. This change would change the behavior so that we get the end of output instead of beginning. I think you could make an argument in either direction. ultimately, a user would hit this situation if their execs were using way too much output, so I am inclined to say not a breaking change? WDYT @saschagrunert @mrunalp @kwilczynski @sohankunkerkar

@giuseppe
Copy link
Member

giuseppe commented Jul 5, 2024

yeah I had a quick look at it and I think it will be confusing. It is better a different option to control logs rotation

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.

3 participants