Skip to content

[shell-operator] fix: isolate panics in ManagerEventsHandler callbacks, self-heal kubernetes bindings, own shared informer lifetime#922

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

[shell-operator] fix: isolate panics in ManagerEventsHandler callbacks, self-heal kubernetes bindings, own shared informer lifetime#922
ldmonster merged 4 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

Four changes, one per commit:

  1. Wraps the kube-event and schedule callbacks in ManagerEventsHandler.Start with recover (new runEventCb helper), mirroring the existing recover in TaskQueue.processOne. A panic inside a callback is logged with the binding name and stack, the event's tail tasks are dropped, and the events goroutine keeps processing.
  2. Makes EnableKubernetesBindings idempotent and self-healing (ported from the release-1.76 fix, [shell-operator] fix: prevent silent empty snapshots from lost kubernetes monitors 1.17 #923): the alreadyEnabled early-return is removed, a Synchronization BindingExecutionInfo is emitted for every binding on every call, and a binding whose link exists but whose monitor is gone (Disable/Enable race) gets its monitor re-created and re-started. Deduplication of repeated Synchronization runs stays on the caller side (addon-operator tracks it via SynchronizationState).
  3. Makes the "configured binding without a live monitor" state observable: SnapshotsFor logs it at Error level and increments the new {PREFIX}binding_monitor_missing_total{binding} counter (shell_operator_ prefix in standalone shell-operator). This state is exactly how a lost or never-started monitor silently feeds empty snapshots into a hook that still "succeeds".
  4. Fixes the shared informer factory lifetime (port of the FactoryStore part of [shell-operator] fix: prevent silent empty snapshots from lost kubernetes monitors 1.17 #923, released in v1.17.7 and already running in deckhouse release-1.76 via [deckhouse-controller] fix empty snapshots deckhouse/deckhouse#21255):
    • factory contexts derive from the store's base context (the owning kubeEventsManager), not from the first consumer's context - stopping the first consumer no longer kills the shared informer for everyone else;
    • Start detects a factory whose informer goroutine has exited, drops it and builds a fresh one instead of silently reusing a corpse whose HasSynced() is still true;
    • AddEventHandler failure fails Start instead of a Warn with a nil registration;
    • Reset() cancels factories before dropping references; the cache-sync poll runs outside the store lock;
    • each dead-factory detection increments the new {PREFIX}factory_informer_dead_total{gvr,namespace} counter (extension over the 1.17 port).

What this PR does / why we need it

The events goroutine had no recover, so a panic inside kubeEventCb / scheduleCb killed the whole process. In production this made deckhouse-controller crash with SIGSEGV: a hook registration defect in addon-operator left orphan module hooks with a nil *HookController in the hook index, and the kube-events dispatcher called CanHandleKubeEvent on the nil receiver. The identical panic arriving through the task queue was recovered and logged as a warn; the one arriving through this goroutine took down the operator (incidents on 2026-06-30 and 2026-05-05, the latter with ~407 restarts).

The root cause is fixed separately in flant/addon-operator#797. This change is the defense-in-depth layer: no single hook dispatch panic, present or future, should escalate to a full operator crash while the task-queue path already tolerates it.

The EnableKubernetesBindings change fixes the second incident mechanism (2026-07-08): after a partial failure (one hook's monitor could not start), the retried call hit the alreadyEnabled early-return and yielded ZERO Synchronization contexts, so the module's hooks never received the values-populating Synchronization run - kubernetes bindings stayed empty and events stayed locked while the module reported success. The same early-return also made the "link present, monitor gone" state left by a DisableModuleHooks racing with the queue worker permanent: snapshots read as empty forever, with no error anywhere.

Before: any panic in a kube-event or schedule callback kills the whole shell-operator process; a retried EnableKubernetesBindings silently drops the Synchronization run; a lost monitor feeds empty snapshots forever with no signal.
After: the panic is recovered and logged ("panic recovered in ManagerEventsHandler" with binding and stack), event processing continues; every EnableKubernetesBindings call emits Synchronization contexts and repairs missing monitors; a snapshot read without a live monitor produces an Error log and increments binding_monitor_missing_total.

Special notes for your reviewer

  • On panic runEventCb returns nil tail tasks; AddTailTasks with an empty list is a no-op, so non-panicking paths are unchanged.
  • New tests, all pass under -race:
    • TestManagerEventsHandlerSurvivesPanicInKubeEventCb / ...InScheduleCb - on unfixed code they crash the whole test binary, the incident in miniature
    • TestEnableKubernetesBindings_RepeatCallStillEmitsSynchronization - regression: the second call used to return 0 contexts
    • TestEnableKubernetesBindings_SelfHealsMissingMonitor - the Disable/Enable race state is repaired by the next enable
    • TestSnapshotsFor_MissingMonitorIsCounted - the counter fires with the binding label
  • Monitor creation idempotency is per-binding (!linked || !HasMonitor), so the removed method-level early-return does not reintroduce duplicate monitors.
  • The metric storage reaches the bindings controller via an optional accessor on the kube events manager; no public constructor or interface signatures change, callers without a storage just get the Error log.
  • Semantics change for library consumers: EnableKubernetesBindings now returns Synchronization infos on every call (previously only the first). addon-operator handles repeated runs via SynchronizationState; flag this in the changelog.
  • API change for library consumers: NewFactoryStore() becomes NewFactoryStore(ctx) - same signature change that already shipped upstream in v1.17.7. Decide whether main releases this as v1.20.2 or v1.21.0.
  • The factory port is byte-identical to the reviewed v1.17.7 code except the added metric; new tests TestFactoryStore_SurvivesConsumerContextCancel and TestFactoryStore_RecreatesDeadFactory are ported as-is (the latter extended with the metric assertion). Known gap, same as upstream: no unit test forces the AddEventHandler error path (needs a stopped informer injected between the corpse check and handler registration).
  • Follow-up worth a look after this lands: WaitStopped semantics (a live factory returns instantly via an already-closed channel) predate this change and may deserve a revisit now that factories no longer die with their first consumer.
  • Companion PRs: [addon-operator] fix: make module hook registration idempotent and race-free addon-operator#797 (root-cause fix, pins this branch as a pseudo-version), [deckhouse] fix race condition panic in addon operator deckhouse/deckhouse#21201 (bump + live A/B fault-injection verification on a dev cluster), [deckhouse-controller] fix empty snapshots deckhouse/deckhouse#21255 (release-1.76 bump carrying the same fixes as v1.17.7/v1.21.19).

A panic inside kubeEventCb/scheduleCb killed the events goroutine and
with it the whole process. Wrap both callbacks with recover, mirroring
TaskQueue.processOne: log the panic with binding and stack, drop the
event's tail tasks, keep processing events.

Signed-off-by: Roman Berezkin <roman.berezkin@flant.com>
@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:27
@Glitchy-Sheep Glitchy-Sheep marked this pull request as ready for review July 9, 2026 14:27
EnableKubernetesBindings dropped the Synchronization run after a partial
failure and reused a link whose monitor was gone, so hooks read an empty
snapshot while still succeeding. Make the method idempotent, repair a
missing monitor, and warn when a configured binding has no live monitor.

Ported from #923.

Signed-off-by: Roman Berezkin <roman.berezkin@flant.com>
A configured kubernetes binding whose monitor is not running feeds an
empty snapshot into a hook that still succeeds. SnapshotsFor now logs
this state at Error level and increments the new
{PREFIX}binding_monitor_missing_total{binding} counter.

The metric storage reaches the bindings controller through an optional
accessor on the kube events manager, so no public constructor or
interface signatures change; callers without a storage just get the
Error log.

Signed-off-by: Roman Berezkin <roman.berezkin@flant.com>
… first consumer

Port of the FactoryStore part of #923 (released in v1.17.7, already
running in deckhouse release-1.76):

- factory contexts derive from the store's base context (the owning
  kubeEventsManager); stopping the consumer that created a factory no
  longer kills the shared informer for every other consumer
- Start detects a factory whose informer goroutine has exited, drops it
  and builds a fresh one instead of reusing a corpse whose HasSynced()
  is still true and which never delivers events
- AddEventHandler failure fails Start instead of a Warn with a nil
  registration
- Reset() cancels factories before dropping references
- the cache-sync poll runs outside the store lock, so one slow LIST
  does not serialize Start/Stop of every informer in the process

On top of the port: each dead-factory detection increments the new
{PREFIX}factory_informer_dead_total{gvr,namespace} counter.

NewFactoryStore() becomes NewFactoryStore(ctx) - the same exported API
change that already shipped upstream in v1.17.7.

Signed-off-by: Roman Berezkin <roman.berezkin@flant.com>
Glitchy-Sheep added a commit to flant/addon-operator that referenced this pull request Jul 10, 2026
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>
Glitchy-Sheep added a commit to flant/addon-operator that referenced this pull request Jul 10, 2026
…time fix

Picks up 38c7e4f (Error log + binding_monitor_missing_total in
SnapshotsFor) and ed5d828 (shared informer factory lifetime owned by
the manager + factory_informer_dead_total). Re-point to the tagged
release once flant/shell-operator#922 merges.

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
…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 [shell-operator] isolate panics in ManagerEventsHandler callbacks [shell-operator] fix: isolate panics in ManagerEventsHandler callbacks, self-heal kubernetes bindings, own shared informer lifetime Jul 10, 2026
Glitchy-Sheep added a commit to flant/addon-operator that referenced this pull request Jul 10, 2026
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>
Glitchy-Sheep added a commit to flant/addon-operator that referenced this pull request Jul 10, 2026
…time fix

Picks up 38c7e4f (Error log + binding_monitor_missing_total in
SnapshotsFor) and ed5d828 (shared informer factory lifetime owned by
the manager + factory_informer_dead_total). Re-point to the tagged
release once flant/shell-operator#922 merges.

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
…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>
@ldmonster ldmonster merged commit 742b3ba into main Jul 10, 2026
9 checks passed
@ldmonster ldmonster deleted the fix/race-during-module-hooks-registration branch July 10, 2026 15:24
Glitchy-Sheep added a commit to flant/addon-operator that referenced this pull request Jul 10, 2026
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 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