Commit Graph
10 Commits
Author SHA1 Message Date
“Naeel” 919e84396c fix(reconciler): propagate SA/executor errors to NamespaceManager so failed NSes are retried
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/...
2026-05-18 11:10:46 +04:00
Naeel 2857398e11 layer1: checkpoint pending utils changes 2026-04-26 10:20:56 +03:00
Naeel 9ce9829f3b layer1: fix sa namespace routing step 2 2026-04-26 09:32:08 +03:00
Naeel 161de70576 multi-tenant: EnsureNamespaceSA + ns_watcher SA provisioning (v8) 2026-04-26 07:41:46 +03:00
Sanket SudakeandGitHub 59267e3a6b Add path safety fixes (#3061)
* Add path safety fixes

Signed-off-by: Sanket Sudake <sanketsudake@gmail.com>

* minor changes

Signed-off-by: Sanket Sudake <sanketsudake@gmail.com>

* revert test_huge_response test

Signed-off-by: Sanket Sudake <sanketsudake@gmail.com>

---------

Signed-off-by: Sanket Sudake <sanketsudake@gmail.com>
2024-11-18 12:19:34 +05:30
Sanket SudakeandGitHub c6329ee3db Switch to google/uuid from satori/go.uuid (#2852)
Signed-off-by: Sanket Sudake <sanketsudake@gmail.com>
2023-10-17 12:19:17 +05:30
Shubham BansalandGitHub 5db09a899a Create roles for custom resource in function and builder namespace (#2695)
* create roles for custom resource in function and builder namespace
* Create roles for CR for builder in function and builder namespace
* convert warn to info to remove error stack trace
2023-01-17 22:15:04 +05:30
Sanket SudakeandGitHub 8df4fd0e7c Allow service account check to run only once at start of executor (#2673)
Signed-off-by: Sanket Sudake <sanketsudake@gmail.com>

Signed-off-by: Sanket Sudake <sanketsudake@gmail.com>
2022-12-15 15:04:10 +05:30
Shubham BansalandGitHub 3e25f474b0 Remove singleDefaultNamespace field from helm chart (#2670)
* remove singleDefaultNamespace field from helm chart

* changed permission from list to get
2022-12-14 14:17:34 +05:30
Shubham BansalandGitHub 16cbb87eab Create role/role binding/service account required for builder/function pods (#2667)
* service account changes for fission-builder
* moved code into executor
* changed default timing to 30 min
* code refactor and add new roles for executor
* create role if serviceAccountCheck is enabled
2022-12-14 09:06:20 +05:30