Repository navigation
OSAC-3162: add GpuSpec to ComputeInstance CRD - #217
Conversation
|
@Tzif-Morgen: This pull request references OSAC-3162 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughThe ComputeInstance API now supports optional GPU passthrough configuration. The CRD validates GPU count, PCI selector, resource name, and immutability. Deepcopy methods and validation tests cover the new field. ChangesGPU specification
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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 `@osac-operator/api/v1alpha1/computeinstance_types.go`:
- Around line 59-62: Update the PciDeviceSelector validation markers to require
exactly four hexadecimal vendor digits, a colon, and four hexadecimal device
digits using an allow-list pattern; regenerate both CRD manifests from the
updated API type and add a test confirming malformed selectors such as “invalid”
are rejected.
In `@osac-operator/internal/controller/computeinstance_validation_test.go`:
- Around line 509-515: Update the test case around createValidInstance and the
gpu omitempty behavior to marshal the instance using encoding/json, then assert
the serialized JSON does not contain the "gpu" property. Remove the
create-and-get assertions, since they cannot distinguish omission from a null
value.
🪄 Autofix
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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3f36f1b6-cb5f-4835-b1b2-ffe16fb8b620
📒 Files selected for processing (6)
osac-operator/api/v1alpha1/computeinstance_types.goosac-operator/api/v1alpha1/computeinstance_types_test.goosac-operator/api/v1alpha1/zz_generated.deepcopy.goosac-operator/charts/operator-crds/templates/osac.openshift.io_computeinstances.yamlosac-operator/config/crd/bases/osac.openshift.io_computeinstances.yamlosac-operator/internal/controller/computeinstance_validation_test.go
8a6547b to
d90176d
Compare
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
|
🤖 Finished Review · ✅ Success · Started 2:31 PM UTC · Completed 2:48 PM UTC Commit: |
|
/lgtm |
Add XValidation immutability rule to the Gpu field, consistent with all other hardware fields (cores, memoryGiB, bootDisk, etc.). Fix the optionality marker from // +optional to // +kubebuilder:validation:Optional to match codebase convention. Signed-off-by: Tzif <tmorgens@redhat.com> Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Tzif <tmorgens@redhat.com>
d90176d to
f9c1c6f
Compare
Auto-dismissed: only Prow labels gate merging
|
🤖 Review · ❌ Terminated · Started 7:02 AM UTC · Ended 7:15 AM UTC Commit: |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
the
ready-for-merge
|
🤖 Finished Review · ✅ Success · Started 7:02 AM UTC · Completed 7:15 AM UTC Commit: |
|
@ygalblum I Addressed fullsend review feedback - added GPU immutability rule (consistent with cores/memoryGiB/etc.) and fixed the optional marker convention. Can you give another lgtm? |
Move GPU immutability from field-level self == oldSelf to spec-level has() check to also block absent-to-present transitions. Add test for the absent→present case. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Tzif <tmorgens@redhat.com>
|
🤖 Review · ❌ Terminated · Started 2:27 PM UTC · Ended 2:45 PM UTC Commit: |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Tzif-Morgen, ygalblum The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
|
||
| Describe("MaxItems validation", func() { | ||
| It("should reject creating ComputeInstance with more than 8 networkAttachments", func() { | ||
| instance := createValidInstance("test-max-attachments") |
There was a problem hiding this comment.
[low] test adequacy
The GPU immutability tests cover 'reject changing gpu' and 'reject adding gpu after creation', but do not cover the symmetric case of removing GPU after creation (creating with GPU set, then updating with Gpu set to nil). The CEL rule has(self.gpu) == has(oldSelf.gpu) correctly rejects this case via the same sub-expression, but the behavior is untested. Other immutable fields in this file test both directions.
Suggested fix: Add a test case 'should reject removing gpu after creation' that creates an instance with a valid GpuSpec, fetches it, sets Gpu to nil, and asserts that Update returns an IsInvalid error containing 'gpu is immutable'.
| } | ||
|
|
||
| // ComputeInstanceSpec defines the desired state of ComputeInstance | ||
| // +kubebuilder:validation:XValidation:rule="has(self.gpu) == has(oldSelf.gpu) && (!has(self.gpu) || self.gpu == oldSelf.gpu)",message="gpu is immutable" |
There was a problem hiding this comment.
[low] pattern-divergence
The GPU immutability rule is enforced at the spec level via XValidation on ComputeInstanceSpec using has() checks, while other immutable optional fields use field-level self == oldSelf. The spec-level approach is intentionally more robust for optional struct pointers. A brief code comment explaining this design choice would help future contributors.
Suggested fix: Add a comment above the XValidation rule explaining that has() checks are needed because field-level self == oldSelf does not prevent nil-to-value transitions on optional struct pointer fields.
|
🤖 Finished Review · ✅ Success · Started 2:27 PM UTC · Completed 2:45 PM UTC Commit: |
This branch was previously deployed
4 participants
OSAC-3162: add GpuSpec to ComputeInstance CRD
Jira: OSAC-3162
Story type: [DEV]
Epic: OSAC-3158 — GPU ComputeInstance Provisioning
Summary
Adds a
GpuSpecstruct and optionalGpufield to the ComputeInstance CRD, following the existingImageSpec/DiskSpecnested struct pattern. This enables the fulfillment reconciler (OSAC-3182) to stamp GPU configuration from InstanceTypes onto ComputeInstance CRs for the operator to consume during provisioning.Changes
osac-operator/api/v1alpha1/computeinstance_types.go— AddedGpuSpecstruct withPciDeviceSelector(string),ResourceName(string), andCount(int32) fields. Added optionalGpu *GpuSpecfield toComputeInstanceSpec.PciDeviceSelector,ResourceName: Required, MinLength=1Count: Required, Minimum=1, Maximum=16Gpufield: optional withomitemptymake manifests generateand synced to Helm charts viamake helm-crdsTesting
GpuSpecfield construction and access onComputeInstanceSpeccount = 0rejected (below minimum)count = 17rejected (above maximum)pciDeviceSelectorrejectedresourceNamerejectedGpuSpectype are tested via public interfacesAcceptance Criteria
Summary by CodeRabbit