Cluster-api-provider-aws: Instances should be removed from LB when they're being shut down

[benmoss]

/kind bug

What steps did you take and what happened:
While doing a control plane upgrade workflow I was continuously doing kubectl get nodes on the new cluster. I saw that I got periodic timeouts/failures and I'm pretty sure it's because the instance is being included in the cluster load balancer before it's ready to handle requests. You also can see in the KCP logs these kinds of errors as well:

[capi-kubeadm-control-plane-controller-manager-596d455d75-vz552 manager] E0408 21:11:25.275888      98 controller.go:258] controller-runtime/controller "msg"="Reconciler error" "error"="failed to reconcile the remote kubelet RBAC role: failed to determine if kubelet config rbac role \"kubeadm:kubelet-config-1.17\" already exists: Get \"https://bmo-apiserver-1404960449.us-east-1.elb.amazonaws.com:6443/apis/rbac.authorization.k8s.io/v1/namespaces/kube-system/roles/kubeadm:kubelet-config-1.17?timeout=30s\": http2: server sent GOAWAY and closed the connection; LastStreamID=9369, ErrCode=NO_ERROR, debug=\"\""  "controller"="kubeadmcontrolplane" "request"={"Namespace":"default","Name":"bmo-control-plane"}
[capi-kubeadm-control-plane-controller-manager-596d455d75-vz552 manager] W0408 21:11:44.858497      98 http.go:392] Error reading backend response: unexpected EOF

What did you expect to happen:
It should stay out of the pool until it's ready to serve

Environment:

• Cluster-api-provider-aws version: e2c994b
• Kubernetes version: (use kubectl version): 1.17.4

[detiber]

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

[benmoss]

/assign

[benmoss]

I think this might just be actually an inevitable jitter as a result of there being timeouts / intervals involved in the LB healthcheck. I think it most commonly happens as a control plane node is being removed from the cluster, it is first removed from etcd which causes it to become unresponsive and the load balancer isn't able to react fast enough

The API server has an option --shutdown-delay-duration that should help us here, but until https://github.com/kubernetes/kubernetes/pull/88911 was merged it was buggy and the /readyz endpoint didn't start returning non-200 codes until after the shutdown delay elapsed. Something to look at in the future maybe.

[benmoss]

Note that that would also require us to actually shut down the API server, right I think we just shutdown the machine

[detiber]

@benmoss that is correct, currently we only shutdown the Machine.

[benmoss]

I'd say we should just defer this issue and open another about KCP using --shutdown-delay-duration and shutting down the API servers properly, at which point CAPA should switch its LB healthcheck to check for a 200 on /readyz

[randomvariable]

We did switch CAPV to using /readyz so the change makes sense here too.

[benmoss]

Closing in favor of #1705