PMM-15285 Remove dead supervisord event-scraping code - #5723
Conversation
pmm-managed ran a goroutine that shelled out to `supervisorctl maintail -f`, scanned every line of supervisord's main log against eight regexes and dispatched typed events to subscribers. There were no subscribers: PMM-14132 (#4110) removed `subscribe` and `UpdateRunning`, the only writer to `subs` and the only reader of `lastEvents`, but left the producer side in place. Remove maintail.go and its test, the `Run` loop, the `subs`/`lastEvents`/ `eventsM` fields, the `sub` type and `parseStatus`, plus the call sites in pmm-managed's main and the devcontainer test. This also drops the only place PMM depended on supervisord's log text, which would otherwise have to be ported to any process-manager replacement. Folded in two adjacent cleanups: - Remove [unix_http_server] and [supervisorctl] from the generated pmm.ini (existing TODO). Both duplicate /etc/supervisord.conf; the removed `chmod = 0700` is supervisord's default, verified identical (0700 pmm:root) on a running container built with this change. - Pass the program name to supervisorctl as its own argv element in the encryption rotation helpers. This is not a live bug — supervisorctl rejoins its argv, so the single-string form works today — but it only works by accident of that quirk and breaks under any other CLI.
WalkthroughSupervisord event monitoring and status parsing were removed. Control configuration was simplified. ChangesSupervisord simplification
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5723 +/- ##
==========================================
+ Coverage 43.59% 45.35% +1.76%
==========================================
Files 415 417 +2
Lines 43134 43253 +119
==========================================
+ Hits 18804 19618 +814
+ Misses 22454 21698 -756
- Partials 1876 1937 +61
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
|
||
| // TODO: remove [unix_http_server] and [supervisorctl] as they duplicate supervisord.conf. | ||
| var pmmTemplate = template.Must(template.New("").Option("missingkey=error").Parse(`[unix_http_server] | ||
| chmod = 0700 |
There was a problem hiding this comment.
2 issues:
- these were duplicating the default values of
/etc/supervisord.conf. pmm.confis a wrong place to configure these values.
|
Updated 2026-08-02 — supersedes the original table below, which was measured before Compared stock
No observable difference. The credentials moved out of the generated per-boot pmm.ini into the static Probe method: |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@build/ansible/roles/supervisord/tasks/main.yml`:
- Around line 29-42: Remove the hard-coded username and password from the two
“Modify supervisord.conf” ini_file tasks. Use the repository’s approved
protected configuration mechanism for Supervisord credentials, or remove the
unix_http_server authentication options when socket permissions are the intended
access boundary.
- Around line 29-42: Remove the duplicate Supervisord control configuration
sections from pmm.ini, including the unix_http_server and supervisorctl
settings; eliminate the corresponding Modify supervisord.conf tasks rather than
maintaining overlapping entries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 306077f8-148d-4acf-890c-c98d35cdb5ad
📒 Files selected for processing (1)
build/ansible/roles/supervisord/tasks/main.yml
There was a problem hiding this comment.
♻️ Duplicate comments (1)
build/ansible/roles/supervisord/tasks/main.yml (1)
29-42: 🔒 Security & Privacy | 🟠 MajorRemove the hard-coded Supervisord credentials.
These tasks write
dummyas both theunix_http_serverusername and password. The generated configuration is world-readable, so these values do not provide a meaningful access boundary. Remove these tasks, or load credentials from the approved protected configuration mechanism if authentication is required.As per coding guidelines, files under
build/**/*must not add secrets or credentials to build scripts or Dockerfiles.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@build/ansible/roles/supervisord/tasks/main.yml` around lines 29 - 42, Remove the two ini_file tasks that set hard-coded username and password values under the unix_http_server section; if authentication is required, source both credentials from the approved protected configuration mechanism instead, without adding secrets to build configuration.Source: Coding guidelines
🧹 Nitpick comments (1)
managed/services/encryption/encryption_rotation.go (1)
74-74: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftPropagate cancellation context to supervisor commands.
Replace
signal.Ignorewithsignal.NotifyContext, pass the context through the rotation helpers, useexec.CommandContext, and make retry waits context-aware. Remove the inline//nolint:noctxsuppressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@managed/services/encryption/encryption_rotation.go` at line 74, Update the encryption rotation flow to use signal.NotifyContext instead of signal.Ignore, propagate the resulting context through the rotation helper methods, and replace supervisor command execution with exec.CommandContext. Make retry waits honor context cancellation, remove the inline noctx suppressions, and preserve existing rotation behavior when the context remains active.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@build/ansible/roles/supervisord/tasks/main.yml`:
- Around line 29-42: Remove the two ini_file tasks that set hard-coded username
and password values under the unix_http_server section; if authentication is
required, source both credentials from the approved protected configuration
mechanism instead, without adding secrets to build configuration.
---
Nitpick comments:
In `@managed/services/encryption/encryption_rotation.go`:
- Line 74: Update the encryption rotation flow to use signal.NotifyContext
instead of signal.Ignore, propagate the resulting context through the rotation
helper methods, and replace supervisor command execution with
exec.CommandContext. Make retry waits honor context cancellation, remove the
inline noctx suppressions, and preserve existing rotation behavior when the
context remains active.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cbfc99ed-a20d-4d2d-83a2-4b422fae7767
📒 Files selected for processing (12)
build/ansible/roles/supervisord/tasks/main.ymlmanaged/cmd/pmm-managed/main.gomanaged/services/encryption/encryption_rotation.gomanaged/services/supervisord/devcontainer_test.gomanaged/services/supervisord/maintail.gomanaged/services/supervisord/maintail_test.gomanaged/services/supervisord/pmm_config.gomanaged/services/supervisord/supervisord.gomanaged/services/supervisord/supervisord_test.gomanaged/testdata/supervisord.d/pmm-ch_low_memory.inimanaged/testdata/supervisord.d/pmm-db_disabled.inimanaged/testdata/supervisord.d/pmm-db_enabled.ini
💤 Files with no reviewable changes (9)
- managed/testdata/supervisord.d/pmm-ch_low_memory.ini
- managed/services/supervisord/supervisord_test.go
- managed/services/supervisord/maintail.go
- managed/services/supervisord/devcontainer_test.go
- managed/testdata/supervisord.d/pmm-db_enabled.ini
- managed/cmd/pmm-managed/main.go
- managed/testdata/supervisord.d/pmm-db_disabled.ini
- managed/services/supervisord/maintail_test.go
- managed/services/supervisord/supervisord.go
3 participants
Ticket number: PMM-15285
Feature build: Percona-Lab/pmm-submodules#4503
What
pmm-managed ran a goroutine that shelled out to
supervisorctl maintail -f, scanned every line of supervisord's main log against eight regexes, and dispatched typed events to subscribers.There are no subscribers. PMM-14132 (#4110) removed
subscribeandUpdateRunning— the only writer tosubsand the only reader oflastEvents— but left the entire producer side in place.parseStatusis likewise referenced only by its own test.This removes
managed/services/supervisord/maintail.goand its test, theRunloop, thesubs/lastEvents/eventsMfields, thesubtype andparseStatus, plus the call sites inmanaged/cmd/pmm-managed/main.goandmanaged/services/supervisord/devcontainer_test.go.Beyond deleting a permanently running subprocess that feeds nothing, this drops the only place PMM depended on supervisord's log text — which would otherwise have to be ported to any process-manager replacement.
Folded-in cleanups
1. The pmm.ini TODO at
managed/services/supervisord/pmm_config.go:88—[unix_http_server]and[supervisorctl]are removed from the generated pmm.ini, which is what the TODO asked for.The
[supervisorctl]section was a pure duplicate of/etc/supervisord.conf. The[unix_http_server]section was not quite: besideschmod = 0700(which is supervisord's own default) it carried the dummy credentials that enable server-side HTTP Basic auth on the control socket. Removing it alone would therefore have disabled that auth.So the credentials are moved rather than dropped:
build/ansible/roles/supervisord/tasks/main.ymlnow sets them on[unix_http_server]in/etc/supervisord.conf, alongside the[supervisorctl]pair already set there. Net effect is that both halves of the auth config live in one static file instead of being re-emitted into a generated per-boot file, with no behaviour change. Verified — see the comparison comment below.2. supervisorctl argv in the encryption rotation helpers — the program name is now its own argv element.
This is not a bug fix. Verified against supervisorctl 4.2.4 in a running
percona/pmm-server:3.9.0: the single-string form works, because supervisorctl rejoins its argv and re-parses through Python'scmdmodule.supervisorctl "stop pmm-managed"exits 0 and does stop the program. It works only by accident of that quirk and breaks under any CLI that parses argv normally, so it is a latent portability hazard, not a defect.Verification
Feature build
perconalab/pmm-server-fb:PR-4503-3385efbcompared against stockpercona/pmm-server:3.9.0on a running container — identical on every probe: control-socket auth (401 without credentials, 200 with, 401 with wrong), socket mode700 pmm:root, non-owner UID unable to connect, 10 programs RUNNING plus pmm-init EXITED, andreadyz200. Details in the comment below.Also:
pmm-managed.log, no supervisord errors or warnings.go build ./...andgo vetclean.--new-from-rev(what CI enforces): 0 issues. Touching the threeexec.Commandlines made them count as new, hence//nolint:noctx, matching the existing directive on the same call inmanaged/services/supervisord/supervisord.go. Threading a real context would have changed the exportedRotateEncryptionKeysignature — out of scope here.TestDevContainerandTestEncryptionRotationcannot run outside the dev container and fail identically onmain(they needsupervisorctlon PATH, and Postgres plus a live supervisord, respectively).Net: 370 deletions, 4 insertions across 11 files in the Go change, plus 14 lines in the ansible role.