OSAC-1222: remove cores/memory_gib from ComputeInstance in favor of instance_type

[ygalblum]

Remove the deprecated cores and memory_gib fields from ComputeInstance and ComputeInstanceTemplate protos, replacing them with reserved directives. Update all server logic, CLI flags, reconciler, spec defaults, and tests to use instance_type as the sole compute specification method.

Assisted-by: Claude Code noreply@anthropic.com

Summary by CodeRabbit

• New Features

  • Compute instances and templates now use instance type as the primary way to choose resources.

• Bug Fixes

  • Removed outdated CPU and memory inputs from creation and update flows.
  • Validation now consistently checks instance type availability and state.
  • Default values and required-field checks were updated to match the new instance-type-based behavior.
  • API responses and warnings no longer reference legacy CPU/memory settings.

[openshift-ci-robot]

@ygalblum: This pull request references OSAC-1222 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:

Remove the deprecated cores and memory_gib fields from ComputeInstance and ComputeInstanceTemplate protos, replacing them with reserved directives. Update all server logic, CLI flags, reconciler, spec defaults, and tests to use instance_type as the sole compute specification method.

Assisted-by: Claude Code noreply@anthropic.com

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]

Walkthrough

This PR removes legacy cores/memory_gib fields from compute instance and template protobuf schemas (reserving the field numbers/names), and eliminates all related CLI flags, mutual-exclusivity validation, and defaulting logic. instance_type becomes the sole resource selector; the reconciler now resolves cores/memory via an instance-type lookup. Tests across CLI, servers, reconciler, and integration suites are updated accordingly.

Changes

instance_type-only resource sizing migration

Layer / File(s)	Summary
Proto contracts: reserve cores/memory_gib proto/private/.../compute_instance_type.proto, proto/private/.../compute_instance_template_type.proto, proto/public/.../compute_instance_type.proto, proto/public/.../compute_instance_template_type.proto	Removes cores/memory_gib fields from ComputeInstanceSpec and ComputeInstanceTemplateSpecDefaults, adds reserved declarations, and updates instance_type docs to describe reconciler-based cores/memory resolution.
Spec defaults and required-field validation internal/utils/spec_defaults.go, internal/utils/spec_defaults_test.go	ApplySpecDefaults and ValidateRequiredSpecFields now default/require instance_type unconditionally, removing legacy cores/memory_gib defaulting and mutual-exclusivity logic; tests rewritten to assert instance_type-centric behavior.
Reconciler instance-type resolution internal/controllers/computeinstance/computeinstance_reconciler_function.go, internal/controllers/computeinstance/computeinstance_reconciler_function_test.go	addExplicitFields requires instance_type, resolves it via instanceTypesClient.Get, and sets Cores/MemoryGiB from the result; extensive test updates add mock instanceTypesClient wiring and InstanceType fixtures across buildSpec, OS-mapping, subnetRef, and hub-persistence tests, plus a new error test for missing instance_type.
Server-side validation cleanup internal/servers/compute_instances_server.go, internal/servers/private_compute_instances_server.go, internal/servers/private_compute_instance_templates_server.go, internal/servers/private_compute_instance_catalog_items_server.go, and corresponding *_test.go files	Removes gRPC-based mutual-exclusivity validation for instance_type vs. cores/memory_gib across create/template/catalog-item servers; test suites now provision default InstanceType fixtures and add a new instance_type lifecycle-state validation suite (NotFound/OBSOLETE/DEPRECATED/ACTIVE).
CLI flags and integration tests internal/cmd/cli/create/computeinstance/create_compute_instance_cmd.go, it/it_compute_subnet_test.go	Removes --cores/--memory-gib CLI flags, related struct fields, and mutual-exclusivity constraints; both spec-build paths omit Cores/MemoryGib assignment; integration test creates an InstanceType and wires Spec.InstanceType instead of direct Cores/MemoryGib.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant ComputeInstancesServer
  participant SpecDefaultsUtil
  participant Reconciler
  participant InstanceTypesClient

  Client->>ComputeInstancesServer: Create(ComputeInstance with instance_type)
  ComputeInstancesServer->>SpecDefaultsUtil: ApplySpecDefaults / ValidateRequiredSpecFields
  SpecDefaultsUtil-->>ComputeInstancesServer: instance_type required/defaulted
  ComputeInstancesServer-->>Client: stored spec (instance_type only)
  Reconciler->>Reconciler: addExplicitFields(ciSpec)
  Reconciler->>InstanceTypesClient: Get(instance_type)
  InstanceTypesClient-->>Reconciler: InstanceTypeSpec (cores, memory_gib)
  Reconciler->>Reconciler: set spec.Cores, spec.MemoryGiB

Loading

Possibly related PRs

• osac-project/fulfillment-service#414: Both PRs modify the same internal/utils/spec_defaults.go required-field/defaulting flow, this PR replacing cores/memory_gib handling with instance_type-only logic.
• osac-project/fulfillment-service#735: This PR removes the legacy cores/memory_gib support that PR #735 introduced alongside instance_type across CLI, reconciler, and server validation paths.

Suggested labels: lgtm

Suggested reviewers: adriengentil, omer-vishlitzky, SiddarthR56

🚥 Pre-merge checks | ✅ 10 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (10 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: removing cores/memory_gib in favor of instance_type.
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	Scanned all touched files for secret-like literals, credential URLs, private keys, and long base64 strings; no matches in the PR changes.
No-Weak-Crypto	✅ Passed	Scanned all touched files and diff hunks; no MD5/SHA1/DES/RC4/3DES/Blowfish/ECB, custom crypto, or constant-time compare issues appeared.
No-Injection-Vectors	✅ Passed	No new SQL/shell/eval/yaml-load sinks appear in the touched code; the only dynamic filter uses strconv.Quote on the key.
Container-Privileges	✅ Passed	No manifest files changed, and the touched files contain no privileged/hostPID/hostNetwork/hostIPC/SYS_ADMIN/allowPrivilegeEscalation settings.
No-Sensitive-Data-In-Logs	✅ Passed	No new log statements or sensitive values were added; changes are validation/defaulting updates and comment-only wording tweaks.
Ai-Attribution	✅ Passed	HEAD commit includes an Assisted-by trailer for Claude Code, and no Co-Authored-By AI attribution was found.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[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 `@internal/controllers/computeinstance/computeinstance_reconciler_function.go`:
- Around line 632-651: The addExplicitFields path in task now hard-fails when
ComputeInstanceSpec has no instance_type, which will stall reconciliation for
legacy ComputeInstances. Add a migration or one-time backfill/legacy resolution
before the empty-instanceTypeName check in addExplicitFields so older records
can be populated or mapped to a default before resolving resources.

🪄 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: f9ce3558-e819-4340-94a5-a4490b0881f0

📥 Commits

Reviewing files that changed from the base of the PR and between f5b04c5 and 6be4a68.

⛔ Files ignored due to path filters (8)

• internal/api/osac/private/v1/compute_instance_template_type.pb.go is excluded by !**/*.pb.go
• internal/api/osac/private/v1/compute_instance_template_type_protoopaque.pb.go is excluded by !**/*.pb.go
• internal/api/osac/private/v1/compute_instance_type.pb.go is excluded by !**/*.pb.go
• internal/api/osac/private/v1/compute_instance_type_protoopaque.pb.go is excluded by !**/*.pb.go
• internal/api/osac/public/v1/compute_instance_template_type.pb.go is excluded by !**/*.pb.go
• internal/api/osac/public/v1/compute_instance_template_type_protoopaque.pb.go is excluded by !**/*.pb.go
• internal/api/osac/public/v1/compute_instance_type.pb.go is excluded by !**/*.pb.go
• internal/api/osac/public/v1/compute_instance_type_protoopaque.pb.go is excluded by !**/*.pb.go

📒 Files selected for processing (18)

• internal/cmd/cli/create/computeinstance/create_compute_instance_cmd.go
• internal/controllers/computeinstance/computeinstance_reconciler_function.go
• internal/controllers/computeinstance/computeinstance_reconciler_function_test.go
• internal/servers/compute_instances_server.go
• internal/servers/compute_instances_server_test.go
• internal/servers/private_compute_instance_catalog_items_server.go
• internal/servers/private_compute_instance_catalog_items_server_test.go
• internal/servers/private_compute_instance_templates_server.go
• internal/servers/private_compute_instance_templates_server_test.go
• internal/servers/private_compute_instances_server.go
• internal/servers/private_compute_instances_server_test.go
• internal/utils/spec_defaults.go
• internal/utils/spec_defaults_test.go
• it/it_compute_subnet_test.go
• proto/private/osac/private/v1/compute_instance_template_type.proto
• proto/private/osac/private/v1/compute_instance_type.proto
• proto/public/osac/public/v1/compute_instance_template_type.proto
• proto/public/osac/public/v1/compute_instance_type.proto

💤 Files with no reviewable changes (1)

• internal/servers/private_compute_instance_templates_server_test.go

[jhernand]

Try to use spec.SetCores(...) , like you are using itSpec.GetCores(). We are currently using the hybrid API, but would like to move to the fully opaque API in the future, and this will not compile when we do that. I know this was already there.

[openshift-ci]

[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

Details

Needs approval from an approver in each of these files:

• OWNERS [jhernand,ygalblum]

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