Skip to content

OSAC-3162: add GpuSpec to ComputeInstance CRD #217

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
omer-vishlitzky merged 3 commits into from
Aug 12, 2026
Merged

OSAC-3162: add GpuSpec to ComputeInstance CRD #217

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
02b215f
OSAC-3162: add GpuSpec to ComputeInstance CRD
Tzif-Morgen Aug 9, 2026
f9c1c6f
OSAC-3162: add immutability rule and fix optional marker for GPU field
Tzif-Morgen Aug 12, 2026
5788783
OSAC-3162: use spec-level has() rule for GPU immutability
Tzif-Morgen Aug 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions osac-operator/api/v1alpha1/computeinstance_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,25 @@ type DiskSpec struct {
StorageTier string `json:"storageTier,omitempty"`
}

// GpuSpec defines GPU passthrough configuration resolved from the InstanceType.
type GpuSpec struct {
// PciDeviceSelector is the PCI vendor:device ID (e.g., "10DE:20B0").
// +kubebuilder:validation:Required
// +kubebuilder:validation:MinLength=1
PciDeviceSelector string `json:"pciDeviceSelector"`
Comment thread
coderabbitai[bot] marked this conversation as resolved.

// ResourceName is the Kubernetes device plugin resource (e.g., "nvidia.com/A100").
// +kubebuilder:validation:Required
// +kubebuilder:validation:MinLength=1
ResourceName string `json:"resourceName"`

// Count is the number of GPU devices of this type.
// +kubebuilder:validation:Required
// +kubebuilder:validation:Minimum=1
// +kubebuilder:validation:Maximum=16
Count int32 `json:"count"`
}

// RunStrategyType defines valid VM run strategies
// +kubebuilder:validation:Enum=Always;Halted
type RunStrategyType string
Expand Down Expand Up @@ -88,6 +107,7 @@ type NetworkAttachment struct {
}

// 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"

@fullsend-ai-review fullsend-ai-review Bot Aug 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

type ComputeInstanceSpec struct {
// TemplateID is the unique identifier of the compute instance template to use when creating this compute instance
// +kubebuilder:validation:Required
Expand Down Expand Up @@ -137,6 +157,10 @@ type ComputeInstanceSpec struct {
// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="additionalDisks is immutable"
AdditionalDisks []DiskSpec `json:"additionalDisks,omitempty"`

// Gpu defines GPU passthrough configuration resolved from the InstanceType.
// +kubebuilder:validation:Optional
Gpu *GpuSpec `json:"gpu,omitempty"`

@fullsend-ai-review fullsend-ai-review Bot Aug 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[high] missing immutability constraint

The new Gpu field is missing the +kubebuilder:validation:XValidation:rule="self == oldSelf",message="gpu is immutable" marker that every other hardware-specification field in ComputeInstanceSpec carries (image, cores, memoryGiB, bootDisk, additionalDisks, userDataSecretRef, sshKey, guestOSFamily). GPU passthrough configuration maps directly to KubeVirt VM hardware — changing it after creation would require recreating the underlying VM. Without this marker, a user can update the GPU spec on a live ComputeInstance, producing a state the controller cannot reconcile without VM destruction and re-creation.

Suggested fix: Add // +kubebuilder:validation:XValidation:rule="self == oldSelf",message="gpu is immutable" above the Gpu field, regenerate CRDs (make manifests generate && make helm-crds), and add a corresponding immutability test.

@Tzif-Morgen Tzif-Morgen Aug 12, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


// RunStrategy controls VM running state (MUTABLE)
// +kubebuilder:validation:Required
RunStrategy RunStrategyType `json:"runStrategy"`
Expand Down
24 changes: 24 additions & 0 deletions osac-operator/api/v1alpha1/computeinstance_types_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,30 @@ var _ = Describe("ComputeInstanceSpec", func() {
Expect(spec.UserDataSecretRef.Name).To(Equal("my-cloud-init"))
})

It("should support GPU spec", func() {
spec := v1alpha1.ComputeInstanceSpec{
TemplateID: "test-template",
Image: v1alpha1.ImageSpec{
SourceType: v1alpha1.ImageSourceTypeRegistry,
SourceRef: "test-image:latest",
},
Cores: 8,
MemoryGiB: 64,
BootDisk: v1alpha1.DiskSpec{SizeGiB: 100},
Gpu: &v1alpha1.GpuSpec{
PciDeviceSelector: "10DE:20B0",
ResourceName: "nvidia.com/A100",
Count: 2,
},
RunStrategy: v1alpha1.RunStrategyAlways,
}

Expect(spec.Gpu).ToNot(BeNil())
Expect(spec.Gpu.PciDeviceSelector).To(Equal("10DE:20B0"))
Expect(spec.Gpu.ResourceName).To(Equal("nvidia.com/A100"))
Expect(spec.Gpu.Count).To(Equal(int32(2)))
})

It("should support SSH key", func() {
sshKey := "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQ..."
spec := v1alpha1.ComputeInstanceSpec{
Expand Down
20 changes: 20 additions & 0 deletions osac-operator/api/v1alpha1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

29 changes: 29 additions & 0 deletions osac-operator/charts/operator-crds/templates/osac.openshift.io_computeinstances.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,31 @@ spec:
x-kubernetes-validations:
- message: cores is immutable
rule: self == oldSelf
gpu:
description: Gpu defines GPU passthrough configuration resolved from
the InstanceType.
properties:
count:
description: Count is the number of GPU devices of this type.
format: int32
maximum: 16
minimum: 1
type: integer
pciDeviceSelector:
description: PciDeviceSelector is the PCI vendor:device ID (e.g.,
"10DE:20B0").
minLength: 1
type: string
resourceName:
description: ResourceName is the Kubernetes device plugin resource
(e.g., "nvidia.com/A100").
minLength: 1
type: string
required:
- count
- pciDeviceSelector
- resourceName
type: object
guestOSFamily:
description: |-
GuestOSFamily specifies the guest operating system family for the VM.
Expand Down Expand Up @@ -281,6 +306,10 @@ spec:
- runStrategy
- templateID
type: object
x-kubernetes-validations:
- message: gpu is immutable
rule: has(self.gpu) == has(oldSelf.gpu) && (!has(self.gpu) || self.gpu
== oldSelf.gpu)
status:
description: status defines the observed state of ComputeInstance
properties:
Expand Down
29 changes: 29 additions & 0 deletions osac-operator/config/crd/bases/osac.openshift.io_computeinstances.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,31 @@ spec:
x-kubernetes-validations:
- message: cores is immutable
rule: self == oldSelf
gpu:
description: Gpu defines GPU passthrough configuration resolved from
the InstanceType.
properties:
count:
description: Count is the number of GPU devices of this type.
format: int32
maximum: 16
minimum: 1
type: integer
pciDeviceSelector:
description: PciDeviceSelector is the PCI vendor:device ID (e.g.,
"10DE:20B0").
minLength: 1
type: string
resourceName:
description: ResourceName is the Kubernetes device plugin resource
(e.g., "nvidia.com/A100").
minLength: 1
type: string
required:
- count
- pciDeviceSelector
- resourceName
type: object
guestOSFamily:
description: |-
GuestOSFamily specifies the guest operating system family for the VM.
Expand Down Expand Up @@ -279,6 +304,10 @@ spec:
- runStrategy
- templateID
type: object
x-kubernetes-validations:
- message: gpu is immutable
rule: has(self.gpu) == has(oldSelf.gpu) && (!has(self.gpu) || self.gpu
== oldSelf.gpu)
status:
description: status defines the observed state of ComputeInstance
properties:
Expand Down
114 changes: 114 additions & 0 deletions osac-operator/internal/controller/computeinstance_validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -488,6 +488,120 @@ var _ = Describe("ComputeInstance CEL Validation", func() {
})
})

@fullsend-ai-review fullsend-ai-review Bot Aug 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[medium] test gap — missing immutability test

The new GPU validation test block validates creation-time constraints (min/max count, empty strings, optional presence) but does not include an immutability-on-update test. Every other hardware field has a dedicated test that creates a valid instance, fetches it, mutates the field, and asserts the update is rejected with an "is immutable" error.

Suggested fix: Add an immutability test in the GPU validation Describe block that creates with GPU, fetches, mutates, and asserts rejection.

@Tzif-Morgen Tzif-Morgen Aug 12, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Describe("GPU validation", func() {
It("should allow creation with valid GPU spec", func() {
instance := createValidInstance("test-gpu-valid")
instance.Spec.Gpu = &osacv1alpha1.GpuSpec{
PciDeviceSelector: "10DE:20B0",
ResourceName: "nvidia.com/A100",
Count: 1,
}

Expect(k8sClient.Create(ctx, instance)).To(Succeed())
})

It("should allow creation without GPU spec", func() {
instance := createValidInstance("test-gpu-none")

Expect(k8sClient.Create(ctx, instance)).To(Succeed())
})

It("should omit gpu from JSON when not set (omitempty)", func() {
instance := createValidInstance("test-gpu-omitempty")
Expect(k8sClient.Create(ctx, instance)).To(Succeed())

Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(instance), instance)).To(Succeed())
Expect(instance.Spec.Gpu).To(BeNil())
})
Comment thread
coderabbitai[bot] marked this conversation as resolved.

It("should reject GPU with count below minimum", func() {
instance := createValidInstance("test-gpu-count-zero")
instance.Spec.Gpu = &osacv1alpha1.GpuSpec{
PciDeviceSelector: "10DE:20B0",
ResourceName: "nvidia.com/A100",
Count: 0,
}

err := k8sClient.Create(ctx, instance)
Expect(err).To(HaveOccurred())
Expect(apierrors.IsInvalid(err)).To(BeTrue())
})

It("should reject GPU with count above maximum", func() {
instance := createValidInstance("test-gpu-count-max")
instance.Spec.Gpu = &osacv1alpha1.GpuSpec{
PciDeviceSelector: "10DE:20B0",
ResourceName: "nvidia.com/A100",
Count: 17,
}

err := k8sClient.Create(ctx, instance)
Expect(err).To(HaveOccurred())
Expect(apierrors.IsInvalid(err)).To(BeTrue())
})

It("should reject GPU with empty pciDeviceSelector", func() {
instance := createValidInstance("test-gpu-empty-pci")
instance.Spec.Gpu = &osacv1alpha1.GpuSpec{
PciDeviceSelector: "",
ResourceName: "nvidia.com/A100",
Count: 1,
}

err := k8sClient.Create(ctx, instance)
Expect(err).To(HaveOccurred())
Expect(apierrors.IsInvalid(err)).To(BeTrue())
})

It("should reject GPU with empty resourceName", func() {
instance := createValidInstance("test-gpu-empty-rn")
instance.Spec.Gpu = &osacv1alpha1.GpuSpec{
PciDeviceSelector: "10DE:20B0",
ResourceName: "",
Count: 1,
}

err := k8sClient.Create(ctx, instance)
Expect(err).To(HaveOccurred())
Expect(apierrors.IsInvalid(err)).To(BeTrue())
})

It("should reject changing gpu", func() {
instance := createValidInstance("test-gpu-immutable")
instance.Spec.Gpu = &osacv1alpha1.GpuSpec{
PciDeviceSelector: "10DE:20B0",
ResourceName: "nvidia.com/A100",
Count: 1,
}
Expect(k8sClient.Create(ctx, instance)).To(Succeed())

Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(instance), instance)).To(Succeed())

instance.Spec.Gpu.Count = 2
err := k8sClient.Update(ctx, instance)
Expect(err).To(HaveOccurred())
Expect(apierrors.IsInvalid(err)).To(BeTrue())
Expect(err.Error()).To(ContainSubstring("gpu is immutable"))
})

It("should reject adding gpu after creation", func() {
instance := createValidInstance("test-gpu-add-after")
Expect(k8sClient.Create(ctx, instance)).To(Succeed())

Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(instance), instance)).To(Succeed())

instance.Spec.Gpu = &osacv1alpha1.GpuSpec{
PciDeviceSelector: "10DE:20B0",
ResourceName: "nvidia.com/A100",
Count: 1,
}
err := k8sClient.Update(ctx, instance)
Expect(err).To(HaveOccurred())
Expect(apierrors.IsInvalid(err)).To(BeTrue())
Expect(err.Error()).To(ContainSubstring("gpu is immutable"))
})
})

Describe("MaxItems validation", func() {
It("should reject creating ComputeInstance with more than 8 networkAttachments", func() {
instance := createValidInstance("test-max-attachments")

@fullsend-ai-review fullsend-ai-review Bot Aug 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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'.

Expand Down
Loading