Journald Logger #1800

wasup-yash · 2023-10-26T19:52:43Z

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.

conmon-rs/server/src/container_log/journald.rs

saschagrunert · 2023-11-20T08:10:20Z

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

wasup-yash · 2023-11-20T20:04:15Z

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

yes surely

saschagrunert · 2023-12-08T08:11:14Z

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

conmon-rs/server/src/container_io.rs

conmon-rs/server/src/container_log/container_log.rs

pkg/client/client.go

grassdionera · 2024-10-09T06:32:28Z

would love to see this merged. then cri-o/cri-o can support journald logging when they switch to common-rs

saschagrunert · 2024-10-14T13:20:47Z

@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 · 2024-10-14T19:01:52Z

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

openshift-ci · 2024-10-14T19:02:02Z

[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:

~~OWNERS~~ [haircommander]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

rphillips · 2024-10-14T19:15:39Z

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

openshift-ci · 2024-10-15T07:21:57Z

New changes are detected. LGTM label has been removed.

saschagrunert · 2024-10-15T07:24:30Z

@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 · 2024-10-15T12:23:52Z

@haircommander memory usage ok?

openshift-ci bot added do-not-merge/work-in-progress do-not-merge/release-note-label-needed labels Oct 26, 2023

wasup-yash changed the title ~~Journal~~ Journald Logger Oct 26, 2023

haircommander reviewed Oct 26, 2023

View reviewed changes

conmon-rs/server/src/container_log/journald.rs Outdated Show resolved Hide resolved

openshift-merge-robot added the needs-rebase label Nov 19, 2023

openshift-merge-robot removed the needs-rebase label Nov 20, 2023

wasup-yash force-pushed the journal branch from 45399e3 to 93b156d Compare November 30, 2023 17:28

openshift-merge-robot added the needs-rebase label Nov 30, 2023

wasup-yash force-pushed the journal branch from 93b156d to db759b4 Compare November 30, 2023 17:30

openshift-ci bot added release-note and removed do-not-merge/release-note-label-needed labels Dec 7, 2023

wasup-yash force-pushed the journal branch from db759b4 to 844f82b Compare December 10, 2023 20:40

openshift-merge-robot removed the needs-rebase label Dec 10, 2023

wasup-yash marked this pull request as ready for review January 5, 2024 12:37

openshift-ci bot removed the do-not-merge/work-in-progress label Jan 5, 2024