Siyao Meng created HDDS-16895:
---------------------------------
Summary: SCM can leave safe mode below the container threshold
when a container report is processed during the container rule refresh
Key: HDDS-16895
URL: https://issues.apache.org/jira/browse/HDDS-16895
Project: Apache Ozone
Issue Type: Bug
Reporter: Siyao Meng
Attachments: MC-1-count-only-containers-removed-by-deleted-scan.patch,
TestBugMC1RefreshSubtractsReportedContainers.java
h3. Mechanism
{{AbstractContainerSafeModeRule.reinitializeRule}} removes DELETED containers
from the rule's container map and then lowers {{totalContainers}} by the
difference between the map size before and after the scan. The refresh holds
the rule's lock, but report processing does not: {{SafeModeExitRule.onMessage}}
calls {{process()}} between two {{validate()}} calls, and
{{handleReportedContainer}} removes each reported container from the same map
and increments {{containersWithMinReplicas}}. Every container that a report
removes while the refresh is between its two size reads is therefore counted as
reported and also subtracted from the total. The ratio can exceed 1.0, the rule
validates, and once the other rules pass SCM leaves safe mode. The exit is not
reversed.
{{ECContainerSafeModeRule.handleReportedContainer}} has a second, smaller gap
of the same kind: it increments the count without checking that its own
{{remove()}} removed the container, so an EC container removed by the refresh
between {{containsKey}} and {{remove}} is still counted (code reading, not run).
h3. Trigger
# SCM starts in safe mode. The {{safemode-refresh-thread}} calls
{{refresh(false)}} on every rule every
{{hdds.scm.safemode.rule.refresh.interval}} (default 5s) while the rule is not
met.
# A refresh starts while a datanode's registration container report is being
processed.
No fault and no operator action is needed. {{refresh()}} and {{validate()}} are
synchronized on the rule, and {{onMessage}} calls {{validate()}} before
{{process()}}, so a report that arrives during a refresh waits for it. Only the
one report already inside {{process()}} when the refresh starts can overlap,
and the chance of a hit per registration report is roughly that report's
processing time divided by the refresh interval. The window itself is the
DELETED lookup (a copy of the DELETED containers under the container state read
lock) plus the removal loop: about 50ms per 100,000 DELETED records in a unit
timing with a mocked {{ContainerManager}}. It decides how much of the report in
progress fits inside the window.
h3. Impact
* SCM leaves safe mode with fewer containers reported than
{{hdds.scm.safemode.threshold.pct}} (default 0.99). Each refresh can subtract
at most the containers that the one report in progress when it starts removes
during the refresh, because the next report waits for the rule lock in
{{validate()}}. Successive refreshes add up. At the default 5s interval a hit
is rare, and the expected shortfall is estimated at a few percent of the
containers below the threshold (estimated from the code, not measured). A
single hit on a large early report in a small cluster can cost more, up to that
report's share of the containers. The other rules, mainly the 90% one replica
pipeline rule, still need most datanodes registered before SCM can exit.
* On exit, block allocation and {{allocateContainer}} stop returning
{{SAFE_MODE_EXCEPTION}}, and {{getContainerWithPipeline}} stops refusing open
containers without enough replicas. ReplicationManager, block deletion and the
pipeline scrubber start after {{hdds.scm.wait.time.after.safemode.exit}}
(default 5m). The background pipeline creator already runs in safe mode once
the pre check passes ({{hdds.scm.safemode.pipeline.creation}}, default true),
so the exit does not change it. Containers whose replicas have still not been
reported by then are handled as missing or under replicated (argued from the
code, not run). No data loss is expected.
* The subtraction was added by HDDS-15238 and is in 2.2.0 and 2.2.1. There the
refresh runs from {{SCMStateMachine.applyTransaction}} while in safe mode
instead of from the periodic thread, so the same race should be reachable
(argued, not run on 2.2.x).
h3. Reproduction
[^TestBugMC1RefreshSubtractsReportedContainers.java]
({{hadoop-hdds/server-scm}}) uses the real {{SCMSafeModeManager}}, its refresh
thread (interval lowered to 100ms) and the EventQueue handler of
{{RatisContainerSafeModeRule}}, with a mocked {{ContainerManager}} holding 3
closed containers. One datanode reports 2 of them. Two latches only order the
threads: the handler waits just before {{process()}} until the refresh is
inside the DELETED lookup, and the refresh waits there until the report is
processed. On the unmodified commit it fails every time with {{SCM left safe
mode with 2 of 3 containers reported}}, and the log shows {{Refreshed RATIS
Containers threshold count to 1.}} followed by {{SCM exiting safe mode.}} With
the patch it passes.
Without any ordering, a stress run (20,000 closed containers, 200,000 DELETED
containers in the mock, one report of 19,700 containers, i.e. 98.5%, fired at a
random offset to a 100ms refresh) left safe mode in 6 of 20 trials on the
unmodified commit and in 0 of 20 with the patch. In the failing trials the
total fell to as little as 5,132 of 20,000. That size comes from the stress
settings: a 100ms interval and a DELETED lookup of about 100ms (200,000
records) against one report of 19,700 containers, so a large part of that
single report fell inside refresh windows. It is not a typical shortfall. The
stress harness is not attached. Not run at the default 5s interval or on a real
cluster.
h3. Patch
[^MC-1-count-only-containers-removed-by-deleted-scan.patch], against
b0aa6475b7856d2c7f467dd3db12b9a1dd116f63. It also applies to master at
6818707698c.
{{reinitializeRule}} now subtracts only the containers that its own
{{containers.remove()}} removed (non null return) instead of the change in map
size. Each container is then removed exactly once, either by a report (counted,
kept in the total) or by the DELETED scan (subtracted, not counted).
{{ECContainerSafeModeRule.handleReportedContainer}} increments only when its
{{remove()}} returned a value, as the Ratis rule already does. Report
processing stays outside the rule lock. Making {{process()}} synchronized would
also close the race, but it would hold the lock for a whole registration report
and block {{validate()}}, the safe mode status call and the refresh meanwhile.
Covered by {{testRefreshKeepsContainersReportedDuringRefresh}} in the existing
{{AbstractContainerSafeModeRuleTest}}, which runs for the Ratis and the EC
rule. A report is processed inside the mocked DELETED lookup, and the test
expects only the DELETED container to leave the total. Without the change it
fails for both rules ({{expected: <2> but was: <1>}}). With it, the
{{org.apache.hadoop.hdds.scm.safemode}} unit tests (76) and
{{TestSCMSafeModeWithPipelineRules}} on a mini cluster (2) pass, and checkstyle
is clean.
Found by TLA+ model checking and code review of the SCM safe mode rules under
HDDS-15926, on commit b0aa6475b7856d2c7f467dd3db12b9a1dd116f63. 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
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
