feat(tkn): add compute-sizes, GPU, and spot-excluded-regions params to SNC task#867
Merged
adrianriobo merged 1 commit intoJul 27, 2026
Merged
Conversation
…o SNC task The SNC Tekton task template was missing several params that the Go CLI already supports via AddComputeRequestFlags(). This prevented Tekton pipeline users from specifying exact instance types or GPU requirements for SNC clusters. Add compute-sizes, gpus, gpu-manufacturer, and spot-excluded-regions params to the SNC task template, matching the pattern already used in the RHEL task template. When compute-sizes is set, it takes precedence over cpus/memory/gpus args. Closes redhat-developer#858 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe SNC Tekton task and template add explicit compute-size, GPU, and spot-excluded-region parameters. Cluster creation conditionally passes compute sizes or CPU, memory, and GPU flags, while spot arguments include excluded regions and quoted values. ChangesSNC provisioning configuration
Estimated code review effort: 3 (Moderate) | ~20 minutes 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tkn/infra-aws-ocp-snc.yaml`:
- Around line 258-265: Remove eval-based command-string construction from the
mapt invocation. In tkn/infra-aws-ocp-snc.yaml at lines 258-265 and 283-287, and
tkn/template/infra-aws-ocp-snc.yaml at lines 258-265 and 283-287, build
arguments with safe quoting and invoke mapt directly for compute/GPU and spot
parameters, preserving the existing parameter selection behavior.
🪄 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: a68abbb3-1b59-4aa9-9d4c-de13458eae8c
📒 Files selected for processing (2)
tkn/infra-aws-ocp-snc.yamltkn/template/infra-aws-ocp-snc.yaml
Comment on lines
+258
to
+265
| if [[ "$(params.compute-sizes)" != "" ]]; then | ||
| cmd+="--compute-sizes '$(params.compute-sizes)' " | ||
| else | ||
| cmd+="--cpus '$(params.cpus)' " | ||
| cmd+="--memory '$(params.memory)' " | ||
| cmd+="--gpus '$(params.gpus)' " | ||
| cmd+="--gpu-manufacturer '$(params.gpu-manufacturer)' " | ||
| fi |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
Remove eval from the new parameter wiring.
The compute, GPU, and spot values are inserted into a shell command string and reparsed by eval at Line [307]. The surrounding quotes do not safely escape Tekton substitutions, allowing caller-controlled shell syntax to execute with AWS credentials.
tkn/infra-aws-ocp-snc.yaml#L258-L265: build quoted arguments and directly invokemaptfor compute and GPU parameters.tkn/infra-aws-ocp-snc.yaml#L283-L287: apply the same safe argument handling to spot parameters.tkn/template/infra-aws-ocp-snc.yaml#L258-L265: update the source template's compute and GPU argument handling.tkn/template/infra-aws-ocp-snc.yaml#L283-L287: update the source template's spot argument handling.
As per path instructions, this is a major security issue rather than a shell-style nit.
📍 Affects 2 files
tkn/infra-aws-ocp-snc.yaml#L258-L265(this comment)tkn/infra-aws-ocp-snc.yaml#L283-L287tkn/template/infra-aws-ocp-snc.yaml#L258-L265tkn/template/infra-aws-ocp-snc.yaml#L283-L287
🤖 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 258 - 265, Remove eval-based
command-string construction from the mapt invocation. In
tkn/infra-aws-ocp-snc.yaml at lines 258-265 and 283-287, and
tkn/template/infra-aws-ocp-snc.yaml at lines 258-265 and 283-287, build
arguments with safe quoting and invoke mapt directly for compute/GPU and spot
parameters, preserving the existing parameter selection behavior.
Source: Path instructions
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
2 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
compute-sizes,gpus,gpu-manufacturer, andspot-excluded-regionsparams to the SNC Tekton task templatecompute-sizesis set, it takes precedence overcpus/memory/gpusargs (same pattern as RHEL task)tkn/infra-aws-ocp-snc.yamlviamake tkn-updateCloses #858
Verification results
Script logic dry-run (3 scenarios)
m5.xlarge,m5.2xlarge--compute-sizesused, no--cpus/--memory/--gpus, all spot flags present--cpus/--memory/--gpus/--gpu-manufacturer, spot flags presentc5.4xlarge--compute-sizesused, zero spot flagsLive Tekton TaskRun (OCP 4.21.0 cluster)
Deployed SNO cluster, installed OpenShift Pipelines v1.22.4, applied the modified task, and ran a TaskRun with
debug=trueto inspect the built command. Debug output confirms correct param resolution:--compute-sizespresent,--cpus/--memory/--gpuscorrectly absent (if/else branch works)--spot-excluded-regionspassed through with correct value--cpus/--memoryleakage whencompute-sizesis setTest plan
<IMAGE>/<VERSION>substitution differs)compute-sizesset — confirmed--compute-sizesused, no fallback paramsspot-excluded-regions— confirmed value passed throughcompute-sizes— confirm fallback tocpus/memory/gpusworks as before (covered by dry-run scenario 2)🤖 Generated with Claude Code