cloudprovider: aws: return true on existence check for stopped instances

[sjenning]

xref https://bugzilla.redhat.com/show_bug.cgi?id=1559271
xref openshift/origin#19899

background #45986 (comment)

Basically our customers are hitting this issue where the Node resource is deleted when the AWS instances stop (not terminate). If the instances restart, the Nodes lose any labeling/taints.

Openstack cloudprovider already made this change #59931

fixes #45118 for AWS

Reviewer note: valid AWS instance states are pending | running | shutting-down | terminated | stopping | stopped. There might be a case for returning false for instances in pending and/or terminated state. Discuss!

InstanceID() changes from #45986 credit @rrati

@derekwaynecarr @smarterclayton @liggitt @justinsb @jsafrane @countspongebob

[sjenning]

/release-note-none
/sig-aws

[smarterclayton]

To be clear, what is the behavior of the other cloud providers (vmware, GCP, etc)? Are they like AWS or like OpenStack now?

[sjenning]

#51409 implemented InstanceExistsByProviderID() for most of the cloudproviders. vshpere has since removed its vm.IsActive() check in #55845.

The following DO NOT filter on state:
Azure, CloudStack, GCP, vshpere, Openstack

The following DO filter on state:
Openstack (see update below), AWS

#59931 doesn't actually get the job done for Openstack due to https://github.com/kubernetes/kubernetes/blob/master/pkg/cloudprovider/providers/openstack/openstack_instances.go#L125-L128

[sjenning]

Actually looking at this again, the node controller calls InstanceID() in ensureNodeExistsByProviderID() for built-in cloudprovider usage and falls back to InstanceExistsByProviderID() only in the case of an external cloudprovider (i.e. providerID == "")

https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/cloud/node_controller.go#L446-L467

Removal of the Openstack filtering was done in the external cloudprovider code in kubernetes/cloud-provider-openstack#43.

So #59931 disables filtering for the built-in Openstack provider and kubernetes/cloud-provider-openstack#43 removes it for the external Openstack cloud provider.

I need to adjust InstanceID() for AWS then as well.

[sjenning]

The external AWS cloud provider simply vendors its code from kube atm, so the change to InstanceExistsByProviderID() is needed in-tree so that when they bump their k8s dep, they will get the fix as well.

https://github.com/kubernetes/cloud-provider-aws/blob/master/cmd/aws-cloud-controller-manager/main.go

[sjenning]

Also note that vsphere DOES filter with the built-in but not with the external. What a crazy place this is...

https://github.com/kubernetes/kubernetes/blob/master/pkg/cloudprovider/providers/vsphere/vsphere.go#L755-L764

[derekwaynecarr]

I agree w/ this change, and would say that the usage of taints and labels continues to grow and indirect loss of such attributes on a node have a number of unintended consequences for operators. I think the current ux provided by the AWS cloud provider is confusing end-users and should be changed.

See my prior comment:
#45986 (comment)

[neolit123]

/sig cloud-provider
@kubernetes/sig-aws-misc

[ingvagabund]

/test pull-kubernetes-e2e-kops-aws

[liggitt]

Also see the discussion at #46442 (comment)

This was brought up in sig-aws last week, and is on the agenda for sig-cloud-provider next week

The summary of where the various cloud providers are at currently is a good data point.

[nckturner]

I also agree with this change. So, do the other cloud providers ever return terminated instances in the results? With this change, does AWS need to filter out recently terminated instances?

[liggitt]

Reviewer note: valid AWS instance states are pending | running | shutting-down | terminated | stopping | stopped. There might be a case for returning false for instances in pending and/or terminated state. Discuss!

based on https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/ec2-instance-lifecycle.html, a 'pending' state can occur while starting up a stopped instance, so it should still return true.

I could see returning false for instances in terminated state.

/cc @d-nishi

[k8s-ci-robot]

@liggitt: GitHub didn't allow me to request PR reviews from the following users: d-nishi.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

Reviewer note: valid AWS instance states are pending | running | shutting-down | terminated | stopping | stopped. There might be a case for returning false for instances in pending and/or terminated state. Discuss!

based on https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/ec2-instance-lifecycle.html, a 'pending' state can occur while starting up a stopped instance, so it should still return true.

I could see returning false for instances in terminated state.

/cc @d-nishi

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.

[derekwaynecarr]

per #67254 this pr seems like the appropriate follow-on.

[derekwaynecarr]

@jsafrane can you review and approve? see the reference to prereq approved pr on interface documentation change.

[gnufied]

@sjenning AWS and Openstack are somewhat unique in this regard IIRC. Only these cloud providers remove the node when a node is shutdown.

In general writing disruptive tests that simulate this behaviour and making sure nothing breaks has been harder because kops currently deploys everything in a ASG and a shutdown node is automatically terminated and removed from ASG. I am in general okay with this change but we must keep in mind that, there are almost no e2e tests that capture this change.

[gnufied]

Hmm looks like behavior of Openstack was fixed sometime back but I do remember it removing a shutdown/stopped node originally. GCE also keeps shutdown nodes in the list and does not remove them.

GCE fortunately has many disruptive tests, AWS has zero.

[gnufied]

Storage has - https://github.com/kubernetes/kubernetes/blob/master/test/e2e/storage/pd.go#L307 and there are probably more.

a while back - we wrote some disruptive tests for AWS - gnufied@3df33c7 . Specifically gnufied@3df33c7#diff-cf6d6091f38a1751fd35b8257efc60a3 but there is no way to run them in current test-infra setup, so we ended up deleting those.

[sjenning]

I have updated the commit to exclude instances in terminated state

[liggitt]

/retest

[gnufied]

The regression we noticed today where volumes were not being detached from AWS is possibly because of - #65392 . Before this patch, a stopped node caused deletion of all pods on it and now the pods on stopped nodes stick around in "unknown" but "Running" phase.

[gnufied]

minor nit - can we make a constant out of the states we consider "alive" ?

[sjenning]

Rebased and fixed nit.

Please note that #59930 merged about 24 hours ago and removes the state check from InstanceExistsByProviderID(). I'm adding it back to consider instances in terminated state as dead and filter them out.

fyi @zetaab

[zetaab]

@sjenning filtering out terminated instances is fine. However, you could use constants (those are defined in aws ec2 lib)

[sjenning]

@zetaab good call. updated.

[zetaab]

/approve

[gnufied]

/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gnufied, sjenning, zetaab

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:

• pkg/cloudprovider/providers/aws/OWNERS [gnufied]

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

[gnufied]

cc @childsb @derekwaynecarr please add it to 1.12 milestone.

[derekwaynecarr]

impacts sig node.

/sig node
/milestone v1.12
/priority critical-urgent

[fejta-bot]

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

[k8s-github-robot]

Automatic merge from submit-queue (batch tested with PRs 67571, 67284, 66835, 68096, 68152). If you want to cherry-pick this change to another branch, please follow the instructions here: https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md.