HYPERFLEET-853 - feat: add Reconciled condition alongside Ready for backward-compatible transition#111
Conversation
…ackward-compatible transition
🚥 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.
Actionable comments posted: 2
🧹 Nitpick comments (3)
pkg/services/node_pool_test.go (1)
536-551: Add parity checks (ReconciledvsReady) in transition testThis currently verifies only existence of
Reconciled. Asserting status/observed-generation parity withReadyat each step would make regressions much easier to catch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/services/node_pool_test.go` around lines 536 - 551, The test currently collects conditions into available, ready and reconciled (loop over conds and variables available, ready, reconciled) and only asserts they are non-nil; add parity assertions to compare the Reconciled and Ready conditions—specifically assert that reconciled.Status equals ready.Status and reconciled.ObservedGeneration equals ready.ObservedGeneration (or vice versa) after the g.Expect(reconciled/available/ready).ToNot(BeNil()) checks so each transition verifies status and observed-generation parity between Reconciled and Ready.pkg/services/cluster_test.go (1)
675-690: Strengthen transition assertions forReconciledbehaviorYou validate presence of
Reconciled, but not that it mirrorsReadythrough each transition in this test. Returning/assertingreconciledalongsidereadywould better protect the Phase 1 contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/services/cluster_test.go` around lines 675 - 690, The test currently finds available, ready, and reconciled but only returns available and ready; update the assertions to verify reconciled is present and that its Status/Reason/Message (the same fields used for Ready) equals the Ready condition on every transition, and modify the function to return reconciled as well so callers can assert parity; locate the variables available, ready, reconciled and the final g.Expect checks and add equality assertions between reconciled and ready (and add reconciled to the return tuple).pkg/services/aggregation.go (1)
311-361: Consider adding a comment noting the intentional duplication.
computeReconciledmirrorscomputeReadyexactly (except forType). Per the PR objectives, this is intentional for Phase 1, with Phase 2 (HYPERFLEET-854) planned to diverge the logic for deletion lifecycle handling.A brief comment would help future maintainers understand this is not accidental duplication:
📝 Optional: Add clarifying comment
+// computeReconciled mirrors computeReady in Phase 1 (HYPERFLEET-853). +// Phase 2 (HYPERFLEET-854) will diverge: Reconciled will aggregate Available +// during normal lifecycle and Finalized during deletion lifecycle. func computeReconciled( resourceGen int32,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/services/aggregation.go` around lines 311 - 361, Add a short in-code comment at the top of the computeReconciled function stating that its implementation intentionally mirrors computeReady for Phase 1 and will diverge in a later change (reference HYPERFLEET-854) to handle deletion lifecycle; mention that the duplication is deliberate to avoid confusion for future maintainers and cite computeReady so readers can see the counterpart.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openapi/openapi.yaml`:
- Around line 1180-1187: The OpenAPI schema for the cluster's status conditions
is too strict: change the array constraint on status_conditions by lowering
minItems from 3 to 2 (or make it absent) so GET responses that currently only
include "Ready" and "Available" remain valid during phased rollout, and plan a
migration/backfill to populate the "Reconciled" condition before later
tightening to minItems: 3; update the schema element referencing minItems and
the description that lists mandatory conditions (mentioning "Ready",
"Reconciled", "Available") so they remain accurate during the phased rollout.
In `@pkg/db/migrations/202604211859_add_reconciled_index.go`:
- Around line 15-29: The current expression indexes
idx_clusters_reconciled_status and idx_node_pools_reconciled_status index the
whole JSON object returned by jsonb_path_query_first(status_conditions, ...)
which doesn’t help when queries filter on the scalar "status" field; change the
index expressions to extract and cast the 'status' scalar (e.g., using
jsonb_path_query_first(...) ->> 'status' and CAST to VARCHAR) so the index can
be used for comparisons like ->> 'status' = 'True'; update both CREATE INDEX
statements that reference jsonb_path_query_first(status_conditions, '$[*] ?
(@.type == "Reconciled")') to use the extracted/cast scalar instead.
---
Nitpick comments:
In `@pkg/services/aggregation.go`:
- Around line 311-361: Add a short in-code comment at the top of the
computeReconciled function stating that its implementation intentionally mirrors
computeReady for Phase 1 and will diverge in a later change (reference
HYPERFLEET-854) to handle deletion lifecycle; mention that the duplication is
deliberate to avoid confusion for future maintainers and cite computeReady so
readers can see the counterpart.
In `@pkg/services/cluster_test.go`:
- Around line 675-690: The test currently finds available, ready, and reconciled
but only returns available and ready; update the assertions to verify reconciled
is present and that its Status/Reason/Message (the same fields used for Ready)
equals the Ready condition on every transition, and modify the function to
return reconciled as well so callers can assert parity; locate the variables
available, ready, reconciled and the final g.Expect checks and add equality
assertions between reconciled and ready (and add reconciled to the return
tuple).
In `@pkg/services/node_pool_test.go`:
- Around line 536-551: The test currently collects conditions into available,
ready and reconciled (loop over conds and variables available, ready,
reconciled) and only asserts they are non-nil; add parity assertions to compare
the Reconciled and Ready conditions—specifically assert that reconciled.Status
equals ready.Status and reconciled.ObservedGeneration equals
ready.ObservedGeneration (or vice versa) after the
g.Expect(reconciled/available/ready).ToNot(BeNil()) checks so each transition
verifies status and observed-generation parity between Reconciled and Ready.
🪄 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: Pro Plus
Run ID: 5e88d4a7-ca45-40d6-9090-11e3cedd1b75
📒 Files selected for processing (10)
openapi/openapi.yamlpkg/api/status_types.gopkg/db/migrations/202604211859_add_reconciled_index.gopkg/db/migrations/migration_structs.gopkg/services/aggregation.gopkg/services/aggregation_test.gopkg/services/cluster.gopkg/services/cluster_test.gopkg/services/node_pool.gopkg/services/node_pool_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/services/aggregation_test.go (1)
1294-1349: Strengthen the mirror-contract assertions forReconciledvsReady.Line 1343 currently checks
LastUpdatedTime, butLastTransitionTimeis also part of the mirrored timestamp behavior and is not asserted. Add it here so mirror regressions are caught in one place.Suggested test addition
if !ready.LastUpdatedTime.Equal(reconciled.LastUpdatedTime) { t.Errorf("LastUpdatedTime mismatch: Ready=%v, Reconciled=%v", ready.LastUpdatedTime, reconciled.LastUpdatedTime) } + if !ready.LastTransitionTime.Equal(reconciled.LastTransitionTime) { + t.Errorf("LastTransitionTime mismatch: Ready=%v, Reconciled=%v", + ready.LastTransitionTime, reconciled.LastTransitionTime) + }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 the current code and only fix it if needed. In `@pkg/services/aggregation_test.go` around lines 1294 - 1349, Add an assertion to mirror LastTransitionTime between the Ready and Reconciled results in the test that calls AggregateResourceStatus (the subtest block using variables ready and reconciled); specifically, after the LastUpdatedTime equality check, compare ready.LastTransitionTime and reconciled.LastTransitionTime and fail the test if they differ so the test verifies both timestamp fields are mirrored.pkg/services/aggregation.go (1)
312-362:computeReconciledduplicatescomputeReadylogic and can drift.Line 312 onward repeats the same algorithm/timestamp/reason flow as
computeReady, which makes future updates error-prone when Phase 1 requires mirror semantics. Prefer a shared helper parameterized by condition type + previous condition.Refactor direction
-func computeReady(...) api.ResourceCondition { ... } -func computeReconciled(...) api.ResourceCondition { ... } +func computeGenerationGatedCondition( + conditionType string, + resourceGen int32, + refTime time.Time, + prev *api.ResourceCondition, + required []string, + byAdapter map[string]adapterAvailableSnapshot, +) api.ResourceCondition { ... } + +func computeReady(...) api.ResourceCondition { + return computeGenerationGatedCondition(api.ConditionTypeReady, resourceGen, refTime, prev, required, byAdapter) +} + +func computeReconciled(...) api.ResourceCondition { + return computeGenerationGatedCondition(api.ConditionTypeReconciled, resourceGen, refTime, prev, required, byAdapter) +}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 the current code and only fix it if needed. In `@pkg/services/aggregation.go` around lines 312 - 362, computeReconciled duplicates the same algorithm/timestamp/reason flow as computeReady, which risks drift; refactor by extracting the shared logic into a single helper (e.g., computeConditionCommon or computeAvailabilityCondition) that is parameterized by the condition Type (api.ConditionTypeReconciled vs api.ConditionTypeReady), the previous condition pointer, and any condition-specific defaults (reason/message builders like reasonAllAdaptersAvailable vs reasonMissingRequiredAdapters and functions like buildReadyFalseMessage), then update both computeReconciled and computeReady to call that helper with the appropriate parameters so timestamps, status computation, reason selection, and message construction are centralized and maintained in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/services/aggregation_test.go`:
- Around line 1294-1349: Add an assertion to mirror LastTransitionTime between
the Ready and Reconciled results in the test that calls AggregateResourceStatus
(the subtest block using variables ready and reconciled); specifically, after
the LastUpdatedTime equality check, compare ready.LastTransitionTime and
reconciled.LastTransitionTime and fail the test if they differ so the test
verifies both timestamp fields are mirrored.
In `@pkg/services/aggregation.go`:
- Around line 312-362: computeReconciled duplicates the same
algorithm/timestamp/reason flow as computeReady, which risks drift; refactor by
extracting the shared logic into a single helper (e.g., computeConditionCommon
or computeAvailabilityCondition) that is parameterized by the condition Type
(api.ConditionTypeReconciled vs api.ConditionTypeReady), the previous condition
pointer, and any condition-specific defaults (reason/message builders like
reasonAllAdaptersAvailable vs reasonMissingRequiredAdapters and functions like
buildReadyFalseMessage), then update both computeReconciled and computeReady to
call that helper with the appropriate parameters so timestamps, status
computation, reason selection, and message construction are centralized and
maintained in one place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e4a6f332-e5f9-441b-952a-6b73ae03ee9d
📒 Files selected for processing (2)
pkg/services/aggregation.gopkg/services/aggregation_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/services/cluster_test.go (1)
667-689: Strengthen transition test by asserting Reconciled mirrors Ready at each stepThe helper now checks
Reconciledexists, but the scenario assertions still validate onlyReady/Available. Consider returningReconciledfromgetSynth()and assertingReconciled.Status+ObservedGenerationmatchReadyon each transition to catch divergence regressions early.Also applies to: 718-782
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/services/cluster_test.go` around lines 667 - 689, getSynth currently returns only Available and Ready even though it verifies Reconciled exists; modify getSynth to return the reconciled condition as a third return value (change signature to return (api.ResourceCondition, api.ResourceCondition, api.ResourceCondition)), and update callers to capture that third value. In each transition/assertion block where Ready/Available are compared, add assertions that reconciled.Status equals ready.Status and reconciled.ObservedGeneration equals ready.ObservedGeneration to ensure Reconciled mirrors Ready; use the same stored/status parsing logic in getSynth (clusterDao.Get, json.Unmarshal of stored.StatusConditions) to locate the api.ConditionTypeReconciled entry. Ensure all call sites (including the other similar helper usage) are updated to handle the extra return value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/services/cluster_test.go`:
- Around line 667-689: getSynth currently returns only Available and Ready even
though it verifies Reconciled exists; modify getSynth to return the reconciled
condition as a third return value (change signature to return
(api.ResourceCondition, api.ResourceCondition, api.ResourceCondition)), and
update callers to capture that third value. In each transition/assertion block
where Ready/Available are compared, add assertions that reconciled.Status equals
ready.Status and reconciled.ObservedGeneration equals ready.ObservedGeneration
to ensure Reconciled mirrors Ready; use the same stored/status parsing logic in
getSynth (clusterDao.Get, json.Unmarshal of stored.StatusConditions) to locate
the api.ConditionTypeReconciled entry. Ensure all call sites (including the
other similar helper usage) are updated to handle the extra return value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d5e32d3c-edca-411e-8d43-9949a744f0fc
📒 Files selected for processing (3)
openapi/openapi.yamlpkg/services/cluster_test.gopkg/services/node_pool_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/services/node_pool_test.go
| reconciled = computeReconciled( | ||
| in.ResourceGeneration, | ||
| in.RefTime, | ||
| prevReconciled, | ||
| in.RequiredAdapters, | ||
| reports, | ||
| ) |
There was a problem hiding this comment.
I get from the PR comment that this duplication is intentional to mirror today's Available aggregated condition, and that in the future it will diverge when we include the Finalized condition from adapters when computing the Reconciled.
Let me challenge a bit this decision.
IMO the computation has to be exactly the same for Reconciled and Available, even after we add the new formula to take into account Finalized conditions.
This means, that it will be simpler/safer to just do:
reconciled = available
- And avoid the code duplication.
- You will not need to introduce the
prevReconciledin the code signature
We could even go a bit further and rename code to work with Reconciled instead of Available and do the opposite:
- rename
computeAvailable->computeReconciled available=computeReconciled
There was a problem hiding this comment.
As an additional bonus points for not duplicating the functions, is that IMO we don't need to introduce more tests, since we are not adding the code
There was a problem hiding this comment.
Since we've added the deletion lifecycle branching in this PR, i.e., when deleted_time is set, computeReconciled checks Finalized instead of Available, I think they're now fundamentally different computations and can't be collapsed into reconciled = available. Wdyt?
There was a problem hiding this comment.
Before these changes we didn't have the concept of deleted resources, now he have
This changes the meaning of Ready
- When a resource exists, being Ready means that all adapters report Available=True
- When a resource doesn't exist being ready means that all adapters report Finalized=True
And then... we are just renaming Ready to Reconciled
These are not two separate things
We are just keeping them both for now to have some compatibility with e2e and infra repos
There was a problem hiding this comment.
Computed only Reconciled and Ready is now replicating the same
| created_time: '2021-01-01T10:00:00Z' | ||
| last_updated_time: '2021-01-01T10:00:00Z' | ||
| last_transition_time: '2021-01-01T10:00:00Z' | ||
| - type: Reconciled |
There was a problem hiding this comment.
openapi.yaml changes need to update the openapi.info.version of the contract (at the top)
Moreover, the changes to the OpenAPI contract should be performed first in the https://github.com/openshift-hyperfleet/hyperfleet-api-spec repository and then the generated openapi.yaml file is copied to this repository.
There was a problem hiding this comment.
Created PR openshift-hyperfleet/hyperfleet-api-spec#34 in openapi-spec
| last_transition_time: '2021-01-01T10:00:00Z' | ||
| - type: Reconciled | ||
| status: 'True' | ||
| reason: All adapters reconciled at the current generation |
There was a problem hiding this comment.
Tip
nit — non-blocking suggestion
Category: Inconsistency
The OpenAPI examples for Reconciled show reason: All adapters reconciled at the current generation, but computeReconciled uses reasonAllAdaptersAvailable which produces "All required adapters reported Available=True at the current generation".
Either update the examples to match the actual code output, or introduce a dedicated reason constant for Reconciled (e.g. reasonAllAdaptersReconciled) — the latter would also help when Phase 2 diverges the logic.
Same mismatch on lines 184, 387, 1057 (clusters) and the nodepool schema example.
There was a problem hiding this comment.
Fixed the message here and openshift-hyperfleet/hyperfleet-api-spec#34
| if r != nil || a != nil || len(m) != 0 { | ||
| r, a, rc, m := parsePrevConditions(context.Background(), nil) | ||
| if r != nil || a != nil || rc != nil || len(m) != 0 { | ||
| t.Fatalf("expected (nil,nil,empty), got (%v,%v,%v)", r, a, m) |
There was a problem hiding this comment.
Tip
nit — non-blocking suggestion
Category: Improvement
The error messages in the "nil input", "empty input", and "invalid JSON" subtests still format 3 values (r, a, m) but the function now returns 4 (r, a, rc, m). If rc triggers the failure, its value won't appear in the output.
// Before
t.Fatalf("expected (nil,nil,empty), got (%v,%v,%v)", r, a, m)
// After
t.Fatalf("expected (nil,nil,nil,empty), got (%v,%v,%v,%v)", r, a, rc, m)Same fix needed on lines 115, 123, and 131.
| allConditions := make([]api.ResourceCondition, 0, 2+len(adapterConditions)) | ||
| allConditions = append(allConditions, ready, available) | ||
| allConditions := make([]api.ResourceCondition, 0, 3+len(adapterConditions)) | ||
| allConditions = append(allConditions, ready, available, reconciled) |
There was a problem hiding this comment.
Tip
nit — non-blocking suggestion
Category: Inconsistency
OpenAPI examples list conditions as Ready → Reconciled → Available, but the code appends them as Ready → Available → Reconciled. Not a functional issue since consumers filter by type, but aligning the order avoids confusion when comparing spec vs real responses.
// Current
allConditions = append(allConditions, ready, available, reconciled)
// Matching OpenAPI example order
allConditions = append(allConditions, ready, reconciled, available)Same in node_pool.go:205.
There was a problem hiding this comment.
Makes sense. Fixed the order in append
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/services/aggregation.go (1)
332-345: Consider adding early break for consistency and efficiency.When
deletedTime == nil, the loop continues even after settingallAtCurrent = false, unlike the deletion path which breaks early. This is functionally correct but inconsistent withcomputeReadyand wastes iterations.♻️ Suggested refactor for consistency
allAtCurrent := true for _, name := range required { s, ok := byAdapter[name] if !ok || !s.availableTrue || s.observedGeneration != resourceGen { allAtCurrent = false + if deletedTime == nil { + break + } } if deletedTime != nil { // Deletion lifecycle: check Finalized if !s.finalizedTrue { allAtCurrent = false break } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/services/aggregation.go` around lines 332 - 345, The loop over required adapters should exit early when we already know not all are current to match computeReady's behavior: inside the for loop in aggregation.go (iterating required and using byAdapter, s.availableTrue, s.observedGeneration, resourceGen, deletedTime, allAtCurrent, s.finalizedTrue) add a break after you set allAtCurrent = false for the non-deletion path (i.e., when deletedTime == nil) so the loop stops immediately rather than continuing extra iterations; keep the existing deletion-path check (which already breaks when finalized is false).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/services/aggregation.go`:
- Around line 332-345: The loop over required adapters should exit early when we
already know not all are current to match computeReady's behavior: inside the
for loop in aggregation.go (iterating required and using byAdapter,
s.availableTrue, s.observedGeneration, resourceGen, deletedTime, allAtCurrent,
s.finalizedTrue) add a break after you set allAtCurrent = false for the
non-deletion path (i.e., when deletedTime == nil) so the loop stops immediately
rather than continuing extra iterations; keep the existing deletion-path check
(which already breaks when finalized is false).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: e9a02c3a-e640-48d5-aea3-8c1b0b13f9cd
📒 Files selected for processing (5)
pkg/api/status_types.gopkg/services/aggregation.gopkg/services/aggregation_test.gopkg/services/cluster.gopkg/services/status_helpers.go
✅ Files skipped from review due to trivial changes (2)
- pkg/api/status_types.go
- pkg/services/aggregation_test.go
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
pkg/services/aggregation_test.go (1)
123-144:⚠️ Potential issue | 🟡 MinorFailure output still omits the new
rcreturn value.These subtests now validate four values, but the
t.Fatalfmessages still print only three. Ifrcis wrong, the failure output hides it.Suggested fix
- t.Fatalf("expected (nil,nil,empty), got (%v,%v,%v)", r, a, m) + t.Fatalf("expected (nil,nil,nil,empty), got (%v,%v,%v,%v)", r, a, rc, m) ... - t.Fatalf("expected (nil,nil,empty), got (%v,%v,%v)", r, a, m) + t.Fatalf("expected (nil,nil,nil,empty), got (%v,%v,%v,%v)", r, a, rc, m) ... - t.Fatalf("expected (nil,nil,empty) on bad JSON, got (%v,%v,%v)", r, a, m) + t.Fatalf("expected (nil,nil,nil,empty) on bad JSON, got (%v,%v,%v,%v)", r, a, rc, m)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/services/aggregation_test.go` around lines 123 - 144, The tests calling parsePrevConditions now return four values (r, a, rc, m) but the t.Fatalf messages only print three; update each failing t.Fatalf in the "nil input", "empty input", and "invalid JSON" subtests to include rc in the formatted output so failures show all four values (e.g., include %v for rc and add rc to the argument list) and ensure the expectation string mentions the four-tuple to match the returned values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/services/aggregation_test.go`:
- Around line 1468-1488: The test's deletion fixture incorrectly sets the
adapters to Available=True via finalizedConds, causing Ready to be true; update
the fixture for this case so the adapters reflect Available=False at the current
generation (so AggregateResourceStatus returns Ready=False while
Reconciled=True). Concretely, modify the adapter status entries created by
makeAdapterStatus/finalizedConds in this test (the t.Run block) to use a
finalized condition set where Available is False (or use a helper that sets
Available=False) so ready.Status becomes api.ConditionFalse while
reconciled.Status remains api.ConditionTrue.
In `@pkg/services/aggregation.go`:
- Around line 364-369: The current branch sets reason/message using "Available"
wording even for deleted resources; change the logic to detect deletion (e.g.,
resourceGen.DeletionTimestamp != nil or an IsBeingDeleted helper) and when
deleted emit Finalized-specific reasons/messages instead of Available ones: use
a Finalized true reason (e.g., reasonAllAdaptersFinalized) when status ==
api.ConditionTrue and a Finalized false reason/message (you may need a new
reasonMissingRequiredAdaptersFinalized and a
buildFinalizedFalseMessage(required, byAdapter, resourceGen) or reuse
buildReadyFalseMessage but with Finalized wording) otherwise; keep the existing
variables (reason, message, required, byAdapter, resourceGen, status,
api.ConditionTrue, reasonAllAdaptersAvailable) as anchors when updating the
branches.
- Around line 332-345: The loop that computes allAtCurrent improperly requires
s.availableTrue before it checks deletion semantics when deletedTime != nil;
adjust the iteration in aggregation.go so that when deletedTime is non-nil you
skip the Available=True check and only validate Finalized=True and
observedGeneration == resourceGen for each adapter (i.e., for each name in
required check byAdapter[name], ensure s.observedGeneration == resourceGen and
if deletedTime != nil require s.finalizedTrue, else require s.availableTrue),
using the existing variables and fields (allAtCurrent, required, byAdapter,
deletedTime, s.availableTrue, s.finalizedTrue, s.observedGeneration,
resourceGen) to short-circuit or set allAtCurrent accordingly.
---
Duplicate comments:
In `@pkg/services/aggregation_test.go`:
- Around line 123-144: The tests calling parsePrevConditions now return four
values (r, a, rc, m) but the t.Fatalf messages only print three; update each
failing t.Fatalf in the "nil input", "empty input", and "invalid JSON" subtests
to include rc in the formatted output so failures show all four values (e.g.,
include %v for rc and add rc to the argument list) and ensure the expectation
string mentions the four-tuple to match the returned 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: 0a1ae4c1-ed00-47e6-9091-e649595abd99
📒 Files selected for processing (5)
pkg/api/status_types.gopkg/services/aggregation.gopkg/services/aggregation_test.gopkg/services/cluster.gopkg/services/status_helpers.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/api/status_types.go
- pkg/services/cluster.go
| // required adapters have not yet reported Available=True at the current resource generation. | ||
| const reasonMissingRequiredAdapters = "MissingRequiredAdapters" | ||
| const reasonAllAdaptersAvailable = "All required adapters reported Available=True at the current generation" | ||
| const reasonAllAdaptersReconciled = "All required adapters reconciled at the current generation" |
There was a problem hiding this comment.
Same comment as in the api-spec, we need to be more specific for what it means that a required adapter "is reconciled"
There was a problem hiding this comment.
Changed to All required adapters reported Available=True or Finalized=True at the current generation (similar to openapi)
| reconciled = computeReconciled( | ||
| in.ResourceGeneration, | ||
| in.RefTime, | ||
| in.DeletedTime, | ||
| prevReconciled, | ||
| in.RequiredAdapters, | ||
| reports, | ||
| ) |
There was a problem hiding this comment.
Doesn't ready and reconciled have the same value?
Shouldn't they use the same function for computation?
You could think of the changes as "ready" being just a mirror of "reconciled"
In this case, the mirror may not even happen at the service layer but in the presentation layer.
Think about like this
- Internally, the service works only with "reconciled"
- Which is a renaming of the previous "ready" condition
- Then, when presenting the JSON, you present both ready and reconciled
There was a problem hiding this comment.
As discussed, Ready now replicated Reconciled. However, we have kept the ready = reconciled assignment one layer below the presentation layer since we still need to support search implementation for Ready for backward compatibility (search queries (status.conditions.Ready='True') hit the DB JSONB column directly)
…d remove Ready implementation with test fixes
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/services/aggregation.go (1)
343-358:⚠️ Potential issue | 🟠 MajorUse a Finalized-specific false reason during deletion.
On Lines 344-346, deleted resources still emit
MissingRequiredAdapterseven though the predicate and message are nowFinalized=True. That leavesstatus.conditions[].reasonmisleading for clients that inspect reasons during teardown.Suggested direction
+const reasonMissingRequiredFinalizedAdapters = "MissingRequiredFinalizedAdapters" + var reason, message string if deletedTime != nil { - reason = reasonMissingRequiredAdapters + reason = reasonMissingRequiredFinalizedAdapters message = buildFinalizedFalseMessage(required, byAdapter, resourceGen) if status == api.ConditionTrue { reason = reasonAllAdaptersReconciled message = reason }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/services/aggregation.go` around lines 343 - 358, When handling deleted resources (deletedTime != nil) use a Finalized-specific false reason instead of reasonMissingRequiredAdapters: set reason = reasonMissingRequiredAdaptersFinalized (create this new constant) and message = buildFinalizedFalseMessage(required, byAdapter, resourceGen) when status != api.ConditionTrue; keep the status == api.ConditionTrue branch assigning reasonAllAdaptersReconciled as-is. Leave the non-deleted (else) branch unchanged so it still uses reasonMissingRequiredAdapters with buildReadyFalseMessage.
🧹 Nitpick comments (1)
pkg/services/status_helpers.go (1)
22-37: Extract the shared Ready/Reconciled assembly helper.This block now duplicates the same
reconciled → deprecated ready mirror → ordered condition sliceflow aspkg/services/cluster.go:218-233. Pulling that into one helper would reduce drift in the compatibility layer.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 the current code and only fix it if needed. In `@pkg/services/status_helpers.go` around lines 22 - 37, Extract the duplicated "reconciled → ready mirror → ordered condition slice" into a single helper (e.g., BuildReadyReconciledConditions or AssembleResourceConditions) that accepts the reconciled and available api.ResourceCondition values plus adapterConditions ([]api.ResourceCondition) and returns the final []api.ResourceCondition; replace the inline logic in the current function (where reconciled, available, adapterConditions are produced by AggregateResourceStatus) and the similar block in Cluster code with calls to this helper so the ready mirror is created (ready := reconciled; ready.Type = api.ConditionTypeReady) and the ordering (ready, reconciled, available, adapterConditions...) is centralized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/services/aggregation.go`:
- Around line 343-358: When handling deleted resources (deletedTime != nil) use
a Finalized-specific false reason instead of reasonMissingRequiredAdapters: set
reason = reasonMissingRequiredAdaptersFinalized (create this new constant) and
message = buildFinalizedFalseMessage(required, byAdapter, resourceGen) when
status != api.ConditionTrue; keep the status == api.ConditionTrue branch
assigning reasonAllAdaptersReconciled as-is. Leave the non-deleted (else) branch
unchanged so it still uses reasonMissingRequiredAdapters with
buildReadyFalseMessage.
---
Nitpick comments:
In `@pkg/services/status_helpers.go`:
- Around line 22-37: Extract the duplicated "reconciled → ready mirror → ordered
condition slice" into a single helper (e.g., BuildReadyReconciledConditions or
AssembleResourceConditions) that accepts the reconciled and available
api.ResourceCondition values plus adapterConditions ([]api.ResourceCondition)
and returns the final []api.ResourceCondition; replace the inline logic in the
current function (where reconciled, available, adapterConditions are produced by
AggregateResourceStatus) and the similar block in Cluster code with calls to
this helper so the ready mirror is created (ready := reconciled; ready.Type =
api.ConditionTypeReady) and the ordering (ready, reconciled, available,
adapterConditions...) is centralized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 9eca8c7d-8836-4573-aa4b-09c941dcefeb
📒 Files selected for processing (5)
openapi/openapi.yamlpkg/services/aggregation.gopkg/services/aggregation_test.gopkg/services/cluster.gopkg/services/status_helpers.go
🚧 Files skipped from review as they are similar to previous changes (1)
- openapi/openapi.yaml
|
I see a missing changes to CHANGELOG.md, consider adding that there as well for visibility (even though I see it's unreleased...) |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rh-amarin 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 f31a6c9
into
openshift-hyperfleet:main
4 participants
Summary
Readycomputation withReconciledas the primary aggregated condition (HYPERFLEET-853)Reconciledis a dual-mode condition:deleted_timenot set): all adaptersAvailable=Trueat current generationdeleted_timeset): all adaptersFinalized=Trueat current generationReadyis deprecated — derived fromReconciledat the service layer for backward compatibilityFinalizedcondition type constant (not enforced as mandatory — keeps current adapters compatible)status_conditionsfor efficientReconciledqueriesHow it works
Internally, only
Reconciledis computed.Readyis a copy ofReconciledwith the type changed:Both are stored in the DB so existing status.conditions.Ready='True' search queries continue to work. Customers can migrate to status.conditions.Reconciled='True' at their own pace.
Backward compatibility for existing DB data
parsePrevConditions uses stored Ready as a fallback for prevReconciled when no Reconciled condition exists yet — ensures correct transition time tracking for records written before this change.
Context
The deletion design (https://github.com/openshift-hyperfleet/architecture/blob/main/hyperfleet/components/adapter/framework/adapter-deletion-flow-design.md) requires a condition that adapts based on resource lifecycle. Ready doesn't work for deletion ("Ready=True" for a deleted resource is semantically wrong). Reconciled means "the desired state has been achieved" — works for both creation and
deletion.
Since no adapter sends Finalized in production today, Reconciled will always be False for deleted resources — which is correct (no cleanup has happened). The deletion-aware logic is ready for when adapters implement Finalized and the hard-delete mechanism (HYPERFLEET-854) lands.
Test plan