Skip to content

Conversation

trutx
Copy link
Contributor

@trutx trutx commented Oct 21, 2022

Description of changes:

This PR enriches Kubernetes events when running in SQS mode by adding annotations to such events, so they can later be used to get information about the node involved in the event, create filters, etc. Similar to what was done in #411

I have decided to add almost the same annotations the events in IMDS mode have, but I'd like some thoughts here because most of the information is gathered from node labels so we could simply attach all node labels as tags instead, or even do the two approaches simultaneously.

The only annotation I'm not adding here is instance-life-cycle because that's something that can be easily obtained from the IMDS endpoint but it's not present in the Kubernetes Node object, so it would have to be fetched from the AWS API instead (doable of course, but at the cost of another AWS API call). Again, I don't have a strong opinion on either option so thoughts welcome.

I intend to add a unit test (similar to test/e2e/spot-interruption-test-events-on once the set of annotations the events will have is decided.

Also, I will submit another PR to update the documentation once this PR and also #703 and #706 land. At New Relic we are running an image containing the changes in all 3 PRs combined and it works great.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@trutx trutx requested a review from a team as a code owner October 21, 2022 15:14
annotations["account-id"] = nodeMetadata.AccountId
annotations["availability-zone"] = nodeMetadata.AvailabilityZone
annotations["instance-id"] = nodeMetadata.InstanceID
annotations["instance-life-cycle"] = nodeMetadata.InstanceLifeCycle
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with your decision to leave instance-life-cycle out of the annotations for SQS mode, since including it would require an additional AWS API call. If it turns out customers do want it later, we can discuss adding it in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can also add a new node-labels-to-event-annotations feature. Similar to the extra annotations we already have, but in this case the user would define a set (or even all?) of Node object labels that NTH would add to the event annotations map. Then the user could label all spot/ondemand instances they way they see fit (or any other arbitrary label FWIW) and instruct NTH to add such label as event annotation. NTH calls the k8s API anyway to get the Node object, so this extra feature would come for free in terms of remote API calls.

@snay2 snay2 merged commit 5c39cc4 into aws:main Nov 1, 2022
@trutx trutx deleted the rtorrents/sqs_events_annotations branch November 2, 2022 09:07
@snay2 snay2 added the Pending-Release Pending an NTH or eks-charts release label Nov 21, 2022
@snay2
Copy link
Contributor

snay2 commented Nov 22, 2022

Released in NTH v1.18.0 (Helm chart version v0.20.0).

@snay2 snay2 removed the Pending-Release Pending an NTH or eks-charts release label Nov 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants