feat: add role fingerprint to system journal#614
feat: add role fingerprint to system journal#614richm merged 1 commit intolinux-system-roles:mainfrom
Conversation
Reviewer's GuideIntroduces a new custom Ansible module sr_fingerprint that logs role usage messages to syslog, wires it into the storage role to emit begin/success fingerprints, and extends the default test to validate these fingerprints via journalctl, while keeping ansible-sanity ignores in sync for supported versions. Sequence diagram for role execution with sr_fingerprint loggingsequenceDiagram
actor Operator
participant AnsibleController
participant storage_role
participant sr_fingerprint_module
participant Syslog
Operator ->> AnsibleController: Run playbook including storage_role
AnsibleController ->> storage_role: Execute role tasks
Note over storage_role: At role start (set_vars.yml)
storage_role ->> sr_fingerprint_module: sr_message="begin system_role:storage ..."
sr_fingerprint_module ->> Syslog: log(begin_message_with_timestamp)
storage_role ->> storage_role: Perform storage configuration tasks
Note over storage_role: At role end (main.yml)
storage_role ->> sr_fingerprint_module: sr_message="success system_role:storage ..."
sr_fingerprint_module ->> Syslog: log(success_message_with_timestamp)
Syslog -->> Operator: Logs visible via journalctl and exports
Class diagram for the new sr_fingerprint Ansible moduleclassDiagram
class sr_fingerprint_module {
+run_module()
+main()
+_local_iso8601_no_microseconds()
}
class AnsibleModule {
}
sr_fingerprint_module ..> AnsibleModule : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
sr_fingerprintmodule always reportschanged=True, but the role immediately overrides this withchanged_when: false; consider making the module itself reportchanged=False(or configurable via a parameter) so its behavior is consistent and consumers don’t need to override it. - In
sr_fingerprint.run_module, you might want to validate thatsr_messageis non-empty/meaningful (e.g., not just whitespace) before logging, to avoid writing accidental empty fingerprints to syslog.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `sr_fingerprint` module always reports `changed=True`, but the role immediately overrides this with `changed_when: false`; consider making the module itself report `changed=False` (or configurable via a parameter) so its behavior is consistent and consumers don’t need to override it.
- In `sr_fingerprint.run_module`, you might want to validate that `sr_message` is non-empty/meaningful (e.g., not just whitespace) before logging, to avoid writing accidental empty fingerprints to syslog.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #614 +/- ##
==========================================
- Coverage 16.54% 10.16% -6.39%
==========================================
Files 2 9 +7
Lines 284 2056 +1772
Branches 79 0 -79
==========================================
+ Hits 47 209 +162
- Misses 237 1847 +1610
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
richm
force-pushed
the
fingerprint-journal
branch
from
f72d6b5 to
4cc798d
Compare
|
[citest] |
richm
force-pushed
the
fingerprint-journal
branch
from
4cc798d to
01ba9b2
Compare
|
[citest] |
| --- | ||
| - name: Record storage role fingerprint in syslog | ||
| sr_fingerprint: | ||
| sr_message: "system_role:storage" |
There was a problem hiding this comment.
I am thinking about other useful info to be added with the fingerprint.
We can add timestamp in the message.
Also, maybe print Ansible version and platform for simplicity. Yes we can probably script getting this info from sos report content other than this line, but having it on the same line with the role fingerprint would make things easier, WDYT?
Would be interesting to see how many nodes ansible inventory had, I guess we can get this info on managed nodes by counting ansible_play_hosts.
There was a problem hiding this comment.
I am thinking about other useful info to be added with the fingerprint. We can add timestamp in the message.
This is what the message looks like in the journal:
Apr 17 10:38:55 hostname.domain.com python3.12[5169]: ansible-sr_fingerprint system_role:storage
I guess we could add an ISO 8601 format timestamp since the default doesn't have year or timezone:
Apr 17 10:38:55 hostname.domain.com python3.12[5169]: ansible-sr_fingerprint system_role:storage 2026-04-17T08:42:40-06:00
Also, maybe print Ansible version and platform for simplicity. Yes we can probably script getting this info from sos report content other than this line, but having it on the same line with the role fingerprint would make things easier, WDYT? Would be interesting to see how many nodes ansible inventory had, I guess we can get this info on managed nodes by counting ansible_play_hosts.
ok - I'll add ansible version and os-major.minor
There was a problem hiding this comment.
problem with adding platform: {{ ansible_facts['distribution'] }}-{{ ansible_facts['distribution_version'] }} - since this runs as the first task in the role, this runs before fact gathering - so these facts are not available - if we add the fingerprint after fact gathering, we don't get the benefit of having the fingerprint before everything else
There was a problem hiding this comment.
Here is the output with timestamp and ansible version:
Apr 17 11:33:12 hostname.domain.com python3.12[4732]: ansible-sr_fingerprint system_role:storage ansible_version=2.20.5rc1 2026-04-17T11:33:12-04:00
There was a problem hiding this comment.
problem with adding platform:
{{ ansible_facts['distribution'] }}-{{ ansible_facts['distribution_version'] }}- since this runs as the first task in the role, this runs before fact gathering - so these facts are not available - if we add the fingerprint after fact gathering, we don't get the benefit of having the fingerprint before everything else
To me, adding the fingerprint as the first thing is not really useful because it shows all attempts to run the role. If someone runs a role, fails, and never tries sysroles again, we still count them as our users.
I'd say let's add the fingerprint at the very end of a successful role run.
There was a problem hiding this comment.
Why add fingerprint at the very beginning?
There was a problem hiding this comment.
Why add fingerprint at the very beginning?
I thought it would be useful to know any and all role invocations at the very beginning, to see exactly when someone invoked the role.
Pros:
- know when someone invoked the role - can easily correlate this with other logs
- have a record of an attempt to invoke the role, even if the role fails or errors out
Cons:
- no facts available until after fact gathering - so not much available fingerprint metadata
- don't know if the role invocation was successful
I suppose we could combine the approaches - have a fingerprint at the beginning and at the end - the problem of doing it at the end is if you want to emit the fingerprint upon success or failure or error with the role result - yes, we could refactor the roles so that we run the role in a block/rescue/always - but there are some issues:
- if you use a
rescue, you need to "re-raise" the error, but you cannot reconstruct the exact sameansible_failed_resultto re-raise - if you only use an
always, I guess we could set a variable as the last task in theblockto indicate that the role completed successfully, so we could report success or failure/error, but we couldn't report the exact error
There was a problem hiding this comment.
Good point, well if we do two fingerprints for init and success, the absence of the success fingerprint indicates that the role failed. So no need to introduce rescue blocks everywhere.
Maybe still include the init fingerprint after collecting facts to provide platform?
There was a problem hiding this comment.
Good point, well if we do two fingerprints for
initandsuccess, the absence of thesuccessfingerprint indicates that the role failed. So no need to introduce rescue blocks everywhere. Maybe still include theinitfingerprint after collecting facts to provide platform?
ok - I can add the init fingerprint just after fact gathering, and add the success fingerprint as the last task in tasks/main.yml
richm
force-pushed
the
fingerprint-journal
branch
from
01ba9b2 to
079d844
Compare
|
[citest] |
richm
force-pushed
the
fingerprint-journal
branch
from
079d844 to
800953f
Compare
|
[citest] |
richm
force-pushed
the
fingerprint-journal
branch
from
800953f to
5699b5b
Compare
|
[citest] |
richm
force-pushed
the
fingerprint-journal
branch
from
5699b5b to
2d2c283
Compare
|
[citest] |
|
[citest_bad] |
richm
force-pushed
the
fingerprint-journal
branch
from
2d2c283 to
7c55be8
Compare
|
@spetrosi new implementation - here is what gets logged: |
spetrosi
left a comment
spetrosi
left a comment
There was a problem hiding this comment.
The fingerprint looks very good!
|
[citest] |
richm
force-pushed
the
fingerprint-journal
branch
from
7c55be8 to
f27c17c
Compare
|
[citest] |
Feature: Add role fingerprint to system journal as the first task in the role. Reason: The journal is often exported to reporting systems making it easy to tell if the role is being used, and when. Result: Users can get information in their reporting and log aggregation systems of role usage.
richm
force-pushed
the
fingerprint-journal
branch
from
f27c17c to
4ca81eb
Compare
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The begin/success fingerprint messages duplicate the same
ansible_versionand distribution formatting logic in two tasks; consider factoring this into a variable or helper so the format stays consistent if you ever need to change it. - The fingerprint messages hardcode
system_role:storage; if this pattern is intended to be reused in other roles, it might be worth deriving the role identifier from a variable so the module use stays generic.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The begin/success fingerprint messages duplicate the same `ansible_version` and distribution formatting logic in two tasks; consider factoring this into a variable or helper so the format stays consistent if you ever need to change it.
- The fingerprint messages hardcode `system_role:storage`; if this pattern is intended to be reused in other roles, it might be worth deriving the role identifier from a variable so the module use stays generic.
## Individual Comments
### Comment 1
<location path="library/sr_fingerprint.py" line_range="80" />
<code_context>
+ sr_message=dict(type="str", required=True),
+ )
+
+ module = AnsibleModule(
+ argument_spec=module_args,
+ supports_check_mode=True,
+ )
+
+ log_message = "%s %s" % (
+ module.params["sr_message"],
+ _local_iso8601_no_microseconds(),
+ )
+
+ if module.check_mode:
+ module.exit_json(
+ changed=False,
+ message="Check mode: message not logged - [%s]" % log_message,
+ )
+
+ module.log(log_message)
+
+ # we don't actually change anything, so we're not changed - writing a log message
+ # is not considered a change
+ # also, we don't want to report changed every time the role runs
+ module.exit_json(changed=False)
+
+
</code_context>
<issue_to_address>
**suggestion:** Include the logged message in the module result to aid debugging and play recap visibility.
In the non-check path, the module currently calls `exit_json(changed=False)` with no extra data. Consider adding the final `log_message` (or at least `sr_message`) to the result, e.g. `module.exit_json(changed=False, logged_message=log_message)`, so callbacks and play recap can show what was logged without requiring syslog access.
```suggestion
module.exit_json(changed=False, logged_message=log_message)
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| # we don't actually change anything, so we're not changed - writing a log message | ||
| # is not considered a change | ||
| # also, we don't want to report changed every time the role runs | ||
| module.exit_json(changed=False) |
There was a problem hiding this comment.
suggestion: Include the logged message in the module result to aid debugging and play recap visibility.
In the non-check path, the module currently calls exit_json(changed=False) with no extra data. Consider adding the final log_message (or at least sr_message) to the result, e.g. module.exit_json(changed=False, logged_message=log_message), so callbacks and play recap can show what was logged without requiring syslog access.
| module.exit_json(changed=False) | |
| module.exit_json(changed=False, logged_message=log_message) |
2 participants
Feature: Add role fingerprint to system journal as the first task in the role.
Reason: The journal is often exported to reporting systems making it easy to
tell if the role is being used, and when.
Result: Users can get information in their reporting and log aggregation systems
of role usage.
Summary by Sourcery
Add a custom fingerprinting mechanism that logs role begin and success markers to the system journal for the storage system role.
New Features:
Tests: