OSAC-3629: add storage_tier to proto and CRD definitions - #159
Conversation
|
@clobrano: This pull request references OSAC-3629 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: Pro Plus Run ID: ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (11)
🚧 Files skipped from review as they are similar to previous changes (9)
WalkthroughCompute instance disks now support an optional ChangesStorage tier support
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant FulfillmentAPI
participant ComputeInstanceReconciler
participant KubernetesComputeInstance
FulfillmentAPI->>ComputeInstanceReconciler: provide boot and additional disk storage tiers
ComputeInstanceReconciler->>KubernetesComputeInstance: set DiskSpec.StorageTier values
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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 |
a56d608 to
bf8ec8c
Compare
bf8ec8c to
93b92c1
Compare
93b92c1 to
61697e1
Compare
61697e1 to
0a39cd5
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@osac-operator/api/v1alpha1/computeinstance_types.go`:
- Around line 56-60: Update addExplicitFields to propagate GetStorageTier() when
mapping both the boot disk and additional disks, alongside the existing SizeGiB
mapping, so the requested tier reaches the ComputeInstance CR. Add conversion
tests covering StorageTier for both disk paths.
🪄 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: 82a88b7e-133b-45f2-8208-db6b3da8ea34
⛔ Files ignored due to path filters (4)
fulfillment-service/internal/api/osac/private/v1/compute_instance_type.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/private/v1/compute_instance_type_protoopaque.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/public/v1/compute_instance_type.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/public/v1/compute_instance_type_protoopaque.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (10)
fulfillment-service/proto/private/osac/private/v1/compute_instance_type.protofulfillment-service/proto/public/osac/public/v1/compute_instance_type.protoosac-operator/api/v1alpha1/computeinstance_types.goosac-operator/api/v1alpha1/computeinstance_types_test.goosac-operator/charts/operator-crds/templates/osac.openshift.io_computeinstances.yamlosac-operator/config/crd/bases/osac.openshift.io_computeinstances.yamlosac-operator/config/samples/osac_v1alpha1_computeinstance.yamlosac-operator/internal/controller/computeinstance_validation_test.goosac-operator/internal/controller/suite_test.goosac-operator/test/integration/console_proxy_test.go
0a39cd5 to
6a8d057
Compare
6a8d057 to
461d400
Compare
|
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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: clobrano, 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 |
Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Your Name <you@example.com>
Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Your Name <you@example.com>
Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Your Name <you@example.com>
4f4a18c to
c749b29
Compare
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
|
/lgtm |
|
🤖 Finished Review · ✅ Success · Started 2:31 PM UTC · Completed 2:50 PM UTC Commit: |
ReviewFindingsMedium
Low
Labels: PR adds storage_tier field to ComputeInstanceDisk proto and CRD, directly related to storage tier selection feature |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| SizeGiB int32 `json:"sizeGiB"` | ||
|
|
||
| // StorageTier is the name of the storage tier for this disk | ||
| // +kubebuilder:validation:MaxLength=63 |
There was a problem hiding this comment.
[medium] pattern-inconsistency
The new StorageTier field uses // +optional to mark it as optional, but every other optional spec field in this file uses // +kubebuilder:validation:Optional. While both markers work, this breaks the established convention.
Suggested fix: Replace // +optional with // +kubebuilder:validation:Optional.
| message ComputeInstanceDisk { | ||
| // Disk size in GiB. | ||
| int32 size_gib = 1; | ||
|
|
There was a problem hiding this comment.
[low] api-contract
The new storage_tier proto field has no buf.validate annotations. This is consistent with the existing size_gib field in the same message, but adding validation at the proto layer would provide earlier error feedback for invalid values. Same applies to the public proto.
Suggested fix: Add buf.validate annotations matching the CRD constraints.
| message ComputeInstanceDisk { | ||
| // Disk size in GiB. | ||
| int32 size_gib = 1; | ||
|
|
There was a problem hiding this comment.
[low] breaking-schema
The osac-operator and osac-metering pin to BSR versions that predate this change. A new BSR version should be published and downstream pins bumped so the field is usable end-to-end. Wire compatibility is preserved in the interim.
Suggested fix: Publish a new BSR version and bump downstream pins.
| spec.BootDisk = osacv1alpha1.DiskSpec{ | ||
| SizeGiB: ciSpec.GetBootDisk().GetSizeGib(), | ||
| SizeGiB: ciSpec.GetBootDisk().GetSizeGib(), | ||
| StorageTier: ciSpec.GetBootDisk().GetStorageTier(), |
There was a problem hiding this comment.
[low] api-contract
The proto optional string presence semantics (HasStorageTier()) are lost when mapped to the CRD's plain string with omitempty. This works correctly in practice.
fullsend-ai-review
Bot
added
requires-manual-review
3e9b25c
4 participants
OSAC-3629: add storage_tier to proto and CRD definitions
Jira: OSAC-3629
Story type: [DEV]
Epic: OSAC-3627 — Per-Disk Storage Tier Selection
Feature: OSAC-1710 — ComputeInstance StorageTier Selection
Summary
Adds
storage_tierfield toComputeInstanceDiskin both private and public proto definitions, andStorageTiertoDiskSpecin the CRD type. This enables tenants to specify a storage tier per disk when creating ComputeInstances. The CRD enforces validation (required, pattern, max length) and immutability (inherited from existingself == oldSelfXValidation rules).Changes
fulfillment-service (proto):
optional string storage_tier = 2toComputeInstanceDiskin both private and public protosbuf generateosac-operator (CRD):
StorageTier stringtoDiskSpecwith kubebuilder markers: Required, MinLength=1, MaxLength=63, Pattern=^[a-z0-9]([a-z0-9._-]*[a-z0-9])?$storageTier: standardTests:
DiskSpectest fixtures (3 files, 17 literals) to includeStorageTier: "standard"Testing
Acceptance Criteria
storage_tierstring on each disk and the value is persisted and returnedstorageTiervalue that is empty, exceeds 63 characters, or does not match the patternstorageTieron an existing ComputeInstance is rejected by CRD validationSummary by CodeRabbit
New Features
Bug Fixes