feat(rbgsa-controller): rollout-aware scale-down and spec.replicas drift detection

[sebest]

This PR adds two related RBGSA-controller behaviors. They are bundled because the second reuses the watch infrastructure introduced for the first.

Summary

1. Rollout-aware scale-down gating (original scope)

• Gate scale-down at the RBGSA controller level for partition-based workloads (StatefulSet, LWS, RoleInstanceSet) while a rolling update is in progress.
• StatefulSet deletes highest-ordinal pods on scale-down, which are the already-updated pods — this destroys rollout progress.
• Scale-up and Deployment-backed roles are unaffected.
• Opt-in via scalingAdapter.scaleDownPolicy: DeferDuringRollout (default is Unrestricted for backward compatibility).

2. Wake on per-role spec.replicas drift (new — second commit)

• Extend the RBG-watch predicate (renamed RBGRoleStatusPredicate → RBGRoleSpecOrStatusPredicate) to also fire when any role's spec.replicas changes between old and new.
• Without this, the RBGSA controller's writeback path only wakes on status changes, so an external mutator that stomps spec.roles[].replicas (e.g. helm upgrade whose template stops emitting the field, kubectl edit from a manifest that omits replicas, non-SSA scripts) waits for the next status-change event before the apiserver-stored spec is corrected.
• Pairs with the in-memory override added in fix(rbg-controller): override role.Replicas from bound RBGSA #306: that override prevents the workload from being sized to the stale value on the next reconcile, and this predicate change closes the latency gap by waking the writeback immediately on the stomp instead of when status next changes.
• New rolesReplicasDiffer helper matches roles by name; added, removed, and renamed roles all count as a difference. Other spec fields (image, env, command) deliberately do not fire the predicate — the adapter reconcile is irrelevant to them.

Changes

Rollout-aware scale-down:

• Add Conditions field and ScaleDownDeferred condition to RBGSA status
• Add ScaleDownPolicy field to ScalingAdapter spec (DeferDuringRollout / Unrestricted)
• Detect rollout via RoleStatus.UpdatedReplicas < Replicas (workload-agnostic)
• Gate only partition-based workloads using existing IsStatefulRole() helper
• Add Watches() on RBG for event-driven rollout completion detection (resolves existing TODO)
• Emit Warning event on first deferral only (rate-limited, not on every requeue)
• Use SSA for all status writes (consistent field manager ownership)
• DeepCopy after Get to prevent informer cache corruption
• Expand doc/features/autoscaler.md with full scaling adapter documentation
• Update API reference and variables docs

Spec drift detection:

• Rename RBGRoleStatusPredicate → RBGRoleSpecOrStatusPredicate; restructure UpdateFunc to OR-in a per-role spec.replicas check
• Add rolesReplicasDiffer helper (matches by role name; nil/non-nil pointer transitions and added/removed/renamed roles all count as differences)

Design proposal

Closes #269

Companion PR

• fix(rbg-controller): override role.Replicas from bound RBGSA #306 — adds applyRBGSAReplicasOverride at the top of the RBG Reconcile so the workload is never sized from a stale spec value. Together with the predicate extension here, this provides defense-in-depth against external mutators that drop spec.roles[].replicas.

Test plan

• All RBGSA controller unit tests pass, including:
  • TestScaleDownDeferredDuringRollout, TestScaleDownDeferredConditionLifecycle, TestFindScalingAdaptersByRBG (rollout gating)
  • TestRBGRoleSpecOrStatusPredicate (renamed; extended with 6 new sub-tests covering spec.replicas nil↔set, equal-but-other-fields-differ, added/removed/renamed roles)
  • TestRBGStatusChangedPredicate (envtest-side, also updated to use the renamed predicate)
• Full controller test suite passes (go test ./internal/controller/workloads/...)
• make generate && make manifests — codegen clean
• make lint — passes (cyclomatic complexity under 30)
• Manual: create RBG with STS role + RBGSA + HPA, trigger rollout, observe scale-down deferred
• Manual: simulate helm upgrade that drops spec.roles[].replicas, observe RBGSA writeback fires immediately (paired with fix(rbg-controller): override role.Replicas from bound RBGSA #306)

[gemini-code-assist]

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

[Syspretor]

@sebest I think the motivation behind this change makes a lot of sense. Please give us a little time to think through the API and workflow design, and I’ll follow up with feedback afterward.

[Syspretor]

@sebest We reviewed this PR in our community weekly meeting, and overall everyone really appreciated both the motivation and the design.

The only feedback we had was around the naming/semantics of DeferDuringRollout, which feels a bit ambiguous to us. Would it make sense to consider a clearer name, something along the lines of DeferUntilRolloutCompleted, to make the intended behavior more explicit?

[Syspretor]

@sebest the e2e failure is not caused by this PR. It was due to an issue on the main branch, which I’ve already fixed. Could you please rebase onto the latest main branch and push again to rerun the e2e tests? Thanks!

[Syspretor]

@sebest btw, We’d also be glad to have you join the community discussions. If you’re interested, please feel free to reach out to me at tongyuguo@outlook.com.

[cheyang]

Structured logger (klog/logr) doesn't interpret %s format verbs — this ends up logging the literal %s in the message string. Should be key-value style:

logger.Error(err, "Failed to update status", "rbgScalingAdapterName", rbgScalingAdapter.Name)

[cheyang]

Review: rollout-aware scale-down & spec.replicas drift detection

Status: This PR currently has merge conflicts — needs rebase onto latest main (as previously requested).

Architecture & Design

The approach is well-considered:

• Gating at the RBGSA controller level rather than the workload controller is the right call — it keeps the scaling adapter self-contained and avoids entangling rollout logic with the core RBG reconciler.
• The distinction between partition-based workloads (STS, LWS, RoleInstanceSet) and Deployments is correctly scoped via IsStatefulRole().
• Default of Unrestricted preserves backward compatibility without surprise behavioral changes for existing users.
• The rolesReplicasDiffer predicate for spec.replicas drift detection is narrowly scoped (only fires on replica count changes, not other spec mutations), which avoids excessive reconcile churn.

Correctness

• deferScaleDownIfNeeded correctly short-circuits for scale-up (desiredReplicas >= currentReplicas).
• Rollout detection via UpdatedReplicas < Replicas from RBG status is reasonable — it reflects the workload controller's view of partition progress.
• Event emission only on first deferral (checking condition presence before emitting) avoids log/event noise on every 30s requeue.
• The DeepCopy after Get prevents informer cache mutation — important since conditions are modified in-place.
• SSA for status writes is consistent with the existing pattern in this controller.

Minor Issue

One structured-logger misuse (see inline comment): logger.Error(err, "Failed to update status for %s", ...) — %s isn't interpolated by logr.

Tests

Coverage is thorough:

• 13 table-driven unit test cases covering all permutations of workload type, policy, rollout state, and scaling direction
• Condition lifecycle tests (set/clear/idempotent re-set)
• 6 envtest integration tests covering the full controller interaction including scale subresource writes and RBG status watch
• Predicate tests for the new rolesReplicasDiffer logic

Summary

The design and implementation are solid. The blocking issue is the merge conflict. Once rebased and CI passes, this looks ready to merge pending the one-line logger fix.

[cheyang]

Nicely designed feature with strong test coverage (810 lines of unit tests + 350 lines of envtest). The rollout-gating logic, condition lifecycle, and SSA status writes all look correct. The DeepCopy-after-Get fix for informer cache safety is a good catch.

I left one concrete comment on a malformed logger.Error call in the scale-down error path, and a minor test coverage suggestion.

The PR currently has merge conflicts with main -- a rebase is needed before this can be merged.

[cheyang]

This logger.Error call uses %s in the message string and passes rbgScalingAdapter.Name as a lone positional arg:

logger.Error(err, "Failed to update status for %s", rbgScalingAdapter.Name)

In logr, Error(err, msg, keysAndValues...) does not interpolate format verbs -- %s appears literally in the output, and the adapter name becomes an orphaned key (odd-numbered arg) that triggers a MISSING value warning in the structured log.

Consider:

logger.Error(err, "Failed to update status", "rbgScalingAdapterName", rbgScalingAdapter.Name)

[cheyang]

The condition lifecycle tests cover deferral, rollout completion, and HPA adjusting upward, but there is no test for the scenario where the HPA adjusts desired replicas to exactly match current replicas while a ScaleDownDeferred condition is active. In that case clearScaleDownDeferredCondition on the "nothing to do" path should remove the stale condition. A small table-driven case would close this gap.

This is a minor gap -- the path is exercised implicitly, but an explicit test documents the expected behavior.

[cheyang]

Hi @sebest — this PR now conflicts with main. Please rebase onto the latest main and force-push so we can validate the changes with green CI. The previously noted logger.Error format-string fix is still outstanding as well. Thanks!