OSAC-1210: Add InstanceType servers with CRUD, state lifecycle, and CLI rendering#660
Open
ygalblum wants to merge 8 commits into
Open
OSAC-1210: Add InstanceType servers with CRUD, state lifecycle, and CLI rendering#660ygalblum wants to merge 8 commits into
ygalblum wants to merge 8 commits into
Conversation
|
@ygalblum: This pull request references OSAC-1210 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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR implements the complete gRPC InstanceTypes service across public and private APIs, including CRUD operations, state-transition management, deletion guards, enum-aware filtering, integration tests, and supporting infrastructure refactoring. ChangesInstance Types Vertical Slice
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Risk Assessment: Severity: Medium-High
Mitigation: All high-risk logic is covered by comprehensive unit and integration tests, including edge cases for state transitions, immutability, and deletion guards. DAO enum translation includes error cases for unknown enum values. Possibly related PRs
Suggested labels
Suggested reviewers
🔕 Pre-merge checks override appliedThe pre-merge checks have been overridden successfully. You can now proceed with the merge. Overridden by ❌ Failed checks (2 errors, 2 warnings)
✅ Passed checks (7 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@internal/servers/instance_types_server.go`:
- Around line 195-196: Replace the brittle substring check
strings.Contains(filter, "this.spec.state") with AST-level detection: parse the
CEL expression (using the CEL parser/checker used elsewhere in the repo) and
walk the parsed AST to detect a real field selection chain this -> spec -> state
(i.e., FieldSelection/Select nodes or Ident/MemberExpr nodes whose identifiers
are "this", "spec", "state"); only treat the filter as an explicit state filter
when such a field-reference exists (not when the text appears in a string
literal or other token). Update the logic around the existing filter variable to
call the parser, traverse nodes, and return early only on a true AST
field-reference match.
- Around line 316-319: The public Update handler currently builds privateRequest
with SetObject(existingPrivateInstanceType) and SetLock(request.GetLock()) but
does not forward the client's update mask, which risks persisting unintended
fields; modify the code that constructs privateRequest (the variable named
privateRequest used with s.delegate.Update) to also forward the mask from the
incoming request by calling
privateRequest.SetUpdateMask(request.GetUpdateMask()) (or the generated setter
for the update_mask field) so the delegate.Update receives and applies the
caller's update_mask.
In `@internal/servers/private_instance_types_server.go`:
- Around line 208-214: The update currently calls s.generic.Update with the
caller's original update mask so auto-populated fields set by
handleInstanceTypeStateTransition (e.g.,
merged.GetSpec().Deprecation.*_timestamp) can be dropped; change the
request/update mask before calling s.generic.Update to ensure those
auto-populated spec fields are persisted — either expand the mask to include the
specific deprecation timestamp fields you set on merged.GetSpec() or clear the
mask to perform a full-spec write after calling
handleInstanceTypeStateTransition(existing, merged) and
request.GetObject().SetSpec(merged.GetSpec()) so s.generic.Update will persist
the timestamps.
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 34c4be44-e743-4037-a01a-bcf4d5a646cc
📒 Files selected for processing (7)
internal/cmd/service/start/grpcserver/start_grpc_server_cmd.gointernal/cmd/service/start/restgateway/start_rest_gateway_cmd.gointernal/rendering/tables/osac.private.v1.InstanceType.yamlinternal/rendering/tables/osac.public.v1.InstanceType.yamlinternal/servers/instance_type_reference_checker.gointernal/servers/instance_types_server.gointernal/servers/private_instance_types_server.go
ygalblum
force-pushed
the
feat/instancetype-server
branch
from
e12d844 to
c96cb3b
Compare
Contributor
Author
Sorry for that. The ep-to-jira skill split the tests into separate tasks and then the GSD split them into separate phases. I've combined them into the PR. See the additional PRs |
ygalblum
force-pushed
the
feat/instancetype-server
branch
2 times, most recently
from
960a652 to
0e1cbc2
Compare
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 `@internal/database/dao/filter_translator.go`:
- Around line 644-650: The function resolveEnumName currently casts the int64
value to protoreflect.EnumNumber (int32) which can truncate out-of-range values;
add an explicit bounds check on value against the int32 range (min and max of
protoreflect.EnumNumber) before casting in resolveEnumName so any int64 outside
int32 returns "", false instead of being truncated, ensuring callers
(resolveEnum and any uses of resolveEnumName) receive an "unknown enum value"
result for out-of-range integers.
In `@internal/servers/instance_type_reference_checker.go`:
- Around line 36-48: The hasReference method currently swallows all errors from
c.resourceDao.List() which allows deletions on transient failures; update
instanceTypeReferenceChecker.hasReference to inspect the returned error and only
convert the specific "field not found" / CEL unknown-field error into a (false,
nil) result, while returning any other errors unchanged (e.g., return false,
err) so callers can fail safely; locate the call using
c.resourceDao.List().SetFilter(filter).SetLimit(1).Do(ctx) and add error
classification (match on the DAO's sentinel error type or specific error
text/code from the CEL/DAO) to distinguish expected missing-field errors from
real DB/transport errors.
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: c3e92d42-9cae-4ff1-98dd-50fbf9d0c1b7
📒 Files selected for processing (11)
internal/cmd/service/start/grpcserver/start_grpc_server_cmd.gointernal/cmd/service/start/restgateway/start_rest_gateway_cmd.gointernal/database/dao/filter_translator.gointernal/rendering/tables/osac.private.v1.InstanceType.yamlinternal/rendering/tables/osac.public.v1.InstanceType.yamlinternal/servers/instance_type_reference_checker.gointernal/servers/instance_types_server.gointernal/servers/private_instance_types_server.gointernal/servers/private_instance_types_server_test.goit/it_private_instance_types_test.goit/it_public_instance_types_test.go
ygalblum
force-pushed
the
feat/instancetype-server
branch
2 times, most recently
from
2bcd987 to
f880fbd
Compare
jhernand
reviewed
Jun 11, 2026
jhernand
reviewed
Jun 11, 2026
jhernand
requested changes
Jun 11, 2026
jhernand
reviewed
Jun 11, 2026
ygalblum
force-pushed
the
feat/instancetype-server
branch
from
f880fbd to
9007dfa
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@internal/database/dao/filter_translator.go`:
- Around line 388-392: The enum comparison handling currently only succeeds when
the numeric side is a literal (checked via hasIntValue), otherwise callers
(e.g., where result, err = t.translateEnumEquals(leftTr, rightTr) and the
similar not-equals path) fall through to assembleBinarySQL producing invalid
text-vs-numeric SQL; update the enum translation functions (translateEnumEquals
and the corresponding not-equals handler used by the code paths around those
calls and the other enum-compare block later) to detect non-literal numeric
expressions and either convert/normalize them safely or return a translator
error/result indicating unsupported non-literal numeric enum comparison so
callers do not fall back to assembleBinarySQL; ensure both equals and not-equals
code paths (and the later enum handling block) are updated to return a non-empty
result.sql or an explicit error when hasIntValue is false.
In `@internal/servers/instance_type_reference_checker.go`:
- Around line 39-43: Replace the full count query with an EXISTS check to avoid
scanning all matches: change the tx.QueryRow call that currently queries `SELECT
count(*) FROM compute_instances WHERE data->'spec'->>'instance_type' = $1` (and
the int64 `count` variable) to query `SELECT EXISTS(SELECT 1 FROM
compute_instances WHERE data->'spec'->>'instance_type' = $1)` and Scan into a
bool (e.g., `exists`). Update the subsequent logic that tests `count > 0` to use
the boolean `exists` instead so the reference check remains semantically
identical but far more efficient.
In `@internal/servers/instance_types_server.go`:
- Around line 77-80: The builder method SetAttributionLogic on
InstanceTypesServerBuilder is documented as optional but Build() and the
downstream delegate creation assume attributionLogic is present, causing runtime
failures; update the builder contract by either making attributionLogic truly
optional (modify Build()/delegate construction to handle a nil/zero
auth.AttributionLogic safely) or require it explicitly (change
SetAttributionLogic to be documented/validated as required and add a Build-time
check that returns an error or panics if attributionLogic is unset). Locate
InstanceTypesServerBuilder, its SetAttributionLogic method, the Build() method
and the delegate construction that reads b.attributionLogic, and implement one
of the two fixes: add nil/zero checks and fallbacks/defaults in Build()/delegate
creation, or add validation in Build() that enforces attributionLogic is set and
returns a clear error message indicating SetAttributionLogic must be called.
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 7b0b23e3-7c77-499d-b035-c088dc2ec428
⛔ Files ignored due to path filters (2)
internal/api/osac/tests/v1/object_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/tests/v1/object_type_protoopaque.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (17)
charts/service/templates/grpc-server/authconfig.yamlinternal/cmd/service/start/grpcserver/start_grpc_server_cmd.gointernal/cmd/service/start/restgateway/start_rest_gateway_cmd.gointernal/database/dao/filter_translator.gointernal/database/dao/filter_translator_test.gointernal/database/dao/generic_dao_test.gointernal/rendering/tables/osac.private.v1.InstanceType.yamlinternal/rendering/tables/osac.public.v1.InstanceType.yamlinternal/servers/instance_type_reference_checker.gointernal/servers/instance_types_server.gointernal/servers/instance_types_server_test.gointernal/servers/private_instance_types_server.gointernal/servers/private_instance_types_server_test.goit/it_private_instance_types_test.goit/it_public_instance_types_test.gomanifests/base/grpc-server/authconfig.yamlproto/tests/osac/tests/v1/object_type.proto
💤 Files with no reviewable changes (6)
- manifests/base/grpc-server/authconfig.yaml
- proto/tests/osac/tests/v1/object_type.proto
- it/it_public_instance_types_test.go
- it/it_private_instance_types_test.go
- internal/servers/private_instance_types_server_test.go
- internal/servers/private_instance_types_server.go
jhernand
reviewed
Jun 11, 2026
| } | ||
| var count int64 | ||
| err = tx.QueryRow(ctx, | ||
| `SELECT count(*) FROM compute_instances WHERE data->'spec'->>'instance_type' = $1`, |
Contributor
There was a problem hiding this comment.
This will fail if new compute instances are added that reference the instance type after this check but before what comes next.
Contributor
Author
There was a problem hiding this comment.
You are correct and it seems like a repeating pattern. In 85be0d6 I consolidate the reference checkers, but it does not solve the issue. However, since the same issue exists in other places, I think I'd rather keep this behaviour and address this holistically.
AFAIK the best way to do so is using FKs. WDYT?
Contributor
There was a problem hiding this comment.
You can address in a follow up patch. The approach I recommend is using a materialized relationship table, as explained here:
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 `@internal/servers/reference_checker.go`:
- Around line 59-73: The sqlReferenceChecker.hasReference function currently
returns wrapped fmt.Errorf values that propagate raw DB errors to callers;
change its error handling to return a generic gRPC internal error consistent
with daoReferenceChecker (e.g., return status.Error(codes.Internal, "failed to
check resource references") or an equivalent package-specific internal error)
whenever tx retrieval or the QueryRow call fails, and log the original error
internally instead of including it in the returned message so no
database-specific details (query, hostnames, schema) are exposed to clients.
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: b8c7695d-6148-4260-af8b-cb8e4c7f3347
📒 Files selected for processing (12)
internal/servers/catalog_item_reference_checker.gointernal/servers/cluster_catalog_items_server.gointernal/servers/cluster_catalog_items_server_test.gointernal/servers/compute_instance_catalog_items_server.gointernal/servers/compute_instance_catalog_items_server_test.gointernal/servers/private_cluster_catalog_items_server.gointernal/servers/private_cluster_catalog_items_server_test.gointernal/servers/private_compute_instance_catalog_items_server.gointernal/servers/private_compute_instance_catalog_items_server_test.gointernal/servers/private_instance_types_server.gointernal/servers/reference_checker.gointernal/servers/reference_checker_mock.go
💤 Files with no reviewable changes (1)
- internal/servers/catalog_item_reference_checker.go
jhernand
reviewed
Jun 11, 2026
Contributor
There was a problem hiding this comment.
How is this mock generated? I don't see the go:generate ... line to generate it.
Contributor
Author
There was a problem hiding this comment.
You're right. It was written "manually" by mistake. Fixed in af39ffa
Contributor
Author
There was a problem hiding this comment.
This file is actually now removed since the consolidation was reverted
ygalblum
force-pushed
the
feat/instancetype-server
branch
from
af39ffa to
f21501c
Compare
Contributor
Author
|
@jhernand Since I moved the check to the DB, I reverted the consolidation of the reference checker. To avoid having changes and their reverting commits merged to main, I squashed and reordered the commits in the branch. So, the links might not exist any more. |
jhernand
approved these changes
Jun 12, 2026
Contributor
|
/retest |
2 similar comments
Contributor
Author
|
/retest |
Contributor
Author
|
/retest |
…state transitions, and deletion protection - PrivateInstanceTypesServer with full CRUD + Signal gRPC methods - Create validates spec.cores > 0 and spec.memory_gib > 0, sets id=name, defaults state to ACTIVE - Update enforces immutability on spec.cores, spec.memory_gib, and name via GetSpec() accessors - Bidirectional state transitions (ACTIVE/DEPRECATED/OBSOLETE) with auto-populated timestamps per D-01 through D-05 - Delete checks ComputeInstance references via instanceTypeReferenceChecker before allowing deletion (OSAC-1212) - instanceTypeReferenceChecker queries ComputeInstance DAO with this.spec.instance_type filter (Phase 4 wiring per D-06) Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Ygal Blum <ygal.blum@gmail.com>
- Private table (osac.private.v1.InstanceType.yaml): NAME, CORES, MEMORY, STATE, DESCRIPTION columns - Public table (osac.public.v1.InstanceType.yaml): same column structure with public InstanceTypeState type - All columns use spec-nested field paths (this.spec.cores, this.spec.memory_gib, this.spec.state) per D-00e - NAME column uses this.id since id == name per Phase 1 D-01 Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Ygal Blum <ygal.blum@gmail.com>
…ion and default state filter - Public server wraps private server via GenericMapper for field mapping - List defaults to ACTIVE+DEPRECATED via this.spec.state filter, OBSOLETE opt-in via explicit filter - Get returns any instance type regardless of state - Create/Update/Delete delegate through inMapper/outMapper following host_types_server pattern - CEL syntax validation on user-provided filters before composition Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Ygal Blum <ygal.blum@gmail.com>
…eway - Register public InstanceTypesServer via publicv1.RegisterInstanceTypesServer - Register private InstanceTypesServer via privatev1.RegisterInstanceTypesServer - Register public REST handler via publicv1.RegisterInstanceTypesHandler - Register private REST handler via privatev1.RegisterInstanceTypesHandler Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Ygal Blum <ygal.blum@gmail.com>
CEL types proto enums as integers, but protojson stores them as string names in PostgreSQL JSONB. Add enum-aware comparison logic to the filter translator that resolves numeric enum values to their proto string names for SQL generation. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Ygal Blum <ygal.blum@gmail.com>
- Builder validation: logger and tenancy logic mandatory checks - CRUD: create, list, get, update, delete with field mask - Label/annotation validation via DescribeTable - State transitions: all 6 bidirectional transitions with timestamp verification - Same-state update no-op and re-entry timestamp update - Immutability enforcement: cores, memory_gib, name rejection on update - Deletion protection: blocks delete when compute instances reference type - Public server: default state filter hides OBSOLETE, filter override Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Ygal Blum <ygal.blum@gmail.com>
…luster - Happy path CRUD: create, list, get, update description, delete with finalizer - State transitions: ACTIVE->DEPRECATED, DEPRECATED->OBSOLETE, reactivation with timestamp retention - Error paths: immutable cores/memory_gib rejection with InvalidArgument - Deletion protection: documents pre-Phase-4 behavior (succeeds because ComputeInstance lacks instance_type field) - OBSOLETE filtering: private API returns all states including OBSOLETE Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Ygal Blum <ygal.blum@gmail.com>
The wait timeout context was shared between WaitForNotification and processNotification, so by the time a notification arrived the context was nearly expired, causing the payload query to time out. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Ygal Blum <ygal.blum@gmail.com>
ygalblum
force-pushed
the
feat/instancetype-server
branch
from
f21501c to
269646b
Compare
jhernand
approved these changes
Jun 15, 2026
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhernand, 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 |
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
3 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
Implements the InstanceType server layer for the fulfillment-service, providing a catalog of pre-configured compute bundles (cores, memory) that users reference by name when creating VMs.
Jira
Key Design Decisions
m5.xlarge)cores,memory_gib, andnamecannot be modified after creation — changing compute specs requires creating a new instance typedeprecated_at,obsoleted_at)Test plan
ginkgo run -r internal/servers— unit tests for private and public server logicbuf lint— proto lint passesgo build ./...— clean buildginkgo run it— integration tests pass with new servers registeredSummary by CodeRabbit
Release Notes
New Features
Tests