[discovery-ec2] Plugin doesn't retry EC2 describe instance calls

[Bukhtawar]

Problem Description

Discovery EC2 doesn't retry EC2 describe calls on throttle or exceptions due to NO_RETRY_CONDITION which has the potential to make the throttling worse. Although there is a logic to exponentially backoff in such cases, backoff doesn't kick in.

elasticsearch/plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/AwsEc2ServiceImpl.java

Lines 82 to 90 in 7203cee

	RetryPolicy.RetryCondition.NO_RETRY_CONDITION,
	(originalRequest, exception, retriesAttempted) -> {
	// with 10 retries the max delay time is 320s/320000ms (10 * 2^5 * 1 * 1000)
	logger.warn("EC2 API request failed, retry again. Reason was:", exception);
	return 1000L * (long) (10d * Math.pow(2, retriesAttempted / 2.0d) * (1.0d + rand.nextDouble()));
	},
	10,
	false);
	clientConfiguration.setRetryPolicy(retryPolicy);

AWS reference

[elasticmachine]

Pinging @elastic/es-distributed (:Distributed/Discovery-Plugins)

[Bukhtawar]

Do you mind if I can raise a PR for the same @original-brownbear . It's a small change I'm fine either ways

[original-brownbear]

@Bukhtawar I'm not sure this is entirely a small change. It seems we're not just using the EC2 SDK to make calls to the EC2 metadata service but also use org.elasticsearch.discovery.ec2.Ec2NameResolver#resolve which just works from java.net.URL#openConnection() to load single metadata values. Wouldn't we have to fix those as well to really get proper retrying here? It seems like we'd only be fixing part of the calls to retry if we just did something like move to using the default retry condition in the code you pasted?

(I ran into this when I tried to test retries by faking 500s in the EC2 mock we use, if we only fixed this one spot which I'm not sure is all that valueable it still wouldn't be entirely trivial to test the fix either would it or do you have an idea?)

[Bukhtawar]

@original-brownbear Please correct me if I misunderstood your point but aren't the two calls made to different endpoint and hence logically different entities.

1. DescribeInstance - EC2 control plane(via SDK)
2. Instance Metadata - http://169.254.169.254/latest/meta-data/ (via URL#openConnection())

While for (1) we need retries for cases when a large cluster(100 node or more) loses a master and then PeerFinder on all nodes do a per second EC2 call causing account level limits to be breached.
(2) might be different than this though(haven't encountered throttling so far for this) and I certainly think (1) is valueable and worth the change.

Testing
We tested (1) by restarting masters on a 150 node cluster, which lead to more calls getting throttled without the fix. While DEFAULT_RETRY ensured back off kicks in. We could see from the timestamp of the exception stack trace. Having said this, we are working on a better test plan

[original-brownbear]

Thanks @Bukhtawar, you are correct I think. (Thanks @DaveCTurner for chiming in here!). I think I found a way for a proper test and was will raise a PR for this one shortly. We have a bunch of HTTP mocking infrastructure from the snapshot repository tests that we can reuse here and I think I was able to craft a valid test from that :)

[Bukhtawar]

Thanks @original-brownbear and @DaveCTurner. I had one more proposal wrt to the backoff strategy. It is to use the SDKDefaultBackoffStrategy which has equal/full jitters configured for throttle/non-throttle exceptions in the newer AWS SDK versions. Defaults for some configurations can be overridden as below.

RetryPolicy retryPolicy = new RetryPolicy(PredefinedRetryPolicies.DEFAULT_RETRY_CONDITION,
            (originalRequest, exception, retriesAttempted) -> {
                logger.warn("EC2 API request failed, retry again. Reason was:", exception);
                return new PredefinedBackoffStrategies.SDKDefaultBackoffStrategy(500,
                    1000,
                    200 * 1000).delayBeforeNextRetry(originalRequest, exception, retriesAttempted);

            },
            10,
            false);
        clientConfiguration.setRetryPolicy(retryPolicy);