Skip to content

Make log severity configurable and consistent between syslog and systemd#84574

Open
andrew-sayers wants to merge 4 commits intoansible:develfrom
andrew-sayers:log_severity
Open

Make log severity configurable and consistent between syslog and systemd#84574
andrew-sayers wants to merge 4 commits intoansible:develfrom
andrew-sayers:log_severity

Conversation

@andrew-sayers
Copy link

@andrew-sayers andrew-sayers commented Jan 17, 2025

SUMMARY

This builds on a discussion in ansible/proposals#214, and replaces #84504.

syslog messages are currently hardcoded to LOG_INFO, systemd messages have no defined priority.
Add a TARGET_LOG_SEVERITY option and use it in both logging frameworks.

Ansible log messages are sent through systemd on systems that have the python3 sdnotify package installed. This could be deliberate, but in my case, I just happen to have some services that depend on Debian's python3-sdnotify package. Setting a priority for these messages makes it easier to filter them.

There is already a syslog_facility option, so it seems natural to add a severity option as part of this job.

Note: the final commit in this PR changes the value of syslog_facility in parameters.py. Ansible tried to evaluate this as syslog.INFO, failed because there's no such symbol, and fell back to LOG_USER. I've changed the value to LOG_USER in this PR because it's a tangentially related and easy to review at the same time, but the first two commits can be accepted separately if that's better.

ISSUE TYPE
  • Feature Pull Request

See also ansible/proposals#214

C# EventLogEntryType values do not map neatly to syslog-style
severity levels, so this option is ignored in C#.
lib/ansible/module_utils/basic.py sets self._syslog_facility = 'LOG_USER'.
'INFO' is not a valid facility, so this probably never worked as expected.
@ansibot ansibot added feature This issue/PR relates to a feature request. needs_triage Needs a first human triage before being processed. labels Jan 17, 2025
@s-hertel s-hertel removed the needs_triage Needs a first human triage before being processed. label Jan 21, 2025
Copy link
Contributor

@s-hertel s-hertel 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 add a changelog please?

@s-hertel s-hertel requested a review from jborean93 January 21, 2025 15:46
@andrew-sayers
Copy link
Author

@s-hertel - done.

I think this counts as a scope-less change - if not, please let me know what the scope would be.

This shouldn't break ansible tasks per se, but could break related scripts for a user who monitors Ansible by e.g. grabbing systemd logs with a missing PRIORITY, or who fills in a priority other than LOG_INFO. I've marked it as a breaking_change, but happy to change if that doesn't count.

@@ -0,0 +1,2 @@
breaking_changes:
- Ansible's systemd logs now contain a PRIORITY field (deault value: 5). Downstream programs that inspect Ansible's systemd logs should be updated if they expect the PRIORITY field to be missing (https://github.com/ansible/ansible/pull/84574).
Copy link
Member

Choose a reason for hiding this comment

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

https://dev.azure.com/ansible/ansible/_build/results?buildId=135086&view=logs&j=6b8fca62-2c63-5c74-2051-15ea31c8c104&t=e6a77036-d6ac-54d6-bbbb-976ea775a86e&l=479

It's a bit unobvious, but since you have that : in the middle (with a whitespace), the YAML parser makes it a key-value mapping. Here's one way to fix it:

Suggested change
- Ansible's systemd logs now contain a PRIORITY field (deault value: 5). Downstream programs that inspect Ansible's systemd logs should be updated if they expect the PRIORITY field to be missing (https://github.com/ansible/ansible/pull/84574).
- >-
Ansible's systemd logs now contain a PRIORITY field (deault value: 5).
Downstream programs that inspect Ansible's systemd logs should be updated if
they expect the PRIORITY field to be missing
(https://github.com/ansible/ansible/pull/84574).

@webknjaz webknjaz added the ci_verified Changes made in this PR are causing tests to fail. label Jan 21, 2025
@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed ci_verified Changes made in this PR are causing tests to fail. labels Jan 21, 2025
Copy link
Contributor

@jborean93 jborean93 left a comment

Choose a reason for hiding this comment

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

From an implementation perspective I'm fine with the changes made to Ansible.Basic.cs

@ansibot ansibot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Jan 22, 2025
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Feb 4, 2025
@ansibot ansibot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Apr 15, 2025
@ansibot ansibot added the stale_pr This PR has not been pushed to for more than one year. label Jan 29, 2026
@Akasurde
Copy link
Member

@andrew-sayers Could you please rebase this branch? Thanks,

@Akasurde Akasurde added the needs_info This issue requires further information. Please answer any outstanding questions. label Feb 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature This issue/PR relates to a feature request. needs_info This issue requires further information. Please answer any outstanding questions. needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. stale_pr This PR has not been pushed to for more than one year.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants