Make RedshiftDeleteClusterOperator busy-retry window configurable#69499
Make RedshiftDeleteClusterOperator busy-retry window configurable#69499seanghaeli wants to merge 2 commits into
Conversation
The delete operator retries on InvalidClusterStateFault when the cluster is mid-transition (e.g. pausing/resuming). The retry budget was hardcoded to 10 attempts * 15s = 2.5 min, which expires long before a ~15 min pause settles into a deletable state, causing the task to fail and the cluster to leak. Promote the two hardcoded constants to __init__ params (busy_retry_attempts, busy_retry_interval) and raise the default to 60 * 15s = 15 min so the retry outlasts a pause. Both are overridable. poll_interval/max_attempts (completion waiter) are unchanged. Note: this synchronous retry loop still runs before the deferrable branch, so it blocks the worker in both sync and deferrable modes. Moving the busy retry into the async trigger for deferrable mode is a follow-up. Generated-by: Claude Code (Opus)
boring-cyborg
Bot
added
area:providers
provider:amazon
seanghaeli
changed the title
In deferrable mode the operator no longer runs the synchronous busy-retry loop that blocked the worker/triggerer for up to the ~15 minute busy-retry window. Instead it attempts the delete once and, if the cluster is mid-transition (InvalidClusterStateFault), defers to a new RedshiftClusterSettledTrigger that asynchronously polls until the cluster leaves every transitional lifecycle (creating/modifying/resizing/pausing/ resuming/deleting/rebooting/...). The _retry_delete_when_settled callback then re-issues the delete: on success it defers to the existing RedshiftDeleteClusterTrigger (cluster_deleted) -> execute_complete; on a race (still InvalidClusterStateFault) it re-defers to the settle-wait trigger, bounded by busy_retry_attempts. This mirrors the EksCreateCluster re-defer pattern. Sync mode (deferrable=False) keeps the existing synchronous busy-retry loop unchanged. A poll-until-settled trigger (rather than a single-target-status poller or a custom botocore "not-transitional" waiter) is used because a busy cluster settles into different terminal states depending on the in-flight operation (pausing -> paused, resizing -> available), and the delete is acceptable in any non-transitional lifecycle. Generated-by: Claude Code (Opus)
seanghaeli
force-pushed
the
feature/redshift-delete-configurable-busy-retry
branch
from
46b1088 to
737bb87
Compare
vincbeck
left a comment
vincbeck
left a comment
There was a problem hiding this comment.
Why creating new parameters for that? It seems unnecessary. Why not just increasing the value of existing parameters?
|
@vincbeck We could just make the default way longer but this does synchronously wait so we shouldn’t unnecessarily use resources to wait. Parameters seem to strike to balance of being usually short but configurable to longer if needed |
seanghaeli
changed the title
I'm not sure what you mean by being "usually short"? It looks like the default is set to 15mins for those parameters? So the idea of making them short and extending if needed seems like it's not being used here. In reality, I think most people just want the operator to work, even if they have to wait, since the other outcome is much worse. So I'd maybe just simplify them back to constant values like before. But I don't feel tooo strongly about it |
|
@o-nikolas As in the default is 2.5 mins but then you can configure it to be longer in your case if you need to. I can also make the default case just be 20 minutes which could capture most situations. On the other hand, waiting dynamically for a completion signal would require new features in the triggerer that I'm not sure is worth writing currently, but maybe we decide this approach is justified. Is there any downside to making this an optional parameter? |
|
Closing this in favor of #69574 which implements the async handling as well |
3 participants
Why
RedshiftDeleteClusterOperatorretries onInvalidClusterStateFaultwhen the cluster is mid-transition (e.g. pausing/resuming/resizing), but the retry budget was hardcoded to10 attempts * 15s = 2.5 minutes. A cluster transition routinely takes much longer than that (a pause is ~2–15 min), so the loop exhausts and re-raises long before the cluster reaches a deletable state. The task fails and the cluster keeps running until manual deletion.Example errors:
Unable to delete the cluster ... You can only delete clusters with PAUSED, ..., ACTIVE, ... lifecycles.What
Promote the two hardcoded constants to
__init__parameters and raise the default so the retry outlasts a real transition:busy_retry_attempts: int = 60busy_retry_interval: int = 15Default is now
60 * 15s = 15 minutes, and both are overridable.This retry loop is synchronous and runs before the
deferrablebranch, so in deferrable mode it still blocks the worker for the retry window. This PR fixes the synchronous path (the reported leak). The correct async fix for deferrable mode is a larger change that adds a new serialized trigger, so it will be a separate follow-up PR to keep this one small and focused.End-to-end before/after test verification
Verified against a real Redshift cluster:
BEFORE (stock
main, 2.5-min budget) — task FAILS, cluster leaks:AFTER (this PR, 15-min budget) — task SUCCEEDS, cluster deleted: