Skip to content

Allow passing an ID to journald log driver #1541

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
Merged
merged 1 commit into from
Sep 3, 2024
Merged

Allow passing an ID to journald log driver #1541

merged 1 commit into from
Sep 3, 2024

Conversation

saschagrunert
Copy link
Member

@saschagrunert saschagrunert commented Sep 2, 2024 •
edited
Loading

The ID has been dropped before this patch, but we can use it to refer a container ID or anything else identifiable.

Will be used by conmon-rs in https://github.com/containers/conmon-rs/pull/2401/files#diff-5d1350be7d73e8dfd3f43f70d9d71838500a7c04b2ee5ee19f1c4521f9df7d12R395

Refers to #1525

@packit-as-a-service Packit-as-a-Service
Copy link

We were not able to find or create Copr project packit/containers-crun-1541 specified in the config with the following error:

Packit received HTTP 500 Internal Server Error from Copr Service. Check the Copr status page: https://copr.fedorainfracloud.org/status/stats/, or ask for help in Fedora Build System matrix channel: https://matrix.to/#/#buildsys:fedoraproject.org.

Unless the HTTP status code above is >= 500, please check your configuration for:

  1. typos in owner and project name (groups need to be prefixed with @)
  2. whether the project name doesn't contain not allowed characters (only letters, digits, underscores, dashes and dots must be used)
  3. whether the project itself exists (Packit creates projects only in its own namespace)
  4. whether Packit is allowed to build in your Copr project
  5. whether your Copr project/group is not private

@kwilczynski
Copy link
Member

/approve
/lgtm

@packit-as-a-service Packit-as-a-Service
Copy link

Ephemeral COPR build failed. @containers/packit-build please check.

kwilczynski
kwilczynski reviewed Sep 2, 2024
View reviewed changes
src/libcrun/error.c
case LOG_TYPE_JOURNALD:
*new_output_handler = log_write_to_journald;
*new_output_handler_arg = NULL;
*new_output_handler_arg = (void *) id;
Copy link
Member

Choose a reason for hiding this comment

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

An idea...

Looking at the callback signature:

void log_write_to_journald (int errno_, const char *msg, int verbosity, void *arg arg_unused)

We could rename the arg_unused to something else now, especially since it will be used.

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense, I renamed it to id.

Copy link
Member Author

@saschagrunert saschagrunert Sep 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ah I had to revert since it's still technically unused when building without systemd support: https://github.com/containers/crun/actions/runs/10678078263/job/29594436938?pr=1541

@packit-as-a-service Packit-as-a-Service
Copy link

podman system tests failed. @containers/packit-build please check.

@giuseppe
Copy link
Member

giuseppe commented Sep 2, 2024

LGTM, what do you think about the comment from @kwilczynski?

flouthoc
flouthoc approved these changes Sep 2, 2024
View reviewed changes
Copy link
Collaborator

@flouthoc flouthoc left a comment

Choose a reason for hiding this comment

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

LGTM

@packit-as-a-service Packit-as-a-Service
Copy link

Ephemeral COPR build failed. @containers/packit-build please check.

@saschagrunert
Allow passing an ID to journald log driver
544fe3f 
The ID has been dropped before this patch, but we can use it to refer a
container ID or anything else identifiable.

Signed-off-by: Sascha Grunert <sgrunert@redhat.com>
@packit-as-a-service Packit-as-a-Service
Copy link

podman system tests failed. @containers/packit-build please check.

giuseppe
giuseppe approved these changes Sep 3, 2024
View reviewed changes
Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

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

LGTM

@giuseppe giuseppe merged commit 28b47d6 into containers:main Sep 3, 2024
34 of 56 checks passed
@saschagrunert saschagrunert deleted the journal-id branch September 3, 2024 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@giuseppe giuseppe giuseppe approved these changes

@flouthoc flouthoc flouthoc approved these changes

+1 more reviewer

@kwilczynski kwilczynski kwilczynski left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@saschagrunert @kwilczynski @giuseppe @flouthoc