Skip to content

Conversation

@haircommander
Copy link
Collaborator

Signed-off-by: Peter Hunt [email protected]

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 19, 2019
@haircommander
Copy link
Collaborator Author

(ignore this for now folks, starting this by seeing if updating CRI-O breaks CI)

@rh-atomic-bot
Copy link
Collaborator

☔ The latest upstream changes (presumably #2833) made this pull request unmergeable. Please resolve the merge conflicts.

@rhatdan
Copy link
Member

rhatdan commented Apr 3, 2019

@haircommander Needs rebase.

@haircommander
Copy link
Collaborator Author

@rhatdan needs much more than that :) thanks for the heads up

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 13, 2019
@openshift-ci-robot openshift-ci-robot added size/L and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/S labels May 19, 2019
@haircommander haircommander force-pushed the journald branch 2 times, most recently from 9c43499 to 6499206 Compare May 19, 2019 02:07
@haircommander haircommander force-pushed the journald branch 3 times, most recently from cac4e1b to 35a030d Compare May 20, 2019 16:45
@haircommander haircommander changed the title WIP Add libpod journald logging Add libpod journald logging May 20, 2019
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 20, 2019
@haircommander
Copy link
Collaborator Author

Alrighty team, the last of the changes I wanted are in.
--log-driver journald is now supported, as well as podman logs with a container that runs a journald logger.
Also added tests :)

The one thing that is missing is podman logs --follow, but given the sdjournal package API, it'll be kind of annoying to translate that for what we have, so I've opted to disable it and document doing so.

The one thing I'd like to wait for is bumping conmon version to 0.2.0 and make sure it's documented users should have that version to ensure they can actually use this feature. In the meantime, PTAL

@baude @mheon @vrothberg @rhatdan @jwhonce

@mheon
Copy link
Member

mheon commented May 20, 2019

/approve

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: haircommander, mheon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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 May 20, 2019
@rh-atomic-bot
Copy link
Collaborator

☔ The latest upstream changes (presumably #3190) made this pull request unmergeable. Please resolve the merge conflicts.

Signed-off-by: Peter Hunt <[email protected]>
Add a journald reader that translates the journald entry to a k8s-file formatted line, to be added as a log line

Note: --follow with journald hasn't been implemented. It's going to be a larger undertaking that can wait.

Signed-off-by: Peter Hunt <[email protected]>
Signed-off-by: Peter Hunt <[email protected]>
@haircommander
Copy link
Collaborator Author

last remaining issue: containers/conmon#34
which is only an issue when -t --log-driver journald

@@ -367,8 +367,6 @@ func (r *OCIRuntime) execContainer(c *Container, cmd, capAdd, env []string, tty
args := []string{}

// TODO - should we maintain separate logpaths for exec sessions?
Copy link
Member

Choose a reason for hiding this comment

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

Is this comment still valid

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It will no longer be once i finish conmon exec :)

@rhatdan
Copy link
Member

rhatdan commented May 29, 2019

LGTM

@haircommander
Copy link
Collaborator Author

@mheon @baude PTAL--I think the --follow -t issues should come in a follow up, they require some changes in conmon

@mheon
Copy link
Member

mheon commented May 29, 2019

@haircommander What do we think about json-file? Are we going to keep aliasing this to the Kube logs indefinitely?

@mheon
Copy link
Member

mheon commented May 29, 2019

I'm willing to let this into 1.4.0, as it's pretty self contained

@haircommander
Copy link
Collaborator Author

@mheon json-file is in my list of things to do--needs addition in conmon first and it isn't high on my priority list. in other words: it'll be an alias until I get to fixing it :)

@haircommander
Copy link
Collaborator Author

containers/conmon#37 here's a tracker--I can file an issue here if you'd like it

@mheon
Copy link
Member

mheon commented May 29, 2019

Can we throw a not-implemented error instead? I'd prefer to not establish an alias now and break it later

@haircommander
Copy link
Collaborator Author

haircommander commented May 29, 2019

@mheon Actually, it's not an alias

$ ./bin/podman run --log-driver json-file alpine ls
[conmon:e]: No such log driver json-file
Error: exit status 1

@haircommander
Copy link
Collaborator Author

though, would you prefer if I moved the error higher up the stack? It'll be caught in conmon but may be better not to waste time pretending it'll work in podman

@mheon
Copy link
Member

mheon commented May 29, 2019

Alright, LGTM then

@mheon
Copy link
Member

mheon commented May 29, 2019

Eh, I think we're ok with asking Conmon nicely... no changes on our end when it is implemented
/lgtm

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

6 participants