Siyao Meng created HDDS-17524:
---------------------------------
Summary: Container Balancer stop can be overwritten by the
balancing task's progress save, so the stopped balancer resumes after SCM
failover or restart
Key: HDDS-17524
URL: https://issues.apache.org/jira/browse/HDDS-17524
Project: Apache Ozone
Issue Type: Bug
Reporter: Siyao Meng
Attachments: MC-1-stop-balancing-task-before-persisting-stop.patch,
TestBugMC1StopUndoneByProgressSave.java
h3. Mechanism
{{ContainerBalancer.stopBalancer}} persists {{shouldRun=false}} first and only
then marks the running task as stopping ({{recordStopReason}},
{{ContainerBalancerTask.stop}}). The balancing task persists its progress after
every completed iteration as {{shouldRun=true}} with the next iteration index
({{ContainerBalancerTask.saveConfiguration}}). That save is guarded only by the
task's own status check ({{taskStatus == RUNNING}} under the task monitor) and
does not take {{ContainerBalancer.lock}}. So while the stop's save is in flight
through Ratis, the task can still see RUNNING and submit its own save, which
Ratis applies after the stop's. {{stopBalancer}} then marks the task stopping,
waits for the thread and returns success. The task exits without another save,
and the persisted state says {{shouldRun=true}}.
h3. Trigger
# The balancer is running.
# The operator runs {{ozone admin containerbalancer stop}} while an iteration
is ending.
# The task's progress save is submitted while the stop's save of
{{shouldRun=false}} has not returned yet.
# On the next leader change, safe mode exit or SCM restart,
{{ContainerBalancer.notifyStatusChanged}} reads {{shouldRun=true}} and starts
the balancer again from the saved iteration index.
The window is about one Ratis round trip of the stop's save, and there is at
most one progress save every
{{hdds.container.balancer.balancing.iteration.interval}} (default 3 minutes)
plus the iteration time, so at defaults it is rare per stop; a short interval
or a slow Ratis commit makes it more likely (both argued from the code and
defaults, not measured).
h3. Impact
A stop that the CLI reported as successful does not last: the balancer the
operator stopped starts moving containers again after a failover or restart,
and {{status}} shows it running with no indication why. It continues until it
finishes or cannot balance any more, or until it is stopped again. No data is
lost. Start and stop require SCM admin access.
h3. Reproduction
PASS on unmodified source, deterministic.
[^TestBugMC1StopUndoneByProgressSave.java] ({{hadoop-hdds/server-scm}}) runs
the real {{ContainerBalancer}} and {{ContainerBalancerTask}} on the
{{MockedSCM}} / {{MockCluster}} harness of the balancer unit tests, with the
default balancing interval. Only the persisted state store is an in memory map.
The single timing aid is in that store: it holds the task's first progress save
until the stop's save has been applied, which is the order Ratis gives two
concurrent submissions. The test asserts that {{stopBalancer}} returns without
error, the task is STOPPED with reason USER_REQUESTED, the persisted state is
{{shouldRun=true}} with next iteration index 1, a failover
({{notifyStatusChanged}} on step down, then on leader ready) starts a new
balancing thread, and a new {{ContainerBalancer}} over the same store (restart)
starts too. A passing test means the defect is present. Not run on a mini
cluster or an HA ring.
h3. Suggested fix
Mark the task stopping before saving {{shouldRun=false}}.
{{ContainerBalancerTask.stop}} takes the task monitor that the progress save
holds, so a save already in flight completes first and no later one passes the
RUNNING check, which makes the stop's save the last write. Making the task's
save take {{ContainerBalancer.lock}} instead would invert the lock order
against the task's own stop path ({{tryStopWithSaveConfiguration}} saves under
the task monitor) and risks a deadlock like HDDS-6928.
h3. Patch
[^MC-1-stop-balancing-task-before-persisting-stop.patch], against
9118d5a85a37db8c64404d56703b7bacb040e063. It also applies to master at
f59f8d3d123.
{{stopBalancer}} now records the stop reason and calls {{task.stop()}} before
{{saveConfiguration(config, false, 0)}}, still under
{{ContainerBalancer.lock}}, and the wait for the thread ({{blockTillTaskStop}})
moves into the {{finally}} after the unlock. The save stays unconditional, so
stopping a balancer that is not running still clears {{shouldRun}}
(HDDS-13694), and the join stays outside the lock. One behaviour changes: if
the save fails, the stop still throws, but the task is now stopped on this SCM
instead of continuing to run; the persisted state is whatever the failed save
left (normally still {{shouldRun=true}}, so a later failover or restart resumes
the balancer unless the stop is repeated). Repeating the stop clears it, since
{{stopBalancer}} saves even when no task is running (HDDS-13694). If a save
that timed out did commit, the stopped task and the persisted state now agree,
where today the task keeps running with {{shouldRun=false}} persisted until its
next progress save writes {{shouldRun=true}} again.
Covered by two tests in the existing {{TestContainerBalancer}}.
{{testStopBalancerNotUndoneByProgressSave}} holds the task's progress save in
the same way and asserts {{shouldRun()}} is false after the stop; without the
change it fails with "Expecting value to be false but was true".
{{testStopBalancerStopsTaskWhenSaveFails}} makes the stop's save throw and
asserts the error reaches the caller and the task is STOPPED.
Found by TLA+ model checking and code review of the SCM container balancer
start, stop and failover resume paths under HDDS-15926, on commit
9118d5a85a37db8c64404d56703b7bacb040e063. Checked against HDDS issues and
apache/ozone pull requests for duplicates before filing. The attached patch is
a proposal for review. Generated with Specula (Claude Opus 5.5).
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
