diff --git a/doc/thinking/2026-04-26-namespace-manager-logic-detailed.md b/doc/thinking/2026-04-26-namespace-manager-logic-detailed.md new file mode 100644 index 00000000..e8df99a8 --- /dev/null +++ b/doc/thinking/2026-04-26-namespace-manager-logic-detailed.md @@ -0,0 +1,589 @@ +# 2026-04-26 — Layer 1 namespace rewrite: подробная логика правок + +## Зачем этот документ + +Нужен не просто список коммитов, а объяснение инженерной логики: + +- что именно было не так в коде; +- почему исправление выбрано именно таким; +- почему изменения разбиты на маленькие шаги; +- какие инварианты я старался сохранить; +- что уже исправлено, а что еще нет. + +Этот документ описывает серию маленьких безопасных шагов в ветке +`rewrite/layer1-namespace-manager-step1`. + +Основной принцип серии: + +1. Не делать большой взрывной rewrite. +2. Сначала сузить race-surface и разъединить старую статическую модель от новой динамической. +3. Исправлять реальные дефекты отдельно от mechanical refactor. +4. После каждого шага отдельно проверять соответствующий пакет тестами. + +--- + +## Исходная архитектурная проблема + +Переделанный Layer 1 жил в гибридном состоянии. + +Старая модель Fission: + +- список resource namespaces задается один раз на старте; +- компоненты считают этот список immutable; +- informer factories строятся из startup configuration. + +Новая multi-tenant модель: + +- namespace появляется позже, уже после старта процесса; +- watcher видит label `fission.io/managed=true`; +- компоненты должны подключить новый namespace на лету. + +Из-за этого в коде образовался разрыв между двумя мирами: + +1. Часть кода уже работает как dynamic system. +2. Часть кода все еще читает глобальную map namespace-ов напрямую, как будто она immutable. +3. В некоторых компонентах startup-path и dynamic-path оказались несимметричными. +4. В некоторых местах общий global dedup конфликтует с локальной логикой конкретного компонента. + +Это и есть корневой дефект всей подсистемы: не один конкретный баг, а отсутствие единого namespace lifecycle contract. + +--- + +## Что было решено не делать сразу + +Я сознательно не пошел в большой rewrite в один коммит. + +Почему: + +1. Слишком много точек входа: executor, router, buildermgr, storagesvc, utils. +2. Если переписать все сразу, невозможно будет локализовать регрессию. +3. Уже были реальные functional дефекты в нескольких местах, их удобнее чинить изолированно. +4. Пользователь отдельно попросил идти последовательно и проверять после каждого изменения. + +Поэтому выбран bounded rewrite: сначала вычищать старые опасные предположения, затем исправлять функциональные несовпадения, и только потом идти к более крупному NamespaceManager. + +--- + +## Инварианты серии + +Во всех шагах я старался держать одинаковые правила. + +### 1. Не ломать действующий onboarding contract + +Если namespace приходит через label watcher, компоненты должны продолжать подключать его без рестарта. Нельзя было ради рефактора возвращаться к статической модели. + +### 2. Не менять лишние контракты одновременно + +Если шаг про snapshot API, он не должен заодно переписывать cleanup semantics. + +### 3. Сначала механические и безопасные сдвиги, потом functional fixes + +Это нужно, чтобы понимать, баг возник из-за новой логики или уже существовал ранее. + +### 4. Каждый шаг должен быть проверяем локально + +После каждого шага запускались тесты по затронутому пакету, а не абстрактное «кажется, всё нормально». + +--- + +## Step 1 — Snapshot API для namespace resolver + +Коммит: `c987fa0` + +### Что было не так + +`NamespaceResolver` уже имел mutex для записи через `AddNamespace`, но многие потребители читали `FissionResourceNS` напрямую. + +Это означало следующее: + +1. Запись в map уже динамическая. +2. Чтение в части мест по-прежнему не thread-safe. +3. Код внешне выглядел как безопасный, потому что mutex в структуре есть, но контракт чтения не был централизован. + +То есть защита существовала только наполовину. + +### Что я сделал + +В `pkg/utils/namespace.go` добавлены: + +- `Snapshot()` +- `SnapshotWithOptions()` + +Их логика: + +1. Под read lock взять текущее состояние. +2. Скопировать его в detached slice. +3. Отсортировать, чтобы получить стабильный детерминированный порядок. + +Почему именно slice snapshot, а не снова map: + +1. Читателям в основном нужен именно проход по namespace-ам. +2. Slice удобнее для безопасной итерации. +3. Сортировка убирает дрожание порядка и делает поведение более предсказуемым в тестах и логике startup factory generation. + +### Почему это был правильный первый шаг + +Этот шаг почти не меняет бизнес-логику. Он не трогает watchers, RBAC, cleanup, lifecycle events. Он вводит базовый безопасный API, на который потом можно переводить потребителей. + +### Что было переведено сразу + +Чтобы snapshot API не оставался мертвым кодом, на него были переведены: + +- `pkg/utils/informer.go` +- startup factory creation в `pkg/executor/executor.go` + +Логика этого выбора: + +1. Это общие helper path. +2. Они касаются большого числа компонентов. +3. Но при этом change поверхностный: вместо прямой итерации по map берется snapshot. + +### Отдельный мелкий дефект, найденный на шаге 1 + +Новые тесты создали локальный `NamespaceResolver` без logger. Выяснилось, что часть методов предполагает ненулевой logger. Это нехорошо само по себе: utility object не должен падать только потому, что его используют вне global singleton. + +Поэтому были добавлены nil checks вокруг debug/info логов в resolver. + +### Проверка шага + +Проверялось: + +- `go test ./pkg/utils/...` +- `go test ./pkg/executor/...` + +Смысл проверки: + +1. Убедиться, что snapshot API корректен как utility layer. +2. Убедиться, что startup path executor не поменял поведение. + +--- + +## Step 2 — Исправление namespace routing в serviceaccount checker + +Коммит: `9ce9829` + +### Что было не так + +В `pkg/utils/serviceaccount.go` был более тонкий дефект, чем просто прямое чтение map. + +В `runSACheck()` одна и та же переменная `ns` переиспользовалась внутри цикла по permission groups. + +Смысл проблемы: + +1. Есть исходный base namespace. +2. Для fetcher нужен путь через `GetFunctionNS(baseNS)`. +3. Для builder нужен путь через `GetBuilderNS(baseNS)`. +4. Но код мутировал саму переменную `ns` по мере обхода permission sets. + +Это опасно, потому что builder resolution начинает зависеть от предыдущего шага цикла, а не от исходного namespace. + +Если `FunctionNamespace` и `BuilderNamespace` различаются, route builder SA может поехать. + +### Что я сделал + +Изменение было разбито на две части: + +1. Итерироваться не по `FissionResourceNS` напрямую, а по `Snapshot()`. +2. Явно вычислять `targetNS` из `baseNS` через отдельный метод `resolveSANamespace(baseNS, saName)`. + +Почему выделен отдельный метод: + +1. Логика namespace routing становится читаемой как отдельный контракт. +2. Её можно тестировать отдельно. +3. В коде исчезает скрытая мутация переменной цикла. + +### Почему я не переписывал весь serviceaccount.go сразу + +В файле еще остаются спорные места: + +- глобальные `fetcherCheck` / `builderCheck`; +- мутация `permission.exists`; +- runtime provisioning через `LocalSubjectAccessReview`. + +Но если решать всё сразу, шаг становится слишком широким. На этом этапе была цель исправить именно namespace routing bug и убрать прямую итерацию по общей map. + +### Какой тест был добавлен + +Добавлен unit test на `resolveSANamespace()`: + +- fetcher на default namespace должен идти в function namespace; +- builder на default namespace должен идти в builder namespace; +- tenant namespace должен сохраняться как tenant namespace. + +Тест важен не из-за синтаксиса, а потому что он фиксирует смысловую развязку между двумя namespace path. + +### Проверка шага + +Проверялось: + +- `go test ./pkg/utils/...` +- `go test ./pkg/executor/...` + +--- + +## Step 3 — Перевод runtime loops на snapshot API + +Коммит: `6102b27` + +### Что было не так + +Даже после появления snapshot API ещё оставались runtime loops, которые напрямую читали общую map namespace-ов в горячих путях: + +- adopt existing resources; +- idle object reaper; +- orphan archive pruning. + +Это плохо не только из-за race. Это также концептуально закрепляет старую модель «список namespace-ов — это просто глобальная map, в которую можно смотреть отовсюду». + +### Что я сделал + +Перевёл на `Snapshot()` следующие места: + +- `pkg/executor/executortype/container/containermgr.go` +- `pkg/executor/executortype/newdeploy/newdeploymgr.go` +- `pkg/executor/executortype/poolmgr/gpm.go` +- `pkg/storagesvc/archivePruner.go` + +### Почему именно эти места были хорошим кандидатом + +Потому что это mechanical refactor: + +1. Логика списков не меняется. +2. Namespace source меняется с raw map на stable snapshot. +3. Поведение должно оставаться тем же, кроме устранения unsafe read. + +### Что это дало + +1. Уменьшило площадь прямого доступа к глобальному mutable состоянию. +2. Подготовило код к следующему этапу, когда namespace registry станет ещё более централизованным. +3. Сделало background loops более предсказуемыми при одновременном dynamic onboarding. + +### Проверка шага + +Проверялось: + +- `go test ./pkg/executor/... ./pkg/storagesvc/...` + +--- + +## Step 4 — Исправление buildermgr dedup bug + +Коммит: `56a499a` + +### Это уже не mechanical refactor, а реальный functional fix + +### Что было не так + +`buildermgr.StartNSWatcher()` при появлении нового namespace делал: + +1. `envw.AddNamespace()` +2. `pkgw.AddNamespace()` + +Но оба watcher-а использовали один и тот же глобальный dedup через `nsResolver.AddNamespace()`. + +Фактический эффект: + +1. Первый вызов успешно добавляет namespace в global resolver. +2. Второй вызов видит, что namespace уже «есть». +3. И просто выходит. + +То есть в buildermgr динамический namespace мог получить только часть подписок. + +Это уже не theoretical risk, а реальный дефект логики. + +### Почему проблема архитектурная + +Здесь смешались два уровня ответственности: + +1. Global registry должен знать, что namespace существует. +2. Конкретный компонент должен знать, подписался ли он уже на этот namespace. + +Это разные виды dedup. + +Один глобальный dedup не может корректно заменить локальный dedup для двух разных subcomponents. + +### Что я сделал + +Логику развёл по уровням: + +1. В `pkg/buildermgr/ns_watcher.go` global resolver обновляется один раз. +2. `environmentWatcher` dedup делает по своей map `envWatchInformer`. +3. `packageWatcher` dedup делает по своей map `pkgInformer`. + +### Почему это правильнее + +Теперь структура похожа на executor path: + +1. Глобальный реестр говорит: namespace известен системе. +2. Каждый компонент сам решает: свои informers он уже поднял или нет. + +Именно так должен выглядеть multi-component dynamic onboarding. + +### Что я сознательно не делал + +Не добавлял remove/cleanup и не переделывал buildermgr lifecycle целиком. На шаге требовалось только убрать ошибку дедупликации. + +### Проверка шага + +Проверялось: + +- `go test ./pkg/buildermgr/...` + +Тестов в пакете немного, но для этого шага важно было хотя бы подтвердить, что wiring собирается и не поломан compile-time. + +--- + +## Step 5 — Исправление parity gap в newdeploy + +Коммит: `94f26b6` + +### Что было не так + +`MakeNewDeploy()` на старте процесса регистрировал оба типа handler-ов: + +- `FunctionEventHandlers()` +- `EnvEventHandlers()` + +Но `AddNamespace()` для динамически появившегося namespace регистрировал только `FunctionEventHandlers()`. + +Это значит, что два namespace-а с одинаковым содержимым вели себя по-разному только из-за времени появления: + +1. startup namespace обслуживается полным code path; +2. dynamic namespace обслуживается урезанным code path. + +Это очень плохое свойство для Layer 1, потому что поведение перестаёт зависеть только от данных и начинает зависеть от истории запуска процесса. + +### Что я сделал + +В `newdeploy.AddNamespace()` добавил регистрацию `EnvEventHandlers()` рядом с `FunctionEventHandlers()`. + +### Почему fix именно такой + +Потому что это минимальное исправление семантической несимметрии. + +Я не придумывал новую абстракцию, а привёл dynamic path к уже существующему startup contract. + +### Инженерный смысл шага + +Это важный принцип всей серии: если startup-path и late onboarding-path делают похожую работу, они должны проходить через один и тот же контракт, а не через два слегка разных набора side effects. + +### Проверка шага + +Проверялось: + +- `go test ./pkg/executor/executortype/newdeploy` + +--- + +## Step 6 — Защита router informer maps от гонок + +Коммит: `87477d4` + +### Что было не так + +В router динамический namespace добавляет новые informer-ы в две map: + +- `triggerInformer` +- `funcInformer` + +Параллельно `updateRouter()` итерируется по тем же map, собирая триггеры и функции для rebuild router-а. + +Плюс `functionReferenceResolver` получает `funcInformer` и тоже читает его напрямую. + +Это создаёт классическую проблему: + +1. одна goroutine пишет в map; +2. другая одновременно по ней итерируется; +3. третья читает её через resolver. + +Результат может быть от паники `concurrent map iteration and map write` до тихого чтения неполного состояния. + +### Почему шаг стал чуть шире + +Простой mutex только вокруг `HTTPTriggerSet.AddNamespace()` не решал бы проблему полностью, потому что `functionReferenceResolver` держал свою ссылку на ту же mutable структуру. + +Поэтому понадобилось сделать две вещи одновременно: + +1. Защитить maps в `HTTPTriggerSet` через `RWMutex` и snapshot helpers. +2. Дать `functionReferenceResolver` собственный thread-safe путь доступа к informer registry. + +### Что я сделал + +В `HTTPTriggerSet`: + +- добавлен `RWMutex`; +- добавлены `snapshotTriggerInformers()`; +- добавлены `snapshotFuncInformers()`; +- `updateRouter()` и setup handlers теперь работают по snapshot-спискам. + +В `functionReferenceResolver`: + +- добавлен `RWMutex`; +- чтение informer-а по namespace теперь под read lock; +- добавлен `addInformer()` для безопасного добавления нового namespace. + +В `router.AddNamespace()`: + +- запись в `triggerInformer` и `funcInformer` идёт под lock; +- resolver получает новый informer через собственный безопасный метод. + +### Почему именно snapshot-helpers, а не держать lock во время всей итерации + +Потому что rebuild router-а и чтение store-ов могут быть относительно дорогими. Держать глобальный lock на всё это время было бы лишним. Нам нужен был не coarse lock на длинный процесс, а короткий lock на получение стабильного снимка ссылок на informer-ы. + +То есть стратегия такая: + +1. Быстро снять snapshot ссылок. +2. Отпустить lock. +3. Работать со snapshot уже без блокировки записи. + +Это лучше и по безопасности, и по latency. + +### Проверка шага + +Проверялось: + +- `go test ./pkg/router/...` + +--- + +## Почему шаги документировались отдельно + +Я сохранял отдельный thinking-файл на каждый шаг не ради бюрократии, а ради трассируемости. + +Когда изменения маленькие, отдельные документы позволяют понять: + +1. какой дефект исправлял именно этот коммит; +2. что было осознанно оставлено за рамками; +3. какой тест подтверждал именно этот шаг; +4. где functional fix, а где только mechanical safety refactor. + +Именно это позволяет потом анализировать regressions не по памяти, а по истории. + +--- + +## Что осталось нерешённым после step 6 + +Несмотря на шесть шагов, это ещё не финальный NamespaceManager rewrite. + +Остаются важные вопросы. + +### 1. Нет remove/cleanup semantics + +Система умеет add, но почти не умеет delete/relabel cleanup. + +Что это значит practically: + +- informer-ы и локальные registry entries живут вечно; +- once onboarded, always onboarded; +- короткоживущие tenant namespace-ы будут оставлять мусор. + +### 2. `serviceaccount.go` всё ещё не идеален + +Текущий `serviceaccount.go` уже лучше, чем до step 2, но файл всё ещё сложный: + +- глобальные `fetcherCheck` / `builderCheck` живут как process-wide mutable objects; +- `permission.exists` мутируется в runtime; +- provisioning и permission-check тесно сцеплены. + +Это отдельный кандидат на следующий bounded refactor, но уже не маленький mechanical шаг. + +### 3. Глобальный resolver всё ещё остаётся transitional abstraction + +`NamespaceResolver` теперь безопаснее для чтения, но это пока ещё не полноценный NamespaceManager с событиями, remove lifecycle и подписками. + +Он всё ещё ближе к thread-safe registry, чем к полной orchestration layer. + +### 4. Cleanup/restart/backfill lifecycle ещё не централизован + +Часть компонентов уже ближе к единообразию, но по-прежнему нет одного центрального orchestration contract вида: + +- add existing namespaces on startup; +- reconcile on relabel; +- remove on delete; +- rebuild after restart; +- re-register late component safely. + +--- + +## Почему я не стал сразу делать remove/cleanup + +Потому что это уже следующая категория сложности. + +До step 6 изменения укладывались в схему: + +- локальный и понятный дефект; +- ограниченный blast radius; +- тестируемый пакет; +- отдельный маленький commit. + +Remove/cleanup меняет уже жизненный цикл системы и затрагивает много мест одновременно: + +- watcher behavior; +- manager lifecycle; +- informer shutdown semantics; +- cache invalidation; +- resolver state. + +Это не тот шаг, который разумно смешивать с небольшими safety fixes. + +--- + +## Почему такая стратегия лучше, чем «переписать всё сразу» + +Потому что сейчас уже есть видимый результат с низким риском: + +1. Уменьшено число прямых доступов к общей mutable map. +2. Исправлен реальный functional bug в buildermgr. +3. Исправлена реальная логическая ошибка в serviceaccount namespace routing. +4. Исправлена несимметрия в newdeploy dynamic path. +5. Закрыта явная router race-surface. + +И всё это не одним большим коммитом, а серией шагов с локальной верификацией. + +Для инфраструктурного кода это важнее, чем «красивый большой rewrite», который сложно раскладывать при регрессиях. + +--- + +## Какие проверки были прогнаны по ходу серии + +После шагов запускались: + +- `go test ./pkg/utils/...` +- `go test ./pkg/executor/...` +- `go test ./pkg/storagesvc/...` +- `go test ./pkg/buildermgr/...` +- `go test ./pkg/router/...` + +Логика была такая: + +1. Не гонять каждый раз всю репу, если шаг локальный. +2. Но обязательно проверять затронутый пакет и соседний пакет, если change касается shared utility layer. + +--- + +## Текущее состояние после серии + +Серия шагов 1-6 не завершает rewrite, но заметно улучшает базу для следующего этапа. + +Что теперь стало лучше: + +1. Namespace reads стали заметно более дисциплинированными. +2. Dynamic namespace onboarding стал логически ровнее между компонентами. +3. В router исчезла наиболее явная race-surface на informer maps. +4. Buildermgr больше не теряет часть подписок на новый namespace из-за неправильного dedup. + +Что остаётся следующим осмысленным этапом: + +1. Вынесение уже полноценного NamespaceManager как orchestration layer. +2. Remove/cleanup lifecycle. +3. Разделение discovery, registry и provisioning. +4. Дополнительные тесты на restart/relabel/delete/burst onboarding. + +--- + +## Отдельная заметка про `serviceaccount.go` + +На момент написания этого документа файл `pkg/utils/serviceaccount.go` был заново перечитан по текущему содержимому. Документ описывает актуальную логику файла в его текущем состоянии, а не только то состояние, которое было в момент коммита step 2. + +Это важно, потому что именно в этом файле пользовательский контекст отдельно предупредил о возможных дополнительных изменениях между сообщениями. \ No newline at end of file