I've observed a system where we had more than 4 outstanding
shaping_data lookups from different ConfigEpochs; that system
had slow dns resolution and we ended up looping for these because
the capacity was not high enough and the population of the result
always took too long.
Let's just give ourselves a bit more headroom; this coupled
with the earlier commits that adjust retry behavior should
help to smooth things out in this situation.
It's not a good idea to automatically retry internally at the lruttl
layer, as there is no bound on the retries and not enough context in the
cache layer to decide if we really should retry.
This commit removes the `retry_on_sema_timeout` flag from lruttl
and replaces the implementation of the mod-memoize
retry_on_populate_timeout flag with a simple 3 attempt retry
loop in the failure case.
Importantly, those retries in mod-memoize have the opportunity
to adjust to a bumped ConfigEpoch, which is important for the
pathological situation where a flood of changes stream in from
TSA daemon and there is a delay in handling populate, and the
cache capacity is not sufficient to accommodate that run
of epoch bumps.
I saw an instance where the throttle_insert_ready_queue event was
trying to inspect the metadata but failing because the metadata was
not loaded.
This should avoid that problem.
This commit does two things:
* Migrate the limit of 128 concurrent lookups from the shaping code
and into the general mx lookup code
* Adjust the mx lookup implementation to run inside the cache
getter, which applies thundering herd protection to the lookup.
These together will constrain the amount of queries being sent
to the upstream dns resolver and reduce the chances of it being
overwhelmed, especially if we are doing duplicate queries from
multiple concurrent contexts simultaneously.
We cannot default it to enabled because a fully working
dane setup requires additional configuration in the resolver
and we cannot guarantee that that has happened here.
It's a little awkward to reason about whether you're running the latest
build or not after an upgrade, so let's explicitly print the version
to the journal.
For posterity, regardless of whether you have this particular commit or
not, you can determine the running/online version by looking at the
openapi schema document:
curl -s 'http://127.0.0.1:8000/api-docs/openapi.json' | jq .info.version
There is a potential race between the timerwheel tick and bulk queue
operations (flushing, bouncing). The logic in the tick case has
some accommodations for this, but there is another outstanding
that can result in racing to manage the metadata load state
on a message.
In the tick case, we load it if needed, then resolve the queue
name, so that we can resolve the Queue handle.
We need the queue handle to decide whether we are responsible
for the message, or whether we might be racing with a concurrent
action.
In the race case, the other actor may have decided to release
the message metadata, which will cause the queue name resolution
to fail.
While we could just shrug and silently ignore the error in that
case, that makes me uneasy.
What this commit does is adjust the v1 wheel entry so that we
capture weak references to both the message and its containing
queue.
Then when we tick, we can simply upgrade both of those and
reconcile without needing to manipulate any message metadata.
These changes unfortunately result in re-duplication of some
of the logic that I recently refactored to be shared with the
v2 tick implementation.
The v2 tick implementation has the same edge case around
metadata, but cannot be easily adjusted to use the same
pattern.
So this commit sticks a comment in the code; nobody is using
v2 AFAIK, and there have been reports of some wonky behavior
with it. My recommendation at this time is to avoid using
the v2 wheel and we can fix this all up for it later.
Some of the series ended up with bland labels like "lua" with
no context. Let's include the overall metric name for those.
The consequence of this is that the labels column gets wider,
but I think that now that we have scrolling, we can adjust
that presentation in a follow up commit.
There are a lot of metrics these days, we need to scroll through them!
Use the arrow keys, page up/down and home/end for this purpose.
closes: https://github.com/KumoCorp/kumomta/issues/372
This was probably the casualty of some earlier refactoring
that has gone unnoticed until now.
Since we don't have explicit context on which key format to
parse in this helper function that is used in multiple places,
let's just make it try to parse both rsa and ed25519.
closes: #368
It is sometimes necessary to set environment variables
to adjust some early-startup options that need to be
applied before enough of the system is ready to read
lua policy files.
While it is possible to use `systemctl edit SERVICE`
and enter systemd-specific directives to configure that,
it is not the most convenient thing.
What this commit does is introduce 3 new optional files:
* /opt/kumomta/etc/kumo-common.env: sourced by kumod and tsa
* /opt/kumomta/etc/tsa-daemon.env: sourced by tsa
* /opt/kumomta/etc/kumod.env: sourced by kumod
The format of these files is `NAME=VALUE` on each line, as
per the `EnvironmentFile` docs in systemd.
One practical application for this is on an over-committed system
where the number of reported cores is larger than the number of
available cores.
For such a system you will need to configure the
`KUMO_AVAILABLE_PARALLELISM` environment variable for each kumo
service running on your system. With these environment files you
can do that simply by creating `/opt/kumomta/etc/kumo-common.env`
with a line like this inside:
```
KUMO_AVAILABLE_PARALLELISM=48
```
Running the lockbud deadlock detector over the code, it thought that the
Once based initialization of the wheels might recursively attempt to
lock an internal mutex.
Seems like a false positive to me, but if it could trigger, it would
only be on startup, and we've had no reports of this being a thing.
Regardless, it made lockbud think really hard about it for just under 7 hours,
and adjusting the logic makes lockbud run in just under a minute, so
it's worth appeasing it.
Adjusting the initialization/insertion logic to look more like
the v2 case makes lockbud happy and continues to pass the
integration test suite.
```json
{
"DoubleLock": {
"bug_kind": "DoubleLock",
"possibility": "Probably",
"diagnosis": {
"first_lock_type": "ParkingLotMutex(timeq::TimeQ<message::message::WeakMessage>)",
"first_lock_span": "crates/kumod/src/queue/strategy.rs:204:23: 204:45 (#0)",
"second_lock_type": "ParkingLotMutex(timeq::TimeQ<message::message::WeakMessage>)",
"second_lock_span": "crates/kumod/src/queue/maintainer.rs:278:17: 278:26 (#0)",
"callchains": [
[
[
"crates/kumod/src/queue/strategy.rs:206:25: 206:51 (#0)"
],
[
"crates/kumod/src/queue/maintainer.rs:357:5: 363:7 (#0)"
],
[],
[
"/home/wez/.rustup/toolchains/nightly-2025-02-01-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sync/poison/once.rs:158:41: 158:60 (#0)"
],
[
"crates/kumod/src/queue/maintainer.rs:358:9: 362:11 (#0)"
],
[],
[
"crates/kumod/src/queue/maintainer.rs:348:17: 348:36 (#0)"
],
[
"/home/wez/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tokio-1.44.1/src/runtime/runtime.rs:342:13: 342:74 (#0)"
],
[
"/home/wez/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tokio-1.44.1/src/runtime/runtime.rs:368:47: 368:88 (#0)"
],
[],
[],
[
"/home/wez/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tokio-1.44.1/src/runtime/scheduler/current_thread/mod.rs:211:49: 211:73 (#0)"
],
[
"/home/wez/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tracing-0.1.41/src/instrument.rs:321:9: 321:23 (#0)"
],
[
"crates/kumod/src/queue/maintainer.rs:359:56: 359:61 (#14652)"
],
[
"crates/kumod/src/queue/maintainer.rs:290:39: 290:44 (#0)"
]
],
]
}
}
}
```
Occasionally we'll have someone report that systemd timed out
and sigkill'd their kumo on shutdown.
One possible scenario for this is a lua delivery handler that
is taking too long, presumably because the other end of it
(eg: webhook or other custom endpoint) is not responding in
a timely fashion.
The way that we handle shutdown is that we compute a maximum
theoretical timeout value by summing up all of the smtp client
timeout values. Some of those can be several minutes in
duration because the are using default values derived from
a very conservative set of values suggested by the SMTP
RFCs from the '70s.
Those obviously should not apply to a custom delivery handler,
but also, in the context of an established SMTP session, we
should not add in the connection-establishment-specific values
when we're just waiting for a per-message send.
This commit addresses this situation on two fronts:
* Introduce a new system_shutdown_timeout value that allows the
user to conveniently express their desired timeout value
in a single option. This is *not* set by default!
* The default value for system_shutdown_timeout is computed by
summing the per-message-delivery smtp timeout options, which
is a much more reasonable, and more importantly, shorter than
our 300s TimeoutStopSec value in kumomta.service
Previously, we'd use the qmaint pool to spawn both the scheduled
and ready queue maintenance tasks.
This commit splits them apart in order to avoid the potential for
cross-task contention on the same threads if a scheduled queue
and ready queue pair are communicating with each other.
Put them in the main pool; that way there can't be any contention
between their overall management tasks and the actual work that
needs to be carried out in qmaint.
If we hit this case, it tends to be when we have multiple wakers,
so this warning floods the logs. Let's tone that down.
We bump an error counter in this case, so there are easier ways
to monitor this condition.
I've traced this on a couple of occasions and the protocol sync
error we log is a false positive for apple because they are subtly
non-conforming with the SMTP RFCs when pipelining.
During shutdown we fail all calls to QueueManager::resolve with the
error "shutting down".
Let's avoid spamming the journal with those; they are expected and
normal.
We've been seeing stalls for some users when loading shaping data. The
two likely dependencies for this are the DNS resolution for site name
resolving and the HTTP requests for fetching from the TSA daemon.
We tell the underlying reqwest http client to use an overall deadline,
but just to be ridiculously extra-sure, this commit puts our own timeout
around the operation, as well as explicitly sets the timeouts for
connecting and reading in the underlying client.
We recently observed a system where tsa-daemon run out of file
descriptors because it hit a systemd default of 1024 on the host system,
which had 192 cores. We spawn a thread per core for internal processing
in tsa-daemon, and that has associated with it a number of kernel
objects that each have an fd.
1024 is an unreasonably small limit for the number of fds, so let's
just try to raise to match the normally much higher hard limit, just as
we do in kumod.
We recently observed a system running on an over-committed VM that
reported 4x the actually available parallelism.
Since we scale our thread pool sizes from this value, it resulted
in an extra-over-committed configuration for kumod.
You may now set KUMO_AVAILABLE_PARALLELISM in the environment to
override the value that we see both interally and expose via
the lua function with the same name.
This commit adds an undocumented and temporary knob for spawning
the reinsertion part of the singleton timer wheel in the interest
of reducing delays between wheel ticks.
From poking around at this, the default behavior is that the
singleton ticking wakes up on an arbitrary thread in the qmaint
pool. That's normally desirable, but under heavy load that pool
is fairly saturated and that can lead to increased latency in
dispatching the ticks.
With this commit, when `kumo.set_timeq_spawn_reinsertion(true)`
is called during `pre_init`, we'll spawn a dedicated thread to
tick the singleton wheel, and each time it dispatches, it will
launch the spawned message batch into the qmaint pool.
This keeps the pop interval consistent.
This tracks the time between calls to TimeQ::pop.
At the time of writing, this should be approx 3 seconds in a healthy system.
Occasional longer delays are OK, because the pop can catch up, but if
the system is continually lagging here, or lagging by a large amount,
then we can become tardy in dispatching messages from the scheduled
queue.
extract this function from both singleton wheel implementations:
it is the same for both except for the name of the enum
variant that contains the underlying hash set.
This should have no functional impact.
This commit adds unit tests to validate the behavior when delaying
a message by a negative timestamp.
The expected behavior when that message is inserted into a scheduled
queue is for it to return it back to us instead of accepting it,
because it is immediately due.
This was true for 3 out of the 4 strategies; the skiplist strategy
allowed insertion but woke up the maintainer. The net effect
of that should overall be equivalent, but it is technically
different.
This commit fixes it up so that it behaves the same as the
other strategies.
Wez Furlong
Edgar Vaitkevicius