fix(rest-api): Treat instance status consistently across reporting, filtering, and statistics#2342
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
rest-api/db/pkg/db/model/instance.go (1)
654-669:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSearch still matches the raw lifecycle
status.
Statusesnow filters oninstanceAggregatedStatusSQL(), but the free-text path below still tokenizes and matchesi.status. A ready instance withpower_status='Rebooting'will therefore count/filter asRebootingyet remain searchable asReady, which keeps status handling inconsistent in the same endpoint.🤖 Prompt for 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. In `@rest-api/db/pkg/db/model/instance.go` around lines 654 - 669, The free-text search still references the raw column i.status, causing inconsistencies with the aggregated-status filtering; update the search clause in the NormalizeSearchQuery handling (where searchQuery, searchTokens, ok are used) to use the same instanceAggregatedStatusSQL() expression instead of i.status—ensure both the to_tsvector concatenation and the WhereOr ILIKE checks replace i.status with the aggregated-status SQL (or include the aggregated expression) so full-text and ILIKE searches match the same lifecycle value used by the Statuses filter.rest-api/api/pkg/api/model/instance.go (1)
1904-1923:⚠️ Potential issue | 🟡 MinorUse aggregated instance status in
NewAPIInstanceSummary
NewAPIInstanceSummarysetsAPIInstanceSummary.Statustodbist.Status(raw) while nearby logic usescdbm.AggregatedInstanceStatus(dbinst.Status, dbinst.PowerStatus)(line ~1832). Update the summary to use the aggregated status as well (instead of the raw status) to keep list/detail reporting consistent.🤖 Prompt for 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. In `@rest-api/api/pkg/api/model/instance.go` around lines 1904 - 1923, NewAPIInstanceSummary currently assigns APIInstanceSummary.Status directly from dbist.Status; change this to compute and assign the aggregated status using cdbm.AggregatedInstanceStatus(dbist.Status, dbist.PowerStatus) so list/detail reporting matches other code paths. In function NewAPIInstanceSummary replace the Status assignment to call cdbm.AggregatedInstanceStatus with dbist.Status and dbist.PowerStatus and set ins.Status to that result (ensure the cdbm symbol is in scope if needed).
🤖 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 `@rest-api/db/pkg/db/model/instance.go`:
- Around line 209-221: Add a Terminated count to the API struct and handle the
terminated status in the counting function: extend the InstanceStatusCounts
struct with a Terminated int field and update GetCountByStatus to include an
InstanceStatusTerminated branch that increments the new Terminated field instead
of falling through to default/Unknown; ensure any callers that build or
serialize InstanceStatusCounts (and any tests) are updated to expect the new
field so terminated instances are correctly reported rather than lumped into
Unknown.
---
Outside diff comments:
In `@rest-api/api/pkg/api/model/instance.go`:
- Around line 1904-1923: NewAPIInstanceSummary currently assigns
APIInstanceSummary.Status directly from dbist.Status; change this to compute and
assign the aggregated status using cdbm.AggregatedInstanceStatus(dbist.Status,
dbist.PowerStatus) so list/detail reporting matches other code paths. In
function NewAPIInstanceSummary replace the Status assignment to call
cdbm.AggregatedInstanceStatus with dbist.Status and dbist.PowerStatus and set
ins.Status to that result (ensure the cdbm symbol is in scope if needed).
In `@rest-api/db/pkg/db/model/instance.go`:
- Around line 654-669: The free-text search still references the raw column
i.status, causing inconsistencies with the aggregated-status filtering; update
the search clause in the NormalizeSearchQuery handling (where searchQuery,
searchTokens, ok are used) to use the same instanceAggregatedStatusSQL()
expression instead of i.status—ensure both the to_tsvector concatenation and the
WhereOr ILIKE checks replace i.status with the aggregated-status SQL (or include
the aggregated expression) so full-text and ILIKE searches match the same
lifecycle value used by the Statuses filter.
🪄 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: 47d00d56-05e4-466d-acaa-5f9f03bc0089
📒 Files selected for processing (10)
rest-api/api/pkg/api/handler/tenant.gorest-api/api/pkg/api/model/instance.gorest-api/api/pkg/api/model/instance_test.gorest-api/api/pkg/api/model/tenant.gorest-api/db/pkg/db/model/instance.gorest-api/db/pkg/db/model/instance_test.gorest-api/docs/index.htmlrest-api/openapi/spec.yamlrest-api/sdk/standard/model_instance_count_by_status.gorest-api/sdk/standard/model_tenant_identity_config_create_or_update_request.go
💤 Files with no reviewable changes (2)
- rest-api/sdk/standard/model_tenant_identity_config_create_or_update_request.go
- rest-api/api/pkg/api/model/instance_test.go
Filter and count instances using aggregated lifecycle and power status so list queries and tenant stats match the status returned by the API. Signed-off-by: Leah Itagaki <litagaki@nvidia.com>
nvlitagaki
force-pushed
the
fix/instance-status-filter
branch
from
61c9fe7 to
1caa5d2
Compare
|
🌿 Preview your docs: https://nvidia-preview-pull-request-2342.docs.buildwithfern.com/infra-controller |
🔐 TruffleHog Secret Scan✅ No secrets or credentials found! Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉 🕐 Last updated: 2026-06-10 19:38:18 UTC | Commit: e026f18 |
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
| // Instance is the data structure to capture API representation of an Instance Stats associated with tenant | ||
| Instance APIInstanceStats `json:"instance"` | ||
| // Instance holds aggregated instance status counts for the tenant. | ||
| Instance cdbm.InstanceStatusCounts `json:"instance"` |
There was a problem hiding this comment.
This will break existant pattern, cdbm.InstanceStatusCounts. Instead, we can have APIInstanceStats and inside we can have cdbm.InstanceStatusCounts
nvlitagaki
force-pushed
the
fix/instance-status-filter
branch
from
dc2f56b to
2bfe41d
Compare
nvlitagaki
force-pushed
the
fix/instance-status-filter
branch
from
57a5618 to
9f349c3
Compare
|
|
||
| // AggregatedInstanceStatus returns the status reported by the API by | ||
| // combining lifecycle status with power status when the Instance is Ready. | ||
| func AggregatedInstanceStatus(status string, powerStatus *string) string { |
There was a problem hiding this comment.
This would be better as GetAggregatedStatus receiver function of Instance struct.
| // Counts reflect the same status values returned on individual Instance objects | ||
| // (lifecycle status with Ready+power overrides applied via instanceAggregatedStatusSQL). | ||
|
|
||
| type InstanceStatusCounts struct { |
There was a problem hiding this comment.
InstanceCountByStatus might be better.
| TotalCount int64 `bun:"total_count"` | ||
| } | ||
|
|
||
| func instanceAggregatedStatusSQL() string { |
There was a problem hiding this comment.
Better as instanceAggregatedStatusQuery
| Unknown int `json:"unknown"` | ||
| } | ||
|
|
||
| type instanceStatusCountRow struct { |
There was a problem hiding this comment.
Better as instanceStatusCountQueryResult
| } | ||
|
|
||
| var results InstanceStatusCounts | ||
| for _, row := range statusQueryResults { |
There was a problem hiding this comment.
This can be FromQueryResults receiver function of InstanceCountByStatus
| } | ||
|
|
||
| // APIInstanceStat holds aggregated instance status counts at the API layer. | ||
| type APIInstanceStat = cdbm.InstanceStatusCounts |
There was a problem hiding this comment.
This should be called APIInstanceStats (missing s) and I'd leave it in the Instance model.
3 participants
Description
Previously, when the status of an individual instance was reported, the value given was derived from both the status and powerStatus fields in the database. However, when the list of instances was filtered or the summary of instance counts by status was reported, only the status field was considered. I've updated logic so that the derived or 'aggregate' status is used in all places.
The summary of instances by status that we send as part of tenant statistics had gotten out of alignment with the set of statuses used for instances. To avoid future trouble, I introduced the more strongly typed
InstanceStatusCountsat the db layer and just pass it directly to the API rather than having the API specificAPIInstanceStatsthat had to be kept in sync with the logic of the Instance DAO functionGetCountByStatus. I also updated the openAPI spec to match.Type of Change
Related Issues (Optional)
Breaking Changes
Testing
Additional Notes
NVBug 5844304