feat(aws): add --instance-families flag to filter instance selector output - #872
Open
amastbau wants to merge 7 commits into
Open
feat(aws): add --instance-families flag to filter instance selector output#872amastbau wants to merge 7 commits into
amastbau wants to merge 7 commits into
Conversation
added 3 commits
July 30, 2026 00:07
Post-filters getInstanceTypes() output to only instance types whose family prefix matches the allowlist. Bypassed when ComputeSizes is set. Fixes: redhat-developer#684
Comma-separated allowlist of AWS family prefixes (e.g. m5,m6i,m7i). Post-filters instance selector output. No-op when --compute-sizes is set.
Passes --instance-families to mapt when set. Conditional: only in the else branch (when compute-sizes is empty) since compute-sizes bypasses the selector entirely.
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds an optional AWS instance-family allowlist to compute request arguments, filters computed instance types by exact family prefixes, and passes the setting through CLI and Tekton provisioning commands when ChangesInstance-family filtering
Operator-channel wiring
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant TektonTask
participant MaptCLI
participant ComputeRequest
participant AWSInstanceSelector
TektonTask->>MaptCLI: Pass --instance-families during create
MaptCLI->>ComputeRequest: Populate InstanceFamilies
ComputeRequest->>AWSInstanceSelector: Request instance types
AWSInstanceSelector->>AWSInstanceSelector: Filter exact family prefixes
AWSInstanceSelector-->>ComputeRequest: Return filtered types capped by MaxResults
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/provider/aws/data/compute-request.go`:
- Line 52: Update the compute request filtering flow around filterByFamily so
InstanceFamilies is applied before MaxResults truncates selector results. When
args.InstanceFamilies is provided, avoid the premature cap or expand it
sufficiently while filtering the allowlisted families, then apply the requested
result limit to the filtered set.
In `@tkn/template/infra-aws-ocp-snc.yaml`:
- Around line 268-270: Stop interpolating the externally supplied
instance-families value into the eval-based command construction; build the
command as an argument list and invoke it directly with safe quoting or a Bash
array. Apply the fix to tkn/template/infra-aws-ocp-snc.yaml lines 268-270, then
regenerate or apply the equivalent change to tkn/infra-aws-ocp-snc.yaml lines
268-270; make the same change in tkn/template/infra-aws-rhel.yaml lines 283-285
and tkn/infra-aws-rhel.yaml lines 283-285.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b3182e54-14b7-4474-af4a-489a865a6fdd
📒 Files selected for processing (8)
cmd/mapt/cmd/params/params.gopkg/provider/api/compute-request/compute-request.gopkg/provider/aws/data/compute-request.gopkg/provider/aws/data/compute-request_test.gotkn/infra-aws-ocp-snc.yamltkn/infra-aws-rhel.yamltkn/template/infra-aws-ocp-snc.yamltkn/template/infra-aws-rhel.yaml
added 3 commits
July 30, 2026 01:15
When InstanceFamilies is set, skip MaxResults in the selector so allowlisted families ranked outside the top 20 are not silently dropped. Cap to MaxResults manually after filterByFamily.
Exposes the --operator-channel CLI flag as a Tekton task param so callers can override OLM subscription channels per operator. Example: rhods-operator=stable-3.x to install RHOAI 3.x instead of the default stable (2.25.x) channel.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tkn/infra-aws-ocp-snc.yaml (1)
274-276: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not interpolate these parameters into an
evalcommand.The assembled command is executed with
evalat Line 325.operator-channelis inserted unquoted, allowing values such aspackage=stable; ...to execute arbitrary shell commands; an apostrophe or command substitution ininstance-familiescan similarly escape its wrapper. Because the task loads AWS credentials, this can expose credentials or alter provisioning.Build the command as a Bash argument array and invoke it without
eval, or strictly validate and shell-escape both parameters before appending them; also reject malformedpackage=channelentries.As per path instructions, focus on major issues impacting security and avoid nitpicks and verbosity.
Also applies to: 294-299
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tkn/infra-aws-ocp-snc.yaml` around lines 274 - 276, Replace the eval-based command construction with a Bash argument array and invoke it directly, preserving each parameter as a separate argument. Safely pass params.instance-families and operator-channel without interpolation, and validate operator-channel as a well-formed package=channel entry before execution. Ensure malformed or unexpected values are rejected before the AWS provisioning command runs.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tkn/infra-aws-ocp-snc.yaml`:
- Around line 274-276: Replace the eval-based command construction with a Bash
argument array and invoke it directly, preserving each parameter as a separate
argument. Safely pass params.instance-families and operator-channel without
interpolation, and validate operator-channel as a well-formed package=channel
entry before execution. Ensure malformed or unexpected values are rejected
before the AWS provisioning command runs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 27bc3a1f-0472-444d-8cfe-c8c48cdba136
📒 Files selected for processing (1)
tkn/infra-aws-ocp-snc.yaml
Verifies that allowlisted families ranked outside the top MaxResults are not silently dropped when InstanceFamilies is set.
ppitonak
requested changes
Jul 30, 2026
ppitonak
left a comment
ppitonak
left a comment
Collaborator
There was a problem hiding this comment.
Any reason not to update other infra-aws-* Tekton tasks?
Collaborator
|
I tested spot instances and it seems to work fine. |
Author
done! |
adrianriobo
reviewed
Jul 30, 2026
| nestedVirtDesc string = "Use cloud instance that has nested virtualization support" | ||
| computeSizes string = "compute-sizes" | ||
| computeSizesDesc string = "Comma seperated list of sizes for the machines to be requested. If set this takes precedence over compute by args" | ||
| instanceFamilies string = "instance-families" |
Collaborator
There was a problem hiding this comment.
Can we name it computeFamilies as this may / should have a matching functionality in azure?
Collaborator
|
Aso rebase and fix conflicts |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
--instance-familiesflag (comma-separated allowlist of AWS family prefixes, e.g.m5,m6i,m7i) that post-filters the EC2 instance selector output to only matching families.Fixes #684
Problem
When using
--cpusand--memory, the instance selector returns up to 20 types including expensive specialized families (d3en, p3, x1e — dense storage, GPU, high-memory) alongside cheap general-purpose ones. Spot picks the cheapest at that moment, which may be a temporarily cheap specialized instance. Example:d3en.12xlargeandm5a.12xlargehave identical vCPU/Memory but the former is ~3x more expensive.Changes
pkg/provider/api/compute-request/compute-request.go— addInstanceFamilies []stringfield toComputeRequestArgspkg/provider/aws/data/compute-request.go— extractfilterByFamily()helper; skipMaxResultscap in selector when family filter active; apply cap manually after filteringpkg/provider/aws/data/compute-request_test.go— 8 unit tests forfilterByFamily()including cap-after-filter regressioncmd/mapt/cmd/params/params.go— add--instance-familiesflag, populate fieldtkn/template/infra-aws-ocp-snc.yaml,tkn/template/infra-aws-rhel.yaml— addinstance-familiesparam; pass flag in else branch of compute-sizes conditionaltkn/infra-aws-ocp-snc.yaml,tkn/infra-aws-rhel.yamlBehavior
--instance-families m5,m6i,m7i→ selector returns onlym5.*,m6i.*,m7i.*types--instance-familiesis a no-op when--compute-sizesis set (compute-sizes bypasses the selector entirely — Tekton template also gates the param in the else branch)--instance-families(default) → no restriction, existing behavior unchangedstrings.HasPrefix(t, fam+".")) som5does not matchm5aCap fix
When
InstanceFamiliesis set,MaxResultsis no longer passed toFilterVerbose. Without this fix, the selector could return 20 results all outside the allowlist (e.g. GPU/storage types ranked highest), leaving nothing afterfilterByFamilyeven though matching types exist beyond position 20. The cap is now applied after filtering.Verification — live AWS API (us-east-1)
Tested against real
DescribeInstanceTypesAPI using theamazon-ec2-instance-selectorlibrary (same library mapt uses internally).4 vCPU / 16 GiB
Without
--instance-families— 20 types returned, including GPU and storage-optimized:With
--instance-families m5,m6i,m7i:Note:
m5a.xlargecorrectly excluded — dot-separator enforced (m5does not matchm5a).8 vCPU / 64 GiB
Without
--instance-families:With
--instance-families r5,r6i:Full e2e (mapt CLI → EC2 → destroy)
Debug log confirms:
Requesting an on-demand instance of type: m5.xlargeInstance provisioned, SSH reachable, destroyed cleanly (14 resources). No GPU or storage-optimized type selected.
🤖 Generated with Claude Code