OSAC-3182: stamp GPU fields from InstanceType onto ComputeInstance CR

[Tzif-Morgen]

OSAC-3182: stamp GPU fields from InstanceType onto ComputeInstance CR

Jira: OSAC-3182
Story type: [DEV]
Depends on: #217 (OSAC-3162 — GpuSpec CRD type)

Summary

Extends the ComputeInstance reconciler's addExplicitFields method to read GPU data from the resolved InstanceType and stamp it onto the K8s ComputeInstance CR spec. Follows the same pattern as the existing cores/memory stamping. When the InstanceType has no GPU, the CR's gpu field remains nil.

Changes

• fulfillment-service/internal/controllers/computeinstance/computeinstance_reconciler_function.go

  • Added GPU nil-check and osacv1alpha1.GpuSpec construction after cores/memory assignment in addExplicitFields

• fulfillment-service/internal/controllers/computeinstance/computeinstance_reconciler_function_test.go

  • Added 3 unit tests in the "instance_type resolution in reconciler" describe block:
    • "resolves instance_type gpu fields onto CR spec" — verifies all 3 GPU fields
    • "leaves gpu nil when InstanceType has no gpu" — verifies nil preservation
    • "gpu stamping is idempotent" — verifies repeated calls produce identical results

Testing

• Unit tests: 3 new Ginkgo tests covering GPU present, GPU absent, and idempotency
• Integration tests: Deferred to OSAC-3184 (QE story)
• Coverage: All behavioral paths through the GPU stamping code are exercised through the public buildSpec interface

Acceptance Criteria

• AC-1: GPU-enabled InstanceType → reconciler stamps gpu struct (pciDeviceSelector, resourceName, count) onto CR
• AC-2: Non-GPU InstanceType → CR gpu field remains nil
• AC-3: GPU stamping is idempotent — re-reconciliation produces no change
• AC-4: GPU ComputeInstances carry the same tenant isolation metadata as non-GPU instances

Summary by CodeRabbit

• New Features

  • Compute instances now support optional GPU passthrough configuration.
  • GPU settings include device selection, resource name, and device count.
  • GPU configurations are validated, limited to 1–16 devices, and immutable after creation.
  • GPU settings are automatically applied when resolving GPU-enabled instance types.

• Bug Fixes

  • Ensured GPU settings remain consistent when compute instance specifications are rebuilt.

[openshift-ci-robot]

@Tzif-Morgen: This pull request references OSAC-3182 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.

Details

In response to this:

OSAC-3182: stamp GPU fields from InstanceType onto ComputeInstance CR

Jira: OSAC-3182
Story type: [DEV]
Depends on: #217 (OSAC-3162 — GpuSpec CRD type)

Summary

Extends the ComputeInstance reconciler's addExplicitFields method to read GPU data from the resolved InstanceType and stamp it onto the K8s ComputeInstance CR spec. Follows the same pattern as the existing cores/memory stamping. When the InstanceType has no GPU, the CR's gpu field remains nil.

Changes

• fulfillment-service/internal/controllers/computeinstance/computeinstance_reconciler_function.go

• Added GPU nil-check and osacv1alpha1.GpuSpec construction after cores/memory assignment in addExplicitFields

• fulfillment-service/internal/controllers/computeinstance/computeinstance_reconciler_function_test.go

• Added 3 unit tests in the "instance_type resolution in reconciler" describe block:

  • "resolves instance_type gpu fields onto CR spec" — verifies all 3 GPU fields
  • "leaves gpu nil when InstanceType has no gpu" — verifies nil preservation
  • "gpu stamping is idempotent" — verifies repeated calls produce identical results

Testing

• Unit tests: 3 new Ginkgo tests covering GPU present, GPU absent, and idempotency
• Integration tests: Deferred to OSAC-3184 (QE story)
• Coverage: All behavioral paths through the GPU stamping code are exercised through the public buildSpec interface

Acceptance Criteria

• AC-1: GPU-enabled InstanceType → reconciler stamps gpu struct (pciDeviceSelector, resourceName, count) onto CR
• AC-2: Non-GPU InstanceType → CR gpu field remains nil
• AC-3: GPU stamping is idempotent — re-reconciliation produces no change
• AC-4: GPU ComputeInstances carry the same tenant isolation metadata as non-GPU instances

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.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 337d0dc8-f752-4017-b4d3-e7eaf639d618

📥 Commits

Reviewing files that changed from the base of the PR and between 135b6d5 and 47d6254.

📒 Files selected for processing (4)

• osac-operator/api/v1alpha1/computeinstance_types.go
• osac-operator/charts/operator-crds/templates/osac.openshift.io_computeinstances.yaml
• osac-operator/config/crd/bases/osac.openshift.io_computeinstances.yaml
• osac-operator/internal/controller/computeinstance_validation_test.go

🚧 Files skipped from review as they are similar to previous changes (4)

• osac-operator/config/crd/bases/osac.openshift.io_computeinstances.yaml
• osac-operator/api/v1alpha1/computeinstance_types.go
• osac-operator/internal/controller/computeinstance_validation_test.go
• osac-operator/charts/operator-crds/templates/osac.openshift.io_computeinstances.yaml

---

Walkthrough

The change adds optional immutable GPU settings to ComputeInstanceSpec, defines CRD validation and deepcopy behavior, and propagates resolved GPU selector, resource name, and count values during ComputeInstance reconciliation.

Changes

GPU configuration

Layer / File(s)	Summary
GPU contract and schema osac-operator/api/v1alpha1/computeinstance_types.go, osac-operator/api/v1alpha1/zz_generated.deepcopy.go, osac-operator/charts/operator-crds/templates/..., osac-operator/config/crd/bases/..., osac-operator/api/v1alpha1/computeinstance_types_test.go	Defines GpuSpec and optional immutable ComputeInstanceSpec.Gpu. CRD schemas require the selector, resource name, and a count from 1 to 16. Deepcopy support and valid-spec coverage are included.
Resolved GPU propagation fulfillment-service/internal/controllers/computeinstance/computeinstance_reconciler_function.go, fulfillment-service/internal/controllers/computeinstance/computeinstance_reconciler_function_test.go	Copies resolved GPU settings into generated specs. Tests cover GPU and non-GPU instance types and repeated spec builds.
GPU validation coverage osac-operator/internal/controller/computeinstance_validation_test.go	Tests omission, JSON serialization, required fields, count bounds, and immutability after creation.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant InstanceType
  participant Reconciler
  participant ComputeInstanceSpec
  InstanceType->>Reconciler: provide resolved GPU settings
  Reconciler->>ComputeInstanceSpec: set selector, resource name, and count
  ComputeInstanceSpec-->>Reconciler: return generated spec

Loading

Suggested reviewers: larsks, vladikr, eranco74, omer-vishlitzky

🚥 Pre-merge checks | ✅ 11

✅ Passed checks (11 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes stamping GPU fields from InstanceType onto the ComputeInstance CR, which is the main change.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
No-Hardcoded-Secrets	✅ Passed	The PR additions contain only GPU identifiers, schema validation, and test fixtures; scans found no credential assignments, private-key material, embedded-credential URLs, or long base64/hex secrets.
No-Weak-Crypto	✅ Passed	The PR only adds GPU fields, validation, deepcopy code, and reconciler mapping; added lines contain no weak crypto, custom crypto, or secret/token comparisons.
No-Injection-Vectors	✅ Passed	The commit adds only typed GPU field copying and tests. Changed files contain no SQL concatenation, shell execution, eval/exec, unsafe YAML/pickle loading, or HTML injection.
Container-Privileges	✅ Passed	The PR adds GPU API/CRD fields and reconciler mapping only; no added privileged:true, hostPID, hostNetwork, hostIPC, SYS_ADMIN, allowPrivilegeEscalation:true, or root execution.
No-Sensitive-Data-In-Logs	✅ Passed	The diff adds GPU field assignment and tests only. It adds no logging calls or log arguments, and the GPU values are hardware/resource identifiers, not sensitive data listed by this check.
Ai-Attribution	✅ Passed	All four PR-range commits include an Assisted-by: Claude Code trailer, and no Co-Authored-By marker appears.

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 8:23 AM UTC · Ended 8:24 AM UTC

Commit: 135b6d5 · View workflow run →

[Tzif-Morgen]

/hold
Depends on: #217 (OSAC-3162 - GpuSpec CRD type)

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 8:25 AM UTC · Ended 8:31 AM UTC

Commit: 135b6d5 · View workflow run →

[coderabbitai]

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 `@osac-operator/api/v1alpha1/computeinstance_types.go`:
- Around line 159-162: Move the GPU immutability XValidation from GpuSpec to
ComputeInstanceSpec, using a rule that compares both GPU presence and value so
absent-to-present updates are rejected; update
osac-operator/api/v1alpha1/computeinstance_types.go accordingly. Regenerate the
CRD validation in
osac-operator/charts/operator-crds/templates/osac.openshift.io_computeinstances.yaml
lines 124-151 and
osac-operator/config/crd/bases/osac.openshift.io_computeinstances.yaml lines
122-149. Add coverage in
osac-operator/internal/controller/computeinstance_validation_test.go lines
569-585 for rejecting an absent-to-present GPU update.

🪄 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: Enterprise

Run ID: e9d825e0-813d-4827-b586-c43175e25a2b

📥 Commits

Reviewing files that changed from the base of the PR and between 8675130 and 135b6d5.

📒 Files selected for processing (8)

• fulfillment-service/internal/controllers/computeinstance/computeinstance_reconciler_function.go
• fulfillment-service/internal/controllers/computeinstance/computeinstance_reconciler_function_test.go
• osac-operator/api/v1alpha1/computeinstance_types.go
• osac-operator/api/v1alpha1/computeinstance_types_test.go
• osac-operator/api/v1alpha1/zz_generated.deepcopy.go
• osac-operator/charts/operator-crds/templates/osac.openshift.io_computeinstances.yaml
• osac-operator/config/crd/bases/osac.openshift.io_computeinstances.yaml
• osac-operator/internal/controller/computeinstance_validation_test.go

Auto-dismissed: only Prow labels gate merging

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 8:32 AM UTC · Completed 8:49 AM UTC

Commit: 135b6d5 · View workflow run →

[fullsend-ai-review]

Looks good to me

Previous run

Review

Findings

Medium

• [consumer completeness] fulfillment-service/internal/controllers/computeinstance/computeinstance_reconciler_function.go:708 — The new spec.gpu field is stamped onto the ComputeInstance CR but the Ansible playbook (playbook_osac_create_compute_instance.yml) does not extract spec.gpu to construct the gpu_devices list that the ocp_virt_vm role expects (checked ocp_virt_vm/tasks/create.yaml line 74: gpu_devices | default([]) | length > 0). VMs provisioned with a GPU-enabled InstanceType will have GPU data on the CR but it will not reach KubeVirt. The PR description indicates this is intentionally deferred to a follow-up — confirm with the team that the Ansible wiring is tracked separately.

• [validation marker placement] osac-operator/api/v1alpha1/computeinstance_types.go:90 — The GPU immutability rule is placed as a spec-level XValidation marker on ComputeInstanceSpec using has() guards, while every other immutable field (cores, memoryGiB, image, bootDisk, additionalDisks, sshKey, userDataSecretRef) uses a field-level self == oldSelf marker. Notably, userDataSecretRef is also an optional pointer type (*corev1.LocalObjectReference) and successfully uses the field-level pattern. Consider moving the GPU immutability to a field-level marker for consistency:

  // +kubebuilder:validation:XValidation:rule="self == oldSelf",message="gpu is immutable"
  Gpu *GpuSpec `json:"gpu,omitempty"`

• [scope-overlap-with-dependency] osac-operator/api/v1alpha1/computeinstance_types.go — This PR declares a dependency on PR OSAC-3162: add GpuSpec to ComputeInstance CRD #217 (OSAC-3162) but re-introduces the exact same osac-operator changes: GpuSpec type definition, Gpu field, immutability marker, types test, deepcopy, CRD YAML, and validation tests. Both PRs modify identical files with identical diffs. Consider rebasing onto OSAC-3162: add GpuSpec to ComputeInstance CRD #217's branch so the CRD type changes come from OSAC-3162: add GpuSpec to ComputeInstance CRD #217 and this PR only adds the reconciler stamping code + validation tests.

Low

• [edge case] fulfillment-service/internal/controllers/computeinstance/computeinstance_reconciler_function.go:708 — If the protobuf GpuSpec message is present but has zero/default values (empty strings, count 0), the nil check passes and invalid data is stamped onto the CR. CRD validation (minLength=1, minimum=1) will reject it at the API server, but the error surfaces as a Kubernetes admission failure rather than a clear reconciler error.

• [incomplete-doc] fulfillment-service/docs/CATALOG_ITEMS.md:225 — The InstanceType example shows only cores and memory_gib but does not include the gpu field. Consider adding a GPU example for discoverability.

• [incomplete-doc] osac-operator/config/samples/osac_v1alpha1_computeinstance.yaml:20 — The sample ComputeInstance CR does not include a commented-out gpu section. Other optional fields like networkAttachments include detailed comments explaining immutability rules.

Previous run (2)

Review

Findings

Medium

• [unmerged-dependency] osac-operator/api/v1alpha1/computeinstance_types.go — This PR depends on PR OSAC-3162: add GpuSpec to ComputeInstance CRD #217 (OSAC-3162 — GpuSpec CRD type), which is still open. The GpuSpec type definition and CRD YAML changes appear in both PRs. The do-not-merge/hold label correctly prevents premature merge, but consider rebasing onto OSAC-3162: add GpuSpec to ComputeInstance CRD #217's branch to avoid duplication drift if OSAC-3162: add GpuSpec to ComputeInstance CRD #217's type definition changes during review.

Low

• [naming-convention] osac-operator/api/v1alpha1/computeinstance_types.go:64 — GpuSpec/Gpu use mixed-case for the GPU acronym. The codebase capitalizes some acronyms (IPAddress, SSHKey, TemplateID), but the generated protobuf code already uses GpuSpec/Gpu (from gpu_spec/gpu proto fields). Renaming only the CRD side to GPUSpec while proto stays GpuSpec would create a cross-layer inconsistency. This is a debatable style preference.

• [naming-convention] osac-operator/api/v1alpha1/computeinstance_types.go:67 — PciDeviceSelector uses mixed-case for the PCI acronym, mirroring the generated protobuf name GetPciDeviceSelector(). Same cross-layer consistency rationale applies.

• [edge-case] fulfillment-service/internal/controllers/computeinstance/computeinstance_reconciler_function.go:708 — The nil guard if gpu := itSpec.GetGpu(); gpu != nil passes for a proto GpuSpec present with all zero-value fields (Count=0, empty strings). CRD admission validation (Minimum=1, MinLength=1) catches this, but the error message would be a generic API validation error rather than a descriptive reconciler error. Upstream InstanceType API validation prevents this state in practice.

• [incomplete-documentation] fulfillment-service/docs/CATALOG_ITEMS.md:225 — The InstanceType example YAML shows only cores, memory_gib, and state. Consider adding a GPU-enabled example or noting the optional gpu spec.

• [incomplete-documentation] fulfillment-service/docs/CATALOG_ITEMS.md:285 — The instance_type path description reads "includes number of CPU cores, memory, etc." — consider explicitly mentioning GPU passthrough configuration.

• [doc-comment-format] osac-operator/api/v1alpha1/computeinstance_types.go:63 — GpuSpec comments use trailing periods; the file is inconsistent on this convention (both styles coexist), so this is non-actionable.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 2:46 PM UTC · Completed 3:03 PM UTC

Commit: 47d6254 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 6:20 AM UTC · Completed 6:34 AM UTC

Commit: dae45b0 · View workflow run →

[ygalblum]

/approve
/lgtm

[openshift-ci]

[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

Details

Needs approval from an approver in each of these files:

• fulfillment-service/OWNERS [ygalblum]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[Tzif-Morgen]

/unhold