Repository navigation
Do not fail a transient run on a throttled DescribeCluster - #107
Conversation
3fee8f9 to
5c43a05
Compare
Live testing against real EMRTested against real EMR in the sandbox account (719197435995, eu-central-1), this branch compared against Common setupMaster-only clusters, one The playbook choice matters more than it looks. With a 2-second step the launch waiter never observes Six techniques do all the work below:
Binaries were built from a given git ref with an optional patch applied, so old and new could be run side by side under identical conditions. 1. What is the account's actual
|
| predecessor | this branch | |
|---|---|---|
| outcome | abandoned the run at 5 polls / 3m41s | no give-up, ran the full 15-minute bound |
| recovery describe budget | 3 attempts (short) | 6 attempts (long) |
A three-and-a-half-minute network partition used to produce the same false failure this PR exists to remove, reached through a different error code. It no longer does, and the read budget moved with the wait.
Unbroken blindness beyond maxBlindDuration. Run: the bound scaled from 6 hours to 6 minutes, with a throttle fault so the rule above cannot fire first. It gave up at 7m10s on 9 consecutive polls, then the recovery describe spent its own budget on top. So the effective figure at production settings is nearer 6h15m than 6h: the bound is only evaluated when a poll happens, and the recovery describe follows it.
7. Bootstrap-failure retry, both paths
Run: a bootstrap action that always exit 1, on run-transient and on up.
Assert: three distinct jobflow IDs (from list-clusters, not our own logs), BOOTSTRAP_FAILURE on each, exit 1, and the retry logged at WARN not ERROR.
Result: all four, both paths. Re-run after the job-phase pre-flight was removed, since TERMINATED_WITH_ERRORS is a failure acceptor on the terminated waiter — so the bootstrap path now reaches its reason through the waiter and a recovery describe rather than the short-circuit that used to catch it. Also asserted as a negative: none of the bootstrap-failed clusters produced the "may still be running" leak warning, since they are terminal.
8. Does up name a persistent cluster it leaves behind?
up launches with KeepJobFlowAliveWhenNoSteps=true, so a cluster it fails to hand back never self-terminates. Two paths reach that.
Blind: fault permanently, bound 8 minutes. Launch timeout: no fault, bound cut to 2 minutes so the waiter gives up while the cluster is still STARTING.
Assert: the error names the jobflow ID; then confirm out-of-band that the cluster is WAITING with AutoTerminate: false.
Result: both paths name it, both orphans confirmed non-self-terminating and cleaned up by hand.
9. Persistent path, run latency, and fail-fast
up / run / down under sustained throttling, six-step playbook: all exit 0.
down under saturation exits 0, riding out three throttles on the terminate path's budget — it used to fail in three seconds, the same bug in another command.
run timing was measured specifically, because adaptive retry mode's limiter sleeps before sending once it has learned throttling, and run polls every 15s with one DescribeStep per step — so it could compound. It did not: 190s under saturation against a 209s baseline, i.e. no slower.
Successful up logs its state change reason at info, not error, and returns the jobflow ID.
Bad --emr-cluster: fails in 6s, unchanged. Real EMR returns InvalidRequestException for both malformed and well-formed-but-absent IDs, and the SDK does not classify it as a throttle — so the long throttle budget does not catch a hopeless answer.
Behaviour changes reviewers should weigh
- Detection latency rises — measured against clusters' own end times: 10s → 65s unthrottled on a short run, and minutes when the budget is saturated.
- A launch gives up after ~63 minutes when EMR is throttling and never clears, against a few seconds before. No longer silent, and a recoverable outage now resolves into a successful run — but during a regional incident processes hold their slot for an hour rather than failing fast. Worth checking against whatever supervises these runs.
- The job phase is bounded at 6 hours of unbroken blindness (
maxBlindDuration), insidetransientMaxWaitDuration's 14 days. Effectively ~6h15m, per test 6. - A wait now aborts after three consecutive failures that AWS answered. Good — it turns an hour-long hang on a misconfigured role into a 92-second failure. But it makes the retryable/not-retryable classification load-bearing for liveness, where previously it only chose a retry budget: an error misclassified as hopeless would abort a wait rather than merely get less patience. Nothing misclassified across 22,612 real throttles, and the connection-error case is covered in test 6.
- The post-waiter recovery describe is where a completed run could still be reported as failed. On a fully saturated run it used 4 of its 6 attempts before getting through. It succeeded, but that is the narrowest margin measured anywhere in this branch, and it is where to look if throttling ever costs a run again.
Not covered
run's step polling is untouched — 15s polls, oneDescribeStepper step, so six steps is 0.4 calls/sec against the ~0.8/sec measured in test 1. Half the account's budget from one process, and 20× a transient cluster poll after this change.ListStepsalready returns each step's status. Needs production CloudTrail to know whether it is implicated.- Fleet scale. All tests were one runner against a saturated budget, not many runners sharing one. By arithmetic the launch phase saturates at ~48 concurrent launches rather than ~18, and decorrelated rather than in lockstep — calculation, not measurement.
- The specific production clusters from the incident, and what else calls EMR in that account: both need production credentials.
5418da6 to
11b7820
Compare
Production runs were reported as failed after a single ThrottlingException. None had failed: clusters had come up, or terminated cleanly, and we exited anyway — in the launch cases leaving a cluster running its steps unwatched. Live testing since reproduced this exactly: the old binary abandoned a cluster that went on to reach ALL_STEPS_COMPLETED four minutes later. The waiters swallow API errors and keep polling, so the throttled call was never a poll. It was the describe around one: the existence check before it, or the re-describe after it, since waiter.Wait discards its response and the status had to be fetched again. WaitForOutput hands back the response that satisfied the acceptor, so that second call is gone from the happy path entirely; what remains covers only a waiter that errored or timed out, and a TERMINATED that EMR has not yet attached a reason to. Both transient waits lose their existence check as well. Their callers pass a jobflow ID RunJobFlow has just returned, so it guards against nothing, and the waiter's first poll is issued with no delay — it asks the same question and resolves the same terminal states through its own acceptors. Dropping it removes a duplicate call from the path where call volume is the problem, and turns a throttled first poll into something the waiter absorbs rather than something a retry budget has to outlast. Only `down`'s wait keeps one, since only `down` takes a cluster ID from the operator. Removing the job phase's check also closes a second false failure, one no throttling was needed to reach. That check returned an already-TERMINATED cluster with a nil error whether or not EMR had attached its reason yet, and a reasonless TERMINATED reaches the caller's "cannot be confirmed complete" branch. The waiter's output is now only trusted when the reason is present, and recovered by a retried describe when it is not — and a status recovered that way never downgrades one the launch already classified, since a bootstrap failure often presents as plain TERMINATED and losing that code loses the relaunch. A launch that cannot be read is left alone rather than terminated: not observing RUNNING says nothing about whether the cluster reached it, and killing one mid-step leaves half-written output, which is worse than an unwatched cluster that finishes and self-terminates. Both paths that give up without a status say which cluster they gave up on — and a wait that did see the cluster terminate hands that status back, so nobody is sent after a cluster that has already gone. Waiting is bounded at both ends by what could still change the answer. A throttle, a dropped connection, a timeout or a 5xx gets the whole wait; an answer AWS actually gave us — a role without DescribeCluster, an ID that does not exist — ends it after three polls rather than holding the waiter for its full bound. Waiters warn after five consecutive failed polls and every five after that, counted in polls so the threshold means the same on a 30-90s launch wait as on a 60-300s job one. Six hours of unbroken blindness ends the wait outright, so the job phase cannot spend its fortnight-long bound on a cluster that finished long ago. The same line decides how long a call outside the waiters retries. Failures that might clear are waited out for minutes, because these calls are how a run learns whether it succeeded and each is issued while a cluster is already up or once it has gone. The rest get the few seconds they always had — enough that EMR briefly not recognising a jobflow ID it just issued is survivable, while a bad --emr-cluster is still reported in seconds. That covers ListSteps and DescribeStep, which sit on the transient success path and could equally turn a completed run into a failed one, and TerminateJobFlows, which is idempotent and whose throttling would otherwise fail `down` in three seconds. To reduce the pressure in the first place, waiters poll at 30-90s while a cluster launches and 60-300s during a transient job, rather than a fixed 30s: smithy only jitters when the bounds differ, so every runner had been polling on the same beat against a budget measured at ~18 requests of burst and ~0.8/sec sustained for the whole account. That reduction, not the retries, is what removes load, and the note by the budgets now points at the step-status loop as the larger remaining source. Both EMR clients move to adaptive retry mode, which damps a single process's own burst rather than coordinating anything. Neither raises its attempt count, which is passed per call instead, and only on calls that are safe to repeat: RunJobFlow and AddJobFlowSteps share those clients and no EMR write has an idempotency token, so retrying one whose response was merely lost would launch a second cluster or submit the playbook's steps twice. Three fixes to what `up` tells the operator. A successful launch no longer logs at error level, nor fails when the waiter resolves on RUNNING rather than WAITING. A bootstrap failure that is about to be retried is a warning, matching the transient path, and no longer sleeps before the attempt that gives up. And an `up` that fails after launching names the cluster it left behind — it sets KeepJobFlowAliveWhenNoSteps, so that cluster will not terminate itself, and its ID reached the operator nowhere else. Also drops fetchStateChangeReason, which has had no callers since before e9939d3 and was the last unretried DescribeCluster in the file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| // the cluster becomes visible, without disturbing the decision itself. | ||
| // | ||
| // A waiter treats an API error as "not there yet" and polls again. That is what | ||
| // stops a throttled poll failing a run, and it is why a throttled launch phase is |
There was a problem hiding this comment.
This is version-sensitive. return inner(...) defers the verdict to the SDK's generated acceptor, polls again is only true while that acceptor returns (true, nil) on an API error.
In the current v1.47.0 the acceptor ends in return true, nil (error falls through → re-polls), but as of v1.47.5 it gained a top-level if err != nil { return false, err } so the first throttled poll ends the wait with the error, silently reverting exactly the failure this PR removes. None of the current tests exercise a waiter polling through an API error.
Could we add a test, in the spirit of TestSDKRetryAssumptions, asserting the pinned waiter's acceptor returns (true, nil) on an API error, so a bump crossing v1.47.5 fails the build?
17c8095 to
a7041df
Compare
pollThroughBlindness now decides for itself that a failed poll means "ask again", instead of deferring to the generated waiter acceptor — which returns (true, nil) on an API error in service/emr v1.47.0 but (false, err) from v1.47.5 onward. The acceptors are consulted only when there is a response to inspect. No behaviour change on the pinned version. Also reworks the unreadable-launch test to reach its branch via a cancelled context, and corrects the blind-poll threshold comment, which implied a healthy run never warns. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
a7041df to
dbda780
Compare
|
Oguzhan Unlu (@oguzhanunlu) you're right about the change in behaviour after I've gone a bit further than the test you suggested. I didn't add the assertion you proposed, because once the decision moves it pins something we no longer depend on and would block the upgrade it's meant to enable. |
Oguzhan Unlu (oguzhanunlu)
left a comment
Oguzhan Unlu (oguzhanunlu)
left a comment
There was a problem hiding this comment.
Makes sense!
2 participants
Jira ref: PDP-2824
Production runs were reported as failed after a single ThrottlingException. None had failed: clusters had come up, or terminated cleanly, and we exited anyway — in the launch cases leaving a cluster running its steps unwatched. Live testing since reproduced this exactly: the old binary abandoned a cluster that went on to reach ALL_STEPS_COMPLETED four minutes later.
The waiters swallow API errors and keep polling, so the throttled call was never a poll. It was the describe around one: the existence check before it, or the re-describe after it, since waiter.Wait discards its response and the status had to be fetched again. WaitForOutput hands back the response that satisfied the acceptor, so that second call is gone from the happy path entirely; what remains covers only a waiter that errored or timed out, and a TERMINATED that EMR has not yet attached a reason to.
Both transient waits lose their existence check as well. Their callers pass a jobflow ID RunJobFlow has just returned, so it guards against nothing, and the waiter's first poll is issued with no delay — it asks the same question and resolves the same terminal states through its own acceptors. Dropping it removes a duplicate call from the path where call volume is the problem, and turns a throttled first poll into something the waiter absorbs rather than something a retry budget has to outlast. Only
down's wait keeps one, since onlydowntakes a cluster ID from the operator.Removing the job phase's check also closes a second false failure, one no throttling was needed to reach. That check returned an already-TERMINATED cluster with a nil error whether or not EMR had attached its reason yet, and a reasonless TERMINATED reaches the caller's "cannot be confirmed complete" branch. The waiter's output is now only trusted when the reason is present, and recovered by a retried describe when it is not — and a status recovered that way never downgrades one the launch already classified, since a bootstrap failure often presents as plain TERMINATED and losing that code loses the relaunch.
A launch that cannot be read is left alone rather than terminated: not observing RUNNING says nothing about whether the cluster reached it, and killing one mid-step leaves half-written output, which is worse than an unwatched cluster that finishes and self-terminates. Both paths that give up without a status say which cluster they gave up on — and a wait that did see the cluster terminate hands that status back, so nobody is sent after a cluster that has already gone.
Waiting is bounded at both ends by what could still change the answer. A throttle, a dropped connection, a timeout or a 5xx gets the whole wait; an answer AWS actually gave us — a role without DescribeCluster, an ID that does not exist — ends it after three polls rather than holding the waiter for its full bound. Waiters warn after five consecutive failed polls and every five after that, counted in polls so the threshold means the same on a 30-90s launch wait as on a 60-300s job one. Six hours of unbroken blindness ends the wait outright, so the job phase cannot spend its fortnight-long bound on a cluster that finished long ago.
The same line decides how long a call outside the waiters retries. Failures that might clear are waited out for minutes, because these calls are how a run learns whether it succeeded and each is issued while a cluster is already up or once it has gone. The rest get the few seconds they always had — enough that EMR briefly not recognising a jobflow ID it just issued is survivable, while a bad --emr-cluster is still reported in seconds. That covers ListSteps and DescribeStep, which sit on the transient success path and could equally turn a completed run into a failed one, and TerminateJobFlows, which is idempotent and whose throttling would otherwise fail
downin three seconds.To reduce the pressure in the first place, waiters poll at 30-90s while a cluster launches and 60-300s during a transient job, rather than a fixed 30s: smithy only jitters when the bounds differ, so every runner had been polling on the same beat against a budget measured at ~18 requests of burst and ~0.8/sec sustained for the whole account. That reduction, not the retries, is what removes load, and the note by the budgets now points at the step-status loop as the larger remaining source.
Both EMR clients move to adaptive retry mode, which damps a single process's own burst rather than coordinating anything. Neither raises its attempt count, which is passed per call instead, and only on calls that are safe to repeat: RunJobFlow and AddJobFlowSteps share those clients and no EMR write has an idempotency token, so retrying one whose response was merely lost would launch a second cluster or submit the playbook's steps twice.
Three fixes to what
uptells the operator. A successful launch no longer logs at error level, nor fails when the waiter resolves on RUNNING rather than WAITING. A bootstrap failure that is about to be retried is a warning, matching the transient path, and no longer sleeps before the attempt that gives up. And anupthat fails after launching names the cluster it left behind — it sets KeepJobFlowAliveWhenNoSteps, so that cluster will not terminate itself, and its ID reached the operator nowhere else.Also drops fetchStateChangeReason, which has had no callers since before e9939d3 and was the last unretried DescribeCluster in the file.