Bug 285053 - sysutils/py-salt: "service salt_minion (status|stop)" does not work if py-setproctitle is installed
Summary: sysutils/py-salt: "service salt_minion (status|stop)" does not work if py-set...
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: Kirill Ponomarev
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2025-02-26 16:11 UTC by Alan Somers
Modified: 2025-03-18 14:45 UTC (History)
1 user (show)

See Also:


Attachments
Fix the bug by never setting process title (1.74 KB, patch)
2025-02-27 17:57 UTC, Alan Somers
no flags Details | Diff
Fix the bug by always setting process title (3.12 KB, patch)
2025-02-27 17:59 UTC, Alan Somers
no flags Details | Diff
Fix the bug by always setting process title (3.22 KB, patch)
2025-02-27 20:51 UTC, Alan Somers
asomers: maintainer-approval?
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alan Somers freebsd_committer freebsd_triage 2025-02-26 16:11:33 UTC
Commands like "service salt_minion status" always return "salt_minion not running? (check /var/run/salt-minion.pid)", even after starting the minion.  That seems to be because the process name is unexpected.  The minion's process name is now "python3.11:  MultiMinionProcessManager MinionProcessManager (python3.11)"

In version 3006.9_2,1 , by contrast, the service command worked, and the minion's process name was "/usr/local/bin/python3.11 /usr/local/bin/salt-minion -c /usr/local/etc/salt --pid-file=/var/run/salt-minion.pid -d"
Comment 1 Alan Somers freebsd_committer freebsd_triage 2025-02-26 21:36:22 UTC
UPDATE: it's not a version issue

The key difference between the servers where the minion's process is named /usr/local/bin/salt-minion vs those where it's named "MultiMinionProcessManager MinionProcessManager" is whether the py311-setproctitle port is installed.  That port is not listed as a dependency of this one.  But if it is present, then the minion will set its process title.

So either we need to make this a mandatory depedency, so the process title will be predictable, or else modify the RC script so it won't care about the process title.

https://github.com/saltstack/salt/blob/0ad9b3beee323019044b4044715c0be5090b1011/salt/utils/process.py#L43
Comment 2 Kirill Ponomarev freebsd_committer freebsd_triage 2025-02-27 15:11:34 UTC
I lean more towards making py311-setproctitle a dependency. I think it makes sense to make it a mandatory dependency for the package.
Comment 3 commit-hook freebsd_committer freebsd_triage 2025-02-27 15:32:07 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/ports/commit/?id=c639efd990267a3970299c0c5b2444d46a4bd29c

commit c639efd990267a3970299c0c5b2444d46a4bd29c
Author:     Kirill Ponomarev <krion@FreeBSD.org>
AuthorDate: 2025-02-27 15:30:42 +0000
Commit:     Kirill Ponomarev <krion@FreeBSD.org>
CommitDate: 2025-02-27 15:30:42 +0000

    sysutils/py-salt: Add py311-setproctitle as mandatory dependency

    The salt-minion process sets its process title differently when
    py311-setproctitle is installed, changing from "/usr/local/bin/salt-minion"
    to "python3.11: MultiMinionProcessManager MinionProcessManager". This affects
    the RC script's ability to properly check process status and stop the service.

    Making py311-setproctitle a mandatory dependency ensures consistent process
    naming behavior, fixing the "service salt_minion status" and
    "service salt_minion stop" commands.

    PR:             285053
    Reported by:    asomers

 sysutils/py-salt/Makefile | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
Comment 4 Alan Somers freebsd_committer freebsd_triage 2025-02-27 15:35:57 UTC
Did you test that change, Kirill?  Without also changing the rc script, I think it will now always fail, rather than sometimes fail and sometimes succeed.
Comment 5 Alan Somers freebsd_committer freebsd_triage 2025-02-27 17:43:00 UTC
Reopening the bug, because the commit did not fix the issue.
Comment 6 Alan Somers freebsd_committer freebsd_triage 2025-02-27 17:57:06 UTC
Created attachment 258039 [details]
Fix the bug by never setting process title

This patch fixes the bug by never setting the process title, even if py-setproctitle is installed.  It's the simplest solution, works for both minion and master, and causes no warnings to be emitted.  The only downside is that it's farther from what upstream intends.  Perhaps that will lead to other bugs, but I doubt it.
Comment 7 Alan Somers freebsd_committer freebsd_triage 2025-02-27 17:59:47 UTC
Created attachment 258040 [details]
Fix the bug by always setting process title

This alternative patch fixes the bug by always setting the process title.  It adds py-setproctitle as a dependency, fixes the process title by removing an extraneous space, and adjust the RC script accordingly.  The downsides are that:
* It requires an extra package to be installed
* It doesn't fix the salt_master, which I lack the ability to test
* It causes an annoying but harmless warning to be printed to the terminal:
"/usr/local/etc/rc.d/salt_minion: WARNING: cannot read shebang line from MultiMinionProcessManager"

The advantage of this patch, compared to the other, is that it's closer to what upstream intends.

Kirill, which option do you prefer?
Comment 8 Kirill Ponomarev freebsd_committer freebsd_triage 2025-02-27 20:18:11 UTC
I didn't test the change thoroughly enough. I was relying on your expertise in this matter.
I don't have the ability to test on master right now, so I think it would be more correct to use your latest patch, which is closer to what upstream is suggesting.
Comment 9 Alan Somers freebsd_committer freebsd_triage 2025-02-27 20:51:52 UTC
Created attachment 258046 [details]
Fix the bug by always setting process title

This fixes a bug in the first patch: it would remove spaces between individual words of a multiword process title.
Comment 10 Kirill Ponomarev freebsd_committer freebsd_triage 2025-02-27 20:53:17 UTC
Thanks! Go ahead and commit it if you'd like to.
Comment 11 Alan Somers freebsd_committer freebsd_triage 2025-02-27 20:55:52 UTC
Ok!  Pending a poudriere run, I'll commit it.
Comment 12 commit-hook freebsd_committer freebsd_triage 2025-02-27 21:14:55 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/ports/commit/?id=ce4c8c49210b7a646d63b3956ee5e71b74dbf902

commit ce4c8c49210b7a646d63b3956ee5e71b74dbf902
Author:     Alan Somers <asomers@FreeBSD.org>
AuthorDate: 2025-02-27 17:40:20 +0000
Commit:     Alan Somers <asomers@FreeBSD.org>
CommitDate: 2025-02-27 21:04:59 +0000

    sysutils/py-salt: fix commands like "service salt_minion status"

    When py-processtitle is installed, the master and minion daemons will
    change their process titles in a way that breaks commands like "service
    salt_minion status".  status, stop, restart, etc are all broken.  Fix
    this bug by:

    * Always installing py-processtitle unconditionally,
    * Fixing utils/process.py to not insert an extra space in the title
    * Adjusting the RC scripts accordingly

    A downside is that many service subcommands will now print this harmless
    warning message:
    ```
    /usr/local/etc/rc.d/salt_minion: WARNING: cannot read shebang line from
    MultiMinionProcessManager
    ```

    Approved by:    krion (maintainer)
    Sponsored by:   ConnectWise
    PR:             285053

 sysutils/py-salt/Makefile                                |  2 +-
 sysutils/py-salt/files/patch-salt_utils_process.py (new) | 14 ++++++++++++++
 sysutils/py-salt/files/salt_master.in                    |  1 +
 sysutils/py-salt/files/salt_minion.in                    |  1 +
 4 files changed, 17 insertions(+), 1 deletion(-)
Comment 13 Ofloo 2025-03-10 11:16:27 UTC
even with all the patches applied the issue still stands, .. 

Also I noticed that currently with the latest version the rc scripts have been adjusted however process.py part isn't. At least not according to the provided patches.

# pkg version | grep -i salt
py311-salt-3006.9_4,1              =
Comment 14 Alan Somers freebsd_committer freebsd_triage 2025-03-10 12:59:56 UTC
(In reply to Ofloo from comment #13)
Ofloo the two patches are meant to be either/or.  You should not apply both.  Does the the problem still exist for you if you only apply one?  Or for that matter, does the problem still exist for you if you use the lastest py-salt in ports, which includes one of these patches?
Comment 15 Ofloo 2025-03-18 14:08:01 UTC
# ps aux | grep -i python
root  92631   0.0  0.1 12808  2396  0  S+   15:06       0:00.00 grep -i python
# pkg version | grep -i salt
py311-salt-3006.9_5,1              =
# service salt_minion start
/usr/local/etc/rc.d/salt_minion: WARNING: cannot read shebang line from MultiMinionProcessManager
Starting salt_minion.
#
Comment 16 Ofloo 2025-03-18 14:11:21 UTC
And here no changes have been made this is just a random client with the latest version. 

And this dependency is missing, filled up the harddrive (logfile) for all my freebsd clients that have salt minion installed.

py311-paramiko
Comment 17 Alan Somers freebsd_committer freebsd_triage 2025-03-18 14:45:04 UTC
Ofloo , what your screenshot showed is that the service command _did_ work.  However, it printed a harmless error message as noted in the commit message.