[
https://issues.apache.org/jira/browse/HDDS-17524?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Siyao Meng updated HDDS-17524:
------------------------------
Affects Version/s: 2.2.1
2.0.0
1.4.0
2.1.2
2.1.0
1.4.1
2.1.1
1.3.0
2.2.0
> 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
> Affects Versions: 1.3.0, 1.4.0, 2.0.0, 1.4.1, 2.1.0, 2.2.0, 2.1.1, 2.1.2,
> 2.2.1
> Reporter: Siyao Meng
> Priority: Minor
> 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]
