-
Notifications
You must be signed in to change notification settings - Fork 305
Enable newer arm64 instance types (m7g+) #7823
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
base: main
Are you sure you want to change the base?
Enable newer arm64 instance types (m7g+) #7823
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: grdryn The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Hi @grdryn. Thanks for your PR. I'm waiting for a redhat-appstudio member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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-sigs/prow repository. |
Code Review by Gemini```diff
--- a/components/multi-platform-controller/production/stone-prd-rh01/host-config.yaml
+++ b/components/multi-platform-controller/production/stone-prd-rh01/host-config.yaml
@@ -21,20 +21,22 @@ data:
linux-m2xlarge/amd64,\
linux-m2xlarge/arm64,\
linux-d160-m2xlarge/amd64,\
linux-d160-m2xlarge/arm64,\
linux-m4xlarge/amd64,\
linux-m4xlarge/arm64,\
linux-d160-m4xlarge/amd64,\
linux-d160-m4xlarge/arm64,\
linux-m8xlarge/amd64,\
linux-m8xlarge/arm64,\
- linux-d160-m8-8xlarge/arm64,\
+ linux-d160-m7g-8xlarge/arm64,\
linux-d160-m7-8xlarge/amd64,\
linux-d160-m8xlarge/amd64,\
linux-d160-m8xlarge/arm64,\
linux-c6gd2xlarge/arm64,\
linux-cxlarge/amd64,\
linux-cxlarge/arm64,\
linux-c2xlarge/amd64,\
linux-c2xlarge/arm64,\
linux-c4xlarge/amd64,\
linux-c4xlarge/arm64,\
linux-c8xlarge/amd64,\
@@ -135,20 +137,33 @@ data:
dynamic.linux-m8xlarge-arm64.ami: ami-03d6a5256a46c9feb
dynamic.linux-m8xlarge-arm64.instance-type: m6g.8xlarge
dynamic.linux-m8xlarge-arm64.instance-tag: prod-arm64-m8xlarge
dynamic.linux-m8xlarge-arm64.key-name: konflux-prod-ext-mab01
dynamic.linux-m8xlarge-arm64.aws-secret: aws-account
dynamic.linux-m8xlarge-arm64.ssh-secret: aws-ssh-key
dynamic.linux-m8xlarge-arm64.security-group-id: sg-0fbf35ced0d59fd4a
dynamic.linux-m8xlarge-arm64.max-instances: "5"
dynamic.linux-m8xlarge-arm64.subnet-id: subnet-0c39ff75f819abfc5
- dynamic.linux-d160-m8-8xlarge-arm64.type: aws
- dynamic.linux-d160-m8-8xlarge-arm64.region: us-east-1
- dynamic.linux-d160-m8-8xlarge-arm64.ami: ami-03d6a5256a46c9feb
- dynamic.linux-d160-m8-8xlarge-arm64.instance-type: m8g.8xlarge
- dynamic.linux-d160-m8-8xlarge-arm64.instance-tag: prod-arm64-m8-8xlarge-d160
- dynamic.linux-d160-m8-8xlarge-arm64.key-name: kflux-prd-rh03-key-pair
- dynamic.linux-d160-m8-8xlarge-arm64.aws-secret: aws-account
- dynamic.linux-d160-m8-8xlarge-arm64.ssh-secret: aws-ssh-key
- dynamic.linux-d160-m8-8xlarge-arm64.security-group-id: sg-0759f4a43faada557
- dynamic.linux-d160-m8-8xlarge-arm64.max-instances: "5"
- dynamic.linux-d160-m8-8xlarge-arm64.subnet-id: subnet-0263af86f44821eac
- dynamic.linux-d160-m8-8xlarge-arm64.disk: "160"
+ dynamic.linux-d160-m7g-8xlarge-arm64.type: aws
+ dynamic.linux-d160-m7g-8xlarge-arm64.region: us-east-1
+ dynamic.linux-d160-m7g-8xlarge-arm64.ami: ami-03d6a5256a46c9feb
+ dynamic.linux-d160-m7g-8xlarge-arm64.instance-type: m7g.8xlarge
+ dynamic.linux-d160-m7g-8xlarge-arm64.instance-tag: prod-arm64-m7g-8xlarge-d160
+ dynamic.linux-d160-m7g-8xlarge-arm64.key-name: kflux-prd-rh03-key-pair
+ dynamic.linux-d160-m7g-8xlarge-arm64.aws-secret: aws-account
+ dynamic.linux-d160-m7g-8xlarge-arm64.ssh-secret: aws-ssh-key
+ dynamic.linux-d160-m7g-8xlarge-arm64.security-group-id: sg-0759f4a43faada557
+ dynamic.linux-d160-m7g-8xlarge-arm64.max-instances: "5"
+ dynamic.linux-d160-m7g-8xlarge-arm64.subnet-id: subnet-0263af86f44821eac
+ dynamic.linux-d160-m7g-8xlarge-arm64.disk: "160"
dynamic.linux-d160-m8xlarge-arm64.type: aws
dynamic.linux-d160-m8xlarge-arm64.region: us-east-1
dynamic.linux-d160-m8xlarge-arm64.ami: ami-03d6a5256a46c9feb
dynamic.linux-d160-m8xlarge-arm64.instance-type: m6g.8xlarge
dynamic.linux-d160-m8xlarge-arm64.instance-tag: prod-arm64-m8xlarge-d160
dynamic.linux-d160-m8xlarge-arm64.key-name: konflux-prod-ext-mab01
dynamic.linux-d160-m8xlarge-arm64.aws-secret: aws-account
dynamic.linux-d160-m8xlarge-arm64.ssh-secret: aws-ssh-key
dynamic.linux-d160-m8xlarge-arm64.security-group-id: sg-0fbf35ced0d59fd4a
dynamic.linux-d160-m8xlarge-arm64.max-instances: "5" |
This is a copy of 75ebe71 (PR redhat-appstudio#7409) for stone-prd-rh01.
63b1584
to
ca1dd91
Compare
Code Review by Gemini## Code Review
### Issues and Suggestions
1. **Potential Invalid AWS Instance Type for ARM64**
* **Issue**: The `instance-type` for `dynamic.linux-d160-m8-8xlarge-arm64` is set to `m8g.8xlarge`. As of current AWS offerings, `m8g` instance types are not generally available. The commit title "Enable newer arm64 instance types (m7g+)" suggests that `m7g` (Graviton3) or newer generations are intended. If `m8g` is a typo and `m7g` was intended, this configuration will fail to provision instances.
* **Suggestion**: Verify if `m8g.8xlarge` is a valid and intended instance type for this environment. If `m7g.8xlarge` was intended, please update the configuration.
```diff
--- a/components/multi-platform-controller/production/stone-prd-rh01/host-config.yaml
+++ b/components/multi-platform-controller/production/stone-prd-rh01/host-config.yaml
@@ -140,7 +140,7 @@
dynamic.linux-d160-m8-8xlarge-arm64.type: aws
dynamic.linux-d160-m8-8xlarge-arm64.region: us-east-1
dynamic.linux-d160-m8-8xlarge-arm64.ami: ami-03d6a5256a46c9feb
- dynamic.linux-d160-m8-8xlarge-arm64.instance-type: m8g.8xlarge
+ dynamic.linux-d160-m8-8xlarge-arm64.instance-type: m7g.8xlarge # Or confirm if m8g is a custom/future type
dynamic.linux-d160-m8-8xlarge-arm64.instance-tag: prod-arm64-m8-8xlarge-d160
dynamic.linux-d160-m8-8xlarge-arm64.key-name: konflux-prod-ext-mab01
dynamic.linux-d160-m8-8xlarge-arm64.aws-secret: aws-account
```
2. **Inconsistent `max-instances` for new types**
* **Observation**: The `max-instances` for `dynamic.linux-d160-m8-8xlarge-arm64` is set to `"5"`, while for `dynamic.linux-d160-m7-8xlarge-amd64` it is set to `"10"`.
* **Suggestion**: Confirm if this difference in scaling limits between the new ARM64 and AMD64 instance types is intentional. If not, align them as appropriate. (No specific line change suggested as this might be intentional).
3. **Minor Whitespace Changes**
* **Observation**: There are minor whitespace changes (blank lines added/removed) in the `init-script-arm64` and `init-script-amd64` sections. These do not affect functionality but can sometimes make diffs harder to read.
* **Suggestion**: While not a bug, it's generally good practice to avoid unrelated whitespace changes in a PR unless they are part of a larger formatting effort. (No specific line change suggested as this is purely stylistic and harmless). |
This is a copy of 75ebe71 (PR #7409) for stone-prd-rh01.