Summary
Follow-ups from reviewing #1977, which records a process kill's planned targets
before delegating the kill so that final cleanup can restart a node stopped by a
partially-successful multi-node kill. Neither blocks that change.
The two kind guards can be one unconditional reset!
;; jepsen/src/jepsen/openraft/nemesis/process.clj, start-disruption!
;; A multi-node kill may partially succeed before the delegate
;; reports an error, so retain every node that may need restarting.
(when (= :process kind)
(reset! active disruption))
(nemesis/invoke! delegate test
(assoc op
:f delegate-f
:value targets))
(when (= :pause kind)
(reset! active disruption))The two guards are mutually exclusive and kind is one of exactly :process
and :pause, so together they are the original single reset! with its
position made kind-dependent. start-disruption! does already branch on kind
twice — narrowing eligible-nodes to reachable nodes for a pause, and choosing
between throwing and skipping when there is no quorum-safe target — but each of
those encodes a difference in what the two disruptions do. This pair encodes
no difference in behavior, only in bookkeeping order, and the pause half is the
order the change argues against.
Moving the reset! unconditionally ahead of the delegate call changes three
things on the pause path, and none of them is a regression:
resume-process takes its node list from (:nodes test) and never from
@paused, so recovery coverage after a partially-successful pause is
unchanged.- A
resume-process following a failed pause-process records
{:paused <disruption>, :resumed ...} instead of {:paused nil, :resumed ...}. next-pause-state reads only :resumed and the error flag, so the
checker verdict is unchanged and the history entry gains the target list —
the same accuracy argument the process path is being changed for.
- The
(when @active (throw ...)) guard at the top of start-disruption! would
now fire on a pause-process that follows a failed pause-process with no
intervening resume. pause-generator alternates pause and resume so this does
not arise, and were it to arise, throwing is correct: some nodes may in fact
be paused.
So:
;; A multi-node disruption may partially succeed before the delegate
;; reports an error, so retain every node that may need recovering.
(reset! active disruption)
(nemesis/invoke! delegate test
(assoc op
:f delegate-f
:value targets))lein test passes unchanged with this form — 83 tests, 339 assertions.
restarts-all-planned-processes-after-a-kill-error asserts only the delegate log
;; jepsen/test/jepsen/openraft/nemesis/process_test.clj
(nemesis/invoke! subject test {:type :info :f :restart-process})
(is (= [[:kill ["n2" "n3"]]
[:start ["n2" "n3"]]]
(mapv (juxt :f :value) @invocations)))The restart-process return value is discarded. Its sibling
restarts-the-processes-that-were-killed, three deftests up, does assert it:
(is (= ["n1"] (get-in restarted [:value :nodes])))
Retaining the targets has two visible effects, and the test covers one. The
delegate log covers that the nodes are started. What it does not cover is that
the completion carries the disruption instead of :no-processes-killed — which
is the half that reaches history.edn, and the symptom #1976 opens with. One
(is (= ["n2" "n3"] (get-in restarted [:value :nodes]))) covers it.
Non-goals
Not proposed here: any change to how the delegate's exception propagates or how
it is classified, which #1975 and #1976 settle; restarting killed processes from
ProcessNemesis/teardown! to mirror PauseNemesis/teardown!, which would be
pointless because db/teardown! kills and wipes every node immediately after;
or folding the remaining kind branches in start-disruption! into
disruption-start-specs, since those two encode real behavioral differences and
read fine where they are.
Summary
Follow-ups from reviewing #1977, which records a process kill's planned targets
before delegating the kill so that final cleanup can restart a node stopped by a
partially-successful multi-node kill. Neither blocks that change.
The two
kindguards can be one unconditionalreset!The two guards are mutually exclusive and
kindis one of exactly:processand
:pause, so together they are the original singlereset!with itsposition made kind-dependent.
start-disruption!does already branch onkindtwice — narrowing
eligible-nodesto reachable nodes for a pause, and choosingbetween throwing and skipping when there is no quorum-safe target — but each of
those encodes a difference in what the two disruptions do. This pair encodes
no difference in behavior, only in bookkeeping order, and the pause half is the
order the change argues against.
Moving the
reset!unconditionally ahead of the delegate call changes threethings on the pause path, and none of them is a regression:
resume-processtakes its node list from(:nodes test)and never from@paused, so recovery coverage after a partially-successful pause isunchanged.
resume-processfollowing a failedpause-processrecords{:paused <disruption>, :resumed ...}instead of{:paused nil, :resumed ...}.next-pause-statereads only:resumedand the error flag, so thechecker verdict is unchanged and the history entry gains the target list —
the same accuracy argument the process path is being changed for.
(when @active (throw ...))guard at the top ofstart-disruption!wouldnow fire on a
pause-processthat follows a failedpause-processwith nointervening resume.
pause-generatoralternates pause and resume so this doesnot arise, and were it to arise, throwing is correct: some nodes may in fact
be paused.
So:
lein testpasses unchanged with this form — 83 tests, 339 assertions.restarts-all-planned-processes-after-a-kill-errorasserts only the delegate logThe
restart-processreturn value is discarded. Its siblingrestarts-the-processes-that-were-killed, three deftests up, does assert it:Retaining the targets has two visible effects, and the test covers one. The
delegate log covers that the nodes are started. What it does not cover is that
the completion carries the disruption instead of
:no-processes-killed— whichis the half that reaches
history.edn, and the symptom #1976 opens with. One(is (= ["n2" "n3"] (get-in restarted [:value :nodes])))covers it.Non-goals
Not proposed here: any change to how the delegate's exception propagates or how
it is classified, which #1975 and #1976 settle; restarting killed processes from
ProcessNemesis/teardown!to mirrorPauseNemesis/teardown!, which would bepointless because
db/teardown!kills and wipes every node immediately after;or folding the remaining
kindbranches instart-disruption!intodisruption-start-specs, since those two encode real behavioral differences andread fine where they are.