Lewis/improve postfix check #665

ls339 · 2017-08-08T19:59:54Z

What does this PR do?

Will add an option to run the check using postqueue -p to gather message counts for hold, active, and deferred mail queues.

Motivation

The postqueue binary is ran with set-group ID privileges, so that it can connect to Postfix daemon processes without the use of sudo.

Testing Guidelines

Tested on a postfix 2.11.1.

Versioning

[] Bumped the version check in manifest.json
Updated CHANGELOG.md

Additional Notes

The caveat is that postqueue will gather message counts for only the hold, active, and deferred queues.

bits-bot · 2017-08-08T19:59:56Z

@ls339, thanks for your PR! By analyzing the history of the files in this pull request, we identified @masci, @gmmeyer and @truthbk to be potential reviewers.

…grations-core into lewis/improve_postfix_check pull in changes from master

truthbk

@ls339 looks pretty good, and dropping the sudo dependency would be sweet. You might need to add some more instructions though :)

truthbk · 2017-08-11T13:41:17Z

postfix/CHANGELOG.md

@@ -5,6 +5,14 @@

 ### Changes

+* [IMPROVEMENT] option to run the check using postqueue -p


If 1.1.0 is unreleased, then I guess this should all be below? Looks like we have two 1.1.0 entries in the changelog.

👍 Gotcha, moved my change under 1.1.0

truthbk · 2017-08-11T13:42:38Z

postfix/check.py

@@ -24,15 +24,24 @@ class PostfixCheck(AgentCheck):
    YAML config options:
        "directory" - the value of 'postconf -h queue_directory'
        "queues" - the postfix mail queues you would like to get message count totals for
+
+    Optionally we can run the check to use `postqueue -p` which is ran with set-group ID privileges,


Can we grab all metrics doing this? Do you have a link to the doc detailing this behavior? It would be nice to have here + understand what our continued dependency on sudo may be.

Also, please note that on a production server, it's unlikely that postqueue would have access to all queues by all users. Configuration would typically be set in the configuration files:

Postfix doesn't use unix permission feature to limit access to postqueue. Instead it uses parameters like ' authorized_mailq_users' in its configuration files.

We'd have to explain how to do this in our docs/config.

Added some documentation in the code. Will also add this to the config.

truthbk · 2017-08-11T13:55:24Z

postfix/check.py

+        hold_count = 0
+        deferred_count = 0
+
+        for line in output.splitlines():


Could you add a small comment here showing some sample output from postqueue -p

truthbk · 2017-08-11T13:57:23Z

postfix/check.py

+            if line[0:1].isdigit():
+                deferred_count += 1
+        self.log.debug('Postfix Version: %s' % postfix_version)
+        self.log.debug('active_count: %d' % active_count)


Not sure we need to log active_count, hold_count and deferred_count to be honest - we can see those values on the UI. Postfix version is 👍

truthbk

I actually approved accidentally 😊

add a sample postqueue -p output to code.

irabinovitch · 2017-08-16T04:11:22Z

This seems like a more sane default than find. Thoughts on making it the normal behavior rather than an opt in? I understand we lose incoming here, but that seems like a reasonable trade off for sudo privs.

irabinovitch · 2017-08-16T04:15:55Z

postfix/check.py

+    Optionally we can run the check to use "postqueue -p" which is ran with set-group ID privileges
+    so that the agent user can connect to Postfix daemon processes without sudo.
+
+    Monitoring the mail queue using "postqueu" will only monitor the following queues.


missing an e here? "postqueue"

irabinovitch · 2017-08-16T04:16:08Z

postfix/check.py

+    - hold
+    - active
+    - deferred
+    Unlike using the "sudo" method, the "incoming" queue will not be moitored. Postqueue does


"monitored"

irabinovitch · 2017-08-16T04:23:06Z

Dont forget we need to update the README to discuss this as well under the Configuration section.

ls339 · 2017-08-17T19:27:24Z

We can also add a service check in the future to check if the active queue has hit its limit. I believe its at this limit that we can tell if the incoming queue is backed up or will start to be.

http://www.postfix.org/QSHAPE_README.html#incoming_queue

The queue manager scans the incoming queue bringing any new mail into the "active" queue if the
active queue resource limits have not been exceeded. By default, the active queue accommodates
at most 20000 messages. Once the active queue message limit is reached, the queue manager
stops scanning the incoming (and deferred, see below) queue.

truthbk

Looks good, also backward compatible which was my main concern. As much as the new option is saner, we probably should stick to respecting the current behavior. We can revisit how many people are using postfix.queue.size in monitors and dashboards before making a decision to make it the default behavior.

Autodetect agent_major_version from agent_version

ls339 added 2 commits August 8, 2017 15:31

add an option to use postqueue instead of sudo

2148536

update the changelog

58dd744

Lewis Soto and others added 4 commits August 8, 2017 16:00

Merge branch 'master' into lewis/improve_postfix_check

b63be09

remove some lint from check.py

b19892b

Merge branch 'lewis/improve_postfix_check' of github.com:DataDog/inte…

3e3e4ce

…grations-core into lewis/improve_postfix_check pull in changes from master

remove some lint from check.py

11b797a

masci mentioned this pull request Aug 9, 2017

postfix integration should not require sudo to root #593

Closed

truthbk approved these changes Aug 11, 2017

View reviewed changes

truthbk requested changes Aug 11, 2017

View reviewed changes

truthbk added this to the 5.17 milestone Aug 11, 2017

ls339 added 3 commits August 15, 2017 14:32

Will add more documentation to the config file and

bb20fee

add a sample postqueue -p output to code.

fix the version number in manifest.json

6b572fb

fix some linting errors

f4aadd6

irabinovitch reviewed Aug 16, 2017

View reviewed changes

hush-hush assigned hush-hush, derekwbrown and truthbk and unassigned hush-hush and derekwbrown Aug 17, 2017

ls339 and others added 2 commits August 17, 2017 15:15

add more configuration docs

62c607c

Merge branch 'master' into lewis/improve_postfix_check

a447574

olivielpeau modified the milestones: 5.18, 5.17 Aug 22, 2017

ls339 and others added 2 commits September 25, 2017 12:16

resolved changelog conficts

46e9943

Merge branch 'master' into lewis/improve_postfix_check

56e9a65

truthbk approved these changes Oct 4, 2017

View reviewed changes

truthbk merged commit 9b41d24 into master Oct 4, 2017

ls339 deleted the lewis/improve_postfix_check branch October 25, 2017 03:02

gml3ff pushed a commit that referenced this pull request May 14, 2020

Merge pull request #665 from DataDog/albertvaka/autodetect-major-version

921317f

Autodetect agent_major_version from agent_version

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lewis/improve postfix check #665

Lewis/improve postfix check #665

ls339 commented Aug 8, 2017 •

edited

bits-bot commented Aug 8, 2017

truthbk left a comment

truthbk Aug 11, 2017 •

edited

ls339 Aug 15, 2017

truthbk Aug 11, 2017

ls339 Aug 15, 2017

truthbk Aug 11, 2017

truthbk Aug 11, 2017

truthbk left a comment

irabinovitch commented Aug 16, 2017 •

edited

irabinovitch Aug 16, 2017

irabinovitch Aug 16, 2017

irabinovitch commented Aug 16, 2017

ls339 commented Aug 17, 2017

truthbk left a comment

		@@ -5,6 +5,14 @@

		### Changes

		* [IMPROVEMENT] option to run the check using postqueue -p

Lewis/improve postfix check #665

Lewis/improve postfix check #665

Conversation

ls339 commented Aug 8, 2017 • edited

What does this PR do?

Motivation

Testing Guidelines

Versioning

Additional Notes

bits-bot commented Aug 8, 2017

truthbk left a comment

Choose a reason for hiding this comment

truthbk Aug 11, 2017 • edited

Choose a reason for hiding this comment

ls339 Aug 15, 2017

Choose a reason for hiding this comment

truthbk Aug 11, 2017

Choose a reason for hiding this comment

ls339 Aug 15, 2017

Choose a reason for hiding this comment

truthbk Aug 11, 2017

Choose a reason for hiding this comment

truthbk Aug 11, 2017

Choose a reason for hiding this comment

truthbk left a comment

Choose a reason for hiding this comment

irabinovitch commented Aug 16, 2017 • edited

irabinovitch Aug 16, 2017

Choose a reason for hiding this comment

irabinovitch Aug 16, 2017

Choose a reason for hiding this comment

irabinovitch commented Aug 16, 2017

ls339 commented Aug 17, 2017

truthbk left a comment

Choose a reason for hiding this comment

ls339 commented Aug 8, 2017 •

edited

truthbk Aug 11, 2017 •

edited

irabinovitch commented Aug 16, 2017 •

edited