Handle error case when no instances returned after calling AWS RunInstances

[elliotrushton]

Description

Handles the error case that can arise when requests to run new AWS instances returns no errors but an empty struct of results.

Fixes #4746 - where there's an unhandled case where calls to AWS RunInstances returns an empty list of reservations and no error, which results in a panic and leaves half-created resources.

This PR fixes the problem in a similar way to how this case is handled in Deploy Kit https://github.com/docker/deploykit/blob/master/pkg/provider/aws/plugin/instance/ec2_instance.go#L272

Related issue(s)

#4746

[GordonTheTurtle]

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "patch-1" [email protected]:elliotrushton/machine.git somewhere
$ cd somewhere
$ git commit --amend -s --no-edit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

[jbensso]

I'm really hoping this change is accepted. My organization keeps running into this issue using docker-machine on AWS t3-medium instances. A fix would save time and lots of money!

[bogklug]

It seems at least one other place needs to be changed. EC2 DescribeInstances call can return empty reservation set when waiting for instance ip (called from mcnutils.WaitFor(d.instanceIpAvailable)).

--- a/drivers/amazonec2/amazonec2.go
+++ b/drivers/amazonec2/amazonec2.go
@@ -947,6 +947,9 @@ func (d *Driver) getInstance() (*ec2.Instance, error) {
        if err != nil {
                return nil, err
        }
+       if instances == nil || len(instances.Reservations) < 1 || len(instances.Reservations[0].Instances) < 1 {
+               return nil, fmt.Errorf("Error getting instance: %v", errors.New("Unexpected AWS API response"))
+       }
        return instances.Reservations[0].Instances[0], nil
 }

[romanr]

Please accept the PR. This bug is eating money for a lot of organisations!

[dahlb]

this is the code for a lamda function we are using that stops us losing any more money until the bug is fixed (it's very simple if a instance is started of the type we use for runners and it doesn't get a tag within a few minutes terminate the instance)

node 10

const AWS = require('aws-sdk');
const ec2 = new AWS.EC2();

const delay = async () => {
return new Promise((resolve) => {
setTimeout(function () {
resolve();
}, 60000);
});
};

const terminate = async (ec2InstanceId) => {
var params = {
InstanceIds: [
ec2InstanceId
]
};
const data = await ec2.terminateInstances(params).promise();
return data;
};

const instanceType = async (ec2InstanceId) => {
const params = {
Attribute: "instanceType",
InstanceId: ec2InstanceId
};
const data = await ec2.describeInstanceAttribute(params).promise();
return data['InstanceType']['Value'];
};

const tags = async (ec2InstanceId) => {
var params = {
Filters: [
{
Name: "resource-id",
Values: [
ec2InstanceId
]
}
]
};
const data = await ec2.describeTags(params).promise();
return data['Tags'];
};

exports.handler = async (event) => {
console.info("EVENT\n" + JSON.stringify(event, null, 2));
const instanceId = event.detail['instance-id'];
console.log("InstanceId: " + instanceId);
const type = await instanceType(instanceId);
console.log("Type: " + type);
if (type !== 't3.xlarge') {
console.log("ignoring not runner type");
return;
}
await delay();
const localTags = await tags(instanceId);
console.log("Tags Length: " + localTags.length);
if (localTags.length > 0) {
return;
}
console.log("Terminating: " + instanceId);
await terminate(instanceId);
};

this is the event pattern we trigger on
{
"detail-type": [
"EC2 Instance State-change Notification"
],
"source": [
"aws.ec2"
],
"detail": {
"state": [
"running"
]
}
}