Skip to content

fix: require instance type machine capabilities#2949

Open
osu wants to merge 4 commits into
NVIDIA:mainfrom
osu:issue-2350-machine-capabilities
Open

fix: require instance type machine capabilities#2949
osu wants to merge 4 commits into
NVIDIA:mainfrom
osu:issue-2350-machine-capabilities

Conversation

@osu

@osu osu commented Jun 28, 2026

Copy link
Copy Markdown
Member

Description

Require each instance type to declare at least one machineCapabilities entry. This prevents instance types with no provider-supplied capability constraints from being created or updated.

This change:

  • rejects omitted or empty desired capabilities in API Core CreateInstanceType and UpdateInstanceType requests;
  • requires a non-empty machineCapabilities list when creating an instance type through REST;
  • preserves partial REST updates that omit the field, while rejecting an explicitly empty list;
  • marks the REST/OpenAPI contract and generated standard SDK models as requiring machine capabilities where applicable;
  • adds Core and REST model/handler coverage for missing and empty capability lists; and
  • regenerates the checked-in REST protobuf bindings and synchronizes the generated Core metrics reference.

Related issues

Closes #2350

Type of Change

  • Add - New feature or capability
  • Change - Changes in existing functionality
  • Fix - Bug fixes
  • Remove - Removed features or deprecated functionality
  • Internal - Internal changes (refactoring, tests, docs, etc.)

Breaking Changes

  • This PR contains breaking changes

CreateInstanceType callers and REST POST /v2/org/{org}/nico/instance/type clients must now provide a non-empty machineCapabilities list. Direct API Core updates must also include desired capabilities; REST partial updates may omit the field but cannot provide an empty list. Generated REST SDK constructors now require capabilities where the OpenAPI contract does.

Testing

  • Unit tests added/updated
  • Integration tests added/updated
  • Manual testing performed
  • No testing required (docs, internal refactor, etc.)

Passed in GitHub CI:

  • Core test-release-container-services, including the database-backed instance-type coverage
  • REST Lint and Test / Test (api), including model and handler coverage for missing and empty capabilities
  • REST OpenAPI lint and breaking-change checks
  • REST generated-file and protobuf-generated-code checks
  • core-ci-pass and rest-ci-pass

Additional Notes

The OpenAPI breaking-change allowlist records the intentional contract changes: machineCapabilities is required and has a minimum of one item for instance-type creation, and an explicitly supplied list on update must also contain at least one item.

The REST generated protobuf snapshots on main were already stale for the Site Explorer change from #2591 and the DHCP lease status enum change from #2877. The regeneration commit includes that deterministic baseline; all protobuf-generated files were produced by cargo make --no-workspace generate-rest-core-proto, not edited by hand.

The carbide_site_explorer_last_run_status row in docs/observability/core_metrics.md is emitted by the existing Core metrics-doc generator and predates this behavior change.

Signed-off-by: Hasan Khan <hasank@nvidia.com>
@osu osu requested a review from a team as a code owner June 28, 2026 02:05
@coderabbitai

coderabbitai Bot commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

Enforces machineCapabilities as required and non-empty across the OpenAPI schema, REST validation, and gRPC instance-type handling, with matching test updates. It also adds Site Explorer last-run proto metadata, a new Forge RPC, and one observability metric entry.

Changes

machineCapabilities Required Validation

Layer / File(s) Summary
OpenAPI schema constraints
rest-api/openapi/spec.yaml, rest-api/openapi/oasdiff-breaking-changes-ignore.txt
machineCapabilities is added to required lists, minItems: 1 is applied to the array schema definitions, and the breaking-change ignore list is updated accordingly.
REST model validation rules
rest-api/api/pkg/api/model/instancetype.go, rest-api/api/pkg/api/model/instancetype_test.go
APIInstanceTypeCreateRequest.Validate() now requires MachineCapabilities; update validation is conditional on the field being present. Tests cover omitted, empty, and non-empty cases.
gRPC handler desired_capabilities helper
crates/api-core/src/handlers/instance_type.rs, crates/api-core/src/tests/instance_type.rs
A shared helper validates non-empty desired capabilities and is used by both create and update handlers; SQLx-backed tests cover valid creation and invalid missing/empty inputs.
REST handler test coverage
rest-api/api/pkg/api/handler/instancetype_test.go
Create and update handler tests now require MachineCapabilities in valid fixtures and assert http.StatusBadRequest for missing or empty payloads.

Site Explorer metadata API

Layer / File(s) Summary
Site Explorer proto messages
rest-api/flow/internal/nicoapi/nicoproto/site_explorer.proto
SiteExplorationReport gains optional last_run, and new SiteExplorerLastRunResponse and SiteExplorerLastRun messages are added.
Forge RPC definition
rest-api/flow/internal/nicoapi/nicoproto/nico.proto
GetSiteExplorerLastRun is added to the Forge service with an empty request and a SiteExplorerLastRunResponse return type.

Observability metric documentation

Layer / File(s) Summary
Core metrics table entry
docs/observability/core_metrics.md
Adds a gauge metric row for carbide_site_explorer_last_run_status.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning The observability metric and Site Explorer proto/RPC changes are unrelated to machineCapabilities validation. Move the observability and Site Explorer changes into a separate PR unless they are required by a linked issue.
Docstring Coverage ⚠️ Warning Docstring coverage is 37.50% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title concisely states the main change: requiring instance type machine capabilities.
Linked Issues check ✅ Passed [#2350] The PR enforces non-empty machineCapabilities across Core, REST, and OpenAPI, matching the request.
Description check ✅ Passed The description matches the changeset by explaining the new machineCapabilities validation and related REST/Core/OpenAPI updates.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

Signed-off-by: Hasan Khan <hasank@nvidia.com>
@github-actions

Copy link
Copy Markdown

🔐 TruffleHog Secret Scan

No secrets or credentials found!

Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉

🔗 View scan details

🕐 Last updated: 2026-06-28 02:09:26 UTC | Commit: a3eb2d0

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 `@crates/api-core/src/tests/instance_type.rs`:
- Line 158: The test setup is using an invalid value for InstanceTypeId in
create_instance_type, so the request fails during ID parsing before it reaches
the empty desired_capabilities branch. Update the seeded id in the instance_type
tests to a valid InstanceTypeId value and keep the empty capabilities payload to
exercise the intended handler path in create_instance_type and the later
valid-create assertion.
🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: ae9eda83-6728-4cc1-8216-4d6c1fb75351

📥 Commits

Reviewing files that changed from the base of the PR and between 87a5337 and a3eb2d0.

⛔ Files ignored due to path filters (2)
  • rest-api/sdk/standard/model_instance_type.go is excluded by !rest-api/sdk/standard/model_*.go
  • rest-api/sdk/standard/model_instance_type_create_request.go is excluded by !rest-api/sdk/standard/model_*.go
📒 Files selected for processing (6)
  • crates/api-core/src/handlers/instance_type.rs
  • crates/api-core/src/tests/instance_type.rs
  • rest-api/api/pkg/api/handler/instancetype_test.go
  • rest-api/api/pkg/api/model/instancetype.go
  • rest-api/api/pkg/api/model/instancetype_test.go
  • rest-api/openapi/spec.yaml

Comment thread crates/api-core/src/tests/instance_type.rs
osu added 2 commits June 27, 2026 19:10
Signed-off-by: Hasan Khan <hasank@nvidia.com>
Signed-off-by: Hasan Khan <hasank@nvidia.com>
@github-actions

github-actions Bot commented Jun 28, 2026

Copy link
Copy Markdown

🔍 Container Scan Summary

Service Total Critical High Medium Low Other
boot-artifacts-aarch64 3 0 0 3 0 0
boot-artifacts-x86_64 3 0 0 3 0 0
forge-admin-cli-x86_64 285 6 25 103 7 144
machine-validation-runner 748 32 187 272 36 221
machine_validation 748 32 187 272 36 221
machine_validation-aarch64 748 32 187 272 36 221
nvmetal-carbide 748 32 187 272 36 221
TOTAL 3283 134 773 1197 151 1028

Per-CVE detail lives in the per-service grype-* artifacts (JSON + SARIF). Severity counts only — no CVE IDs published here.

@osu

osu commented Jun 28, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 28, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat: machineCapabilities in List Instance Types should be mandatory

1 participant