Add Available/Progressing conditions to DisaggregatedSet status (#980)
* Add Available/Progressing conditions to DisaggregatedSet status Follow-up to #933 (which added observedGeneration and roleStatuses), per @yankay's request for focused review of the condition model. status.conditions now reports: - Available: every role's replicas (aggregated across slices) are at their desired count, all ready, and all updated to the current revision. A role with a desired count of 0 (the documented all-roles-paused state) is satisfied once fully drained to 0, rather than reading as permanently Progressing. - Progressing: otherwise. A role whose running replicas are internally consistent (ready == updated == running) but below its desired count still counts as Progressing - matching on desired, not just on self-consistency, is what makes the zero-replica case work correctly without special-casing it. Available and Progressing are mutually exclusive; LastTransitionTime only changes on an actual Status flip, not on every ObservedGeneration bump, per the metav1.Condition contract. No Degraded condition: distinguishing "still rolling out" from "genuinely stuck" needs a stronger signal (pod failure reasons, timeout heuristics) than what's derivable from the owned LWS statuses today. * Use scaler-resolved target for External roles; keep condition fields in sync Two fixes from Copilot review on #980: - updateStatus compared each role's replicas against spec.replicas, but that field is documented as ignored for roles with scaling.mode: External - the effective target comes from the role's DisaggregatedSetRoleScaler instead. Switched to the same getTargetReplicas helper reconcileRoleSimple already uses, so status and actual reconcile behavior can't disagree. Added TestStatusUsesScalerTargetForExternalRoles. - setDisaggregatedSetCondition left a flipped-to-false condition's old Reason/Message in place (e.g. Available=False but Reason=AllRolesReady), and only synced ObservedGeneration - not Reason/Message - when Status stayed the same. Both paths now sync every field, still only touching LastTransitionTime on an actual Status flip. Added TestSetDisaggregatedSetCondition_SameStatusSyncsReasonAndMessage and extended the existing flip test to assert on Reason/Message too. * Scope condition exclusivity to the Available/Progressing pair; treat a missing External-role scaler as Progressing Two more Copilot findings on #980: - setDisaggregatedSetCondition flipped ANY Status=True condition to False when setting a new one true, not just the specific Available/Progressing pair. Harmless today (only those two types exist), but would silently clobber any future condition type, or one written by another controller. Added exclusiveConditionTypes, mirroring the existing pattern in leaderworkerset_controller.go, and scoped the flip to it. Added TestSetDisaggregatedSetCondition_LeavesUnrelatedConditionTypeUntouched. - For an External role whose scaler is missing from the map (e.g. its generated name collided with a foreign, non-owned DisaggregatedSetRoleScaler - the same class of issue as #981, but for scalers instead of LWS), getTargetReplicas fell back to a literal 0. If the role also happened to have 0 actual replicas, that read as satisfied instead of unknown. Now treated as explicitly Progressing. Added TestStatusProgressingWhenExternalRoleScalerMissing. * Fix goimports formatting in disaggregatedset_controller_test.go pull-lws-verify-main flagged a misaligned struct-literal field (RoleLabelKey) in the foreignScaler test fixture added in the previous commit. gofmt-only fix, no behavior change. * Sync ObservedGeneration on the exclusive-pair condition even when it stays False setDisaggregatedSetCondition only touched the other condition in the Available/Progressing pair via the Status=True-to-False flip branch. Once that condition had settled to False, later reconciles left its ObservedGeneration frozen at whatever it was on the last flip, even as the DisaggregatedSet's own generation kept advancing while the active condition (e.g. Available) stayed True with no real transition. That's misleading for a client reading conditions[Progressing].observedGeneration expecting it to track how current the reported state is. Added the missing branch: when the exclusive-pair condition is already False and stays False, only ObservedGeneration catches up (Reason and Message are left alone, since they still accurately describe why it became false; LastTransitionTime is untouched since Status didn't transition). Added a regression test pinning this exact scenario. * Update Get call sites for the ownership-aware signature merged in main Rebasing onto main (which now has the LeaderWorkerSetManager.Get signature change) auto-resolved every pre-existing call site via the three-way merge, since main's own fix touched the same lines. Two calls this branch added itself (in TestStatusUsesScalerTargetForExternalRoles, which doesn't exist on main) still used the old Get(ctx, namespace, name) form and needed a manual update to Get(ctx, disaggregatedSet, name).
B
Ben Ayed Mohamed Ala committed
060ab4da17eea1f0865fcbd3236bb1075e2cea32
Parent: 3824359
Committed by GitHub <noreply@github.com>
on 8/21/2026, 6:05:16 PM