fix(mappings): serialise module mapping writes for a system (PPT-2816) - #293
Merged
Merged
Conversation
Two updates of one system could run set_mappings at the same time. Each cleared the redis hash and wrote its keys one by one, so the two writers interleaved and the hash ended up in an arbitrary order. Drivers that pick a module by position in that hash, the mailers in particular, then picked the wrong module; on HIO UAT this routed every templated email to the Calendar module and made the Template Mailer send to itself. The writes for a system now take a per-system lock, and the mapping is resolved before the hash is cleared so it is only empty while being written. PPT-2816
Member
|
let's just have a single mapping lock instead of per-system, I don't see contention being high enough to require a lock per-system, also redis writes are serialized anyway so this won't impact performance significantly |
github-actions
Bot
added
type: bug
Member
|
actually, please fix the CI |
github-actions
Bot
added
type: bug
…inio from pgsty The spec still asked for a lock by system id. The minio and mc images moved to pgsty/minio and pgsty/mc, and the old names no longer pull, which failed every CI run before the specs started. PPT-2816
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
ControlSystemModules.set_mappingscleared a system's redis hash and then wrote its keys one by one. Two updates of the same system arriving together (on HIO UAT on 24 Sep: a system PUT and the module delete cascade from the same edit, within one second) ran it concurrently, and because each redis call is a fibre switch the two writers interleaved. The hash ended up in an arbitrary order with the Calendar module ahead of the mailers. Drivers pick modules by position in that hash, so the visitor mailer sent through Calendar and the Template Mailer forwarded to itself until core-0 was OOM killed. Full timeline on PPT-2816.A no-op system update also rewrites the hash (
!system.changed?is treated as needing a write), and so does a module rename throughModuleNames, so any two of those overlapping had the same effect.The writes for a system now take a per-system lock, and the mapping is resolved from the database before the hash is cleared, so the hash is only empty for the time it takes to write it rather than for the duration of the module lookups.
Testing
set_mappingsfor that system is still waiting 300 ms later, and completes with the mapping in the system's order once the lock is released. Run in the compose test runner: 7 examples, 0 failures. With the lock bypassed the same spec fails at the "still waiting" assertion.src/core-app.crclean.Related
implementingreturns modules in the system's module order rather than hash order, so drivers no longer depend on this hash's order at all.Rollout: the shard change reaches drivers when they are rebuilt against a new driver release; this change ships with the next core release. HIO's core pod also runs at a 2 GiB limit with every driver process inside it, which is what turned the recursion into an outage.
PPT-2816