-
Notifications
You must be signed in to change notification settings - Fork 2k
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
env_aws: get ec2 cpu perf data from AWS API #9038
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/hashicorp/nomad/qclwi2h0r |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love the automation and code generation! Few questions!
@@ -0,0 +1,1620 @@ | |||
// Code generated from hashicorpnomad/ec2info; DO NOT EDIT. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seeing that this is basically a shell script, I'd include it in the OSS repo with instructions. One benefit might be getting contributions when the data is stale.
|
||
var ec2ProcSpeedTable = map[string]map[string]ec2specs{ | ||
|
||
"ap-northeast-1": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we worried about specs for the same instance varying by region? I'd find that very surprising if that's the case?
Looking at the current file, I don't see any divergence. Deduplicating it reduces the elements from 1569 map entries to 212.
Also, I suspect some missing data: c5a.16xlarge
and c5.18xlarge
are available in multiple regions according to their press releases but they are only listed for eu-north-1 here: https://aws.amazon.com/blogs/aws/new-amazon-ec2-c5a-instances-powered-by-2nd-gen-amd-epyc-processors/, and https://aws.amazon.com/blogs/aws/now-available-compute-intensive-c5-instances-for-amazon-ec2/ -
Previously, Nomad was using a hand-made lookup table for looking up EC2 CPU performance characteristics (core count + speed = ticks). This data was incomplete and incorrect depending on region. The AWS API has the correct data but requires API keys to use (i.e. should not be queried directly from Nomad). This change introduces a lookup table generated by a small command line tool in Nomad's tools module which uses the Amazon AWS API. Running the tool requires AWS_* environment variables set. $ # in nomad/tools/cpuinfo $ go run . Going forward, Nomad can incorporate regeneration of the lookup table somewhere in the CI pipeline so that we remain up-to-date on the latest offerings from EC2. Fixes #7830
@notnoop So yeah, I wrote a little tool to check if instance types ever vary by region, and (at least for now) they do not. I then cleaned up the tool so that it generates a minimal table and added it to Nomad's There is no more Docker image or anything external, contributes now only need to set |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
CHANGELOG.md
Outdated
@@ -8,6 +8,7 @@ IMPROVEMENTS: | |||
* client: Added support for Azure fingerprinting. [[GH-8979](https://github.com/hashicorp/nomad/issues/8979)] | |||
* client: Added support for fingerprinting the client node's Consul segment. [[GH-7214](https://github.com/hashicorp/nomad/issues/7214)] | |||
* client: Updated consul-template to v0.25.0 - config function_blacklist deprecated and replaced with function_denylist [[GH-8988](https://github.com/hashicorp/nomad/pull/8988)] | |||
* client: Use ec2 CPU perf data from AWS API [[GH-7830](https://github.com/hashicorp/nomad/issues/7830)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changelog nitpick: this line should go above the longer lines above it.
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
Previously, Nomad was using a hand-made lookup table for looking
up EC2 CPU performance characteristics (core count + speed = ticks).
This data was incomplete and incorrect depending on region. The AWS
API has the correct data but requires API keys to use (i.e. should not
be queried directly from Nomad).
This change introduces a lookup table generated by a docker image based
on the Amazon cli tool.
https://hub.docker.com/r/hashicorpnomad/ec2info
The docker image is created from https://github.com/hashicorp/nomad-ec2info.
Going forward, Nomad can incorporate regeneration of the lookup table
somewhere in the CI pipeline so that we remain up-to-date on the latest
offerings from EC2.
Fixes #7830