Fix EC2 throttling during computenode launch is not retried and is reported as ICE when using multi InstanceTypes or SubnetIds - #734
Conversation
…acity
CreateFleet returns one error entry per launch template override, so with more than one instance type or
subnet a single failure also produces an ("UnfulfillableCapacity", "Failed to fulfill capacity. Please
review errors in the response.") entry for every other override. The pre-existing `len(err_list) == 1`
condition therefore never held, and every cause was flattened into a hardcoded
InsufficientInstanceCapacity, which is in EC2_ICE_ERROR_CODES and fails the compute resource over for
insufficient_capacity_timeout.
Drop those entries before choosing what to report, keeping the response as is when it carries nothing else.
Other UnfulfillableCapacity messages, notably the MinTargetCapacity one that all-or-nothing scaling
produces, do describe a cause and are kept. Throttling is preferred over any remaining cause so that a
launch which only needs a retry is not abandoned. Single-override compute resources are unaffected.
A throttled batch now consumes the retry budget on launch_ec2_instances (10 attempts, 810s of backoff),
which can delay _store_assigned_hostnames past the window compute nodes use to read their hostname from
DynamoDB.
hehe7318
changed the title
hehe7318
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #734 +/- ##
===========================================
+ Coverage 91.06% 91.09% +0.02%
===========================================
Files 20 20
Lines 3213 3222 +9
===========================================
+ Hits 2926 2935 +9
Misses 287 287
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
|
||
| # An override that CreateFleet did not fulfill because another override failed is reported with this exact | ||
| # code and message pair, which points at the real error rather than being one. | ||
| UNFULFILLED_OVERRIDE_ERROR = ( |
There was a problem hiding this comment.
What does "override" mean here
There was a problem hiding this comment.
It means the launch template "override".
| real_errors = [ | ||
| err | ||
| for err in err_list | ||
| if (err.get("ErrorCode"), err.get("ErrorMessage")) != UNFULFILLED_OVERRIDE_ERROR |
There was a problem hiding this comment.
UnfulfillableCapacity is real error to right?
According to https://docs.aws.amazon.com/ec2/latest/devguide/errors-overview.html:
At this time there isn't enough spare capacity to fulfill your request for Spot Instances. You can wait a few minutes to see whether capacity becomes available for your request. Alternatively, create a more flexible request. For example, include additional instance types, include additional Availability Zones, or use the capacity-optimized allocation strategy.
Is it safe to drop it?
There was a problem hiding this comment.
Yes it's safe. The filter matches the code and the error message as a pair, not the code
alone.
The code+message pair we dropped is:
("UnfulfillableCapacity", "Failed to fulfill capacity. Please review errors in the response.")
Every other UnfulfillableCapacity message passes through untouched, including
"Unable to fulfill request due to MinTargetCapacity constraints. Please adjust your request and try again.",
which is what all-or-nothing produces and is a genuine cause.
And the message itself is a evidence. "Please review errors in the response" means something if the response carries another error to review. As the sole entry it would be self-referential.
| # A single cause is normally left. Should there be several, prefer throttling as a safety net: it is | ||
| # the only cause that resolves on its own, and reporting it as insufficient capacity would instead | ||
| # fail the compute resource over. | ||
| throttling = next( |
There was a problem hiding this comment.
I agree with the safety net approach, but not on limiting it to throttling.
With your change, we surface throttling if we have a mix of root causes because we want to favor retryable errors and I agree. However, throttling is not the only retriable error, .e.g internal errors are retriable as well.
There was a problem hiding this comment.
That's a real behavior change and we should not do it without clear decision doc. Other error code will be handled in other code path. It's safe to only handle and retry the throttling error here - the exponential backoff is designed only for it.
Also I raise a doubt here:
For example VCpuLimitExceed error, I don't think we should retry, but we should fail, requeue the job, print the error in log.
There was a problem hiding this comment.
I agree that we should not limit it to throttling. If there are other errors due to real issues, it does not make sense to retry when it will fail anyways.
Also, it there is more than one error code, then the current behavior is to also not retry. I think we should keep this existing behavior.
| ) | ||
| if throttling: | ||
| raise LaunchInstancesError(throttling.get("ErrorCode"), throttling.get("ErrorMessage")) | ||
| # Normally a single cause is left. Reporting the first one of several is a second safety net: the |
There was a problem hiding this comment.
I imagine that you are keeping this approach of bubbling up only the first error to keep the fix minimal and I agree on the overall minimization approach. However:
- Why do we still want to bubble up only the first error code?
- The alternative is to let
LaunchInstancesErrorcontain all the types of errors so that the upstream logic can take a decision on a complete observation. Why not doing that?
Example: if the request fails due to ICE + another unretriable error E2, but E2 is listed first, we will bubble up only E2. What would be the consequence?
There was a problem hiding this comment.
Just to clarify - if everything goes correct and well, after the fix, after removing the duplicated UnfulfillableCapacity, the err_list should only contain 1 error. I added a safety net here in case there's edge case.
Why do we still want to bubble up only the first error code?
To introduce the minimal changes, because node package is sensitive and I want to avoid any regression possibility.
The alternative is to let LaunchInstancesError contain all the types of errors so that the upstream logic can take a decision on a complete observation. Why not doing that
Good question, I tried once, but other code chain also use this returned error. To do it we need to restructure and it introduce too much risky code changes. To solve an edge case, I don't think it deserve.
if the request fails due to ICE + another unretriable error E2, but E2 is listed first, we will bubble up only E2. What would be the consequence?
Our code logic will treat it as an ICE issue and trigger fast failover mechanism. Which aligns with today's(3.16.0) behavior. If it's the opposite, another unretriable error E1 + ICE, we fail and requeue job without retry, which fix today's wrong behavior to the correct behavior.
There was a problem hiding this comment.
If we are using a fallback to always retry if there is throttling, why bubble up into one error code. It seems like the behavior would be the same whether we do that or not.
4 participants
Description of changes
Customer impact
Since #578, EC2 throttling during a compute node scale-up is never retried, and every launch failure is instead reported as insufficient capacity.
Preconditions: a compute resource with more than one instance type or more than one subnet, and a scale-up large enough to exhaust the
RunInstancesresource token bucket, which holds 1000 instances and refills at 2/s. See the number in ref link.On
best-effort(the default): a single throttled call disables the whole compute resource, not only the batch that failed. Measured withMaxCount 3000andsbatch -N 1500: 500 nodes went DOWN and the remaining 1500 gotTemporarily disabling node due to insufficient capacity— 2000 of 3000 nodes unusable for 600s (insufficient_capacity_timeout), with the job stuckPENDING.On
all-or-nothing: the compute resource cannot scale up reliably. Every round releases the instances already launched and starts over, so a large scale-up would fail.Root cause
CreateFleetreturns one entry inErrorsper launch template override, so a compute resource with more than one instance type or more than one subnet gets an entry for every override that was not fulfilled, not only for the one that actually failed. Those extra entries all carry the same pair:which points at the real error rather than being one.
With 36 overrides a single failure therefore arrives as 36 entries, so the pre-existing
len(err_list) == 1condition inEc2CreateFleetManager._launch_instancesnever holds. NoLaunchInstancesErroris raised, andInstanceManager._launch_ec2_instancesfalls back to a hardcodedInsufficientInstanceCapacity. Three things follow:launch_ec2_instancesalready implements the exponential backoff the EC2 documentation recommends, sized to wait for the bucket to refill a fulllaunch_max_batch_sizebatch, but no exception ever reaches it. The batch is abandoned after a single attempt even though retrying is all it needed.InsufficientInstanceCapacityis inSlurmNode.EC2_ICE_ERROR_CODES, soclustermgtdtreats the failure as a capacity shortage and disables the whole compute resource, not just the batch that failed.VcpuLimitExceeded, anUnsupportedinstance type or anUnauthorizedOperationis all flattened into insufficient capacity.The fix
len(err_list) == 1condition works again for multi-override compute resources. The response is left untouched when those entries are all it carries, so the worst case is today's behaviour.Tests
References
Checklist
developadd the branch name as prefix in the PR title (e.g.[release-3.6]).Please review the guidelines for contributing and Pull Request Instructions.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.