fix(operator): bound lazy storage-gate dial by step timeout#8
Merged
Conversation
Addresses review findings on #7's lazy storage-gate change (#7 (comment)): - lazyStorageGate's closure now wraps ctx in context.WithTimeout(timeout) before calling BuildStorageGate, so the unbounded TrueNAS dial+auth (truenas.New has no internal deadline) shares the step's timeout budget with WaitDetached instead of only bounding the wait. Guarded on timeout > 0 so a zero/unset StorageTimeout degrades to "unbounded, ctx-only" rather than an instantly-expired context, matching how other steps (e.g. waitPowerOff) treat <=0 as "no override". - Reworded three comments left over from the eager storage-gate lifecycle (Config's env-at-build-time doc, Backends.Close's TrueNAS-websocket mention, and the reconciler's Close comment) to describe the current lazy dial/close-inside-the-gate-call behavior. - Added operate.TrueNASConfigured(), deduplicating the three copies of the TrueNAS env-presence check (lazyStorageGate, BuildStorageGate, and the CLI's drain-shutdown plan display). lazyStorageGate and BuildStorageGate still return a nil gate when unconfigured; only the presence check moved. The drain-path eager-identity-validation question raised in the same review is intentionally not addressed here, pending a design decision.
Review follow-ups on #8: - lazyStorageGate now defaults timeout<=0 to 5 minutes (mirroring waitPowerOff) and always bounds ctx with context.WithTimeout, instead of guarding on timeout>0. The previous comment mis-cited waitPowerOff as treating <=0 as "no override" (it defaults to 5m), and leaving ctx unbounded for <=0 was incoherent anyway: iscsi.Gate.WaitClear applies WithTimeout unconditionally, so timeout=0 (reachable via the unvalidated --storage-timeout flag) meant "unbounded dial, instantly-expired wait". Now dial + wait share one coherent budget. - drop the dead buildStorageGate CLI wrapper from cmd/nightwatch/drainshutdown.go (nothing calls it since the run path moved to operate.RealBuilder) and its unused lifecycle import; its no-creds test moves to the operate package, next to BuildStorageGate.
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
1 participant
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.
Summary
Addresses the review on #7: #7 (comment)
lazyStorageGate's closure ininternal/operate/operate.gonow defaults a<=0timeout to 5 minutes (mirroringwaitPowerOffininternal/lifecycle/drainshutdown.go) and always wrapsctxincontext.WithTimeout(ctx, timeout)before callingBuildStorageGate, so the unbounded dial+auth (truenas.Newhas no internal deadline) shares one coherent budget withWaitDetachedinstead of only the wait being bounded. This also fixes thetimeout=0case (reachable via the unvalidated--storage-timeoutflag), which previously meant "unbounded dial, instantly-expired wait" becauseiscsi.Gate.WaitClearappliesWithTimeoutunconditionally.Config's doc comment,Backends.Close's doc comment (both ininternal/operate/operate.go), and the reconciler'sdefer be.Close()comment ininternal/controller/elasticnode_controller.go— all now describe the lazy dial / close-inside-the-gate-call behavior instead of the old eager-TrueNAS-websocket one.operate.TrueNASConfigured() booland used it inlazyStorageGate,BuildStorageGate, andcmd/nightwatch/drainshutdown.go's plan display, replacing three independent copies ofhost == "" || user == "" || key == "".lazyStorageGateandBuildStorageGatestill return a nil gate/interface when unconfigured — only the presence check itself moved behind the helper.Follow-up (second commit, from review on this PR): also removed the dead
buildStorageGatewrapper fromcmd/nightwatch/drainshutdown.go(nothing called it since the run path moved tooperate.RealBuilder); its no-creds test moved to the operate package next toBuildStorageGate.Not addressed here: the same review raised a question about eager validation of
iscsi_initiator_addron the drain path (nodes missing that field currently drain fully before the storage-gate step surfaces the config error). That's a behavior-change decision, not a bug fix, and is intentionally left for a separate PR.Tests
gofmt -l .— cleango build ./...— cleango vet ./...— cleango test ./...— all packages pass