Skip to content

feat(scaling-adapter): add readyReplicas to RBGSA with sole-writer pattern#243

Merged
Syspretor merged 2 commits intosgl-project:mainfrom
sebest:feat/scaling-adapter-ready-replicas
Apr 13, 2026
Merged

feat(scaling-adapter): add readyReplicas to RBGSA with sole-writer pattern#243
Syspretor merged 2 commits intosgl-project:mainfrom
sebest:feat/scaling-adapter-ready-replicas

Conversation

@sebest
Copy link
Copy Markdown
Contributor

@sebest sebest commented Mar 26, 2026

Summary

  • Adds status.readyReplicas field to the RoleBasedGroupScalingAdapter CRD (v1alpha1 + v1alpha2), mirrored from the parent RBG's status.roleStatuses[].readyReplicas
  • Makes the RBGSA controller the sole writer of its own status, following the Kubernetes convention (Deployment/ReplicaSet, JobSet/Job). The RBG controller continues to own RBGSA lifecycle (create/delete/labels) but does not touch RBGSA status
  • Extracts the inline RBG watch predicate into a named RBGRoleStatusPredicate() function for independent testability
  • Uses minimal SSA patches for readyReplicas sync to avoid taking ownership of unrelated status fields
  • Includes deepcopy, apply configs, CRD YAML generation, printcolumn, and e2e validation

Test plan

  • make fmt vet passes
  • go test ./internal/controller/workloads/ passes (all 16 new sub-tests pass)
  • TestRoleBasedGroupScalingAdapterReconciler_ReadyReplicasSync covers nil→value, stale→updated, no-op, roleStatus-not-found, and patch-error cases
  • TestReadyReplicasSyncWithScale verifies readyReplicas preserved when scale occurs in same reconcile
  • TestRoleBasedGroupScalingAdapterReconciler_Reconcile updated with bind-path-without-roleStatus case
  • TestMapRBGToScalingAdapters covers happy path, wrong type, list error, no adapters
  • TestRBGRoleStatusPredicate covers RoleStatuses changed/unchanged and wrong type
  • E2e validation added to check readyReplicas consistency with RBG role status
  • E2e test run

@gemini-code-assist
Copy link
Copy Markdown

Warning

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

@sebest sebest marked this pull request as draft March 26, 2026 04:33
@sebest sebest force-pushed the feat/scaling-adapter-ready-replicas branch from 1cfd329 to 9a0c5e3 Compare March 26, 2026 04:44
@Syspretor Syspretor self-requested a review March 26, 2026 05:44
@cheyang
Copy link
Copy Markdown
Collaborator

cheyang commented Mar 27, 2026

@sebest Pls fix the conflict issue. Thanks.

@sebest sebest force-pushed the feat/scaling-adapter-ready-replicas branch from 16e1802 to bda3ac8 Compare March 28, 2026 16:34
@sebest
Copy link
Copy Markdown
Contributor Author

sebest commented Mar 28, 2026

@sebest Pls fix the conflict issue. Thanks.

@cheyang the conflict has been resolved, thank you.

Going to address the linter errors.

@sebest sebest force-pushed the feat/scaling-adapter-ready-replicas branch 2 times, most recently from 51931f8 to 98321b5 Compare March 28, 2026 16:46
…ttern

Add readyReplicas field to RBGSA status, synced from RBG role status.
Extract RBG watch predicate into named RBGRoleStatusPredicate function.
Use ToRoleBasedGroupScalingAdapterStatusApplyConfiguration to preserve
existing status fields during readyReplicas sync.

Add comprehensive unit tests covering all new code paths:
- Bind path with/without roleStatus
- Steady-state sync (nil→value, stale→updated, no-op, patch error, roleStatus not found)
- readyReplicas sync + scale in same reconcile
- mapRBGToScalingAdapters (happy path, wrong type, list error, no adapters)
- RBGRoleStatusPredicate (all 4 event funcs: create, update, delete, generic)
@sebest sebest force-pushed the feat/scaling-adapter-ready-replicas branch from 98321b5 to 0294e2d Compare March 28, 2026 17:03
@sebest sebest marked this pull request as ready for review March 28, 2026 17:37
@sebest
Copy link
Copy Markdown
Contributor Author

sebest commented Apr 1, 2026

@cheyang Any remaining blocker ?

@Syspretor
Copy link
Copy Markdown
Collaborator

@cheyang Any remaining blocker ?

@sebest Sorry for the delay in handling this and I believe this is a very necessary change.
One small comment: since the v1alpha1 API is no longer evolving, could you remove the changes related to the v1alpha1 API? After that, I’ll merge this PR.

The v1alpha1 API is frozen and no longer evolving. Remove the
readyReplicas field, printcolumn, deepcopy, and apply configuration
additions from v1alpha1, keeping them only in v1alpha2.

Addresses review feedback on PR sgl-project#243.
@sebest
Copy link
Copy Markdown
Contributor Author

sebest commented Apr 11, 2026

@Syspretor I removed the changes to the v1alpha1 API. thanks

Copy link
Copy Markdown
Collaborator

@Syspretor Syspretor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@Syspretor Syspretor merged commit 97cbfbf into sgl-project:main Apr 13, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants