On my test system, running `k6 run tsa-torture.js` (included in this
commit) could achieve ~26,000 qps.
With the changes in this commit I see ~60,000 qps.
The gains are realized by introducing batching and transactions in the
publish code path, and allowing sqlite to avoid a synchronous flush.
Turning down the tracing::info! that was logging when rules
were matched made a non-trivial impact on throughput;
35k qps with info!, 60k qps with it at the current debug!, so I've
put it at debug.
This commit introduces a processor pool with n-cpus as
the number of threads as the way that we ingest log
records.
The ingestion is done via a batch-channel which will
allow the potential for automatic batching into larger
transactions in a later commit.
This commit causes publish requests to return as soon
as the record is queued into the channel, rather than
once the record has been processed. This means that
there is now a window of opportunity where, if tsa-daemon
were to panic or be killed, the associated records will
be forgotten. There is a maximum of 128k records that
can be in the queue at one time, which puts an upper
bound on this.
In practice, it is not critical if records are lost
in this way: it is not significantly different from
the time window where a node might continue to send
mail before the effect of an automation kicks in.
The net effect of this is that the submission latency
is now much lower while the system remains un-saturated,
but will then block additional submissions until the
backlog is cleared.
Previously we'd use the Debug impl of ResolvedAddress, which rendered
like `ResolvedAddress { name: "some.host.", addr: "10.0.0.1" }`, which
is a bit heavyweight when it shows up in a log where we're indicating
that none of the hosts could be connected.
This commit adds a Display impl that renders that same struct in a
more compact form: `some.host./10.0.0.1`.
At reception time, we've supported using the special queue name
"null" as a way to indicate that a message should be accepted
but then silently dropped.
Since that was built, we've added a couple of ways through which
a message can be assigned to a different queue after reception.
In a few situations we've had users that want to assign those
messages to the null queue to indicate that they should be dropped.
That has "worked" but not for the right reasons: setting the queue
name to "null" is equivalent to setting the routing domain to the
domain named "null", which doesn't resolve to a valid MX and that
will eventually cause messages that were queued there to expire
out of the system.
This commit introduces a special case for a queue named "null"
that configures it with a special "Null" delivery handler.
Similar to the maildir handler, this one will immediately
handle ready messages without instantiating a corresponding
ready queue for them.
In this case, handling them means simply deleting them with
no other logging.
We don't log here in order to be consistent with the reception-time
behavior of not logging. It might be desirable to add some
configuration around this in the future, but for the moment, the primary
way that messages might end up assigned to a "null" queue is via a
rebind operation, and that will log an `AdminRebind` record that can be
used to capture that change in responsibility.
If logging a bounce is desired, it is recommended to use kumo.reject to
bounce the message during the rebind event instead of assigning it to
the null queue.
The primary purpose of this is so that I can do this:
```
ROCKSDB_LIB_DIR=/usr/lib/x86_64-linux-gnu
```
on my ubuntu system and cut down on rocksdb related build times.
This should help to age out messages in pathological cases where
the bulk of the messages are not actually being attempted, but
rather being requeued due to excessive connection failures or
other bulk queue operations.
Really, I just wanted the debug output in CI to be less overwhelming,
but while I'm in here, might as well switch to parking_lot.
This commit switches from the derived Debug impl to just serializing
the metadata as json, which is much more compact.
previously, we'd generate a trace_id only in the SmtpDispatcher
for the tracing subsystem.
We want to capture a session id in the logs, so the first step
is to formalize this field.
refs: https://github.com/KumoCorp/kumomta/issues/316
Following on from b64219bab4, if we
do encounter a 354 response to a message send, it must indicate
a protocol synchronization issue, so let's treat the associated
message as it had transiently failed and explicitly break the
connection (like we do for a 421).
I did consider making the associated message immediately eligible
to retry, but I worry that if that particular message is a poison
pill we might get stuck in a short cycle of retries for it.
I don't think it is likely for this particular message to be the
poison pill, because it is most likely the victim of an earlier
issue on the connection, but I still want to be cautious here.
The outcome here is still not ideal, but it is better than marking
the associated message as a permanent failure.
This makes it easier to set a default for remember_broken_tls
without it causing issues for sites that have a transient blip
with TLS, when TLS is set to required for those sites.
We've seen evidence in some logs that it is possible to end up
with a protocol synchronization issue where we get back a 354
response (which can only occur from the DATA verb) when we think
we just sent RCPT TO.
It is unclear where the error is coming from as the logic in
the smtp client is already fairly defensive and emits speculative
RSET calls for each message.
What I suspect might be happening is that we somehow encounter
a transient issue where we don't assertively close the connection.
We do have what I considered to be fairly exhaustive handling of
the various error cases in the smtp_dispatcher deliver_message
function, so it's not obvious where the issue might be coming
from.
So, what does this commit do?
* Refactor the smtp client logic code that is responsible for sending
the request side of things so that we just have two cases, rather
than the previous handful
* If the write fails either due to a timeout or because of some
other transport error, we now expliticly break the client by
clearing out its socket field. This will prevent continued
use of the client.
* in smtp_dispatcher, if we end a send with a broken client and
haven't already propagated an error up to the caller, we will
raise an error. That will cause the session to try the next
host in the connection plan or just tear down the session
depending on the overall state of that session.
Previously, we added `batch_size` which acts as a maximum batch
size; this was the simplest possible way to achieve batching
with a constrained upper bound with no additional latency.
This commit introduces a minimum batch size and a maximum latency.
The behavior for delivery handlers that configure min_batch_size > 1
is that once we've started a batch, we'll allow for up to
max_batch_latency time to pass to accumulate more messages into
the batch before we just go ahead and send it.
If you wanted to batch say 5,000-10,000 messages per batch or whatever
you get in a 10 second time period you would set:
* min_batch_size = 5000
* batch_size = 10000
* max_batch_latency = "10s"
Note that the default idle_timeout in the product is "5s" so if you want
max_batch_latency to be effective you will also need to increase the
idle_timeout in your shaping configuration/egress path config for that
handler.
Wez Furlong
Dirkjan Ochtman