From 919e84396c283386fb932d5b7e346dc461a1f311 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=80=9CNaeel=E2=80=9D?= Date: Mon, 18 May 2026 11:10:46 +0400 Subject: [PATCH] fix(reconciler): propagate SA/executor errors to NamespaceManager so failed NSes are retried MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Problem ------- The namespace reconciler (RunReconciler, added previously) retries namespaces in NamespacePhaseFailed every 30s by calling DispatchResync. But the phase could never actually reach NamespacePhaseFailed for the executor component because the executor's NamespaceSubscriber always returned nil — swallowing any SA-provisioning or informer-init errors. The reconciler was dead code for the executor path. Root cause chain ---------------- 1. setupSAAndRoleBindings() — void, errors only logged internally. 2. EnsureNamespaceSA() — void, just called setupSAAndRoleBindings. 3. registerNamespace() — void, errors from both functions lost. 4. Executor AddFunc/ResyncFunc — always returned nil to dispatch(). 5. dispatch() marks parts Active unconditionally → NamespacePhaseFailed is never triggered for executor → RunReconciler never fires for executor. Consequence: if EnsureNamespaceSA failed (transient k8s 503, RBAC webhook timeout, etc.) the namespace appeared Active in the manager but the fetcher ServiceAccount was missing. Pool pods would CrashLoopBackOff on every call to that namespace until a full process restart. Changes ------- pkg/utils/serviceaccount.go - setupSAAndRoleBindings: void → error. Returns the first k8s API error so callers can decide whether to retry. - runSACheck: ignores the error with _ = (same behaviour as before, it's a periodic background loop that already logs internally). - EnsureNamespaceSA: void → error, propagates setupSAAndRoleBindings. Updated godoc to explain the retry contract. pkg/executor/multitenant/ns_watcher.go - registerNamespace: void → error. * EnsureNamespaceSA error → wrapped as 'EnsureNamespaceSA: ...' and returned. * registerExecutorTypes error → wrapped as 'registerExecutorTypes: ...' and returned. * Success log line only emitted when both succeed. - Added 'fmt' import for error wrapping. pkg/executor/multitenant/namespace_subscriber.go - AddFunc: return registerNamespace(...) instead of ignoring its error. - ResyncFunc: same — plus a comment explaining why it is safe to call registerNamespace again (SA creation is idempotent, executor-type AddNamespace guards against duplicate informer creation). pkg/utils/namespace_manager.go - RunReconciler interface signature: added *zap.Logger parameter. Callers pass the component logger so retries are visible in prod logs. - RunReconciler implementation: * Accepts logger; falls back to zap.NewNop() if nil. * Skips the tick entirely when no failed namespaces are found (no log spam). * Logs 'retrying failed namespaces' with count + list when found. * Logs per-namespace 'dispatching resync'. * Logs 'resync succeeded' or 'resync still failing, will retry' with error. - RunManagedNamespaceWatcher: passes logger to RunReconciler. End-to-end flow after this fix ------------------------------- 1. EnsureNamespaceSA fails (k8s 503). 2. registerNamespace returns error. 3. Executor AddFunc returns error. 4. dispatch() calls MarkPartFailed("executor") → deriveNamespacePhase → NamespacePhaseFailed. 5. RunReconciler tick (30s) finds the namespace → DispatchResync → registerNamespace called again → EnsureNamespaceSA (idempotent) → if API recovered: success → MarkPartActive → NamespacePhaseActive. 6. Log line 'namespace reconciler: resync succeeded' confirms recovery. Backward compatibility ---------------------- - NamespaceManager interface: RunReconciler gained a *zap.Logger param. There is exactly one implementation (inMemoryNamespaceManager) and one call site (RunManagedNamespaceWatcher). No external mocks. - EnsureNamespaceSA: callers outside this codebase (if any) that ignore the error will still compile (Go allows ignoring return values). - All 26 affected tests pass: go test ./pkg/utils/... ./pkg/executor/... ./pkg/buildermgr/... ./pkg/router/... --- .../multitenant/namespace_subscriber.go | 9 ++--- pkg/executor/multitenant/ns_watcher.go | 18 ++++++++-- pkg/utils/namespace_manager.go | 35 ++++++++++++++----- pkg/utils/serviceaccount.go | 18 ++++++---- 4 files changed, 58 insertions(+), 22 deletions(-) diff --git a/pkg/executor/multitenant/namespace_subscriber.go b/pkg/executor/multitenant/namespace_subscriber.go index 087f7612..d57d023f 100644 --- a/pkg/executor/multitenant/namespace_subscriber.go +++ b/pkg/executor/multitenant/namespace_subscriber.go @@ -21,15 +21,16 @@ func NewNamespaceSubscriber( return utils.NamespaceSubscriberFuncs{ SubscriberName: "executor", AddFunc: func(ctx context.Context, record utils.NamespaceRecord) error { - registerNamespace(ctx, logger, kubernetesClient, record.Name, executorTypes, mgr) - return nil + return registerNamespace(ctx, logger, kubernetesClient, record.Name, executorTypes, mgr) }, RemoveFunc: func(ctx context.Context, record utils.NamespaceRecord) error { return deregisterNamespace(ctx, logger, record.Name, executorTypes) }, ResyncFunc: func(ctx context.Context, record utils.NamespaceRecord) error { - registerNamespace(ctx, logger, kubernetesClient, record.Name, executorTypes, mgr) - return nil + // Reconciler calls this for namespaces in NamespacePhaseFailed. + // registerNamespace is idempotent: SA creation is a no-op if SA exists, + // executor type AddNamespace guards against duplicate informer creation. + return registerNamespace(ctx, logger, kubernetesClient, record.Name, executorTypes, mgr) }, } } diff --git a/pkg/executor/multitenant/ns_watcher.go b/pkg/executor/multitenant/ns_watcher.go index 70e68a82..b1544ede 100644 --- a/pkg/executor/multitenant/ns_watcher.go +++ b/pkg/executor/multitenant/ns_watcher.go @@ -62,6 +62,7 @@ package multitenant import ( "context" "errors" + "fmt" "go.uber.org/zap" "k8s.io/client-go/kubernetes" @@ -100,6 +101,10 @@ func StartNSWatcher( // Each executor type uses its own internal state for deduplication instead of the // global resolver, so all executor types receive the AddNamespace call regardless of // iteration order. +// +// Returns an error if SA provisioning or any executor-type initialization fails. +// The error is propagated to the NamespaceSubscriber so the NamespaceManager can +// mark the namespace as NamespacePhaseFailed and the reconciler will retry automatically. func registerNamespace( ctx context.Context, logger *zap.Logger, @@ -107,14 +112,21 @@ func registerNamespace( ns string, executorTypes map[fv1.ExecutorType]executortype.ExecutorType, mgr manager.Interface, -) { +) error { // Update the global resolver once here. Each executor type must NOT call // DefaultNSResolver().AddNamespace() for dedup — they have their own checks. utils.DefaultNSResolver().AddNamespace(ns) // Ensure fission-fetcher SA exists in the new namespace so pool pods can start. - utils.EnsureNamespaceSA(ctx, kubernetesClient, logger, ns) - registerExecutorTypes(ctx, logger, ns, executorTypes, mgr) + // A failure here means function pods will crash (no SA to pull fetcher image) — + // propagate so the reconciler retries until the API is available again. + if err := utils.EnsureNamespaceSA(ctx, kubernetesClient, logger, ns); err != nil { + return fmt.Errorf("EnsureNamespaceSA: %w", err) + } + if err := registerExecutorTypes(ctx, logger, ns, executorTypes, mgr); err != nil { + return fmt.Errorf("registerExecutorTypes: %w", err) + } logger.Info("multitenant.NSWatcher: registered namespace", zap.String("namespace", ns)) + return nil } func registerExecutorTypes( diff --git a/pkg/utils/namespace_manager.go b/pkg/utils/namespace_manager.go index 73a59bfd..435bbbc3 100644 --- a/pkg/utils/namespace_manager.go +++ b/pkg/utils/namespace_manager.go @@ -76,7 +76,8 @@ type NamespaceManager interface { Remove(name string) bool // RunReconciler periodically retries namespaces stuck in NamespacePhaseFailed. // Must be started as a goroutine; exits when ctx is cancelled. - RunReconciler(ctx context.Context) + // logger is used to report retry attempts and outcomes; pass zap.NewNop() to silence. + RunReconciler(ctx context.Context, logger *zap.Logger) } type inMemoryNamespaceManager struct { @@ -148,7 +149,7 @@ func RunManagedNamespaceWatcher(ctx context.Context, logger *zap.Logger, kubeCli // Start reconciler: retries namespaces stuck in NamespacePhaseFailed every 30s. mgr.Add(ctx, func(ctx context.Context) { logger.Info(config.Component + ": namespace reconciler started") - manager.RunReconciler(ctx) + manager.RunReconciler(ctx, logger) logger.Info(config.Component + ": namespace reconciler stopped") }) LogNamespaceManagerSummary(logger, config.Component+": started namespace watcher", manager.Summary()) @@ -612,9 +613,14 @@ func (m *inMemoryNamespaceManager) Remove(name string) bool { } // RunReconciler periodically finds namespaces in NamespacePhaseFailed and retries them -// via DispatchResync. This ensures transient k8s API errors (e.g. temporary 503) do not -// permanently strand a namespace. Exits when ctx is cancelled. -func (m *inMemoryNamespaceManager) RunReconciler(ctx context.Context) { +// via DispatchResync. This ensures transient k8s API errors (e.g. temporary 503 on +// EnsureNamespaceSA or executor-type AddNamespace) do not permanently strand a namespace. +// logger receives one log line per retry attempt and per outcome. +// Exits when ctx is cancelled. +func (m *inMemoryNamespaceManager) RunReconciler(ctx context.Context, logger *zap.Logger) { + if logger == nil { + logger = zap.NewNop() + } ticker := time.NewTicker(30 * time.Second) defer ticker.Stop() for { @@ -630,10 +636,23 @@ func (m *inMemoryNamespaceManager) RunReconciler(ctx context.Context) { } } m.mu.RUnlock() + if len(failedNS) == 0 { + continue + } + logger.Info("namespace reconciler: retrying failed namespaces", + zap.Int("count", len(failedNS)), + zap.Strings("namespaces", failedNS), + ) for _, ns := range failedNS { - if _, _, err := m.DispatchResync(ctx, ns); err != nil { - // Still failing — will retry on next tick. - _ = err + logger.Info("namespace reconciler: dispatching resync", zap.String("namespace", ns)) + _, _, err := m.DispatchResync(ctx, ns) + if err != nil { + logger.Error("namespace reconciler: resync still failing, will retry", + zap.String("namespace", ns), + zap.Error(err), + ) + } else { + logger.Info("namespace reconciler: resync succeeded", zap.String("namespace", ns)) } } } diff --git a/pkg/utils/serviceaccount.go b/pkg/utils/serviceaccount.go index f4937e75..4caba9bd 100644 --- a/pkg/utils/serviceaccount.go +++ b/pkg/utils/serviceaccount.go @@ -121,7 +121,8 @@ func (sa *ServiceAccount) runSACheck(ctx context.Context) { for _, baseNS := range sa.nsResolver.Snapshot() { for _, permission := range sa.permissions { targetNS := sa.resolveSANamespace(baseNS, permission.saName) - setupSAAndRoleBindings(ctx, sa.kubernetesClient, sa.logger, targetNS, permission) + // Errors are already logged inside setupSAAndRoleBindings; periodic loop ignores them. + _ = setupSAAndRoleBindings(ctx, sa.kubernetesClient, sa.logger, targetNS, permission) } } } @@ -134,14 +135,14 @@ func (sa *ServiceAccount) resolveSANamespace(baseNS, saName string) string { return sa.nsResolver.GetFunctionNS(baseNS) } -func setupSAAndRoleBindings(ctx context.Context, client kubernetes.Interface, logger *zap.Logger, namespace string, ps *ServiceAccountPermissions) { +func setupSAAndRoleBindings(ctx context.Context, client kubernetes.Interface, logger *zap.Logger, namespace string, ps *ServiceAccountPermissions) error { SAObj, err := createGetSA(ctx, client, ps.saName, namespace) if err != nil { logger.Error("error while creating or getting service account", zap.String("sa_name", ps.saName), zap.String("namespace", namespace), zap.Error(err)) - return + return err } var rules []rbac.PolicyRule @@ -177,14 +178,15 @@ func setupSAAndRoleBindings(ctx context.Context, client kubernetes.Interface, lo role, err := setupRoles(ctx, client, logger, SAObj, rules, suffix) if err != nil { logger.Error("error while creating roles", zap.Error(err)) - return + return err } _, err = setupRoleBinding(ctx, client, logger, SAObj, role, suffix) if err != nil { logger.Error("error while creating role bindings", zap.Error(err)) - return + return err } } + return nil } func setupRoles(ctx context.Context, client kubernetes.Interface, logger *zap.Logger, sa *v1.ServiceAccount, rules []rbac.PolicyRule, suffix string) (*rbac.Role, error) { @@ -314,8 +316,10 @@ func getSAInterval() time.Duration { } // EnsureNamespaceSA creates the fission-fetcher ServiceAccount and its Role/RoleBinding +// in the given namespace. Returns an error if SA or RoleBinding creation fails so callers +// can propagate it to the namespace lifecycle manager and trigger a reconcile retry. // in the given namespace if they do not already exist. Safe to call repeatedly. // Used by the multi-tenant NS watcher to provision per-namespace SA on NS registration. -func EnsureNamespaceSA(ctx context.Context, client kubernetes.Interface, logger *zap.Logger, ns string) { - setupSAAndRoleBindings(ctx, client, logger, ns, fetcherCheck) +func EnsureNamespaceSA(ctx context.Context, client kubernetes.Interface, logger *zap.Logger, ns string) error { + return setupSAAndRoleBindings(ctx, client, logger, ns, fetcherCheck) }