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.

Sign up for GitHub

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

Jump to bottom

core: introduce BindJournalSockets= #32487

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

core: introduce BindJournalSockets= #32487

wants to merge 5 commits into from

Conversation

YHNdnzj
Copy link
Member

@YHNdnzj YHNdnzj commented Apr 25, 2024 •
edited by github-actions bot

As requested in #32478.

Note that I cannot reproduce the mentioned problem, i.e. sd-executor logs fine for me even when using the said combinations. However, this setting is generally useful IMO.

@YHNdnzj YHNdnzj added the pid1 label Apr 25, 2024
@github-actions github-actions bot added documentation util-lib please-review PR is ready for (re-)review by a maintainer labels Apr 25, 2024
@YHNdnzj YHNdnzj added the ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR label Apr 25, 2024
@github-actions github-actions bot removed the ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR label Apr 26, 2024
@YHNdnzj YHNdnzj marked this pull request as draft April 26, 2024 02:25
@github-actions GitHub Actions
Copy link

Important

An -rc1 tag has been created and a release is being prepared, so please note that PRs introducing new features and APIs will be held back until the new version has been released.

@YHNdnzj YHNdnzj removed the please-review PR is ready for (re-)review by a maintainer label Apr 26, 2024
@YHNdnzj YHNdnzj mentioned this pull request Apr 26, 2024
Merged
@YHNdnzj YHNdnzj force-pushed the bind-journal-sockets branch 2 times, most recently from 581b586 to 9705498 Compare April 26, 2024 03:10
@YHNdnzj YHNdnzj added this to the v257 milestone Apr 26, 2024
@YHNdnzj YHNdnzj linked an issue Apr 26, 2024 that may be closed by this pull request
The journal socket should be mounted automatically if MountAPIVFS= is applied and StandardOutput/Error=journal #32478
Open
@YHNdnzj YHNdnzj marked this pull request as ready for review April 26, 2024 04:16
@github-actions github-actions bot added the please-review PR is ready for (re-)review by a maintainer label Apr 26, 2024
DaanDeMeyer
DaanDeMeyer requested changes Apr 26, 2024
View reviewed changes
Copy link
Contributor

@DaanDeMeyer DaanDeMeyer left a comment

Choose a reason for hiding this comment

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

Can you change the portable profiles and the integration tests to stop using ReadOnlyPaths= to bind in the journal log sockets as well? And maybe set the option explicitly in a few cases in TEST-50-DISSECT so we have some extra coverage.

@DaanDeMeyer DaanDeMeyer added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks and removed please-review PR is ready for (re-)review by a maintainer labels Apr 26, 2024
@github-actions github-actions bot added tests portable Anything to do with systemd-portable and portablectl and portables please-review PR is ready for (re-)review by a maintainer and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Apr 26, 2024
@DaanDeMeyer DaanDeMeyer added good-to-merge/after-next-release and removed please-review PR is ready for (re-)review by a maintainer labels Apr 26, 2024
@YHNdnzj YHNdnzj force-pushed the bind-journal-sockets branch 2 times, most recently from 4d7e18f to 324f71e Compare April 26, 2024 11:03
@mrc0mmand

This comment was marked as resolved.

Sign in to view
poettering
poettering reviewed Apr 26, 2024
View reviewed changes
man/systemd.exec.xml Outdated Show resolved Hide resolved
poettering
poettering reviewed Apr 26, 2024
View reviewed changes
man/systemd.exec.xml
in conjunction with either <varname>RootDirectory=</varname> or <varname>RootImage=</varname>.</para>

<xi:include href="version-info.xml" xpointer="v257"/></listitem>
</varlistentry>
Copy link
Member

Choose a reason for hiding this comment

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

I don't grok the usecase for an explicit switch?

Copy link
Member Author

@YHNdnzj YHNdnzj Apr 26, 2024 •
edited

Choose a reason for hiding this comment

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

Hmm, so in the majority of cases this would be implied, but it feels very weird to me if this is directly bound to MountAPIVFS=yes. E.g. people might use TemporaryFileSystem=/, and for that they might only want the journal sockets but no other API file systems. Also, if they use something like StandardOutput/Error=null, they'll probably also close down journal socket access.

@YHNdnzj YHNdnzj mentioned this pull request Apr 27, 2024
Merged
@YHNdnzj YHNdnzj force-pushed the bind-journal-sockets branch 2 times, most recently from ddbfb96 to ad2b388 Compare April 29, 2024 13:29
@YHNdnzj YHNdnzj removed the selinux label May 1, 2024
@YHNdnzj YHNdnzj force-pushed the bind-journal-sockets branch 2 times, most recently from c1e51d5 to c8bcbf3 Compare May 8, 2024 17:01
@YHNdnzj YHNdnzj mentioned this pull request May 9, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@poettering poettering poettering left review comments

@DaanDeMeyer DaanDeMeyer DaanDeMeyer approved these changes

Assignees
No one assigned
Labels
documentation good-to-merge/after-next-release pid1 portable Anything to do with systemd-portable and portablectl and portables tests util-lib
Milestone
v257
Development

Successfully merging this pull request may close these issues.

The journal socket should be mounted automatically if MountAPIVFS= is applied and StandardOutput/Error=journal
4 participants
@YHNdnzj @mrc0mmand @poettering @DaanDeMeyer