Skip to content

[addon-operator] fix: make module hook registration idempotent and race-free#797

Merged
ldmonster merged 6 commits into
mainfrom
fix/race-during-module-hooks-registration
Jul 10, 2026
Merged

[addon-operator] fix: make module hook registration idempotent and race-free#797
ldmonster merged 6 commits into
mainfrom
fix/race-during-module-hooks-registration

Conversation

@Glitchy-Sheep

@Glitchy-Sheep Glitchy-Sheep commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Overview

Fixes three bugs in the module hook machinery:

  • Hook registration is idempotent and race-free:
    • AddHook replaces the old hook object in all bindings instead of appending a duplicate
    • registered / controllersReady flags are read and written under the storage lock
    • RegisterHooks is serialized with a mutex
    • getHooks returns a sorted copy instead of the shared slice
  • A module no longer loses its Synchronization run after a partial failure:
    • HandleModuleEnableKubernetesBindings tries every hook and aggregates errors with errors.Join, instead of aborting on the first failure
    • on retry, shell-operator v1.20.2 re-emits Synchronization contexts for all hooks (idempotent EnableKubernetesBindings)
  • Synchronization tasks are not silently dropped anymore:
    • hook queues are ensured (idempotently) at the start of the QueueSynchronizationTasks phase
    • a task that cannot be queued fails the phase and is retried, instead of leaving its binding falsely "completed" with kubernetes events locked forever

Also bumps shell-operator to the released v1.20.2.

Why we need it

Production incidents:

  • 2026-05-05, 2026-06-30: startup crash loop (SIGSEGV). A failed registration attempt left hooks with a nil HookController, the retry appended duplicates next to them, and the kube-events dispatcher crashed on the stale entry.
  • 2026-07-08: one hook's monitor failed to start, the whole module lost its Synchronization run. Hooks read empty snapshots while "succeeding", a beforeHelm hook wiped internal values, and the module render shrank.

Before: one transient failure poisons the hook index or drops the module's Synchronization run for good.
After: a registration retry replaces stale hooks; a partial enable failure is retried, and the retry re-emits Synchronization for every hook.

Notes for the reviewer

@Glitchy-Sheep Glitchy-Sheep self-assigned this Jul 8, 2026
@Glitchy-Sheep Glitchy-Sheep added the bug Something isn't working label Jul 8, 2026
@Glitchy-Sheep Glitchy-Sheep requested a review from ldmonster July 9, 2026 14:25
@Glitchy-Sheep Glitchy-Sheep force-pushed the fix/race-during-module-hooks-registration branch from b592a4c to c22479a Compare July 9, 2026 17:26
Glitchy-Sheep added a commit to deckhouse/deckhouse that referenced this pull request Jul 10, 2026
…ifetime fixes

Picks up shell-operator 38c7e4f (Error log + binding_monitor_missing_total
in SnapshotsFor) and ed5d828 (shared informer factory lifetime owned by the
manager + factory_informer_dead_total), and addon-operator 74703bc
(Synchronization retry test, fail-on-unqueued-sync-task fix, shell-operator
pin bump). Root and tools modules together, so 'go generate' stays green.

Pseudo-versions to be replaced with release tags once the upstream PRs
merge (flant/shell-operator#922, flant/addon-operator#797).

Signed-off-by: Roman Berezkin <roman.berezkin@flant.com>
@Glitchy-Sheep Glitchy-Sheep changed the title [addon-operator] make module hook registration idempotent [addon-operator] fix: make module hook registration idempotent and race-free Jul 10, 2026
- AddHook replaces the previously registered hook object in all bindings
  instead of appending a duplicate: a stale entry keeps a nil
  HookController and crashes the kube-events dispatcher after a
  registration retry
- registered/controllersReady flags are read and written under the
  storage lock, giving a happens-before edge between setting hook
  controllers and observing the readiness flag
- RegisterHooks is serialized with a dedicated mutex so two concurrent
  ModuleRun tasks cannot register the same module twice
- getHooks sorts and returns a copy instead of the shared slice

Signed-off-by: Roman Berezkin <roman.berezkin@flant.com>
One hook whose monitor could not start aborted HandleModuleEnableKubernetesBindings
for the whole module, so the remaining hooks never emitted their Synchronization
contexts. Collect errors with errors.Join instead of returning on the first one.

Ported from #798.

Signed-off-by: Roman Berezkin <roman.berezkin@flant.com>
The best-effort enable loop (errors.Join) is only safe together with
the idempotent EnableKubernetesBindings from shell-operator c52b11d:
on an older shell-operator, pass 1 starts monitors for every successful
hook and the retry then hits the alreadyEnabled early-return, which
yields zero Synchronization contexts for all of them - a wider loss
window than before the aggregation. Re-point to the tagged release
once flant/shell-operator#922 merges.

Signed-off-by: Roman Berezkin <roman.berezkin@flant.com>
Encodes the acceptance scenario for the lost-Synchronization incident:
pass 1 fails on one hook while the remaining hooks still emit their
Synchronization contexts, pass 2 re-emits contexts for every hook.

The failing hook sorts first, so the test catches both regressions:
a revert of the best-effort aggregation in
HandleModuleEnableKubernetesBindings (pass 1 would emit zero contexts)
and a non-idempotent shell-operator Enable (pass 2 would emit only the
failed hook's context).

Signed-off-by: Roman Berezkin <roman.berezkin@flant.com>
A Synchronization task that is built but never queued left its binding
invisible to SynchronizationState.IsCompleted (vacuously completed) and
its kubernetes events locked forever, with only an Error log.

- ensure hook queues exist at the start of the QueueSynchronizationTasks
  phase: queues are normally created in the Startup phase only when
  DoModuleStartup is set, and a converge restart can displace that task,
  making every later AddLastTaskToQueue fail deterministically
- collect queueing errors and fail the phase instead of skipping the
  task: the retry rebuilds every Synchronization context because
  EnableKubernetesBindings is idempotent
- log 'append' only for tasks that were actually queued

Signed-off-by: Roman Berezkin <roman.berezkin@flant.com>
@Glitchy-Sheep Glitchy-Sheep force-pushed the fix/race-during-module-hooks-registration branch from 74703bc to 8877274 Compare July 10, 2026 14:26
Glitchy-Sheep added a commit to deckhouse/deckhouse that referenced this pull request Jul 10, 2026
…ifetime fixes

Picks up shell-operator 38c7e4f (Error log + binding_monitor_missing_total
in SnapshotsFor) and ed5d828 (shared informer factory lifetime owned by the
manager + factory_informer_dead_total), and addon-operator 8877274 (rebased
onto main past v1.24.0: Synchronization retry test, fail-on-unqueued-sync-task
fix, shell-operator pin bump; brings module-sdk v0.12.0 transitively).
Root and tools modules together, so 'go generate' stays green.

Pseudo-versions to be replaced with release tags once the upstream PRs
merge (flant/shell-operator#922, flant/addon-operator#797).

Signed-off-by: Roman Berezkin <roman.berezkin@flant.com>
Release tag for flant/shell-operator#922 (tree-identical to the previously
pinned branch pseudo-version): panic isolation in ManagerEventsHandler,
idempotent self-healing EnableKubernetesBindings, binding_monitor_missing_total,
FactoryStore lifetime owned by the manager + factory_informer_dead_total.

The best-effort enable loop (errors.Join) is only safe together with the
idempotent EnableKubernetesBindings from this release: on an older
shell-operator, pass 1 starts monitors for every successful hook and the
retry then hits the alreadyEnabled early-return, which yields zero
Synchronization contexts for all of them.

Signed-off-by: Roman Berezkin <roman.berezkin@flant.com>
@Glitchy-Sheep Glitchy-Sheep force-pushed the fix/race-during-module-hooks-registration branch from 8877274 to 39f7473 Compare July 10, 2026 15:31
@Glitchy-Sheep Glitchy-Sheep marked this pull request as ready for review July 10, 2026 15:40
@ldmonster ldmonster merged commit e82d4ea into main Jul 10, 2026
8 of 9 checks passed
@ldmonster ldmonster deleted the fix/race-during-module-hooks-registration branch July 10, 2026 16:19
Glitchy-Sheep added a commit to deckhouse/deckhouse that referenced this pull request Jul 10, 2026
Release tags for the hook machinery fixes (flant/shell-operator#922,
flant/addon-operator#797, both merged):

- panic isolation in ManagerEventsHandler
- idempotent self-healing EnableKubernetesBindings, Synchronization
  is re-emitted on every enable call
- module hook registration is idempotent and race-free
- Synchronization tasks are not silently dropped on queueing errors
- shared informer factory lifetime is owned by the kube events manager
- new counters: binding_monitor_missing_total, factory_informer_dead_total

Root and tools modules together, so 'go generate' stays green.
Brings module-sdk v0.12.0 transitively.

Signed-off-by: Roman Berezkin <roman.berezkin@flant.com>
Glitchy-Sheep added a commit to deckhouse/deckhouse that referenced this pull request Jul 10, 2026
Release tags for the hook machinery fixes (flant/shell-operator#922,
flant/addon-operator#797, both merged):

- panic isolation in ManagerEventsHandler
- idempotent self-healing EnableKubernetesBindings, Synchronization
  is re-emitted on every enable call
- module hook registration is idempotent and race-free
- Synchronization tasks are not silently dropped on queueing errors
- shared informer factory lifetime is owned by the kube events manager
- new counters: binding_monitor_missing_total, factory_informer_dead_total

Root and tools modules together, so 'go generate' stays green.
Brings module-sdk v0.12.0 transitively.

Signed-off-by: Roman Berezkin <roman.berezkin@flant.com>
Glitchy-Sheep added a commit to deckhouse/deckhouse that referenced this pull request Jul 10, 2026
Release tags for the hook machinery fixes (flant/shell-operator#922,
flant/addon-operator#797, both merged):

- panic isolation in ManagerEventsHandler
- idempotent self-healing EnableKubernetesBindings, Synchronization
  is re-emitted on every enable call
- module hook registration is idempotent and race-free
- Synchronization tasks are not silently dropped on queueing errors
- shared informer factory lifetime is owned by the kube events manager
- new counters: binding_monitor_missing_total, factory_informer_dead_total

Root and tools modules together, so 'go generate' stays green.
Brings module-sdk v0.12.0 transitively.

Signed-off-by: Roman Berezkin <roman.berezkin@flant.com>
Glitchy-Sheep added a commit to deckhouse/deckhouse that referenced this pull request Jul 10, 2026
Release tags for the hook machinery fixes (flant/shell-operator#922,
flant/addon-operator#797, both merged):

- panic isolation in ManagerEventsHandler
- idempotent self-healing EnableKubernetesBindings, Synchronization
  is re-emitted on every enable call
- module hook registration is idempotent and race-free
- Synchronization tasks are not silently dropped on queueing errors
- shared informer factory lifetime is owned by the kube events manager
- new counters: binding_monitor_missing_total, factory_informer_dead_total

Root and tools modules together, so 'go generate' stays green.
Brings module-sdk v0.12.0 transitively.

Signed-off-by: Roman Berezkin <roman.berezkin@flant.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants