Skip to content

journal: add StderrIsJournalStream function#410

Merged
lucab merged 4 commits into
coreos:mainfrom
WGH-:env-journal-stream
Nov 7, 2022
Merged

journal: add StderrIsJournalStream function#410
lucab merged 4 commits into
coreos:mainfrom
WGH-:env-journal-stream

Conversation

@WGH-

@WGH- WGH- commented Oct 19, 2022

Copy link
Copy Markdown
Contributor

This function can be used for automatic protocol upgrade described in [1].

Both unit tests and runnable example are included. Only the latter requires systemd, as unit tests are self-sufficient, and only test that JOURNAL_STREAM environment variable is checked properly.

[1] https://systemd.io/JOURNAL_NATIVE_PROTOCOL/#automatic-protocol-upgrading

@WGH- WGH- force-pushed the env-journal-stream branch from ee7ac9c to b259d42 Compare October 19, 2022 18:14
@lucab

lucab commented Oct 20, 2022

Copy link
Copy Markdown
Contributor

Thanks for the PR!

At the high level, I think this API should be renamed to avoid mentioning stderr. In fact, the upgrade dance works the same way for stderr as well as for stdout, it's just that the systemd documentation is slightly deceiving on that front. See systemd/systemd#6800 for a concrete example of what I'm talking about.

In the Rust land this logic is implemented by https://docs.rs/libsystemd/0.5.0/libsystemd/logging/fn.connected_to_journal.html, so I think similarly here it could be called IsConnectedToJournal(). Also, it should check device and inode matching against stdout too (the final result is the OR of the two checks).

@WGH-

WGH- commented Oct 20, 2022

Copy link
Copy Markdown
Contributor Author

As a counter-example, systemd components check only stderr when deciding whether to upgrade to the native journal protocol or not: https://github.com/systemd/systemd/blob/40c05a34595ed769ce676206f3c5de874f9a9234/src/basic/log.c#L217-L241 (this is not a part of e.g. public libsystemd, it's used internally by various systemd binaries)

In theory, stdout and stderr might be configured differently. StandardOutput=/ StandardError= can be changed explicitly, and shell-outs/shell pipelines are likely to make them different. It would be surprising if, for example, if stdout is configured to go to a file or a pipe, and application would redirect output (that it used to print to stdout) to the journal anyway (because stderr is still connected to the journal).

@lucab

lucab commented Oct 20, 2022

Copy link
Copy Markdown
Contributor

The upgrade protocol is a bit subpar from the point of view of an application, as it uses a single env-variable and allow mixing two different concerns into that.
But I think you have a good point about mixed-use scenarios, and the docs also highlight the same case.
I'll reach out privately to the systemd folks for some customary feedback, but I guess it would be good to expose checks for stdout/stderr separately. That will give more flexibility to applications/consumers.

@WGH- WGH- force-pushed the env-journal-stream branch from b259d42 to 6b5ddca Compare October 20, 2022 16:13
@lucab

lucab commented Nov 3, 2022

Copy link
Copy Markdown
Contributor

I had an out-of-band chat with systemd upstream folks and I'm coming back with two main clarifications:

  • probing logic should be performed separately for stdout and stderr. That is, it makes sense to have two helpers to check is *stdout* a journal stream? and is *stderr* a journal stream? separately.
  • the protocol upgrading logic described in https://systemd.io/JOURNAL_NATIVE_PROTOCOL/#automatic-protocol-upgrading is only concerned with stderr. That is, if and only if stderr is a journal stream then automatic protocol upgrading is feasible.

@lucab

lucab commented Nov 3, 2022

Copy link
Copy Markdown
Contributor

Given the above, the StderrIsJournalStream() method here is fine to stay.
Ideally it would be good to have a StdoutIsJournalStream() next to it.

Regarding protocol upgrade detection, this is effectively the same logic as the stderr check.
We can decide to either leave it out, or add something dedicated like IsConnectedToJournal() with a docstring to explain the protocol upgrade dance.
I have a slightly preference for the latter, as it helps capturing the nuances of this ticket discussion directly in the code.

lucab
lucab previously requested changes Nov 3, 2022
Comment thread journal/journal_unix.go Outdated
// is present, but malformed, fstat syscall fails, etc.
//
// [Journal Native Protocol]: https://systemd.io/JOURNAL_NATIVE_PROTOCOL/#automatic-protocol-upgrading
func StderrIsJournalStream() (res bool, err error) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd be happier if we could avoid named return values, as they can become the source of subtle bugs when capturing. Plain explicit return statements are better, thanks!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@WGH- WGH- force-pushed the env-journal-stream branch from 6b5ddca to e87ae12 Compare November 3, 2022 14:42

@WGH- WGH- left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I suppose I could try to write an integration tests that would run on CI. That would take some time, though. I'll try for a couple of days, and if I don't reply here, consider I've given up.

@WGH-

WGH- commented Nov 3, 2022

Copy link
Copy Markdown
Contributor Author

Given the above, the StderrIsJournalStream() method here is fine to stay. Ideally it would be good to have a StdoutIsJournalStream() next to it.

So you want me to add it then?

Regarding protocol upgrade detection, this is effectively the same logic as the stderr check. We can decide to either leave it out, or add something dedicated like IsConnectedToJournal() with a docstring to explain the protocol upgrade dance. I have a slightly preference for the latter, as it helps capturing the nuances of this ticket discussion directly in the code.

StderrIsJournalStream()'s docstring already refers to https://systemd.io/JOURNAL_NATIVE_PROTOCOL/#automatic-protocol-upgrading, and there's an example of said dance that gets rendered in godoc, isn't it enough?

And if we introduce IsConnectedToJournal(), is it going to be simply an alias for StderrIsJournalStream()?

@lucab

lucab commented Nov 3, 2022

Copy link
Copy Markdown
Contributor

So you want me to add it then?

Yes, if you are up for it. Otherwise I'll handle that in a subsequent PR.

there's an example of said dance that gets rendered in godoc, isn't it enough?
And if we introduce IsConnectedToJournal(), is it going to be simply an alias for StderrIsJournalStream()?

Yes, it would be a plain alias. Leaving this one up to you. If it already feels clear enough, I'm ok with. We can always add new helpers later if we change idea in the future.

WGH- added a commit to WGH-/go-systemd that referenced this pull request Nov 4, 2022
@WGH- WGH- force-pushed the env-journal-stream branch from e87ae12 to 78aa072 Compare November 4, 2022 13:29
WGH- added a commit to WGH-/go-systemd that referenced this pull request Nov 4, 2022
@WGH- WGH- force-pushed the env-journal-stream branch 3 times, most recently from bbbf9b7 to 1e01118 Compare November 5, 2022 10:26
This function can be used for automatic protocol upgrade described in
[1].

Both unit tests and runnable example are included. Only the latter
requires systemd, as unit tests are self-sufficient, and only test
that JOURNAL_STREAM environment variable is checked properly.

[1] https://systemd.io/JOURNAL_NATIVE_PROTOCOL/#automatic-protocol-upgrading
WGH- added a commit to WGH-/go-systemd that referenced this pull request Nov 5, 2022
@WGH- WGH- force-pushed the env-journal-stream branch 2 times, most recently from 3e5dce4 to ea42749 Compare November 5, 2022 11:04
@WGH-

WGH- commented Nov 5, 2022

Copy link
Copy Markdown
Contributor Author

So I added StdoutIsJournalStream, and written some documentation as I saw fit. Please correct me if my understanding of its usefulness and limitations is wrong.

Additionally, I've written a somewhat complicated integration test for StderrIsJournalStream that executes itself as a transient systemd service to ensure it picks up JOURNAL_STREAM as it's initialized by systemd.

Unfortunately, it doesn't work as is on Ubuntu 16.04 due to systemd-run not supporting --wait. Instead of adding more workarounds to an already complicated test, I bumped distro versions instead. Ubuntu 16.04 is old and not supported anyway (except for commercial ESM). If that's inappropriate, let's discuss other options.

@WGH- WGH- requested a review from lucab November 5, 2022 11:12
@WGH- WGH- force-pushed the env-journal-stream branch from ea42749 to 2a4f72f Compare November 5, 2022 16:43
@lucab

lucab commented Nov 7, 2022

Copy link
Copy Markdown
Contributor

I bumped distro versions instead. Ubuntu 16.04 is old and not supported anyway (except for commercial ESM). If that's inappropriate, let's discuss other options.

That's totally fine. The old version there is just because we hadn't recently touched the CI setup.

@lucab lucab dismissed their stale review November 7, 2022 09:10

stale

Comment thread journal/journal_unix.go Outdated
Comment thread journal/journal_unix.go Outdated
Comment thread journal/journal_windows.go
WGH- added a commit to WGH-/go-systemd that referenced this pull request Nov 7, 2022
@WGH- WGH- force-pushed the env-journal-stream branch from 2a4f72f to b6075d1 Compare November 7, 2022 13:12
WGH- added 3 commits November 7, 2022 16:35
A test that will be added in a latter commit requires systemd-run --wait,
which is not available on Ubuntu 16.04.

DEBIAN_FRONTEND=noninteractive prevents hang on "Setting up tzdata"
that became a problem on Ubuntu 20.04 (though something similar
could happen on any Debian/Ubuntu version).
To ensure that StderrIsJournalStream properly works in real conditions,
this test re-executes itself with systemd-run, and observes exit code
and logged entries.
@WGH- WGH- force-pushed the env-journal-stream branch from b6075d1 to 4ca6222 Compare November 7, 2022 13:35
@lucab lucab enabled auto-merge November 7, 2022 13:49

@lucab lucab left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, thanks a lot for your contribution!

@lucab lucab merged commit d5623bf into coreos:main Nov 7, 2022
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.

2 participants