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

Conmon none log driver #172

Merged
merged 2 commits into from
Jun 10, 2020
Merged

Conversation

goochjj
Copy link
Contributor

@goochjj goochjj commented Jun 10, 2020

Implement "none" log-driver

Supports off, none, or null.

@goochjj
Copy link
Contributor Author

goochjj commented Jun 10, 2020

@haircommander I was hoping to figure out how to manually run conmon so I could verify the none driver actually never logs anything in the test runner. I can verify when I run it manually no file is ever created, while if I pass in something like "this", it does create an empty file.

@haircommander
Copy link
Collaborator

haircommander commented Jun 10, 2020

thanks for the PR @goochjj ! I feel like I'd prefer to allow the user to not specify --log-driver, instead of adding no-op log drivers. WDYT

@haircommander
Copy link
Collaborator

I have just reminded myself that none is a valid log driver in dockerland...

@haircommander
Copy link
Collaborator

@goochjj can you verify that running
conmon --log-driver journald: --log-driver none
doesn't leave use_journald_logging=TRUE?

@goochjj
Copy link
Contributor Author

goochjj commented Jun 10, 2020

@haircommander I bet it will - I wasn't sure how to handle that case, I noticed it was in a loop. But, what happens if
conmon --log-driver k8s-file:file1 --log-driver k8s-file:file2

I imagine file 2 will win?

@goochjj
Copy link
Contributor Author

goochjj commented Jun 10, 2020

It seems like you made --log-driver k8s-file and --log-driver journald coexist, but that's the only case when multiple log-driver args makes sense.

@haircommander
Copy link
Collaborator

haircommander commented Jun 10, 2020

yeah 2 wins. luckily, conmon is only meant to be called by tools that users will call, so one would hope these funky situations don't arise...

Right now, conmon logging seems to let users shoot themselves in the foot (in the k8s-file example you've described), so I suppose it's idiomatic to allow none with other drivers, thus making none a no-op. I am ok with that

I would say it could be a follow up to make error handling with logging more sane

@haircommander
Copy link
Collaborator

LGTM

@goochjj
Copy link
Contributor Author

goochjj commented Jun 10, 2020

I just tested with docker, and did --log-driver json --log-driver journald and got journald.

I think what's probably going to end up happening if you implement more of these, depending on how specialized syslog gets or fluentd or elasticsearch or whatever else you add, clearly you're not going to implement booleans for every single one, so when that abstraction happens you'll make decisions like do we really want 1 log driver, or is many acceptable, and if it is acceptable, maybe k8s writing to 2 files is... or writing to journald and k8s file(s)... I'd imagine 1 journald would be enough, unless the journald driver is extended to support namespaces in the future.

TLDR, I assume sometime soon there's going to be a refactor on that logic anyway.

@haircommander
Copy link
Collaborator

what's a little more tech debt 😆 thanks for checking, one more +1 and I'll merge 😃

@rhatdan
Copy link
Member

rhatdan commented Jun 10, 2020

LGTM

@rhatdan rhatdan merged commit 2c32b99 into containers:master Jun 10, 2020
@goochjj goochjj deleted the conmon_none_log_driver branch June 10, 2020 20:30
@mheon
Copy link
Member

mheon commented Jun 10, 2020

@haircommander @rhatdan Trouble you folks for a fresh Conmon release so I can use this in Podman CI?

@haircommander
Copy link
Collaborator

on it

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