From 695bfb74d4ad163ddba7adc99bd7c67cfc3bf021 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=80=9CNaeel=E2=80=9D?= Date: Mon, 18 May 2026 09:08:35 +0400 Subject: [PATCH] doc: impl notes for namespace lifecycle hardening (2026-05-18) --- ...5-18-namespace-lifecycle-hardening-impl.md | 567 ++++++++++++++++++ 1 file changed, 567 insertions(+) create mode 100644 doc/2026-05-18-namespace-lifecycle-hardening-impl.md diff --git a/doc/2026-05-18-namespace-lifecycle-hardening-impl.md b/doc/2026-05-18-namespace-lifecycle-hardening-impl.md new file mode 100644 index 00000000..cc22b3ac --- /dev/null +++ b/doc/2026-05-18-namespace-lifecycle-hardening-impl.md @@ -0,0 +1,567 @@ +# Namespace Lifecycle Hardening — Implementation Notes +## Дата: 2026-05-18 +## Ветка: `fix/namespace-lifecycle-hardening` +## Коммиты: `3b93c5dc` (предыдущая сессия) → `4eedf95f` (эта сессия) + +--- + +## 1. Контекст: что было сделано до этой сессии + +### Предыдущие правки (коммит `3b93c5dc`) +1. **`RemoveNamespace(ns string) bool`** — добавлен в `NamespaceResolver` (`pkg/utils/namespace.go`). + `HandleWatcherNamespaceRemoval` теперь вызывает его при любой стратегии, очищая глобальный resolver. Это исправляет дедупликацию router/buildermgr при re-add NS. + +2. **Параллельный `dispatch()`** — `pkg/utils/namespace_manager.go`: заменён последовательный обход подписчиков на параллельный с `sync.WaitGroup`. Исправляет 30-минутное окно, когда HTTPTrigger не видел namespace из-за того что обход был последовательным. + +3. **`RunReconciler()`** — добавлен в `NamespaceManager` interface и реализован в `inMemoryNamespaceManager`. Каждые 30 секунд сканирует namespace-ы в фазе `NamespacePhaseFailed` и вызывает `DispatchResync`. Исправляет постоянно stuck-failed namespace при транзиентных k8s API ошибках. + +### Что оставалось нерешённым (из аудита) +Три проблемы, зафиксированные в `FORENSIC_ARCHITECTURE_AUDIT.md`: + +**Проблема 1 — Executor dedup gap (Critical)** +При удалении NS и повторном добавлении executor молча пропускал его. +Причина: `gpm.poolPodC.envLister[ns]` и `deploy.deplLister[ns]` проверялись как дедупликация в `AddNamespace`, но никогда не очищались при удалении NS. +Результат: повторно добавленный namespace не получал informers в executor → функции не запускались. + +**Проблема 2 — Goroutine/FD leak (High)** +При удалении NS старые informer factories продолжали работать (goroutines, file descriptors, LIST-запросы к k8s API каждые 30 минут). +Причина: informers запускались с `ctx.Done()` родительского контекста всего процесса, без механизма per-NS остановки. + +**Проблема 3 — Router stale routes (Medium)** +После удаления NS router продолжал держать HTTPTrigger routes для этого namespace. +Причина: `triggerInformer[ns]` и `funcInformer[ns]` не чистились, `syncTriggers()` не вызывался. + +--- + +## 2. Анализ перед реализацией + +### 2.1 Чтение интерфейса ExecutorType +Файл: `pkg/executor/executortype/executortype.go` + +```go +// До правки — нет RemoveNamespace +AddNamespace(ctx context.Context, ns string, mgr manager.Interface) error +} +``` + +Подтверждено: ни `grep`, ни LSP не нашли `RemoveNamespace` в executor types. + +### 2.2 Анализ механизма дедупликации по каждому executor type + +**poolmgr** (`gpm.go` строка 807): +```go +if _, ok := gpm.poolPodC.envLister[ns]; ok { + return nil // already registered +} +``` +Деdup через `PoolPodController.envLister[ns]` — локальная карта, не связана с глобальным resolver. + +**newdeploy** (`newdeploymgr.go` строка 917): +```go +if _, ok := deploy.deplLister[ns]; ok { + return nil // already registered +} +``` +Деdup через `deploy.deplLister[ns]`. + +**container** (`containermgr.go` строка 805): +```go +if _, ok := caaf.deplLister[ns]; ok { + return nil // already registered +} +``` +Деdup через `caaf.deplLister[ns]`. + +**router** (`httpTriggers.go` строка 461): +```go +if !utils.DefaultNSResolver().AddNamespace(ns) { + return nil // already registered +} +``` +Деdup через глобальный resolver — **уже починен** предыдущим коммитом (`RemoveNamespace` в resolver). + +**buildermgr envwatcher** (`envwatcher.go` строка 506): +```go +if _, exists := envw.envWatchInformer[ns]; exists { + return +} +``` +Деdup через `envw.envWatchInformer[ns]`. + +**buildermgr pkgwatcher** (`pkgwatcher.go` строка 337): +```go +if _, exists := pkgw.pkgInformer[ns]; exists { + return +} +``` +Деdup через `pkgw.pkgInformer[ns]`. + +### 2.3 Анализ стратегии удаления + +`NewDefaultManagedNamespaceWatcherConfig` создаёт конфиг с `RemovalStrategy: NamespaceRemovalStrategyTrackOnly`. +При `TrackOnly` — `HandleWatcherNamespaceRemoval` вызывает `DefaultNSResolver().RemoveNamespace()` (наш предыдущий фикс), но **не** вызывает `manager.DispatchRemove()` → `subscriber.OnNamespaceRemove()` → `RemoveFunc` не срабатывает. + +Для вызова `RemoveFunc` нужна стратегия `DispatchRemove`. + +### 2.4 Решение: per-namespace context cancellation + +Informers запускаются через `factory.Start(ctx.Done())`. Стандартный способ остановить отдельный informer — отменить контекст, с которым он запущен. + +**Решение:** +```go +nsCtx, nsCancel := context.WithCancel(ctx) +gpm.nsCancels[ns] = nsCancel +finformer.Start(nsCtx.Done()) // вместо ctx.Done() +``` + +При `RemoveNamespace`: +```go +if cancel, ok := gpm.nsCancels[ns]; ok { + cancel() // останавливает goroutines informer factories + delete(gpm.nsCancels, ns) +} +``` + +Это чисто и не требует изменения k8s client-go. + +### 2.5 Mutex и thread-safety + +Существующий код в executor types не защищает lister maps мьютексами. Записи в них происходят только при `AddNamespace` (из subscriber goroutine). Добавление `RemoveNamespace` добавляет ещё одну запись из той же goroutine. Race condition с event handlers (которые читают эти maps) — известное ограничение существующего дизайна, не добавляем мьютексы чтобы не выходить за рамки задачи. + +`HTTPTriggerSet` уже имеет `informerMu sync.RWMutex` — его и используем в `RemoveNamespace` при удалении из `triggerInformer`/`funcInformer`. + +--- + +## 3. Реализация — пошаговое описание + +### Шаг 1: `pkg/executor/executortype/executortype.go` + +Добавлен метод в `ExecutorType` interface: + +```go +// RemoveNamespace deregisters a namespace from the executor, cancelling its +// informer goroutines and clearing dedup state so that a re-add works correctly. +// Called when a Namespace with label fission.io/managed=true is removed. +RemoveNamespace(ctx context.Context, ns string) error +``` + +**Почему:** Все три executor type реализуют этот интерфейс. Добавление в интерфейс гарантирует, что новый тип executor не забудет реализовать метод (компилятор поймает). + +--- + +### Шаг 2: `pkg/executor/executortype/poolmgr/gpm.go` + +**2a. Добавлено поле в struct:** +```go +// nsCancels holds per-namespace context cancel functions so informer +// factories started in AddNamespace can be stopped on RemoveNamespace. +nsCancels map[string]context.CancelFunc +``` + +**2b. Инициализация в `MakeGenericPoolManager`:** +```go +nsCancels: make(map[string]context.CancelFunc), +``` + +**2c. Изменение в `AddNamespace`:** вместо `ctx.Done()` передаём `nsCtx.Done()`: +```go +nsCtx, nsCancel := context.WithCancel(ctx) +gpm.nsCancels[ns] = nsCancel +finformer.Start(nsCtx.Done()) +gpmInformer.Start(nsCtx.Done()) +``` + +**2d. Новый метод `RemoveNamespace`:** +```go +func (gpm *GenericPoolManager) RemoveNamespace(ctx context.Context, ns string) error { + if ns == "" { return nil } + gpm.logger.Info("RemoveNamespace: cleaning up namespace (poolmgr)", ...) + if cancel, ok := gpm.nsCancels[ns]; ok { + cancel() + delete(gpm.nsCancels, ns) + } + delete(gpm.podLister, ns) + delete(gpm.podListerSynced, ns) + gpm.poolPodC.RemoveNamespace(ns) // очищает envLister/podLister в PoolPodController + return nil +} +``` + +--- + +### Шаг 3: `pkg/executor/executortype/poolmgr/poolpodcontroller.go` + +Добавлен метод, очищающий lister maps в `PoolPodController`: + +```go +func (p *PoolPodController) RemoveNamespace(ns string) { + delete(p.envLister, ns) + delete(p.envListerSynced, ns) + delete(p.podLister, ns) + delete(p.podListerSynced, ns) + p.logger.Info("PoolPodController.RemoveNamespace: cleared lister state", ...) +} +``` + +**Почему отдельный метод:** `PoolPodController` — отдельная структура внутри poolmgr. Доступ к её полям из `GenericPoolManager.RemoveNamespace` требовал бы либо экспорта полей, либо метода. Метод — чище. + +--- + +### Шаг 4: `pkg/executor/executortype/newdeploy/newdeploymgr.go` + +Аналогично gpm: +- Добавлен `nsCancels map[string]context.CancelFunc` в struct `NewDeploy` +- Инициализирован в `MakeNewDeploy` +- `AddNamespace` переключён на `nsCtx.Done()` +- Добавлен `RemoveNamespace` очищающий `deplLister`, `deplListerSynced`, `svcLister`, `svcListerSynced` + +--- + +### Шаг 5: `pkg/executor/executortype/container/containermgr.go` + +Аналогично. Struct `Container` получил `nsCancels`. `AddNamespace` использует `nsCtx.Done()`. `RemoveNamespace` очищает `deplLister`, `deplListerSynced`, `svcLister`, `svcListerSynced`. + +--- + +### Шаг 6: `pkg/executor/multitenant/namespace_subscriber.go` + +Добавлен `RemoveFunc` в `NamespaceSubscriberFuncs`: + +```go +RemoveFunc: func(ctx context.Context, record utils.NamespaceRecord) error { + return deregisterNamespace(ctx, logger, record.Name, executorTypes) +}, +``` + +`deregisterNamespace` итерирует все executor types и вызывает `et.RemoveNamespace(ctx, ns)`. + +--- + +### Шаг 7: `pkg/executor/multitenant/ns_watcher.go` + +**7a. Добавлен `deregisterNamespace`:** +```go +func deregisterNamespace(ctx, logger, ns, executorTypes) error { + var joinErr error + for _, et := range executorTypes { + if err := et.RemoveNamespace(ctx, ns); err != nil { + joinErr = errors.Join(joinErr, err) + } + } + logger.Info("multitenant.NSWatcher: deregistered namespace", ...) + return joinErr +} +``` + +**7b. Изменён `StartNSWatcher`:** стратегия `TrackOnly` → `DispatchRemove`: +```go +config := utils.NewDefaultManagedNamespaceWatcherConfig(...) +config.RemovalStrategy = utils.NamespaceRemovalStrategyDispatchRemove +``` + +**Почему:** Без `DispatchRemove` `RemoveFunc` подписчика никогда не вызывается. `TrackOnly` вызывает только `DefaultNSResolver().RemoveNamespace()` (что сделано в `HandleWatcherNamespaceRemoval`), но не диспетчирует событие подписчикам. + +--- + +### Шаг 8: `pkg/buildermgr/envwatcher.go` + +- Добавлен `nsCancels map[string]context.CancelFunc` в struct `environmentWatcher` +- Инициализирован в `makeEnvironmentWatcher` (там же где `envWatchInformer`) +- `AddNamespace` переключён на per-NS context: + ```go + nsCtx, nsCancel := context.WithCancel(ctx) + envw.nsCancels[ns] = nsCancel + factory.Start(nsCtx.Done()) + ``` +- Добавлен `RemoveNamespace(ns string)`: + ```go + func (envw *environmentWatcher) RemoveNamespace(ns string) { + if cancel, ok := envw.nsCancels[ns]; ok { cancel(); delete(...) } + delete(envw.envWatchInformer, ns) + } + ``` + +**Ошибка при первой попытке:** replace_string_in_file добавил `nsCancels` с тройным отступом (три таба вместо двух) и без закрывающего `}` struct literal — синтаксическая ошибка компиляции. Исправлено вторым вызовом replace. + +--- + +### Шаг 9: `pkg/buildermgr/pkgwatcher.go` + +Аналогично envwatcher: +- `nsCancels` в struct `packageWatcher` +- Инициализация в `makePackageWatcher` +- `AddNamespace` → per-NS ctx для `fissionFactory.Start()` и `podFactory.Start()` +- `RemoveNamespace(ns string)` очищает `pkgInformer[ns]`, `podInformer[ns]` + +--- + +### Шаг 10: `pkg/buildermgr/namespace_subscriber.go` + +Добавлены два новых интерфейса: +```go +type builderEnvNamespaceRemover interface { + RemoveNamespace(ns string) +} +type builderPkgNamespaceRemover interface { + RemoveNamespace(ns string) +} +``` + +Добавлен `RemoveFunc`: +```go +RemoveFunc: func(ctx context.Context, record utils.NamespaceRecord) error { + deregisterBuilderNamespace(record.Name, envw, pkgw) + return nil +}, +``` + +`deregisterBuilderNamespace` через type assertion вызывает `RemoveNamespace` если интерфейс реализован: +```go +func deregisterBuilderNamespace(namespace string, envw, pkgw) { + utils.DefaultNSResolver().RemoveNamespace(namespace) + if r, ok := envw.(builderEnvNamespaceRemover); ok { r.RemoveNamespace(namespace) } + if r, ok := pkgw.(builderPkgNamespaceRemover); ok { r.RemoveNamespace(namespace) } +} +``` + +**Почему type assertion:** `builderEnvNamespaceAdder` — интерфейс-параметр функции `NewNamespaceSubscriber`. Вместо добавления `RemoveNamespace` в существующий интерфейс (что сломало бы тестовые фейки) используем опциональный интерфейс через type assertion. + +--- + +### Шаг 11: `pkg/buildermgr/ns_watcher.go` + +Стратегия изменена на `DispatchRemove` аналогично executor. + +--- + +### Шаг 12: `pkg/router/httpTriggers.go` + +**12a. Добавлен `nsCancels` в struct:** +```go +// nsCancels holds per-namespace context cancel functions for informer lifecycle. +nsCancels map[string]context.CancelFunc +``` + +**12b. Инициализация в `makeHTTPTriggerSet`:** +```go +nsCancels: make(map[string]context.CancelFunc), +``` + +**12c. Изменён `AddNamespace`:** per-NS ctx: +```go +nsCtx, nsCancel := context.WithCancel(ctx) +ts.nsCancels[ns] = nsCancel +factory.Start(nsCtx.Done()) +k8sCache.WaitForCacheSync(nsCtx.Done(), ...) // тоже nsCtx +``` + +**12d. Новый метод `RemoveNamespace`:** +```go +func (ts *HTTPTriggerSet) RemoveNamespace(ns string) { + if cancel, ok := ts.nsCancels[ns]; ok { cancel(); delete(...) } + ts.informerMu.Lock() + delete(ts.triggerInformer, ns) + delete(ts.funcInformer, ns) + ts.informerMu.Unlock() + ts.syncTriggers() // немедленно перестраивает routing table без удалённого NS +} +``` + +**Почему `informerMu.Lock()`:** `HTTPTriggerSet` уже имеет `informerMu sync.RWMutex` для защиты `triggerInformer`/`funcInformer`. Используем его — не добавляем новые мьютексы. + +--- + +### Шаг 13: `pkg/router/namespace_subscriber.go` + +Добавлен `routerNamespaceRemover` interface и `RemoveFunc`: + +```go +type routerNamespaceRemover interface { + RemoveNamespace(ns string) +} + +RemoveFunc: func(ctx context.Context, record utils.NamespaceRecord) error { + if r, ok := ts.(routerNamespaceRemover); ok { + r.RemoveNamespace(record.Name) + } + return nil +}, +``` + +--- + +### Шаг 14: `pkg/router/ns_watcher.go` + +Стратегия → `DispatchRemove`. + +--- + +### Шаг 15: Тест-фейк `pkg/executor/multitenant/ns_watcher_test.go` + +`fakeExecutorType` не реализовывал новый метод → ошибка компиляции: +``` +*fakeExecutorType does not implement executortype.ExecutorType (missing method RemoveNamespace) +``` + +Добавлена заглушка: +```go +func (f *fakeExecutorType) RemoveNamespace(ctx context.Context, ns string) error { return nil } +``` + +--- + +## 4. Результат компиляции и тестов + +``` +go build ./pkg/... ./cmd/... → нет вывода (успех) + +go test ./pkg/utils/... + ./pkg/executor/... + ./pkg/buildermgr/... + ./pkg/router/... + +ok github.com/fission/fission/pkg/utils +ok github.com/fission/fission/pkg/executor/executortype/newdeploy +ok github.com/fission/fission/pkg/executor/executortype/poolmgr +ok github.com/fission/fission/pkg/executor/fscache +ok github.com/fission/fission/pkg/executor/multitenant +ok github.com/fission/fission/pkg/executor/util +ok github.com/fission/fission/pkg/buildermgr +ok github.com/fission/fission/pkg/router +``` + +--- + +## 5. Схема потока при удалении NS (после всех правок) + +``` +k8s: Namespace label fission.io/managed=true удалён/NS удалён + │ + ▼ +ManagedNamespaceWatcher (DispatchRemove стратегия) + │ + ├─► HandleWatcherNamespaceRemoval() + │ DefaultNSResolver().RemoveNamespace(ns) ← сброс глобального guard + │ manager.DispatchRemove(ctx, ns) + │ + ▼ +inMemoryNamespaceManager.DispatchRemove() + │ + ├─► goroutine: subscriber[executor].OnNamespaceRemove(record) + │ deregisterNamespace(ctx, logger, ns, executorTypes) + │ gpm.RemoveNamespace(ctx, ns) + │ nsCancel() ← останавливает informer goroutines + │ delete(podLister[ns]) + │ delete(podListerSynced[ns]) + │ poolPodC.RemoveNamespace(ns) + │ delete(envLister[ns]) + │ delete(envListerSynced[ns]) + │ delete(podLister[ns]) + │ delete(podListerSynced[ns]) + │ deploy.RemoveNamespace(ctx, ns) + │ nsCancel() + │ delete(deplLister[ns]) + │ delete(deplListerSynced[ns]) + │ delete(svcLister[ns]) + │ delete(svcListerSynced[ns]) + │ container.RemoveNamespace(ctx, ns) + │ nsCancel() + │ delete(deplLister[ns]) + │ delete(svcLister[ns]) + │ + ├─► goroutine: subscriber[buildermgr].OnNamespaceRemove(record) + │ deregisterBuilderNamespace(ns, envw, pkgw) + │ DefaultNSResolver().RemoveNamespace(ns) ← повторно (безопасно) + │ envw.RemoveNamespace(ns) + │ nsCancel() + │ delete(envWatchInformer[ns]) + │ pkgw.RemoveNamespace(ns) + │ nsCancel() + │ delete(pkgInformer[ns]) + │ delete(podInformer[ns]) + │ + └─► goroutine: subscriber[router].OnNamespaceRemove(record) + ts.RemoveNamespace(ns) + nsCancel() ← останавливает triggerInf/funcInf goroutines + informerMu.Lock() + delete(triggerInformer[ns]) + delete(funcInformer[ns]) + informerMu.Unlock() + syncTriggers() ← немедленно убирает routes для удалённого NS +``` + +--- + +## 6. Что НЕ было реализовано и почему + +**`FunctionServiceCache.DeleteByNamespace(ns string)`** — не реализовано. + +Причина: `idleObjectReaper` периодически вызывает `IsValid()` для всех записей. Для удалённого NS k8s API возвращает 404/403 → `IsValid()` вернёт `false` → запись будет удалена reaperом естественным образом. Это создаёт несколько минут "грязных" записей и 404 ошибки в логах, но не влияет на корректность: для удалённого NS новые запросы не придут (router очистил routes), а reaper уберёт старые записи. + +Реализация `DeleteByNamespace` потребовала бы добавления namespace-индекса в `byFunction`/`byAddress`/`byFunctionUID` кэшах (нетривиально), или дорогого линейного прохода по всем записям. Не было делать без явного запроса. + +--- + +## 7. Затронутые файлы (17 изменённых) + +| Файл | Тип изменения | +|------|---------------| +| `pkg/executor/executortype/executortype.go` | +метод в interface | +| `pkg/executor/executortype/poolmgr/gpm.go` | +поле nsCancels, modify AddNamespace, +RemoveNamespace | +| `pkg/executor/executortype/poolmgr/poolpodcontroller.go` | +RemoveNamespace | +| `pkg/executor/executortype/newdeploy/newdeploymgr.go` | +поле nsCancels, modify AddNamespace, +RemoveNamespace | +| `pkg/executor/executortype/container/containermgr.go` | +поле nsCancels, modify AddNamespace, +RemoveNamespace | +| `pkg/executor/multitenant/namespace_subscriber.go` | +RemoveFunc | +| `pkg/executor/multitenant/ns_watcher.go` | +deregisterNamespace, DispatchRemove | +| `pkg/executor/multitenant/ns_watcher_test.go` | +RemoveNamespace в fakeExecutorType | +| `pkg/buildermgr/namespace_subscriber.go` | +интерфейсы remover, +RemoveFunc, +deregisterBuilderNamespace | +| `pkg/buildermgr/envwatcher.go` | +nsCancels, modify AddNamespace, +RemoveNamespace | +| `pkg/buildermgr/pkgwatcher.go` | +nsCancels, modify AddNamespace, +RemoveNamespace | +| `pkg/buildermgr/ns_watcher.go` | DispatchRemove | +| `pkg/router/httpTriggers.go` | +nsCancels, modify AddNamespace, +RemoveNamespace | +| `pkg/router/namespace_subscriber.go` | +routerNamespaceRemover, +RemoveFunc | +| `pkg/router/ns_watcher.go` | DispatchRemove | +| `doc/FORENSIC_ARCHITECTURE_AUDIT.md` | перемещён из корня (git rename) | +| `doc/console-compat-2026-05-15.md` | создан (отдельная задача) | + +--- + +## 8. Ошибки в процессе + +### Ошибка 1: Синтаксическая ошибка в envwatcher.go +**Что случилось:** При попытке заменить блок инициализации struct добавился `nsCancels:` с тройным отступом и без закрывающей `}`: +``` +// Стало (неверно): + enableOwnerReferences: utils.IsOwnerReferencesEnabled(), + nsCancels: make(map[string]context.CancelFunc), + err := envWatcher.EnvWatchEventHandlers(ctx) +// ← пропущена } закрывающая struct literal +``` + +**Причина:** replace_string_in_file не нашёл точное совпадение с нужным whitespace и применил замену частично некорректно. + +**Исправление:** второй вызов replace_string_in_file с правильным контекстом (включая соседние строки для однозначного совпадения). + +**Вывод компилятора:** +``` +pkg/buildermgr/envwatcher.go:117:6: syntax error: unexpected := in composite literal; possibly missing comma or } +``` + +### Ошибка 2: Тест-фейк не реализует интерфейс +**Что случилось:** После добавления `RemoveNamespace` в interface `ExecutorType`, тест `ns_watcher_test.go` не компилировался: +``` +*fakeExecutorType does not implement executortype.ExecutorType (missing method RemoveNamespace) +``` + +**Исправление:** добавлена заглушка в `fakeExecutorType`. + +--- + +## 9. Инварианты безопасности + +1. `nsCancel()` идемпотентен: повторный вызов не паникует (context package гарантирует это) +2. `RemoveNamespace("")` защищён early return во всех реализациях +3. `deregisterBuilderNamespace` через type assertion — безопасно если интерфейс не реализован (просто пропускает) +4. `routerNamespaceRemover` через type assertion в router subscriber — аналогично +5. Goroutines informer factories останавливаются асинхронно после `cancel()` — это нормально, k8s client-go гарантирует graceful shutdown при отмене контекста +6. После `RemoveNamespace` и до следующего `AddNamespace` — любые события от k8s для этого NS будут проигнорированы (informers остановлены, listers удалены)