Cluster-api-provider-aws: ProviderID more consistent with kube manager cloud provider

[enxebre]

/kind bug

What steps did you take and what happened:
Create a machine.
See the providerID. This value https://github.com/kubernetes-sigs/cluster-api-provider-aws/blob/22be31dd76da04409c425c77e2967db327f409a4/controllers/awsmachine_controller.go#L406

What did you expect to happen:
providerID to look consistent with what the cloud provider would set in the node one.

Anything else you would like to add:
We could do something similar to https://github.com/kubernetes/kubernetes/blob/103e926604de6f79161b78af3e792d0ed282bc06/staging/src/k8s.io/legacy-cloud-providers/aws/aws.go#L1627

https://github.com/kubernetes/kubernetes/blob/103e926604de6f79161b78af3e792d0ed282bc06/staging/src/k8s.io/legacy-cloud-providers/aws/aws.go#L1813

Environment:

• Cluster-api-provider-aws version:
• Kubernetes version: (use kubectl version):
• OS (e.g. from /etc/os-release):

[detiber]

Just to double check that this wouldn't affect us when we go to consume the external aws cloud provider eventually it looks like it is currently consuming the legacy-cloud-provider so should be setting the same values currently.

I'm +1 to this change assuming that we can do it in a way that does not break existing clusters.

[detiber]

/priority important-soon
/milestone v0.5.x
/help

[k8s-ci-robot]

@detiber:
This request has been marked as needing help from a contributor.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

/priority important-soon
/milestone v0.5.x
/help

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.

[elmiko]

doing some testing and exploration with the autoscaler, i think this issue is also showing up there as well. essentially the autoscaler is looking at these values and seeing a mismatch which causes it to think that the nodes are unregistered.

@JoelSpeed noted this issue with the PR mentioned above, i just wanted to add a comment here to help add some reference.

[randomvariable]

Maybe worth creating an umbrella "epic" for autoscaler support with reference to this issue @detiber ?

[ncdc]

Given that CAPI has its own utility for parsing provider IDs, and IIRC we match solely on provider and instance ID (ignoring any intermediate data in the string, such as region/zone), I _think_ this change should be backwards compatible.

[JoelSpeed]

One question that came to mind when thinking about a fix for this, would we want to fix IDs for existing machines in the cluster, or should we just put the new format for IDs on the new machines

Looks from the code we overwrite the providerID on every reconcile so leaving old IDs alone would involve some extra work, just not sure if that would break anything
https://github.com/kubernetes-sigs/cluster-api-provider-aws/blob/22be31dd76da04409c425c77e2967db327f409a4/controllers/awsmachine_controller.go#L406

[elmiko]

One question that came to mind when thinking about a fix for this, would we want to fix IDs for existing machines in the cluster, or should we just put the new format for IDs on the new machines

in terms of the extended autoscaler issues, i think we could almost skip fixing the broken IDs as the autoscaler will quickly cycle out those machines as "unregistered nodes". this might be slightly disruptive, but at the least it should be safe.

[detiber]

Looks from the code we overwrite the providerID on every reconcile so leaving old IDs alone would involve some extra work, just not sure if that would break anything

I wouldn't anticipate it breaking anything, but we could always add a release note for the change in case there exists a user that is relying on the current value and would need to update their integrations

[alexander-demichev]

/assign