-
Notifications
You must be signed in to change notification settings - Fork 77
fix: harden topology sync against transient API server errors at startup #669
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,15 +18,68 @@ package clustertopology | |
|
|
||
| import ( | ||
| "context" | ||
| "errors" | ||
| "fmt" | ||
| "io" | ||
| "net" | ||
| "syscall" | ||
| "time" | ||
|
|
||
| grovecorev1alpha1 "github.com/ai-dynamo/grove/operator/api/core/v1alpha1" | ||
| "github.com/ai-dynamo/grove/operator/internal/scheduler" | ||
|
|
||
| "github.com/go-logr/logr" | ||
| apierrors "k8s.io/apimachinery/pkg/api/errors" | ||
| "k8s.io/apimachinery/pkg/util/wait" | ||
| "k8s.io/client-go/util/retry" | ||
| "sigs.k8s.io/controller-runtime/pkg/client" | ||
| ) | ||
|
|
||
| // DefaultSyncRetryBackoff is the retry backoff used by SynchronizeTopologyWithRetry. | ||
| // Six steps with doubling delay gives roughly 60 seconds of retry budget (1+2+4+8+16+32). | ||
| var DefaultSyncRetryBackoff = wait.Backoff{ | ||
| Duration: time.Second, | ||
| Factor: 2.0, | ||
| Jitter: 0.1, | ||
| Steps: 6, | ||
| } | ||
|
|
||
| // SynchronizeTopologyWithRetry wraps SynchronizeTopology with a bounded exponential | ||
| // backoff so that transient API-server errors (5xx, timeouts, service-unavailable) | ||
| // at operator startup do not crash the operator process. | ||
| // Permanent errors (Forbidden, Unauthorized) are returned immediately without retrying. | ||
| func SynchronizeTopologyWithRetry(ctx context.Context, cl client.Client, logger logr.Logger, backends map[string]scheduler.TopologyAwareBackend, backoff wait.Backoff) error { | ||
|
Comment on lines
+50
to
+51
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This Retry functionality will be useful in other places I would go with something more like // Retrier holds retry configuration. Create one per logical operation or share
// a single instance across all calls with the same policy.
type Retrier struct {
backoff wait.Backoff
isRetryable func(error) bool
onRetry OnRetryFunc
}
// New creates a Retrier. Pass nil for onRetry to skip logging.
func New(backoff wait.Backoff, isRetryable func(error) bool, onRetry OnRetryFunc) *Retrier {
return &Retrier{backoff: backoff, isRetryable: isRetryable, onRetry: onRetry}
}
// Do retries fn using the configured backoff. Use a closure to capture any
// number of arguments — including ctx when it doesn't change between retries:
//
// err := r.Do(func() error {
// return SynchronizeTopology(ctx, cl, logger, backends)
// })
func (r *Retrier) Do(fn func() error) error {
attempt := 0
var lastErr error
return retry.OnError(r.backoff, r.isRetryable, func() error {
if attempt > 0 && r.onRetry != nil {
r.onRetry(attempt, lastErr)
}
attempt++
lastErr = fn()
return lastErr
})
}
----------
var topoRetrier = k8sretry.New(
clustertopology.DefaultSyncRetryBackoff,
k8sErrors.IsTransientAPIError,
func(n int, _ error) {
logger.Info("Retrying topology synchronization", "attempt", n+1)
},
)
err = topoRetrier.Do(func() error {
return clustertopology.SynchronizeTopology(ctx, cl, logger, schedRegistry.AllTopologyAware())
}) |
||
| attempt := 0 | ||
| return retry.OnError(backoff, isTransientAPIError, func() error { | ||
| if attempt > 0 { | ||
| logger.Info("Retrying topology synchronization after transient error", "attempt", attempt+1) | ||
| } | ||
| attempt++ | ||
| return SynchronizeTopology(ctx, cl, logger, backends) | ||
| }) | ||
| } | ||
|
|
||
| // isTransientAPIError reports whether err is a transient error that is safe to retry. | ||
| // It covers both Kubernetes API-level errors (5xx, timeouts) and transport-level | ||
| // errors (connection refused, EOF, net timeout) that surface before a StatusError | ||
| // is returned — e.g. during an API-server rolling restart or network partition. | ||
| // Permanent errors (Forbidden, Unauthorized) return false. | ||
| func isTransientAPIError(err error) bool { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice work on this One thing on the transient classification: it currently only matches Kubernetes StatusErrors. But several of the recoverable startup cases in #654 — API server rolling restart, network partition, connection timeout — surface from the Go client as transport-level errors (e.g. connection refused, EOF, TLS handshake timeout, i/o timeout) rather than StatusErrors. Those go through reasonAndCodeForError as (Unknown, 0), so every apierrors.IsXxx returns false and the operator would still exit. Could we extend the predicate to cover transport-level transient errors (net.Error timeout, ECONNREFUSED, io.EOF) and add a unit test for at least one of them? |
||
| if apierrors.IsServerTimeout(err) || | ||
| apierrors.IsServiceUnavailable(err) || | ||
| apierrors.IsInternalError(err) || | ||
| apierrors.IsTimeout(err) || | ||
| apierrors.IsTooManyRequests(err) { | ||
| return true | ||
| } | ||
| // Transport-level transient errors. | ||
| var netErr net.Error | ||
| if errors.As(err, &netErr) && netErr.Timeout() { | ||
| return true | ||
| } | ||
| return errors.Is(err, io.EOF) || errors.Is(err, syscall.ECONNREFUSED) | ||
| } | ||
|
|
||
| // SynchronizeTopology synchronizes scheduler-specific topology resources at operator startup. | ||
| // Lists all existing ClusterTopologyBinding resources and ensures backend topologies exist for each. | ||
| // Called before controllers start to avoid races with PCS reconciliation. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HI @AsadShahid04
Nice PR.
(lower priority / optional): SynchronizeTopologyWithRetry puts startup retry/backoff/classification into the clustertopology package, which is otherwise focused on the sync itself. Would it make sense to keep the retry orchestration in main.go or a small startup helper, ideally with a context-aware backoff so it cancels cleanly on SIGTERM? Happy either way — just a thought.