8000 fix(bridge): gate component deactivation on CMS dependency safety by VLukiantsev · Pull Request #992 · google/xmc · GitHub
[go: up one dir, main page]
Skip to content

fix(bridge): gate component deactivation on CMS dependency safety - #992

Merged
VLukiantsev merged 3 commits into
feat/bridge-dependency-gatingfrom
fix/bridge-deactivation-gating
Sep 28, 2026
Merged

fix(bridge): gate component deactivation on CMS dependency safety#992
VLukiantsev merged 3 commits into
feat/bridge-dependency-gatingfrom
fix/bridge-deactivation-gating

Conversation

@VLukiantsev
@VLukiantsev VLukiantsev commented Sep 25, 2026 •
edited by AntonTyb
Loading
Copy link
Copy Markdown
Collaborator

Summary

Setting enabled: false on a Component now goes through CMS deactivation safety. Disabling a component while enabled components depend on it is blocked, and its workload keeps running.

Stack 4/4: CMS dependencies (0) → 1P health (B) → dependency-gated actuation (C1) → this PR.

Motivation & Context

reconcileDisabled deleted the 1P workload and reported DISABLED without consulting CMS. So redpanda could be disabled through its CR while ingestion-gateway depended on it, even though the store's EvaluateDeactivationSafety would have rejected that.

Key Changes

  • CMS client: the bridge CMSClient, the server adapter and the fakes gain PrepareDeactivation and CompleteDeactivation.
  • Disable flow: PrepareDeactivation{force:false}, then delete the Helm workload, then CompleteDeactivation, then UpdateObservedGeneration(DISABLED).
  • Blocked (FailedPrecondition): the workload is kept. The CR gets DeactivationBlocked=True and Ready=False/DeactivationBlocked (Degraded=False), with a message naming the active dependents. The check re-runs at most every 30s.
  • A failure after teardown gives DeactivationIncomplete (Degraded=True) and is retried idempotently.
  • Steady state: the handshake is skipped once the CR reports ComponentDisabled at the current generation.
  • Previously active: wasComponentPreviouslyActive ignores never-actuated reasons (DependenciesNotReady, DependencyCycle).
  • Docs: the READMEs cover the deactivation steps, the conditions and the recheck interval.

Assumptions

  • The bridge does not wait for the recommended drain time between Prepare and Complete, which is unchanged from before. — Draining is a separate follow-up. — Workloads are removed without a drain window.
  • A component that was never registered in CMS (NotFound) is disabled without the handshake.

Verification & Testing

  • Commands Executed: bazel test //pkg/components/... //cmd/xmc_bridge/... (9/9 pass), ./.githooks/pre-commit (pass). Size vs parent: 110 prod / 399 total.
  • New tests:
    • TestReconcileComponent_GatedDependentBlocksDeactivationInCMS, which uses the real cms.NewServer, MemoryStore and adapter;
    • deactivation gating: blocked, allowed, failed Prepare, failed Complete after teardown, blocked retry;
    • multi-pass blocked → unblocked;
    • recheck throttle;
    • wasComponentPreviouslyActive table test.

Live Deployment & Manual Testing (GKE ..., whole stack + #969/#974)

The only action taken was kubectl patch component <name> --type merge -p '{"spec":{"enabled":<bool>}}'.

Live run (sanitized) 
S0 11:28:00 redpanda: ENABLED Ready=True | ingestion-gateway: ENABLED Ready=True (Helm release "xmc-cmp-ingestion-gateway" deployed)
S1 PATCH redpanda enabled=false
   redpanda: PENDING_RECONCILIATION Ready=False/DeactivationBlocked blocked=True
     msg=Cannot disable "redpanda": component redpanda has active dependents: [ingestion-gateway]
   ingestion-gateway: ENABLED Ready=True, release deployed, pods 2/2
S2 PATCH ingestion-gateway enabled=false
   ingestion-gateway: DISABLED, release + Deployment removed
   redpanda: DISABLED/ComponentDisabled (deactivation now allowed)
S3 PATCH ingestion-gateway enabled=true
   ingestion-gateway: PENDING Ready=False/DependenciesNotReady msg=waiting on dependencies: redpanda (disabled); no release
S4 PATCH redpanda enabled=true
   redpanda: ENABLED Ready=True; ingestion-gateway: release installed by bridge, ENABLED Ready=True
S5 out-of-band `helm uninstall` -> ingestion-gateway Ready=False/HelmReleaseNotFound;
   bridge restart -> release reinstalled, ENABLED Ready=True
Bridge ERROR log lines in the last 6 minutes (bridge pod restarted at S5): 0

Related Issues / Bugs

Found while working on bug 548435582.

TAG=agy
CONV=baca6192-74d3-4579-9f73-0a3263d9a2a7

@VLukiantsev
VLukiantsev added this pull request to stack #993 September 25, 2026 12:12
gemini-code-assist[bot]
gemini-code-assist Bot reviewed Sep 25, 2026
View reviewed changes
@gemini-code-assist gemini-code-assist Bot left a comment
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request implements a CMS deactivation safety handshake when disabling components (spec.enabled: false). It introduces PrepareDeactivation and CompleteDeactivation steps to prevent tearing down workloads if active dependents exist, managing blocked or incomplete deactivations via new status conditions (DeactivationBlocked and DeactivationIncomplete). Feedback on the changes highlights a potential nil pointer dereference in reportDeactivationBlocked where the result of findCondition is accessed without a nil check, which could lead to a runtime panic.

Comment thread pkg/components/bridge/reconciler.go Resolved
@VLukiantsev
VLukiantsev commented Sep 25, 2026
Copy link
Copy Markdown
Collaborator Author

/gemini review

gemini-code-assist[bot]
gemini-code-assist Bot reviewed Sep 25, 2026
View reviewed changes
@gemini-code-assist gemini-code-assist Bot left a comment
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request implements CMS deactivation safety gating in the xmc-bridge reconciler. When a component is disabled (spec.enabled: false), the reconciler now calls PrepareDeactivation to verify if other active components depend on it. If active dependents exist, deactivation is blocked, the workload continues running, and the status reports DeactivationBlocked. Otherwise, the workload is deleted, and CompleteDeactivation is called to finalize the handshake. If the handshake fails after teardown, the state is marked as DeactivationIncomplete and retried. The PR also includes updated documentation and comprehensive unit tests for these deactivation flows. I have no feedback to provide as there are no review comments.

@VLukiantsev
VLukiantsev force-pushed the fix/bridge-deactivation-gating branch 2 times, most recently from 38e8b84 to a013c84 Compare September 25, 2026 13:19
@VLukiantsev
VLukiantsev removed this pull request from stack #993 September 25, 2026 14:49
@VLukiantsev
VLukiantsev added this pull request to stack #995 September 25, 2026 14:49
This was referenced Sep 25, 2026
Merged
Merged
@VLukiantsev
VLukiantsev marked this pull request as ready for review September 25, 2026 14:57
@github-actions
github-actions Bot requested a review from janzak-dev September 25, 2026 14:57
@VLukiantsev
VLukiantsev force-pushed the fix/bridge-deactivation-gating branch from a013c84 to e7dd760 Compare September 28, 2026 07:39
@VLukiantsev
VLukiantsev force-pushed the fix/bridge-deactivation-gating branch from e7dd760 to 637cd26 Compare September 28, 2026 08:26
janzak-dev
janzak-dev previously approved these changes Sep 28, 2026
View reviewed changes
Comment thread pkg/components/bridge/reconciler.go Resolved
Comment thread pkg/components/bridge/reconciler.go Resolved
github-actions[bot]
github-actions Bot previously approved these changes Sep 28, 2026
View reviewed changes
@github-actions github-actions Bot left a comment
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Incremental PR Approval

PR author is a code owner and changes since last approval (4 lines) are within the 10-line grace delta.

Preserving prior approval by @janzak-dev on commit 637cd26.

@VLukiantsev
VLukiantsev force-pushed the fix/bridge-deactivation-gating branch from b27a433 to 1549fc9 Compare September 28, 2026 11:15
github-actions[bot]
github-actions Bot previously approved these changes Sep 28, 2026
View reviewed changes
@github-actions github-actions Bot left a comment
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Incremental PR Approval

PR author is a code owner and changes since last approval (4 lines) are within the 10-line grace delta.

Preserving prior approval by @janzak-dev on commit 637cd26.

@VLukiantsev
fix(bridge): gate component deactivation on CMS dependency safety
27da026 
Setting spec.enabled: false now runs the CMS deactivation handshake
before tearing down workloads:

- PrepareDeactivation (blocking mode) is called first. If CMS returns
  FailedPrecondition because enabled components (including gated but
  registered ones) depend on this one, the workload keeps running and the
  CR reports DeactivationBlocked=True with Ready=False reason
  DeactivationBlocked. CMS is re-checked at most every 30s.
- Otherwise the 1P workload is deleted and CompleteDeactivation moves
  CMS from DISABLING to DISABLED. A handshake failure after teardown
  reports DeactivationIncomplete and is retried on every pass.
- wasComponentPreviouslyActive no longer treats gated components
  (DependenciesNotReady, DependencyCycle) as having a workload.
- CMSClient and the in-process adapter gain PrepareDeactivation and
  CompleteDeactivation.

BUG=b/548435582
TAG=agy
@VLukiantsev
fix(bridge): guard DeactivationBlocked condition lookup against nil
e3ed16e 
BUG=b/548435582
TAG=agy
CONV=baca6192-74d3-4579-9f73-0a3263d9a2a7
@VLukiantsev
fix(bridge): log CMS detail when deactivation is blocked; add split TODO
e5d766c 
Log the PrepareDeactivation FailedPrecondition detail on each fresh CMS
check (at most once per deactivation recheck interval) so operators can
see which dependents block the disable. Add a TODO to split reconciler.go
by responsibility as a follow-up.

Comment link: #992 (comment)
Comment link: #992 (comment)

BUG=b/548435582
TAG=agy
CONV=baca6192-74d3-4579-9f73-0a3263d9a2a7
@VLukiantsev
VLukiantsev force-pushed the fix/bridge-deactivation-gating branch from 1549fc9 to e5d766c Compare September 28, 2026 12:32
github-actions[bot]
github-actions Bot approved these changes Sep 28, 2026
View reviewed changes
@github-actions github-actions Bot left a comment
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Incremental PR Approval

PR author is a code owner and changes since last approval (6 lines) are within the 10-line grace delta.

Preserving prior approval by @janzak-dev on commit 637cd26.

@VLukiantsev
VLukiantsev added this pull request to the merge queue Sep 28, 2026
Merged via the queue into main with commit b762960 Sep 28, 2026
19 of 21 checks passed
This was referenced Sep 28, 2026
Merged
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@github-actions github-actions[bot] github-actions[bot] approved these changes
@janzak-dev janzak-dev janzak-dev left review comments
+1 more reviewer
@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@VLukiantsev @janzak-dev
0