🐛 fixing crashes on State-change events #313
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
universam1
changed the title
|
nice - think I have seen such crashes already would be happy if this fix gets merged 👍 |
haugenj
reviewed
Dec 10, 2020
|
@universam1 This is great! Left you two small nitpick comments but this is really awesome, thanks! |
NTH crashes often from "EC2 Instance State-change Notification" events. The underlying issue was that an empty parsed NodeName derived from PrivateDnsName for forwarded unverified, creating cascading problems. This solves the root cause, that "EC2 Instance State-change Notification" can arrive at a time where the instance is in shutting-down, terminated or any other not-online situation where the PrivateDnsName metadata is empty! Instead of just ignoring these errors this implementation gets and decides based on the state of the instance message if it is an error to fail or to ignore. Therefor such messages are dropped in above situation because they are useless.
universam1
force-pushed
the
fixCrashOnStateEvents
branch
from
9e5d9e2 to
93163e0
Compare
universam1
commented
Dec 10, 2020
| if nthConfig.CordonOnly || drainEvent.IsRebalanceRecommendation() { | ||
| err := node.Cordon(nodeName) | ||
| if err != nil { | ||
| if errors.IsNotFound(err) { |
There was a problem hiding this comment.
@haugenj found another situation where events for other nodes that do not belong to this cluster could bubble up an error - I think for this case we can only do best effort to ignore such cases without removing all the safety guards
There was a problem hiding this comment.
I think this is the right approach. Originally I was thinking we would totally remove the os.Exit(1) here, so your solution feels like a better middle-ground
universam1
force-pushed
the
fixCrashOnStateEvents
branch
from
93163e0 to
12bde5b
Compare
haugenj
pushed a commit
to haugenj/aws-node-termination-handler
that referenced
this pull request
Dec 14, 2020
NTH crashes often from "EC2 Instance State-change Notification" events. The underlying issue was that an empty parsed NodeName derived from PrivateDnsName was forwarded unverified, creating cascading problems. This solves the root cause, that "EC2 Instance State-change Notification" can arrive at a time where the instance is in shutting-down, terminated or any other not-online situation where the PrivateDnsName metadata is empty! Instead of just ignoring these errors this implementation gets and decides based on the state of the instance message if it is an error to fail or to ignore. Therefor such messages are dropped in above situation because they are useless.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
NTH crashes often from "EC2 Instance State-change Notification" events.
fixes #307
The underlying issue was that an empty parsed NodeName derived from PrivateDnsName for forwarded unverified, creating cascading problems.
This solves the root cause, that "EC2 Instance State-change Notification" can arrive at a time where the instance is in shutting-down, terminated or any other not-online situation where the PrivateDnsName metadata is empty!
Instead of just ignoring these errors this implementation gets and decides based on the state of the instance message if it is an error to fail or to ignore.
Therefor such messages are dropped in above situation because they are useless.
@haugenj
Background: went all the way back, instead of just removing the os.exit(1) found that actually the nodeName was empty but not verified.
The existing unit test was flawed in this regard.
The empty nodeName parsing derives from
EC2.DescribeInstancesthat have the metadata ofPrivateDnsNameempty, simply because the node is not running any more.So created a custom error that allows to handle that case.
example event:
{ "AmiLaunchIndex": 1, "Architecture": "x86_64", "ImageId": "ami-0e44cbb6cd97xxxx", "InstanceId": "i-xxxxxxxx", "Platform": null, "PrivateDnsName": "", "PrivateIpAddress": null, "ProductCodes": null, "PublicDnsName": "", "PublicIpAddress": null, "State": { "Code": 48, "Name": "terminated" }, "StateReason": { "Code": "Client.UserInitiatedShutdown", "Message": "Client.UserInitiatedShutdown: User initiated shutdown" }, }