589 lines
28 KiB
Markdown
589 lines
28 KiB
Markdown
# 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.
|
||
|
||
Это важно, потому что именно в этом файле пользовательский контекст отдельно предупредил о возможных дополнительных изменениях между сообщениями. |