Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a restart/migration deadlock in the StorageNodeSet action reconcile path by ensuring spdk-proxy EndpointSlice/DNS publication continues during actions and by making backend node actions idempotent across requeues.
Changes:
- Reconcile spdk-proxy
EndpointSliceobjects even whenspec.actionis set (action early-return path). - Add an idempotency guard for long-running backend node actions using a pre-read of backend node status plus persisted
ActionStatusfields (Triggered,ObservedGeneration). - Add regression unit tests covering both the EndpointSlice publication during actions and idempotent action triggering.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
internal/controller/simplyblockstoragenodeset_controller.go |
Runs spdk-proxy EndpointSlice reconciliation during actions; adds idempotency guard + helper funcs; wires ActionStatus.Triggered/ObservedGeneration. |
internal/controller/simplyblockstoragenodeset_controller_unit_test.go |
Adds regression tests for EndpointSlice publication during action flow and for idempotent action triggering. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
noctarius
changed the title
During a restart/migration (spec.action=restart with a target workerNode)
two operator bugs combined to leave the node stuck in in_restart.
Defect A — spdk-proxy DNS never published during actions:
Reconcile early-returns into reconcileAction when spec.action is set, which
skipped reconcileSpdkProxyEndpointSlices for the whole action. The target
worker's spdk-proxy pod came up but its headless-service DNS entry was never
written, so the control plane's iobuf_set_options RPC failed with
NameResolutionError until it gave up. The pod-driven slice reconciliation now
also runs on the action path, so the entry is published as soon as the new
spdk-proxy pod becomes Ready.
Defect B — restart retry storm resets node state:
performNodeAction re-POSTed the restart on every reconcile, and handleNodeAction
only short-circuited on a prior success. While a migration was in flight the
node reads in_restart (longer than the ~4min completion budget), so each requeue
re-fired /restart, reset online->in_restart, and spawned a duplicate task the
runner aborted ("node is restarting, stopping task"). Add an idempotency guard
that reads the backend node status first and skips the trigger when the action
is already in progress (polls instead) or already complete, wiring up the
previously-unused ActionStatus.Triggered/ObservedGeneration fields. Mirrors the
existing pattern in nodedrain_controller.
Repeated reconciles during a long migration are now benign polling: no state
resets, no duplicate tasks. The added status GET is one cheap read per action
reconcile and replaces the repeated restart POSTs, so overall API traffic drops.
Adds regression tests for both fixes.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
During a restart/migration (spec.action=restart with a target workerNode) two operator bugs combined to leave the node stuck in in_restart.
Defect A — spdk-proxy DNS never published during actions: Reconcile early-returns into reconcileAction when spec.action is set, which skipped reconcileSpdkProxyEndpointSlices for the whole action. The target worker's spdk-proxy pod came up but its headless-service DNS entry was never written, so the control plane's iobuf_set_options RPC failed with NameResolutionError until it gave up. The pod-driven slice reconciliation now also runs on the action path, so the entry is published as soon as the new spdk-proxy pod becomes Ready.
Defect B — restart retry storm resets node state:
performNodeAction re-POSTed the restart on every reconcile, and handleNodeAction only short-circuited on a prior success. While a migration was in flight the node reads in_restart (longer than the ~4min completion budget), so each requeue re-fired /restart, reset online->in_restart, and spawned a duplicate task the runner aborted ("node is restarting, stopping task"). Add an idempotency guard that reads the backend node status first and skips the trigger when the action is already in progress (polls instead) or already complete, wiring up the previously-unused ActionStatus.Triggered/ObservedGeneration fields. Mirrors the existing pattern in nodedrain_controller.
Repeated reconciles during a long migration are now benign polling: no state resets, no duplicate tasks. The added status GET is one cheap read per action reconcile and replaces the repeated restart POSTs, so overall API traffic drops.
Adds regression tests for both fixes.