🔴 Hot News: sgl project/rbg/pull/

Syspretor · 2026-05-19T03:16:26Z

Summary

Remove RecreateRBGOnPodRestart restart policy and the entire Pod Controller that implements it
Remove RestartInProgress condition type and RecreateWorkload interface method
Simplify RBG controller status management by removing dual-controller race condition

Motivation

The RecreateRBGOnPodRestart policy has no valid production use case while introducing significant complexity:

SSA race condition: The Pod Controller and RBG Controller both write to rbg.status.conditions with Force=true, causing RestartInProgress to become permanently stuck at True (root cause of flaky e2e tests)
Increased control plane pressure: The Pod Controller watches all RBG-owned pods and triggers full RBG reconciliation on any pod event
Unnecessary code complexity: RecreateWorkload method on every WorkloadReconciler implementation, dedicated field manager handling, and a separate condition lifecycle — all for a feature no deployment uses

Migration

Users should switch to:

RecreateRoleInstanceOnPodRestart — recreates only the affected role instance (recommended for distributed workloads)
None — relies on native workload controller replacement (recommended for stateless workloads)

Changes

Delete pod_controller.go and pod_controller_test.go
Remove RecreateRBGOnPodRestart enum from v1alpha1 and v1alpha2 API types
Remove RoleBasedGroupRestartInProgress condition type
Remove RecreateWorkload from WorkloadReconciler interface and all implementations
Remove PodControllerFieldManager and PatchStatusWithFieldManager utilities
Simplify RBG controller updateRBGStatus (remove RestartInProgress filtering)
Update restart policy mappings in LWS and RoleInstanceSet reconcilers
Remove associated e2e and unit tests

Test plan

go build ./... passes
go test ./pkg/... ./internal/... ./api/... ./cmd/... all pass
go vet ./... passes
CRD manifests regenerated (make manifests) to remove enum value
Envtest suite passes (make test-envtest)

Fixes #339

🤖 Generated with Claude Code

The RestartInProgress condition could get permanently stuck at True because of a race between the RBG controller's Force=true SSA patch and the Pod controller's Status().Update(). When the RBG controller read the condition before the Pod controller's update was visible, the SSA patch would overwrite False back to True with no subsequent event to correct it. Fix: use separate SSA field managers for each controller. The RBG controller now excludes RestartInProgress from its SSA patch, and the Pod controller uses SSA with PodControllerFieldManager. Since conditions is listType=map, each field manager owns its entries independently, eliminating the race entirely. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: 玖宇 <guotongyu.gty@alibaba-inc.com>

… Controller Remove the RecreateRBGOnPodRestart restart policy entirely along with its implementing Pod Controller. This policy had no valid production use case while introducing significant complexity: - Dual-controller pod management caused SSA race conditions where RestartInProgress condition could become permanently stuck at True - The Pod Controller watching all RBG-owned pods amplified control plane request pressure with no offsetting value - RecreateWorkload method on every WorkloadReconciler added code complexity for a feature no deployment uses Changes: - Delete pod_controller.go and pod_controller_test.go - Remove RecreateRBGOnPodRestart enum from v1alpha1 and v1alpha2 API types - Remove RoleBasedGroupRestartInProgress condition type - Remove RecreateWorkload from WorkloadReconciler interface and all impls - Remove PodControllerFieldManager and PatchStatusWithFieldManager utils - Simplify RBG controller status update (remove RestartInProgress filtering) - Update restart policy mappings in LWS and RoleInstanceSet reconcilers - Remove associated e2e and unit tests Users should migrate to RecreateRoleInstanceOnPodRestart (recreates only the affected instance) or None (relies on native workload controller replacement). Fixes: sgl-project#339 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: 玖宇 <guotongyu.gty@alibaba-inc.com>

gemini-code-assist · 2026-05-19T03:16:29Z

Warning

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

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: 玖宇 <guotongyu.gty@alibaba-inc.com>

Copilot

Pull request overview

This PR removes the RecreateRBGOnPodRestart restart policy and its supporting implementation (the dedicated Pod Controller), along with related API surface area (RestartInProgress condition and RecreateWorkload interface method). It also simplifies RoleBasedGroup (RBG) status updates now that status-condition ownership no longer needs to be shared across controllers.

Changes:

Delete the Pod Controller and all associated unit/e2e tests and test utilities.
Remove RecreateRBGOnPodRestart and RestartInProgress from v1alpha1/v1alpha2 APIs and regenerated CRDs.
Simplify RBG status SSA patching and update restart-policy mapping logic in workload reconcilers.

Reviewed changes

Copilot reviewed 28 out of 28 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
test/utils/utils.go	Removes pod-deletion and container-restart simulation helpers used by the deleted policy’s tests.
test/envtest/testutil/setup.go	Stops registering the removed Pod Controller in envtest.
test/e2e/testcase/v1alpha2/sts.go	Removes v1alpha2 e2e coverage for the removed restart policy on StatefulSet.
test/e2e/testcase/v1alpha2/lws.go	Removes v1alpha2 e2e coverage for the removed restart policy on LWS.
test/e2e/testcase/v1alpha2/inactive_pod.go	Removes cases/assertions tied to the deleted restart condition and policy.
test/e2e/testcase/v1alpha2/deploy.go	Removes v1alpha2 e2e coverage for the removed restart policy on Deployment.
test/e2e/testcase/v1alpha1/sts.go	Removes v1alpha1 e2e coverage for the removed restart policy on StatefulSet.
test/e2e/testcase/v1alpha1/lws.go	Removes v1alpha1 e2e coverage for the removed restart policy on LWS.
test/e2e/testcase/v1alpha1/deploy.go	Removes v1alpha1 e2e coverage for the removed restart policy on Deployment.
pkg/utils/utils.go	Removes Pod-Controller-specific field manager and status patch helper.
pkg/reconciler/workload_reconciler.go	Removes `RecreateWorkload` from the reconciler interface.
pkg/reconciler/sts_reconciler.go	Removes StatefulSet `RecreateWorkload` implementation.
pkg/reconciler/sts_reconciler_test.go	Removes unit tests for the deleted StatefulSet `RecreateWorkload`.
pkg/reconciler/roleinstanceset_reconciler.go	Updates restart-policy mapping and removes `RecreateWorkload` implementation.
pkg/reconciler/roleinstance/sync/instance_scale.go	Updates comments to reflect removal of `RecreateRBGOnPodRestart`.
pkg/reconciler/lws_reconciler.go	Updates restart-policy mapping and removes `RecreateWorkload` implementation.
pkg/reconciler/lws_reconciler_test.go	Removes unit tests for the deleted LWS `RecreateWorkload`.
pkg/reconciler/deploy_reconciler.go	Removes Deployment `RecreateWorkload` implementation.
pkg/reconciler/deploy_reconciler_test.go	Removes unit tests for the deleted Deployment `RecreateWorkload`.
internal/controller/workloads/rolebasedgroup_controller.go	Simplifies status update logic and inlines status apply-configuration building.
internal/controller/workloads/pod_controller.go	Deletes the Pod Controller that implemented `RecreateRBGOnPodRestart`.
internal/controller/workloads/pod_controller_test.go	Deletes unit tests for the removed Pod Controller.
config/crd/bases/workloads.x-k8s.io_rolebasedgroupsets.yaml	Regenerates CRD schema to remove the deleted enum value.
config/crd/bases/workloads.x-k8s.io_rolebasedgroups.yaml	Regenerates CRD schema to remove the deleted enum value.
cmd/rbgs/main.go	Removes Pod Controller registration from the manager startup.
api/workloads/v1alpha2/rolebasedgroup_types.go	Removes enum value and condition type from the v1alpha2 API.
api/workloads/v1alpha1/rolebasedgroup_types.go	Removes enum value and condition type from the v1alpha1 API.
api/workloads/v1alpha1/constant.go	Removes the v1alpha1 `RecreateRBGOnPodRestart` constant.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

cheyang

/lgtm
/approve

- Fix slice mutation bug: use explicit alloc instead of append(pods, ...) which could modify the original pods slice - Reorder Scale logic: delete Failed pods before creating replacements, avoiding unnecessary reconcile cycles when hasOrphanPod blocks creation - Remove dead code: ContainerRestarted and PatioRuntimeContainerName are no longer used after Pod Controller removal in sgl-project#340 Refs: sgl-project#342 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: 玖宇 <guotongyu.gty@alibaba-inc.com>

- Fix E2E Case 2: use expectedPodCount from actual Instance pod count instead of hardcoded check, preventing flaky assertion when pod count varies during recreation - Update KEP to reflect upstream sgl-project#340 changes: remove Pod Controller references (deleted), remove RecreateRBGOnPodRestart (deprecated), update test plan to reference actual existing tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: 玖宇 <guotongyu.gty@alibaba-inc.com>

* fix(rbg): handle inactive pod recreation and clean up dead code Two-level Failed pod handling in RoleInstance Controller: - Level 1: Delete Failed pods so replacements can be created (all policies) - Level 2: Recreate entire Instance for RecreateRoleInstanceOnPodRestart policy Pass inactivePods through reconciler chain so Failed pods are visible to shouldRecreateInstance. Remove dead utility functions (PodBecameInactive, PodBecameFailed, IsPodEvicted, etc.) and their tests. Update Pod Controller predicate test to match actual logic. Clean up "Dimension" terminology from code, tests, and KEP. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: 玖宇 <guotongyu.gty@alibaba-inc.com> * fix(rbg): address PR #337 review comments - Restore ContainerRestartCountChanged predicate filter to avoid performance regression from triggering on every pod status update. Only trigger on container restart count changes or DeletionTimestamp transitions. - Fix incorrect comment in shouldRecreateInstance: container restart with RecreateRoleInstanceOnPodRestart is handled by the underlying workload controller (e.g. LWS controller), not the Pod Controller. - Remove dead code getExpectedPodCount and its tests. - Update predicate tests to match restored filter logic. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: 玖宇 <guotongyu.gty@alibaba-inc.com> * fix(rbg): fix Scale ordering and remove dead code - Fix slice mutation bug: use explicit alloc instead of append(pods, ...) which could modify the original pods slice - Reorder Scale logic: delete Failed pods before creating replacements, avoiding unnecessary reconcile cycles when hasOrphanPod blocks creation - Remove dead code: ContainerRestarted and PatioRuntimeContainerName are no longer used after Pod Controller removal in #340 Refs: #342 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: 玖宇 <guotongyu.gty@alibaba-inc.com> * fix: update KEP and fix E2E flakiness for inactive pod handling - Fix E2E Case 2: use expectedPodCount from actual Instance pod count instead of hardcoded check, preventing flaky assertion when pod count varies during recreation - Update KEP to reflect upstream #340 changes: remove Pod Controller references (deleted), remove RecreateRBGOnPodRestart (deprecated), update test plan to reference actual existing tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: 玖宇 <guotongyu.gty@alibaba-inc.com> --------- Signed-off-by: 玖宇 <guotongyu.gty@alibaba-inc.com> Co-authored-by: 玖宇 <guotongyu.gty@alibaba-inc.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>

…eferences Address PR review comments: - Remove all references to RecreateRBGOnPodRestart (removed in upstream sgl-project#340) - Update annotation godoc to only mention RecreateRoleInstanceOnPodRestart - Fix failure-handling.md to reflect current restart policy types - Update example YAML annotation comment - Fix "Use Cases" section to not list ignoreRestartPolicy as a policy enum Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

#332) * feat: add annotation to exclude components from triggering restart policy Add ComponentNoTriggerRestartPolicyAnnotationKey to allow specific components in customComponentsPattern to be excluded from triggering the role's restart policy. When this annotation is set to "true" on a component's pod template, pod restart/delete events from this component will NOT trigger RecreateRoleInstanceOnPodRestart or RecreateRBGOnPodRestart actions. This is useful for auxiliary components (e.g., monitoring, logging sidecars) whose failures should not affect the main workload. Usage: template: metadata: annotations: rbg.workloads.x-k8s.io/component-no-trigger-restart: "true" Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat: change restart trigger annotation to policy enum format Change the annotation from boolean format to policy enum format: - Annotation key: rbg.workloads.x-k8s.io/restart-trigger-policy - Valid values: "Inherit" (default) and "Ignore" "Inherit" means the component follows the role's restart policy. "Ignore" means the component's pod events will not trigger restart policy. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix comments * feat: re-implement restart-trigger-policy annotation on upstream main Re-implement the component-level restart-trigger-policy annotation after rebase onto upstream/main which removed pod_controller.go. The logic now lives in shouldRecreateInstance (instance_scale.go): - Skip pods with restart-trigger-policy=Ignore when checking for Failed phase or container restart (RestartCount > 0) - Add containerRestarted check for RoleInstanceSet-managed instances - Add unit tests for hasTriggerPolicyIgnore, containerRestarted, and shouldRecreateInstance with annotation scenarios - Add e2e tests for customComponentsPattern verifying ignored vs non-ignored component pod failure behavior Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: align docs with implementation, remove RecreateRBGOnPodRestart references Address PR review comments: - Remove all references to RecreateRBGOnPodRestart (removed in upstream #340) - Update annotation godoc to only mention RecreateRoleInstanceOnPodRestart - Fix failure-handling.md to reflect current restart policy types - Update example YAML annotation comment - Fix "Use Cases" section to not list ignoreRestartPolicy as a policy enum Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: add trailing newline to custom-components-pattern.yaml Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat: add Restarting condition to prevent cascading restart-policy recreations When restartPolicy=RecreateRoleInstanceOnPodRestart triggers, newly recreated pods may have transient RestartCount > 0, which would re-trigger recreation in an infinite loop. Add a Restarting=True condition that is set when recreation triggers and cleared when the instance becomes Ready again, breaking the cascade. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: 玖宇 <guotongyu.gty@alibaba-inc.com> * fix: use in-memory cache + apiReader for restart-loop prevention Replace the inline Restarting condition check (which relied on the informer cache) with a defer-based guard in shouldRecreateInstanceGuarded. The guard first checks a sync.Map cache (zero latency, immune to informer lag), then falls back to a direct API server read via apiReader. This eliminates the race window where informer cache lag could allow a cascading restart to slip through. Remove allPodsTerminating and isInstanceRestartingFromCache helpers that are superseded by this approach. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: only propagate Restarting condition while scaling is active The Restarting condition was being unconditionally propagated from updateInstance to newStatus on every reconcile. Since updateInstance carries the persisted status (including Restarting=True from a previous reconcile), this prevented setInstanceConditions from clearing it when the instance became Ready. Move the propagation inside the `if scaling` block so it is only carried forward while pod deletion/creation is in progress. Once scaling completes, the condition is no longer injected into newStatus, allowing setInstanceConditions to clear it via its instanceReady guard. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Signed-off-by: 玖宇 <guotongyu.gty@alibaba-inc.com> Co-authored-by: 玖宇 <guotongyu.gty@alibaba-inc.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>

玖宇 and others added 2 commits May 18, 2026 15:48

Syspretor changed the title ~~Deprecate and remove RecreateRBGOnPodRestart restart policy~~ chore(rbg): deprecate and remove RecreateRBGOnPodRestart restart policy May 19, 2026

Syspretor requested a review from cheyang May 19, 2026 03:17

chore: regenerate CRD manifests to remove RecreateRBGOnPodRestart enum

71b5d9c

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: 玖宇 <guotongyu.gty@alibaba-inc.com>

cheyang requested a review from Copilot May 19, 2026 03:43

Copilot started reviewing on behalf of cheyang May 19, 2026 03:43

View session

Copilot AI reviewed May 19, 2026

View reviewed changes

Comment thread pkg/reconciler/lws_reconciler.go

Comment thread pkg/reconciler/roleinstanceset_reconciler.go

cheyang approved these changes May 19, 2026

View reviewed changes

cheyang merged commit 213b0c7 into sgl-project:main May 19, 2026
13 of 14 checks passed

Syspretor mentioned this pull request May 19, 2026

RFC: Two-level Failed Pod handling in RoleInstance Controller #342

Open

cheyang mentioned this pull request Jun 12, 2026

fix: prevent updates of RestartInProgress condition be covered by updating role status #88

Open

3 tasks done

cheyang mentioned this pull request Jul 28, 2026

chore: add rbg-inference-deploy agent skill and CLAUDE.md #410

Open

Navigation Menu

Repository navigation

chore(rbg): deprecate and remove RecreateRBGOnPodRestart restart policy - #340

chore(rbg): deprecate and remove RecreateRBGOnPodRestart restart policy#340
cheyang merged 3 commits into
sgl-project:mainfrom
Syspretor:deprecate/remove-recreate-rbg-on-pod-restart

Syspretor commented May 19, 2026 •

edited by bcfre

Loading

Uh oh!

gemini-code-assist Bot commented May 19, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

cheyang left a comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

Syspretor commented May 19, 2026 • edited by bcfre Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Motivation

Migration

Changes

Test plan

Uh oh!

gemini-code-assist Bot commented May 19, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

cheyang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Syspretor commented May 19, 2026 •

edited by bcfre

Loading