cyrusimap / cassandane

Other
6 stars 11 forks source link

Sandbox SMTP email submissions in Cassandane #40

Closed rsto closed 6 years ago

rsto commented 6 years ago

Please don't merge this PR if you are OK with it. I'll need to update Cyrus master first.

We want to migrate the Cyrus-internal sendmail code to use SMTP backends. But this requires us to sandbox SMTP requests within Cassandane, otherwise we risk testers to unintentionally send out spam.

This PR adds a dummy SMTP server to Cassandane using the following approach:

After implementing this I learned that Cassandane::PortManager::alloc() might help with the port allocation. We could certainly get rid of the pipe and use that one? We will however need to keep the shutdown callback. I'm not sure if Cassandane::Cyrus::TestCase is the right place to add the callbacks?

The SMTP server is taking care of forking, so we could run a single instance for all Cassandane tests.

rsto commented 6 years ago

I rewrote the patch to use Cassandane::PortManager, I never really liked using a pipe. I upgraded the maximum port range to 20 in PortManager, but still see Cassandane failing rarely, most probably because of exceeding its available port range.

My most preferred option would be to start a single SMTP server on Cassandane startup and server the TestCases from the pre-forking server. I just don't know where to best start the SMTP server in the Cassandane start-up control flow.

elliefm commented 6 years ago

Cassandane doesn't really have much startup flow, testrunner.pl is basically it. Once it gets into running tests, pretty much everything gets set up and torn down again for each test. I think it's better to have the smtp service follow this pattern -- it means that if at some point we want to capture data from it in some way other than logging, it will be trivial to associate that data with the test instance that it came from.

Implementing the service as a forked sub is interesting -- as you say, it necessitates the shut down call back. I'm not sure what other ramifications there are. I would've done it as a standalone script, and add_service'd it into the cyrus.conf so that master would take care of starting and stopping it. But this seems like it'll work fine too.

rsto commented 6 years ago

Travis dies because of a broken Ruby setup in the Travis builder. But this PR needs to land, because https://github.com/cyrusimap/cyrus-imapd/pull/2225 needs to land.

elliefm commented 6 years ago

I don't think we have travis set up for cassandane. The ruby error seems to be due to github assuming, in absence of a travis config file, that the project is ruby (it's not). I don't think it used to do this? Anyway that failed check means nothing cause it's irrelevant :)

elliefm commented 6 years ago

I'm getting hundreds of the smtp processes at a time when I run cassandane, and it runs my 4GB VM out of memory and the OOM killer starts killing things.

I've added aba2a53 so that the smtpd process gets its own name in ps listings, so now you can do something like:

watch -n1 "ps -ef | grep 'cassandane smtpd' | wc -l"

to keep an eye on how many of them are running at any given moment. Watching this while Cassandane runs, it's sometimes up into the 300's!

I'm not sure what to expect here, but I'm not entirely surprised that having hundreds of copies of Cassandane in memory at a time is pretty hard on memory.... I wonder if we would see improvement if the smtpd was in a small, standalone script which we fork/exec? I'm not sure if there's a reason it's not already done this way? Looks like maybe it might need to be like that for xlog but I'm not sure.

Any thoughts?

elliefm commented 6 years ago

Ohhh, the Net::Server::PreForkSimple defaults to pre-forking 50 children for processing requests...... dang!

1602823 instantiates it to only use 2 instead. I'm not sure what a reasonable number here is, but when I run Cassandane now my load averages are an order of magnitude lower and there's plenty of memory head room.