Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UPSTREAM: 66835: cloudprovider: aws: return true on existence check for stopped instances #20663

Merged
merged 1 commit into from
Aug 24, 2018

Conversation

sjenning
Copy link
Contributor

@sjenning sjenning commented Aug 15, 2018

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 15, 2018
@derekwaynecarr
Copy link
Member

upstream directional change is blessed, but we can carry this in the interim pending it merge upstream.

/approve
/lgtm

@openshift-ci-robot openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 16, 2018
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@sjenning
Copy link
Contributor Author

/retest

@gnufied
Copy link
Member

gnufied commented Aug 16, 2018

There are couple of things that needs verified. Please do not merge this without approval from @openshift/sig-storage

/hold

@openshift-ci-robot openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. sig/storage labels Aug 16, 2018
@gnufied
Copy link
Member

gnufied commented Aug 16, 2018

Things that needs to be verified before we merge this:

a. We need to make sure that, nodes that are shutdown can have its pod migrated without attach/detach issues when node object is still around.
b. We need to make sure that - volumes get detached from static pods(not backed by rc/rs) when a node is down for long period of time.
c. We need to make sure that - there are no race conditions for AWS as mentioned in kubernetes/kubernetes#52735 wrt to attach/detach controller attempting to reattach volumes to terminated nodes because on terminate AWS detaches all volumes from node but this PR will keep the node object still around and hence attach/detach controller will consider this as error state and try to "fix" it.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Aug 17, 2018
@sjenning
Copy link
Contributor Author

@gnufied re: our discussions on IRC, I think you ok with this now right? If so, please comment confirming and re-add lgtm. @derekwaynecarr already approved. Thanks!

@gnufied
Copy link
Member

gnufied commented Aug 24, 2018

Yeah I think I am okay with merging this with master. storage team will backport the other fix for detaching volume from shutdown node.

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 24, 2018
@gnufied
Copy link
Member

gnufied commented Aug 24, 2018

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 24, 2018
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

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

@gnufied
Copy link
Member

gnufied commented Aug 24, 2018

Filed https://bugzilla.redhat.com/show_bug.cgi?id=1622175 for tracking the storage bug of volumes not getting detached from stopped AWS nodes.

@openshift-merge-robot openshift-merge-robot merged commit c4d30d2 into openshift:master Aug 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. sig/storage size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Node labels are lost after Deregister/Register Node events
7 participants