Skip to content

fix: avoid double restart of pmlogger on config changes - #92

Merged
richm merged 1 commit into
performancecopilot:mainfrom
kurik:logger-restart
Jun 9, 2026
Merged

fix: avoid double restart of pmlogger on config changes#92
richm merged 1 commit into
performancecopilot:mainfrom
kurik:logger-restart

Conversation

@kurik

@kurik kurik commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

The pmlogger timers and control.d tasks notified a handler to restart pmlogger, but those same changes already set __pcp_restart_pmlogger and triggered a restart in the main service task. On first playbook run this could restart pmlogger twice in quick succession, causing systemd to fail the second start on Type=notify services.
Drop the redundant handler notifications and the Restart pmlogger handler; rely on the existing restart task, consistent with pmie.

Resolves: RHEL-182971

The pmlogger timers and control.d tasks notified a handler to restart
pmlogger, but those same changes already set __pcp_restart_pmlogger and
triggered a restart in the main service task. On first playbook run this
could restart pmlogger twice in quick succession, causing systemd to fail
the second start on Type=notify services.
Drop the redundant handler notifications and the Restart pmlogger handler;
rely on the existing restart task, consistent with pmie.
@coderabbitai

coderabbitai Bot commented Jun 9, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

This change removes the pmlogger handler and consolidates restart logic into the task file. The handler removal eliminates the separate notification target, while task refactoring introduces targeted-host and single-control templating with conditional gates and a computed restart flag that unifies restart decisions based on detected changes.

Changes

pmlogger Restart Logic Refactoring

Layer / File(s) Summary
Handler removal for pmlogger
roles/pcp/handlers/main.yml
Deletes the Restart pmlogger handler task, leaving only handlers for pmcd, pmie, and pmproxy.
Task restart logic consolidation and config templating
roles/pcp/tasks/pmlogger.yml
Removes direct restart notification from the retention/timers template task. Adds targeted-host templating for control.d layout per host and single-control templating task, each with conditional guards. Centralizes restart behavior by computing __pcp_restart_pmlogger from multiple change signals and choosing start vs restart based on boot status and that flag.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 Handlers fade, tasks take the wheel,
Pmlogger restarts now feel more real,
Control files dance in loops so neat,
Restart logic, centralized, complete! 🎯

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: avoiding double restart of pmlogger by removing redundant handler notifications.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed The PR description clearly explains the issue, the fix, and includes a reference to the related Jira ticket, though it deviates from the template structure.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@kurik
kurik requested a review from richm June 9, 2026 05:28
@kurik

kurik commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

@richm may I ask you for review please? This is sort of a race condition I randomly observe during down stream testing on RHEL.

@richm
richm merged commit ae9851d into performancecopilot:main Jun 9, 2026
6 of 8 checks passed
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