Implements InstancesV2 interface

[mmerkes]

What type of PR is this?

/kind cleanup
/kind feature

What this PR does / why we need it:

The InstancesV2 interface is the path forward for cloud providers. To enable new changes, like providing custom labels, we need to migrate to InstancesV2 as they won't be implemented in the original Instances interface.

This change is needed to prepare to support #300

Which issue(s) this PR fixes:
N/A

Special notes for your reviewer:

I've added unit tests here and the automation should run. I'll do some manual testing with an EKS cluster, but if you have suggestions on things I should test specifically, let me know.

Does this PR introduce a user-facing change?:

Migrate to InstancesV2 interface

[k8s-ci-robot]

This issue is currently awaiting triage.

If cloud-provider-aws contributors determine this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[mmerkes]

/assign @cartermckinnon @dims

[dims]

is return nil, err enough?

[mmerkes]

Maybe I'm wrong, but I thought you needed to return an empty object with Go... I might also be conflating with another language.

[cartermckinnon]

Sometimes folks use signatures that return (struct, error) instead of (*struct, error). The former forces you to return an empty struct in this case, while the latter lets you return nil, err.

The caller should be paying attention to your err, so I think nil is some nice encouragement :) (they'll panic if they don't have error handling)

[mmerkes]

Tx for being big brained @cartermckinnon

[cartermckinnon]

There should already be an impl for this in aws.go, and the provider ID should be suffixed by the AZ

[mmerkes]

One would think that... But it's not.

the provider ID should be suffixed by the AZ

Ya, I knew I needed to update this, but ran out of time. I just copied the code from k8s/k8s to get the provider ID, so I was a little confused by it. I'll dig in further and make sure it returns the right output with some unit tests.

[mmerkes]

Ok, I was a little confused by this because I copied the logic upstream and didn't totally close the loop on it. However, looks like this is actually the right behavior. In totally obvious fashion, instanceID, in this case, is actually NOT the instance ID. It's the instance ID plus the zone we tack on the provider ID...

cloud-provider-aws/pkg/providers/v1/aws.go

Lines 1773 to 1788 in 2fc845d

	func (c *Cloud) InstanceID(ctx context.Context, nodeName types.NodeName) (string, error) {
	// In the future it is possible to also return an endpoint as:
	// <endpoint>/<zone>/<instanceid>
	if c.selfAWSInstance.nodeName == nodeName {
	return "/" + c.selfAWSInstance.availabilityZone + "/" + c.selfAWSInstance.awsID, nil
	}
	inst, err := c.getInstanceByNodeName(nodeName)
	if err != nil {
	if err == cloudprovider.InstanceNotFound {
	// The Instances interface requires that we return InstanceNotFound (without wrapping)
	return "", err
	}
	return "", fmt.Errorf("getInstanceByNodeName failed for %q with %q", nodeName, err)
	}
	return "/" + aws.StringValue(inst.Placement.AvailabilityZone) + "/" + aws.StringValue(inst.InstanceId), nil
	}

[olemarkus]

Am I missing something, or does this function ignore the provider ID already set on the node resource?
At least some installers set the provider ID through the kubelet flag, which in turn sets it on the node resource as it is created.

[cartermckinnon]

Hm looks like the provider ID returned here won't overwrite an existing one: https://github.com/kubernetes/kubernetes/blob/5031ae11590748f247c15c81f9f35390a786104b/staging/src/k8s.io/cloud-provider/controllers/node/node_controller.go#L524-L530

But we could save some EC2 calls if we use the existing provider ID.

[mmerkes]

Probably won't save any calls to EC2 because we use cached values and will need the describe values for the other fields, but getting it directly, if it's set, makes sense.

[mmerkes]

/retest

[dims]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, olemarkus

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [dims,olemarkus]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[olemarkus]

/lgtm