fix(relations): skip database request without a database name - #1897
Draft
marceloneppel wants to merge 3 commits into
Draft
fix(relations): skip database request without a database name#1897marceloneppel wants to merge 3 commits into
marceloneppel wants to merge 3 commits into
Conversation
A database_requested event deferred while Patroni is unavailable is replayed by ops on every later dispatch. If the relation has gone away in the meantime, ops hands the handler a dead relation whose remote application cannot be resolved, so the requested database name reads back as absent. The handler coerced that to an empty string and then indexed it, raising an uncaught IndexError. Because ops replays deferred notices before the hook's own event, and only drops a notice once its handler returns, that exception aborted every subsequent hook on the leader unit permanently. No client relation could be provisioned and no endpoint refreshed, so related applications stayed blocked without credentials even though the cluster itself was healthy. Bail out when the name is absent rather than coercing it, so the stale notice drains on its next replay and the unit recovers on its own. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
The regression test only checked that the handler stopped short of creating a database or user. A guard that logged and then deferred again would satisfy that while leaving the notice queued forever, so the unit would still never recover - the failure this change exists to prevent went unasserted. Returning without deferring is what lets ops drop the notice, so assert it directly. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
Pinning only the absent name left the guard's shape untested: a variant checking `is None` passes every test while still walking an empty string into the index that raised in production. Covering both values locks in the behaviour the fix depends on rather than the particular expression used to get it. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
|
We reported the same issue in two places:
Should we update #1859 with the learnings from ISREQ-3825 and link to #1859 in this PR? |
Member
Author
Hi, @alnvdl-work! Yes, we should. |
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.
Issue
On a leader unit, a
database_requestedevent that was deferred while Patroni was unavailable gets replayed by ops on every subsequent dispatch. If the relation has gone away in the meantime, ops supplies a dead relation whose remote application cannot be resolved, so the requested database name reads back as absent._collect_databasescoerced that missing value to an empty string, and_on_database_requestedthen indexed it, raising an uncaughtIndexError: string index out of range.ops replays deferred notices before the hook's own event, and only drops a notice once its handler returns. The exception therefore aborts every later hook on that unit permanently, actions included. The leader can no longer provision client relations or refresh endpoints, so any client relation that had not yet been served stays unserved. The unit itself sits in
erroron a failed hook.Reported downstream as ISREQ-3825.
Solution
Return early from
_collect_databaseswhen the relation no longer carries a database name, instead of coercing the missing value to an empty string. The handler already treats aNonereturn as "nothing to do", so the stale notice drains on its next replay. Note this only applies once the fixed revision is running: a unit already wedged on an older revision needs to be refreshed and have its failed hook cleared before the guard can execute.An empty name is not reachable from a live requirer: Juju drops a databag key whose value is an empty string, and the library only emits
database_requestedwhen that key is added. The guard therefore only affects the replayed-stale-notice path. The remainingdatabase[-1]sites in this module are already protected by truthiness checks and are left unchanged.Adds a regression test that drives the handler with an absent database name, which reproduces the
IndexErrorwithout the guard.Checklist