HYPERFLEET-1017 - refactor: rename aggregated Available to LastKnownReconciled#127
Open
rafabene wants to merge 1 commit intoopenshift-hyperfleet:mainfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
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 replaces the aggregated status condition "Available" with "LastKnownReconciled" across API schemas, docs, tests, and implementation. It adds api.ConditionTypeLastKnownReconciled, rewrites aggregation to compute LastKnownReconciled (observed generation, last_updated_time, last_transition_time), introduces helpers for per-adapter/missing/deleted semantics, and includes migration logic for legacy Available records. OpenAPI examples and operator/user docs were updated; tests and aggregation return signatures were adjusted to emit LastKnownReconciled. Sequence Diagram(s)sequenceDiagram
participant Client
participant AggregationService as Aggregation Service
participant AdapterStatuses as Adapter Status Sources
participant LegacyCompat as Legacy Compatibility
participant StatusComputation as Status Computation
Client->>AggregationService: Request aggregated status
AggregationService->>AdapterStatuses: Collect adapter conditions
AdapterStatuses-->>AggregationService: Return adapter conditions
AggregationService->>LegacyCompat: Parse previous conditions
LegacyCompat-->>AggregationService: Provide prev-state (map Available→prevAvail)
AggregationService->>StatusComputation: computeLastKnownReconciled(adapterSnapshots, prevState)
StatusComputation->>StatusComputation: Evaluate per-adapter signals, generations
StatusComputation->>StatusComputation: Compute observed_generation, last_updated_time, last_transition_time
StatusComputation-->>AggregationService: Return LastKnownReconciled condition
AggregationService->>Client: Respond with aggregated conditions including LastKnownReconciled
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
openapi/openapi.yaml (1)
1195-1204:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpdate the schema cardinality to match the new mandatory condition set.
These descriptions now say
Ready,Reconciled, andLastKnownReconciledare all mandatory, but both schemas still allowminItems: 2. That leaves the public contract inconsistent and lets generated validators accept payloads that this PR says are invalid.Suggested fix
ClusterStatus: type: object required: - conditions properties: conditions: type: array items: $ref: '#/components/schemas/ResourceCondition' - minItems: 2 + minItems: 3NodePoolStatus: type: object required: - conditions properties: conditions: type: array items: $ref: '#/components/schemas/ResourceCondition' - minItems: 2 + minItems: 3Also applies to: 1557-1570
🤖 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 `@openapi/openapi.yaml` around lines 1195 - 1204, The schema's conditions array currently uses "minItems: 2" but the description lists three mandatory condition types ("Ready", "Reconciled", "LastKnownReconciled"); update the schema(s) that define the cluster/status conditions array (the entries containing "minItems: 2" and the description block listing those three types) to use "minItems: 3" so the OpenAPI contract enforces all three required conditions; apply the same change to the second matching schema mentioned in the comment (the other block that currently has minItems: 2 at the later location).pkg/services/aggregation.go (1)
477-514:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDifferent false states are all reported as
AdaptersNotAtSameGeneration.
computeLastKnownReconciledStatusreturnsFalsefor missing reports and explicit adapter failures as well, but this function always emits the same false reason/message. That makes the new canonical condition misleading for dashboards and operators during the rollout, because “adapter failed” and “adapter hasn’t reported yet” are indistinguishable from a mixed-generation case.Please branch the false
reason/messageby root cause before building the condition.🤖 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 `@pkg/services/aggregation.go` around lines 477 - 514, computeLastKnownReconciled currently emits the same false reason/message; change it to pick the false reason/message based on the root cause by inspecting sameGenerationAllTrue(required, byAdapter), the individual adapter states in byAdapter and whether any required adapter reports Available=False or is missing a report: if status==api.ConditionTrue keep the existing "All required..." reason/message; if any required adapter explicitly reports Available=False set reason like "AdapterReportedNotAvailable" with a message naming that root cause; else if required adapters are missing reports set reason like "AdaptersMissingReports" with an explanatory message; otherwise (mixed generations) keep "AdaptersNotAtSameGeneration". Use the existing helper calls sameGenerationAllTrue, computeLastKnownReconciledStatus and the byAdapter map to determine which branch to use and assign reason/message before returning the api.ResourceCondition.
🤖 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.
Outside diff comments:
In `@openapi/openapi.yaml`:
- Around line 1195-1204: The schema's conditions array currently uses "minItems:
2" but the description lists three mandatory condition types ("Ready",
"Reconciled", "LastKnownReconciled"); update the schema(s) that define the
cluster/status conditions array (the entries containing "minItems: 2" and the
description block listing those three types) to use "minItems: 3" so the OpenAPI
contract enforces all three required conditions; apply the same change to the
second matching schema mentioned in the comment (the other block that currently
has minItems: 2 at the later location).
In `@pkg/services/aggregation.go`:
- Around line 477-514: computeLastKnownReconciled currently emits the same false
reason/message; change it to pick the false reason/message based on the root
cause by inspecting sameGenerationAllTrue(required, byAdapter), the individual
adapter states in byAdapter and whether any required adapter reports
Available=False or is missing a report: if status==api.ConditionTrue keep the
existing "All required..." reason/message; if any required adapter explicitly
reports Available=False set reason like "AdapterReportedNotAvailable" with a
message naming that root cause; else if required adapters are missing reports
set reason like "AdaptersMissingReports" with an explanatory message; otherwise
(mixed generations) keep "AdaptersNotAtSameGeneration". Use the existing helper
calls sameGenerationAllTrue, computeLastKnownReconciledStatus and the byAdapter
map to determine which branch to use and assign reason/message before returning
the api.ResourceCondition.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 4f7f6aee-c198-432b-a2e6-76055dfae55c
📒 Files selected for processing (6)
openapi/openapi.yamlpkg/api/status_types.gopkg/services/aggregation.gopkg/services/aggregation_test.gopkg/services/cluster_test.gopkg/services/node_pool_test.go
rafabene
force-pushed
the
rbenevid/HYPERFLEET-1017/rename-available-to-last-known-reconciled
branch
from
7c6c332 to
da7f311
Compare
2 tasks
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 (1)
docs/api-operator-guide.md (1)
147-167:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInclude
Reconciledin this response example.The implementation still persists three top-level synthetic conditions —
Ready,Reconciled, andLastKnownReconciled— so this example now under-documents the actual response shape. That’s easy for operators to copy into broken jq filters or dashboards.🤖 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 `@docs/api-operator-guide.md` around lines 147 - 167, Update the example response under "status" to reflect the actual persisted synthetic conditions by adding a third condition object for "Reconciled" (matching the existing "type", "status", "observed_generation", and "last_transition_time" fields used for "Ready" and "LastKnownReconciled"); ensure the conditions array contains objects with "type": "Reconciled", a "status" value (e.g., "True"/"False"), "observed_generation": 1 and a realistic "last_transition_time" timestamp so the example matches the real API shape that includes Ready, Reconciled, and LastKnownReconciled.
🧹 Nitpick comments (2)
pkg/services/aggregation.go (1)
110-138: ⚡ Quick winRename the aggregated
availablevalue tolastKnownReconciled.This function is now the semantic boundary between adapter
Availableand aggregatedLastKnownReconciled. Keeping the old identifier here makes those two concepts easy to confuse again in callers and follow-up changes.Suggested rename
func AggregateResourceStatus(ctx context.Context, in AggregateResourceStatusInput) ( - reconciled, available api.ResourceCondition, adapterConditions []api.ResourceCondition, + reconciled, lastKnownReconciled api.ResourceCondition, adapterConditions []api.ResourceCondition, ) { prevReconciled, prevAvail, prevAdapterByType := parsePrevConditions(ctx, in.PrevConditionsJSON) reports := normalizeAdapterReportsForAggregation(ctx, in.AdapterStatuses, in.RequiredAdapters, in.ResourceGeneration) reconciled = computeReconciled( in.ResourceGeneration, in.RefTime, in.DeletedTime, prevReconciled, in.RequiredAdapters, reports, in.HasChildResources, ) - available = computeLastKnownReconciled( + lastKnownReconciled = computeLastKnownReconciled( in.RefTime, prevAvail, in.RequiredAdapters, reports, ) adapterConditions = computeAdapterConditions(in.RequiredAdapters, reports, prevAdapterByType, in.RefTime) - return reconciled, available, adapterConditions + return reconciled, lastKnownReconciled, adapterConditions }🤖 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 `@pkg/services/aggregation.go` around lines 110 - 138, The aggregated variable named available in AggregateResourceStatus should be renamed to lastKnownReconciled to reflect that this function produces the aggregated LastKnownReconciled (distinct from adapter Available); update the local variable declaration and assignment (currently assigned from computeLastKnownReconciled) and the function's returned identifier list so the third return remains adapterConditions and the second return is lastKnownReconciled, and update any internal references and the function comment to use lastKnownReconciled instead of available (references: AggregateResourceStatus, computeLastKnownReconciled, adapterConditions).pkg/api/status_types.go (1)
31-37: Add an explicit consumer migration note.Resource-level
status.conditions.Availableis being replaced here, while adapter-levelconditions[type=Available]stays unchanged. A release note or upgrade note calling out that distinction would reduce Sentinel/dashboard regressions during rollout.🤖 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 `@pkg/api/status_types.go` around lines 31 - 37, Add an explicit upgrade/migration note that resource-level status.conditions.Available (referenced by ConditionTypeAvailable in pkg/api/status_types.go) is being replaced while adapter-level conditions[type=Available] remain unchanged; update the release notes/upgrade docs to call out this distinction, mention any expected dashboards/Sentinel impacts and recommended actions (e.g., remap watchers or dashboards to the new resource-level condition name or keep using adapter-level condition where appropriate), and include an example mapping to avoid regressions during rollout.
🤖 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 `@openapi/openapi.yaml`:
- Line 53: Docs state three mandatory status conditions ("LastKnownReconciled",
"Ready", "Reconciled") but the schemas for ClusterStatus.conditions and
NodePoolStatus.conditions still use minItems: 2; update both schema definitions
to minItems: 3 and update their descriptions/examples to explicitly list the
three mandatory condition names so generated clients/validators enforce the
3-item minimum (locate the ClusterStatus.conditions and
NodePoolStatus.conditions entries in openapi.yaml and change minItems: 2 ->
minItems: 3 and adjust the human-readable description accordingly).
---
Outside diff comments:
In `@docs/api-operator-guide.md`:
- Around line 147-167: Update the example response under "status" to reflect the
actual persisted synthetic conditions by adding a third condition object for
"Reconciled" (matching the existing "type", "status", "observed_generation", and
"last_transition_time" fields used for "Ready" and "LastKnownReconciled");
ensure the conditions array contains objects with "type": "Reconciled", a
"status" value (e.g., "True"/"False"), "observed_generation": 1 and a realistic
"last_transition_time" timestamp so the example matches the real API shape that
includes Ready, Reconciled, and LastKnownReconciled.
---
Nitpick comments:
In `@pkg/api/status_types.go`:
- Around line 31-37: Add an explicit upgrade/migration note that resource-level
status.conditions.Available (referenced by ConditionTypeAvailable in
pkg/api/status_types.go) is being replaced while adapter-level
conditions[type=Available] remain unchanged; update the release notes/upgrade
docs to call out this distinction, mention any expected dashboards/Sentinel
impacts and recommended actions (e.g., remap watchers or dashboards to the new
resource-level condition name or keep using adapter-level condition where
appropriate), and include an example mapping to avoid regressions during
rollout.
In `@pkg/services/aggregation.go`:
- Around line 110-138: The aggregated variable named available in
AggregateResourceStatus should be renamed to lastKnownReconciled to reflect that
this function produces the aggregated LastKnownReconciled (distinct from adapter
Available); update the local variable declaration and assignment (currently
assigned from computeLastKnownReconciled) and the function's returned identifier
list so the third return remains adapterConditions and the second return is
lastKnownReconciled, and update any internal references and the function comment
to use lastKnownReconciled instead of available (references:
AggregateResourceStatus, computeLastKnownReconciled, adapterConditions).
🪄 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 2baa6cf6-a79c-4b58-b84c-61a89fb7ef82
📒 Files selected for processing (9)
docs/api-operator-guide.mddocs/api-resources.mddocs/search.mdopenapi/openapi.yamlpkg/api/status_types.gopkg/services/aggregation.gopkg/services/aggregation_test.gopkg/services/cluster_test.gopkg/services/node_pool_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/api-resources.md
rafabene
force-pushed
the
rbenevid/HYPERFLEET-1017/rename-available-to-last-known-reconciled
branch
from
da7f311 to
964de33
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/services/cluster.go (1)
225-240: ⚡ Quick winUpdate stale method comment that still references
Available.The implementation now aggregates
lastKnownReconciled(Line 225 / Line 240), but the method comment above still says “Reconciled, Available.” Please align that comment to prevent future confusion between adapter-levelAvailableand aggregatedLastKnownReconciled.As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 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 `@pkg/services/cluster.go` around lines 225 - 240, The comment above AggregateResourceStatus is stale: it still references "Reconciled, Available" while the implementation now aggregates lastKnownReconciled and uses ready := reconciled / lastKnownReconciled; update the method comment to reflect the current outputs (e.g., "Reconciled, LastKnownReconciled" or similar), and clarify that adapter-level Available remains distinct from the aggregated lastKnownReconciled produced by AggregateResourceStatus and returned in the adapterConditions/lastKnownReconciled values.
🤖 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 `@docs/api-operator-guide.md`:
- Line 332: Update the docs line for LastKnownReconciled to match runtime logic:
clarify that LastKnownReconciled=True requires adapters to report Available=True
for a common reconciled generation or that the controller's "sticky-true"
conditions have already been met; mixed-generation Available=True reports alone
do not necessarily set LastKnownReconciled=True unless the sticky-true path in
the aggregation logic (see the aggregation/availability handling in
pkg/services/aggregation.go, e.g., the sticky-true / aggregate status
evaluation) is satisfied.
In `@docs/api-resources.md`:
- Around line 56-57: Update all status examples and the status-fields note to
include the required synthesized condition "Reconciled" alongside "Ready" and
"LastKnownReconciled"; search for the examples that show only
"LastKnownReconciled" and "Ready" (occurrences near the examples around the
swapped snippets corresponding to the symbols Ready, Reconciled,
LastKnownReconciled) and add the "Reconciled" condition with appropriate "type"
and "status" entries so every example reflects the API contract's three
mandatory conditions.
---
Nitpick comments:
In `@pkg/services/cluster.go`:
- Around line 225-240: The comment above AggregateResourceStatus is stale: it
still references "Reconciled, Available" while the implementation now aggregates
lastKnownReconciled and uses ready := reconciled / lastKnownReconciled; update
the method comment to reflect the current outputs (e.g., "Reconciled,
LastKnownReconciled" or similar), and clarify that adapter-level Available
remains distinct from the aggregated lastKnownReconciled produced by
AggregateResourceStatus and returned in the
adapterConditions/lastKnownReconciled values.
🪄 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 25db7dd3-db39-4c52-8f37-d736014035e4
📒 Files selected for processing (11)
docs/api-operator-guide.mddocs/api-resources.mddocs/search.mdopenapi/openapi.yamlpkg/api/status_types.gopkg/services/aggregation.gopkg/services/aggregation_test.gopkg/services/cluster.gopkg/services/cluster_test.gopkg/services/node_pool_test.gopkg/services/status_helpers.go
rafabene
force-pushed
the
rbenevid/HYPERFLEET-1017/rename-available-to-last-known-reconciled
branch
from
964de33 to
8445ee0
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/services/cluster.go (1)
191-192: 💤 Low valueStale comment references old condition name.
The comment says "Reconciled, Available, and per-adapter conditions" but should reference
LastKnownReconciledto match the implementation.📝 Suggested comment update
-// UpdateClusterStatusFromAdapters recomputes aggregated Reconciled, Available, and per-adapter conditions +// UpdateClusterStatusFromAdapters recomputes aggregated Reconciled, LastKnownReconciled, and per-adapter conditions // from stored adapter rows and persists them to the cluster's status.🤖 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 `@pkg/services/cluster.go` around lines 191 - 192, Update the stale comment above the UpdateClusterStatusFromAdapters function: replace the mention of "Reconciled" with the current condition name "LastKnownReconciled" so the comment reads that it recomputes aggregated LastKnownReconciled, Available, and per-adapter conditions from stored adapter rows and persists them to the cluster's status; ensure the comment text matches the implemented condition name exactly.
🤖 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.
Nitpick comments:
In `@pkg/services/cluster.go`:
- Around line 191-192: Update the stale comment above the
UpdateClusterStatusFromAdapters function: replace the mention of "Reconciled"
with the current condition name "LastKnownReconciled" so the comment reads that
it recomputes aggregated LastKnownReconciled, Available, and per-adapter
conditions from stored adapter rows and persists them to the cluster's status;
ensure the comment text matches the implemented condition name exactly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 989a98bf-34b5-4c8d-8b81-aee2e44b2f33
📒 Files selected for processing (11)
docs/api-operator-guide.mddocs/api-resources.mddocs/search.mdopenapi/openapi.yamlpkg/api/status_types.gopkg/services/aggregation.gopkg/services/aggregation_test.gopkg/services/cluster.gopkg/services/cluster_test.gopkg/services/node_pool_test.gopkg/services/status_helpers.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/services/cluster_test.go
rafabene
force-pushed
the
rbenevid/HYPERFLEET-1017/rename-available-to-last-known-reconciled
branch
from
8445ee0 to
d29226e
Compare
…LastKnownReconciled Rename the API-computed aggregated condition from Available to LastKnownReconciled. The adapter-level Available condition remains unchanged. Includes backward compatibility for legacy DB records, differentiated reason/message for false conditions, minItems: 3 enforcement in OpenAPI schemas, and updated documentation.
rafabene
force-pushed
the
rbenevid/HYPERFLEET-1017/rename-available-to-last-known-reconciled
branch
from
d29226e to
72a1271
Compare
rh-amarin
reviewed
May 6, 2026
Comment on lines
+172
to
+176
| // Migration: legacy records stored this as "Available" | ||
| if prevAvail == nil { | ||
| prevAvail = &c | ||
| } | ||
| case api.ConditionTypeLastKnownReconciled: |
Contributor
There was a problem hiding this comment.
I don't think we need migration
rh-amarin
reviewed
May 6, 2026
| } | ||
|
|
||
| // mkPrevAvail builds an Available ResourceCondition for use as a prev fixture. | ||
| // mkPrevAvail builds a previous LastKnownReconciled (formerly Available) ResourceCondition for backward-compat testing. |
Contributor
There was a problem hiding this comment.
Do we need backward-compatibility or precedence?
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
2 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
AvailabletoLastKnownReconciledin cluster and node pool status responsesparsePrevConditions()so existing DB records with the oldAvailabletype are still recognizedcomputeAvailable→computeLastKnownReconciled, etc.) to match the new condition nameLastKnownReconciledLastKnownReconciledtakes precedence over legacyAvailableregardless of array orderAvailablecondition (reported by adapters) is NOT renamedTest plan
make verify-allpasses (823 tests, 0 lint issues)make test-integrationpassesSummary by CodeRabbit
Schema & API Updates
Improvements
Documentation