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

Journald Logger #1800

Merged
merged 1 commit into from
Oct 21, 2024
Merged

Journald Logger #1800

merged 1 commit into from
Oct 21, 2024

Conversation

wasup-yash
Copy link
Contributor

@wasup-yash wasup-yash commented Oct 26, 2023

What type of PR is this?

/kind feature

What this PR does / why we need it:

This PR is to implement additional log driver support for the Journald which enables the container logs written to the systemd journal.

Which issue(s) this PR fixes:

part of #1126

Special notes for your reviewer:

None

Does this PR introduce a user-facing change?

Added journald container logger support.

@saschagrunert
Copy link
Member

@wasup-yash may I ask you to rebase this one?

@wasup-yash
Copy link
Contributor Author

@wasup-yash may I ask you to rebase this one

yes surely

@saschagrunert
Copy link
Member

@wasup-yash do you want to give this one a rebase on top of main?

pkg/client/client.go Outdated Show resolved Hide resolved
pkg/client/client.go Outdated Show resolved Hide resolved
@wasup-yash wasup-yash force-pushed the journal branch 2 times, most recently from 9dafa25 to c1768fc Compare January 6, 2024 18:47
@wasup-yash wasup-yash force-pushed the journal branch 5 times, most recently from 69cd676 to d9f25af Compare January 12, 2024 19:51
@saschagrunert saschagrunert force-pushed the journal branch 9 times, most recently from 2cce5b3 to b3eaea6 Compare October 14, 2024 12:53
@saschagrunert
Copy link
Member

@haircommander @kwilczynski I rebased this one and added some fixes. I guess we can follow-up with integration tests in a dedicated PR, but that's now ready to get merged from my point of view. PTAL.

@haircommander
Copy link
Collaborator

I worry about merging before integration tests TBH
/approve
/lgtm
/hold

You can lift the hold if you'd like, but we should get the tests ASAP to make sure the structure is correct IMO

Copy link
Contributor

openshift-ci bot commented Oct 14, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: haircommander, wasup-yash

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

@rphillips
Copy link
Collaborator

I'm good with merging. Looks like we default to the CRI logger anyway.
+1 to integration tests as a followup.

@saschagrunert
Copy link
Member

@haircommander I added an integration test to ensure the correct functionality.

Signed-off-by: wasup-yash <[email protected]>
Signed-off-by: Peter Hunt <[email protected]>
Signed-off-by: Sascha Grunert <[email protected]>
@rphillips
Copy link
Collaborator

@haircommander memory usage ok?

@saschagrunert
Copy link
Member

@haircommander PTAL

@haircommander
Copy link
Collaborator

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Oct 21, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit fd287ed into containers:main Oct 21, 2024
35 checks passed
@saschagrunert saschagrunert deleted the journal branch October 21, 2024 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants