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

Do not share container log driver for exec #6560

Merged
merged 1 commit into from
Jun 17, 2020

Conversation

mheon
Copy link
Member

@mheon mheon commented Jun 10, 2020

When the container uses journald logging, we don't want to automatically use the same driver for its exec sessions. If we do we will pollute the journal (particularly in the case of healthchecks) with large amounts of undesired logs. Instead, force exec sessions logs to file for now; we can add a log-driver
flag later (we'll probably want to add a podman logs command that reads exec session logs at the same time).

Fixes #6555

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mheon

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 10, 2020
@rhatdan
Copy link
Member

rhatdan commented Jun 10, 2020

Should we just drop the logging altogether rather then saving them to a log. Does Docker exec get logged?

@mheon
Copy link
Member Author

mheon commented Jun 10, 2020

Conmon currently requires a log driver. Adding a null log driver to Conmon would alleviate this, but would add a lot of complexity to this simple fix.

@goochjj
Copy link
Contributor

goochjj commented Jun 10, 2020

docker exec is never logged (fyi)

@rhatdan
Copy link
Member

rhatdan commented Jun 10, 2020

@haircommander How hard would it be to add the null driver to conmon? Not for this PR but for future ones.

@rhatdan
Copy link
Member

rhatdan commented Jun 10, 2020

@mheon I think you should tell conmon to log to /dev/null then on exec, rather then taking up disk space and having to deal with cleaning it up. Once conmon can handle no log driver or the null driver, we can remove it.

@mheon
Copy link
Member Author

mheon commented Jun 10, 2020

@rhatdan @haircommander My one concern there is breaking log rotation - will conmon try to rotate /dev/null?

@goochjj
Copy link
Contributor

goochjj commented Jun 10, 2020

@haircommander How hard would it be to add the null driver to conmon? Not for this PR but for future ones.

I've been all over the conmon source recently - I think it'd be pretty trivial. i.e.

static gboolean use_journald_logging = FALSE;
static gboolean use_k8s_logging = FALSE;

Keeping both of those false, instead of always setting one.

@haircommander
Copy link
Collaborator

it should be a simple fix

@goochjj
Copy link
Contributor

goochjj commented Jun 10, 2020

@goochjj
Copy link
Contributor

goochjj commented Jun 10, 2020

All said - this patch does work - it logs to exec_log and is cleaned up when the process exits.

@goochjj
Copy link
Contributor

goochjj commented Jun 16, 2020

@mheon do you want to squash this so the gating check passes....

@mheon
Copy link
Member Author

mheon commented Jun 16, 2020

We actually have a new Conmon out now, so we're expecting that tests should start passing - let me get this passing

@mheon mheon force-pushed the fix_exec_logdriver branch 2 times, most recently from d93e579 to d739cce Compare June 17, 2020 14:07
@mheon
Copy link
Member Author

mheon commented Jun 17, 2020

Rebased, but could use a few tests for the new functionality, let me add some.

@mheon mheon force-pushed the fix_exec_logdriver branch from d739cce to 9777750 Compare June 17, 2020 14:26
@mheon
Copy link
Member Author

mheon commented Jun 17, 2020

Added a test for the none logdriver. Re-enabled exec tests on ubuntu, which should work now that Conmon 2.0.18 is available in the libcontainers repos.

When the container uses journald logging, we don't want to
automatically use the same driver for its exec sessions. If we do
we will pollute the journal (particularly in the case of
healthchecks) with large amounts of undesired logs. Instead,
force exec sessions logs to file for now; we can add a log-driver
flag later (we'll probably want to add a `podman logs` command
that reads exec session logs at the same time).

As part of this, add support for the new 'none' logs driver in
Conmon. It will be the default log driver for exec sessions, and
can be optionally selected for containers.

Great thanks to Joe Gooch ([email protected]) for adding support
to Conmon for a null log driver, and wiring it in here.

Fixes containers#6555

Signed-off-by: Matthew Heon <[email protected]>
@mheon mheon force-pushed the fix_exec_logdriver branch from 71d1ad2 to 0e171b7 Compare June 17, 2020 15:11
@mheon
Copy link
Member Author

mheon commented Jun 17, 2020

[+0123s] Error: AppArmor profile "container-default" specified but not loaded on ubuntu, again... I have no idea where this is coming from. Going to restart, hopefully it's an environment issue.

@mheon
Copy link
Member Author

mheon commented Jun 17, 2020

I think they're going to pass this time...
@rhatdan @baude @vrothberg @giuseppe @TomSweeneyRedHat PTAL

@rhatdan
Copy link
Member

rhatdan commented Jun 17, 2020

LGTM

@TomSweeneyRedHat
Copy link
Member

LGTM
and happy green test buttons!

@rhatdan
Copy link
Member

rhatdan commented Jun 17, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 17, 2020
@openshift-merge-robot openshift-merge-robot merged commit 7b00e49 into containers:master Jun 17, 2020
@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 24, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 24, 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.

podman exec commands use log-driver from podman run
7 participants