HYPERFLEET-386 - feat: use ready/available conditions instead of phase #25
Conversation
…uster and nodepool statuses - Added mandatory conditions "Available" and "Ready" to the status object in the OpenAPI and TSP schemas. - Updated descriptions to clarify the status conditions and their implications. - Regenerated GCP schemas to reflect these changes.
WalkthroughThis pull request replaces phase-based status fields with a conditions-based model across the API: it removes the Sequence Diagram(s)Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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
🤖 Fix all issues with AI agents
In `@models/clusters/model.tsp`:
- Around line 29-39: The schema-level `@minItems` on conditions
(ResourceCondition[] in model.tsp) cannot guarantee the documented mandatory
condition types ("Available" and "Ready"); update the implementation to enforce
this at runtime by adding explicit validation where Cluster resources are
created/updated (e.g., in the Cluster creation/update handler or a
validateCluster function) that checks the conditions array contains entries with
type === "Available" and type === "Ready" (and returns a clear validation error
if missing), and update the field comment to state this is a runtime/contract
requirement rather than an OpenAPI-enforced schema constraint (or alternatively
migrate the OpenAPI target to 3.1 if you choose schema-level enforcement).
rh-amarin
changed the title
ciaranRoche
left a comment
ciaranRoche
left a comment
There was a problem hiding this comment.
Two small comments, but otherwise looks good to me
| model SearchParams { | ||
| /** Filter results using TSL (Tree Search Language) query syntax. | ||
| * Examples: `status.phase='NotReady'`, `name in ('c1','c2')`, `labels.region='us-east'` */ | ||
| * Examples: `status.conditions.Ready='True'`, `name in ('c1','c2')`, `labels.region='us-east'` */ |
There was a problem hiding this comment.
Is this correct? Considering the conditions is an array?
There was a problem hiding this comment.
The status.conditions is today a JSON blob in the DB
- The service transforms the search query into a the correct db query.
- If using more direct approach, the syntax is something like (telling by memory)
status.conditions @> [{"Type":"Ready","status":"True"}]
The current approach is on the one hand convenient, but less flexible if we want to provide full conditions in the search
IMO we have yet to design the search capabilities for our API
Here I went to explore this alternative for a future enhancement of the Sentinel where it only queries Ready=false or Ready.last_updated_time>30m
- Today, Sentinel loops over all clusters and discards in memory
models/nodepools/model.tsp
Outdated
| * - `type: "Available"`: indicates if the nodepool is accessible and functional. | ||
| * - `type: "Ready"`: indicates if the nodepool has reached its desired state. |
There was a problem hiding this comment.
Should these be constants? just for a little type safety, or is there a reason not to have this 🤔
There was a problem hiding this comment.
AFAIK you can have enums in an OpenAPI contract, but Available and Ready are only two of the unbounded possibilities...
If you mean in the Typespec files, yes we can have these as constants.
I've updated the contract to use constants, the generated openapi.yaml is the same
- Updated status conditions in various TSP models to utilize the new ConditionType enum for "Available" and "Ready". - Ensured consistency across cluster and nodepool status definitions in the models.
- Updated various TSP models to replace ConditionStatus with ResourceConditionStatus for consistency across cluster and nodepool definitions. - Ensured that all condition types now align with the new enum structure, enhancing clarity and maintainability.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@models-core/nodepool/example_nodepool.tsp`:
- Around line 23-33: In the Ready condition object (the hash with type: "Ready",
status: ResourceConditionStatus.True, reason: "ResourceReady"), remove the
duplicate message entry so only a single "message" key remains (choose the
intended string, e.g., "All conditions successful for observed_generation" or
"NodePool is ready") to avoid the second message overwriting the first; update
the object in example_nodepool.tsp where the Ready condition is defined.
In `@models/common/model.tsp`:
- Around line 203-228: The ResourceCondition model uses the enum
ResourceConditionStatus but the file models/common/model.tsp lacks the import;
open models/common/model.tsp and add an import for the statuses file that
exports ResourceConditionStatus (e.g. import the "../statuses/model.tsp" module)
alongside the other imports at the top so ResourceConditionStatus is in scope
for the ResourceCondition definition.
♻️ Duplicate comments (1)
models/clusters/model.tsp (1)
27-39: Schema limitation already addressed via runtime validation.The
@minItems(2)constraint enforces array length but cannot guarantee the presence of specific condition types (AvailableandReady) at the schema level. This limitation was previously discussed, and since oapi-codegen doesn't support OpenAPI 3.1, runtime validation is the accepted approach. The documentation clearly communicates the contract requirement.
🧹 Nitpick comments (2)
models/nodepools/model.tsp (1)
50-50: Consider: Plural field name with singular type.The field
owner_referencesuses a plural name but has a singular typeObjectReference. If only one owner is expected, consider renaming toowner_referencefor clarity. If multiple owners are possible, the type should beObjectReference[].models-core/nodepool/example_nodepool.tsp (1)
23-43: Message text inconsistency with other example files.The messages for Ready and Available appear swapped compared to
models-gcp/cluster/example_cluster.tspand the OpenAPI schemas:
- Here: Ready says "observed_generation", Available says "current spec generation"
- Elsewhere: Ready says "current spec generation", Available says "observed_generation"
Consider aligning the messages for consistency across all examples.
| /** | ||
| * Condition in Cluster/NodePool status | ||
| * Used for semantic condition types: "ValidationSuccessful", "DNSSuccessful", "NodePoolSuccessful", etc. | ||
| * Includes observed_generation and last_updated_time to track adapter-specific state | ||
| */ | ||
| model ResourceCondition { | ||
| ...ConditionBase; | ||
|
|
||
| status: ResourceConditionStatus; | ||
|
|
||
| /** | ||
| * Generation of the spec that this condition reflects | ||
| */ | ||
| observed_generation: int32; | ||
|
|
||
| /** | ||
| * When this condition was first created (API-managed) | ||
| */ | ||
| @format("date-time") created_time: string; | ||
|
|
||
| /** | ||
| * When the corresponding adapter last reported (API-managed) | ||
| * Updated every time the adapter POSTs, even if condition status hasn't changed | ||
| * Copied from AdapterStatus.last_report_time | ||
| */ | ||
| @format("date-time") last_updated_time: string; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for ResourceConditionStatus definition and references
rg -n "ResourceConditionStatus" --type-list | grep -i tsp || rg -n "ResourceConditionStatus" -A 2 -B 2Repository: openshift-hyperfleet/hyperfleet-api-spec
Length of output: 9681
🏁 Script executed:
#!/bin/bash
# Check imports in models/common/model.tsp
echo "=== Checking models/common/model.tsp ===" && head -50 models/common/model.tsp | cat -nRepository: openshift-hyperfleet/hyperfleet-api-spec
Length of output: 1520
🏁 Script executed:
#!/bin/bash
# Verify the file structure and find ResourceConditionStatus definition
echo "=== Searching for ResourceConditionStatus definition ===" && rg -n "ResourceConditionStatus"Repository: openshift-hyperfleet/hyperfleet-api-spec
Length of output: 2141
🏁 Script executed:
#!/bin/bash
# Check if models/statuses/model.tsp exists and contains ResourceConditionStatus
echo "=== Checking models/statuses/model.tsp ===" && ls -la models/statuses/ 2>/dev/null || echo "Directory not found"Repository: openshift-hyperfleet/hyperfleet-api-spec
Length of output: 372
🏁 Script executed:
#!/bin/bash
# Get the complete import section and first 100 lines of models/common/model.tsp
head -100 models/common/model.tsp | cat -nRepository: openshift-hyperfleet/hyperfleet-api-spec
Length of output: 3193
🏁 Script executed:
#!/bin/bash
# Also check if there's a main.tsp or namespace organization file
find models -name "main.tsp" -o -name "namespace.tsp" | head -20Repository: openshift-hyperfleet/hyperfleet-api-spec
Length of output: 66
🏁 Script executed:
#!/bin/bash
# Check the complete models/statuses/model.tsp file to understand the structure
cat models/statuses/model.tspRepository: openshift-hyperfleet/hyperfleet-api-spec
Length of output: 2840
🏁 Script executed:
#!/bin/bash
# Look for any TypeSpec or project configuration files
find . -maxdepth 2 -name "tspconfig.json" -o -name "package.json" -o -name "tsconfig.json" | grep -v node_modulesRepository: openshift-hyperfleet/hyperfleet-api-spec
Length of output: 98
🏁 Script executed:
#!/bin/bash
# Check if there are namespace declarations in models/common/model.tsp beyond line 100
grep -n "namespace\|using\|import" models/common/model.tsp | head -20Repository: openshift-hyperfleet/hyperfleet-api-spec
Length of output: 245
🏁 Script executed:
#!/bin/bash
# Check if there are namespace declarations in models/statuses/model.tsp
grep -n "namespace\|using\|import" models/statuses/model.tspRepository: openshift-hyperfleet/hyperfleet-api-spec
Length of output: 187
🏁 Script executed:
#!/bin/bash
# Look at the ResourceCondition model context - lines 200-228 in models/common/model.tsp
sed -n '195,235p' models/common/model.tsp | cat -nRepository: openshift-hyperfleet/hyperfleet-api-spec
Length of output: 1252
🏁 Script executed:
#!/bin/bash
# Check if there's a main.tsp or root-level organization file
find models -name "*.tsp" -maxdepth 1Repository: openshift-hyperfleet/hyperfleet-api-spec
Length of output: 66
🏁 Script executed:
#!/bin/bash
# Look for any namespace declarations or export patterns in the entire models directory
grep -r "namespace\|export" models --include="*.tsp" | head -30Repository: openshift-hyperfleet/hyperfleet-api-spec
Length of output: 303
🏁 Script executed:
#!/bin/bash
# Check package.json to understand the TypeSpec project structure
cat package.json | head -50Repository: openshift-hyperfleet/hyperfleet-api-spec
Length of output: 965
Add missing import for ResourceConditionStatus in models/common/model.tsp.
ResourceConditionStatus is used on line 211 but not imported. Add import "../statuses/model.tsp"; at the top of the file with the other imports to bring this enum into scope.
🤖 Prompt for AI Agents
In `@models/common/model.tsp` around lines 203 - 228, The ResourceCondition model
uses the enum ResourceConditionStatus but the file models/common/model.tsp lacks
the import; open models/common/model.tsp and add an import for the statuses file
that exports ResourceConditionStatus (e.g. import the "../statuses/model.tsp"
module) alongside the other imports at the top so ResourceConditionStatus is in
scope for the ResourceCondition definition.
models/clusters/model.tsp
Outdated
| * | ||
| * **Mandatory conditions**: | ||
| * - `type: "Available"`: indicates if the cluster is accessible and functional. | ||
| * - `type: "Ready"`: indicates if the cluster has reached its desired state. |
There was a problem hiding this comment.
As same as openshift-hyperfleet/hyperfleet-api#39 (comment) and what you agree openshift-hyperfleet/hyperfleet-api#39 (comment), The semantics of available here are inappropriate. It indicates that all required adapters successfully completed their reconciliation tasks at the observed_generation, but it does not fully represent that the cluster is accessible and functional.
| const ExampleAdapter2HealthReason: string = "All adapter2 operations completed successfully"; | ||
| const ExampleAdapter2HealthMessage: string = "All adapter2 runtime operations completed successfully"; | ||
| const ExampleAdapter2AvaliableReason: string = "This adapter2 is available"; | ||
| const ExampleAdapter2AvaliableMessage: string = "This adapter2 is available"; |
There was a problem hiding this comment.
I am not sure which should be better, showing in the example directly to improve the readability or centralize message/reason management. Whatever, it is acceptable.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: yasun1 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 |
openshift-merge-bot
bot
merged commit 23e7d9f
into
openshift-hyperfleet:main
3 participants
https://issues.redhat.com/browse/HYPERFLEET-386
This PR aligns the API with the changes in the "sentinel pulse" spike
Removes from the root of resources state:
phaseobserved_generationlast_transition_timelast_updated_timeThe information on these fields is superseded by the information in the
status.conditionsAvailableReadyVersion updated to
v1.0.4Summary by CodeRabbit
New Features
Chores
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.