-
Notifications
You must be signed in to change notification settings - Fork 753
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 trn1 limits #2092
Add trn1 limits #2092
Conversation
"c7g.8xlarge": {ENILimit: 8, IPv4Limit: 30, HypervisorType: "nitro", IsBareMetal: false}, | ||
"c7g.large": {ENILimit: 3, IPv4Limit: 10, HypervisorType: "nitro", IsBareMetal: false}, | ||
"c7g.medium": {ENILimit: 2, IPv4Limit: 4, HypervisorType: "nitro", IsBareMetal: false}, | ||
"c7g.xlarge": {ENILimit: 4, IPv4Limit: 15, HypervisorType: "nitro", IsBareMetal: false}, |
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.
I removed these based on the scripts output (with the above modifications):
Delete "c6g.xlarge": {4 15 nitro false} is already correct in the API
Delete "c7g.12xlarge": {8 30 nitro false} is already correct in the API
Delete "c7g.16xlarge": {15 50 nitro false} is already correct in the API
Delete "c7g.2xlarge": {4 15 nitro false} is already correct in the API
Delete "c7g.4xlarge": {8 30 nitro false} is already correct in the API
Delete "c7g.8xlarge": {8 30 nitro false} is already correct in the API
Delete "c7g.large": {3 10 nitro false} is already correct in the API
Delete "c7g.medium": {2 4 nitro false} is already correct in the API
Delete "c7g.xlarge": {4 15 nitro false} is already correct in the API
Replacing API value {15 50 nitro false} with override {15 50 unknown false} for "dl1.24xlarge"
Replacing API value {15 50 nitro false} with override {15 50 unknown false} for "p4d.24xlarge"
@jayanthvn I can't reproduce the |
Can you just try running "make format" ? |
Yes, |
Yeah, I did try to repo and I don't see any diff either -
Will review and merge it. I will test on master post merge. |
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
Even the master looks clean after the merge -
|
Weird. But thanks for your help! |
What type of PR is this?
Feature
Which issue does this PR fix:
N/A
What does this PR do / Why do we need it:
This adds
trn1
instance types to the files generated bymake generate-limits
. I also modified the script to properly handle instance types with multiple network cards, which allowed me to remove some of the manual overrides. I also removed other manual overrides that were no longer necessary.Testing done on this change:
Automation added to e2e:
N/A
Will this PR introduce any new dependencies?:
No.
Will this break upgrades or downgrades. Has updating a running cluster been tested?:
No.
Does this change require updates to the CNI daemonset config files to work?:
No.
Does this PR introduce any user-facing change?:
Yes.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.