This is lingering from the past; we don't need LocalSet any
more. I thought this might be causing some scheduling issues,
but it was neutral overall, so this commit is really just
a code cleanup.
For sites with large numbers of admin bounce entries, it is
desirable to negatively cache the lookup so that we can avoid
extraneous matching overhead.
We use a generation counter to determine if any rules have changed
and allow that to drive the invalidation of negative caching.
Otherwise, we cache the last lookup and, if the generation is the
same, then we cache the prior result and respect its expiration.
After running this at a couple of sites, this appears to be
good overall, so let's remove the option, reduce complexity
and make things better by default.
I noticed on a busy system with over 1500 active admin bounce
entries (created via automation) that cpu was quite busy in qmaint.
I believe that is because we were potentially spawning a new
async qmaint task for every message moving from the wheel and into
the ready queue.
This commit removes that logic, and replaces it with a per-message
check in insert_ready.
Queue insertion can generate log records if queues are full,
so holding a backlog permit could lead to blocking-like
behavior when the system is relatively saturated.
Let's release the permit prior to insertion; that way
it still provides some benefit (limiting the should-enq hook
calls) while not getting in the way.
The pattern used in the logic was that in the Pending case,
once we had acquired the semaphore, we'd do some prep work,
then await the completion of the populator function, then
store the result and then close the semaphore to force all
wakers to kick out and consult the return value.
The await is a cancellation point, which means that if the
call stack above the population function encounters a timeout
(eg: data_processing_timeout) then the whole state is dropped
and never consulted again.
The drop would release the semaphore permit and allow another
actor to acquire it, which is partially OK, but we really do
want to close the semaphore to wake up everyone in that case.
This commit switches from the explicit close at the end of
the scope to using defer! to handle it in all exits from
that particular scope, including the drop on cancellation.
Doing that highlighted a busy loop in response to the cancellation; to
deal with that, while we have the entry locked, if the semaphore is
already closed we'll treat it the same way as an expired ttl and allow
kicking off a new resolve.
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)"
]
],
]
}
}
}
```
Wez Furlong
Mike Hillyer
Edgar Vaitkevicius