feat: add exponential backoff for restart policy#394
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces exponential backoff configurations (CrashPodRestartLimit, BaseDelaySeconds, and MaxDelaySeconds) for restart policies across workload patterns and role instances, tracking restart metrics in RoleInstanceStatus to pace recreation attempts. The review feedback highlights a critical integer overflow vulnerability in the backoff delay calculation, recommends implementing a mechanism to reset restart tracking once an instance has been stable for a prolonged period, and suggests adding defensive nil receiver checks to the new helper methods to prevent potential panics.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| func calculateRestartDelay(baseDelaySeconds, maxDelaySeconds, restartCount int32) int32 { | ||
| if baseDelaySeconds == 0 { | ||
| return 0 | ||
| } | ||
| // Prevent overflow for large restart counts | ||
| if restartCount > 30 { | ||
| return maxDelaySeconds | ||
| } | ||
| delay := baseDelaySeconds * (1 << restartCount) | ||
| if maxDelaySeconds > 0 && delay > maxDelaySeconds { | ||
| return maxDelaySeconds | ||
| } | ||
| return delay | ||
| } |
There was a problem hiding this comment.
The current implementation of calculateRestartDelay is vulnerable to integer overflow.
Specifically, baseDelaySeconds * (1 << restartCount) is calculated using 32-bit signed integer arithmetic. If restartCount is large (e.g., 30) and baseDelaySeconds is 30, the multiplication will overflow int32 and wrap around to a negative value. This negative value will bypass the delay > maxDelaySeconds check and return a negative delay, which completely breaks the backoff mechanism and leads to rapid, infinite restart loops.
To prevent this, perform the calculation using int64 and cap the result appropriately before converting back to int32.
func calculateRestartDelay(baseDelaySeconds, maxDelaySeconds, restartCount int32) int32 {
if baseDelaySeconds == 0 {
return 0
}
// Prevent overflow for large restart counts
if restartCount > 30 {
if maxDelaySeconds > 0 {
return maxDelaySeconds
}
return 2147483647
}
delay := int64(baseDelaySeconds) * (int64(1) << restartCount)
if maxDelaySeconds > 0 && delay > int64(maxDelaySeconds) {
return maxDelaySeconds
}
if delay > 2147483647 {
return 2147483647
}
return int32(delay)
}| func (c *realControl) calculateDiffsWithExpectation(ctx context.Context, updateInstance *workloadsv1alpha2.RoleInstance, | ||
| currentRevision, updateRevision *apps.ControllerRevision, | ||
| revisions []*apps.ControllerRevision, pods []*v1.Pod, inactivePods []*v1.Pod) (*expectationDiff, error) { | ||
|
|
||
| // Check restart backoff BEFORE any pod processing (including inactive pod cleanup). | ||
| // If the delay from the last restart hasn't elapsed, skip all scaling and requeue. | ||
| // This preserves crashed pods during the backoff window for log observability. | ||
| if requeue := c.checkRestartBackoff(ctx, updateInstance, pods, inactivePods); requeue > 0 { | ||
| return &expectationDiff{requeueAfter: requeue}, nil | ||
| } |
There was a problem hiding this comment.
Currently, there is no mechanism to reset RestartCount and LastRestartTime when the instance becomes healthy and runs successfully for a long time. Without a reset mechanism, any future crash (even weeks or months later) will immediately suffer the maximum backoff delay, defeating the purpose of an "exponential" backoff.
To fix this, reset the restart tracking fields if the instance has been stable/healthy (no restart triggered) for a duration longer than max(maxDelaySeconds * 2, 10 minutes).
func (c *realControl) calculateDiffsWithExpectation(ctx context.Context, updateInstance *workloadsv1alpha2.RoleInstance,
currentRevision, updateRevision *apps.ControllerRevision,
revisions []*apps.ControllerRevision, pods []*v1.Pod, inactivePods []*v1.Pod) (*expectationDiff, error) {
// Reset restart backoff tracking if the instance has been stable/healthy for a long time.
if updateInstance.Status.RestartCount > 0 && updateInstance.Status.LastRestartTime != nil {
maxDelay := updateInstance.Spec.GetMaxDelaySeconds()
threshold := time.Duration(maxDelay) * time.Second * 2
if threshold < 10*time.Minute {
threshold = 10 * time.Minute
}
if time.Since(updateInstance.Status.LastRestartTime.Time) > threshold {
updateInstance.Status.RestartCount = 0
updateInstance.Status.LastRestartTime = nil
}
}
// Check restart backoff BEFORE any pod processing (including inactive pod cleanup).
// If the delay from the last restart hasn't elapsed, skip all scaling and requeue.
// This preserves crashed pods during the backoff window for log observability.
if requeue := c.checkRestartBackoff(ctx, updateInstance, pods, inactivePods); requeue > 0 {
return &expectationDiff{requeueAfter: requeue}, nil
}| func (r *RoleSpec) GetCrashPodRestartLimit() int32 { | ||
| if r.LeaderWorkerPattern != nil && r.LeaderWorkerPattern.CrashPodRestartLimit != nil { | ||
| return *r.LeaderWorkerPattern.CrashPodRestartLimit | ||
| } | ||
| if r.CustomComponentsPattern != nil && r.CustomComponentsPattern.CrashPodRestartLimit != nil { | ||
| return *r.CustomComponentsPattern.CrashPodRestartLimit | ||
| } | ||
| return DefaultCrashPodRestartLimit | ||
| } | ||
|
|
||
| // GetBaseDelaySeconds returns the configured base delay seconds, | ||
| // or the default (30) if not set. | ||
| func (r *RoleSpec) GetBaseDelaySeconds() int32 { | ||
| if r.LeaderWorkerPattern != nil && r.LeaderWorkerPattern.BaseDelaySeconds != nil { | ||
| return *r.LeaderWorkerPattern.BaseDelaySeconds | ||
| } | ||
| if r.CustomComponentsPattern != nil && r.CustomComponentsPattern.BaseDelaySeconds != nil { | ||
| return *r.CustomComponentsPattern.BaseDelaySeconds | ||
| } | ||
| return DefaultBaseDelaySeconds | ||
| } | ||
|
|
||
| // GetMaxDelaySeconds returns the configured max delay seconds, | ||
| // or the default (600) if not set. | ||
| func (r *RoleSpec) GetMaxDelaySeconds() int32 { | ||
| if r.LeaderWorkerPattern != nil && r.LeaderWorkerPattern.MaxDelaySeconds != nil { | ||
| return *r.LeaderWorkerPattern.MaxDelaySeconds | ||
| } | ||
| if r.CustomComponentsPattern != nil && r.CustomComponentsPattern.MaxDelaySeconds != nil { | ||
| return *r.CustomComponentsPattern.MaxDelaySeconds | ||
| } | ||
| return DefaultMaxDelaySeconds | ||
| } |
There was a problem hiding this comment.
Add defensive nil receiver checks to GetCrashPodRestartLimit, GetBaseDelaySeconds, and GetMaxDelaySeconds to prevent potential nil pointer dereference panics if these methods are called on a nil *RoleSpec pointer (consistent with other helper methods like GetWorkloadType).
func (r *RoleSpec) GetCrashPodRestartLimit() int32 {
if r == nil {
return DefaultCrashPodRestartLimit
}
if r.LeaderWorkerPattern != nil && r.LeaderWorkerPattern.CrashPodRestartLimit != nil {
return *r.LeaderWorkerPattern.CrashPodRestartLimit
}
if r.CustomComponentsPattern != nil && r.CustomComponentsPattern.CrashPodRestartLimit != nil {
return *r.CustomComponentsPattern.CrashPodRestartLimit
}
return DefaultCrashPodRestartLimit
}
// GetBaseDelaySeconds returns the configured base delay seconds,
// or the default (30) if not set.
func (r *RoleSpec) GetBaseDelaySeconds() int32 {
if r == nil {
return DefaultBaseDelaySeconds
}
if r.LeaderWorkerPattern != nil && r.LeaderWorkerPattern.BaseDelaySeconds != nil {
return *r.LeaderWorkerPattern.BaseDelaySeconds
}
if r.CustomComponentsPattern != nil && r.CustomComponentsPattern.BaseDelaySeconds != nil {
return *r.CustomComponentsPattern.BaseDelaySeconds
}
return DefaultBaseDelaySeconds
}
// GetMaxDelaySeconds returns the configured max delay seconds,
// or the default (600) if not set.
func (r *RoleSpec) GetMaxDelaySeconds() int32 {
if r == nil {
return DefaultMaxDelaySeconds
}
if r.LeaderWorkerPattern != nil && r.LeaderWorkerPattern.MaxDelaySeconds != nil {
return *r.LeaderWorkerPattern.MaxDelaySeconds
}
if r.CustomComponentsPattern != nil && r.CustomComponentsPattern.MaxDelaySeconds != nil {
return *r.CustomComponentsPattern.MaxDelaySeconds
}
return DefaultMaxDelaySeconds
}
Syspretor
force-pushed
the
feature/restart-policy-backoff
branch
from
cb82a48 to
0b0a479
Compare
|
/copilot review |
|
@gemini-code-assist /review |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
| return maxDelaySeconds | ||
| } | ||
| return delay | ||
| } |
There was a problem hiding this comment.
This overflows int32 when restartCount is in the ~27-30 range (e.g., 30 * (1<<27) = 4B > int32 max). The current > 30 guard doesn't cover this window. The wrapped negative value then bypasses the max-cap check and gets returned as-is, effectively disabling backoff under sustained crashes.
Suggestion: do the shift in int64, cap, then convert back:
delay := int64(baseDelaySeconds) * (int64(1) << restartCount)
if maxDelaySeconds > 0 && delay > int64(maxDelaySeconds) {
return maxDelaySeconds
}
if delay > math.MaxInt32 {
return maxDelaySeconds
}
return int32(delay)
| // This preserves crashed pods during the backoff window for log observability. | ||
| if requeue := c.checkRestartBackoff(ctx, updateInstance, pods, inactivePods); requeue > 0 { | ||
| return &expectationDiff{requeueAfter: requeue}, nil | ||
| } |
There was a problem hiding this comment.
nit (non-blocking): RestartCount only grows and is never reset. After the instance has been healthy for a long stretch (say, 2 * maxDelaySeconds), consider zeroing it out so a future one-off crash doesn't immediately land at the maximum backoff. Could be a follow-up PR.
| ConditionType RoleInstanceConditionType `json:"conditionType"` | ||
| } | ||
|
|
||
| // Default values for restart policy delay configuration on RoleInstance. |
There was a problem hiding this comment.
nit: these duplicate DefaultBaseDelaySeconds / DefaultMaxDelaySeconds from helper.go with different names but identical values. Minor, but sharing one pair reduces the chance they drift out of sync.
When RestartPolicy=RecreateRoleInstanceOnPodRestart triggers a full RoleInstance recreation on pod crash, add configurable exponential backoff delay to prevent infinite rapid restart loops. API changes: - Add BaseDelaySeconds, MaxDelaySeconds fields to LeaderWorkerPattern and CustomComponentsPattern (RBG spec) - Add corresponding fields to RoleInstanceSpec for propagation - Add RestartCount and LastRestartTime to RoleInstanceStatus for tracking restart history Controller changes: - checkRestartBackoff: computes delay = min(base * 2^count, max) and requeues if delay not elapsed since LastRestartTime - calculateRestartDelay: exponential backoff formula with overflow protection (following client-go workqueue convention) - updateRestartTracking: increments RestartCount and sets LastRestartTime when recreation is triggered - Scale() interface updated to return requeue duration Defaults: baseDelaySeconds=30, maxDelaySeconds=600 Signed-off-by: Claude Opus 4.6 <noreply@anthropic.com>
Syspretor
force-pushed
the
feature/restart-policy-backoff
branch
from
0b0a479 to
f843b25
Compare
Add comprehensive test coverage for the restart backoff feature: - Envtest: RestartCount/LastRestartTime tracking, backoff delay verification, config propagation (RBG → RoleInstance), RestartCount reset after stable period - E2E: backoff delays second recreation with real pod lifecycle, config propagation verification - Refactored e2e restart_policy_stability.go into separate functions to reduce cyclomatic complexity - Added WithBaseDelaySeconds/WithMaxDelaySeconds to LeaderWorkerRoleWrapper Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
/copilot review |
|
@gemini-code-assist /review |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Both previous blocking issues resolved (int32 overflow fixed with int64 arithmetic, e2e tests for backoff added). Implementation is solid. The unrelated CI failure in |
| // Following client-go's workqueue.ItemExponentialFailureRateLimiter convention. | ||
| // When maxDelaySeconds is 0, no cap is applied (unbounded backoff). | ||
| func calculateRestartDelay(baseDelaySeconds, maxDelaySeconds, restartCount int32) int32 { | ||
| if baseDelaySeconds == 0 { |
There was a problem hiding this comment.
nit (non-blocking): With restartCount >= 63, int64(1) << restartCount wraps to math.MinInt64, and the multiplication overflows int64 -- bypassing both cap checks. Practically unreachable with default maxDelay=600 (cap triggers at restartCount=5) plus the reset mechanism, but a one-liner if restartCount >= 62 { restartCount = 62 } before the shift would seal it completely.
The Consistently check in runRestartBackoffDelayTest failed immediately because the Restarting condition persisted in RoleInstance status during the creation phase (scaling=true), causing checkRestartBackoff to skip the backoff delay on the second crash. Two fixes: 1. E2E test: add waitForInstanceFullyRecovered() that waits for RoleInstance Ready=True AND Restarting condition cleared before triggering the second crash. 2. Production: use in-memory restarting cache (isAlreadyRestarting) instead of persisted status (isInstanceRestarting) in checkRestartBackoff for more accurate restart-in-progress detection. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ApplyRevision produces zero-value status fields for updateInstance. When scaling=true during the pod creation phase (not crash-triggered deletion), updateRestartTracking is NOT called, so updateInstance.Status retains zero values for RestartCount and LastRestartTime. The unconditional propagation from updateInstance to newStatus would overwrite the correct values (loaded from the live object at reconcile start), resetting RestartCount to 0 on every pod creation cycle. This caused both envtest and e2e tests to fail because: 1. First crash: RestartCount=1 correctly persisted during deletion phase 2. Creation phase: unconditional propagation overwrote it to 0 3. Subsequent crash: checkRestartBackoff saw LastRestartTime=nil → no backoff Fix: only propagate RestartCount/LastRestartTime from updateInstance when they were actually modified (non-zero / non-nil), i.e., when updateRestartTracking was called during the deletion phase. Also revert checkRestartBackoff from isAlreadyRestarting back to isInstanceRestarting (the in-memory cache has the same staleness issue) and remove debug logging. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Two bugs were causing e2e test failures: 1. During backoff requeue (scaleRequeue > 0), the Restarting condition was propagated from updateInstance to newStatus. This caused the Restarting condition to be persisted alongside Ready=False (since pods are Failed). On the next reconcile after backoff elapses, shouldRecreateInstance checks wasInstanceReady(), which returns false when Ready=False, preventing recreation from ever proceeding — the instance is stuck indefinitely. Fix: do not propagate the Restarting condition during backoff requeue. The condition should only be set when a restart is actually in progress (scaling=true for deletion/creation), not while waiting for backoff. 2. The e2e Consistently check used filterActivePods() which excludes Failed pods. During backoff, the original pods are Failed (not "active"), so filterActivePods returns empty and the check fails immediately. Fix: check that no pod has a UID different from the originals — any new UID means recreation happened, which is the correct semantic for backoff. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
/copilot review |
|
@gemini-code-assist /review |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
| // was triggered). ApplyRevision produces zero-value status fields, so | ||
| // unconditional propagation would clobber the correct values from the | ||
| // live object during the pod creation phase. | ||
| if updateInstance.Status.RestartCount > 0 { |
There was a problem hiding this comment.
This push introduces the conditional propagation if updateInstance.Status.RestartCount > 0 { newStatus.RestartCount = updateInstance.Status.RestartCount } and the matching guard for LastRestartTime. It only fires while scaling == true, which is only true on the reconcile where updateRestartTracking actually incremented the counter. On the very next reconcile (pods already deleted / being recreated, or Ready again) scaling is false, so we skip the block entirely and UpdateInstanceStatus writes the freshly-initialized newStatus.RestartCount = 0 / LastRestartTime = nil back to the API server. The counter is effectively zeroed one reconcile after each restart, which breaks backoff exactly when it needs to kick in.
CI on this SHA reflects the regression:
- envtest
TestRestartPolicy– three newExponential Backoffcases time out at 60s:should track RestartCount and LastRestartTime after recreation(line 867)should delay second recreation due to backoff(line 930)should reset RestartCount after stable period(line 1060)
- e2e
restart policy stability–RecreateRoleInstanceOnPodRestart with backoff delays second recreationfails after ~1s at line 345 (pod gets recreated during the backoff window). Repeats deterministically in bothe2e-testande2e-test-manifestmatrix jobs.
These are the tests this PR itself added, so the failure is a direct signal that the new propagation path is losing state, not that the tests are flaky. The right fix is likely to either propagate RestartCount / LastRestartTime unconditionally from the live instance (or from updateInstance after ApplyRevision) even when scaling == false, or to seed newStatus from the current instance.Status for these two fields before Scale runs. Please restore persistence and re-run envtest+e2e locally before pushing again.
| elapsed := time.Since(instance.Status.LastRestartTime.Time) | ||
| remaining := time.Duration(delay)*time.Second - elapsed | ||
| if remaining > 0 { | ||
| klog.Infof("Restart backoff: instance %s waiting %v (restartCount=%d, delay=%ds)", |
There was a problem hiding this comment.
The backoff-waiting message is now logged at Info instead of V(2). For a role instance in a genuine crash loop this reconciles on every backoff tick, so the operator log will get one line per instance per requeue. Not a correctness issue, but worth double-checking whether Info is the right level here — V(1) or V(2) is closer to the surrounding conventions and avoids drowning out other events during long backoff windows.
| return true | ||
| } | ||
| // Slow path: read fresh from API server (bypasses informer cache) | ||
| if c.apiReader == nil { |
There was a problem hiding this comment.
The added if c.apiReader == nil { return false } guard is a sensible defensive check and matches the pattern used elsewhere for unit-testable controls. No action needed.
checkRestartBackoff now reads RestartCount and LastRestartTime from the API server directly, preventing stale informer data from causing backoff to be skipped. Additionally, updateStatus preserves live restart tracking values to prevent a stale-cache reconcile from rolling back a RestartCount that was already incremented by a prior reconcile. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…rt backoff Three informer-cache staleness issues were causing backoff and recreation to fail: 1. checkRestartBackoff read the Restarting condition from the stale informer cache. A stale Restarting=True (from a previous restart that completed) incorrectly skipped backoff. Now reads fresh from API server. 2. shouldRecreateInstance checked wasInstanceReady() which returned false after the backoff status update set Ready=False. Added a bypass when LastRestartTime is set (proving the instance was previously stable). 3. calculateDiffsWithExpectation read LastRestartTime from the stale informer cache. When nil, the wasInstanceReady bypass didn't trigger. Now reads fresh from API server when the informer shows LastRestartTime=nil. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds configurable exponential backoff to the existing RecreateRoleInstanceOnPodRestart restart policy by persisting restart history (RestartCount, LastRestartTime) on RoleInstanceStatus, propagating delay settings from higher-level specs, and requeueing reconciles when the backoff window hasn’t elapsed.
Changes:
- Introduces
baseDelaySeconds/maxDelaySecondsAPI fields (LeaderWorker/CustomComponents patterns + RoleInstance spec) and propagates them into RoleInstances. - Implements restart backoff logic in the RoleInstance scale path, including status tracking of restart history.
- Expands envtest + e2e coverage for restart backoff behavior and propagation.
Reviewed changes
Copilot reviewed 16 out of 23 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/wrappers/v1alpha2/role_wrapper.go | Adds wrapper helpers to set baseDelaySeconds / maxDelaySeconds in tests. |
| test/stress/scenario.go | Formatting/alignment changes to the stress Scenario struct fields. |
| test/envtest/testcase/restart_policy/restart_policy_test.go | Adds envtest integration cases for backoff tracking, delay, propagation, and reset behavior. |
| test/e2e/testcase/v1alpha2/restart_policy_stability.go | Refactors restart policy e2e tests into helpers and adds backoff-focused e2e cases. |
| pkg/reconciler/roleinstanceset_reconciler.go | Propagates restart policy delay settings into RoleInstance templates via applyconfig. |
| pkg/reconciler/roleinstance/sync/instance_scale.go | Core implementation: backoff requeue path, restart tracking updates, and delay calculation. |
| pkg/reconciler/roleinstance/sync/instance_scale_test.go | Adds unit tests for delay calculation and backoff decision logic. |
| pkg/reconciler/roleinstance/sync/api.go | Updates sync interface to return an additional requeueAfter duration from Scale. |
| pkg/reconciler/roleinstance/instance_status.go | Ensures status updates treat restart tracking as part of inconsistency detection and avoid clobbering newer values. |
| pkg/reconciler/roleinstance/instance_reconciler.go | Wires Scale requeue duration into reconcile flow; propagates restart tracking during active scaling. |
| deploy/kubectl/manifests.yaml | Updates generated manifests to include new API fields and status fields. |
| config/crd/bases/workloads.x-k8s.io_roleinstances.yaml | Adds RoleInstance CRD schema for delay fields + restart tracking status fields. |
| config/crd/bases/workloads.x-k8s.io_rolebasedgroupsets.yaml | Adds delay field schema to RBGSets patterns in the CRD. |
| config/crd/bases/workloads.x-k8s.io_rolebasedgroups.yaml | Adds delay field schema to RBG patterns in the CRD. |
| client-go/applyconfiguration/workloads/v1alpha2/roleinstancetemplate.go | Adds applyconfig setters for RoleInstance template delay fields. |
| client-go/applyconfiguration/workloads/v1alpha2/roleinstancestatus.go | Adds applyconfig support for restartCount and lastRestartTime status fields. |
| client-go/applyconfiguration/workloads/v1alpha2/roleinstancespec.go | Adds applyconfig setters for RoleInstance spec delay fields. |
| client-go/applyconfiguration/workloads/v1alpha2/leaderworkerpattern.go | Adds applyconfig setters for pattern delay fields. |
| client-go/applyconfiguration/workloads/v1alpha2/customcomponentspattern.go | Adds applyconfig setters for pattern delay fields. |
| api/workloads/v1alpha2/zz_generated.deepcopy.go | Updates deep-copies to include new delay fields and LastRestartTime. |
| api/workloads/v1alpha2/roleinstance_types.go | Adds RoleInstance spec delay fields + status restart tracking, plus getters with defaults. |
| api/workloads/v1alpha2/rolebasedgroup_types.go | Adds delay configuration fields to LeaderWorker/CustomComponents patterns with defaults. |
| api/workloads/v1alpha2/helper.go | Introduces default delay constants + RoleSpec helper getters for delay settings. |
Files not reviewed (6)
- api/workloads/v1alpha2/zz_generated.deepcopy.go: Generated file
- client-go/applyconfiguration/workloads/v1alpha2/customcomponentspattern.go: Generated file
- client-go/applyconfiguration/workloads/v1alpha2/leaderworkerpattern.go: Generated file
- client-go/applyconfiguration/workloads/v1alpha2/roleinstancespec.go: Generated file
- client-go/applyconfiguration/workloads/v1alpha2/roleinstancestatus.go: Generated file
- client-go/applyconfiguration/workloads/v1alpha2/roleinstancetemplate.go: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func calculateRestartDelay(baseDelaySeconds, maxDelaySeconds, restartCount int32) int32 { | ||
| if baseDelaySeconds == 0 { | ||
| return 0 | ||
| } | ||
| // Use int64 arithmetic to prevent overflow for large restart counts. | ||
| // Example: baseDelay=30, restartCount=27 → 30 * 2^27 = 4B > int32 max. | ||
| delay := int64(baseDelaySeconds) * (int64(1) << restartCount) | ||
| // Apply maxDelay cap if configured | ||
| if maxDelaySeconds > 0 && delay > int64(maxDelaySeconds) { | ||
| return maxDelaySeconds | ||
| } | ||
| // If no cap and delay exceeds int32, cap at int32 max to prevent overflow | ||
| if delay > int64(0x7FFFFFFF) { | ||
| return 0x7FFFFFFF | ||
| } | ||
| return int32(delay) | ||
| } |
| // No previous restart tracking from informer — check fresh data below | ||
| if instance.Status.LastRestartTime == nil { | ||
| // Also check fresh from API server below | ||
| } |
| if lastRestartTime == nil { | ||
| return 0 | ||
| } |
There was a problem hiding this comment.
Thanks for the PR — the direction is right, and introducing backoff addresses the "instant furious restart" problem. Nice touches too: the int64 overflow guard and following client-go's ItemExponentialFailureRateLimiter convention.
Against issue #363, I left a few inline suggestions below. Three that don't map to a specific line:
- No durable crash forensics for the unattended case: preserving the pod during the backoff window is great for live
kubectl logs -p, but once the window closes the evidence is gone. Consider recording podName/node/container/exitCode/reason/finishedAt into status (e.g.LastCrashInfo) before the rebuild, so a 3am crash can still be diagnosed the next morning. - Consider grouping the API fields:
baseDelaySeconds/maxDelaySecondsare flat on both patterns; if more recovery-related knobs get added later, aRestartPolicyConfigsub-struct would be easier to extend and wouldn't touch theRestartPolicyfield type. - The defaults change recovery behavior:
base=30/max=600turns "rebuild immediately on crash" into "wait 30s first, escalating to 10min". Worth calling this default change out in the PR description, or consider a more conservative default (e.g. backoff off by default / opt-in).
Nice starting point overall.
| // meaning the instance WAS stable before. The status update during backoff may have set | ||
| // Ready=False (because pods are Failed), so wasInstanceReady() returns false. We bypass | ||
| // this check to allow the recreation to proceed after backoff expires. | ||
| if (!wasInstanceReady(instance) && instance.Status.LastRestartTime == nil) || |
There was a problem hiding this comment.
🔴 This weakens the Ready invariant of shouldRecreateInstance and may regress.
Once LastRestartTime has been set even once, the instance will trigger a rebuild even if it was never Ready. For example, a pod that keeps crashing during init and never reaches Ready would keep satisfying the recreate condition.
Suggest not using LastRestartTime != nil to permanently bypass the Ready gate. Instead, use an explicit "recovering" state (the Restarting condition): relax the Ready check only while that state is active, and clear it once recovery completes.
| restarting := isInstanceRestarting(instance) | ||
| if c.apiReader != nil { | ||
| fresh := &workloadsv1alpha2.RoleInstance{} | ||
| if err := c.apiReader.Get(ctx, client.ObjectKeyFromObject(instance), fresh); err == nil { |
There was a problem hiding this comment.
🔴 State model doesn't fit the reconcile loop well: several apiReader.Get direct reads.
Here (and at lines 112, 485) you read straight from the API server to work around the informer cache, potentially multiple times per reconcile. That's usually a sign the state has more than one source of truth, and it adds apiserver load.
Suggest anchoring the backoff on the Restarting condition's LastTransitionTime (naturally visible within reconcile), combined with the in-memory restartingCache for informer lag, so you don't need a fresh read every round.
| } | ||
| // Use int64 arithmetic to prevent overflow for large restart counts. | ||
| // Example: baseDelay=30, restartCount=27 → 30 * 2^27 = 4B > int32 max. | ||
| delay := int64(baseDelaySeconds) * (int64(1) << restartCount) |
There was a problem hiding this comment.
🟡 Overflow at very large restartCount.
If restartCount grows to 63+: int64(1) << 63 goes negative and << 64 becomes 0, so the computed delay turns negative or zero → backoff silently disappears.
Suggest moving the comparison before the shift: if restartCount is already large enough that base << count must exceed max, return max directly; or cap the shift count (e.g. via bits.Len).
| if stableThreshold < 10*time.Minute { | ||
| stableThreshold = 10 * time.Minute | ||
| } | ||
| if time.Since(instance.Status.LastRestartTime.Time) > stableThreshold { |
There was a problem hiding this comment.
🟡 The RestartCount reset semantics are a bit fragile.
This only resets "when the next restart happens", based on "time since the last restart", rather than "the instance has stayed Ready". A crashloop where each rebuild takes longer than maxDelay*2 before crashing again would get reset every round.
Suggest gating the reset on wasInstanceReady plus a sustained stability window (with hysteresis), so a flapping recovery can't keep zeroing the counter.
| return 0 | ||
| } | ||
| // No previous restart tracking from informer — check fresh data below | ||
| if instance.Status.LastRestartTime == nil { |
There was a problem hiding this comment.
⚪ Empty dead code.
This if body is empty with only a comment — no actual behavior. Suggest removing it, or filling in the intended fresh-read logic.
Summary
Implements the simplified restart policy enhancement from #380:
RestartPolicy=RecreateRoleInstanceOnPodRestarttriggers, all pods (including the crashed one) are deleted and recreated — unchanged.baseDelaySecondsandmaxDelaySecondscontrol the pacing between restart attempts. The delay grows asmin(base * 2^restartCount, max), preventing infinite rapid restart loops.RestartCountandLastRestartTimeinRoleInstanceStatuspersist across reconciles to compute backoff.New API fields
baseDelaySecondsmaxDelaySecondsExample
Backoff progression (defaults)
Test plan
calculateRestartDelay(exponential formula, overflow protection, cap)checkRestartBackoff(no backoff on first restart, delay elapsed/not elapsed, RestartPolicy=None)make test)make lint)Related
🤖 Generated with Claude Code