Small suggestion for proc name

[realsimix]

Hi Maxim,

I can confirm that #30 and #31 are fine now, thanks for that.

Now that we can dynamically reload using SIGHUP, I just had a small idea: why not show some useful runtime information in the proc name instead of only 'child'?

That way one should be able to see which versions are actually running by the child. At least that's how I understood it, when you SIGHUP the processes re-execute with new config and probably new modules so they should show it at runtime, right?

This patch is what I tried here: spampd-2.60-procname.patch.txt

Regards,
Simon

[mpaperno]

Hi Simon,

I hadn't considered the proc name as a conveyor of status information, but if it doesn't cause issues with anything else, I don't see why not.

OTOH I don't know what is considered "acceptable" on all possible platforms. I did some searching and it seems like anything allowed in a file name would be acceptable... but this is a vague determination so far. Looking at a random proc list on a box, most of them seem to keep the name simple (but I do see at least one exception). Do you have any guidance on the subject?

For example in your patch/idea (which I otherwise like), I wonder about some of the "special" chars like []:, (colon from net_server_type())... and there could in theory be some special chars in version names (eg. an SA "dev" build may have extra revision data). And possibly the length, though that can be shortened somewhat.

It does seem to work in my limited testing (on a Debian-based Linux), I just have no idea how others may react. In fact sounds like it may be a god idea to wrap the actual $0 = ... assignment in an eval{} block in any case.

I've found only limited/useless information so far:
SO: What is the maximum allowed limit on the length of a process name? - Doesn't seem to apply here?
Oracle Cloud: Naming Conventions for Processes - Possibly totally irrelevant to us.
SO: What characters should be restricted from a Unix file name? - Anything goes!?

Thanks,
-Max

[realsimix]

Hi Maxim,

I'm not sure about the limitations. My idea was inspired by Zabbix which makes use of this feature on a different operating systems. On our Zabbix server it shows:

 8136 ?        S      0:00 zabbix_server
 8150 ?        S      1:58  \_ zabbix_server: configuration syncer [synced configuration in 0.754875 sec, idle 60 sec]
 8154 ?        S      0:04  \_ zabbix_server: housekeeper [deleted 271629 hist/trends, 0 items/triggers, 15 events, 2 sessions, 0 alarms, 3 audit 
 8155 ?        S      0:00  \_ zabbix_server: timer #1 [updated 0 hosts, suppressed 0 events in 0.000278 sec, idle 59 sec]
 8156 ?        S     34:44  \_ zabbix_server: http poller #1 [got 0 values in 0.000327 sec, idle 5 sec]
 8157 ?        S      0:00  \_ zabbix_server: discoverer #1 [processed 0 rules in 0.003025 sec, idle 60 sec]
 8158 ?        S      0:24  \_ zabbix_server: history syncer #1 [processed 66 values, 14 triggers in 0.003704 sec, idle 1 sec]
 8159 ?        S      0:25  \_ zabbix_server: history syncer #2 [processed 0 values, 0 triggers in 0.000038 sec, idle 1 sec]
 8161 ?        S      0:24  \_ zabbix_server: history syncer #3 [processed 0 values, 0 triggers in 0.000030 sec, idle 1 sec]
 8162 ?        S      0:24  \_ zabbix_server: history syncer #4 [processed 0 values, 0 triggers in 0.000030 sec, idle 1 sec]
 8163 ?        S      0:00  \_ zabbix_server: escalator #1 [processed 0 escalations in 0.000783 sec, idle 3 sec]
 ...

Some info they have here https://support.zabbix.com/browse/ZBXNEXT-1855

I think it can be done on most systems but it may also need support for it in with Perl on the platform. Do all the Perls have the ability to set the proctitle and how to handle the case where it doesn't work?

If in doubt I could also keep it as a patch in my RPMs where I know that it works on the target platform.

Apart from that, I've missed one of the important info which is the UPDATE version of the SA rules.

Do you know how to read the UPDATE version? I see that there is a macro which can be used in rules but how can we use it in SpamPD:

 _RULESVERSION_    comma-separated list of rules versions, retrieved from
                   an '# UPDATE version' comment in rules files; if there is
                   more than one set of rules (update channels) the order
                   is unspecified (currently sorted by names of files);

That way one could see if SpamPD really runs with the latest updates.

Thanks,
Simon

[mpaperno]

Hi Simon,

Well... that was a bit of a rabbit hole... :) I'd love your feedback in PR #33 .

Not only does a HUP reload new configs and any updated module(s) but one can even update SpamPD code itself on the fly. Pretty neat, didn't really realize that before.

Great suggestion on "RULESVERSION," I didn't know about that. Found a way to get ahold of it at startup, though it's a bit "unconventional" I suppose. Don't see much of an impact on startup time. I added it to the logging as well as the proc name.

I shortened "SpamAssassin" to "SA" in the proc name... Maybe "Net::Server::PreFork[Simple]" could also be shortened... or maybe they both should be spelled out... on the fence there (and maybe doesn't matter).

The Zabbix stuff is interesting... actually stats came to mind right away when you brought up the child name thing. Though currently the only really interesting stat SpamPD tracks is number of processed messages per child. Which could be useful? It would have to update the child name after every message of course, so that would need to be optimized a bit.

Thanks,
-Max

[realsimix]

Hi Max, Just some quick thoughts, real tests may have to wait for tomorrow.

Hi Simon, Well... that was a bit of a rabbit hole... :) I'd love your feedback in PR #33 . Not only does a HUP reload new configs and any updated module(s) but one can even update SpamPD code itself on the fly. Pretty neat, didn't really realize that before.

And that's where a config file makes more sense than options on the command line.

Great suggestion on "RULESVERSION," I didn't know about that. Found a way to get ahold of it at startup, though it's a bit "unconventional" I suppose. Don't see much of an impact on startup time. I added it to the logging as well as the proc name.

Thanks, that's really nice.

I shortened "SpamAssassin" to "SA" in the proc name... Maybe "Net::Server::PreFork[Simple]" could also be shortened... or maybe they both should be spelled out... on the fence there (and maybe doesn't matter).

If we always use the same module PreFork, then why not shortening it. I wasn't sure we never change it.

The Zabbix stuff is interesting... actually stats came to mind right away when you brought up the child name thing. Though currently the only really interesting stat SpamPD tracks is number of processed messages per child. Which could be useful? It would have to update the child name after every message of course, so that would need to be optimized a bit.

As you say, I thought it would be interesting to see how many requests a child has already served and the maximum of requests it will do, like 34/100. One thing that also came to my mind is if it was possible to add a custom header to show the engine which processed the message, like X-Spam-Filter: SpamPD 2.60/3.4.6 Maybe even as a config option so that we can define how much info we'd like to reveal in the message. What do you think? Thanks, Simon

[realsimix]

Of course it would be nice to inject the X-Spam-Filter: header before SA adds the other X-Spam-*: headers.

[mpaperno]

If we always use the same module PreFork, then why not shortening it. I wasn't sure we never change it.

It can now be PreFork (which is new) or PreForkSimple (default). Which could be useful to know I suppose. But we could easily just grab the last part of the module name and/or shorten the whole thing to just the upper case letter (for example).

As you say, I thought it would be interesting to see how many requests a child has already served and the maximum of requests it will do, like 34/100.

That's what I was thinking. I guess the question is if it's worth the little bit of extra overhead. Though with my other changes the net result should still be quite positive.

One thing that also came to my mind is if it was possible to add a custom header to show the engine which processed the message, like X-Spam-Filter: SpamPD 2.60/3.4.6 Maybe even as a config option so that we can define how much info we'd like to reveal in the message.

Of course it would be nice to inject the X-Spam-Filter: header before SA adds the other X-Spam-*: headers.

My practice so far has been to let the user set up their injected headers using SA configs, to keep that all centralized (with the exception of X-Envelope-To/From of course since that's transport level). If we could provide the SpamPD version number as a header macro for SA, would that satisfy those requirements/ideas?

I think there's a way to do that as an SA "plugin" but I'm not really sure what's involved (I'll check). Barring that, we could of course make an option to do it in SPD itself. My initial thought on that would be maybe some kind of template the user can provide which includes the actual header name as well (maybe "X-Spam-Filter" is already used, for example).

eg. --add-header "X-Spam-Filter: SpamPD $spdVersion/SA $saVersion/rules $rulesVersion"

[mpaperno]

If we could provide the SpamPD version number as a header macro for SA, would that satisfy those requirements/ideas?

OK well that's done, anyway. Very simple, at least once I figured out there couldn't be any separators in the macro name. c5ad090 (and added to the PR).

Another 👍🏼 for the idea, thanks!

[realsimix]

Hi Maxim,

> If we always use the same module PreFork, then why not shortening it. I > wasn't sure we never change it. It can now be PreFork (which is new) or PreForkSimple (default). Which could be useful to know I suppose. But we could easily just grab the last part of the module name and/or shorten the whole thing to just the upper case letter (for example).

That's exactly what I thought and I don't really care about long proc lines. On our large systems I see lots of processes with command lines of hundreds of chars.

> As you say, I thought it would be interesting to see how many requests a > child has already served and the maximum of requests it will do, like > 34/100. That's what I was thinking. I guess the question is if it's worth the little bit of extra overhead. Though with my other changes the net result should still be quite positive.

I had the impression that SA takes so much time to process a message that such things like setproctitle won't really matter :) One more idea, do we have the absolute number of children started by the parent? That could also be interesting to know in case of debugging leaks. Thanks, Simon

[realsimix]

I've just tested with PR #33 and it works nicely with one small issue:

On a newly installed system, before running sa-update, the rules version reported is empty, like

spampd: child v2.61 [Perl 5.16.3, Net::Server::PreForkSimple 2.007, SA 3.4.6, rules v]

maybe in such case it would be better to show [unknown] or something.

BTW, just in case it's possible to add the child number and request numbers, may I suggest something like this:

spampd: child v2.61 #1234 [request 43/100, Perl 5.16.3, N::S::PreForkSimple 2.007, SA 3.4.6, rules 1891912]

(remove the v after rules?)
What do you think?

[realsimix]

Attached patch fixes the unknown rules version for me by showing <unknown> instead of a version number.
The problem here is that a SA install usually has a base version installed but, this version is only known at install time. At least for Red Hat based distributions, the version of the rules tarball can not be found on the installed system and therefore <unknown> seems justified.

rules.patch.txt

[mpaperno]

Thanks Simon,

I'm working on a way to have the child name be configurable via a template. Something like this:

child_name_templ  => '$base_name: child [requests $req_count/$req_total] [v$spampd_ver, SA $sa_ver (rules $sa_rls_ver), NS$ns_typ_acr $ns_ver, Perl $perl_ver]',  # default child name template

Seems most flexible?

One more idea, do we have the absolute number of children started by the parent?
...
BTW, just in case it's possible to add the child number

Do you mean a sequence number, like the first launched child will be #1 and incremented for each subsequent one? I don't see an existing counter for this, but we could add one (using the child creation hook). Or something else?

Net::Server doesn't seems to keep any other stats. One thing we could show is if the child is currently connected/working. One other one I thought may be interesting to track would be average time to process a message (we already know the time for each message). Though this may be most interesting for all the children as a whole, so I'm not sure how useful a number that would be to see in a child proc name. OTOH there's no other easy way to get this stat right now.

BTW does that header solution work for you? I mean using SA add_header to add SpamPD version/etc.

-M

[mpaperno]

PS. I meant we could keep an avg. processing time per child and/or overall across all children. Of course for the overall counter then we get into if it's a average over all time since started, or a moving window, or.... but at least some indicator seems potentially useful.

[realsimix]

Hi Maxim,

Thanks Simon, I'm working on a way to have the child name be configurable via a template. Something like this: ```perl child_name_templ => '$base_name: child [requests $req_count/$req_total] [v$spampd_ver, SA $sa_ver (rules $sa_rls_ver), NS$ns_typ_acr $ns_ver, Perl $perl_ver]', # default child name template ``` Seems most flexible?

Sure, that would be great, I like the idea.

> One more idea, do we have the absolute number of children started by the > parent? ... > BTW, just in case it's possible to add the child number Do you mean a sequence number, like the first launched child will be `#1` and incremented for each subsequent one? I don't see an existing counter for this, but we could add one (using the child creation hook). Or something else?

Exactly, that's what I had in mind. Since we can now reconfigure dynamically with SIGHUP, I expect long running instances because we don't have to restart every few days for rules update. So it's interesting to see how many children SpamPD has already launched in total and also to see how many requests each child has processed. Usually such info can only be found in logfiles and it's difficult to filter out the interesting values. Using the procnames to show these stats helps to get a quick overview of what's going on.

Net::Server doesn't seems to keep any other stats. One thing we could show is if the child is currently connected/working. One other one I

Oh yes, a child status would be nice.

thought may be interesting to track would be average time to process a message (we already know the time for each message). Though this may be most interesting for all the children as a whole, so I'm not sure how useful a number that would be to see in a child proc name. OTOH there's no other easy way to get this stat right now.

Maybe always show two values: the avg. at the time when the child was started and the time the last message took?

BTW does that header solution work for you? I mean using SA `add_header` to add SpamPD version/etc.

I've added the config to our production host which is not running 2.6x yet. The header is being added fine but of course the _SPAMPDVERSION_ does not show up yet. So far it looks good. Thanks, Simon

[realsimix]

Just tested and I can now confirm that add_header all Filter-Version SpamPD _SPAMPDVERSION_ ... works as expected.

[mpaperno]

Another rabbit hole visited... :) Pushed a new commit to PR #33.

You can now format the child name as you see fit (see new POD section for some details). I made the default somewhat of a middle ground with information most likely to change:

spampd: child #3(D) [req 11/30, time lst/avg/ttl 0.023/0.025/0.271, ham/spm 3/8] [SA rules v1891891)]

The "D" is for disconnected. The total child count seems to work, since we can count spawns in the parent. The other counters are only tracked per child.

Unfortunately there's no (simple-ish) way to get data from a child back to the parent process. So there's currently there's no way to track overall stats like time, or

the avg. at the time when the child was started

To overcome this we'd basically need a separate custom communication process between the parent/child. Net::Server actually has a sort-of hook for this, using a UNIX socket which it can open up for you. But the actual protocol is up to the user. Nor does it seem like PreForkSimple flavor is quite set up for this, so it may require a bit of extra finagling (PreFork seems more complete). Something to consider for the future perhaps.

Just tested and I can now confirm that add_header ... works as expected.

Great, thanks. Earlier I was also checking that this met your needs, vs. having SpamPD add the header. Now that we have a template system already, it would be easy to add (including with any of those other statistics I added for child procs). But no need for redundant functionality.

Cheers,
-Max

[mpaperno]

On a newly installed system, before running sa-update, the rules version reported is empty,
maybe in such case it would be better to show [unknown] or something.

That's good to know, and thanks for checking, I wouldn't have thought of it. I made the default be "(unknown)", whether there's been no update like you said or SA version is too old (or the lookup fails for some other reason). I kept the round brackets just because it's consistent with a couple other places in the logging code (hope that's OK).

[mpaperno]

I made the default be "(unknown)"...

And just pushed a fix (hopefully) for that when there hasn't been a rules update yet.

[realsimix]

Seems my older systems do not like some of the newer code style. On RHEL7 I get:

Variable "$hash" will not stay shared at /usr/sbin/spampd line 1480.
Variable "@args" will not stay shared at /usr/sbin/spampd line 1481.
"my sub" not yet implemented at /usr/sbin/spampd line 1487.

On a even older system I get this:

Bareword found where operator expected at /usr/sbin/spampd line 574, near "s/[a-z]//rg"
Possible unintended interpolation of @startup_args in string at /usr/sbin/spampd line 589.
syntax error at /usr/sbin/spampd line 574, near "s/[a-z]//rg"
Global symbol "$self" requires explicit package name at /usr/sbin/spampd line 589.
Global symbol "$self" requires explicit package name at /usr/sbin/spampd line 589.
Global symbol "$self" requires explicit package name at /usr/sbin/spampd line 589.
Global symbol "$self" requires explicit package name at /usr/sbin/spampd line 589.
Global symbol "@startup_args" requires explicit package name at /usr/sbin/spampd line 589.
Global symbol "$spd_p" requires explicit package name at /usr/sbin/spampd line 595.
Global symbol "$spd_p" requires explicit package name at /usr/sbin/spampd line 596.
Global symbol "$spd_p" requires explicit package name at /usr/sbin/spampd line 597.
Global symbol "$spd_p" requires explicit package name at /usr/sbin/spampd line 598.
Global symbol "$self" requires explicit package name at /usr/sbin/spampd line 600.
syntax error at /usr/sbin/spampd line 601, near "}"
/usr/sbin/spampd has too many errors.

Any chance we can get this to work here?

Thanks,
Simon

[mpaperno]

Ah older Perl's again... sorry. Hard to tell which versions implement what (though I should have known on the latter issue).

Both fixed I hope!

Thanks,
-Max

[realsimix]

Thanks, all fixed now and it works great!

There's a ) in the end of procname which should not be there.
I also changed it a bit to make it more compact like [SA 3.4.6/1891912]:

--- /usr/sbin/spampd.orig	2021-08-03 16:45:58.000000000 +0200
+++ /usr/sbin/spampd	2021-08-03 16:57:44.308412598 +0200
@@ -461,7 +461,7 @@
       # default child name template
       child_name_templ  => '%base_name: child #%child_count(%child_status) ' .
                            '[req %req_count/%req_max, time lst/avg/ttl %(req_time_last).3f/%(req_time_avg).3f/%(req_time_ttl).3f, ham/spm %req_ham/%req_spam] ' .
-                           '[SA rules v%sa_rls_ver)]',
+                           '[SA %sa_ver/%sa_rls_ver]',
     },
     # this hash is eventually passed to SpamAssassin->new() so it must use valid SA option names. This also becomes the SA object afterwards.
     assassin => {
@@ -585,7 +585,7 @@
     req_spam      => 0,   # count of spam messages scored by child
   };
 
-  my $template = ' v%spampd_ver [Perl %perl_ver, Net::Server::%ns_typ %ns_ver, SA %sa_ver, rules v%sa_rls_ver] ';
+  my $template = ' v%spampd_ver [Perl %perl_ver, Net::Server::%ns_typ %ns_ver, SA %sa_ver/%sa_rls_ver] ';
   $self->inf(ref($self) . $self->format_stats_string($template) . ($self->is_reloading() ? "reloading": "starting") . " with: @startup_args \n");
 
   # Redirect all errors to logger (must do this after SA is compiled, otherwise for some reason we get strange SA errors if anything actually dies).

Thanks,
Simon

[mpaperno]

Thanks Simon, good eye. I'll get that fixed up along with a few documentation niggles.

[realsimix]

Closed #32 via #33.

Thanks, I'll test tomorrow early morning. Simon

[realsimix]

Hi Maxim,

I just wanted to mention how useful the new procname stuff is where the SA rules version is shown: On a system I found that the running system didn't use the updated rules from last night. In the end I found the culprit in this script https://src.fedoraproject.org/rpms/spamassassin/raw/rawhide/f/sa-update.cronscript
It doesn't restart the services as needed because of a missing $PATH entry when running from crond.

So, the recent changes are worth gold 👍

Thanks,
Simon

[mpaperno]

Hi Simon,

Very cool! And useful indeed, w/out combing through any logs. Thanks for letting me know. And that was some great "small suggestion"s :) you had!

I think 2.61 is ready for a release, but of course let me know if anything comes up at any time (including more good ideas).

Cheers!
-Max

[realsimix]

Hi Maxim,

I agree this code is good for a 2.61 release, congratulations!

In case you want to do a last minute change, maybe this:

--- spampd.service.orig	2021-08-06 16:51:18.232897031 +0200
+++ spampd.service	2021-08-06 16:52:59.963385149 +0200
@@ -1,9 +1,14 @@
 [Unit]
 Description=Spamassassin Proxy Daemon
-After=network.target
+After=syslog.target network.target
+Wants=sa-update.timer
 
 [Service]
 ExecStart=/usr/bin/spampd --port=10025 --relayhost=127.0.0.1:10026 --tagall --log-rules-hit --user spampd --group spampd --pid=/home/spampd/spampd.pid --nodetach
+ExecReload=/usr/bin/kill -SIGHUP $MAINPID
+KillMode=mixed
+KillSignal=SIGQUIT
+TimeoutStopSec=30
 
 [Install]
 WantedBy=multi-user.target

Thanks,
Simon

[mpaperno]

Thanks! And yes I did notice the outdated service example file and replaced it with an updated version from the debian branch (haven't pushed that change yet).

But I was wondering, and perhaps you know... in this block:

[Service]
ExecStart=/usr/sbin/spampd --config /etc/spampd.cfg --pid /var/run/spampd.pid
ExecReload=/bin/kill -HUP $MAINPID
PIDFile=/var/run/spampd.pid
...

Is PIDFile necessary here? Or does/can the service controller track pid for $MAINPID some other way?

[realsimix]

Thanks! And yes I did notice the outdated service example file and replaced it with an updated version from the `debian` branch (haven't pushed that change yet). But I was wondering, and perhaps you know... in this block: ``` [Service] ExecStart=/usr/sbin/spampd --config /etc/spampd.cfg --pid /var/run/spampd.pid ExecReload=/bin/kill -HUP $MAINPID PIDFile=/var/run/spampd.pid ... ``` Is `PIDFile` necessary here? Or does/can the service controller track pid for $MAINPID some other way?

I'm a bit confused here. If the service is Type=forking, then a PIDFile is needed. Here, the type seems to be 'simple' (the default), which means the PIDFile is not needed. But then, Spampd has to be started with --nodetach. To be exact, /usr/bin/kill is the binary to call, /bin is now only a compatibility link to /usr/bin. The same applies to the pid file, it goes into /run and /var/run is only a compat link. The systemd developers like to change things :) In my example systemd will send SIGQUIT only to the master, then wait 30 seconds max. for termination, otherwise terminate master and all children. In the past we had many different init scripts, with systemd we have a lot of different unit files :) Regards, Simon

[mpaperno]

Thanks... yea a bit confused here also. I punted and put "both" (simple and forking) versions in the example.

The systemd developers like to change things :)

Understatement :) I can't keep up!

Well 2.61 is official, FWIW. Thanks again for all your help, it's nice working with you.

Best,
-Max

[realsimix]

Thank you for the new release, it's much appreciated.

Well 2.61 is official, FWIW. Thanks again for all your help, it's nice working with you.

Glad I was able to contribute, it was a pleasure for me as well. Thanks, Simon