manager: stop the jobs orchestrator when a manager is demoted - #3295
richarddavenport wants to merge 4 commits into
Conversation
corhere
left a comment
corhere
left a comment
There was a problem hiding this comment.
TestBecomeFollowerStopsAhatBecomeLeaderStarts is effectively a specialized linter, which would be very sensitive to innocent source changes. I would much prefer to see the manager code refactored so all services started when becoming leader are stopped when becoming follower, by construction. My suggestion: roll your own defer-alike so teardown code can all live in becomeLeader immediately following the code which starts each service.
func (m *Manager) becomeLeader() {
// ...
go func(volumeManager *csi.Manager) {
volumeManager.Run(ctx)
}(m.volumeManager)
m.onBecomeFollower(func() {
m.volumeManager.Stop()
m.volumeManager = nil
})
}
func (m *Manager) becomeFollower() {
cleanups := m.becomeFollowerCleanups
m.becomeFollowerCleanups = nil
for _, f := range cleanups {
f()
}
}
|
And please do not reference any issues or PRs in any moby repo from any commit message as GitHub's UI gets very noisy with back-links from forks. Linking from the PR description is fine, though. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3295 +/- ##
=======================================
Coverage 14.76% 14.76%
=======================================
Files 200 198 -2
Lines 93027 93070 +43
=======================================
+ Hits 13736 13743 +7
- Misses 77961 77972 +11
- Partials 1330 1355 +25 🚀 New features to boost your workflow:
|
7312d09 to
38943b8
Compare
|
Done as sketched. Commit messages no longer reference moby issues. |
There was a problem hiding this comment.
🟡 Changes recommended
Core demotion and continued-reconciliation behaviors are not directly covered by regression tests.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Stops leader-only job orchestration after demotion and prevents replicated-job task-count underflow.
Changes:
- Registers and runs leader-component cleanup callbacks.
- Adds saturating task-count subtraction and overshoot logging.
- Updates tests for cleanup ordering and over-completed jobs.
File summaries
| File | Description |
|---|---|
manager/manager.go |
Adds demotion cleanup registry, including jobs orchestrator teardown. |
manager/manager_test.go |
Tests cleanup execution order and idempotency. |
manager/orchestrator/jobs/replicated/reconciler.go |
Prevents unsigned subtraction underflow. |
manager/orchestrator/jobs/replicated/reconciler_test.go |
Updates overshoot expectations. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| func TestBecomeFollowerRunsRegisteredCleanups(t *testing.T) { | ||
| m := &Manager{} | ||
| var ran []string | ||
| m.onBecomeFollower(func() { ran = append(ran, "first") }) | ||
| m.onBecomeFollower(func() { ran = append(ran, "second") }) |
There was a problem hiding this comment.
Not adding one. A real demotion test needs a live raft cluster: becomeFollower stops thirteen components that block on a doneChan only closed by Run, and roleManager.Run dereferences its raft node immediately. No manager test drives a leadership transition today.
An earlier revision of this PR asserted the invariant by parsing the source instead. That was rejected in review in favour of pairing each start with its stop in one place, which is what the first commit does.
| reconcileErr := r.ReconcileService("someService") | ||
| Expect(reconcileErr).To(HaveOccurred()) | ||
| Expect(reconcileErr.Error()).To(ContainSubstring("underflow")) | ||
| Expect(reconcileErr).ToNot(HaveOccurred()) |
There was a problem hiding this comment.
Done. The spec now leaves a task from a previous job iteration in the store and asserts it is marked for removal, which happens after the point that used to return early.
corhere
left a comment
corhere
left a comment
There was a problem hiding this comment.
Looks great; thank you! There are just a couple items that need to be addressed before I can approve.
And I have a request: please split the first commit into two.
- refactor to use
onBecomeFollower - add the missing
Stop()call
| // tasks, both still need to happen. log it and carry on creating zero new | ||
| // tasks. | ||
| if completeTasks+runningTasks > rj.TotalCompletions { | ||
| log.L.WithFields(log.Fields{ |
There was a problem hiding this comment.
Please plumb a context.Context into this function so that the log can be emitted with all the relevant context attached.
| log.L.WithFields(log.Fields{ | |
| log.G(ctx).WithFields(log.Fields{ |
There was a problem hiding this comment.
Done, as commit 3. ReconcileService takes a context on both reconcilers and on the interface. That also cleared the two TODO(dperny) comments asking for a real context at the restart.Restart() calls.
| "totalCompletions": rj.TotalCompletions, | ||
| }).Warn("replicated job has more tasks than TotalCompletions; creating no new tasks") | ||
| } | ||
|
|
There was a problem hiding this comment.
Dropping the early return makes the restartTasks loop reachable when runningTasks > MaxConcurrent, so a job that has already met TotalCompletions would restart surplus failed tasks.
| if completeTasks >= rj.TotalCompletions { | |
| // The job has already reached its goal. Restarting any failed tasks | |
| // now would risk overshooting the desired number of completions. | |
| restartTasks = nil | |
| } | |
There was a problem hiding this comment.
Applied. Added a spec that fails without it: a job at TotalCompletions with a surplus failed task must hand nothing to the restart supervisor.
becomeLeader() starts the components that only run on the leader, and becomeFollower() stops them again. The two lists sit two hundred lines apart and are kept in step by hand, so a component can be added to one and forgotten in the other. Register each component's teardown with onBecomeFollower() immediately after the code that starts it, and have becomeFollower() run and clear the registered teardowns. Both functions are already called under m.mu, so the slice needs no lock of its own. No functional change: the same thirteen components are stopped, and the same ones nilled out. They are now stopped in the order they were started, where becomeFollower() previously kept its own hand-maintained order. Each component watches the store independently and none calls into another, so the order has no effect. Manager.Stop() is left alone. It stops each component directly under a nil guard, so it works whether or not becomeFollower() has run. Signed-off-by: Richard Davenport <richard.davenport@mbpnetwork.com>
becomeLeader() starts the jobs orchestrator alongside the other leader-only components, but becomeFollower() never stopped it. Every other leader-only component -- the replicated and global orchestrators, the task reaper, the scheduler, the constraint and volume enforcers, the role and key managers -- is stopped and nilled out there. The jobs orchestrator was the only one that survived demotion. A manager that has held leadership and then lost it therefore kept a live jobs orchestrator for the lifetime of the daemon. Only Manager.Stop() shut it down, which in practice means a dockerd restart. Cancelling the context does not help: in this package the context carries logging information only, and Stop() is the sole shutdown path. In a cluster where several managers have held leadership at some point, this leaves multiple jobs orchestrators reconciling the same service concurrently. Each one creates its own task for the same job iteration, so a replicated job declared with MaxConcurrent: 1 and TotalCompletions: 1 starts one task per live orchestrator, in the same slot, milliseconds apart. Observed on a five-manager swarm running 27.3.1, where two demoted managers produced two tasks for iteration 0 slot 0, both of which ran the job's payload concurrently. Orchestrator.Stop() closes stopChan inside a sync.Once, so it is safe against repeated calls and cannot hit the double-close problem fixed for the task reaper in 2491. Signed-off-by: Richard Davenport <richard.davenport@mbpnetwork.com>
Neither reconciler had a context to log or trace with, and both called restart.Restart() with context.Background() under a TODO asking for the real one. Take a context.Context in ReconcileService and pass it down. Every caller already has one to hand. This clears both TODOs, and lets the next commit log with the reconciliation's own context attached. No functional change. Signed-off-by: Richard Davenport <richard.davenport@mbpnetwork.com>
ReconcileService computes how many new tasks to create with unsigned
subtraction:
possibleNewTasks := rj.MaxConcurrent - runningTasks
allowedNewTasks := rj.TotalCompletions - completeTasks - runningTasks
If more tasks exist for the current job iteration than the service asked for,
these underflow, and the guard below them returns an error rather than creating
a preposterous number of tasks. Refusing to create the tasks is right; returning
an error is not, because it abandons the rest of the reconciliation. The removal
of tasks belonging to previous job iterations and the restarting of failed tasks
both happen after that point, so a service that has overshot TotalCompletions
once is never reconciled again -- the count cannot come back down, so every
subsequent pass takes the same early return.
Make both subtractions saturating and downgrade the guard to a warning. An
overshot job now creates zero new tasks, which is what the arithmetic was trying
to express, while the rest of the reconcile continues to run.
Dropping the early return leaves the restart loop reachable for a job that has
already met TotalCompletions, where it would restart surplus failed tasks and
overshoot further. Clear the restart list in that case.
This does not prevent the overshoot; it stops the overshoot from being
permanent.
The existing spec asserted the error. It now asserts the new behaviour, and that
the reconcile really does continue: a task belonging to a previous job iteration
is still marked for removal. A second spec covers the restart list being cleared
once the job has met its goal.
Signed-off-by: Richard Davenport <richard.davenport@mbpnetwork.com>
38943b8 to
4b1b562
Compare
|
Split as asked, plus one more. Four commits:
Each builds and passes its package tests on its own. |
A
replicated-jobcan run its payload once per ex-leader, then repeat. Two bugs combine to cause it.becomeFollower()doesn't stop the jobs orchestrator, so a manager that has held leadership keepsone running for the life of the daemon, and several such managers each create a task for the same job
iteration (moby/moby#53632). Separately,
ReconcileServicereturns an error when its unsignedtask-count arithmetic underflows, which abandons the rest of the reconcile, so a job that has
overshot
TotalCompletionsis never reconciled again (moby/moby#42742). The first causes theovershoot; the second makes it permanent.
The first commit is a refactor with no behaviour change. Each leader-only component's teardown is
registered with
onBecomeFollower()next to the code that starts it, andbecomeFollower()runs andclears them. It registers exactly the thirteen components
becomeFollower()already stopped, now instart order, which nothing depends on.
Manager.Stop()is untouched. The second commit adds themissing registration for the jobs orchestrator, and is the actual fix.
The third commit plumbs a
context.ContextintoReconcileService, which the fourth needs for itslog line and which clears two
TODO(dperny)comments. The fourth makes the task-count subtractionssaturating and downgrades the error return to a warning, so an overshot job creates no new tasks and
the reconcile continues. Dropping that early return leaves the restart loop reachable for a job that
has already met
TotalCompletions, so the restart list is cleared in that case.golangci-lint run ./manager/...,make dep-validateandgofmt -sare clean, and each commitbuilds and tests on its own.
go test ./...passes exceptTestManagerRespectsDispatcherRootCAUpdatein./node, which fails the same way on clean master.Two new specs cover the reconciler change and were verified to fail without it: an overshot job still
marks a previous iteration's task for removal, and a job at
TotalCompletionshands no surplusfailed task to the restart supervisor.
Observed on a five-manager swarm running 27.3.1 with
MaxConcurrent: 1, TotalCompletions: 1. Twodemoted managers logged
error reconciling replicated job … node lost leader statusfor hours, andone deploy produced two tasks for
JobIteration 0, Slot 023ms apart. To reproduce, move leadershipoff a manager without restarting its daemon, then run a replicated job, and compare
JobIteration.Index,SlotandCreatedAtwithdocker inspect.- Description for the changelog
Stop the jobs orchestrator when a manager loses leadership, and keep an over-completed replicated job
reconcilable instead of erroring out permanently.