Details

Reviewers

vanmeeuwen
machniak
petersen

Maniphest Tasks

T3751: Emails stuck in wallace queue until wallace is restarted
T2163: wallace fails to connect to ldap then running in debug 9 mode
T2498: Wallace logs errors when run in debug 9 mode

Commits

rP10f1c173941d: With this patch I'm trying to introduce a file-type object in logger, which…

Summary

...in fork mode stderr is not available (Bad file descriptor error) and thus wallace traceback when it tries to send emails

Test Plan

none

Diff Detail

Repository

rP pykolab

Branch

master

Lint

Lint Skipped

Unit

No Test Coverage

Build Status

Buildable 18062
Build 9425: arc lint + arc unit

Event Timeline

adomaitis created this revision.Mar 15 2018, 3:18 PM

This is using the new file-like object from pykolab.logger. With this patch smtplib debug output gets redirected to logger rather than to stderr, which is not available in fork mode

This amendment tries to avoid cyruslib printing to non-available file descriptor, which causes traceback of wallace.

machniak added inline comments.Mar 15 2018, 3:47 PM

pykolab/imap/cyrus.py
147–153	Looks like indentation here is broken.

In addition LDAP trace_file needs to be specified. It is a file-like object define in logger.py Now we get ldap debug logs in pykolab.log file

Fixing intendation

Additionally, making wallace log.exception to log tracebacks

I think we need process id (PID) logged for any level log record, that makes it easy to associate and track wallace and other pykolab modules work flow.

Fixing another wrong intendation

Harbormaster completed remote builds in B16978: Diff 1339.Mar 16 2018, 12:08 PM

Specifying a bit more detailed log message.

Harbormaster completed remote builds in B17032: Diff 1345.Mar 18 2018, 5:03 PM

Need to catch imaplib debug output as well

Harbormaster completed remote builds in B17047: Diff 1351.Mar 18 2018, 10:11 PM

Fixing previous mistake. Need that for messages 9 and lower to be logged.

It turns out smtplib debug logging redirected to logger produces excessive new lines. This is attempt to mitigate the problem, make logs more pretty.

adomaitis added tasks: T2498: Wallace logs errors when run in debug 9 mode, T2163: wallace fails to connect to ldap then running in debug 9 mode.May 2 2018, 11:07 AM

adomaitis added a task: T3751: Emails stuck in wallace queue until wallace is restarted.

Make invitationpolicy module use smtplib sendmail wrapper _sendmail from modules, which now supports debug level 9 logging.

Make all pykolab log messages appear on level <8. This will isolate pykolab debug messages from 3rd partty libraries debug messages. -d debug -l 8 will print pykolab debug messages only, while -l debug -d 9 will print 3rd party libraries messages in addition to level 8 pykolab messages.

Looking good, except for the corner-case of an errmsg over an un-"captured" exception.

pykolab/imap/cyrus.py
152	I think you may need to; except Exception, errmsg: before you can refer to `errmsg` in the call to `log.exception()`.

In send_update_notification() we send mail in a loop trying up to 5 times if an error occurrs. But this is the only place where we do this. I think it would make sense to do this in all cases, i.e. in _sendmail(). I see also we don't call smtp.quit() consistently. Once we do this on success only, once we do this on error too.

In D577#6587, @machniak wrote:

In send_update_notification() we send mail in a loop trying up to 5 times if an error occurrs. But this is the only place where we do this. I think it would make sense to do this in all cases, i.e. in _sendmail(). I see also we don't call smtp.quit() consistently. Once we do this on success only, once we do this on error too.

The calls to modules._sendmail() are themselves looped, by the caller. Does that address the concern?

Eliminating inconsistensies of sending mail from wallace. Now all the logic is in modules._sendmail() wrapper. Invitationpolicy module is using that wrapper and makes it responsible for re-submistion logic.

Fix conner case exception handling.

A few more remarks.

wallace/modules.py
156	This line would prevent `smtp.quit()` and other cleanup from being called.
159–163	Note that the existing `smtplib.SMTP()` used to be rebuilt the connection for here.
163	This does not invoke recreating the `smtplib.SMTP()` instance, therefore possibly looping?