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.

Sign up for GitHub

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

Jump to bottom

🐛 Improve EC2 state handling, and set error for manually deleted EC2 instances #1256

Merged
merged 1 commit into from
Nov 8, 2019
Merged

🐛 Improve EC2 state handling, and set error for manually deleted EC2 instances #1256

merged 1 commit into from
Nov 8, 2019

Conversation

randomvariable
Copy link
Member

What this PR does / why we need it:
Tweaks EC2 instance state handling in the following ways:

  • Emit log event as well as publish event to resource for better traceability
  • Ignore instance attributes for validation when the instance is terminated, or about to transition to terminated from "shutting-down".
  • Don't error when EC2 instance is stopping or is stopped, as machine could be rebooted.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #1239
Fixes #1238

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 24, 2019
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 24, 2019
@randomvariable randomvariable force-pushed the log-for-events branch 4 times, most recently from 12410e6 to 7c7baf2 Compare October 24, 2019 08:56
@detiber
Copy link
Member

detiber commented Oct 24, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 24, 2019
@vincepri
Copy link
Member

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 24, 2019
@vincepri
Copy link
Member

Reviewing

@vincepri
Copy link
Member

/milestone v0.5.0

@k8s-ci-robot k8s-ci-robot added this to the v0.5.0 milestone Oct 24, 2019
vincepri
vincepri reviewed Oct 24, 2019
View reviewed changes
controllers/awsmachine_controller.go Outdated Show resolved Hide resolved
controllers/awsmachine_controller.go Show resolved Hide resolved
controllers/awsmachine_controller.go Outdated Show resolved Hide resolved
@vincepri
Copy link
Member

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 24, 2019
@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Oct 25, 2019
@randomvariable randomvariable changed the title 🐛 Transition manually deleted EC2 instances to failed and support hibernation [wip] 🐛 Transition manually deleted EC2 instances to failed and support hibernation Oct 25, 2019
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 25, 2019
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 25, 2019
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 25, 2019
@randomvariable randomvariable changed the title [wip] 🐛 Transition manually deleted EC2 instances to failed and support hibernation 🐛 Improve EC2 state handling, and set error for manually deleted EC2 instances Oct 25, 2019
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 25, 2019
@randomvariable
Copy link
Member Author

Ready for review.
Have added descriptive unit tests based on the various transitions that can occur.

@detiber
Copy link
Member

detiber commented Oct 25, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 25, 2019
vincepri
vincepri reviewed Oct 26, 2019
View reviewed changes
controllers/awsmachine_controller.go Outdated Show resolved Hide resolved
controllers/awsmachine_controller.go Outdated Show resolved Hide resolved
pkg/cloud/scope/machine.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 28, 2019
@randomvariable
Copy link
Member Author

Tweaked to be less noisy in the logs.

@detiber detiber mentioned this pull request Oct 30, 2019
Merged
@randomvariable randomvariable mentioned this pull request Oct 31, 2019
Closed
vincepri
vincepri approved these changes Nov 8, 2019
View reviewed changes
Copy link
Member

@vincepri vincepri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve
/assign @detiber

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: randomvariable, vincepri

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 8, 2019
@randomvariable
machine controller: improve EC2 state handling
ff912c8 
Signed-off-by: Naadir Jeewa <jeewan@vmware.com>
@detiber
Copy link
Member

detiber commented Nov 8, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 8, 2019
@k8s-ci-robot k8s-ci-robot merged commit a7bcbcb into kubernetes-sigs:master Nov 8, 2019
@randomvariable randomvariable deleted the log-for-events branch November 19, 2019 18:04
@randomvariable randomvariable mentioned this pull request Nov 19, 2019
Merged
@randomvariable randomvariable mentioned this pull request Nov 8, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@detiber detiber detiber left review comments

@vincepri vincepri vincepri approved these changes

@rudoi rudoi Awaiting requested review from rudoi

@sethp-nr sethp-nr Awaiting requested review from sethp-nr

Assignees

@detiber detiber

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v0.5.0
Development

Successfully merging this pull request may close these issues.

Terminated instance not detected Log entry for every event
4 participants
@randomvariable @detiber @vincepri @k8s-ci-robot