fix(ha): don't wedge the app when scale-down lock can't be acquired#65
Draft
delgod wants to merge 1 commit into
Draft
fix(ha): don't wedge the app when scale-down lock can't be acquired#65delgod wants to merge 1 commit into
delgod wants to merge 1 commit into
Conversation
Removing the whole application fires storage-detaching on every unit at once. Each unit races for a single scale-down lock; the losers raised RequestingLockTimedOutError, which failed the hook and parked the unit in error state. Its agent is then torn down before juju can retry the hook, so the unit stays wedged and the application can never finish removing — `juju.wait(APP not in status)` times out (seen in k8s/test_scaling.py:: test_scale_down_remove_application). - ScaleDownLock.request_lock now genuinely retries under contention. The retry count was `min(timeout // 5, 1)` (always 1) and a contended SET NX returns falsy WITHOUT raising, so tenacity never re-polled. Replace it with an explicit poll loop using `max(...)` so the timeout is honored. On retry it re-resolves the primary with the resilient get_primary_ip_for_scale_down() (which rides out transient sentinel failures during teardown) and builds the ValkeyClient once. - _on_storage_detaching no longer raises when the lock is unavailable during full-app removal (planned_units() == 0): it goes away gracefully instead of wedging, logging at warning with the unit name. A partial single-unit scale-down still raises so juju retries the hook. - While acquiring the lock, ValkeyCannotGetPrimaryIPError and ValkeyWorkloadCommandError (e.g. from get_active_sentinel_ips) are tolerated the same way: go away on full-app removal, otherwise re-raise so juju retries — rather than silently skipping failover and the dataset save on a partial scale-down. Covered by unit tests in tests/unit/test_locks.py and tests/unit/test_scaledown.py. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
skourta
reviewed
May 25, 2026
| self.charm.state.unit_server.unit_name, | ||
| ) | ||
| self._set_state_for_going_away() | ||
| return |
Contributor
There was a problem hiding this comment.
If we return here we will not save the databse to disk before stopping the workload. The unit will just be killed by juju
| # must re-attempt on a fixed interval until it is acquired or the timeout | ||
| # elapses. With no timeout we make a single attempt and let the caller | ||
| # decide how to wait (e.g. defer the hook so juju re-runs it later). | ||
| number_of_retries = max(timeout // 5, 1) if timeout else 1 |
Contributor
There was a problem hiding this comment.
This is a good catch. I thinkwe should still use tenacity to manage the retries though.
| e, | ||
| ) | ||
| self._set_state_for_going_away() | ||
| return |
Contributor
There was a problem hiding this comment.
On full app removal, we currently still orchestrate the shutdown to be executed one unit at a time but whther we change that to all units doing it at the same time or not, saving the DB to disk on line 636 should always be executed before the workload is shutdown.
| # How long a unit waits in-process to acquire the scale-down lock before either | ||
| # deferring the storage-detaching hook (so juju re-runs it) or, on full-app | ||
| # removal, going away. Kept well under juju's hook timeout. | ||
| SCALE_DOWN_LOCK_TIMEOUT = 30 |
Contributor
There was a problem hiding this comment.
I think we should increase it to something like 300. This should enable most units to avoid erroring out in storage detaching
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.
Removing the whole application fires storage-detaching on every unit at once. Each unit races for a single scale-down lock; the losers raised RequestingLockTimedOutError, which failed the hook and parked the unit in error state. Its agent is then torn down before juju can retry the hook, so the unit stays wedged and the application can never finish removing —
juju.wait(APP not in status)times out (seen in k8s/test_scaling.py:: test_scale_down_remove_application).ScaleDownLock.request_lock now genuinely retries under contention. The retry count was
min(timeout // 5, 1)(always 1) and a contended SET NX returns falsy WITHOUT raising, so tenacity never re-polled. Replace it with an explicit poll loop usingmax(...)so the timeout is honored. On retry it re-resolves the primary with the resilient get_primary_ip_for_scale_down() (which rides out transient sentinel failures during teardown) and builds the ValkeyClient once._on_storage_detaching no longer raises when the lock is unavailable during full-app removal (planned_units() == 0): it goes away gracefully instead of wedging, logging at warning with the unit name. A partial single-unit scale-down still raises so juju retries the hook.
While acquiring the lock, ValkeyCannotGetPrimaryIPError and ValkeyWorkloadCommandError (e.g. from get_active_sentinel_ips) are tolerated the same way: go away on full-app removal, otherwise re-raise so juju retries — rather than silently skipping failover and the dataset save on a partial scale-down.
Covered by unit tests in tests/unit/test_locks.py and tests/unit/test_scaledown.py.