fix: harden topology sync against transient API server errors at startup

[AsadShahid04]

Summary

• Add SynchronizeTopologyWithRetry wrapping SynchronizeTopology with bounded exponential backoff
• Permanent errors (Forbidden, Unauthorized) propagate immediately without retrying
• main.go updated to call the retry wrapper with a sensible default backoff (~63 s budget)

Problem

clustertopology.SynchronizeTopology is called from main.go before mgr.Start(). Any single transient error — a 5xx from a rolling API server, a webhook pod not yet ready, a brief etcd hiccup — causes the operator to exit via cli.ExitErrSynchronizeTopology. The only recovery path is kubelet restarting the pod, amplifying load on a control plane that is already under stress.

See #654 for the full failure-mode analysis.

Solution

Introduce SynchronizeTopologyWithRetry(ctx, cl, logger, backends, backoff) in internal/clustertopology/clustertopology.go:

• Uses k8s.io/client-go/util/retry.OnError with wait.Backoff (already a transitive dependency).
• isTransientAPIError classifies 5xx, service-unavailable, server-timeout, request-timeout, and 429 as retriable.
• Forbidden and Unauthorized (permanent RBAC/auth failures) are returned immediately — no retrying.
• Default backoff: 1 s initial, factor 2.0, 10% jitter, 6 steps ≈ 63 s total budget.
• Each retry logs "Retrying topology synchronization after transient error" with the attempt number.

main.go is updated to call SynchronizeTopologyWithRetry with DefaultSyncRetryBackoff.

Testing

• go build ./...: passed
• Unit tests: 3 new tests added
  • TestSynchronizeTopologyWithRetry_SucceedsAfterTransientError — asserts success after 2 injected 500 errors
  • TestSynchronizeTopologyWithRetry_PermanentErrorNotRetried — asserts Forbidden stops immediately (1 call only)
  • TestSynchronizeTopologyWithRetry_ExhaustsRetriesOnPersistentTransientError — asserts last transient error returned after budget exhausted
• All existing TestSynchronizeTopology* tests continue to pass (no behaviour change to the base function)
• Full non-e2e suite: all packages pass

Closes #654

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[yankay]

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.

[yankay]

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?

[AsadShahid04]

Pushed fc5b7be addressing your feedback:

Transport-level error coverage — Extended isTransientAPIError to also match:

• net.Error with Timeout() == true (dial/read timeouts during API-server rolling restart)
• io.EOF (connection dropped mid-request)
• syscall.ECONNREFUSED (API server not yet accepting connections)

These all surface before the client wraps the error as a StatusError, so every apierrors.IsXxx() would return false on them and the operator would exit without retrying. All three error kinds are now caught and retried.

Added TestIsTransientAPIError (table-driven, 9 cases) and TestSynchronizeTopologyWithRetry_TransportErrorRetried to pin this behavior.

Retry orchestration location — I'll leave SynchronizeTopologyWithRetry in clustertopology for now; happy to move it to a startup helper in a follow-up if that direction is preferred.

[Ronkahn21]

This Retry functionality will be useful in other places
I would like to make it more genric rather of hand coded wrapping each function

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())
})