feat(aws): add --instance-families flag to filter compute selector output - #872
feat(aws): add --instance-families flag to filter compute selector output#872amastbau wants to merge 8 commits into
Conversation
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 compute-family allowlist to compute request arguments. AWS instance types use exact family-prefix matching. CLI and Tekton create commands pass the allowlist only when ChangesInstance-family filtering
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant TektonTask
participant MaptCLI
participant ComputeRequest
participant AWSInstanceSelector
TektonTask->>MaptCLI: Pass --compute-families during create
MaptCLI->>ComputeRequest: Populate ComputeFamilies
ComputeRequest->>AWSInstanceSelector: Request instance types
AWSInstanceSelector->>AWSInstanceSelector: Filter exact family prefixes
AWSInstanceSelector-->>ComputeRequest: Return filtered types capped by MaxResults
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
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
left a comment
ppitonak
left a comment
There was a problem hiding this comment.
Any reason not to update other infra-aws-* Tekton tasks?
|
I tested spot instances and it seems to work fine. |
done! |
| 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" |
There was a problem hiding this comment.
Can we name it computeFamilies as this may / should have a matching functionality in azure?
There was a problem hiding this comment.
done.but i have not tested azure with the new code.not sure i have access.
|
Aso rebase and fix conflicts |
…ibility Rename all references from instanceFamilies/InstanceFamilies to computeFamilies/ComputeFamilies throughout codebase to support Azure VM family naming (e.g. StandardD8v3Family). Changes: - pkg/provider/api/compute-request: ComputeFamilies field with AWS/Azure docs - cmd/mapt/cmd/params: --compute-families flag and viper binding - pkg/provider/aws/data: use ComputeFamilies in selector and filter - tkn/template: compute-families param in SNC and RHEL tasks - Regenerated tkn/*.yaml Per @adrianriobo review comment on PR redhat-developer#872. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
amastbau
changed the title
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_test.go`:
- Around line 58-79: Update TestFilterByFamily_CapAppliedAfterFilter to exercise
the production filter-and-cap path through getInstanceTypes, using a stubbed
selector or equivalent test setup. Configure computerequest.MaxResults and
assert that matching m6i values beyond the cap remain available, rather than
testing filterByFamily directly.
In `@tkn/infra-aws-ocp-snc.yaml`:
- Around line 268-270: Replace shared eval-based command construction at
tkn/infra-aws-ocp-snc.yaml lines 268-270, tkn/infra-aws-rhel.yaml lines 283-285,
and tkn/template/infra-aws-rhel.yaml lines 283-285 with argument-vector
handling: pass compute-families as a separate data argument, validate each
family token before execution, and preserve omission when the value is empty.
Apply the same change consistently in all three files.
🪄 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: ab517f8b-016f-4e79-a9b3-5e51d1c2f644
📒 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
🚧 Files skipped from review as they are similar to previous changes (3)
- tkn/template/infra-aws-ocp-snc.yaml
- pkg/provider/aws/data/compute-request.go
- cmd/mapt/cmd/params/params.go
| // TestFilterByFamily_CapAppliedAfterFilter verifies that when ComputeFamilies | ||
| // is set, matching types beyond MaxResults are still reachable by filterByFamily | ||
| // (i.e. the cap is applied after filtering, not before). This guards the fix for | ||
| // the bug where FilterVerbose capped results before filterByFamily ran, silently | ||
| // dropping allowlisted families ranked outside the top MaxResults. | ||
| func TestFilterByFamily_CapAppliedAfterFilter(t *testing.T) { | ||
| // Build 25 types: 5 non-matching (d3en) followed by 20 m6i types. | ||
| // If cap were applied before filter, the 5 d3en types would consume cap slots | ||
| // and only 15 m6i types would survive. With cap-after-filter all 20 survive. | ||
| var types []string | ||
| for i := 0; i < 5; i++ { | ||
| types = append(types, "d3en.12xlarge") | ||
| } | ||
| for i := 0; i < 20; i++ { | ||
| types = append(types, "m6i.xlarge") | ||
| } | ||
|
|
||
| got := filterByFamily(types, []string{"m6i"}) | ||
| if len(got) != 20 { | ||
| t.Errorf("got %d results, want 20 — cap must be applied after filter, not before", len(got)) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the cap-order regression test exercise the production path.
TestFilterByFamily_CapAppliedAfterFilter calls only filterByFamily. That helper does not apply computerequest.MaxResults, so the test passes even if getInstanceTypes truncates results before filtering.
Test the filter-and-cap composition, or invoke getInstanceTypes with a stubbed selector. Assert that the allowlisted values beyond the cap remain in the result.
🤖 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 `@pkg/provider/aws/data/compute-request_test.go` around lines 58 - 79, Update
TestFilterByFamily_CapAppliedAfterFilter to exercise the production
filter-and-cap path through getInstanceTypes, using a stubbed selector or
equivalent test setup. Configure computerequest.MaxResults and assert that
matching m6i values beyond the cap remain available, rather than testing
filterByFamily directly.
| if [[ "$(params.compute-families)" != "" ]]; then | ||
| cmd+="--compute-families '$(params.compute-families)' " | ||
| fi |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Remove the shared eval-based command construction.
The new compute-families value is raw shell input. A permitted TaskRun or PipelineRun author can trigger command substitution or quote breaking, and the task exposes AWS credentials. Pass the value as data in an argument vector and validate family tokens before execution.
tkn/infra-aws-ocp-snc.yaml#L268-L270: passcompute-familiesas a separate argument without adding it to an evaluated command string.tkn/infra-aws-rhel.yaml#L283-L285: apply the same safe argument handling in the generated RHEL task.tkn/template/infra-aws-rhel.yaml#L283-L285: apply the same safe argument handling in the RHEL source template.
As per path instructions, focus on major issues impacting performance, readability, maintainability and security; avoid nitpicks and verbosity.
📍 Affects 3 files
tkn/infra-aws-ocp-snc.yaml#L268-L270(this comment)tkn/infra-aws-rhel.yaml#L283-L285tkn/template/infra-aws-rhel.yaml#L283-L285
🤖 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 268 - 270, Replace shared eval-based
command construction at tkn/infra-aws-ocp-snc.yaml lines 268-270,
tkn/infra-aws-rhel.yaml lines 283-285, and tkn/template/infra-aws-rhel.yaml
lines 283-285 with argument-vector handling: pass compute-families as a separate
data argument, validate each family token before execution, and preserve
omission when the value is empty. Apply the same change consistently in all
three files.
Source: Path instructions
3 participants
Summary
Adds
--compute-familiesflag (comma-separated allowlist of family prefixes, e.g.m5,m6i,m7ifor AWS) that post-filters the compute selector output to only matching families.Renamed from
--instance-familiesto--compute-familiesper @adrianriobo review comment to support Azure (which uses family-based naming likeStandardD8v3Family).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— addComputeFamilies []stringfield toComputeRequestArgswith AWS and Azure examples in doc commentpkg/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--compute-familiesflag, populate fieldtkn/template/infra-aws-ocp-snc.yaml,tkn/template/infra-aws-rhel.yaml— addcompute-familiesparam; pass flag in else branch of compute-sizes conditionaltkn/infra-aws-ocp-snc.yaml,tkn/infra-aws-rhel.yamlBehavior
--compute-families m5,m6i,m7i→ selector returns onlym5.*,m6i.*,m7i.*types--compute-familiesis a no-op when--compute-sizesis set (compute-sizes bypasses the selector entirely — Tekton template also gates the param in the else branch)--compute-families(default) → no restriction, existing behavior unchangedstrings.HasPrefix(t, fam+".")) som5does not matchm5aCross-cloud naming
m5inm5.xlarge,c6iinc6i.2xlarge)Familyfield with "Family" suffix (e.g.StandardD8v3Family,StandardE16v4Family)--compute-families D8v3,E16v4Cap fix
When
ComputeFamiliesis 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 — AWS E2E
Unit tests (8/8 pass)
All tests pass including:
m5excludesm5a)Live AWS API test (ComputeSelector with real DescribeInstanceTypes)
Test 1: 4 vCPU / 16 GiB with
--compute-families m5,m6iTest 2: 4 vCPU / 16 GiB with
--compute-families m5onlyFull E2E (mapt CLI → EC2 provision → destroy)
Results:
Requesting an on-demand instance of type: m5.xlargei-0919271674561e88etypem5.xlargecreatedNo GPU or storage-optimized type selected — filter worked correctly in full provision flow.
Comparison (no filter vs with filter)
Without
--compute-families(4 vCPU / 16 GiB):With
--compute-families m5,m6i:🤖 Generated with Claude Code