feat: ability to emit Kubernetes events #411
Conversation
cmd/node-termination-handler.go
Outdated
| @@ -320,11 +335,14 @@ func drainOrCordonIfNecessary(interruptionEventStore *interruptioneventstore.Sto | |||
| log.Err(err).Msgf("node '%s' not found in the cluster", nodeName) | |||
| } else { | |||
| log.Err(err).Msg("There was a problem while trying to cordon and drain the node") | |||
| metrics.NodeActionsInc("cordon-and-drain", nodeName, err) | |||
There was a problem hiding this comment.
Added the metrics line here too so a metric is also created when an error occurs.
|
Looks like tests pass and fail randomly, they passed locally before pushing the PR. Will check tomorrow. |
|
The last |
|
I put this PR on a real world scenario at work and I instantly missed the emitter node AZ and instance type in the events, with which I plan to create reports like types of interruptions by AZ and types of interruptions by instance type. So I took a second pass here and added a default set of annotations to all events, basically all the data that was already gathered from IMDS by |
There was a problem hiding this comment.
Thanks for the pr! I was using events to debug something the other day, so I can see how this would be useful.
Overall looks pretty good to me, and I appreciate you taking the time to clean up some of the docs and the prometheus test. Would you mind posting an example of how these get rendered with kubectl get events ... ?
pkg/observability/k8s-events.go
Outdated
| // Emit a Kubernetes event for the current node and with the given event type, reason and message | ||
| func (r K8sEventRecorder) Emit(eventType, eventReason, eventMsgFmt string, eventMsgArgs ...interface{}) { | ||
| if r.enabled { | ||
| r.AnnotatedEventf(r.node, r.annotations, eventType, eventReason, eventMsgFmt, eventMsgArgs...) |
There was a problem hiding this comment.
in queue processor mode, node might not be the node that's being cordoned and drained, is that intentional? perhaps we should pass in node ?
There was a problem hiding this comment.
Good point, totally omitted that. Revamped it so instead of getting the Node from the API when initialising the recorder we just create an ObjectReference to a Node object with a parameterised name. Such name will be passed in every time recorder.Emit() is called and it will come from the nodeName stored in every particular interruption event, thus making it valid both for IMDS and SQS processors if I'm not mistaken.
pkg/observability/k8s-events.go
Outdated
| annotations["public-ipv4"] = nodeMetadata.PublicIP | ||
| annotations["region"] = nodeMetadata.Region | ||
|
|
||
| // Parse extra annotations |
There was a problem hiding this comment.
nit - you can remove the comments from here through the end of the function, they're just stating what the next lines show
pkg/observability/k8s-events.go
Outdated
| } | ||
|
|
||
| // Create default annotations | ||
| // Worth iterating over nodeMetadata fields using reflect? (trutx) |
There was a problem hiding this comment.
imo it's fine as you've done it
cmd/node-termination-handler.go
Outdated
| } | ||
| metrics.NodeActionsInc("uncordon", nodeName, err) | ||
| recorder.Emit(observability.Normal, observability.UncordonReason, observability.UncordonMsg) |
There was a problem hiding this comment.
should this be in an else ?
There was a problem hiding this comment.
Good catch, did it everywhere but here. Fixed.
| verbs: | ||
| - create | ||
| - get | ||
| - list | ||
| - patch | ||
| - update | ||
| - watch |
There was a problem hiding this comment.
Probably not. Removed them all but create.
|
🤔 pushed changes to my fork but this PR doesn't sync |
|
Ok there we go.
|
docs/kubernetes_events.md
Outdated
|
|
||
| ## Caveats | ||
|
|
||
| ### Default annotations in Queue Processor Mode |
There was a problem hiding this comment.
Added this to document that annotations in SQS mode will match those of the Node where ANTH runs and not those of the Node where the interruption, drain or whatever event actually happens.
Not sure if that's the best approach or we should just don't create default, IMDS-based annotations when running in SQS mode. Happy to code that if preferred.
There was a problem hiding this comment.
Thanks for the helpful explanations 👍🏼
Similar to @haugenj , I'd like to see some examples of events before making that call. If the annotations introduce a lot of noise in QueueProcessor mode, then maybe we omit them
I'll post an example and a screenshot when I have something to show. I'm waiting for actual rebalance or interruption events to happen in my test clusters. Probably when the US wake up since clusters live in |
docs/kubernetes_events.md
Outdated
|
|
||
| ## Default annotations | ||
|
|
||
| If `emit-kubernetes-events` is enabled, AWS Node Termination Handler will automatically inject a set of annotations to each event it emits. Such annotations are gathered from the underlying host's IMDS endpoint and enrich each event with information about the host that emitted it. |
There was a problem hiding this comment.
could you elevate your note here about Queue Processor mode?
Ex: Note: In Queue Processor mode, these annotations will reflect the node running NTH not the node receiving the events. See Caveats for more information
There was a problem hiding this comment.
Done.
Feel free to edit/correct any documentation wording at will. A native's English will always be better than mine.
docs/kubernetes_events.md
Outdated
|
|
||
| ## Caveats | ||
|
|
||
| ### Default annotations in Queue Processor Mode |
There was a problem hiding this comment.
Thanks for the helpful explanations 👍🏼
Similar to @haugenj , I'd like to see some examples of events before making that call. If the annotations introduce a lot of noise in QueueProcessor mode, then maybe we omit them
|
I just realised how relevant it is to have the instance lifecycle available so I added it. Still waiting for a real interruption or recommendation event to happen. Problem is events TTL in k8s is 1 hour by default unless modified and it's too much effort for me to change it in a ~550 node cluster so I guess we'll have to wait and be lucky enough to see it before it expires. |
| @@ -75,9 +75,6 @@ func (m SQSMonitor) ec2StateChangeToInterruptionEvent(event EventBridgeEvent, me | |||
| InstanceID: ec2StateChangeDetail.InstanceID, | |||
| Description: fmt.Sprintf("EC2 State Change event received. Instance went into %s at %s \n", ec2StateChangeDetail.State, event.getTime()), | |||
| } | |||
| if err != nil { | |||
There was a problem hiding this comment.
Adding this error handling here made the ec2-state-change-sqs-test test fail. Not sure if swallowing this error is the way to go but this is out of the scope of this PR. I'm just replacing err with _ for the sake of the linter but IMO this should be revisited.
Valid also for pkg/monitor/sqsevent/rebalance-recommendation-event.go and pkg/monitor/sqsevent/spot-itn-event.go
|
Here's an anonimized example of a rebalance recommendation that just happened: $ k get ev --field-selector source=aws-node-termination-handler
LAST SEEN TYPE REASON OBJECT MESSAGE
13m Normal RebalanceRecommendation node/ip-10-1-2-3.us-east-2.compute.internal Rebalance recommendation received. Instance will be cordoned at 2021-04-26T15:10:48Z
13m Normal PreDrain node/ip-10-1-2-3.us-east-2.compute.internal Pre-drain task successfully executed
13m Normal CordonAndDrain node/ip-10-1-2-3.us-east-2.compute.internal Node successfully cordoned and drained$ k get ev --field-selector source=aws-node-termination-handler -o wide
LAST SEEN TYPE REASON OBJECT SUBOBJECT SOURCE MESSAGE FIRST SEEN COUNT NAME
13m Normal RebalanceRecommendation node/ip-10-1-2-3.us-east-2.compute.internal aws-node-termination-handler, ip-10-1-2-3.us-east-2.compute.internal Rebalance recommendation received. Instance will be cordoned at 2021-04-26T15:10:48Z 13m 1 ip-10-1-2-3.us-east-2.compute.internal.1679710f4148ba6a
13m Normal PreDrain node/ip-10-1-2-3.us-east-2.compute.internal aws-node-termination-handler, ip-10-1-2-3.us-east-2.compute.internal Pre-drain task successfully executed 13m 1 ip-10-1-2-3.us-east-2.compute.internal.1679710f48cde7e6
13m Normal CordonAndDrain node/ip-10-1-2-3.us-east-2.compute.internal aws-node-termination-handler, ip-10-1-2-3.us-east-2.compute.internal Node successfully cordoned and drained 13m 1 ip-10-1-2-3.us-east-2.compute.internal.167971124e6d316c$ k get ev --field-selector source=aws-node-termination-handler,reason=RebalanceRecommendation -o json
{
"apiVersion": "v1",
"items": [
{
"apiVersion": "v1",
"count": 1,
"eventTime": null,
"firstTimestamp": "2021-04-26T15:10:51Z",
"involvedObject": {
"kind": "Node",
"name": "ip-10-1-2-3.us-east-2.compute.internal",
"namespace": "default"
},
"kind": "Event",
"lastTimestamp": "2021-04-26T15:10:51Z",
"message": "Rebalance recommendation received. Instance will be cordoned at 2021-04-26T15:10:48Z \n",
"metadata": {
"annotations": {
"account-id": "123456789012",
"availability-zone": "us-east-2a",
"instance-id": "i-abcdef12345678901",
"instance-life-cycle": "spot",
"instance-type": "m5.8xlarge",
"local-hostname": "ip-10-1-2-3.us-east-2.compute.internal",
"local-ipv4": "10.1.2.3",
"public-hostname": "",
"public-ipv4": "",
"region": "us-east-2"
},
"creationTimestamp": "2021-04-26T15:10:51Z",
"name": "ip-10-1-2-3.us-east-2.compute.internal.1679710f4148ba6a",
"namespace": "default",
"resourceVersion": "1488622620",
"selfLink": "/api/v1/namespaces/default/events/ip-10-1-2-3.us-east-2.compute.internal.1679710f4148ba6a",
"uid": "1adcd988-2e9b-4422-bc54-c89982d46d43"
},
"reason": "RebalanceRecommendation",
"reportingComponent": "",
"reportingInstance": "",
"source": {
"component": "aws-node-termination-handler",
"host": "ip-10-1-2-3.us-east-2.compute.internal"
},
"type": "Normal"
}
],
"kind": "List",
"metadata": {
"resourceVersion": "",
"selfLink": ""
}
} |
|
And here's how a few days worth of events in a couple of clusters look like in New Relic One, combined with cluster autoscaler and node events. |
There was a problem hiding this comment.
Thanks for the updates + sample. After seeing the sample, could you make 3 minor updates? After, it'll be good from my side 👍
"message": "Rebalance recommendation received. Instance will be cordoned at 2021-04-26T15:10:48Z \n",
Could you update the Description for sqs-events to contain the instanceID to avoid confusion? This prob should've been done when first adding these events.
The code links are as follows:
- ex: fmt.Sprintf("Rebalance recommendation event received. Instance %s will be cordoned at %s \n", rebalanceRecDetail.InstanceID, event.getTime())
To close out on the other open question, I am good with leaving annotations for both imds+sqs mode.
|
Done. Still struggling with |
|
Here's the output of two consecutive Tried that several times and looks totally random to me, and caused by a third party software anyway. Sometimes we get the |
Interesting-- thanks for looking into this. I'm good merging the PR after @haugenj reviews and we can look into the fix. |
|
Spent some time trying to reproduce the error but I wasn't able to come to a consistent ending, in my laptop I can't get it neither to always success nor to always fail. What looks consistent is the outcome of the Github CI, always fails. There have been changes in https://github.com/gojp/goreportcard recently, might be related despite I've tried to pin the module to an older commit and results were still unpredictable. |
| @@ -4,6 +4,7 @@ go 1.15 | |||
|
|
|||
| require ( | |||
| github.com/aws/aws-sdk-go v1.33.1 | |||
| github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da // indirect | |||
pkg/config/config.go
Outdated
| case "error": | ||
| default: | ||
| return config, fmt.Errorf("Invalid log-level passed: %s Should be one of: info, debug, error", config.LogLevel) | ||
| if err := validateLogLevel(strings.ToLower(config.LogLevel)); err != nil { |
There was a problem hiding this comment.
I don't think this adds value as a separate function, I'd prefer you revert this
There was a problem hiding this comment.
It doesn't add much value, I just did it to reduce the cyclomatic complexity, otherwise go-report-card-test would fail.
There was a problem hiding this comment.
I say revert it and don't worry about the report card test, we'll take care of that afterwards
docs/kubernetes_events.md
Outdated
|
|
||
| Default annotations values are gathered from the IMDS endpoint local to the Node on which AWS Node Termination Handler runs. This is fine when running on IMDS Processor Mode since an AWS Node Termination Handler Pod will be deployed to all Nodes via a `DaemonSet` and each Node will emit all events related to itself with its own default annotations. | ||
|
|
||
| However, when running in Queue Processor Mode AWS Node Termination Handler is deployed to a number of Nodes (1 replica by default) via a `Deployment`. In that case the default annotations values will be gathered from the Node(s) running AWS Node Termination Handler, and so the values in the default annotations stamped to all events will match those of the Node from which the event was emitted, not those of the Node of which the event is about. |
There was a problem hiding this comment.
Are the annotations worth anything in this case? It seems both misleading and unhelpful to print values for a different node than the event actually affected. Perhaps we should just omit the default annotations when running in queue processor mode, thoughts?
There was a problem hiding this comment.
I'm trying to think if you would want to know the metadata of the node running nth in queue processor mode. If not, it looks like the host instance running NTH is included in the event here, so we could omit:
"source": {
"component": "aws-node-termination-handler",
"host": "ip-10-1-2-3.us-east-2.compute.internal"
}
It seems both misleading and unhelpful to print values for a different node than the event actually affected
Also agree, but the latest commit should address this. Now, the messaging shown in the even will include the instanceId being affected:
"message": "Rebalance recommendation received. Instance **<instance-id>** will be cordoned at 2021-04-26T15:10:48Z \n",
There was a problem hiding this comment.
User provided annotations will still be worthy (think of annotations like cluster name, cluster owner, environment, etc) but data coming from IMDS might not make much sense and I agree it can be confusing, hence the notes in the doc. Probably the right move is to omit annotations with data coming from IMDS when in SQS mode.
There was a problem hiding this comment.
Last commit should do it. Also updated the doc to reflect these changes.
|
FYI, this PR reverted some of the changes for #388 |
|
@trutx Nice work on this. I was just looking for this feature and am glad to see that you have made it happen! |
Fixes #414
Description of changes:
This PR adds the ability to emit Kubernetes events when interruption signals are received and when actions like cordon or drain are taken on nodes.
Kubernetes events are very useful for monitoring. At New Relic we push all cluster events using an eventrouter up to New Relic One and then we create alerts and dashboards based on such events. This PR makes NTH emit a Kubernetes event every time a signal is received from AWS and also every time an action is taken on the node NTH is running on, regardless if it ends in success or failure.
The
involvedObjectfor these events is the Kubernetes node NTH runs on, so they are published in thedefaultnamespace. To get them runkubectl get events --field-selector source=aws-node-termination-handler.Additionally annotations can be attached to each event. Such annotations are useful for filtering and discovery. I.e. each event can be stamped with an annotation stating the cluster name the event belongs to, the team owning the cluster, etc. These annotations can later be used in the New Relic UI to filter out events belonging to a specific subset via a NRQL query, or can be used to filter the results in a
kubectl get eventsquery piped to a processor likejqoryq.