ec2: Added NIC support for RunInstances

ec2: Added NIC support for RunInstances

This extends RunInstances options to include
a list of NetworkInterfaceSpec options. They
allow specifying NICs to attach to instances
at launch time (either existing or new NICs).

It also adds SubnetId, VPCId, SourceDestCheck,
and NetworkInterfaces fields to the Instance
type, so Instances() and RunInstances() can
return the extended VPC-related information
for an instance.

If any NetworkInterfaceSpec options are provided,
or SubnetID is given, RunInstances() will use the
latest AWS API version (2013-10-15), otherwise it
uses the default version (2011-12-15).

Modified terminateInstances() test helper to wait
and retry when running against live EC2 servers,
to make sure the instances are really gone and
not left hanging after the test (leaking related
resources with them). In order for this to work
with the local testing server, a slight change
was made - when an instance is terminated, the
next time you fetch it with Instances(), it will
report "terminated" (thus simulating the real
state transition).

Added vpc-id and subnet-id filters to ec2test's
Instance and securityGroup types, and also changed
TestGroupFiltering and TestInstanceFiltering to
include tests for them.

Test double changed to support VPC security groups
and verify subnet ID in runInstances, as EC2 does.
Added NewInstancesVPC() method with vpcId and subnetId,
so VPC-enabled instances can be created in the test
server.

Added a TestRunInstancesVPC live test for the new
functionality (only live, because it's not worth
it to change the test double to create NICs in
runInstances()).

https://code.launchpad.net/~dimitern/goamz/vpc-instance-addons/+merge/205148

Requires: https://code.launchpad.net/~dimitern/goamz/nic-api-calls/+merge/204912

(do not edit description out of merge proposal)

[dimitern, 2014-02-06 12:42:03]

Please take a look.

[dimitern, 2014-02-06 14:24:29]

Please take a look.

[rog, 2014-02-06 14:56:14]

LGTM with some suggestions below.

https://codereview.appspot.com/60620043/diff/20001/ec2/ec2.go
File ec2/ec2.go (right):

https://codereview.appspot.com/60620043/diff/20001/ec2/ec2.go#newcode299
ec2/ec2.go:299: // given, a VPC-enabled instances will be started.  Cannot
specify
s/instances/instance/

But this whole paragraph is a bit hard to read.
Can't we just leave it out and let the user
get the gory details from the amz docs, like
most of the other params?

https://codereview.appspot.com/60620043/diff/20001/ec2/ec2t_test.go
File ec2/ec2t_test.go (right):

https://codereview.appspot.com/60620043/diff/20001/ec2/ec2t_test.go#newcode141
ec2/ec2t_test.go:141: c.Check(err, IsNil, Commentf("%d INSTANCES LEFT
RUNNING!!!", len(ids)))
Do we really need to fail the test if we can't terminate the instances
immediately?

https://codereview.appspot.com/60620043/diff/20001/ec2/ec2t_test.go#newcode164
ec2/ec2t_test.go:164: idsLeft[inst.InstanceId] = false
if you did:

    delete(idsLeft, inst.InstanceId)

you wouldn't need the

    if left {

test in the range loop below.

https://codereview.appspot.com/60620043/diff/20001/ec2/ec2t_test.go#newcode653
ec2/ec2t_test.go:653: // some of the reset depend on it, so they can't be
deleted before
s/the some of the reset/some of the rest/ ?

I don't understand how the comment applies to the statement
though - g[0] is first in the deleteGroups call, not g[3].

[dimitern, 2014-02-06 15:52:19]

Please take a look.

https://codereview.appspot.com/60620043/diff/20001/ec2/ec2.go
File ec2/ec2.go (right):

https://codereview.appspot.com/60620043/diff/20001/ec2/ec2.go#newcode299
ec2/ec2.go:299: // given, a VPC-enabled instances will be started.  Cannot
specify
On 2014/02/06 14:56:14, rog wrote:
> s/instances/instance/
> 
> But this whole paragraph is a bit hard to read.
> Can't we just leave it out and let the user
> get the gory details from the amz docs, like
> most of the other params?

Done.

https://codereview.appspot.com/60620043/diff/20001/ec2/ec2t_test.go
File ec2/ec2t_test.go (right):

https://codereview.appspot.com/60620043/diff/20001/ec2/ec2t_test.go#newcode141
ec2/ec2t_test.go:141: c.Check(err, IsNil, Commentf("%d INSTANCES LEFT
RUNNING!!!", len(ids)))
On 2014/02/06 14:56:14, rog wrote:
> Do we really need to fail the test if we can't terminate the instances
> immediately?

Yes we do! We depend on TerminateInstances being successful. If it returns an
error there's no point in the loop below. I even changed it to Assert, so this
more obvious.

https://codereview.appspot.com/60620043/diff/20001/ec2/ec2t_test.go#newcode164
ec2/ec2t_test.go:164: idsLeft[inst.InstanceId] = false
On 2014/02/06 14:56:14, rog wrote:
> if you did:
> 
>     delete(idsLeft, inst.InstanceId)
> 
> you wouldn't need the
> 
>     if left {
> 
> test in the range loop below.

Done.

https://codereview.appspot.com/60620043/diff/20001/ec2/ec2t_test.go#newcode653
ec2/ec2t_test.go:653: // some of the reset depend on it, so they can't be
deleted before
On 2014/02/06 14:56:14, rog wrote:
> s/the some of the reset/some of the rest/ ?
> 
> I don't understand how the comment applies to the statement
> though - g[0] is first in the deleteGroups call, not g[3].

By "first" I meant "first of the set g[3], g[1], g[2]". I'll rephrase.

[dimitern, 2014-02-07 12:59:30]

Please take a look.

[niemeyer, 2014-02-12 14:49:53]

LGTM, but please ask for at least one more review.

https://codereview.appspot.com/60620043/diff/60001/ec2/ec2.go
File ec2/ec2.go (right):

https://codereview.appspot.com/60620043/diff/60001/ec2/ec2.go#newcode232
ec2/ec2.go:232: SecondaryPrivateIPsCount int
s/IPsCount/IPCount/, as per the previous point?

[niemeyer, 2014-02-12 14:52:17]

https://codereview.appspot.com/60620043/diff/60001/ec2/ec2.go
File ec2/ec2.go (right):

https://codereview.appspot.com/60620043/diff/60001/ec2/ec2.go#newcode204
ec2/ec2.go:204: // NetworkInterfaceSpec encapsulates options for a single
network
Ah, and as we discussed, this type should be something along the lines of

    RunNetworkInterface

It's an unfortunate name, but I cannot come up with anything better that is not
absurdly long.

For the record, the generic name "NetworkInterfaceSpec" sounds like a problem
because this is actually a set of options that only really make sense in the
specific context of RunInstances. We already have another bag of data that only
really makes sense in the context of CreateNetworkInterface.  If we need one
more, will we name it NetworkInterface..Data?..Stuff?

[natefinch, 2014-02-12 16:26:29]

LGTM, just a couple nice-to-have changes

https://codereview.appspot.com/60620043/diff/60001/ec2/ec2.go
File ec2/ec2.go (right):

https://codereview.appspot.com/60620043/diff/60001/ec2/ec2.go#newcode305
ec2/ec2.go:305: if options.SubnetId != "" || len(options.NetworkInterfaces) > 0
{
I'd like to have this extracted into a standalone function so it can be tested
by itself.

https://codereview.appspot.com/60620043/diff/60001/ec2/ec2.go#newcode362
ec2/ec2.go:362: for i, ni := range options.NetworkInterfaces {
I'd really prefer if the internals to this and the loop above were factored out
into separate functions. I know you didn't write the loop above, but it would
cut down on the size of this already pretty large function.

https://codereview.appspot.com/60620043/diff/60001/ec2/ec2t_test.go
File ec2/ec2t_test.go (right):

https://codereview.appspot.com/60620043/diff/60001/ec2/ec2t_test.go#newcode141
ec2/ec2t_test.go:141: c.Assert(err, IsNil, Commentf("%d INSTANCES LEFT
RUNNING!!!", ids))
either len(ids) or %v here, right?

https://codereview.appspot.com/60620043/diff/60001/ec2/ec2t_test.go#newcode146
ec2/ec2t_test.go:146: Total: 10 * time.Minute,
do we really want tests that might wait 10 minutes?

[dimitern, 2014-02-12 17:05:57]

*** Submitted:

ec2: Added NIC support for RunInstances

This extends RunInstances options to include
a list of NetworkInterfaceSpec options. They
allow specifying NICs to attach to instances
at launch time (either existing or new NICs).

It also adds SubnetId, VPCId, SourceDestCheck,
and NetworkInterfaces fields to the Instance
type, so Instances() and RunInstances() can
return the extended VPC-related information
for an instance.

If any NetworkInterfaceSpec options are provided,
or SubnetID is given, RunInstances() will use the
latest AWS API version (2013-10-15), otherwise it
uses the default version (2011-12-15).

Modified terminateInstances() test helper to wait
and retry when running against live EC2 servers,
to make sure the instances are really gone and
not left hanging after the test (leaking related
resources with them). In order for this to work
with the local testing server, a slight change
was made - when an instance is terminated, the
next time you fetch it with Instances(), it will
report "terminated" (thus simulating the real
state transition).

Added vpc-id and subnet-id filters to ec2test's
Instance and securityGroup types, and also changed
TestGroupFiltering and TestInstanceFiltering to
include tests for them.

Test double changed to support VPC security groups
and verify subnet ID in runInstances, as EC2 does.
Added NewInstancesVPC() method with vpcId and subnetId,
so VPC-enabled instances can be created in the test
server.

Added a TestRunInstancesVPC live test for the new
functionality (only live, because it's not worth
it to change the test double to create NICs in
runInstances()).

R=rog, niemeyer, nate.finch
CC=
https://codereview.appspot.com/60620043

https://codereview.appspot.com/60620043/diff/60001/ec2/ec2.go
File ec2/ec2.go (right):

https://codereview.appspot.com/60620043/diff/60001/ec2/ec2.go#newcode204
ec2/ec2.go:204: // NetworkInterfaceSpec encapsulates options for a single
network
On 2014/02/12 14:52:17, niemeyer wrote:
> Ah, and as we discussed, this type should be something along the lines of
> 
>     RunNetworkInterface
> 
> It's an unfortunate name, but I cannot come up with anything better that is
not
> absurdly long.
> 
> For the record, the generic name "NetworkInterfaceSpec" sounds like a problem
> because this is actually a set of options that only really make sense in the
> specific context of RunInstances. We already have another bag of data that
only
> really makes sense in the context of CreateNetworkInterface.  If we need one
> more, will we name it NetworkInterface..Data?..Stuff?

Done.

https://codereview.appspot.com/60620043/diff/60001/ec2/ec2.go#newcode232
ec2/ec2.go:232: SecondaryPrivateIPsCount int
On 2014/02/12 14:49:53, niemeyer wrote:
> s/IPsCount/IPCount/, as per the previous point?

Done.

https://codereview.appspot.com/60620043/diff/60001/ec2/ec2.go#newcode305
ec2/ec2.go:305: if options.SubnetId != "" || len(options.NetworkInterfaces) > 0
{
On 2014/02/12 16:26:29, nate.finch wrote:
> I'd like to have this extracted into a standalone function so it can be tested
> by itself.

Extracted and added a test.

https://codereview.appspot.com/60620043/diff/60001/ec2/ec2.go#newcode362
ec2/ec2.go:362: for i, ni := range options.NetworkInterfaces {
On 2014/02/12 16:26:29, nate.finch wrote:
> I'd really prefer if the internals to this and the loop above were factored
out
> into separate functions. I know you didn't write the loop above, but it would
> cut down on the size of this already pretty large function.

I've extracted both loops in separate helpers.

https://codereview.appspot.com/60620043/diff/60001/ec2/ec2t_test.go
File ec2/ec2t_test.go (right):

https://codereview.appspot.com/60620043/diff/60001/ec2/ec2t_test.go#newcode141
ec2/ec2t_test.go:141: c.Assert(err, IsNil, Commentf("%d INSTANCES LEFT
RUNNING!!!", ids))
On 2014/02/12 16:26:29, nate.finch wrote:
> either len(ids) or %v here, right?

Oops, thanks! Done.

https://codereview.appspot.com/60620043/diff/60001/ec2/ec2t_test.go#newcode146
ec2/ec2t_test.go:146: Total: 10 * time.Minute,
On 2014/02/12 16:26:29, nate.finch wrote:
> do we really want tests that might wait 10 minutes?

Yes, I think so. I've adjusted this value after numerous live test runs.
Sometimes EC2 just gets stubborn and an instance is stuck in "shutting-down"
state for like 10m, and you can't even terminate it from the EC2 console. So
shutting down instances cleanly could take a while longer than other entities.
We'll never wait that long when running the "fake live" tests or when EC2 works
as expected.