-
Notifications
You must be signed in to change notification settings - Fork 807
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
Add missing instances to instance store volumes table #1966
Add missing instances to instance store volumes table #1966
Conversation
Code Coverage DiffThis PR does not change the code coverage |
/retest |
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.
Spot checked 20 different instance types. Looks good. A few comments but then will approve.
// / IMDS does not provide NVMe instance store data; we'll just list all instances here | ||
// / TODO: See if we can get these values from DescribeInstanceTypes API | ||
// Generated by command, requires aws cli and jq: | ||
// aws ec2 describe-instance-types --region us-east-1 --filters "Name=instance-storage-supported,Values=true" --query "InstanceTypes[].[InstanceType, InstanceStorageInfo]" | jq -r 'map("\"" + .[0] + "\": " + (.[1].Disks | map(.Count) | add | tostring) + ",") | .[]' | sort | ||
var nvmeInstanceStoreVolumes = map[string]int{ |
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.
Is there a reason we don't support hpc6id.32xlarge instance?
4 instance store volumes.
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.
Hmm, it's only available in us-east-2 so when I go looking in us-east-1 I don't find it. Be ready for the insane kludge of v2
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.
Wow that's unintuitive. Thanks for letting us know the reason and adding in this new script. Really removes some operator toil!
/retest |
2 similar comments
/retest |
/retest |
d2bde02
to
aa15f76
Compare
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
$ ./check.sh
Success! no instances were removed
original="https://raw.githubusercontent.com/kubernetes-sigs/aws-ebs-csi-driver/master/pkg/cloud/volume_limits.go"
updated="https://raw.githubusercontent.com/kubernetes-sigs/aws-ebs-csi-driver/aa15f7650d453e80b262587bd0bcc62242601177/pkg/cloud/volume_limits.go"
extract_table() {
curl -s "$1" | sed -n '/^var nvmeInstanceStoreVolumes = map\[string\]int{/,/^}/p' | grep -v "^var" | grep -v "^}"
}
original_table=$(extract_table "$original")
updated_table=$(extract_table "$updated")
original_keys=$(echo "$original_table" | grep -o '^[^:]*' | sort)
updated_keys=$(echo "$updated_table" | grep -o '^[^:]*' | sort)
removed_instances=$(diff <(echo "$original_keys") <(echo "$updated_keys") | grep "^<" | cut -d ' ' -f 2)
if [ -n "$removed_instances" ]; then
echo "Warning!!! the following instances were incorrectly removed from the table"
echo "$removed_instances"
else
echo "Success! no instances were removed"
fi
/retest |
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.
Great work! Thanks for the initiative with the script.
/retest aws-ebs-csi-driver-verify |
@AndrewSirenko: The
The following commands are available to trigger optional jobs:
Use In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/retest pull-aws-ebs-csi-driver-verify |
@AndrewSirenko: The
The following commands are available to trigger optional jobs:
Use In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/test pull-aws-ebs-csi-driver-verify |
/approve |
aa15f76
to
380b07f
Compare
Signed-off-by: Connor Catlett <[email protected]>
380b07f
to
60807fe
Compare
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
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AndrewSirenko, torredil 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 |
Is this a bug fix or adding new feature?
Bug fix
What is this PR about? / Why do we need it?
Adds missing instance store instances to the table
Also documents command to generate this tableAlso adds a script to generate this table
What testing is done?
Confirmed manually against table on AWS docs, as well as previous list to ensure no types were removed/changed